-
Notifications
You must be signed in to change notification settings - Fork 10
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 prefer-lookaround rule #324
Conversation
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 will do a full review soon. Here are a few minor things until then.
Co-authored-by: Michael Schmidt <mitchi5000.ms@googlemail.com>
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.
Sorry for the wait.
I have a few questions about the general approach you take:
- What does
CharSetTreeRoot.includes
check? I don't understand which property we check here. - Can
CharSetTree
s grow exponentially in size? Let's say I have(?:a|a|a|a){10}
. Will I get 410=1MCharSetTree
instances?
I also have a suggestion for another criterion for lookbehinds:
Let's say we have a capturing group g that is a candidate to become a lookbehind (it's the first capturing group, replace string starting with $1
, etc). Let c be the union of all characters that can be consumed by g. We now go through all single-character elements (no quantifiers, groups, etc) after g. If any of those characters is disjoint with c, then we can replace g with a lookbehind.
Example: "".replace(/(a)Java/g, '$1foo')
(this isn't replaceable with the current implementation.)
Thank you for your review. I found that my method didn't work. |
I changed this PR. Please check it again. |
I fixed the remaining issue. But |
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.
LGTM;
This PR adds
prefer-lookaround
rule.There is a problem that the behavior may be different if you replace it with "lookaround" as you said. To avoid this problem, I implemented to exclude complicated patterns from the check.
This rule is only checked if the pattern consists only of Character, CharacterClass, CharacterSet, CapturingGroup, Group, and Quantifiers max <= 10.It also controls not to report, when the start capturing group, end capturing group, and the patterns between them include those that can match each pattern.
I think the implementation of this rule can have a lot of false negatives, but I think the rule is useful as far as I can think of at this point.
close #183