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

Merged
merged 19 commits into from Mar 5, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
98 changes: 51 additions & 47 deletions lib/rules/no-misleading-character-class.js
fasttime marked this conversation as resolved.
Show resolved Hide resolved
Expand Up @@ -8,6 +8,7 @@ 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("char-source");

//------------------------------------------------------------------------------
// Helpers
Expand Down Expand Up @@ -227,59 +228,61 @@ module.exports = {
const parser = new RegExpParser();

/**
* Generates a granular loc for context.report, if directly calculable.
* @param {Character[]} chars Individual characters being reported on.
* Creates a function used to generate locs for a specified node.
* @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
* @returns {(matches: Character[][]) => Location[]} A function to generate locs for the node.
*/
function generateReportLocation(chars, node) {
function createReportLocationGenerator(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;
// Only literals and expression-less templates generate granular errors.
if (!(node.type === "TemplateLiteral" && !node.expressions.length || node.type === "Literal")) {
Copy link
Member

Choose a reason for hiding this comment

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

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")) {

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

return matches => (matches.length ? [node.loc] : []);
}

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];
}
let charInfos = null;

/**
* Generates a granular loc for context.report.
* @param {Character[][]} matches Lists of individual characters being reported on.
* @returns {Location[]} Granular locs for context.report.
* @see https://github.com/eslint/eslint/pull/17515
*/
function generateReportLocation(matches) {
return matches.map(chars => {
const firstIndex = chars[0].start;
const lastIndex = chars.at(-1).end - 1;
let start;
let end;

if (node.type === "TemplateLiteral") {
const source = sourceCode.getText(node);
const offset = node.range[0];

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

charInfos ??= parseStringLiteral(source);
start = offset + charInfos[firstIndex].start;
end = offset + charInfos[lastIndex].end;
} else { // RegExp Literal
const offset = node.range[0] + 1; // Add 1 to skip the leading slash.

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

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

return locs;
return generateReportLocation;
}

/**
Expand Down Expand Up @@ -320,12 +323,13 @@ module.exports = {
} else {
foundKindMatches.set(kind, [...findCharacterSequences[kind](chars)]);
}

}
}
}
});

const generateReportLocation = createReportLocationGenerator(node);
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

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

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

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

for (const loc of locs) {
context.report({
Expand Down
1 change: 1 addition & 0 deletions package.json
Expand Up @@ -72,6 +72,7 @@
"@nodelib/fs.walk": "^1.2.8",
"ajv": "^6.12.4",
"chalk": "^4.0.0",
"char-source": "^0.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 53e262a, thanks!

"cross-spawn": "^7.0.2",
"debug": "^4.3.2",
"escape-string-regexp": "^4.0.0",
Expand Down