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

Style/ArgumentsForwarding incorrectly fixes a case where an argument is used elsewhere in the method #12089

Closed
davidenglishmusic opened this issue Aug 1, 2023 · 7 comments
Labels

Comments

@davidenglishmusic
Copy link

The auto-correct for the cop can break code by replacing the arguments when one of them is referenced in another part of the method.


Expected behavior

The following should not elicit a correction:

def method_missing(m, *args, **kwargs, &block)
  if @template.respond_to?(m)
    @template.send(m, *args, **kwargs, &block)
  else
    super
  end
end

Actual behavior

It corrects to the following which throws an undefined variable error:

def method_missing(...)
  if @template.respond_to?(m)
    @template.send(...)
  else
    super
  end
end

Steps to reproduce the problem

Auto-correct the code listed under the expected behavior heading.

RuboCop version

1.55.1 (using Parser 3.2.2.3, rubocop-ast 1.29.0, running on ruby 3.1.1) [x86_64-linux]
  - rubocop-capybara 2.18.0
  - rubocop-factory_bot 2.23.1
  - rubocop-performance 1.18.0
  - rubocop-rails 2.20.2
  - rubocop-rspec 2.23.0
@koic koic added the bug label Aug 1, 2023
@mattbrictson
Copy link

I noticed a similar bug.

Original:

def post(*args, &block)
  future_on(executor, *args, &block)

Erroneously corrected by rubocop to:

def post(...)
  future_on(...)

Expected behavior:

def post(...)
  future_on(executor, ...)

@owst
Copy link
Contributor

owst commented Aug 2, 2023

Thanks for the report - I'll take a look at fixing this. There's clearly some overlap/similarity with #12087 too.

@tmikoss
Copy link

tmikoss commented Aug 7, 2023

Adding another similar example. Produced by rubocop -a, version 1.55.1. association_name is used in the fucntion body.

Screenshot 2023-08-07 at 10 31 07

Edit: rubocop -V:

1.55.1 (using Parser 3.2.2.3, rubocop-ast 1.29.0, running on ruby 3.1.2) [arm64-darwin21]
  - rubocop-performance 1.18.0
  - rubocop-rails 2.20.2

@koic
Copy link
Member

koic commented Aug 8, 2023

FYI, I've opened #12101 to fix due to many reports about this issue. Thank you.

koic added a commit to koic/rubocop that referenced this issue Aug 8, 2023
Fixes rubocop#12087, rubocop#12089, rubocop#12096, and rubocop#12100.

This PR fixes false positives for `Style/ArgumentsForwarding`
when leading argument with rest arguments.
owst added a commit to owst/rubocop that referenced this issue Aug 8, 2023
Additionally fixes:
* rubocop#12089
* rubocop#12096
* rubocop#12100

In Ruby < 3.2 this cop was overzealous and not considering various
additional args/kwargs and other reasons to not allow forward-all
(`...`), and was additionally assuming that forward-all meant all
arguments were replaced, which was not always the case.
@koic koic closed this as completed Aug 8, 2023
@chulkilee
Copy link
Contributor

1.56 included the fix (#12103) but it did not fix the issue.

Source

class Foo
  def bar(*args, &block)
    hello(*args, &block)
    world(*args)
  end
end

Script

bin/rubocop --version
1.56.0

bin/rubocop -a test.rb
Inspecting 1 file
C

Offenses:

test.rb:2:11: C: [Corrected] Style/ArgumentsForwarding: Use shorthand syntax ... for arguments forwarding.
  def bar(*args, &block)
          ^^^^^^^^^^^^^
test.rb:3:11: C: [Corrected] Style/ArgumentsForwarding: Use shorthand syntax ... for arguments forwarding.
    hello(*args, &block)
          ^^^^^^^^^^^^^

Output

class Foo
  def bar(...)
    hello(...)
    world(*args)
  end
end

Can we reopen this issue? Or let me know if opening a new issue is preferred.

@owst
Copy link
Contributor

owst commented Aug 11, 2023

@chulkilee apologies that I've still not got the cop quite right - without looking at the code, your issue looks like a slightly different scenario/cause to me, so I think it should be a new issue. I'll try and take a look over the weekend.

@chulkilee
Copy link
Contributor

@owst Got it - created #12117

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants