Recognize hash as optional arg when optional keyword is present #366

edzhelyov · 2019-02-25T19:58:46Z

I hit an issue in one of the projects that I work that I've reported in rspec-mocks#1266.

In short, given foo(a, b = {}, x: false) should recognize a valid method call with args: foo(1, a: 1, b: 2).

I've opted to extend the spec for optional keywords. I'm feeling lost in the specs, so please, give me some pointers if you need the specs re-worked.

I'm OK if you want to change the implementation or try to catch edge cases involving splats.

JonRowe · 2019-02-26T16:03:34Z

Great start at fixing rspec-mocks#1266 I started trying to write a spec for it but got distracted.

What I would like to see here though is this being seperate cases demonstrating it in addition to our existing examples, as I want to ensure we don't regress other paths

edzhelyov · 2019-02-27T08:19:26Z

Thanks, I will rework the specs.

edzhelyov

@JonRowe I've updated the PR. I've used the specs from a method with optional keyword arguments as a base and worked them out to handle the case with an optional argument.

During testing, I found that Ruby will treat each symbol key as a keyword argument if the method signature contains an optional keyword args. It will treat non-symbol arguments as a separate Hash. These are the reasons that I've worked the code in could_contain_kw_args? and split_args. See below some code samples how Ruby treats the hash arguments:

def foo(x, y = {}, z: 1) puts "#{y}, #{z}"; end

foo 1, 'a' => 2, 'b' => 2
{"a"=>2, "b"=>2}, 1

foo 1, 'a' => 2, :z => 3
{"a"=>2}, 3

foo 1, 'a' => 2, :b => 3
ArgumentError (unknown keyword: b)

foo 1, 'a' => 2, :b => 3, :z => 4
ArgumentError (unknown keyword: b)

foo 1, z: 3
{}, 3

foo 'a' => 1, 'b' => 2, z: 3
{}, 1

Waiting for your comments and possible edge cases and platforms that might behave differently.

edzhelyov · 2019-03-04T08:48:06Z

lib/rspec/support/method_signature_verifier.rb

@@ -85,6 +85,10 @@ def has_kw_args_in?(args)
        # contain keyword arguments?
        def could_contain_kw_args?(args)
          return false if args.count <= min_non_kw_args
+          if args.count <= max_non_kw_args && Hash === args.last


The reason I've added Hash === args.last is that we use could_contain_kw_args? in https://github.com/rspec/rspec-support/blob/master/lib/rspec/support/method_signature_verifier.rb#L383 and I'm guarding against the matcher.

Fair enough, this could be inlined into one if though?

JonRowe

No major feedback, just a few nits

spec/rspec/support/method_signature_verifier_spec.rb

JonRowe · 2019-03-04T21:53:17Z

lib/rspec/support/method_signature_verifier.rb

@@ -85,6 +85,10 @@ def has_kw_args_in?(args)
        # contain keyword arguments?
        def could_contain_kw_args?(args)
          return false if args.count <= min_non_kw_args
+          if args.count <= max_non_kw_args && Hash === args.last


Fair enough, this could be inlined into one if though?

edzhelyov

@JonRowe I've applied the style changes.

lib/rspec/support/method_signature_verifier.rb

spec/rspec/support/method_signature_verifier_spec.rb

Given a method foo(a, b = {}, x: false) it should recognize that calling foo(1, 'a' => 1, 'b' => 2) is a valid method call. In case when the method has optional arguments and keyword arguments the ruby interpreter will treat the last Hash as keyword arguments if it contains Symbol keys. In the example above calling foo(1, a: 1) will raise an ArgumentError.

If you pass key-value pairs to a method only the keys that are Symbols will be treated as keyword arguments. If there are optional arguments the rest of the keys will be passed to the optional one. Given a method foo(x = {}, y: 1): * foo('a' => 1) => will work * foo('a' => 1, :y => 2) => will work * foo('a' => 1, :b => 2) => will raise ArgumentError unknown keyword: b

edzhelyov · 2019-03-08T15:54:45Z

@JonRowe, not sure if you have received notification, everything is passing now.

JonRowe · 2019-03-08T20:52:11Z

lib/rspec/support/method_signature_verifier.rb

@@ -85,6 +85,10 @@ def has_kw_args_in?(args)
        # contain keyword arguments?
        def could_contain_kw_args?(args)
          return false if args.count <= min_non_kw_args
+          return false if args.count <= max_non_kw_args &&
+            Hash === args.last &&
+            args.last.keys.none? { |x| x.is_a?(Symbol) }


Did this exceed the line length/

Yes, 125 characters in total.

JonRowe · 2019-03-08T20:53:03Z

lib/rspec/support/method_signature_verifier.rb

+                      last.keys
+                    else
+                      args << non_kw_args
+                      last.select { |k, _| k.is_a?(Symbol) }.keys


Are you saying with this that Ruby will mix keyword args and hash keys?

