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.

Sign up for GitHub

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: make no-misleading-character-class report more granular errors #18082

Merged
merged 19 commits into from
Mar 5, 2024
Merged

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

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
82d7b3c
feat: report granular errors on arbitrary literals
fasttime Feb 4, 2024
1e8525f
use npm dependency
fasttime Feb 4, 2024
45006ec
test with unescaped CRLF
fasttime Feb 4, 2024
836fe39
inline `createReportLocationGenerator`
fasttime Feb 7, 2024
d8fd769
unit test for templates with expressions
fasttime Feb 7, 2024
d351f21
restore old name `getNodeReportLocations`
fasttime Feb 7, 2024
b2cb30e
update JSDoc
fasttime Feb 7, 2024
90bee47
`charInfos` → `codeUnits`
fasttime Feb 7, 2024
53e262a
extract char-source to a utility module
fasttime Feb 8, 2024
448d9a9
add `read` method to `SourceReader`
fasttime Feb 9, 2024
b1cf05f
add `advance` method and JSDoc
fasttime Feb 15, 2024
7bb7e55
fix logic
fasttime Feb 18, 2024
072f256
`SourceReader` → `TextReader`
fasttime Feb 18, 2024
6792594
handle `RegExp` calls with regex patterns
fasttime Feb 25, 2024
ea6f183
fix for browser test
fasttime Feb 25, 2024
ed8d1cd
fix for Node.js 18
fasttime Feb 27, 2024
7eaf8b4
limit applicability of `getStaticValue` for Node.js 18 compatibility
fasttime Feb 29, 2024
ebe11f8
fix for `RegExp()` without arguments
fasttime Mar 3, 2024
64813fb
update JSDoc for `getStaticValueOrRegex`
fasttime Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
168 changes: 105 additions & 63 deletions lib/rules/no-misleading-character-class.js
View file
Open in desktop
fasttime marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,18 @@
*/
"use strict";

const { CALL, CONSTRUCT, ReferenceTracker, getStringIfConstant } = require("@eslint-community/eslint-utils");
const {
CALL,
CONSTRUCT,
ReferenceTracker,
getStaticValue,
getStringIfConstant
} = require("@eslint-community/eslint-utils");
const { RegExpParser, visitRegExpAST } = require("@eslint-community/regexpp");
const { isCombiningCharacter, isEmojiModifier, isRegionalIndicatorSymbol, isSurrogatePair } = require("./utils/unicode");
const astUtils = require("./utils/ast-utils.js");
const { isValidWithUnicodeFlag } = require("./utils/regular-expressions");
const { parseStringLiteral, parseTemplateToken } = require("./utils/char-source");

