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

The autocorrection for Performance/StringIdentifierArgument breaks code #425

Closed
davidenglishmusic opened this issue Dec 18, 2023 · 4 comments

Comments

@davidenglishmusic
Copy link

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

mikdiet commented Dec 19, 2023

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

@collinsauve
Copy link

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

Earlopain commented Dec 19, 2023

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

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

okuramasafumi added a commit to okuramasafumi/rubocop-performance that referenced this issue Dec 21, 2023
[Fix rubocop#425]
`const_get` doesn't accept symbols that are something like
`:"Foo::Bar"`, and that's intended according to this issue.
https://bugs.ruby-lang.org/issues/12319
Therefore, changing String to Symbol might not work as expected.
As far as I know that's related to `const_get` only so we can remove
it from the target of `StringIdentifierArgument`
okuramasafumi added a commit to okuramasafumi/rubocop-performance that referenced this issue Dec 21, 2023
[Fix rubocop#425]
`const_get` doesn't accept symbols that are something like
`:"Foo::Bar"`, and that's intended according to this issue.
https://bugs.ruby-lang.org/issues/12319
Therefore, changing String to Symbol might not work as expected.
As far as I know that's related to `const_get` only so we can remove
it from the target of `StringIdentifierArgument`
razumau added a commit to zendesk/property_sets that referenced this issue Jan 3, 2024
Mostly it’s `Performance/StringIdentifierArgument`: methods like `instance_variable_get` or `send` should use symbols, not strings.

However, `const_get` does not accepts symbols with nested constants, like `Smth::Setting` (see this issue: rubocop/rubocop-performance#425, which might get fixed by rubocop/rubocop-performance#427). I’ve added `# standard:disable` in `spec/property_sets_spec.rb` until that fix is released.
Earlopain added a commit to Earlopain/rubocop-performance that referenced this issue Jan 3, 2024
Earlopain added a commit to Earlopain/rubocop-performance that referenced this issue Jan 3, 2024
@koic koic closed this as completed in 749d072 Jan 4, 2024
koic added a commit that referenced this issue Jan 4, 2024
[Fix #425] Fix a false positive for`Performance/StringIdentifierArgument
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 a pull request may close this issue.

5 participants