search

rspec / rspec-support

Common code needed by the other RSpec gems. Not intended for direct use.
https://rspec.info
MIT License
93 stars 103 forks source link

Improper (not ruby 3 compatible) args forwarding to the method signature verifier #595

Open malcolmohare opened 4 months ago

malcolmohare commented 4 months ago

Subject of the issue

Cross posting issue: https://github.com/rspec/rspec-expectations/issues/1451

The tldr of that issue is for a given function def foo(a=1, b:2) we get ambiguous args in the validator because the code is forwarding args in the ruby 2.x way. The following invocations appear identical to the verifier, but will cause different function parameter assignments.

foo(:b => 2)
foo({:b => 2})

The change I added to allow non-symbolic keys into the kwargs splat uncovered this bug in a very particular regression, see linked issue at the top for details.

I attempted to fix the issue, but as stated above, ran into the problem of unsolvable ambiguity.

The following is a snippet from the ruby3 docs describing the imcompatibility

https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/

Ruby 3 You need to explicitly delegate keyword arguments.

def foo(*args, **kwargs, &block)
  target(*args, **kwargs, &block)
end

Alternatively, if you do not need compatibility with Ruby 2.6 or prior and you don’t alter any arguments, you can use the new delegation syntax (...) that is introduced in Ruby 2.7.

def foo(...)
  target(...)
end

The fix is that the method signature verifier would need to change to accept the args in one of the two ways described in the snippet I linked in ruby 3 environments.
I am not familiar enough with the rspec code base to understand exactly how large a task that is, but I imagine its not small.

I'm open to having my previous contribution reverted until the fix is ready, since its caused at least one regression.

pirj commented 4 months ago

We stick to this https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html as we need to support Ruby 1.8.7+. “…” would be cool, but (unless I’ve missed something in Ruby 3.2+), it can only delegate all args, and foo(1, …) is impossible.

Those discussions may be of some interest:

Does your fix address the regression? If so, I suggest merging it, and if more regressions come up - revert both and thinking about a more generic approach to the problem.

I wonder if your fix would also fix this case?

malcolmohare commented 4 months ago

It seems like I am not the first to try and fix this, and the issue is well identified.

I propose we close this as a duplicate of https://github.com/rspec/rspec-support/issues/522

My fix does not help (and is mostly a more succinct version of the pull request from JonRowe). My original change which caused the reported regression just relaxed the conditions for which the code would incorrectly identify a positional argument as the keyword argument hash.

malcolmohare commented 4 months ago

Actually I took another try at the fix and I think I have something that is more correct, but does rely on callers using the ruby2_keywords annotation.