//------------------------------------------------------------------------------
// Helpers
Expand Down Expand Up @@ -193,6 +200,28 @@ const findCharacterSequences = {

const kinds = Object.keys(findCharacterSequences);

/**
* Gets the value of the given node if it's a static value other than a regular expression object,
* or the node's `regex` property.
* The purpose of this method is to provide a replacement for `getStaticValue` on Node.js 18, where `getStaticValue` returns `null` if a regular expression literal contains the `v` flag.
* A limitation of this method is that it can only detect a regular expression if the specified node is itself a regular expression literal node.
* @param {ASTNode} node The node to be inspected.
* @param {Scope} [initialScope] Optional scope to start finding variables. If this scope was given, this tries to resolve identifier references which are in the given node as much as possible.
* @returns {{ value: any } | { value: undefined, optional?: true } | { regex: { pattern: string, flags: string } } | null} The static value of the node, or `null`.
Copy link
Member

Choose a reason for hiding this comment

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

In what cases does this function return optional: true property?

Copy link
Member Author

Choose a reason for hiding this comment

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

That could only happen if node were an optional MemberExpression (not the ancestor ChainExpression) in something like nullishValue?.foo. That's not the case for this rule. Shall we remove that typing?

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, it looks like this property was added for internal calculations rather than as something that consumers might make use of, and it isn't documented. Either way, if it can't happen in our case, it might be better to remove that typing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I only noticed that because of the JSDoc in the @return tag. Updated in 64813fb.

*/
function getStaticValueOrRegex(node, initialScope) {
if (node.type === "Literal" && node.regex) {
Copy link
Member

Choose a reason for hiding this comment

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

This would crash if there are no arguments:

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

new RegExp();
TypeError: Cannot read properties of undefined (reading 'type')
Occurred while linting C:\projects\eslint\foo.js:3
Rule: "no-misleading-character-class"
    at getStaticValueOrRegex (C:\projects\eslint\lib\rules\no-misleading-character-class.js:213:14)

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad for not checking! Fixed in ebe11f8.

return { regex: node.regex };
}

const staticValue = getStaticValue(node, initialScope);

if (staticValue?.value instanceof RegExp) {
return null;
}
return staticValue;
}

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -225,62 +254,7 @@ module.exports = {
create(context) {
const sourceCode = context.sourceCode;
const parser = new RegExpParser();

/**
* Generates a granular loc for context.report, if directly calculable.
* @param {Character[]} chars Individual characters being reported on.
* @param {Node} node Parent string node to report within.
* @returns {Object | null} Granular loc for context.report, if directly calculable.
* @see https://github.com/eslint/eslint/pull/17515
*/
function generateReportLocation(chars, node) {

// Limit to to literals and expression-less templates with raw values === their value.
switch (node.type) {
case "TemplateLiteral":
if (node.expressions.length || sourceCode.getText(node).slice(1, -1) !== node.quasis[0].value.cooked) {
return null;
}
break;

case "Literal":
if (typeof node.value === "string" && node.value !== node.raw.slice(1, -1)) {
return null;
}
break;

default:
return null;
}

return {
start: sourceCode.getLocFromIndex(node.range[0] + 1 + chars[0].start),
end: sourceCode.getLocFromIndex(node.range[0] + 1 + chars.at(-1).end)
};
}

/**
* Finds the report loc(s) for a range of matches.
* @param {Character[][]} matches Characters that should trigger a report.
* @param {Node} node The node to report.
* @returns {Object | null} Node loc(s) for context.report.
*/
function getNodeReportLocations(matches, node) {
const locs = [];

for (const chars of matches) {
const loc = generateReportLocation(chars, node);

// If a report can't match to a range, don't report any others
if (!loc) {
return [node.loc];
}

locs.push(loc);
}

return locs;
}
const checkedPatternNodes = new Set();

/**
* Verify a given regular expression.
Expand Down Expand Up @@ -320,12 +294,58 @@ module.exports = {
} else {
foundKindMatches.set(kind, [...findCharacterSequences[kind](chars)]);
}

}
}
}
});

let codeUnits = null;

/**
* Finds the report loc(s) for a range of matches.
* Only literals and expression-less templates generate granular errors.
* @param {Character[][]} matches Lists of individual characters being reported on.
* @returns {Location[]} locs for context.report.
* @see https://github.com/eslint/eslint/pull/17515
*/
function getNodeReportLocations(matches) {
if (!astUtils.isStaticTemplateLiteral(node) && node.type !== "Literal") {
return matches.length ? [node.loc] : [];
}
return matches.map(chars => {
const firstIndex = chars[0].start;
const lastIndex = chars.at(-1).end - 1;
let start;
let end;

if (node.type === "TemplateLiteral") {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct! Those cases should be covered by unit tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 7bb7e55.

const source = sourceCode.getText(node);
const offset = node.range[0];

codeUnits ??= parseTemplateToken(source);
start = offset + codeUnits[firstIndex].start;
end = offset + codeUnits[lastIndex].end;
} else if (typeof node.value === "string") { // String Literal
const source = node.raw;
const offset = node.range[0];

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

Choose a reason for hiding this comment

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

else can be anything, for example:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 7bb7e55.

const offset = node.range[0] + 1; // Add 1 to skip the leading slash.

start = offset + firstIndex;
end = offset + lastIndex + 1;
}

return {
start: sourceCode.getLocFromIndex(start),
end: sourceCode.getLocFromIndex(end)
};
});
}

for (const [kind, matches] of foundKindMatches) {
let suggest;

Expand All @@ -336,7 +356,7 @@ module.exports = {
}];
}

const locs = getNodeReportLocations(matches, node);
const locs = getNodeReportLocations(matches);

for (const loc of locs) {
context.report({
Expand All @@ -351,6 +371,9 @@ module.exports = {

return {
"Literal[regex]"(node) {
if (checkedPatternNodes.has(node)) {
return;
}
verify(node, node.regex.pattern, node.regex.flags, fixer => {
if (!isValidWithUnicodeFlag(context.languageOptions.ecmaVersion, node.regex.pattern)) {
return null;
Expand All @@ -371,12 +394,31 @@ module.exports = {
for (const { node: refNode } of tracker.iterateGlobalReferences({
RegExp: { [CALL]: true, [CONSTRUCT]: true }
})) {
let pattern, flags;
const [patternNode, flagsNode] = refNode.arguments;
const pattern = getStringIfConstant(patternNode, scope);
const flags = getStringIfConstant(flagsNode, scope);
const evaluatedPattern = getStaticValueOrRegex(patternNode, scope);

if (!evaluatedPattern) {
continue;
}
if (flagsNode) {
if (evaluatedPattern.regex) {
pattern = evaluatedPattern.regex.pattern;
checkedPatternNodes.add(patternNode);
} else {
pattern = String(evaluatedPattern.value);
}
flags = getStringIfConstant(flagsNode, scope);
} else {
if (evaluatedPattern.regex) {
continue;
}
pattern = String(evaluatedPattern.value);
flags = "";
}

if (typeof pattern === "string") {
verify(patternNode, pattern, flags || "", fixer => {
if (typeof flags === "string") {
verify(patternNode, pattern, flags, fixer => {

if (!isValidWithUnicodeFlag(context.languageOptions.ecmaVersion, pattern)) {
return null;
Expand Down
Loading