feat: `no-misleading-character-class` granular errors #17515

JoshuaKGoldberg · 2023-08-30T17:59:46Z

Prerequisites checklist

I have read the contributing guidelines.

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:

What rule do you want to change?

no-misleading-character-class

What change do you want to make (place an "X" next to just one item)?

[x] Generate more warnings
[ ] Generate fewer warnings
[ ] Implement autofix
[ ] Implement suggestions

How will the change be implemented (place an "X" next to just one item)?

[ ] A new option
[x] A new default behavior
[ ] Other

Please provide some example code that this change will affect:

var r = /[👶🏻]/

What does the rule currently do for this code?

Currently it gives one complaint for the two matched surrogate pairs.

What will the rule do after it's changed?

Give one complaint per surrogate pair.

Latest playground of cases

Fixes #16682.

netlify · 2023-08-30T18:00:04Z

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	`b838339`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/659b10d530f9340008b04a37

lib/rules/no-misleading-character-class.js

tests/lib/rules/no-misleading-character-class.js

lib/rules/no-misleading-character-class.js

Co-authored-by: Francesco Trotta <github@fasttime.org>

fasttime · 2023-09-06T05:35:40Z

Thanks for this PR @JoshuaKGoldberg! Reconsidering the original intent of the rule I'm not sure if we should try to report the exact position of every invalid/misleading character in all possible situations.

The problem is that sometimes there is just no one-to-one correspondence between a positions in the source code and a resulting character in the regex pattern. Even when a correspondence exists, it is not always trivial to calculate just looking at the AST.

