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

fasttime · 2024-02-04T14:10:51Z

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:

/* eslint no-misleading-character-class: "error" */

RegExp("[ \\ufe0f][ \\ufe0f]");

RegExp(`"[👍]"`);

What does the rule currently do for this code?

Reports one problem per RegExp expression (Playground link).

What will the rule do after it's changed?

A granular problem will be reported for each problematic character class. For the first RegExp, two problems will be reported.

What changes did you make? (Give an overview)

This is an attempt to extend granular error reporting for the no-misleading-character-class rule to string and template literal patterns whose source code doesn't match the string value. This includes literals with escape sequences, line continuations, and unescaped CRLF line breaks in templates.

This change was suggested during the implementation of granular errors, but it was postponed to keep the work manageable: #17515 (comment).

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

The new implementation only reports more problems on code that already has one problem with the current implementation, so I think it doesn't qualify as a breaking change, but I'd like others to confirm.
I'm using a self-written dependency to track characters back to their positions in source code. While I'm happy to continue maintaining that project on my own, I think it would be better to extract the core of the code to ESLint for licensing reasons. Alternatively, the repo and npm package could be transferred to the eslint org or to eslint-community.
The new implementation could be further extended to templates with interpolated expression like new RegExp(`[ \\ufe0f]${0}`), but I haven't explored that yet.

netlify · 2024-02-04T14:10:56Z

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	`64813fb`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/65e5666e9c6e36000853e830

nzakas

I think it's fine to produce more errors here and not mark it as breaking. It seems more like a bug fix to me.

nzakas · 2024-02-06T17:52:32Z

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

- default:
- return null;
+ // Only literals and expression-less templates generate granular errors.
+ if (!(node.type === "TemplateLiteral" && !node.expressions.length || node.type === "Literal")) {


Finding this a bit difficult to understand, maybe this?

Suggested change

if (!(node.type === "TemplateLiteral" && !node.expressions.length || node.type === "Literal")) {

const isTemplateWithoutExpressions = node.type === "TemplateLiteral" && node.expressions.length === 0;

if (!isTemplateWithoutExpressions && node.type !== "Literal")) {

Funny, I was going to suggest if (node.type !== "TemplateLiteral" || (node.type === "Literal" && node.expressions.length)) {. No preference between the two from me. 🙂

Thanks! I realized that that check was always falsy, so I just removed it. It's because strings and templates without expressions always produce granular reports now, and so the default case isn't hit any more. Somehow I missed that while testing.

nzakas · 2024-02-06T18:09:52Z

package.json

@@ -72,6 +72,7 @@
 "@nodelib/fs.walk": "^1.2.8",
 "ajv": "^6.12.4",
 "chalk": "^4.0.0",
+ "char-source": "^0.0.0",


Yeah, since you're the author of this package, is there any reason it's just not included in this PR as a utility?

Note that that package contains logic that isn't needed in ESLint. The whole validation part for one, including 50% of the unit tests, is not necessary because the syntax of the arguments can be assumed to be valid since it was checked by the parser. There's also additional information about used features (for example, to tell if a string literal is valid in strict mode or not) which seems superfluous unless we expect to use it in ESLint somewhere later.

So if we are going to extract a utility from that package we should probably do some refactoring to remove the unnecessary logic.

Yes, let's remove the unnecessary code, of course. As it seems like the package was created just for this purpose (?), I'm assuming that's not an issue.

Done in 53e262a, thanks!

JoshuaKGoldberg

Looks like a great start! 👏 on going through the effort of making the parser. Looks from the files on GitHub like it was a lot of tricky work.

Since you mentioned bringing the code inline, requesting changes on that before doing a deeper review. Also I think I might have missed something with the charInfos caching?

JoshuaKGoldberg · 2024-02-06T18:49:45Z

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

+ }]
+ },
+
+ /* eslint-disable lines-around-comment -- see https://github.com/eslint/eslint/issues/18081 */


