Skip to content

Commit

Permalink
Make a safe detection for Performance/StringInclude
Browse files Browse the repository at this point in the history
This PR makes a safe detection for `Performance/StringInclude` cop
when `match?` argument is a variable.
Symbol object does not have an `include?` method.

The following is an auto-corrected example code.

```console
% ruby -e ':symbol.include?(/pattern/)'
Traceback (most recent call last):
-e:1:in `<main>': undefined method `include?' for :symbol:Symbol (NoMethodError)
````

A variable possible to be a symbol object, so if `match?` argument is
a variable, accept it.

As an unsafe option it might be possible to provide detection when
argument is a variable.
First of all, this PR makes detection safe.

This PR haven't added a changelog entry as it is an unreleased feature.
  • Loading branch information
koic committed Jul 2, 2020
1 parent e8bc8e7 commit 403c46e
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 12 deletions.
2 changes: 1 addition & 1 deletion lib/rubocop/cop/performance/string_include.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class StringInclude < Cop

def_node_matcher :redundant_regex?, <<~PATTERN
{(send $!nil? {:match :=~ :match?} (regexp (str $#literal?) (regopt)))
(send (regexp (str $#literal?) (regopt)) {:match :match?} $_)
(send (regexp (str $#literal?) (regopt)) {:match :match?} $str)
(match-with-lvasgn (regexp (str $#literal?) (regopt)) $_)}
PATTERN

Expand Down
29 changes: 18 additions & 11 deletions spec/rubocop/cop/performance/string_include_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
end

it "autocorrects /abc/#{method} str" do
new_source = autocorrect_source("/abc/#{method} str")
expect(new_source).to eq "str.include?('abc')"
new_source = autocorrect_source("/abc/#{method} 'str'")
expect(new_source).to eq "'str'.include?('abc')"
end

# escapes like "\n"
Expand All @@ -24,8 +24,8 @@
end

it "autocorrects /\\#{str}#{method} str/" do
new_source = autocorrect_source("/\\#{str}/#{method} str")
expect(new_source).to eq %{str.include?("\\#{str}")}
new_source = autocorrect_source("/\\#{str}/#{method} 'str'")
expect(new_source).to eq %{'str'.include?("\\#{str}")}
end
end

Expand All @@ -37,8 +37,8 @@
end

it "autocorrects /\\#{str}/#{method} str" do
new_source = autocorrect_source("/\\#{str}/#{method} str")
expect(new_source).to eq "str.include?('#{str}')"
new_source = autocorrect_source("/\\#{str}/#{method} 'str'")
expect(new_source).to eq "'str'.include?('#{str}')"
end

it "doesn't register an error for str#{method} /prefix#{str}/" do
Expand Down Expand Up @@ -69,8 +69,8 @@
end

it "autocorrects /\\#{str}#{method} str/" do
new_source = autocorrect_source("/\\#{str}/#{method} str")
expect(new_source).to eq "str.include?('#{str}')"
new_source = autocorrect_source("/\\#{str}/#{method} 'str'")
expect(new_source).to eq "'str'.include?('#{str}')"
end
end

Expand All @@ -80,7 +80,7 @@
end

it "formats the error message correctly for /abc/#{method} str" do
inspect_source("/abc/#{method} str")
inspect_source("/abc/#{method} 'str'")
expect(cop.messages).to eq(['Use `String#include?` instead of a regex match with literal-only pattern.'])
end

Expand All @@ -90,8 +90,8 @@
end

it "autocorrects /\\\\/#{method} str" do
new_source = autocorrect_source("/\\\\/#{method} str")
expect(new_source).to eq("str.include?('\\\\')")
new_source = autocorrect_source("/\\\\/#{method} 'str'")
expect(new_source).to eq("'str'.include?('\\\\')")
end
end

Expand All @@ -102,4 +102,11 @@
it 'allows match without a receiver' do
expect_no_offenses('expect(subject.spin).to match(/\A\n/)')
end

# Symbol object does not have `include?` method.
# A variable possible to be a symbol object, so if `match?` argument is
# a variable, accept it.
it 'allows argument of `match?` is not a string literal' do
expect_no_offenses('/ /.match?(content_as_symbol)')
end
end

0 comments on commit 403c46e

Please sign in to comment.