The following are examples where the reported start or end positions are off somehow in the new implementation (the patterns should be all equivalent IINM, but it doesn't even matter here):

Playground link

To improve this situation I imagine that we could:

Check if the first argument of RegExp is a simple string, like a string literal or a template literal without interpolations.
If the first argument is not a simple string, do not report granular problems.
If the first argument is a simple string, figure out how to match characters in the regexp pattern to ranges in the source code (not sure what's the best way to do that). If no bijective mapping can be found, do not report granular problems.
When granular problems are reported, use the start and end positions in the source code as determined by the previously generated mapping.

Maybe you have an idea that works better. I would also like to hear what @mdjermanovic thinks.

fasttime · 2023-09-06T05:45:25Z

Also noticed that sometimes the rule suggests to add the u flag where there is already one, for example with new RegExp("[\\ud83d\\u{dc4d}]", "u"); (playground link). This is an existing issue not related to this PR.

mdjermanovic · 2023-09-06T07:59:41Z

Also noticed that sometimes the rule suggests to add the u flag where there is already one, for example with new RegExp("[\\ud83d\\u{dc4d}]", "u"); (playground link). This is an existing issue not related to this PR.

Good catch! Can you open an issue for this bug?

mdjermanovic · 2023-09-06T08:14:12Z

Check if the first argument of RegExp is a simple string, like a string literal or a template literal without interpolations.

If the first argument is not a simple string, do not report granular problems.

If the first argument is a simple string, figure out how to match characters in the regexp pattern to ranges in the source code (not sure what's the best way to do that). If no bijective mapping can be found, do not report granular problems.

When granular problems are reported, use the start and end positions in the source code as determined by the previously generated mapping.

I think that reporting exact positions should be limited to cases where it's a simple string/template literal and the raw text is 1-1 with the pattern. The logic could be the same as in #16544 , where we decided to provide suggestions (which need exact positions to fix the code) only in those cases. Here's related discussion on that PR: #16544 (comment).

fasttime · 2023-09-06T11:54:21Z

Also noticed that sometimes the rule suggests to add the u flag where there is already one, for example with new RegExp("[\\ud83d\\u{dc4d}]", "u"); (playground link). This is an existing issue not related to this PR.

Good catch! Can you open an issue for this bug?

Done: #17537

github-actions · 2023-09-16T22:31:15Z

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.

JoshuaKGoldberg · 2023-09-18T12:47:31Z

Ah, I'd been waiting to see conversation resolve and missed that it had - will pick this back up. Thanks!

nzakas · 2023-09-20T15:22:38Z

@JoshuaKGoldberg just a heads up that there's a merge conflict.

github-actions · 2023-09-30T22:31:27Z

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.

lib/rules/no-misleading-character-class.js

tests/lib/rules/no-misleading-character-class.js

mdjermanovic · 2023-12-15T15:43:56Z

There are merge conflicts in the test file due to 70a686b.

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

github-actions · 2023-12-26T22:31:21Z

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.

nzakas · 2023-12-28T19:16:38Z

Sorry, we are deep in v9 planning and focusing on things that are blockers for v9. We will get back around to this!

In the meantime, can you please rebase to get the latest updates?

nzakas

LGTM. Thanks for all your work on this.

Waiting for @mdjermanovic to verify his review comments.

mdjermanovic · 2024-01-07T20:55:10Z

lib/rules/no-misleading-character-class.js

+                if (kind === "zwj" && locs.length > 1) {
+                    context.report({
+                        loc: {
+                            start: locs[0].start,
+                            end: locs[1].end
+                        },
+                        messageId: kind,
+                        suggest
+                    });


This wouldn't show all if there are more of these in the pattern, but we can fix that in a follow-up.

mdjermanovic · 2024-01-07T20:56:06Z

I'll just close-reopen for a moment to see if there's anything that should be updated for CI to pass.

lib/rules/no-misleading-character-class.js

mdjermanovic

LGTM, thanks!

JoshuaKGoldberg · 2024-01-07T23:20:13Z

Thank you for all the help! I know this was a slog to review on your end, and really appreciate it. And I learned a lot about regular expressions as a result. ❤️

* main: chore: Split Docs CI from core CI (eslint#17897) fix: Ensure config keys are printed for config errors (eslint#17980) chore: delete relative-module-resolver.js (eslint#17981) docs: migration guide entry for `no-inner-declarations` (eslint#17977) docs: Update README feat: maintain latest ecma version in ESLint (eslint#17958) feat!: no-inner-declaration new default behaviour and option (eslint#17885) feat: add `no-useless-assignment` rule (eslint#17625) fix: `no-misleading-character-class` edge cases with granular errors (eslint#17970) feat: `no-misleading-character-class` granular errors (eslint#17515) docs: fix number of code-path events on custom rules page (eslint#17969) docs: reorder entries in v9 migration guide (eslint#17967) fix!: handle `--output-file` for empty output when saving to disk (eslint#17957)

feat: no-misleading-character-class granular errors

25107bc

JoshuaKGoldberg commented Aug 30, 2023

View reviewed changes

lib/rules/no-misleading-character-class.js Show resolved Hide resolved

eslint-github-bot bot added the feature This change adds a new feature to ESLint label Aug 30, 2023

JoshuaKGoldberg marked this pull request as ready for review August 30, 2023 18:22

JoshuaKGoldberg requested a review from a team as a code owner August 30, 2023 18:22

mdjermanovic reviewed Aug 30, 2023

View reviewed changes

tests/lib/rules/no-misleading-character-class.js Outdated Show resolved Hide resolved

tests/lib/rules/no-misleading-character-class.js Outdated Show resolved Hide resolved

mdjermanovic added rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion labels Aug 30, 2023

JoshuaKGoldberg marked this pull request as draft August 31, 2023 14:16

fix: column offsets

abaefa3

JoshuaKGoldberg marked this pull request as ready for review September 5, 2023 02:32

JoshuaKGoldberg requested a review from mdjermanovic September 5, 2023 02:32

fasttime reviewed Sep 5, 2023

View reviewed changes

lib/rules/no-misleading-character-class.js Outdated Show resolved Hide resolved

fasttime reviewed Sep 5, 2023

View reviewed changes

lib/rules/no-misleading-character-class.js Outdated Show resolved Hide resolved

JoshuaKGoldberg and others added 2 commits September 5, 2023 13:46

fix: missing CallExpression

cf27ff2

Apply suggestions from code review

a822015

Co-authored-by: Francesco Trotta <github@fasttime.org>

JoshuaKGoldberg requested a review from fasttime September 5, 2023 21:12

github-actions bot added the Stale label Sep 16, 2023

Rec0iL99 removed the Stale label Sep 18, 2023

github-actions bot added the Stale label Sep 30, 2023

mdjermanovic reviewed Dec 15, 2023

View reviewed changes

lib/rules/no-misleading-character-class.js Outdated Show resolved Hide resolved

tests/lib/rules/no-misleading-character-class.js Outdated Show resolved Hide resolved

JoshuaKGoldberg and others added 7 commits December 15, 2023 17:39

Update lib/rules/no-misleading-character-class.js

ed5d5f2

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Merge branch 'main'

9b2b365

Implement fasttime suggestion for a Map

3244450

nit: for loop

f1480c1

Unify zwj reports

c869b2d

Merge branch 'main'

1441bb9

Added a couple of multiline tests

7d21099

JoshuaKGoldberg requested review from mdjermanovic and fasttime December 16, 2023 04:16

github-actions bot added the Stale label Dec 26, 2023

mdjermanovic removed the Stale label Dec 26, 2023

Merge branch 'main' into no-misleading-character-class-granular-errors

15ebc24

nzakas approved these changes Jan 3, 2024

View reviewed changes

mdjermanovic reviewed Jan 7, 2024

View reviewed changes

mdjermanovic closed this Jan 7, 2024

mdjermanovic reopened this Jan 7, 2024

mdjermanovic reviewed Jan 7, 2024

View reviewed changes

lib/rules/no-misleading-character-class.js Outdated Show resolved Hide resolved

use .at(-1)

b838339

mdjermanovic approved these changes Jan 7, 2024

View reviewed changes

mdjermanovic merged commit 287c4b7 into eslint:main Jan 7, 2024
15 checks passed

JoshuaKGoldberg deleted the no-misleading-character-class-granular-errors branch January 7, 2024 23:18

mdjermanovic mentioned this pull request Jan 7, 2024

fix: no-misleading-character-class edge cases with granular errors #17970

Merged

1 task

fasttime mentioned this pull request Feb 4, 2024

feat: make no-misleading-character-class report more granular errors #18082

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `no-misleading-character-class` granular errors #17515

feat: `no-misleading-character-class` granular errors #17515

JoshuaKGoldberg commented Aug 30, 2023 •

edited

netlify bot commented Aug 30, 2023 •

edited

fasttime commented Sep 6, 2023 •

edited

fasttime commented Sep 6, 2023

mdjermanovic commented Sep 6, 2023

mdjermanovic commented Sep 6, 2023

fasttime commented Sep 6, 2023

github-actions bot commented Sep 16, 2023

JoshuaKGoldberg commented Sep 18, 2023

nzakas commented Sep 20, 2023

github-actions bot commented Sep 30, 2023

mdjermanovic commented Dec 15, 2023

github-actions bot commented Dec 26, 2023

nzakas commented Dec 28, 2023

nzakas left a comment

mdjermanovic Jan 7, 2024

mdjermanovic commented Jan 7, 2024

mdjermanovic left a comment

JoshuaKGoldberg commented Jan 7, 2024

feat: no-misleading-character-class granular errors #17515

feat: no-misleading-character-class granular errors #17515

Conversation

JoshuaKGoldberg commented Aug 30, 2023 • edited

Prerequisites checklist

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

netlify bot commented Aug 30, 2023 • edited

✅ Deploy Preview for docs-eslint canceled.

fasttime commented Sep 6, 2023 • edited

fasttime commented Sep 6, 2023

mdjermanovic commented Sep 6, 2023

mdjermanovic commented Sep 6, 2023

fasttime commented Sep 6, 2023

github-actions bot commented Sep 16, 2023

JoshuaKGoldberg commented Sep 18, 2023

nzakas commented Sep 20, 2023

github-actions bot commented Sep 30, 2023

mdjermanovic commented Dec 15, 2023

github-actions bot commented Dec 26, 2023

nzakas commented Dec 28, 2023

nzakas left a comment

Choose a reason for hiding this comment

mdjermanovic Jan 7, 2024

Choose a reason for hiding this comment

mdjermanovic commented Jan 7, 2024

mdjermanovic left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Jan 7, 2024

feat: `no-misleading-character-class` granular errors #17515

feat: `no-misleading-character-class` granular errors #17515

JoshuaKGoldberg commented Aug 30, 2023 •

edited

netlify bot commented Aug 30, 2023 •

edited

fasttime commented Sep 6, 2023 •

edited