Skip to content
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

bug(linter) incorrect fixer for no-useless-escape #5227

Closed
camc314 opened this issue Aug 26, 2024 · 3 comments · Fixed by #5975
Closed

bug(linter) incorrect fixer for no-useless-escape #5227

camc314 opened this issue Aug 26, 2024 · 3 comments · Fixed by #5975
Assignees
Labels
C-bug Category - Bug good first issue Experience Level - Good for newcomers

Comments

@camc314
Copy link
Collaborator

camc314 commented Aug 26, 2024

const regex = /(https?:\/\/github\.com\/(([^\s]+)\/([^\s]+))\/([^\s]+\/)?(issues|pull)\/([0-9]+))|(([^\s]+)\/([^\s]+))?#([1-9][0-9]*)($|[\s\:\;\-\(\=])/;

is being fixed to

const regex = /(https?:\/\/github\.com\/(([^\s]+)\/([^\s]+))\/([^\s]+\/)?(issues|pull)\/([0-9]+))|(([^\s]+)\/([^\s]+))?#([1-9][0-9]*)($|[\s:;-(=])/;

which is an invalid regex


  × Invalid regular expression: Character class range out of order
   ╭─[./src/vs/workbench/api/test/browser/extHostDocumentData.test.ts:2:142]
 1 │ const regex = /(https?:\/\/github\.com\/(([^\s]+)\/([^\s]+))\/([^\s]+\/)?(issues|pull)\/([0-9]+))|(([^\s]+)\/([^\s]+))?#([1-9][0-9]*)($|[\s:;-(=])/;
   ·                                                                                                                                              ──
   ╰────

Finished in 3ms on 1 file with 93 rules using 12 threads.
Found 0 warnings and 1 error.
@camc314 camc314 added the C-bug Category - Bug label Aug 26, 2024
@camc314 camc314 self-assigned this Aug 26, 2024
@camc314
Copy link
Collaborator Author

camc314 commented Aug 26, 2024

looks like eslint doesn't report an error for this case (escacpe on the \-), but we do.

gonna leave the fixer alone and fix this false positive

oxlint >> https://oxc-project.github.io/oxc/playground/?code=3YCAAICggICAgICAgICxG0qZRraXTnShZDZHIj18ztvE%2FrAEWU6BK3tu8l%2BruCZLm6bBN39t5QCA

eslint >> https://eslint.org/play/#eyJ0ZXh0IjoiY29uc3QgcmVnZXgwID0gL1tcXHNcXDpcXDtcXC1cXChcXD1dLzsiLCJvcHRpb25zIjp7InJ1bGVzIjp7Im5vLXVzZWxlc3MtZXNjYXBlIjpbImVycm9yIl19LCJsYW5ndWFnZU9wdGlvbnMiOnsicGFyc2VyT3B0aW9ucyI6eyJlY21hRmVhdHVyZXMiOnt9fX19fQ==

Screenshot 2024-08-26 at 15 43 11

minimal test case:

/[\s\-(]/;

update: i think it's going to be better if we wait till the linter has access to the pared regex ast and do it that way, else it's just going to be duplicating some of the regex parsing code

@camc314 camc314 removed their assignment Aug 26, 2024
@Boshen
Copy link
Member

Boshen commented Sep 9, 2024

I think it's going to be better if we wait till the linter has access to the pared regex ast and do it that way, else it's just going to be duplicating some of the regex parsing code

We do now 😁

@Boshen Boshen added the good first issue Experience Level - Good for newcomers label Sep 11, 2024
@camchenry camchenry self-assigned this Sep 22, 2024
@DonIsaac DonIsaac linked a pull request Sep 22, 2024 that will close this issue
DonIsaac pushed a commit that referenced this issue Sep 22, 2024
Boshen added a commit that referenced this issue Sep 23, 2024
## [0.9.7] - 2024-09-23

### Features

- d24985e linter: Add `oxc-security/api-keys` (#5906) (DonIsaac)
- f9b44c5 linter: Add unicode sets support to `no-useless-escape` rule
(#5974) (camchenry)
- 0f19848 linter: Implement `no-unexpected-multiline` rule (#5911)
(camchenry)
- 16fe383 linter: Implement `no-extend-native` rule (#5867) (Cam
McHenry)

### Bug Fixes

- eed9ac7 linter: Include actual span size in `no-regex-spaces`
diagnostic (#5957) (camchenry)
- 40c89c2 linter: Move `promise/avoid-new` to style category (#5961)
(DonIsaac)

### Performance

- 608d637 linter: Use `aho-corasick` instead of `regex` for string
matching in `jsx-a11y/img-redundant-alt` (#5892) (camchenry)
- 3148d4b linter: Check file path after checking node kind for
`nextjs/no-head-element` (#5868) (Cam McHenry)

### Refactor

- 0a5a4a9 linter: Use parsed patterns for `unicorn/no-hex-escape`
(#5985) (camchenry)
- 2cf2edd linter: Use parsed patterns in `no-empty-character-class` rule
(#5980) (camchenry)
- a9a8e2a linter: Use regex parser in `eslint/no-regex-spaces` (#5952)
(camchenry)
- 05f592b linter: Use parsed patterns in
`unicorn/prefer-string-starts-ends-with` (#5949) (camchenry)
- 3273b64 linter: Use parsed patterns for
`unicorn/prefer-string-replace-all` rule (#5943) (camchenry)
- ba7b01f linter: Add `LinterBuilder` (#5714) (DonIsaac)
- db4f16a semantic: Call `with_trivias` before `build_with_jsdoc`
(#5875) (Boshen)
- 3d13c6d semantic: Impl `IntoIterator` for `&AstNodes` (#5873)
(DonIsaac)

### Testing

- b681c9a linter: Import test cases for `no-empty-character-class`
(#5981) (camchenry)
- 767602b linter: Add regression test for #5227 (#5975) (camchenry)

---------

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
@camchenry
Copy link
Collaborator

This should be fixed now with #5974 merged and I added a regression test for it here too: #5975

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category - Bug good first issue Experience Level - Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants