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

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

Open
malcolmohare opened this issue Feb 25, 2024 · 3 comments

Comments

@malcolmohare
Copy link
Contributor

Subject of the issue

Cross posting issue: rspec/rspec-expectations#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
Copy link
Member

pirj commented Feb 25, 2024

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
Copy link
Contributor Author

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 #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
Copy link
Contributor Author

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.

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

No branches or pull requests

2 participants