If you provide an anonymous Hash as the last argument only the symbols from that argument will be treated as keyword arguments, the rest will be interpreted as positional one.

def foo(x = {}, y:); puts "#{x}, #{y}"; end foo 'a' => 1, y: 2 => {"a"=>1}, 2 foo x: 3, y: 2 => ArgumentError (unknown keyword: x) def bar(x:, y:); end bar 'x' => 1 => ArgumentError (wrong number of arguments (given 1, expected 0; required keywords: x, y)) bar x: 1, y: 2, z: 3 => ArgumentError (unknown keyword: z)

I'm trying to cover the behavior in this spec

JonRowe · 2019-03-08T20:53:38Z

Sorry for the delay @edzhelyov my cognitive load has been a bit too high to deal with this until now

edzhelyov

@JonRowe no problem, review whenever you can, I'm grateful that you are helping me with moving that forward.

I've moved the symbol arguments logic into has_kw_args_in?, seems a more appropriate place. What do you think?

edzhelyov · 2019-03-09T07:03:26Z

lib/rspec/support/method_signature_verifier.rb

@@ -85,6 +85,10 @@ def has_kw_args_in?(args)
        # contain keyword arguments?
        def could_contain_kw_args?(args)
          return false if args.count <= min_non_kw_args
+          return false if args.count <= max_non_kw_args &&
+            Hash === args.last &&
+            args.last.keys.none? { |x| x.is_a?(Symbol) }


Yes, 125 characters in total.

edzhelyov · 2019-03-09T07:11:51Z

lib/rspec/support/method_signature_verifier.rb

+                      last.keys
+                    else
+                      args << non_kw_args
+                      last.select { |k, _| k.is_a?(Symbol) }.keys


If you provide an anonymous Hash as the last argument only the symbols from that argument will be treated as keyword arguments, the rest will be interpreted as positional one.

def foo(x = {}, y:); puts "#{x}, #{y}"; end foo 'a' => 1, y: 2 => {"a"=>1}, 2 foo x: 3, y: 2 => ArgumentError (unknown keyword: x) def bar(x:, y:); end bar 'x' => 1 => ArgumentError (wrong number of arguments (given 1, expected 0; required keywords: x, y)) bar x: 1, y: 2, z: 3 => ArgumentError (unknown keyword: z)

I'm trying to cover the behavior in this spec

JonRowe · 2019-03-14T22:02:43Z

Thanks so much for your hard work!

…ument Recognize hash as optional arg when optional keyword is present

…ecognize-hash-as-optional-argument Recognize hash as optional arg when optional keyword is present --- This commit was imported from rspec/rspec-support@9601929.

edzhelyov commented Mar 4, 2019

View reviewed changes

JonRowe suggested changes Mar 4, 2019

View reviewed changes

edzhelyov commented Mar 5, 2019

View reviewed changes

lib/rspec/support/method_signature_verifier.rb Show resolved Hide resolved

JonRowe reviewed Mar 5, 2019

View reviewed changes

spec/rspec/support/method_signature_verifier_spec.rb Outdated Show resolved Hide resolved

edzhelyov added 2 commits March 6, 2019 19:26

JonRowe reviewed Mar 8, 2019

View reviewed changes

Move the symbol arguments check in has_kw_args_in?

d24df62

edzhelyov commented Mar 9, 2019

View reviewed changes

JonRowe approved these changes Mar 14, 2019

View reviewed changes

JonRowe merged commit 3619fb6 into rspec:master Mar 14, 2019

JonRowe added a commit that referenced this pull request Mar 14, 2019

Change log for #366

3a6ec66

benoittgt mentioned this pull request Apr 4, 2019

Verify partial doubles fails on method with default hash argument and default keyword argument rspec/rspec-mocks#1266

Closed

JonRowe added a commit that referenced this pull request Oct 2, 2019

Merge pull request #366 from edzhelyov/recognize-hash-as-optional-arg…

9601929

…ument Recognize hash as optional arg when optional keyword is present

pirj mentioned this pull request Dec 11, 2021

Combination of option hash and keyword arguments causes crashes rspec/rspec#104

Open

This was referenced Jan 16, 2024

Allow string keys for method kwarg splats #591

Merged

String keys should be allowed in **kwargs #554

Closed

Uh oh!

Recognize hash as optional arg when optional keyword is present #366

Recognize hash as optional arg when optional keyword is present #366

Uh oh!

Conversation

edzhelyov commented Feb 25, 2019

Uh oh!

JonRowe commented Feb 26, 2019

Uh oh!

edzhelyov commented Feb 27, 2019

Uh oh!

edzhelyov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonRowe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edzhelyov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

edzhelyov commented Mar 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonRowe commented Mar 8, 2019

Uh oh!

edzhelyov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonRowe commented Mar 14, 2019

Uh oh!

Uh oh!

edzhelyov left a comment •

edited

Loading