From 78047b00b91035cf3ed593c3a0e78643f65bbade Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Wed, 6 May 2020 23:13:02 +0900 Subject: [PATCH] Add new `Performance/DeletePrefix` and `Performance/DeleteSuffix` cops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR adds new `Performance/DeletePrefix` and `Performance/DeleteSuffix` cops. In Ruby 2.5, `String#delete_prefix` and `String#delete_suffix` have been added. - https://ruby-doc.org/core-2.5.8/String.html#method-i-delete_prefix - https://ruby-doc.org/core-2.5.8/String.html#method-i-delete_suffix `Performance/DeletePrefix` cop identifies places where `gsub(/\Aprefix/, '')` can be replaced by `delete_prefix('prefix')`. ```ruby # bad str.gsub(/\Aprefix/, '') str.gsub!(/\Aprefix/, '') str.gsub(/^prefix/, '') str.gsub!(/^prefix/, '') # good str.delete_prefix('prefix') str.delete_prefix!('prefix') ``` The `delete_prefix('prefix')` method is faster than `gsub(/\Aprefix/, '')`. ```console % ruby -v ruby 2.5.8p224 (2020-03-31 revision 67882) [x86_64-darwin17] % cat bench.rb require 'benchmark/ips' Benchmark.ips do |x| str = 'foobar' x.report('gsub') { str.gsub(/bar\z/, '') } x.report('gsub!') { str.gsub(/bar\z/, '') } x.report('delete_suffix') { str.delete_suffix('bar') } x.report('delete_suffix!') { str.delete_suffix('bar') } x.compare! end % ruby bench.rb Warming up -------------------------------------- gsub 46.814k i/100ms gsub! 46.896k i/100ms delete_suffix 211.337k i/100ms delete_suffix! 208.332k i/100ms Calculating ------------------------------------- gsub 546.500k (± 1.3%) i/s - 2.762M in 5.054918s gsub! 551.054k (± 1.2%) i/s - 2.767M in 5.021747s delete_suffix 4.780M (± 1.1%) i/s - 24.092M in 5.040850s delete_suffix! 4.770M (± 1.1%) i/s - 23.958M in 5.022823s Comparison: delete_suffix: 4780060.8 i/s delete_suffix!: 4770419.3 i/s - same-ish: difference falls within error gsub!: 551054.2 i/s - 8.67x slower gsub: 546500.1 i/s - 8.75x slower ``` `Performance/DeleteSuffix` cop is the same. --- CHANGELOG.md | 1 + config/default.yml | 10 ++ lib/rubocop/cop/performance/delete_prefix.rb | 88 +++++++++++++ lib/rubocop/cop/performance/delete_suffix.rb | 88 +++++++++++++ lib/rubocop/cop/performance_cops.rb | 2 + manual/cops.md | 2 + manual/cops_performance.md | 56 ++++++++ .../cop/performance/delete_prefix_spec.rb | 123 ++++++++++++++++++ .../cop/performance/delete_suffix_spec.rb | 123 ++++++++++++++++++ 9 files changed, 493 insertions(+) create mode 100644 lib/rubocop/cop/performance/delete_prefix.rb create mode 100644 lib/rubocop/cop/performance/delete_suffix.rb create mode 100644 spec/rubocop/cop/performance/delete_prefix_spec.rb create mode 100644 spec/rubocop/cop/performance/delete_suffix_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 469ed4b525..cb56bef399 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### New features * [#77](https://github.com/rubocop-hq/rubocop-performance/issues/77): Add new `Performance/BindCall` cop. ([@koic][]) +* [#105](https://github.com/rubocop-hq/rubocop-performance/pull/105): Add new `Performance/DeletePrefix` and `Performance/DeleteSuffix` cops. ([@koic][]) ### Changes diff --git a/config/default.yml b/config/default.yml index 05f53b29a6..e9c0ec6ef1 100644 --- a/config/default.yml +++ b/config/default.yml @@ -54,6 +54,16 @@ Performance/Count: VersionAdded: '0.31' VersionChanged: '1.5' +Performance/DeletePrefix: + Description: 'Use `delete_prefix` instead of `gsub`.' + Enabled: true + VersionAdded: '1.6' + +Performance/DeleteSuffix: + Description: 'Use `delete_suffix` instead of `gsub`.' + Enabled: true + VersionAdded: '1.6' + Performance/Detect: Description: >- Use `detect` instead of `select.first`, `find_all.first`, diff --git a/lib/rubocop/cop/performance/delete_prefix.rb b/lib/rubocop/cop/performance/delete_prefix.rb new file mode 100644 index 0000000000..c006bddc4b --- /dev/null +++ b/lib/rubocop/cop/performance/delete_prefix.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Performance + # In Ruby 2.5, `String#delete_prefix` has been added. + # + # This cop identifies places where `gsub(/\Aprefix/, '')` + # can be replaced by `delete_prefix('prefix')`. + # + # The `delete_prefix('prefix')` method is faster than + # `gsub(/\Aprefix/, '')`. + # + # @example + # + # # bad + # str.gsub(/\Aprefix/, '') + # str.gsub!(/\Aprefix/, '') + # str.gsub(/^prefix/, '') + # str.gsub!(/^prefix/, '') + # + # # good + # str.delete_prefix('prefix') + # str.delete_prefix!('prefix') + # + class DeletePrefix < Cop + extend TargetRubyVersion + + minimum_target_ruby_version 2.5 + + MSG = 'Use `%s` instead of `%s`.' + + PREFERRED_METHODS = { + gsub: :delete_prefix, + gsub!: :delete_prefix! + }.freeze + + def_node_matcher :gsub_method?, <<~PATTERN + (send $!nil? ${:gsub :gsub!} (regexp (str $#literal_at_start?) (regopt)) (str $_)) + PATTERN + + def on_send(node) + gsub_method?(node) do |_, bad_method, _, replace_string| + return unless replace_string.blank? + + good_method = PREFERRED_METHODS[bad_method] + + message = format(MSG, current: bad_method, prefer: good_method) + + add_offense(node, location: :selector, message: message) + end + end + + def autocorrect(node) + gsub_method?(node) do |receiver, bad_method, regexp_str, _| + lambda do |corrector| + good_method = PREFERRED_METHODS[bad_method] + + regexp_str = if regexp_str.start_with?('\\A') + regexp_str[2..-1] # drop `\A` anchor + else + regexp_str[1..-1] # drop `^` anchor + end + regexp_str = interpret_string_escapes(regexp_str) + string_literal = to_string_literal(regexp_str) + + new_code = "#{receiver.source}.#{good_method}(#{string_literal})" + + corrector.replace(node, new_code) + end + end + end + + private + + def literal_at_start?(regex_str) + # is this regexp 'literal' in the sense of only matching literal + # chars, rather than using metachars like `.` and `*` and so on? + # also, is it anchored at the start of the string? + # (tricky: \s, \d, and so on are metacharacters, but other characters + # escaped with a slash are just literals. LITERAL_REGEX takes all + # that into account.) + regex_str =~ /\A(\\A|\^)(?:#{LITERAL_REGEX})+\z/ + end + end + end + end +end diff --git a/lib/rubocop/cop/performance/delete_suffix.rb b/lib/rubocop/cop/performance/delete_suffix.rb new file mode 100644 index 0000000000..9872f70bf6 --- /dev/null +++ b/lib/rubocop/cop/performance/delete_suffix.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Performance + # In Ruby 2.5, `String#delete_suffix` has been added. + # + # This cop identifies places where `gsub(/suffix\z/, '')` + # can be replaced by `delete_suffix('suffix')`. + # + # The `delete_suffix('suffix')` method is faster than + # `gsub(/suffix\z/, '')`. + # + # @example + # + # # bad + # str.gsub(/suffix\z/, '') + # str.gsub!(/suffix\z/, '') + # str.gsub(/suffix$/, '') + # str.gsub!(/suffix$/, '') + # + # # good + # str.delete_suffix('suffix') + # str.delete_suffix!('suffix') + # + class DeleteSuffix < Cop + extend TargetRubyVersion + + minimum_target_ruby_version 2.5 + + MSG = 'Use `%s` instead of `%s`.' + + PREFERRED_METHODS = { + gsub: :delete_suffix, + gsub!: :delete_suffix! + }.freeze + + def_node_matcher :gsub_method?, <<~PATTERN + (send $!nil? ${:gsub :gsub!} (regexp (str $#literal_at_end?) (regopt)) (str $_)) + PATTERN + + def on_send(node) + gsub_method?(node) do |_, bad_method, _, replace_string| + return unless replace_string.blank? + + good_method = PREFERRED_METHODS[bad_method] + + message = format(MSG, current: bad_method, prefer: good_method) + + add_offense(node, location: :selector, message: message) + end + end + + def autocorrect(node) + gsub_method?(node) do |receiver, bad_method, regexp_str, _| + lambda do |corrector| + good_method = PREFERRED_METHODS[bad_method] + + regexp_str = if regexp_str.end_with?('\\z') + regexp_str.chomp('\z') # drop `\z` anchor + else + regexp_str.chop # drop `$` anchor + end + regexp_str = interpret_string_escapes(regexp_str) + string_literal = to_string_literal(regexp_str) + + new_code = "#{receiver.source}.#{good_method}(#{string_literal})" + + corrector.replace(node, new_code) + end + end + end + + private + + def literal_at_end?(regex_str) + # is this regexp 'literal' in the sense of only matching literal + # chars, rather than using metachars like `.` and `*` and so on? + # also, is it anchored at the start of the string? + # (tricky: \s, \d, and so on are metacharacters, but other characters + # escaped with a slash are just literals. LITERAL_REGEX takes all + # that into account.) + regex_str =~ /\A(?:#{LITERAL_REGEX})+(\\z|\$)\z/ + end + end + end + end +end diff --git a/lib/rubocop/cop/performance_cops.rb b/lib/rubocop/cop/performance_cops.rb index 402850c431..4dfc431e19 100644 --- a/lib/rubocop/cop/performance_cops.rb +++ b/lib/rubocop/cop/performance_cops.rb @@ -6,6 +6,8 @@ require_relative 'performance/casecmp' require_relative 'performance/compare_with_block' require_relative 'performance/count' +require_relative 'performance/delete_prefix' +require_relative 'performance/delete_suffix' require_relative 'performance/detect' require_relative 'performance/double_start_end_with' require_relative 'performance/end_with' diff --git a/manual/cops.md b/manual/cops.md index dab8d0aa10..f5f48bad27 100644 --- a/manual/cops.md +++ b/manual/cops.md @@ -8,6 +8,8 @@ * [Performance/ChainArrayAllocation](cops_performance.md#performancechainarrayallocation) * [Performance/CompareWithBlock](cops_performance.md#performancecomparewithblock) * [Performance/Count](cops_performance.md#performancecount) +* [Performance/DeletePrefix](cops_performance.md#performancedeleteprefix) +* [Performance/DeleteSuffix](cops_performance.md#performancedeletesuffix) * [Performance/Detect](cops_performance.md#performancedetect) * [Performance/DoubleStartEndWith](cops_performance.md#performancedoublestartendwith) * [Performance/EndWith](cops_performance.md#performanceendwith) diff --git a/manual/cops_performance.md b/manual/cops_performance.md index ae4a4e79f0..06f029bf12 100644 --- a/manual/cops_performance.md +++ b/manual/cops_performance.md @@ -249,6 +249,62 @@ Model.select('field AS field_one').count Model.select(:value).count ``` +## Performance/DeletePrefix + +Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged +--- | --- | --- | --- | --- +Enabled | Yes | Yes | 1.6 | - + +In Ruby 2.5, `String#delete_prefix` has been added. + +This cop identifies places where `gsub(/\Aprefix/, '')` +can be replaced by `delete_prefix('prefix')`. + +The `delete_prefix('prefix')` method is faster than +`gsub(/\Aprefix/, '')`. + +### Examples + +```ruby +# bad +str.gsub(/\Aprefix/, '') +str.gsub!(/\Aprefix/, '') +str.gsub(/^prefix/, '') +str.gsub!(/^prefix/, '') + +# good +str.delete_prefix('prefix') +str.delete_prefix!('prefix') +``` + +## Performance/DeleteSuffix + +Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged +--- | --- | --- | --- | --- +Enabled | Yes | Yes | 1.6 | - + +In Ruby 2.5, `String#delete_suffix` has been added. + +This cop identifies places where `gsub(/suffix\z/, '')` +can be replaced by `delete_suffix('suffix')`. + +The `delete_suffix('suffix')` method is faster than +`gsub(/suffix\z/, '')`. + +### Examples + +```ruby +# bad +str.gsub(/suffix\z/, '') +str.gsub!(/suffix\z/, '') +str.gsub(/suffix$/, '') +str.gsub!(/suffix$/, '') + +# good +str.delete_suffix('suffix') +str.delete_suffix!('suffix') +``` + ## Performance/Detect Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged diff --git a/spec/rubocop/cop/performance/delete_prefix_spec.rb b/spec/rubocop/cop/performance/delete_prefix_spec.rb new file mode 100644 index 0000000000..8a3f020c96 --- /dev/null +++ b/spec/rubocop/cop/performance/delete_prefix_spec.rb @@ -0,0 +1,123 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Performance::DeletePrefix, :config do + subject(:cop) { described_class.new(config) } + + context 'TargetRubyVersion <= 2.4', :ruby24 do + it "does not register an offense when using `gsub(/\Aprefix/, '')`" do + expect_no_offenses(<<~RUBY) + str.gsub(/\\Aprefix/, '') + RUBY + end + + it "does not register an offense when using `gsub!(/\Aprefix/, '')`" do + expect_no_offenses(<<~RUBY) + str.gsub!(/\\Aprefix/, '') + RUBY + end + end + + context 'TargetRubyVersion >= 2.5', :ruby25 do + context 'when using `\A` as starting pattern' do + it "registers an offense and corrects when `gsub(/\Aprefix/, '')`" do + expect_offense(<<~RUBY) + str.gsub(/\\Aprefix/, '') + ^^^^ Use `delete_prefix` instead of `gsub`. + RUBY + + expect_correction(<<~RUBY) + str.delete_prefix('prefix') + RUBY + end + + it "registers an offense and corrects when `gsub!(/\Aprefix/, '')`" do + expect_offense(<<~RUBY) + str.gsub!(/\\Aprefix/, '') + ^^^^^ Use `delete_prefix!` instead of `gsub!`. + RUBY + + expect_correction(<<~RUBY) + str.delete_prefix!('prefix') + RUBY + end + end + + context 'when using `^` as starting pattern' do + it 'registers an offense and corrects when using `gsub`' do + expect_offense(<<~RUBY) + str.gsub(/^prefix/, '') + ^^^^ Use `delete_prefix` instead of `gsub`. + RUBY + + expect_correction(<<~RUBY) + str.delete_prefix('prefix') + RUBY + end + + it 'registers an offense and corrects when using `gsub!`' do + expect_offense(<<~RUBY) + str.gsub!(/^prefix/, '') + ^^^^^ Use `delete_prefix!` instead of `gsub!`. + RUBY + + expect_correction(<<~RUBY) + str.delete_prefix!('prefix') + RUBY + end + end + + context 'when using non-starting pattern' do + it 'does not register an offense when using `gsub`' do + expect_no_offenses(<<~RUBY) + str.gsub(/pattern/, '') + RUBY + end + + it 'does not register an offense when using `gsub!`' do + expect_no_offenses(<<~RUBY) + str.gsub!(/pattern/, '') + RUBY + end + end + + context 'with starting pattern `\A` and ending pattern `\z`' do + it 'does not register an offense and corrects when using `gsub`' do + expect_no_offenses(<<~RUBY) + str.gsub(/\\Aprefix\\z/, '') + RUBY + end + + it 'does not register an offense and corrects when using `gsub!`' do + expect_no_offenses(<<~RUBY) + str.gsub!(/\\Aprefix\\z/, '') + RUBY + end + end + + context 'when using a non-blank string as replacement string' do + it 'does not register an offense and corrects when using `gsub`' do + expect_no_offenses(<<~RUBY) + str.gsub(/\\Aprefix/, 'foo') + RUBY + end + + it 'does not register an offense and corrects when using `gsub!`' do + expect_no_offenses(<<~RUBY) + str.gsub!(/\\Aprefix/, 'foo') + RUBY + end + end + + it 'does not register an offense when using `delete_prefix`' do + expect_no_offenses(<<~RUBY) + str.delete_prefix('prefix') + RUBY + end + + it 'does not register an offense when using `delete_prefix!`' do + expect_no_offenses(<<~RUBY) + str.delete_prefix!('prefix') + RUBY + end + end +end diff --git a/spec/rubocop/cop/performance/delete_suffix_spec.rb b/spec/rubocop/cop/performance/delete_suffix_spec.rb new file mode 100644 index 0000000000..5eee67f59e --- /dev/null +++ b/spec/rubocop/cop/performance/delete_suffix_spec.rb @@ -0,0 +1,123 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Performance::DeleteSuffix, :config do + subject(:cop) { described_class.new(config) } + + context 'TargetRubyVersion <= 2.4', :ruby24 do + it "does not register an offense when using `gsub(/suffix\z/, '')`" do + expect_no_offenses(<<~RUBY) + str.gsub(/suffix\\z/, '') + RUBY + end + + it "does not register an offense when using `gsub!(/suffix\z/, '')`" do + expect_no_offenses(<<~RUBY) + str.gsub!(/suffix\\z/, '') + RUBY + end + end + + context 'TargetRubyVersion >= 2.5', :ruby25 do + context 'when using `\z` as ending pattern' do + it "registers an offense and corrects when `gsub(/suffix\z/, '')`" do + expect_offense(<<~RUBY) + str.gsub(/suffix\\z/, '') + ^^^^ Use `delete_suffix` instead of `gsub`. + RUBY + + expect_correction(<<~RUBY) + str.delete_suffix('suffix') + RUBY + end + + it "registers an offense and corrects when `gsub!(/suffix\z/, '')`" do + expect_offense(<<~RUBY) + str.gsub!(/suffix\\z/, '') + ^^^^^ Use `delete_suffix!` instead of `gsub!`. + RUBY + + expect_correction(<<~RUBY) + str.delete_suffix!('suffix') + RUBY + end + end + + context 'when using `$` as ending pattern' do + it 'registers an offense and corrects when using `gsub`' do + expect_offense(<<~RUBY) + str.gsub(/suffix$/, '') + ^^^^ Use `delete_suffix` instead of `gsub`. + RUBY + + expect_correction(<<~RUBY) + str.delete_suffix('suffix') + RUBY + end + + it 'registers an offense and corrects when using `gsub!`' do + expect_offense(<<~RUBY) + str.gsub!(/suffix$/, '') + ^^^^^ Use `delete_suffix!` instead of `gsub!`. + RUBY + + expect_correction(<<~RUBY) + str.delete_suffix!('suffix') + RUBY + end + end + + context 'when using non-ending pattern' do + it 'does not register an offense when using `gsub`' do + expect_no_offenses(<<~RUBY) + str.gsub(/pattern/, '') + RUBY + end + + it 'does not register an offense when using `gsub!`' do + expect_no_offenses(<<~RUBY) + str.gsub!(/pattern/, '') + RUBY + end + end + + context 'with starting pattern `\A` and ending pattern `\z`' do + it 'does not register an offense and corrects when using `gsub`' do + expect_no_offenses(<<~RUBY) + str.gsub(/\\Asuffix\\z/, '') + RUBY + end + + it 'does not register an offense and corrects when using `gsub!`' do + expect_no_offenses(<<~RUBY) + str.gsub!(/\\Asuffix\\z/, '') + RUBY + end + end + + context 'when using a non-blank string as replacement string' do + it 'does not register an offense and corrects when using `gsub`' do + expect_no_offenses(<<~RUBY) + str.gsub(/suffix\\z/, 'foo') + RUBY + end + + it 'does not register an offense and corrects when using `gsub!`' do + expect_no_offenses(<<~RUBY) + str.gsub!(/suffix\\z/, 'foo') + RUBY + end + end + + it 'does not register an offense when using `delete_suffix`' do + expect_no_offenses(<<~RUBY) + str.delete_suffix('suffix') + RUBY + end + + it 'does not register an offense when using `delete_suffix!`' do + expect_no_offenses(<<~RUBY) + str.delete_suffix!('suffix') + RUBY + end + end +end