Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix keyword arguments in dynamic method dispatch #2509

Closed

Conversation

JoshCheek
Copy link

Ruby 3 changed the way keyword arguments work. Currently, to forward arguments, the only way that works in Ruby 2.6, 2.7, and 3.0, is to call ruby2_keywords :method_name for the method that receives the arguments.

This will cause Ruby to remember that the final hash was created by keyword arguments, and then when expanding out the arguments later, eg here, it will see that the last element was created by keywords, and it will pass that into the keywords area of a method infocation.

If we don't do this, it will consider it an ordinal argument and not line it up with the keywords. This typically causes a "wrong number of arguments" error, because it's passing an extra ordinal argument.

Here is another example from within RSpec, where they have arrived at this solution for the same problem: rspec/matchers/dsl.rb#L538-L540

Currently, my team is hacking around this with:

# What we are currently doing without this patch (we're on Ruby 3.0.1)
def has_updated?(model, ruby2_attrs = {}, **ruby3_attrs)
  attrs = ruby2_attrs.merge(ruby3_attrs)
  # ...
end

# What we will do with this patch
def has_updated?(model, **attrs)
  # ...
end

JoshCheek and others added 2 commits July 14, 2021 19:18
Ruby 3 changed the way keyword arguments work.
Currently, to forward arguments, the only way that works in Ruby 2.6, 2.7, and 3.0,
is to call `ruby2_keywords :method_name` for the method that receives the arguments.

