Skip to content

Commit

Permalink
[Fix #8098]: Fix false-positive in Style/RedundantRegexpCharacterClass
Browse files Browse the repository at this point in the history
Also correct the handling of interpolations and comments - we need to
preserve their width in the checked pattern source without their
contents triggering the cop in question, so that we can correctly
highlight following offenses based on the offset in the pattern_source.
  • Loading branch information
owst authored and bbatsov committed Jun 12, 2020
1 parent 018a042 commit 5de9118
Show file tree
Hide file tree
Showing 6 changed files with 121 additions and 24 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -13,6 +13,7 @@
* [#8115](https://github.com/rubocop-hq/rubocop/issues/8115): Fix false negative for `Lint::FormatParameterMismatch` when argument contains formatting. ([@andrykonchin][])
* [#8131](https://github.com/rubocop-hq/rubocop/pull/8131): Fix false positive for `Style/RedundantRegexpEscape` with escaped delimiters. ([@owst][])
* [#8124](https://github.com/rubocop-hq/rubocop/issues/8124): Fix a false positive for `Lint/FormatParameterMismatch` when using named parameters with escaped `%`. ([@koic][])
* [#8098](https://github.com/rubocop-hq/rubocop/issues/8098): Fix a false positive for `Style/RedundantRegexpCharacterClass` when using interpolations. ([@owst][])

## 0.85.1 (2020-06-07)

Expand Down
27 changes: 27 additions & 0 deletions lib/rubocop/cop/mixin/regexp_literal_help.rb
Expand Up @@ -11,6 +11,33 @@ def freespace_mode_regexp?(node)

regopt.children.include?(:x)
end

def pattern_source(node)
freespace_mode = freespace_mode_regexp?(node)

node.children.reject(&:regopt_type?).map do |child|
source_with_comments_and_interpolations_blanked(child, freespace_mode)
end.join
end

def source_with_comments_and_interpolations_blanked(child, freespace_mode)
source = child.source

# We don't want to consider the contents of interpolations or free-space mode comments as
# part of the pattern source, but need to preserve their width, to allow offsets to
# correctly line up with the original source: spaces have no effect, and preserve width.
if child.begin_type?
replace_match_with_spaces(source, /.*/) # replace all content
elsif freespace_mode
replace_match_with_spaces(source, /(?<!\\)#.*/) # replace any comments
else
source
end
end

def replace_match_with_spaces(source, pattern)
source.sub(pattern) { ' ' * Regexp.last_match[0].length }
end
end
end
end
4 changes: 2 additions & 2 deletions lib/rubocop/cop/style/redundant_regexp_character_class.rb
Expand Up @@ -67,8 +67,8 @@ def autocorrect(node)
end

def each_redundant_character_class(node)
each_match_range(node.loc.expression, PATTERN) do |loc|
yield loc
pattern_source(node).scan(PATTERN) do
yield match_range(node.loc.begin.end, Regexp.last_match)
end
end

Expand Down
15 changes: 0 additions & 15 deletions lib/rubocop/cop/style/redundant_regexp_escape.rb
Expand Up @@ -115,21 +115,6 @@ def escape_range_at_index(node, index)

range_between(start, start + 2)
end

def pattern_source(node)
freespace_mode = freespace_mode_regexp?(node)

node.children.reject(&:regopt_type?).map do |child|
source = child.source

if freespace_mode
# Remove comments to avoid misleading results
source.sub(/(?<!\\)#.*/, '')
else
source
end
end.join
end
end
end
end
Expand Down
60 changes: 53 additions & 7 deletions spec/rubocop/cop/style/redundant_regexp_character_class_spec.rb
Expand Up @@ -142,6 +142,19 @@
end
end

context 'with an interpolated unnecessary-character-class regexp' do
it 'registers an offense and corrects' do
expect_offense(<<~'RUBY')
foo = /a#{/[b]/}c/
^^^ Redundant single-element character class, `[b]` can be replaced with `b`.
RUBY

expect_correction(<<~'RUBY')
foo = /a#{/b/}c/
RUBY
end
end

context 'with a negated character class with a single element' do
it 'does not register an offense' do
expect_no_offenses('foo = /[^x]/')
Expand Down Expand Up @@ -178,6 +191,12 @@
end
end

context 'with an array index inside an interpolation' do
it 'does not register an offense' do
expect_no_offenses('foo = /a#{b[0]}c/')
end
end

context 'with a character class containing a space' do
context 'when not using free-spaced mode' do
it 'registers an offense and corrects' do
Expand All @@ -192,17 +211,44 @@
end
end

context 'when using free-spaced mode' do
it 'does not register an offense with only /x' do
expect_no_offenses('foo = /[ ]/x')
context 'with an unnecessary-character-class after a comment' do
it 'registers an offense and corrects' do
expect_offense(<<~'RUBY')
foo = /
a # This comment shouldn't affect the position of the offense
[b]
^^^ Redundant single-element character class, `[b]` can be replaced with `b`.
/x
RUBY

expect_correction(<<~'RUBY')
foo = /
a # This comment shouldn't affect the position of the offense
b
/x
RUBY
end
end

it 'does not register an offense with /ix' do
expect_no_offenses('foo = /[ ]/ix')
context 'when using free-spaced mode' do
context 'with a commented single-element character class' do
it 'does not register an offense' do
expect_no_offenses('foo = /foo # [a]/x')
end
end

it 'does not register an offense with /iux' do
expect_no_offenses('foo = /[ ]/iux')
context 'with a single space character class' do
it 'does not register an offense with only /x' do
expect_no_offenses('foo = /[ ]/x')
end

it 'does not register an offense with /ix' do
expect_no_offenses('foo = /[ ]/ix')
end

it 'does not register an offense with /iux' do
expect_no_offenses('foo = /[ ]/iux')
end
end
end
end
Expand Down
38 changes: 38 additions & 0 deletions spec/rubocop/cop/style/redundant_regexp_escape_spec.rb
Expand Up @@ -91,6 +91,25 @@
end
end

context 'with an interpolated unnecessary-escape regexp' do
it 'registers an offense and corrects' do
expect_offense(<<~'RUBY')
foo = /a#{/\-/}c/
^^ Redundant escape inside regexp literal
RUBY

expect_correction(<<~'RUBY')
foo = /a#{/-/}c/
RUBY
end
end

context 'with an escape inside an interpolated string' do
it 'does not register an offense' do
expect_no_offenses('foo = /#{"\""}/')
end
end

context 'with an escaped interpolation outside a character class' do
it 'does not register an offense' do
expect_no_offenses('foo = /\#\{[a-z_]+\}/')
Expand Down Expand Up @@ -361,6 +380,25 @@
RUBY
end
end

context 'with a redundant escape after a line with comment' do
it 'registers an offense and corrects' do
expect_offense(<<~'RUBY')
foo = %r{
foo # this should not affect the position of the escape below
\-
^^ Redundant escape inside regexp literal
}x
RUBY

expect_correction(<<~RUBY)
foo = %r{
foo # this should not affect the position of the escape below
-
}x
RUBY
end
end
end

context 'with a multi-line %r// regexp' do
Expand Down

0 comments on commit 5de9118

Please sign in to comment.