Skip to content

Support ruby 3.1 :args_forward shape change#1431

Merged
lsegal merged 1 commit intolsegal:mainfrom
nvasilevski:support-ruby-3-args-forward-shape-change
Jun 1, 2022
Merged

Support ruby 3.1 :args_forward shape change#1431
lsegal merged 1 commit intolsegal:mainfrom
nvasilevski:support-ruby-3-args-forward-shape-change

Conversation

@nvasilevski
Copy link
Contributor

@nvasilevski nvasilevski commented Feb 1, 2022

Description

Currently yard errors when tries to parse method with args forwarding using Ruby 3.1:

[error]: NoMethodError: undefined method `source' for "&":String

params << ['&' + args.block_param.source, nil] if args.block_param
  | [error]: Stack trace:
  | /tmp/bundle/ruby/3.1.0/gems/yard-0.9.27/lib/yard/handlers/ruby/method_handler.rb:100:in `format_args'

This is happening because Ruby 3.1 changes params statement for a method with args forwarding from:
Reference:
https://kddnewton.com/2022/01/08/ripper-changelog-3.1.0.html#argument-forwarding

[:params, nil, nil, [:args_forward], nil, nil, nil, nil]

to:

 [:params, nil, nil, nil, nil, nil, [:args_forward], :&]

So the block_param wrongly assumes that :& is an explicit block argument.
I don't think there is an issue with block_param method, the "shape" of the statement stays the same and the last value in the array is references block param, it's just in this particular case the block param is "anonymous" or "implicit" if I may call it like that.

So I'm adding additional args_forward check to avoid processing :& block.
args_forward method works for both Ruby 3.1 and older versions even though we don't necessarily need it for Ruby lower than 3.1

Also added test for the anonymous block forwarding. It's not related to the issue but I thought it can't hurt having it just in case

Completed Tasks

  • I have read the Contributing Guide.
  • The pull request is complete (implemented / written).
  • Git commits have been cleaned up (squash WIP / revert commits).
  • I wrote tests and ran bundle exec rake locally (if code is attached to PR).

@hx
Copy link

hx commented May 31, 2022

Thanks you @nvasilevski for this PR. It works and makes sense to me. @lsegal is there anything we can do to help get this merged?

@lsegal
Copy link
Owner

lsegal commented Jun 1, 2022

Thanks for the implementation! Sorry for the delay on this, it looks good!

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.

3 participants

Comments