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
Add Style/RedundantRegexpEscape cop #7908
Changes from 8 commits
cc1b1ce
272bfb9
2f9c0c6
6826162
194a63f
0819f27
4057d37
792f242
65789b1
74c8060
e0f9a6c
53b701f
7b26d2c
8790da0
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 |
---|---|---|
@@ -0,0 +1,118 @@ | ||
# frozen_string_literal: true | ||
|
||
module RuboCop | ||
module Cop | ||
module Style | ||
# This cop checks for redundant escapes inside Regexp literals. | ||
# | ||
# @example | ||
# # bad | ||
# %r{foo\/bar} | ||
# | ||
# # good | ||
# %r{foo/bar} | ||
# | ||
# # good | ||
# /foo\/bar/ | ||
# | ||
# # good | ||
# %r/foo\/bar/ | ||
# | ||
# # bad | ||
# /a\-b/ | ||
# | ||
# # good | ||
# /a-b/ | ||
# | ||
# # bad | ||
# /[\+\-]\d/ | ||
# | ||
# # good | ||
# /[+\-]\d/ | ||
class RedundantRegexpEscape < Cop | ||
include RangeHelp | ||
|
||
MSG_REDUNDANT_ESCAPE = 'Redundant escape inside regexp literal' | ||
|
||
ALLOWED_ALWAYS_ESCAPES = ' []^\\#'.chars.freeze | ||
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'm not sure why the space character is always allowed to escape. Is it when you use the 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. Yes, exactly. I could add another explicit message that said "Unnecessary escape of space, when not using free-spacing (x) mode."? |
||
ALLOWED_WITHIN_CHAR_CLASS_METACHAR_ESCAPES = '-'.chars.freeze | ||
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. You can always put a dash at the beginning or end of a character class and not have to escape it, right? It would require more logic in the cop, though. 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. Yes, that's true - it's similar to how My two reasons for not flagging/removing such escapes is that it would involve more logic as you say, and make the resulting character classes slightly more "fragile" to changes over time - either by adding another character or by reordering the elements of the character class, e.g.:
would all change the meaning of the character class. The refactorings may not be common, but I think I fall down on allowing escapes of |
||
ALLOWED_OUTSIDE_CHAR_CLASS_METACHAR_ESCAPES = '.*+?{}()|$'.chars.freeze | ||
|
||
def on_regexp(node) | ||
each_escape(node) do |char, index, within_character_class| | ||
next if allowed_escape?(node, char, within_character_class) | ||
|
||
add_offense( | ||
node, | ||
location: escape_range_at_index(node, index), | ||
message: MSG_REDUNDANT_ESCAPE | ||
) | ||
end | ||
end | ||
|
||
def autocorrect(node) | ||
lambda do |corrector| | ||
each_escape(node) do |char, index, within_character_class| | ||
next if allowed_escape?(node, char, within_character_class) | ||
|
||
corrector.remove_leading(escape_range_at_index(node, index), 1) | ||
end | ||
end | ||
end | ||
|
||
private | ||
|
||
def slash_literal?(node) | ||
['/', '%r/'].include?(node.loc.begin.source) | ||
end | ||
|
||
def allowed_escape?(node, char, within_character_class) | ||
# Strictly speaking a few single-letter metachars are currently | ||
# unneccessary to "escape", e.g. g, i, E, F, but enumerating them is | ||
# rather difficult, and their behaviour could change over time with | ||
# different versions of Ruby so that e.g. /\g/ != /g/ | ||
return true if /[[:alnum:]]/.match?(char) | ||
return true if ALLOWED_ALWAYS_ESCAPES.include?(char) | ||
|
||
if char == '/' | ||
slash_literal?(node) | ||
elsif within_character_class | ||
ALLOWED_WITHIN_CHAR_CLASS_METACHAR_ESCAPES.include?(char) | ||
else | ||
ALLOWED_OUTSIDE_CHAR_CLASS_METACHAR_ESCAPES.include?(char) | ||
end | ||
end | ||
|
||
def each_escape(node) | ||
pattern_source(node).each_char.with_index.reduce( | ||
[nil, false] | ||
) do |(previous, within_character_class), (current, index)| | ||
if previous == '\\' | ||
yield [current, index - 1, within_character_class] | ||
|
||
[nil, within_character_class] | ||
elsif previous == '[' && current != ':' | ||
[current, true] | ||
elsif previous != ':' && current == ']' | ||
[current, false] | ||
else | ||
[current, within_character_class] | ||
end | ||
end | ||
end | ||
|
||
def escape_range_at_index(node, index) | ||
regexp_begin = node.loc.begin.end_pos | ||
|
||
start = regexp_begin + index | ||
|
||
range_between(start, start + 2) | ||
end | ||
|
||
def pattern_source(node) | ||
node.children.reject(&:regopt_type?).map(&:source).join | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5741,6 +5741,42 @@ question = '"What did you say?"' | |
|
||
* [https://rubystyle.guide#percent-q](https://rubystyle.guide#percent-q) | ||
|
||
## Style/RedundantRegexpEscape | ||
|
||
Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged | ||
--- | --- | --- | --- | --- | ||
Enabled | Yes | Yes | 0.83 | - | ||
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. Looks like the file needs to be generated again, looking at the "version added". 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. Oops, yes, good catch, I missed this after I merged master and bumped the |
||
|
||
This cop checks for redundant escapes inside Regexp literals. | ||
|
||
### Examples | ||
|
||
```ruby | ||
# bad | ||
%r{foo\/bar} | ||
|
||
# good | ||
%r{foo/bar} | ||
|
||
# good | ||
/foo\/bar/ | ||
|
||
# good | ||
%r/foo\/bar/ | ||
|
||
# bad | ||
/a\-b/ | ||
|
||
# good | ||
/a-b/ | ||
|
||
# bad | ||
/[\+\-]\d/ | ||
|
||
# good | ||
/[+\-]\d/ | ||
``` | ||
|
||
## Style/RedundantReturn | ||
|
||
Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged | ||
|
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.
I'm not sure if the next version will be
0.85
, or if it will be1.0
. @bbatsov knows.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.
Seems it will be 0.85. Too much development happening recently. I want to give @marcandre time to finish the things he has in progress.