Skip to content

Commit

Permalink
Fix: no-regex-spaces false positives and invalid autofix (fixes #12226)
Browse files Browse the repository at this point in the history
  • Loading branch information
@mdjermanovic
mdjermanovic committed Sep 6, 2019
1 parent e10eeba commit f1932e3
Show file tree
Hide file tree
Showing 2 changed files with 264 additions and 47 deletions.
163 changes: 119 additions & 44 deletions lib/rules/no-regex-spaces.js
View file
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);
}
}
});

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
* 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
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"
}
]
}
]
});

0 comments on commit f1932e3

Please sign in to comment.