This will cause Ruby to remember that the final hash was created by keyword arguments,
and then when expanding out the arguments later, eg [here](https://github.com/rspec/rspec-expectations/blob/43bf64b01f8356979ffbc373b2e81d2ab1389b29/lib/rspec/matchers/built_in/has.rb#L67),
it will see that the last element was created by keywords, and it will pass that into
the keywords area of a method infocation.

If we don't do this, it will consider it an ordinal argument and not line it up with
the keywords. This typically causes a "wrong number of arguments" error, because it's
passing an extra ordinal argument.

Here is another example from within RSpec, where they have arrived at this solution
for the same problem: https://github.com/rspec/rspec-expectations/blob/43bf64b01f8356979ffbc373b2e81d2ab1389b29/lib/rspec/matchers/dsl.rb#L538-L540
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you.
Do you mind adding a Changelog entry?

The discrepancy between the code comment and example description might be a bit confusing. Do you think it makes sense to align them?

Copy link
Member

@JonRowe JonRowe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 So we're aware of this change as you can see from other code, and have tests in place to ensure they work correctly, I'm not sure what exactly you're changing here but it would only affect controller specs? rspec-rails doesn't implement dynamic matchers at all so this fix is at best badly named...

@JoshCheek
Copy link
Author

👋 So we're aware of this change as you can see from other code, and have tests in place to ensure they work correctly, I'm not sure what exactly you're changing here but it would only affect controller specs? rspec-rails doesn't implement dynamic matchers at all so this fix is at best badly named...

Perhaps something more technical, such as it "does not alter keyword missing's keyword arguments"?

@JoshCheek
Copy link
Author

Okay, I changed the name to be more technical, and then felt like it wasn't very obvious how the test was an example of the description. So I changed it to not depend on the group at all. But IDK if this is better or worse, so figure I'll get y'all's feedback before pushing it.

    it "does not alter method_missing's keyword arguments" do
      klass = Class.new.prepend described_class
      klass.define_method(:method_missing) { |a, b, c:, &d| {a: a, b: b, c: c, d: d} }
      block = -> { }
      expect(klass.new.a(:b, c: :c, &block)).to eq a: :a, b: :b, c: :c, d: block
    end

@JoshCheek
Copy link
Author

Like for me, it makes sense, but I'm a little worried it might be too terse/magical for other people to read.

@pirj
Copy link
Member

pirj commented Jul 17, 2021

I suggest keeping the current example code. It is clear that the method is not a named route. Dynamic matcher is one of completely legitimate cases from all possible options.

The title, though, is too specific. It mentions dynamic matchers, while it can be anything else.
What do you think of "it correctly delegates keyword arguments to the superclass implementation"? Does it make sense to clarify "when the method is not a named route helper"?

Comment on lines +24 to +26
example.define_singleton_method(:has_val?) { |val:| val == 1 }
expect(example).to example.have_val(val: 1)
expect(example).to_not example.have_val(val: 2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
example.define_singleton_method(:has_val?) { |val:| val == 1 }
expect(example).to example.have_val(val: 1)
expect(example).to_not example.have_val(val: 2)
example.define_singleton_method(:keyword_value) { |value:| value }
expect(example.keyword_value(value: 42)).to eq 42

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer a clear example method here not something imitating a matcher as thats not what is the issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is not hitting the method_missing. We defined keyword_value and call it.

The current expect(example).to example.have_val(val: 1) works, but it's incredibly confusing.

expect { example.have_val(val: 1) }.not_to raise_error

failed to catch the regression, it doesn't fail if ruby2_keywords is removed.

I'll figure out a self-explanatory spec to cover this. I intend to rely on dynamic matchers, but will try to avoid the expect(example).to part.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just don't want a have_ anything style example, its confusing, its the method dispatch thats the bug here, not matchers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit of both.

To reproduce the issue:

  • it is not sufficient for us to just call the same method that we define, as method_missing won't be called
  • there should be some code that handles method_missing below ControllerExampleGroup and below Rails
  • this code should accept arguments and pass it to a method that accepts keyword arguments

We could write a spec with something like prepend Module.new { def method_missing ... }, but this might be excessively complicated.

There are dynamic matchers. They do exactly what we want - they have a method_missing, and they can delegate to the method we define.

It's still a brain-teaser how to express the case in a spec. The best I could think of so far:

expect {
  example.have_val(val: 1).matches?(example)
}.not_to raise_error

example.have_val goes through ControllerExampleGroup's method_missing, then through Rails' one, and lands in dynamic matchers. They save the kwarg to @args, and the flag that it's a kwarg is already set on the object.
matches? calls has_val? on the example, and passes *@args to it.

It's not necessary to call have_val on example, and we could:

expect(example).to have_val(val: 1)

but in this case ControllerExampleGroup's method_missing is never called, and it's dynamic matchers' method_missing who sets the flag. I.e. this fix would not be covered by such a test.

Frankly speaking, it seems like it doesn't matter much what exactly the spec will be, we'll have to add a chunk of code comments along. And I have strong doubts that in a month from now it will take me less than half an hour to recall all the details about this tangled case.

I'm out of good ideas.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If my change doesn't trigger method missing neither does the original, they both call the method directly I just skip the expect to. So my original point stands here, please construct an example that doesn't look like a matcher.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JonRowe There is a significant difference. You call a method that we've just defined, keyword_value, while the original example calls a non-existent method, have_val, while we define has_val?.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah tired eyes etc, ok fine, implement a method missing and remove the has / have names. This is a lot of conversation for a simple fix, but it needs to not be confused with matchers. Its method_missing and the method names that are causing this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Dynamic matchers are a victim here.
And they are the only known victim. And they apparently are the only way to test the fix without involving in_sub_process { ControllerExampleGroup.prepend Module.new { def method_missing ... end } }.

@JonRowe
Copy link
Member

JonRowe commented Jul 27, 2021

My understanding here is that the method_missing in controller specs is dropping keywords without this check, but it doesn't actually have anything to do with matchers, so I've requested some changes to reflect that

pirj and others added 2 commits July 27, 2021 23:07
Co-authored-by: Jon Rowe <mail@jonrowe.co.uk>
Co-authored-by: Jon Rowe <mail@jonrowe.co.uk>
JonRowe added a commit that referenced this pull request Aug 13, 2021
JonRowe added a commit that referenced this pull request Aug 13, 2021
@JonRowe
Copy link
Member

JonRowe commented Aug 13, 2021

Closing in favour of #2514 which has a spec demonstrating the issue in isolation, thanks for the help @JoshCheek

@JonRowe JonRowe closed this Aug 13, 2021
@JonRowe JonRowe changed the title Fix keyword arguments in dynamic matchers Fix keyword arguments in dynamic method dispatch Aug 13, 2021
JonRowe added a commit that referenced this pull request Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants