Merge pull request #119 from koic/fix_false_positive_for_delete_prefi…

…x_and_delete_suffix

[Fix #118] Fix a false positive for `DeletePrefix` and `DeletePrefix` cops

CHANGELOG.md

@@ -9,6 +9,7 @@
 ### Bug fixes
 
 * [#111](https://github.com/rubocop-hq/rubocop-performance/issues/111): Fix an error for `Performance/DeletePrefix` and `Performance/DeleteSuffix` cops when using autocorrection with RuboCop 0.81 or lower. ([@koic][])
+* [#118](https://github.com/rubocop-hq/rubocop-performance/issues/118): Fix a false positive for `Performance/DeletePrefix`, `Performance/DeleteSuffix`, `Performance/StartWith`, and `Performance/EndWith` cops when receiver is multiline string. ([@koic][])
 
 ## 1.6.0 (2020-05-22)
 

config/default.yml

@@ -58,11 +58,13 @@ Performance/Count:
 Performance/DeletePrefix:
   Description: 'Use `delete_prefix` instead of `gsub`.'
   Enabled: true
+  SafeMultiline: true
   VersionAdded: '1.6'
 
 Performance/DeleteSuffix:
   Description: 'Use `delete_suffix` instead of `gsub`.'
   Enabled: true
+  SafeMultiline: true
   VersionAdded: '1.6'
 
 Performance/Detect:
@@ -99,8 +101,9 @@ Performance/EndWith:
   SafeAutoCorrect: false
   AutoCorrect: false
   Enabled: true
+  SafeMultiline: true
   VersionAdded: '0.36'
-  VersionChanged: '0.44'
+  VersionChanged: '1.6'
 
 Performance/FixedSize:
   Description: 'Do not compute the size of statically sized objects except in constants.'
@@ -193,8 +196,9 @@ Performance/StartWith:
   SafeAutoCorrect: false
   AutoCorrect: false
   Enabled: true
+  SafeMultiline: true
   VersionAdded: '0.36'
-  VersionChanged: '0.44'
+  VersionChanged: '1.6'
 
 Performance/StringReplacement:
   Description: >-

docs/modules/ROOT/pages/cops_performance.adoc

@@ -327,8 +327,11 @@ In Ruby 2.5, `String#delete_prefix` has been added.
 This cop identifies places where `gsub(/\Aprefix/, '')` and `sub(/\Aprefix/, '')`
 can be replaced by `delete_prefix('prefix')`.
 
-The `delete_prefix('prefix')` method is faster than
-`gsub(/\Aprefix/, '')`.
+This cop has `SafeMultiline` configuration option that `true` by default because
+`^prefix` is unsafe as it will behave incompatible with `delete_prefix`
+for receiver is multiline string.
+
+The `delete_prefix('prefix')` method is faster than `gsub(/\Aprefix/, '')`.
 
 === Examples
 
@@ -337,19 +340,47 @@ The `delete_prefix('prefix')` method is faster than
 # bad
 str.gsub(/\Aprefix/, '')
 str.gsub!(/\Aprefix/, '')
-str.gsub(/^prefix/, '')
-str.gsub!(/^prefix/, '')
 
 str.sub(/\Aprefix/, '')
 str.sub!(/\Aprefix/, '')
-str.sub(/^prefix/, '')
-str.sub!(/^prefix/, '')
 
 # good
 str.delete_prefix('prefix')
 str.delete_prefix!('prefix')
 ----
 
+==== SafeMultiline: true (default)
+
+[source,ruby]
+----
+# good
+str.gsub(/^prefix/, '')
+str.gsub!(/^prefix/, '')
+str.sub(/^prefix/, '')
+str.sub!(/^prefix/, '')
+----
+
+==== SafeMultiline: false
+
+[source,ruby]
+----
+# bad
+str.gsub(/^prefix/, '')
+str.gsub!(/^prefix/, '')
+str.sub(/^prefix/, '')
+str.sub!(/^prefix/, '')
+----
+
+=== Configurable attributes
+
+|===
+| Name | Default value | Configurable values
+
+| SafeMultiline
+| `true`
+| Boolean
+|===
+
 == Performance/DeleteSuffix
 
 NOTE: Required Ruby version: 2.5
@@ -369,8 +400,11 @@ In Ruby 2.5, `String#delete_suffix` has been added.
 This cop identifies places where `gsub(/suffix\z/, '')` and `sub(/suffix\z/, '')`
 can be replaced by `delete_suffix('suffix')`.
 
-The `delete_suffix('suffix')` method is faster than
-`gsub(/suffix\z/, '')`.
+This cop has `SafeMultiline` configuration option that `true` by default because
+`suffix$` is unsafe as it will behave incompatible with `delete_suffix?`
+for receiver is multiline string.
+
+The `delete_suffix('suffix')` method is faster than `gsub(/suffix\z/, '')`.
 
 === Examples
 
@@ -379,19 +413,47 @@ The `delete_suffix('suffix')` method is faster than
 # bad
 str.gsub(/suffix\z/, '')
 str.gsub!(/suffix\z/, '')
-str.gsub(/suffix$/, '')
-str.gsub!(/suffix$/, '')
 
 str.sub(/suffix\z/, '')
 str.sub!(/suffix\z/, '')
-str.sub(/suffix$/, '')
-str.sub!(/suffix$/, '')
 
 # good
 str.delete_suffix('suffix')
 str.delete_suffix!('suffix')
 ----
 
+==== SafeMultiline: true (default)
+
+[source,ruby]
+----
+# good
+str.gsub(/suffix$/, '')
+str.gsub!(/suffix$/, '')
+str.sub(/suffix$/, '')
+str.sub!(/suffix$/, '')
+----
+
+==== SafeMultiline: false
+
+[source,ruby]
+----
+# bad
+str.gsub(/suffix$/, '')
+str.gsub!(/suffix$/, '')
+str.sub(/suffix$/, '')
+str.sub!(/suffix$/, '')
+----
+
+=== Configurable attributes
+
+|===
+| Name | Default value | Configurable values
+
+| SafeMultiline
+| `true`
+| Boolean
+|===
+
 == Performance/Detect
 
 |===
@@ -482,11 +544,14 @@ str.end_with?(var1, var2)
 | Yes
 | Yes (Unsafe)
 | 0.36
-| 0.44
+| 1.6
 |===
 
-This cop identifies unnecessary use of a regex where `String#end_with?`
-would suffice.
+This cop identifies unnecessary use of a regex where `String#end_with?` would suffice.
+
+This cop has `SafeMultiline` configuration option that `true` by default because
+`end$` is unsafe as it will behave incompatible with `end_with?`
+for receiver is multiline string.
 
 === Examples
 
@@ -500,15 +565,34 @@ would suffice.
 'abc'.match(/bc\Z/)
 /bc\Z/.match('abc')
 
+# good
+'abc'.end_with?('bc')
+----
+
+==== SafeMultiline: true (default)
+
+[source,ruby]
+----
+# good
 'abc'.match?(/bc$/)
 /bc$/.match?('abc')
 'abc' =~ /bc$/
 /bc$/ =~ 'abc'
 'abc'.match(/bc$/)
 /bc$/.match('abc')
+----
 
-# good
-'abc'.end_with?('bc')
+==== SafeMultiline: false
+
+[source,ruby]
+----
+# bad
+'abc'.match?(/bc$/)
+/bc$/.match?('abc')
+'abc' =~ /bc$/
+/bc$/ =~ 'abc'
+'abc'.match(/bc$/)
+/bc$/.match('abc')
 ----
 
 === Configurable attributes
@@ -519,6 +603,10 @@ would suffice.
 | AutoCorrect
 | `false`
 | Boolean
+
+| SafeMultiline
+| `true`
+| Boolean
 |===
 
 === References
@@ -1057,11 +1145,14 @@ have been assigned to an array or a hash.
 | Yes
 | Yes (Unsafe)
 | 0.36
-| 0.44
+| 1.6
 |===
 
-This cop identifies unnecessary use of a regex where
-`String#start_with?` would suffice.
+This cop identifies unnecessary use of a regex where `String#start_with?` would suffice.
+
+This cop has `SafeMultiline` configuration option that `true` by default because
+`^start` is unsafe as it will behave incompatible with `start_with?`
+for receiver is multiline string.
 
 === Examples
 
@@ -1075,15 +1166,34 @@ This cop identifies unnecessary use of a regex where
 'abc'.match(/\Aab/)
 /\Aab/.match('abc')
 
+# good
+'abc'.start_with?('ab')
+----
+
+==== SafeMultiline: true (default)
+
+[source,ruby]
+----
+# good
 'abc'.match?(/^ab/)
 /^ab/.match?('abc')
 'abc' =~ /^ab/
 /^ab/ =~ 'abc'
 'abc'.match(/^ab/)
 /^ab/.match('abc')
+----
 
-# good
-'abc'.start_with?('ab')
+==== SafeMultiline: false
+
+[source,ruby]
+----
+# bad
+'abc'.match?(/^ab/)
+/^ab/.match?('abc')
+'abc' =~ /^ab/
+/^ab/ =~ 'abc'
+'abc'.match(/^ab/)
+/^ab/.match('abc')
 ----
 
 === Configurable attributes
@@ -1094,6 +1204,10 @@ This cop identifies unnecessary use of a regex where
 | AutoCorrect
 | `false`
 | Boolean
+
+| SafeMultiline
+| `true`
+| Boolean
 |===
 
 === References

lib/rubocop/cop/mixin/regexp_metacharacter.rb

@@ -4,21 +4,52 @@ module RuboCop
   module Cop
     # Common functionality for handling regexp metacharacters.
     module RegexpMetacharacter
-      def literal_at_start?(regex_str)
+      private
+
+      def literal_at_start?(regexp)
+        return true if literal_at_start_with_backslash_a?(regexp)
+
+        !safe_multiline? && literal_at_start_with_caret?(regexp)
+      end
+
+      def literal_at_end?(regexp)
+        return true if literal_at_end_with_backslash_z?(regexp)
+
+        !safe_multiline? && literal_at_end_with_dollar?(regexp)
+      end
+
+      def literal_at_start_with_backslash_a?(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.)
+        /\A\\A(?:#{Util::LITERAL_REGEX})+\z/.match?(regex_str)
+      end
+
+      def literal_at_start_with_caret?(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|\^)(?:#{Util::LITERAL_REGEX})+\z/
+        /\A\^(?:#{Util::LITERAL_REGEX})+\z/.match?(regex_str)
       end
 
-      def literal_at_end?(regex_str)
+      def literal_at_end_with_backslash_z?(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 end of the string?
-        regex_str =~ /\A(?:#{Util::LITERAL_REGEX})+(\\z|\$)\z/
+        /\A(?:#{Util::LITERAL_REGEX})+\\z\z/.match?(regex_str)
+      end
+
+      def literal_at_end_with_dollar?(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 end of the string?
+        /\A(?:#{Util::LITERAL_REGEX})+\$\z/.match?(regex_str)
       end
 
       def drop_start_metacharacter(regexp_string)
@@ -36,6 +67,10 @@ def drop_end_metacharacter(regexp_string)
           regexp_string.chop # drop `$` anchor
         end
       end
+
+      def safe_multiline?
+        cop_config.fetch('SafeMultiline', true)
+      end
     end
   end
 end