[Question] Why not put the comment outside the code? It's not necessary for the test, so I'd think that preferable even ignoring the lines-around-comment shenanigan.

Well, that's not for the comment inside the code, it's this comment that is producing errors now. I though that the comments in the code would make it easier to locate a test in the source code given its title, because the code as it's calculated here doesn't match what is printed in the console.

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

JoshuaKGoldberg · 2024-02-06T19:04:06Z

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

- default:
- return null;
+ // Only literals and expression-less templates generate granular errors.
+ if (!(node.type === "TemplateLiteral" && !node.expressions.length || node.type === "Literal")) {


Funny, I was going to suggest if (node.type !== "TemplateLiteral" || (node.type === "Literal" && node.expressions.length)) {. No preference between the two from me. 🙂

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

nzakas · 2024-02-09T18:03:24Z

lib/rules/utils/char-source.js

+ * @returns {Generator<CodeUnit>} Zero, one or two `CodeUnit` elements.
+ */
+function *mapEscapeSequenceOrLineContinuation(reader) {
+ const start = reader.pos++;


A little unclear here...are you intentionally moving the reader position by one? Or did you intend for const start = reader.pos + 1?

Either way, I think this line needs reworking to make its intention clear.

nzakas · 2024-02-09T18:05:24Z

lib/rules/utils/char-source.js

+/**
+ * An object used to keep track of the position in a source text where the next characters will be read.
+ */
+class SourceReader {


I might rename this as TextSource. SourceReader implies that there's a read() method or something that this object is supposed to be doing, but it's just a data structure.

Thanks for the feedback. I've added a read() method in 448d9a9 to read characters relative to the current position. So it's no longer necessary to store the position in a variable before accessing the source like it was done before. And I'm using += for relative position changes. I think this makes the code a little clearer but if that's not the case we can revert to how it was done previously and just address the suggestions.

I like the read()!

SourceReader is still a little ambiguous IMO. Maybe, CharsReader? CharactersReader? TextReader? Not a blocker from my end.

Thanks! I don't have a preference for the name, so pick one that sounds better to you.

I'd vote for TextReader then, to align with #18082 (comment).

Renamed in 072f256.

nzakas · 2024-02-13T17:49:07Z

lib/rules/utils/char-source.js

@@ -32,6 +32,12 @@ class SourceReader {
 this.source = source;
 this.pos = 0;
 }
+
+ read(offset = 0, length = 1) {


Can you add some JSDoc here?

nzakas · 2024-02-13T17:50:31Z

lib/rules/utils/char-source.js


- reader.pos = posAfterBackslash + octalStr.length;
+ reader.pos += octalStr.length - 1;


Because you refactored to include a read() method in SourceReader, it might make things clearer if you also created a advance() method that moves pos rather than augmenting the value like this. So this code would become:

Suggested change

reader.pos += octalStr.length - 1;

reader.advance(octalStr.length - 1);

good idea 👍🏻

All done in b1cf05f.

nzakas

LGTM. Would like @JoshuaKGoldberg to review his changes before merging.

mdjermanovic · 2024-02-15T20:07:35Z

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

+ codeUnits ??= parseStringLiteral(source);
+ start = offset + codeUnits[firstIndex].start;
+ end = offset + codeUnits[lastIndex].end;
+ } else { // RegExp Literal


else can be anything, for example:

Fixed in 7bb7e55.

mdjermanovic · 2024-02-15T20:48:12Z

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

+ let start;
+ let end;
+
+ if (node.type === "TemplateLiteral") {


We should check if the template literal has expressions. For example, this crashes:

/* eslint no-misleading-character-class: "error" */ new RegExp(`${"[👍]"}`);

TypeError: Cannot read properties of undefined (reading 'start') Occurred while linting C:\projects\eslint\foo.js:3 Rule: "no-misleading-character-class" at C:\projects\eslint\lib\rules\no-misleading-character-class.js:294:64

Correct! Those cases should be covered by unit tests.

Fixed in 7bb7e55.

fasttime · 2024-02-16T12:31:25Z

This could be a bug in the current implementation:

RegExp(/[👍]/u);

Unexpected surrogate pair in character class. Use 'u' flag. (no-misleading-character-class)

The rule complains about a missing 'u' flag which is actually there:

console.log(RegExp(/[👍]/u).flags);
// "u"

feat: report granular errors on arbitrary literals

82d7b3c

eslint-github-bot bot added the feature This change adds a new feature to ESLint label Feb 4, 2024

use npm dependency

1e8525f

fasttime added the rule Relates to ESLint's core rules label Feb 4, 2024

test with unescaped CRLF

45006ec

fasttime marked this pull request as ready for review February 5, 2024 15:51

fasttime requested a review from a team as a code owner February 5, 2024 15:51

fasttime mentioned this pull request Feb 5, 2024

Bug: no-misleading-character-class triggers on seemingly-OK code #15080

Closed

1 task

nzakas requested a review from JoshuaKGoldberg February 5, 2024 18:36

nzakas reviewed Feb 6, 2024

View reviewed changes

JoshuaKGoldberg requested changes Feb 6, 2024

View reviewed changes

fasttime added 6 commits February 7, 2024 12:01

inline createReportLocationGenerator

836fe39

unit test for templates with expressions

d8fd769

restore old name getNodeReportLocations

d351f21

update JSDoc

b2cb30e

charInfos → codeUnits

90bee47

extract char-source to a utility module

53e262a

fasttime requested review from JoshuaKGoldberg and nzakas February 8, 2024 08:46

nzakas requested changes Feb 9, 2024

View reviewed changes

add read method to SourceReader

448d9a9

nzakas reviewed Feb 13, 2024

View reviewed changes

add advance method and JSDoc

b1cf05f

fasttime requested a review from nzakas February 15, 2024 08:28

nzakas approved these changes Feb 15, 2024

View reviewed changes

mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Feb 15, 2024

mdjermanovic reviewed Feb 15, 2024

View reviewed changes

lholmquist mentioned this pull request Jun 8, 2024

[Snyk] Upgrade eslint from 8.57.0 to 9.3.0 nodeshift/kube-service-bindings#165

Closed

RickV85 mentioned this pull request Jun 9, 2024

[Snyk] Upgrade eslint from 8.57.0 to 9.3.0 RickV85/dmbphotography#30

Open

ekmixon mentioned this pull request Jun 9, 2024

[Snyk] Upgrade eslint from 6.8.0 to 9.3.0 ekmixon/aquasecurity-cloudsploit#128

Open

lholmquist mentioned this pull request Jun 10, 2024

[Snyk] Upgrade eslint from 8.57.0 to 9.3.0 nodeshift/kube-service-bindings#166

Closed

RickV85 mentioned this pull request Jun 11, 2024

[Snyk] Upgrade eslint from 8.57.0 to 9.3.0 RickV85/dmbphotography#31

Open

This was referenced Jun 12, 2024

[Snyk] Upgrade eslint from 8.57.0 to 9.3.0 nodeshift/kube-service-bindings#167

Closed

[Snyk] Upgrade eslint from 8.57.0 to 9.3.0 nodeshift/kube-service-bindings#168

Closed

JoKacar mentioned this pull request Jun 13, 2024

[Snyk] Upgrade eslint from 8.56.0 to 9.3.0 JoKacar/configure-aws-credentials#10

Open

lholmquist mentioned this pull request Jun 14, 2024

[Snyk] Upgrade eslint from 8.57.0 to 9.3.0 nodeshift/kube-service-bindings#169

Closed

saidsef mentioned this pull request Jun 14, 2024

[Snyk] Upgrade eslint from 8.57.0 to 9.3.0 saidsef/tracing-node#155

Open

JoKacar mentioned this pull request Jun 15, 2024

[Snyk] Upgrade eslint from 8.57.0 to 9.3.0 JoKacar/pino#9

Open

RickV85 mentioned this pull request Jun 15, 2024

[Snyk] Upgrade eslint from 8.57.0 to 9.3.0 RickV85/dmbphotography#33

Open

This was referenced Jun 15, 2024

[Snyk] Upgrade eslint from 8.57.0 to 9.3.0 JoKacar/codecov-action#7

Merged

[Snyk] Upgrade eslint from 8.29.0 to 9.3.0 JoKacar/bitcoinjs-lib#9

Open

[Snyk] Upgrade eslint from 8.56.0 to 9.3.0 JoKacar/configure-aws-credentials#15

Merged

This was referenced Jun 16, 2024

[Snyk] Upgrade eslint from 8.57.0 to 9.3.0 nodeshift/kube-service-bindings#170

Closed

[Snyk] Upgrade eslint from 8.57.0 to 9.3.0 nodeshift/kube-service-bindings#171

Closed

This was referenced Jun 19, 2024

[Snyk] Upgrade eslint from 8.47.0 to 9.3.0 YoutacRandS-VA/express#3

Open

[Snyk] Upgrade eslint from 7.32.0 to 9.3.0 YoutacRandS-VA/build#1

Open

[Snyk] Upgrade eslint from 8.48.0 to 9.3.0 YoutacRandS-VA/toolkit#5

Open

lholmquist mentioned this pull request Jun 19, 2024

[Snyk] Upgrade eslint from 8.57.0 to 9.3.0 nodeshift/kube-service-bindings#173

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fasttime commented Feb 4, 2024 •

edited

netlify bot commented Feb 4, 2024 •

edited

nzakas left a comment

nzakas Feb 6, 2024

JoshuaKGoldberg Feb 6, 2024

fasttime Feb 7, 2024

nzakas Feb 6, 2024

fasttime Feb 7, 2024

nzakas Feb 7, 2024

fasttime Feb 8, 2024

JoshuaKGoldberg left a comment

JoshuaKGoldberg Feb 6, 2024

fasttime Feb 7, 2024

JoshuaKGoldberg Feb 6, 2024

nzakas Feb 9, 2024

nzakas Feb 9, 2024

fasttime Feb 10, 2024

JoshuaKGoldberg Feb 16, 2024 •

edited

fasttime Feb 17, 2024

JoshuaKGoldberg Feb 17, 2024

fasttime Feb 18, 2024

nzakas Feb 13, 2024

nzakas Feb 13, 2024

fasttime Feb 15, 2024

fasttime Feb 15, 2024

nzakas left a comment

mdjermanovic Feb 15, 2024

fasttime Feb 18, 2024

mdjermanovic Feb 15, 2024

fasttime Feb 16, 2024

fasttime Feb 18, 2024

fasttime commented Feb 16, 2024

	if (!(node.type === "TemplateLiteral" && !node.expressions.length \|\| node.type === "Literal")) {
	const isTemplateWithoutExpressions = node.type === "TemplateLiteral" && node.expressions.length === 0;
	if (!isTemplateWithoutExpressions && node.type !== "Literal")) {


		reader.pos = posAfterBackslash + octalStr.length;
		reader.pos += octalStr.length - 1;

	reader.pos += octalStr.length - 1;
	reader.advance(octalStr.length - 1);

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

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

Conversation

fasttime commented Feb 4, 2024 • edited

Prerequisites checklist

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

What changes did you make? (Give an overview)

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

netlify bot commented Feb 4, 2024 • edited

✅ Deploy Preview for docs-eslint canceled.

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg Feb 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fasttime commented Feb 16, 2024

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

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

fasttime commented Feb 4, 2024 •

edited

netlify bot commented Feb 4, 2024 •

edited

JoshuaKGoldberg Feb 16, 2024 •

edited