Merge pull request #428 from koic/fix_false_negatives_make_performanc…

…e_string_identifier_argument

Fix false negatives for `Performance/StringIdentifierArgument`

changelog/fix_false_negatives_for_performance_string_identifier_argument.md

@@ -0,0 +1 @@
+* [#428](https://github.com/rubocop/rubocop-performance/pull/428): Fix false negatives for `Performance/StringIdentifierArgument` when using multiple string arguments. ([@koic][])

lib/rubocop/cop/performance/string_identifier_argument.rb

@@ -34,6 +34,12 @@ class StringIdentifierArgument < Base
           protected public public_constant module_function
         ].freeze
 
+        TWO_ARGUMENTS_METHOD = :alias_method
+        MULTIPLE_ARGUMENTS_METHODS = %i[
+          attr_accessor attr_reader attr_writer private private_constant
+          protected public public_constant module_function
+        ].freeze
+
         # NOTE: `attr` method is not included in this list as it can cause false positives in Nokogiri API.
         # And `attr` may not be used because `Style/Attr` registers an offense.
         # https://github.com/rubocop/rubocop-performance/issues/278
@@ -47,26 +53,42 @@ class StringIdentifierArgument < Base
           respond_to? send singleton_method __send__
         ] + COMMAND_METHODS).freeze
 
-        # rubocop:disable Metrics/CyclomaticComplexity
         def on_send(node)
           return if COMMAND_METHODS.include?(node.method_name) && node.receiver
-          return unless (first_argument = node.first_argument)
-          return unless first_argument.str_type? || first_argument.dstr_type?
 
-          first_argument_value = first_argument.value
-          return if first_argument_value.include?(' ') || first_argument_value.include?('::')
+          string_arguments(node).each do |string_argument|
+            string_argument_value = string_argument.value
+            next if string_argument_value.include?(' ') || string_argument_value.include?('::')
+
+            register_offense(string_argument, string_argument_value)
+          end
+        end
 
-          replacement = argument_replacement(first_argument, first_argument_value)
+        private
 
-          message = format(MSG, symbol_arg: replacement, string_arg: first_argument.source)
+        def string_arguments(node)
+          arguments = if node.method?(TWO_ARGUMENTS_METHOD)
+                        [node.first_argument, node.arguments[1]]
+                      elsif MULTIPLE_ARGUMENTS_METHODS.include?(node.method_name)
+                        node.arguments
+                      else
+                        [node.first_argument]
+                      end
 
-          add_offense(first_argument, message: message) do |corrector|
-            corrector.replace(first_argument, replacement)
+          arguments.compact.filter do |argument|
+            argument.str_type? || argument.dstr_type?
           end
         end
-        # rubocop:enable Metrics/CyclomaticComplexity
 
-        private
+        def register_offense(argument, argument_value)
+          replacement = argument_replacement(argument, argument_value)
+
+          message = format(MSG, symbol_arg: replacement, string_arg: argument.source)
+
+          add_offense(argument, message: message) do |corrector|
+            corrector.replace(argument, replacement)
+          end
+        end
 
         def argument_replacement(node, value)
           if node.str_type?

spec/rubocop/cop/performance/string_identifier_argument_spec.rb

@@ -2,31 +2,78 @@
 
 RSpec.describe RuboCop::Cop::Performance::StringIdentifierArgument, :config do
   RuboCop::Cop::Performance::StringIdentifierArgument::RESTRICT_ON_SEND.each do |method|
-    it "registers an offense when using string argument for `#{method}` method" do
-      expect_offense(<<~RUBY, method: method)
-        #{method}('do_something')
-        _{method} ^^^^^^^^^^^^^^ Use `:do_something` instead of `'do_something'`.
-      RUBY
+    if method == RuboCop::Cop::Performance::StringIdentifierArgument::TWO_ARGUMENTS_METHOD
+      it 'registers an offense when using string two arguments for `alias_method`' do
+        expect_offense(<<~RUBY)
+          alias_method 'new', 'original'
+                       ^^^^^ Use `:new` instead of `'new'`.
+                              ^^^^^^^^^^ Use `:original` instead of `'original'`.
+        RUBY
 
-      expect_correction(<<~RUBY)
-        #{method}(:do_something)
-      RUBY
-    end
+        expect_correction(<<~RUBY)
+          alias_method :new, :original
+        RUBY
+      end
 
-    it "does not register an offense when using symbol argument for `#{method}` method" do
-      expect_no_offenses(<<~RUBY)
-        #{method}(:do_something)
-      RUBY
+      it 'does not register an offense when using symbol two arguments for `alias_method`' do
+        expect_no_offenses(<<~RUBY)
+          alias_method :new, :original
+        RUBY
+      end
+
+      it 'does not register an offense when using symbol single argument for `alias_method`' do
+        expect_no_offenses(<<~RUBY)
+          alias_method :new
+        RUBY
+      end
+    else
+      it "registers an offense when using string argument for `#{method}` method" do
+        expect_offense(<<~RUBY, method: method)
+          #{method}('do_something')
+          _{method} ^^^^^^^^^^^^^^ Use `:do_something` instead of `'do_something'`.
+        RUBY
+
+        expect_correction(<<~RUBY)
+          #{method}(:do_something)
+        RUBY
+      end
+
+      it "does not register an offense when using symbol argument for `#{method}` method" do
+        expect_no_offenses(<<~RUBY)
+          #{method}(:do_something)
+        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
+      end
     end
+  end
 
-    it 'registers an offense when using interpolated string argument' do
+  RuboCop::Cop::Performance::StringIdentifierArgument::MULTIPLE_ARGUMENTS_METHODS.each do |method|
+    it "registers an offense when using string multiple arguments for `#{method}` method" do
       expect_offense(<<~RUBY, method: method)
-        #{method}("do_something_\#{var}")
-        _{method} ^^^^^^^^^^^^^^^^^^^^^ Use `:"do_something_\#{var}"` instead of `"do_something_\#{var}"`.
+        #{method} 'one', 'two', 'three'
+        _{method} ^^^^^ Use `:one` instead of `'one'`.
+        _{method}        ^^^^^ Use `:two` instead of `'two'`.
+        _{method}               ^^^^^^^ Use `:three` instead of `'three'`.
       RUBY
 
       expect_correction(<<~RUBY)
-        #{method}(:"do_something_\#{var}")
+        #{method} :one, :two, :three
+      RUBY
+    end
+
+    it "does not register an offense when using symbol multiple arguments for `#{method}`" do
+      expect_no_offenses(<<~RUBY)
+        #{method} :one, :two, :three
       RUBY
     end
   end