diff --git a/changelog/fix_false_positive_for_string_identifier_argument.md b/changelog/fix_false_positive_for_string_identifier_argument.md new file mode 100644 index 0000000000..4c5a3e1690 --- /dev/null +++ b/changelog/fix_false_positive_for_string_identifier_argument.md @@ -0,0 +1 @@ +* [#425](https://github.com/rubocop/rubocop-performance/issues/425): Fix a false positive for `Performance/StringIdentifierArgument` when using string interpolation with methods that don't support symbols with `::` inside them. ([@earlopain][]) diff --git a/lib/rubocop/cop/performance/string_identifier_argument.rb b/lib/rubocop/cop/performance/string_identifier_argument.rb index c98121ac15..54c593e0fb 100644 --- a/lib/rubocop/cop/performance/string_identifier_argument.rb +++ b/lib/rubocop/cop/performance/string_identifier_argument.rb @@ -16,13 +16,19 @@ module Performance # send('do_something') # attr_accessor 'do_something' # instance_variable_get('@ivar') - # const_get("string_#{interpolation}") + # respond_to?("string_#{interpolation}") # # # good # send(:do_something) # attr_accessor :do_something # instance_variable_get(:@ivar) - # const_get(:"string_#{interpolation}") + # respond_to?(:"string_#{interpolation}") + # + # # good - these methods don't support namespaced symbols + # const_get("#{module_path}::Base") + # const_source_location("#{module_path}::Base") + # const_defined?("#{module_path}::Base") + # # class StringIdentifierArgument < Base extend AutoCorrector @@ -34,6 +40,8 @@ class StringIdentifierArgument < Base protected public public_constant module_function ].freeze + INTERPOLATION_IGNORE_METHODS = %i[const_get const_source_location const_defined?].freeze + TWO_ARGUMENTS_METHOD = :alias_method MULTIPLE_ARGUMENTS_METHODS = %i[ attr_accessor attr_reader attr_writer private private_constant @@ -44,14 +52,14 @@ class StringIdentifierArgument < Base # And `attr` may not be used because `Style/Attr` registers an offense. # https://github.com/rubocop/rubocop-performance/issues/278 RESTRICT_ON_SEND = (%i[ - class_variable_defined? const_defined? const_get const_set const_source_location + class_variable_defined? const_set define_method instance_method method_defined? private_class_method? private_method_defined? protected_method_defined? public_class_method public_instance_method public_method_defined? remove_class_variable remove_method undef_method class_variable_get class_variable_set deprecate_constant remove_const ruby2_keywords define_singleton_method instance_variable_defined? instance_variable_get instance_variable_set method public_method public_send remove_instance_variable respond_to? send singleton_method __send__ - ] + COMMAND_METHODS).freeze + ] + COMMAND_METHODS + INTERPOLATION_IGNORE_METHODS).freeze def on_send(node) return if COMMAND_METHODS.include?(node.method_name) && node.receiver @@ -75,9 +83,13 @@ def string_arguments(node) [node.first_argument] end - arguments.compact.filter do |argument| - argument.str_type? || argument.dstr_type? - end + arguments.compact.filter { |argument| string_argument_compatible?(argument, node) } + end + + def string_argument_compatible?(argument, node) + return true if argument.str_type? + + argument.dstr_type? && INTERPOLATION_IGNORE_METHODS.none? { |method| node.method?(method) } end def register_offense(argument, argument_value) diff --git a/spec/rubocop/cop/performance/string_identifier_argument_spec.rb b/spec/rubocop/cop/performance/string_identifier_argument_spec.rb index 00e3014d27..d96dc8f771 100644 --- a/spec/rubocop/cop/performance/string_identifier_argument_spec.rb +++ b/spec/rubocop/cop/performance/string_identifier_argument_spec.rb @@ -44,15 +44,26 @@ RUBY end - it 'registers an offense when using interpolated string argument' do - expect_offense(<<~RUBY, method: method) - #{method}("do_something_\#{var}") - _{method} ^^^^^^^^^^^^^^^^^^^^^ Use `:"do_something_\#{var}"` instead of `"do_something_\#{var}"`. - RUBY - - expect_correction(<<~RUBY) - #{method}(:"do_something_\#{var}") - RUBY + if RuboCop::Cop::Performance::StringIdentifierArgument::INTERPOLATION_IGNORE_METHODS.include?(method) + it 'does not register an offense when using string interpolation for `#{method}` method' do + # NOTE: These methods don't support `::` when passing a symbol. const_get('A::B') is valid + # but const_get(:'A::B') isn't. Since interpolated arguments may contain any content these + # cases are not detected as an offense to prevent false positives. + expect_no_offenses(<<~RUBY) + #{method}("\#{module_name}class_name") + RUBY + end + else + it 'registers an offense when using interpolated string argument' do + expect_offense(<<~RUBY, method: method) + #{method}("do_something_\#{var}") + _{method} ^^^^^^^^^^^^^^^^^^^^^ Use `:"do_something_\#{var}"` instead of `"do_something_\#{var}"`. + RUBY + + expect_correction(<<~RUBY) + #{method}(:"do_something_\#{var}") + RUBY + end end end end