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

Jump to bottom

Fix: no-regex-spaces false positives and invalid autofix (fixes #12226) #12231

Merged
merged 4 commits into from Sep 29, 2019
Merged

Fix: no-regex-spaces false positives and invalid autofix (fixes #12226) #12231

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f1932e3
Fix: no-regex-spaces false positives and invalid autofix (fixes #12226)
mdjermanovic Sep 6, 2019
6925851
Remove TODO that was fixed in #7053
mdjermanovic Sep 6, 2019
b8d2f3b
Add more test cases
mdjermanovic Sep 6, 2019
e9a4961
Modify algorithm
mdjermanovic Sep 16, 2019
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
163 changes: 119 additions & 44 deletions lib/rules/no-regex-spaces.js
View file
Open in desktop
Expand Up @@ -5,7 +5,28 @@

"use strict";

//------------------------------------------------------------------------------
// Requirements
//------------------------------------------------------------------------------

const astUtils = require("./utils/ast-utils");
const regexpp = require("regexpp");

//------------------------------------------------------------------------------
// Helpers
//------------------------------------------------------------------------------

const regExpParser = new regexpp.RegExpParser();

/**
* Check if node is a string
* @param {ASTNode} node node to evaluate
* @returns {boolean} True if its a string
* @private
*/
function isString(node) {
return node && node.type === "Literal" && typeof node.value === "string";
}

//------------------------------------------------------------------------------
// Rule Definition
Expand All @@ -27,41 +48,86 @@ module.exports = {
},

create(context) {
const sourceCode = context.getSourceCode();

/**
* Validate regular expressions
* @param {ASTNode} node node to validate
* @param {string} value regular expression to validate
* @param {number} valueStart The start location of the regex/string literal. It will always be the case that
* `sourceCode.getText().slice(valueStart, valueStart + value.length) === value`
* Validate regular expression
*
* @param {ASTNode} nodeToReport Node to report.
* @param {string} pattern Regular expression pattern to validate.
* @param {string} rawPattern Raw representation of the pattern in the source code.
* @param {number} rawPatternStartRange Start range of the pattern in the source code.
* @param {string} flags Regular expression flags.
* @returns {void}
* @private
*/
function checkRegex(node, value, valueStart) {
const multipleSpacesRegex = /( {2,})( [+*{?]|[^+*{?]|$)/u,
regexResults = multipleSpacesRegex.exec(value);
function checkRegex(nodeToReport, pattern, rawPattern, rawPatternStartRange, flags) {

if (regexResults !== null) {
const count = regexResults[1].length;
// Skip if there are no consecutive spaces in the source code
if (!/ {2}/u.test(rawPattern)) {
return;
}

context.report({
node,
message: "Spaces are hard to count. Use {{{count}}}.",
data: { count },
fix(fixer) {
return fixer.replaceTextRange(
[valueStart + regexResults.index, valueStart + regexResults.index + count],
` {${count}}`
);
}
});
let ast;

try {
ast = regExpParser.parsePattern(pattern, 0, pattern.length, flags.includes("u"));
} catch (e) {

/*
* TODO: (platinumazure) Fix message to use rule message
* substitution when api.report is fixed in lib/eslint.js.
*/
// Ignore regular expressions with syntax errors
return;
}

const spaces = [];

regexpp.visitRegExpAST(ast, {
onCharacterEnter(character) {
if (
character.parent.type === "Alternative" &&
(character.raw === " " || character.raw === "\\ ")
) {
spaces.push(character);
mysticatea marked this conversation as resolved.
Show resolved Hide resolved
}
}
});

for (let i = 0; i < spaces.length - 1; i++) {
let j = i;

while (
j < spaces.length - 1 &&
spaces[j].end === spaces[j + 1].start &&
spaces[j + 1].raw === " "
) {
j++;
}

if (j > i) {
const count = j - i + 1;

context.report({
node: nodeToReport,
message: "Spaces are hard to count. Use {{{count}}}.",
data: { count },
fix(fixer) {
if (pattern !== rawPattern) {
return null;
}
return fixer.replaceTextRange(
[rawPatternStartRange + spaces[i + 1].start, rawPatternStartRange + spaces[j].end],
`{${count}}`
);
}
});

// Report only the first occurence of consecutive spaces
return;
}
}

/*
* TODO: (platinumazure) Fix message to use rule message
Copy link
Member

Choose a reason for hiding this comment

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

You are right, this TODO can be removed. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

TODO is removed now :)

* substitution when api.report is fixed in lib/eslint.js.
*/
}

/**
Expand All @@ -71,25 +137,22 @@ module.exports = {
* @private
*/
function checkLiteral(node) {
const token = sourceCode.getFirstToken(node),
nodeType = token.type,
nodeValue = token.value;
if (node.regex) {
const pattern = node.regex.pattern;
const rawPattern = node.raw.slice(1, node.raw.lastIndexOf("/"));
const rawPatternStartRange = node.range[0] + 1;
const flags = node.regex.flags;

if (nodeType === "RegularExpression") {
checkRegex(node, nodeValue, token.range[0]);
checkRegex(
node,
pattern,
rawPattern,
rawPatternStartRange,
flags
);
}
}

/**
* Check if node is a string
* @param {ASTNode} node node to evaluate
* @returns {boolean} True if its a string
* @private
*/
function isString(node) {
return node && node.type === "Literal" && typeof node.value === "string";
}

/**
* Validate strings passed to the RegExp constructor
* @param {ASTNode} node node to validate
Expand All @@ -100,9 +163,22 @@ module.exports = {
const scope = context.getScope();
const regExpVar = astUtils.getVariableByName(scope, "RegExp");
const shadowed = regExpVar && regExpVar.defs.length > 0;
const patternNode = node.arguments[0];
const flagsNode = node.arguments[1];

if (node.callee.type === "Identifier" && node.callee.name === "RegExp" && isString(node.arguments[0]) && !shadowed) {
checkRegex(node, node.arguments[0].value, node.arguments[0].range[0] + 1);
if (node.callee.type === "Identifier" && node.callee.name === "RegExp" && isString(patternNode) && !shadowed) {
const pattern = patternNode.value;
const rawPattern = patternNode.raw.slice(1, -1);
const rawPatternStartRange = patternNode.range[0] + 1;
const flags = isString(flagsNode) ? flagsNode.value : "";

checkRegex(
node,
pattern,
rawPattern,
rawPatternStartRange,
flags
);
}
}

Expand All @@ -111,6 +187,5 @@ module.exports = {
CallExpression: checkFunction,
NewExpression: checkFunction
};

}
};
148 changes: 145 additions & 3 deletions tests/lib/rules/no-regex-spaces.js
View file
Open in desktop
Expand Up @@ -16,18 +16,56 @@ const ruleTester = new RuleTester();

ruleTester.run("no-regex-spaces", rule, {
valid: [
"var foo = /bar {3}baz/;",
"var foo = RegExp('bar {3}baz')",
"var foo = /foo/;",
"var foo = RegExp('foo')",
"var foo = / /;",
"var foo = RegExp(' ')",
"var foo = /bar {3}baz/g;",
"var foo = RegExp('bar {3}baz', 'g')",
"var foo = new RegExp('bar {3}baz')",
"var foo = /bar\t\t\tbaz/;",
"var foo = RegExp('bar\t\t\tbaz');",
"var foo = new RegExp('bar\t\t\tbaz');",
"var RegExp = function() {}; var foo = new RegExp('bar baz');",
"var RegExp = function() {}; var foo = RegExp('bar baz');",
"var foo = / +/;"
"var foo = / +/;",

// don't report if there are no consecutive spaces in the source code
"var foo = /bar \\ baz/;",
"var foo = /bar\\ \\ baz/;",
"var foo = /bar \\u0020 baz/;",
"var foo = /bar\\u0020\\u0020baz/;",
"var foo = new RegExp('bar \\ baz')",
"var foo = new RegExp('bar\\ \\ baz')",
"var foo = new RegExp('bar \\\\ baz')",
"var foo = new RegExp('bar \\u0020 baz')",
"var foo = new RegExp('bar\\u0020\\u0020baz')",
"var foo = new RegExp('bar \\\\u0020 baz')",

// don't report spaces in character classes
"var foo = /[ ]/;",
"var foo = /[ ]/;",
"var foo = / [ ] /;",
"var foo = new RegExp('[ ]');",
"var foo = new RegExp('[ ]');",
"var foo = new RegExp(' [ ] ');",

// don't report invalid regex
"var foo = new RegExp('[ ');",
"var foo = new RegExp('{ ', 'u');"
],

invalid: [
{
code: "var foo = /bar baz/;",
output: "var foo = /bar {2}baz/;",
errors: [
{
message: "Spaces are hard to count. Use {2}.",
type: "Literal"
}
]
},
{
code: "var foo = /bar baz/;",
output: "var foo = /bar {4}baz/;",
Expand Down Expand Up @@ -90,6 +128,110 @@ ruleTester.run("no-regex-spaces", rule, {
type: "NewExpression"
}
]
},
{
code: "var foo = /bar\\ baz/;",
output: "var foo = /bar\\ {2}baz/;",
errors: [
{
message: "Spaces are hard to count. Use {2}.",
type: "Literal"
}
]
},
{
code: "var foo = /[ ] /;",
output: "var foo = /[ ] {2}/;",
errors: [
{
message: "Spaces are hard to count. Use {2}.",
type: "Literal"
}
]
},
{
code: "var foo = new RegExp('[ ] ');",
output: "var foo = new RegExp('[ ] {2}');",
errors: [
{
message: "Spaces are hard to count. Use {2}.",
type: "NewExpression"
}
]
},
{
code: "var foo = /(?: )/;",
output: "var foo = /(?: {2})/;",
errors: [
{
message: "Spaces are hard to count. Use {2}.",
type: "Literal"
}
]
},
{
code: "var foo = RegExp('^foo(?= )');",
output: "var foo = RegExp('^foo(?= {3})');",
errors: [
{
message: "Spaces are hard to count. Use {3}.",
type: "CallExpression"
}
]
},
{
code: "var foo = /\\ /",
output: "var foo = /\\ {2}/",
errors: [
{
message: "Spaces are hard to count. Use {2}.",
type: "Literal"
}
]
},
{
code: "var foo = / \\ /",
output: "var foo = / \\ {2}/",
errors: [
{
message: "Spaces are hard to count. Use {2}.",
type: "Literal"
}
]
},

// report only the first occurence of consecutive spaces
{
code: "var foo = / foo /;",
output: "var foo = / {2}foo /;",
errors: [
{
message: "Spaces are hard to count. Use {2}.",
type: "Literal"
}
]
},

// don't fix strings with escape sequences
{
code: "var foo = new RegExp('\\\\d ')",
output: null,
errors: [
{
message: "Spaces are hard to count. Use {2}.",
type: "NewExpression"
}
]
},
{
code: "var foo = RegExp('\\u0041 ')",
output: null,
errors: [
{
message: "Spaces are hard to count. Use {3}.",
type: "CallExpression"
}
]
}
]
});