From 9a526622cd35edee3d577cb5ba79f570c4eaec36 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Tue, 7 Dec 2021 01:45:18 +0900 Subject: [PATCH] Add new `Performance/StringIdentifierArgument` cop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR adds new `Performance/StringIdentifierArgument` cop. This cop identifies places where string identifier argument can be replaced by symbol identifier argument. It prevents the redundancy of the internal string-to-symbol conversion. This cop targets methods that take identifier (e.g. method name) argument and the following examples are parts of it. ```ruby # bad send('do_something') attr_accessor 'do_something' instance_variable_get('@ivar') # good send(:do_something) attr_accessor :do_something instance_variable_get(:@ivar) ``` The `send` method is a representative and other similar methods are similar benchmarks. The following is an example benchmark. ```ruby % cat symbol.rb puts `ruby -v` require 'benchmark/ips' def foo end Benchmark.ips do |x| x.report('symbol arg') { send(:foo) } x.report('string arg') { send('foo') } x.compare! end ``` ```console % ruby symbol.rb ruby 3.1.0dev (2021-12-05T10:23:42Z master 19f037e452) [x86_64-darwin19] Warming up -------------------------------------- symbol arg 1.025M i/100ms string arg 590.038k i/100ms Calculating ------------------------------------- symbol arg 10.665M (± 1.5%) i/s - 53.318M in 5.000551s string arg 5.895M (± 1.0%) i/s - 29.502M in 5.004999s Comparison: symbol arg: 10665035.8 i/s string arg: 5895132.3 i/s - 1.81x (± 0.00) slower ``` I learned about this performance difference from the book "Polished Ruby Programming". --- ...formance_string_identifier_argument_cop.md | 1 + config/default.yml | 5 ++ .../performance/string_identifier_argument.rb | 59 +++++++++++++++++++ lib/rubocop/cop/performance_cops.rb | 1 + .../string_identifier_argument_spec.rb | 53 +++++++++++++++++ 5 files changed, 119 insertions(+) create mode 100644 changelog/new_add_new_performance_string_identifier_argument_cop.md create mode 100644 lib/rubocop/cop/performance/string_identifier_argument.rb create mode 100644 spec/rubocop/cop/performance/string_identifier_argument_spec.rb diff --git a/changelog/new_add_new_performance_string_identifier_argument_cop.md b/changelog/new_add_new_performance_string_identifier_argument_cop.md new file mode 100644 index 0000000000..0aeccf9bd6 --- /dev/null +++ b/changelog/new_add_new_performance_string_identifier_argument_cop.md @@ -0,0 +1 @@ +* [#276](https://github.com/rubocop/rubocop-performance/pull/276): Add new `Performance/StringIdentifierArgument` cop. ([@koic][]) diff --git a/config/default.yml b/config/default.yml index 8960ec5da0..8292a281ab 100644 --- a/config/default.yml +++ b/config/default.yml @@ -311,6 +311,11 @@ Performance/StartWith: VersionAdded: '0.36' VersionChanged: '1.10' +Performance/StringIdentifierArgument: + Description: 'Use symbol identifier argument instead of string identifier argument.' + Enabled: pending + VersionAdded: '<>' + Performance/StringInclude: Description: 'Use `String#include?` instead of a regex match with literal-only pattern.' Enabled: 'pending' diff --git a/lib/rubocop/cop/performance/string_identifier_argument.rb b/lib/rubocop/cop/performance/string_identifier_argument.rb new file mode 100644 index 0000000000..52b6606a6f --- /dev/null +++ b/lib/rubocop/cop/performance/string_identifier_argument.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Performance + # This cop identifies places where string identifier argument can be replaced + # by symbol identifier argument. + # It prevents the redundancy of the internal string-to-symbol conversion. + # + # This cop targets methods that take identifier (e.g. method name) argument + # and the following examples are parts of it. + # + # @example + # + # # bad + # send('do_something') + # attr_accessor 'do_something' + # instance_variable_get('@ivar') + # + # # good + # send(:do_something) + # attr_accessor :do_something + # instance_variable_get(:@ivar) + # + class StringIdentifierArgument < Base + extend AutoCorrector + + MSG = 'Use `%s` instead of `%s`.' + + RESTRICT_ON_SEND = %i[ + alias_method attr attr_accessor attr_reader attr_writer autoload autoload? + class_variable_defined? const_defined? const_get const_set const_source_location + 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 module_function private private_constant protected public public_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__ + ].freeze + + def on_send(node) + return unless (first_argument = node.first_argument) + return unless first_argument.str_type? + return if first_argument.value.include?(' ') + + replacement = first_argument.value.to_sym.inspect + + message = format(MSG, symbol_arg: replacement, string_arg: first_argument.source) + + add_offense(first_argument, message: message) do |corrector| + corrector.replace(first_argument, replacement) + end + end + end + end + end +end diff --git a/lib/rubocop/cop/performance_cops.rb b/lib/rubocop/cop/performance_cops.rb index fc363b712d..02a64a6300 100644 --- a/lib/rubocop/cop/performance_cops.rb +++ b/lib/rubocop/cop/performance_cops.rb @@ -44,6 +44,7 @@ require_relative 'performance/sort_reverse' require_relative 'performance/squeeze' require_relative 'performance/start_with' +require_relative 'performance/string_identifier_argument' require_relative 'performance/string_include' require_relative 'performance/string_replacement' require_relative 'performance/sum' diff --git a/spec/rubocop/cop/performance/string_identifier_argument_spec.rb b/spec/rubocop/cop/performance/string_identifier_argument_spec.rb new file mode 100644 index 0000000000..9691afa2f7 --- /dev/null +++ b/spec/rubocop/cop/performance/string_identifier_argument_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +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 + + 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 'does not register an offense when using interpolated string argument' do + expect_no_offenses(<<~'RUBY') + send("do_something_#{var}") + RUBY + end + end + + it 'does not register an offense when no arguments' do + expect_no_offenses(<<~RUBY) + send + RUBY + end + + it 'does not register an offense when using integer argument' do + expect_no_offenses(<<~RUBY) + send(42) + RUBY + end + + it 'does not register an offense when using symbol argument for no identifier argument' do + expect_no_offenses(<<~RUBY) + foo('do_something') + RUBY + end + + # e.g. Trunip https://github.com/jnicklas/turnip#calling-steps-from-other-steps + it 'does not register an offense when using string argument includes spaces' do + expect_no_offenses(<<~RUBY) + send(':foo is :bar', foo, bar) + RUBY + end +end