From 056a0c8b790f547284ea7c80cb39cb96e79cf505 Mon Sep 17 00:00:00 2001 From: Eugene Kenny Date: Sun, 27 Oct 2019 23:31:25 +0000 Subject: [PATCH] Let Performance/StartWith and Performance/EndWith correct Regexp#match? and Regexp#=~ Support for `String#match?` was added in 3027c87e93b1109f641dfb2055bccab5f9685dee, but not `Regexp#match?`. Calling `=~` on a regex literal generates a specialised AST node, `match-with-lvasgn`, to support assigning named captures as local variables. The pattern would previously never match this usage. --- CHANGELOG.md | 5 ++ lib/rubocop/cop/performance/end_with.rb | 7 +- lib/rubocop/cop/performance/start_with.rb | 7 +- manual/cops_performance.md | 6 ++ spec/rubocop/cop/performance/end_with_spec.rb | 71 ++++++++++++++++--- .../cop/performance/start_with_spec.rb | 56 ++++++++++++--- 6 files changed, 132 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba2e1a63c7..c59936a5d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## master (unreleased) +### Bug fixes + +* [#82](https://github.com/rubocop-hq/rubocop-performance/pull/82): Let `Performance/StartWith` and `Performance/EndWith` correct `Regexp#match?` and `Regexp#=~`. ([@eugeneius][]) + ## 1.5.0 (2019-10-01) ### Bug fixes @@ -63,3 +67,4 @@ [@tejasbubane]: https://github.com/tejasbubane [@rrosenblum]: https://github.com/rrosenblum [@splattael]: https://github.com/splattael +[@eugeneius]: https://github.com/eugeneius diff --git a/lib/rubocop/cop/performance/end_with.rb b/lib/rubocop/cop/performance/end_with.rb index b4d8fa6d04..66f43a6ce6 100644 --- a/lib/rubocop/cop/performance/end_with.rb +++ b/lib/rubocop/cop/performance/end_with.rb @@ -9,8 +9,11 @@ module Performance # @example # # bad # 'abc'.match?(/bc\Z/) + # /bc\Z/.match?('abc') # 'abc' =~ /bc\Z/ + # /bc\Z/ =~ 'abc' # 'abc'.match(/bc\Z/) + # /bc\Z/.match('abc') # # # good # 'abc'.end_with?('bc') @@ -21,7 +24,8 @@ class EndWith < Cop def_node_matcher :redundant_regex?, <<-PATTERN {(send $!nil? {:match :=~ :match?} (regexp (str $#literal_at_end?) (regopt))) - (send (regexp (str $#literal_at_end?) (regopt)) {:match :=~} $_)} + (send (regexp (str $#literal_at_end?) (regopt)) {:match :match?} $_) + (match-with-lvasgn (regexp (str $#literal_at_end?) (regopt)) $_)} PATTERN def literal_at_end?(regex_str) @@ -36,6 +40,7 @@ def on_send(node) add_offense(node) end + alias on_match_with_lvasgn on_send def autocorrect(node) redundant_regex?(node) do |receiver, regex_str| diff --git a/lib/rubocop/cop/performance/start_with.rb b/lib/rubocop/cop/performance/start_with.rb index 50ba3d4844..b4a7cae8a1 100644 --- a/lib/rubocop/cop/performance/start_with.rb +++ b/lib/rubocop/cop/performance/start_with.rb @@ -9,8 +9,11 @@ module Performance # @example # # bad # 'abc'.match?(/\Aab/) + # /\Aab/.match?('abc') # 'abc' =~ /\Aab/ + # /\Aab/ =~ 'abc' # 'abc'.match(/\Aab/) + # /\Aab/.match('abc') # # # good # 'abc'.start_with?('ab') @@ -21,7 +24,8 @@ class StartWith < Cop def_node_matcher :redundant_regex?, <<-PATTERN {(send $!nil? {:match :=~ :match?} (regexp (str $#literal_at_start?) (regopt))) - (send (regexp (str $#literal_at_start?) (regopt)) {:match :=~} $_)} + (send (regexp (str $#literal_at_start?) (regopt)) {:match :match?} $_) + (match-with-lvasgn (regexp (str $#literal_at_start?) (regopt)) $_)} PATTERN def literal_at_start?(regex_str) @@ -39,6 +43,7 @@ def on_send(node) add_offense(node) end + alias on_match_with_lvasgn on_send def autocorrect(node) redundant_regex?(node) do |receiver, regex_str| diff --git a/manual/cops_performance.md b/manual/cops_performance.md index 3670a4c015..a8d5a9ea86 100644 --- a/manual/cops_performance.md +++ b/manual/cops_performance.md @@ -301,8 +301,11 @@ would suffice. ```ruby # bad 'abc'.match?(/bc\Z/) +/bc\Z/.match?('abc') 'abc' =~ /bc\Z/ +/bc\Z/ =~ 'abc' 'abc'.match(/bc\Z/) +/bc\Z/.match('abc') # good 'abc'.end_with?('bc') @@ -752,8 +755,11 @@ This cop identifies unnecessary use of a regex where ```ruby # bad 'abc'.match?(/\Aab/) +/\Aab/.match?('abc') 'abc' =~ /\Aab/ +/\Aab/ =~ 'abc' 'abc'.match(/\Aab/) +/\Aab/.match('abc') # good 'abc'.start_with?('ab') diff --git a/spec/rubocop/cop/performance/end_with_spec.rb b/spec/rubocop/cop/performance/end_with_spec.rb index b2dd45e6f0..06b4c84708 100644 --- a/spec/rubocop/cop/performance/end_with_spec.rb +++ b/spec/rubocop/cop/performance/end_with_spec.rb @@ -4,68 +4,119 @@ subject(:cop) { described_class.new } shared_examples 'different match methods' do |method| - it "autocorrects #{method} /abc\\z/" do + it "autocorrects str#{method} /abc\\z/" do new_source = autocorrect_source("str#{method} /abc\\z/") expect(new_source).to eq "str.end_with?('abc')" end - it "autocorrects #{method} /\\n\\z/" do + it "autocorrects /abc\\z/#{method} str" do + new_source = autocorrect_source("/abc\\z/#{method} str") + expect(new_source).to eq "str.end_with?('abc')" + end + + it "autocorrects str#{method} /\\n\\z/" do new_source = autocorrect_source("str#{method} /\\n\\z/") expect(new_source).to eq 'str.end_with?("\n")' end - it "autocorrects #{method} /\\t\\z/" do + it "autocorrects /\\n\\z/#{method} str" do + new_source = autocorrect_source("/\\n\\z/#{method} str") + expect(new_source).to eq 'str.end_with?("\n")' + end + + it "autocorrects str#{method} /\\t\\z/" do new_source = autocorrect_source("str#{method} /\\t\\z/") expect(new_source).to eq 'str.end_with?("\t")' end + it "autocorrects /\\t\\z/#{method} str" do + new_source = autocorrect_source("/\\t\\z/#{method} str") + expect(new_source).to eq 'str.end_with?("\t")' + end + + # regexp metacharacters %w[. $ ^ |].each do |str| - it "autocorrects #{method} /\\#{str}\\z/" do + it "autocorrects str#{method} /\\#{str}\\z/" do new_source = autocorrect_source("str#{method} /\\#{str}\\z/") expect(new_source).to eq "str.end_with?('#{str}')" end - it "doesn't register an error for #{method} /#{str}\\z/" do + it "autocorrects /\\#{str}\\z/#{method} str" do + new_source = autocorrect_source("/\\#{str}\\z/#{method} str") + expect(new_source).to eq "str.end_with?('#{str}')" + end + + it "doesn't register an error for str#{method} /#{str}\\z/" do expect_no_offenses("str#{method} /#{str}\\z/") end + + it "doesn't register an error for /#{str}\\z/#{method} str" do + expect_no_offenses("/#{str}\\z/#{method} str") + end end # escapes like "\n" # note that "\b" is a literal backspace char in a double-quoted string... # but in a regex, it's an anchor on a word boundary %w[a e f r t v].each do |str| - it "autocorrects #{method} /\\#{str}\\z/" do + it "autocorrects str#{method} /\\#{str}\\z/" do new_source = autocorrect_source("str#{method} /\\#{str}\\z/") expect(new_source).to eq %{str.end_with?("\\#{str}")} end + + it "autocorrects /\\#{str}\\z/#{method} str" do + new_source = autocorrect_source("/\\#{str}\\z/#{method} str") + expect(new_source).to eq %{str.end_with?("\\#{str}")} + end end # character classes, anchors %w[w W s S d D A Z z G b B h H R X S].each do |str| - it "doesn't register an error for #{method} /\\#{str}\\z/" do + it "doesn't register an error for str#{method} /\\#{str}\\z/" do expect_no_offenses("str#{method} /\\#{str}\\z/") end + + it "doesn't register an error for /\\#{str}\\z/#{method} str" do + expect_no_offenses("/\\#{str}\\z/#{method} str") + end end # characters with no special meaning whatsoever %w[i j l m o q y].each do |str| - it "autocorrects #{method} /\\#{str}\\z/" do + it "autocorrects str#{method} /\\#{str}\\z/" do new_source = autocorrect_source("str#{method} /\\#{str}\\z/") expect(new_source).to eq "str.end_with?('#{str}')" end + + it "autocorrects /\\#{str}\\z/#{method} str" do + new_source = autocorrect_source("/\\#{str}\\z/#{method} str") + expect(new_source).to eq "str.end_with?('#{str}')" + end end - it "formats the error message correctly for #{method} /abc\\z/" do + it "formats the error message correctly for str#{method} /abc\\z/" do inspect_source("str#{method} /abc\\z/") expect(cop.messages).to eq(['Use `String#end_with?` instead of a ' \ 'regex match anchored to the end of ' \ 'the string.']) end - it "autocorrects #{method} /\\\\\\z/" do + it "formats the error message correctly for /abc\\z/#{method} str" do + inspect_source("/abc\\z/#{method} str") + expect(cop.messages).to eq(['Use `String#end_with?` instead of a ' \ + 'regex match anchored to the end of ' \ + 'the string.']) + end + + it "autocorrects str#{method} /\\\\\\z/" do new_source = autocorrect_source("str#{method} /\\\\\\z/") expect(new_source).to eq("str.end_with?('\\\\')") end + + it "autocorrects /\\\\\\z/#{method} str" do + new_source = autocorrect_source("/\\\\\\z/#{method} str") + expect(new_source).to eq("str.end_with?('\\\\')") + end end include_examples('different match methods', '.match?') diff --git a/spec/rubocop/cop/performance/start_with_spec.rb b/spec/rubocop/cop/performance/start_with_spec.rb index 8e125b4ff8..dbe909aa18 100644 --- a/spec/rubocop/cop/performance/start_with_spec.rb +++ b/spec/rubocop/cop/performance/start_with_spec.rb @@ -4,59 +4,99 @@ subject(:cop) { described_class.new } shared_examples 'different match methods' do |method| - it "autocorrects #{method} /\\Aabc/" do + it "autocorrects str#{method} /\\Aabc/" do new_source = autocorrect_source("str#{method} /\\Aabc/") expect(new_source).to eq "str.start_with?('abc')" end + it "autocorrects /\\Aabc/#{method} str" do + new_source = autocorrect_source("/\\Aabc/#{method} str") + expect(new_source).to eq "str.start_with?('abc')" + end + # escapes like "\n" # note that "\b" is a literal backspace char in a double-quoted string... # but in a regex, it's an anchor on a word boundary %w[a e f r t v].each do |str| - it "autocorrects #{method} /\\A\\#{str}/" do + it "autocorrects str#{method} /\\A\\#{str}/" do new_source = autocorrect_source("str#{method} /\\A\\#{str}/") expect(new_source).to eq %{str.start_with?("\\#{str}")} end + + it "autocorrects /\\A\\#{str}#{method} str/" do + new_source = autocorrect_source("/\\A\\#{str}/#{method} str") + expect(new_source).to eq %{str.start_with?("\\#{str}")} + end end # regexp metacharacters %w[. * ? $ ^ |].each do |str| - it "autocorrects #{method} /\\A\\#{str}/" do + it "autocorrects str#{method} /\\A\\#{str}/" do new_source = autocorrect_source("str#{method} /\\A\\#{str}/") expect(new_source).to eq "str.start_with?('#{str}')" end - it "doesn't register an error for #{method} /\\A#{str}/" do + it "autocorrects /\\A\\#{str}/#{method} str" do + new_source = autocorrect_source("/\\A\\#{str}/#{method} str") + expect(new_source).to eq "str.start_with?('#{str}')" + end + + it "doesn't register an error for str#{method} /\\A#{str}/" do expect_no_offenses("str#{method} /\\A#{str}/") end + + it "doesn't register an error for /\\A#{str}/#{method} str" do + expect_no_offenses("/\\A#{str}/#{method} str") + end end # character classes, anchors %w[w W s S d D A Z z G b B h H R X S].each do |str| - it "doesn't register an error for #{method} /\\A\\#{str}/" do + it "doesn't register an error for str#{method} /\\A\\#{str}/" do expect_no_offenses("str#{method} /\\A\\#{str}/") end + + it "doesn't register an error for /\\A\\#{str}/#{method} str" do + expect_no_offenses("/\\A\\#{str}/#{method} str") + end end # characters with no special meaning whatsoever %w[i j l m o q y].each do |str| - it "autocorrects #{method} /\\A\\#{str}/" do + it "autocorrects str#{method} /\\A\\#{str}/" do new_source = autocorrect_source("str#{method} /\\A\\#{str}/") expect(new_source).to eq "str.start_with?('#{str}')" end + + it "autocorrects /\\A\\#{str}#{method} str/" do + new_source = autocorrect_source("/\\A\\#{str}/#{method} str") + expect(new_source).to eq "str.start_with?('#{str}')" + end end - it "formats the error message correctly for #{method} /\\Aabc/" do + it "formats the error message correctly for str#{method} /\\Aabc/" do inspect_source("str#{method} /\\Aabc/") expect(cop.messages).to eq(['Use `String#start_with?` instead of a ' \ 'regex match anchored to the beginning of ' \ 'the string.']) end - it "autocorrects #{method} /\\A\\\\/" do + it "formats the error message correctly for /\\Aabc/#{method} str" do + inspect_source("/\\Aabc/#{method} str") + expect(cop.messages).to eq(['Use `String#start_with?` instead of a ' \ + 'regex match anchored to the beginning of ' \ + 'the string.']) + end + + it "autocorrects str#{method} /\\A\\\\/" do new_source = autocorrect_source("str#{method} /\\A\\\\/") expect(new_source).to eq("str.start_with?('\\\\')") end + + it "autocorrects /\\A\\\\/#{method} str" do + new_source = autocorrect_source("/\\A\\\\/#{method} str") + expect(new_source).to eq("str.start_with?('\\\\')") + end end include_examples('different match methods', '.match?')