From f1932e317e7a1b5002f3e96e03b4ad6935c019d9 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Fri, 6 Sep 2019 02:41:16 +0200 Subject: [PATCH 1/4] Fix: no-regex-spaces false positives and invalid autofix (fixes #12226) --- lib/rules/no-regex-spaces.js | 163 +++++++++++++++++++++-------- tests/lib/rules/no-regex-spaces.js | 148 +++++++++++++++++++++++++- 2 files changed, 264 insertions(+), 47 deletions(-) diff --git a/lib/rules/no-regex-spaces.js b/lib/rules/no-regex-spaces.js index 55e79517cef..fa846ddd3f3 100644 --- a/lib/rules/no-regex-spaces.js +++ b/lib/rules/no-regex-spaces.js @@ -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 @@ -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. + */ } /** @@ -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 @@ -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 + ); } } @@ -111,6 +187,5 @@ module.exports = { CallExpression: checkFunction, NewExpression: checkFunction }; - } }; diff --git a/tests/lib/rules/no-regex-spaces.js b/tests/lib/rules/no-regex-spaces.js index febd5e0dbd6..116fc176b3e 100644 --- a/tests/lib/rules/no-regex-spaces.js +++ b/tests/lib/rules/no-regex-spaces.js @@ -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/;", @@ -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" + } + ] } ] }); From 69258515f28b0c8110c3621856b84df7b5dd3851 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Fri, 6 Sep 2019 22:46:56 +0200 Subject: [PATCH 2/4] Remove TODO that was fixed in #7053 --- lib/rules/no-regex-spaces.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/lib/rules/no-regex-spaces.js b/lib/rules/no-regex-spaces.js index fa846ddd3f3..0fb6f0e8266 100644 --- a/lib/rules/no-regex-spaces.js +++ b/lib/rules/no-regex-spaces.js @@ -123,11 +123,6 @@ module.exports = { return; } } - - /* - * TODO: (platinumazure) Fix message to use rule message - * substitution when api.report is fixed in lib/eslint.js. - */ } /** From b8d2f3b73c7ef782a08f2b0c006a45255480d4d9 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Fri, 6 Sep 2019 22:54:31 +0200 Subject: [PATCH 3/4] Add more test cases --- tests/lib/rules/no-regex-spaces.js | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/lib/rules/no-regex-spaces.js b/tests/lib/rules/no-regex-spaces.js index 116fc176b3e..6cdd42cba91 100644 --- a/tests/lib/rules/no-regex-spaces.js +++ b/tests/lib/rules/no-regex-spaces.js @@ -20,6 +20,7 @@ ruleTester.run("no-regex-spaces", rule, { "var foo = RegExp('foo')", "var foo = / /;", "var foo = RegExp(' ')", + "var foo = / a b c d /;", "var foo = /bar {3}baz/g;", "var foo = RegExp('bar {3}baz', 'g')", "var foo = new RegExp('bar {3}baz')", @@ -76,6 +77,26 @@ ruleTester.run("no-regex-spaces", rule, { } ] }, + { + code: "var foo = / a b c d /;", + output: "var foo = / a b {2}c d /;", + errors: [ + { + message: "Spaces are hard to count. Use {2}.", + type: "Literal" + } + ] + }, + { + code: "var foo = RegExp(' a b c d ');", + output: "var foo = RegExp(' a b c d {2}');", + errors: [ + { + message: "Spaces are hard to count. Use {2}.", + type: "CallExpression" + } + ] + }, { code: "var foo = RegExp('bar baz');", output: "var foo = RegExp('bar {4}baz');", From e9a4961d7697c0a770bab6e8acd0b53d62f9196c Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Mon, 16 Sep 2019 18:43:11 +0200 Subject: [PATCH 4/4] Modify algorithm --- lib/rules/no-regex-spaces.js | 51 ++++++++---------- tests/lib/rules/no-regex-spaces.js | 87 ++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 30 deletions(-) diff --git a/lib/rules/no-regex-spaces.js b/lib/rules/no-regex-spaces.js index 0fb6f0e8266..41f5e149df0 100644 --- a/lib/rules/no-regex-spaces.js +++ b/lib/rules/no-regex-spaces.js @@ -17,6 +17,7 @@ const regexpp = require("regexpp"); //------------------------------------------------------------------------------ const regExpParser = new regexpp.RegExpParser(); +const DOUBLE_SPACE = / {2}/u; /** * Check if node is a string @@ -62,59 +63,49 @@ module.exports = { */ function checkRegex(nodeToReport, pattern, rawPattern, rawPatternStartRange, flags) { - // Skip if there are no consecutive spaces in the source code - if (!/ {2}/u.test(rawPattern)) { + // Skip if there are no consecutive spaces in the source code, to avoid reporting e.g., RegExp(' \ '). + if (!DOUBLE_SPACE.test(rawPattern)) { return; } - let ast; + const characterClassNodes = []; + let regExpAST; try { - ast = regExpParser.parsePattern(pattern, 0, pattern.length, flags.includes("u")); + regExpAST = regExpParser.parsePattern(pattern, 0, pattern.length, flags.includes("u")); } catch (e) { // 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); - } + regexpp.visitRegExpAST(regExpAST, { + onCharacterClassEnter(ccNode) { + characterClassNodes.push(ccNode); } }); - for (let i = 0; i < spaces.length - 1; i++) { - let j = i; + const spacesPattern = /( {2,})(?: [+*{?]|[^+*{?]|$)/gu; + let match; - 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; + while ((match = spacesPattern.exec(pattern))) { + const { 1: { length }, index } = match; + // Report only consecutive spaces that are not in character classes. + if ( + characterClassNodes.every(({ start, end }) => index < start || end <= index) + ) { context.report({ node: nodeToReport, - message: "Spaces are hard to count. Use {{{count}}}.", - data: { count }, + message: "Spaces are hard to count. Use {{{length}}}.", + data: { length }, fix(fixer) { if (pattern !== rawPattern) { return null; } return fixer.replaceTextRange( - [rawPatternStartRange + spaces[i + 1].start, rawPatternStartRange + spaces[j].end], - `{${count}}` + [rawPatternStartRange + index, rawPatternStartRange + index + length], + ` {${length}}` ); } }); diff --git a/tests/lib/rules/no-regex-spaces.js b/tests/lib/rules/no-regex-spaces.js index 6cdd42cba91..60f9dccdfcf 100644 --- a/tests/lib/rules/no-regex-spaces.js +++ b/tests/lib/rules/no-regex-spaces.js @@ -30,6 +30,9 @@ ruleTester.run("no-regex-spaces", rule, { "var RegExp = function() {}; var foo = new RegExp('bar baz');", "var RegExp = function() {}; var foo = RegExp('bar baz');", "var foo = / +/;", + "var foo = / ?/;", + "var foo = / */;", + "var foo = / {2}/;", // don't report if there are no consecutive spaces in the source code "var foo = /bar \\ baz/;", @@ -47,9 +50,13 @@ ruleTester.run("no-regex-spaces", rule, { "var foo = /[ ]/;", "var foo = /[ ]/;", "var foo = / [ ] /;", + "var foo = / [ ] [ ] /;", "var foo = new RegExp('[ ]');", "var foo = new RegExp('[ ]');", "var foo = new RegExp(' [ ] ');", + "var foo = RegExp(' [ ] [ ] ');", + "var foo = new RegExp(' \\[ ');", + "var foo = new RegExp(' \\[ \\] ');", // don't report invalid regex "var foo = new RegExp('[ ');", @@ -130,6 +137,16 @@ ruleTester.run("no-regex-spaces", rule, { } ] }, + { + code: "var foo = /bar {3}baz/;", + output: "var foo = /bar {2} {3}baz/;", + errors: [ + { + message: "Spaces are hard to count. Use {2}.", + type: "Literal" + } + ] + }, { code: "var foo = /bar ?baz/;", output: "var foo = /bar {3} ?baz/;", @@ -140,6 +157,26 @@ ruleTester.run("no-regex-spaces", rule, { } ] }, + { + code: "var foo = new RegExp('bar *baz')", + output: "var foo = new RegExp('bar {2} *baz')", + errors: [ + { + message: "Spaces are hard to count. Use {2}.", + type: "NewExpression" + } + ] + }, + { + code: "var foo = RegExp('bar +baz')", + output: "var foo = RegExp('bar {2} +baz')", + errors: [ + { + message: "Spaces are hard to count. Use {2}.", + type: "CallExpression" + } + ] + }, { code: "var foo = new RegExp('bar ');", output: "var foo = new RegExp('bar {4}');", @@ -170,6 +207,16 @@ ruleTester.run("no-regex-spaces", rule, { } ] }, + { + 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}');", @@ -180,6 +227,36 @@ ruleTester.run("no-regex-spaces", rule, { } ] }, + { + code: "var foo = RegExp(' [ ]');", + output: "var foo = RegExp(' {2}[ ]');", + errors: [ + { + message: "Spaces are hard to count. Use {2}.", + 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" + } + ] + }, { code: "var foo = /(?: )/;", output: "var foo = /(?: {2})/;", @@ -253,6 +330,16 @@ ruleTester.run("no-regex-spaces", rule, { type: "CallExpression" } ] + }, + { + code: "var foo = new RegExp('\\\\[ \\\\]');", + output: null, + errors: [ + { + message: "Spaces are hard to count. Use {2}.", + type: "NewExpression" + } + ] } ] });