New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use regexp_parser
to improve Style/RedundantRegexp...
cops
#8625
Changes from 2 commits
c474274
4bbf4cb
1fd5bf6
1608eb7
a591d13
7598a99
db7cd65
c1520e9
9eac941
ad3540b
33caf59
477e3c8
6d3c405
3b777ac
cb668c1
3e35f83
7693728
fd8b231
201fc15
df4f6b4
cfec9ed
770b91e
9b3fe89
b25fd8e
cb8a04b
37b2e89
064b245
ee5bcd8
85d7f4b
a73a64f
48ed439
86f6e12
a6b3cd1
9b61289
62007c3
71fe7da
a21a007
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,32 +24,15 @@ module Style | |
# | ||
# @api private | ||
class RedundantRegexpCharacterClass < Base | ||
include MatchRange | ||
include RegexpLiteralHelp | ||
extend AutoCorrector | ||
|
||
REQUIRES_ESCAPE_OUTSIDE_CHAR_CLASS_CHARS = '.*+?{}()|$'.chars.freeze | ||
MSG_REDUNDANT_CHARACTER_CLASS = 'Redundant single-element character class, ' \ | ||
'`%<char_class>s` can be replaced with `%<element>s`.' | ||
|
||
PATTERN = / | ||
( | ||
(?<!\\) # No \-prefix (i.e. not escaped) | ||
\[ # Literal [ | ||
(?!\#\{) # Not (the start of) an interpolation | ||
(?: # Either... | ||
\\[^b] | # Any escaped character except b (which would change behaviour) | ||
[^.*+?{}()|$] | # or one that doesn't require escaping outside the character class | ||
\\[upP]\{[^}]+\} # or a unicode code-point or property | ||
) | ||
(?<!\\) # No \-prefix (i.e. not escaped) | ||
\] # Literal ] | ||
) | ||
/x.freeze | ||
|
||
def on_regexp(node) | ||
each_redundant_character_class(node) do |loc| | ||
next if whitespace_in_free_space_mode?(node, loc) | ||
|
||
add_offense( | ||
loc, message: format( | ||
MSG_REDUNDANT_CHARACTER_CLASS, | ||
|
@@ -65,19 +48,54 @@ def on_regexp(node) | |
private | ||
|
||
def each_redundant_character_class(node) | ||
pattern_source(node).scan(PATTERN) do | ||
yield match_range(node.loc.begin.end, Regexp.last_match) | ||
each_single_element_character_class(node) do |char_class| | ||
next unless redundant_single_element_character_class?(node, char_class) | ||
|
||
yield node.loc.begin.adjust(begin_pos: 1 + char_class.ts, end_pos: char_class.te) | ||
end | ||
end | ||
|
||
def each_single_element_character_class(node) | ||
Regexp::Parser.parse(pattern_source(node)).each_expression do |expr| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately diff --git a/lib/rubocop/cop/style/redundant_regexp_character_class.rb b/lib/rubocop/cop/style/redundant_regexp_character_class.rb
index 214a84743..7a3251994 100644
--- a/lib/rubocop/cop/style/redundant_regexp_character_class.rb
+++ b/lib/rubocop/cop/style/redundant_regexp_character_class.rb
@@ -54,7 +54,8 @@ module RuboCop
end
def each_single_element_character_class(node)
- Regexp::Parser.parse(pattern_source(node)).each_expression do |expr|
+ # Regexp::Parser.parse(pattern_source(node)).each_expression do |expr|
+ node.parsed_tree.each_expression do |expr|
next if expr.type != :set || expr.expressions.size != 1
next if expr.negative? || %i[posixclass set].include?(expr.expressions.first.type) mostly of the form N.B. I introduced There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we could incorporate the "interpolation-blanking" into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea of blanking the interpolations, makes sense and could be quite useful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've had a go at this - please let me know what you think @marcandre! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's awesome. |
||
next if expr.type != :set || expr.expressions.size != 1 | ||
next if expr.negative? || %i[posixclass set].include?(expr.expressions.first.type) | ||
|
||
yield expr | ||
end | ||
rescue Regexp::Scanner::ScannerError | ||
marcandre marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# Handle malformed patterns that are accepted by Ruby but cause the regexp_parser gem to | ||
# error, see https://github.com/rubocop-hq/rubocop/issues/8083 for details | ||
end | ||
|
||
def redundant_single_element_character_class?(node, char_class) | ||
class_elem = char_class.expressions.first.text | ||
|
||
non_redundant = | ||
whitespace_in_free_space_mode?(node, class_elem) || | ||
backslash_b?(class_elem) || | ||
requires_escape_outside_char_class?(class_elem) | ||
|
||
!non_redundant | ||
end | ||
|
||
def without_character_class(loc) | ||
loc.source[1..-2] | ||
end | ||
|
||
def whitespace_in_free_space_mode?(node, loc) | ||
def whitespace_in_free_space_mode?(node, elem) | ||
return false unless freespace_mode_regexp?(node) | ||
|
||
/\[\s\]/.match?(loc.source) | ||
/\s/.match?(elem) | ||
end | ||
|
||
def backslash_b?(elem) | ||
# \b's behaviour is different inside and outside of a character class, matching word | ||
# boundaries outside but backspace (0x08) when inside. | ||
elem == '\b' | ||
end | ||
|
||
def requires_escape_outside_char_class?(elem) | ||
REQUIRES_ESCAPE_OUTSIDE_CHAR_CLASS_CHARS.include?(elem) | ||
end | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll have to move this under
master
.