From 403c46efae27a36bb4e9dc7366db8259d6c5cae9 Mon Sep 17 00:00:00 2001
From: Koichi ITO <koic.ito@gmail.com>
Date: Tue, 30 Jun 2020 01:31:33 +0900
Subject: [PATCH] Make a safe detection for `Performance/StringInclude`

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.
---
 lib/rubocop/cop/performance/string_include.rb |  2 +-
 .../cop/performance/string_include_spec.rb    | 29 ++++++++++++-------
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/lib/rubocop/cop/performance/string_include.rb b/lib/rubocop/cop/performance/string_include.rb
index bf610ef9d1..b0be729d48 100644
--- a/lib/rubocop/cop/performance/string_include.rb
+++ b/lib/rubocop/cop/performance/string_include.rb
@@ -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
 
diff --git a/spec/rubocop/cop/performance/string_include_spec.rb b/spec/rubocop/cop/performance/string_include_spec.rb
index 4143fb2c6d..d388b7b5ed 100644
--- a/spec/rubocop/cop/performance/string_include_spec.rb
+++ b/spec/rubocop/cop/performance/string_include_spec.rb
@@ -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"
@@ -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
 
@@ -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
@@ -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
 
@@ -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
 
@@ -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
 
@@ -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