fix: no-warning-comments rule escapes special RegEx characters in terms #16090
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eslint-github-bot
bot
added
the
triage
|
Hi @lachlanhunt!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
✅ Deploy Preview for docs-eslint canceled.
|
lachlanhunt
changed the title
|
Hi @lachlanhunt!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
lachlanhunt
changed the title
mdjermanovic
added
rule
|
Thanks for the PR! I can reproduce this bug in the playground. |
mdjermanovic
reviewed
Jul 3, 2022
This change will basically undo the code change from #10381, which seems fine to me as I don't see what exactly was fixed by #10381. To doublecheck, I tried the version from 640bf07, and when I revert the code changes all tests added in 640bf07 are still passing. |
lachlanhunt
force-pushed
the
no-warning-comments-matching
branch
4 times, most recently
from
d819fa7 to
8f4eb74
Compare
|
I've addressed all comments so far, and rebased and squashed all the commits down to one. |
lachlanhunt
force-pushed
the
no-warning-comments-matching
branch
from
8f4eb74 to
132458e
Compare
lachlanhunt
force-pushed
the
no-warning-comments-matching
branch
from
132458e to
b3de940
Compare
mdjermanovic
reviewed
Jul 5, 2022
No need to squash. It's easier to track changes when new commits are added on top of existing ones, and we'll squash the commits in the end anyway. |
1 task
|
Looks good, but the last commit 2a2569a introduced some changes that cause linting to fail (in particular, parentheses around single params of arrow functions and trailing commas). Can you fix that? You can see the lint errors in the "Files changed" tab https://github.com/eslint/eslint/pull/16090/files or when you run |
oh, oops. I did the edit quickly on a different computer, and forgot to disable |
|
It seems that this change significantly affects the performance of this rule. I enabled the rule with its default settings in our config (added Before this changeAfter this changeAfter this change, the rule takes 10x more time. This is somewhat surprising because for the default @lachlanhunt what do you think about reverting the refactoring part of this change, and just fixing the bug? I believe the bug is in generating this alternative part of the regex, the pattern should be just |
|
I was able to reproduce the performance issue, and verify it is the In the original code, for a simple term like The The performance on my computer with this branch before my latest fix: With my latest fix for this issue, the result is back down to: (This matches what I get when I run the same test on the |
nzakas
approved these changes
Jul 9, 2022
mdjermanovic
requested changes
Jul 9, 2022
lib/rules/no-warning-comments.js
Outdated
Comment on lines
85
to
86
| const prefix = location === "start" ? "^\\s*" : `(?:^|\\b${/^\W/u.test(escaped) ? "|\\W" : ""})`; | ||
| const suffix = `(?:\\b${/\W$/u.test(escaped) ? "|\\W" : ""}|$)`; |
There was a problem hiding this comment.
There will be the same performance issues with terms that end with non-word characters, like:
"no-warning-comments": ["error", { terms: ["foo!", "bar!", "baz!"] }]This takes a lot more time (10-20x) with the new version.
I believe the complexity in the process of generating regular expressions is pretty much the same as it was, but the generated regular expressions are more complex.
For example, instead of a simpler /^\s*foo!/iu, this will generate /^\s*foo!(?:\b|\W|$)/iu, which apparently has a much worse performance (I'm not sure why).
I'd suggest that we just remove the useless (and buggy) + eitherOrWordBoundary + term + wordBoundary part.
|
It looks like |
I'd still rather keep the code from the current In particular, this code looks good so we could keep it for now: eslint/lib/rules/no-warning-comments.js Lines 81 to 94 in ca83178 but this code doesn't look good: eslint/lib/rules/no-warning-comments.js Lines 96 to 120 in ca83178 because it inserts unescaped |
|
Alright, done. Performance is consistently around 12-13ms on my computer now for all kinds of terms. |
If you have a benchmark that shows a big difference in performance between equivalent regexes, it might be worth reporting the problem to https://bugs.chromium.org/p/v8/issues/list. There were some inconsistencies around |
|
I actually tried to write a separate perf test just for that and some other regexes that were performing weirdly slow within eslint, but I couldn't reproduce the performance issues outside of eslint. I just found that if I either removed the I suspect the problem with my simple perf tests were that some compiler optimisations were being applied for them, which for some reason weren't being applied within eslint code. Writing perf tests is hard. |
crapStone
pushed a commit
to Calciumdibromid/CaBr2
that referenced
this pull request
Jul 18, 2022
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.19.0` -> `8.20.0`](https://renovatebot.com/diffs/npm/eslint/8.19.0/8.20.0) | --- ### Release Notes <details> <summary>eslint/eslint</summary> ### [`v8.20.0`](https://github.com/eslint/eslint/releases/tag/v8.20.0) [Compare Source](eslint/eslint@v8.19.0...v8.20.0) #### Features - [`ca83178`](eslint/eslint@ca83178) feat: catch preprocess errors ([#​16105](eslint/eslint#16105)) (JounQin) #### Bug Fixes - [`30be0ed`](eslint/eslint@30be0ed) fix: no-warning-comments rule escapes special RegEx characters in terms ([#​16090](eslint/eslint#16090)) (Lachlan Hunt) - [`bfe5e88`](eslint/eslint@bfe5e88) fix: ignore spacing before `]` and `}` in comma-spacing ([#​16113](eslint/eslint#16113)) (Milos Djermanovic) #### Documentation - [`845c4f4`](eslint/eslint@845c4f4) docs: Add website team details ([#​16115](eslint/eslint#16115)) (Nicholas C. Zakas) - [`5a0dfdb`](eslint/eslint@5a0dfdb) docs: Link to blog post in no-constant-binary-expression ([#​16112](eslint/eslint#16112)) (Jordan Eldredge) - [`bc692a9`](eslint/eslint@bc692a9) docs: remove install command ([#​16084](eslint/eslint#16084)) (Strek) - [`49ca3f0`](eslint/eslint@49ca3f0) docs: don't show toc when content not found ([#​16095](eslint/eslint#16095)) (Amaresh S M) - [`ba19e3f`](eslint/eslint@ba19e3f) docs: enhance 404 page UI ([#​16097](eslint/eslint#16097)) (Amaresh S M) - [`a75d3b4`](eslint/eslint@a75d3b4) docs: remove unused meta.docs.category field in working-with-rules page ([#​16109](eslint/eslint#16109)) (Brandon Scott) - [`cdc0206`](eslint/eslint@cdc0206) docs: add formatters page edit link ([#​16094](eslint/eslint#16094)) (Amaresh S M) - [`4d1ed22`](eslint/eslint@4d1ed22) docs: preselect default theme ([#​16098](eslint/eslint#16098)) (Strek) - [`4b79612`](eslint/eslint@4b79612) docs: add missing correct/incorrect containers ([#​16087](eslint/eslint#16087)) (Milos Djermanovic) - [`09f6acb`](eslint/eslint@09f6acb) docs: fix UI bug on rules index and details pages ([#​16082](eslint/eslint#16082)) (Deepshika S) - [`f5db264`](eslint/eslint@f5db264) docs: remove remaining duplicate rule descriptions ([#​16093](eslint/eslint#16093)) (Milos Djermanovic) - [`32a6b2a`](eslint/eslint@32a6b2a) docs: Add scroll behaviour smooth ([#​16056](eslint/eslint#16056)) (Amaresh S M) #### Chores - [`bbf8df4`](eslint/eslint@bbf8df4) chore: Mark autogenerated release blog post as draft ([#​16130](eslint/eslint#16130)) (Nicholas C. Zakas) - [`eee4306`](eslint/eslint@eee4306) chore: update internal lint dependencies ([#​16088](eslint/eslint#16088)) (Bryan Mishkin) - [`9615a42`](eslint/eslint@9615a42) chore: update formatter examples template to avoid markdown lint error ([#​16085](eslint/eslint#16085)) (Milos Djermanovic) - [`62541ed`](eslint/eslint@62541ed) chore: fix markdown linting error ([#​16083](eslint/eslint#16083)) (唯然) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMTcuMSIsInVwZGF0ZWRJblZlciI6IjMyLjExNy4xIn0=--> Co-authored-by: cabr2-bot <[email protected]> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1466 Reviewed-by: Epsilon_02 <[email protected]> Co-authored-by: Calciumdibromid Bot <[email protected]> Co-committed-by: Calciumdibromid Bot <[email protected]>
eslint-github-bot
bot
added
the
archived due to age
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
Tell us about your environment:
What parser (default,
@babel/eslint-parser,@typescript-eslint/parser, etc.) are you using?Please show your full configuration:
Configuration
What did you do? Please include the actual source code causing the issue.
Regular expression characters in the
termsproperty are inadvertently being treated as actual regular expression symbols when thelocationis"anywhere".What did you expect to happen?
The terms property is intended to be treated as a string literal.
What actually happened? Please include the actual, raw output from ESLint.
This bug appears to have been inadvertently introduced by pull request #10381 where they were attempting to fix a different bug related to location "anywhere", and accidentally allowed the unescaped
termto be injected into the RegExp.What changes did you make? (Give an overview)
Is there anything you'd like reviewers to focus on?
N/A