From 98ca79d74f9a17211af9612442afa06cfecd612c Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Sat, 23 Dec 2023 16:56:05 +0900 Subject: [PATCH] Fix false negatives for `Performance/StringIdentifierArgument` This PR fixes false negatives for `Performance/StringIdentifierArgument` when using multiple string arguments. e.g., `alias_method 'new', 'original'` --- ..._performance_string_identifier_argument.md | 1 + .../performance/string_identifier_argument.rb | 44 +++++++--- .../string_identifier_argument_spec.rb | 81 +++++++++++++++---- 3 files changed, 98 insertions(+), 28 deletions(-) create mode 100644 changelog/fix_false_negatives_for_performance_string_identifier_argument.md diff --git a/changelog/fix_false_negatives_for_performance_string_identifier_argument.md b/changelog/fix_false_negatives_for_performance_string_identifier_argument.md new file mode 100644 index 0000000000..f283876232 --- /dev/null +++ b/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][]) diff --git a/lib/rubocop/cop/performance/string_identifier_argument.rb b/lib/rubocop/cop/performance/string_identifier_argument.rb index 4f358e982f..c98121ac15 100644 --- a/lib/rubocop/cop/performance/string_identifier_argument.rb +++ b/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? diff --git a/spec/rubocop/cop/performance/string_identifier_argument_spec.rb b/spec/rubocop/cop/performance/string_identifier_argument_spec.rb index e6ead8a614..00e3014d27 100644 --- a/spec/rubocop/cop/performance/string_identifier_argument_spec.rb +++ b/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