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

feat: add option allowEscape to no-misleading-character-class rule #18208

Merged
merged 5 commits into from
May 10, 2024

Conversation

fasttime
Copy link
Member

@fasttime fasttime commented Mar 15, 2024

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Fixes #15080

What changes did you make? (Give an overview)

This PR adds an option to allow escape sequences in regexp character classes to the no-misleading-character-class rule. The new option only has effect on nodes whose source location can be statically determined with our tools. These are the same nodes that report granular errors:

  • regular expression literals
  • string literals as first arguments of the RegExp constructor
  • template literals without expressions as first arguments of the RegExp constructor

Is there anything you'd like reviewers to focus on?

I would like to fine tune this rule to allow only escape sequence that are not confusing, and I'm not sure I've got the right balance. For example /[\👍]/ contains an escape sequence where the backslash just escapes the next character without changing its value. I think that those escape sequences should be still forbidden even when the new option is set. Other cases are less trivial.

In the current implementation, the new option will allow any escape sequences except those where the character being escaped is prepended by one or more backslashes. If one or more of the characters in a group are escaped, the group will be allowed.

Node Status
/[A\u0301]/ allowed, second character in class is escaped
/[👨\u200d👩\u200d👦]/u allowed, ZWNJ character is escaped
/[\n̅]/ allowed, \n is a non-trivial escape sequence
/[\e̅]/ not allowed, \e is e

@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Mar 15, 2024
@github-actions github-actions bot added the rule Relates to ESLint's core rules label Mar 15, 2024
Copy link

netlify bot commented Mar 15, 2024

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 3403f4b
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/662149cd2ef5560008aedd91
😎 Deploy Preview https://deploy-preview-18208--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@fasttime fasttime marked this pull request as ready for review March 17, 2024 23:10
@fasttime fasttime requested a review from a team as a code owner March 17, 2024 23:10
@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Mar 22, 2024
@mdjermanovic
Copy link
Member

/[\n̅]/ allowed, \n is a non-trivial escape sequence
/[\e̅]/ not allowed, \e is e

These two look like the same kind of possible error this rule targets - the sequence appears as a single character - so perhaps both should not be allowed?

@fasttime
Copy link
Member Author

/[\n̅]/ allowed, \n is a non-trivial escape sequence
/[\e̅]/ not allowed, \e is e

These two look like the same kind of possible error this rule targets - the sequence appears as a single character - so perhaps both should not be allowed?

Maybe we could require that both characters in are escaped? I'm not sure, because the problem here seems to lie in the unescaped combining character itself rather than the grouping of characters, and the rule does not flag those characters everywhere. This means that character classes like /[̳]/ or /[\n\u0305̳abc]/ will remain allowed despite looking confusing.

Copy link

github-actions bot commented Apr 4, 2024

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Apr 4, 2024
@fasttime
Copy link
Member Author

fasttime commented Apr 5, 2024

Perhaps we could allow only sequences that consist exclusively of ASCII characters. So /[\n\u0305]/ would be allowed, but /[\n̅]/ or /[\n\̅]/ would be not. What do you think @mdjermanovic?

@fasttime fasttime removed the Stale label Apr 5, 2024
Copy link

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Apr 15, 2024
@fasttime
Copy link
Member Author

I think we can require that combining mark characters in a character class are only allowed when written as escape sequences; which is to say regardless of the previous character. This would be the most immediate solution to always disallow /[\n̅]/ and /[\u006e̅]/ along with /[\e̅]/. I'll work on an update.

@fasttime fasttime marked this pull request as draft April 18, 2024 08:01
@github-actions github-actions bot removed the Stale label Apr 18, 2024
@fasttime fasttime marked this pull request as ready for review April 22, 2024 06:05
Copy link

github-actions bot commented May 2, 2024

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label May 2, 2024
@nzakas
Copy link
Member

nzakas commented May 6, 2024

@mdjermanovic looking for your feedback here.

@github-actions github-actions bot removed the Stale label May 6, 2024
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Great work, thanks!

Since this is a new feature, it needs another approval from a team member before merging.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Nov 7, 2024
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Nov 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Bug: no-misleading-character-class triggers on seemingly-OK code
3 participants