rubocop / rubocop-performance

An extension of RuboCop focused on code performance checks.
https://docs.rubocop.org/rubocop-performance
MIT License
670 stars 79 forks source link

The autocorrection for Performance/StringIdentifierArgument breaks code #425

Closed davidenglishmusic closed 8 months ago

davidenglishmusic commented 9 months ago

Expected behavior

The cop's autocorrection should not break code.

Actual behavior

The cop's autocorrection breaks code.

Steps to reproduce the problem

The following code runs:

# frozen_string_literal: true

module MyNamespace
  class MyClass
  end
end

prefix = 'MyNamespace::'
class_name = 'MyClass'
Kernel.const_get("#{prefix}#{class_name}")

Running rubocop on it transforms it to this:

# frozen_string_literal: true

module MyNamespace
  class MyClass
  end
end

prefix = 'MyNamespace::'
class_name = 'MyClass'
Kernel.const_get(:"#{prefix}#{class_name}")

Running the corrected code produces the following error:

app.rb:10:in `const_get': wrong constant name MyNamespace::MyClass (NameError)

Kernel.const_get(:"#{prefix}#{class_name}")
      ^^^^^^^^^^
    from app.rb:10:in `<main>'

RuboCop version

1.59.0 (using Parser 3.2.2.4, rubocop-ast 1.30.0, running on ruby 3.1.1) [x86_64-linux]
  - rubocop-performance 1.20.0
mikdiet commented 8 months ago

I confirm this in ruby 3.2.2 (2023-03-30 revision e51014f9c0) [x86_64-darwin21] too.

collinsauve commented 8 months ago

Hello 👋

Our project updated rubocop-performance today so we noticed this bug.

In addition to the autocorrect being unsafe, this rule matching on string interpolation at all seems (to me) to be incorrect. Rubocop is not able to determine if a string interpolation usage is indeed "redundant".

Earlopain commented 8 months ago

Hm, this is a bit unfortunate. The problem isn't the interpolation per se but just passing a symbol with doube colons in the first place. This on its own raises as well:

Kernel.const_get(:"MyNamespace::MyClass")

I'm actually not sure if this is intended on the side of ruby or not since I'm not finding documentation on this behaviour where I'd expect it. It doesn't seem to recurse if a symbol is given.

okuramasafumi commented 8 months ago

https://bugs.ruby-lang.org/issues/12319 I found this issue in Ruby core. In short, it's an intended behavior.