From 82a205ff0ffdd9b887f5055bb1dff9c88612d438 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Tue, 11 Jun 2019 18:27:42 +0200 Subject: [PATCH 1/3] Fix: prefer-const produces invalid autofix (fixes #11699) --- lib/rules/prefer-const.js | 164 +++++++++----------------------- tests/lib/rules/prefer-const.js | 27 ++++++ 2 files changed, 74 insertions(+), 117 deletions(-) diff --git a/lib/rules/prefer-const.js b/lib/rules/prefer-const.js index 68c07da4ed9..2416247cbe3 100644 --- a/lib/rules/prefer-const.js +++ b/lib/rules/prefer-const.js @@ -306,24 +306,6 @@ function groupByDestructuring(variables, ignoreReadBeforeAssign) { return identifierMap; } -/** - * Finds the nearest parent of node with a given type. - * - * @param {ASTNode} node – The node to search from. - * @param {string} type – The type field of the parent node. - * @param {Function} shouldStop – a predicate that returns true if the traversal should stop, and false otherwise. - * @returns {ASTNode} The closest ancestor with the specified type; null if no such ancestor exists. - */ -function findUp(node, type, shouldStop) { - if (!node || shouldStop(node)) { - return null; - } - if (node.type === type) { - return node; - } - return findUp(node.parent, type, shouldStop); -} - //------------------------------------------------------------------------------ // Rule Definition //------------------------------------------------------------------------------ @@ -361,109 +343,57 @@ module.exports = { const sourceCode = context.getSourceCode(); const shouldMatchAnyDestructuredVariable = options.destructuring !== "all"; const ignoreReadBeforeAssign = options.ignoreReadBeforeAssign === true; - const variables = []; - let reportCount = 0; - let name = ""; - - /** - * Reports given identifier nodes if all of the nodes should be declared - * as const. - * - * The argument 'nodes' is an array of Identifier nodes. - * This node is the result of 'getIdentifierIfShouldBeConst()', so it's - * nullable. In simple declaration or assignment cases, the length of - * the array is 1. In destructuring cases, the length of the array can - * be 2 or more. - * - * @param {(eslint-scope.Reference|null)[]} nodes - - * References which are grouped by destructuring to report. - * @returns {void} - */ - function checkGroup(nodes) { - const nodesToReport = nodes.filter(Boolean); - - if (nodes.length && (shouldMatchAnyDestructuredVariable || nodesToReport.length === nodes.length)) { - const varDeclParent = findUp(nodes[0], "VariableDeclaration", parentNode => parentNode.type.endsWith("Statement")); - const isVarDecParentNull = varDeclParent === null; - - if (!isVarDecParentNull && varDeclParent.declarations.length > 0) { - const firstDeclaration = varDeclParent.declarations[0]; - - if (firstDeclaration.init) { - const firstDecParent = firstDeclaration.init.parent; - - /* - * First we check the declaration type and then depending on - * if the type is a "VariableDeclarator" or its an "ObjectPattern" - * we compare the name from the first identifier, if the names are different - * we assign the new name and reset the count of reportCount and nodeCount in - * order to check each block for the number of reported errors and base our fix - * based on comparing nodes.length and nodesToReport.length. - */ - - if (firstDecParent.type === "VariableDeclarator") { - - if (firstDecParent.id.name !== name) { - name = firstDecParent.id.name; - reportCount = 0; - } - - if (firstDecParent.id.type === "ObjectPattern") { - if (firstDecParent.init.name !== name) { - name = firstDecParent.init.name; - reportCount = 0; - } - } - } - } - } - - let shouldFix = varDeclParent && - - // Don't do a fix unless the variable is initialized (or it's in a for-in or for-of loop) - (varDeclParent.parent.type === "ForInStatement" || varDeclParent.parent.type === "ForOfStatement" || varDeclParent.declarations[0].init) && - - /* - * If options.destructuring is "all", then this warning will not occur unless - * every assignment in the destructuring should be const. In that case, it's safe - * to apply the fix. - */ - nodesToReport.length === nodes.length; - - if (!isVarDecParentNull && varDeclParent.declarations && varDeclParent.declarations.length !== 1) { - - if (varDeclParent && varDeclParent.declarations && varDeclParent.declarations.length >= 1) { - - /* - * Add nodesToReport.length to a count, then comparing the count to the length - * of the declarations in the current block. - */ - - reportCount += nodesToReport.length; - - shouldFix = shouldFix && (reportCount === varDeclParent.declarations.length); - } - } - - nodesToReport.forEach(node => { - context.report({ - node, - messageId: "useConst", - data: node, - fix: shouldFix ? fixer => fixer.replaceText(sourceCode.getFirstToken(varDeclParent), "const") : null - }); - }); - } - } return { - "Program:exit"() { - groupByDestructuring(variables, ignoreReadBeforeAssign).forEach(checkGroup); - }, - VariableDeclaration(node) { if (node.kind === "let" && !isInitOfForStatement(node)) { - variables.push(...context.getDeclaredVariables(node)); + const nodesToReport = []; + let shouldFix = true; + + groupByDestructuring(context.getDeclaredVariables(node), ignoreReadBeforeAssign) + .forEach(identifiers => { + + /* + * 'identifiers' is an array of Identifier nodes from the same group. + * In simple declaration or assignment cases, the length of the array is 1. + * In destructuring cases, the length of the array can be 2 or more. + * + * Each element in this array is the result of 'getIdentifierIfShouldBeConst()', + * so it will be null if the corresponding identifier cannot be const. + * Elements that are not null are the ones that could be const. + */ + const couldBeConstIdentifiers = identifiers.filter(Boolean); + + /* + * If all elements from the same group could be const, report them. + * Otherwise (destructuring where only some could be const), report depending on the configuration. + */ + if (couldBeConstIdentifiers.length && + (couldBeConstIdentifiers.length === identifiers.length || shouldMatchAnyDestructuredVariable)) { + nodesToReport.push(...couldBeConstIdentifiers); + } + + // If at least one identifier cannot be const, the whole declaration cannot be auto-fixed to const + if (couldBeConstIdentifiers.length < identifiers.length) { + shouldFix = false; + } + }); + + // Don't do a fix unless all variables are initialized (or it's in a for-in or for-of loop) + shouldFix = shouldFix && + (node.parent.type === "ForInStatement" || node.parent.type === "ForOfStatement" || + node.declarations.every(declaration => declaration.init)); + + const declarationKindToken = sourceCode.getFirstToken(node); + + nodesToReport.forEach(nodeToReport => { + context.report({ + node: nodeToReport, + messageId: "useConst", + data: nodeToReport, + fix: shouldFix ? fixer => fixer.replaceText(declarationKindToken, "const") : null + }); + }); } } }; diff --git a/tests/lib/rules/prefer-const.js b/tests/lib/rules/prefer-const.js index d8c018bdf3c..1730e662655 100644 --- a/tests/lib/rules/prefer-const.js +++ b/tests/lib/rules/prefer-const.js @@ -508,6 +508,33 @@ ruleTester.run("prefer-const", rule, { { message: "'a' is never reassigned. Use 'const' instead.", type: "Identifier" }, { message: "'b' is never reassigned. Use 'const' instead.", type: "Identifier" } ] + }, + + // https://github.com/eslint/eslint/issues/11699 + { + code: "let {a, b} = c, d;", + output: null, + errors: [ + { messageId: "useConst", data: { name: "a" }, type: "Identifier" }, + { messageId: "useConst", data: { name: "b" }, type: "Identifier" } + ] + }, + { + code: "let {a, b, c} = {}, e, f;", + output: null, + errors: [ + { messageId: "useConst", data: { name: "a" }, type: "Identifier" }, + { messageId: "useConst", data: { name: "b" }, type: "Identifier" }, + { messageId: "useConst", data: { name: "c" }, type: "Identifier" } + ] + }, + { + code: "let {a, b} = {}, c = 0; c = 1;", + output: null, + errors: [ + { messageId: "useConst", data: { name: "a" }, type: "Identifier" }, + { messageId: "useConst", data: { name: "b" }, type: "Identifier" } + ] } ] From 8e241dcee7a70e6abe68356bb6382c65af249f4a Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Wed, 12 Jun 2019 01:41:18 +0200 Subject: [PATCH 2/3] Fixed potential token prefixes and multiple autofixes --- lib/rules/prefer-const.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/rules/prefer-const.js b/lib/rules/prefer-const.js index 2416247cbe3..1cf09754f66 100644 --- a/lib/rules/prefer-const.js +++ b/lib/rules/prefer-const.js @@ -384,7 +384,7 @@ module.exports = { (node.parent.type === "ForInStatement" || node.parent.type === "ForOfStatement" || node.declarations.every(declaration => declaration.init)); - const declarationKindToken = sourceCode.getFirstToken(node); + const declarationKindToken = sourceCode.getFirstToken(node, t => t.value === node.kind); nodesToReport.forEach(nodeToReport => { context.report({ @@ -393,6 +393,9 @@ module.exports = { data: nodeToReport, fix: shouldFix ? fixer => fixer.replaceText(declarationKindToken, "const") : null }); + + // First report will fix the whole declaration + shouldFix = false; }); } } From 52d18f8f7b29ec192ee5da643bbdedf861132af3 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Tue, 25 Jun 2019 16:58:15 +0200 Subject: [PATCH 3/3] Rollback, fix just the Issue and TypeScript --- lib/rules/prefer-const.js | 165 +++++++++++++++++++++++--------- tests/lib/rules/prefer-const.js | 9 -- 2 files changed, 119 insertions(+), 55 deletions(-) diff --git a/lib/rules/prefer-const.js b/lib/rules/prefer-const.js index 1cf09754f66..854da310e4b 100644 --- a/lib/rules/prefer-const.js +++ b/lib/rules/prefer-const.js @@ -306,6 +306,24 @@ function groupByDestructuring(variables, ignoreReadBeforeAssign) { return identifierMap; } +/** + * Finds the nearest parent of node with a given type. + * + * @param {ASTNode} node – The node to search from. + * @param {string} type – The type field of the parent node. + * @param {Function} shouldStop – a predicate that returns true if the traversal should stop, and false otherwise. + * @returns {ASTNode} The closest ancestor with the specified type; null if no such ancestor exists. + */ +function findUp(node, type, shouldStop) { + if (!node || shouldStop(node)) { + return null; + } + if (node.type === type) { + return node; + } + return findUp(node.parent, type, shouldStop); +} + //------------------------------------------------------------------------------ // Rule Definition //------------------------------------------------------------------------------ @@ -343,60 +361,115 @@ module.exports = { const sourceCode = context.getSourceCode(); const shouldMatchAnyDestructuredVariable = options.destructuring !== "all"; const ignoreReadBeforeAssign = options.ignoreReadBeforeAssign === true; - - return { - VariableDeclaration(node) { - if (node.kind === "let" && !isInitOfForStatement(node)) { - const nodesToReport = []; - let shouldFix = true; - - groupByDestructuring(context.getDeclaredVariables(node), ignoreReadBeforeAssign) - .forEach(identifiers => { - - /* - * 'identifiers' is an array of Identifier nodes from the same group. - * In simple declaration or assignment cases, the length of the array is 1. - * In destructuring cases, the length of the array can be 2 or more. - * - * Each element in this array is the result of 'getIdentifierIfShouldBeConst()', - * so it will be null if the corresponding identifier cannot be const. - * Elements that are not null are the ones that could be const. - */ - const couldBeConstIdentifiers = identifiers.filter(Boolean); - - /* - * If all elements from the same group could be const, report them. - * Otherwise (destructuring where only some could be const), report depending on the configuration. - */ - if (couldBeConstIdentifiers.length && - (couldBeConstIdentifiers.length === identifiers.length || shouldMatchAnyDestructuredVariable)) { - nodesToReport.push(...couldBeConstIdentifiers); + const variables = []; + let reportCount = 0; + let name = ""; + + /** + * Reports given identifier nodes if all of the nodes should be declared + * as const. + * + * The argument 'nodes' is an array of Identifier nodes. + * This node is the result of 'getIdentifierIfShouldBeConst()', so it's + * nullable. In simple declaration or assignment cases, the length of + * the array is 1. In destructuring cases, the length of the array can + * be 2 or more. + * + * @param {(eslint-scope.Reference|null)[]} nodes - + * References which are grouped by destructuring to report. + * @returns {void} + */ + function checkGroup(nodes) { + const nodesToReport = nodes.filter(Boolean); + + if (nodes.length && (shouldMatchAnyDestructuredVariable || nodesToReport.length === nodes.length)) { + const varDeclParent = findUp(nodes[0], "VariableDeclaration", parentNode => parentNode.type.endsWith("Statement")); + const isVarDecParentNull = varDeclParent === null; + + if (!isVarDecParentNull && varDeclParent.declarations.length > 0) { + const firstDeclaration = varDeclParent.declarations[0]; + + if (firstDeclaration.init) { + const firstDecParent = firstDeclaration.init.parent; + + /* + * First we check the declaration type and then depending on + * if the type is a "VariableDeclarator" or its an "ObjectPattern" + * we compare the name from the first identifier, if the names are different + * we assign the new name and reset the count of reportCount and nodeCount in + * order to check each block for the number of reported errors and base our fix + * based on comparing nodes.length and nodesToReport.length. + */ + + if (firstDecParent.type === "VariableDeclarator") { + + if (firstDecParent.id.name !== name) { + name = firstDecParent.id.name; + reportCount = 0; } - // If at least one identifier cannot be const, the whole declaration cannot be auto-fixed to const - if (couldBeConstIdentifiers.length < identifiers.length) { - shouldFix = false; + if (firstDecParent.id.type === "ObjectPattern") { + if (firstDecParent.init.name !== name) { + name = firstDecParent.init.name; + reportCount = 0; + } } - }); + } + } + } + + let shouldFix = varDeclParent && - // Don't do a fix unless all variables are initialized (or it's in a for-in or for-of loop) - shouldFix = shouldFix && - (node.parent.type === "ForInStatement" || node.parent.type === "ForOfStatement" || - node.declarations.every(declaration => declaration.init)); + // Don't do a fix unless all variables in the declarations are initialized (or it's in a for-in or for-of loop) + (varDeclParent.parent.type === "ForInStatement" || varDeclParent.parent.type === "ForOfStatement" || + varDeclParent.declarations.every(declaration => declaration.init)) && - const declarationKindToken = sourceCode.getFirstToken(node, t => t.value === node.kind); + /* + * If options.destructuring is "all", then this warning will not occur unless + * every assignment in the destructuring should be const. In that case, it's safe + * to apply the fix. + */ + nodesToReport.length === nodes.length; + + if (!isVarDecParentNull && varDeclParent.declarations && varDeclParent.declarations.length !== 1) { + + if (varDeclParent && varDeclParent.declarations && varDeclParent.declarations.length >= 1) { - nodesToReport.forEach(nodeToReport => { - context.report({ - node: nodeToReport, - messageId: "useConst", - data: nodeToReport, - fix: shouldFix ? fixer => fixer.replaceText(declarationKindToken, "const") : null - }); + /* + * Add nodesToReport.length to a count, then comparing the count to the length + * of the declarations in the current block. + */ + + reportCount += nodesToReport.length; + + shouldFix = shouldFix && (reportCount === varDeclParent.declarations.length); + } + } - // First report will fix the whole declaration - shouldFix = false; + nodesToReport.forEach(node => { + context.report({ + node, + messageId: "useConst", + data: node, + fix: shouldFix + ? fixer => fixer.replaceText( + sourceCode.getFirstToken(varDeclParent, t => t.value === varDeclParent.kind), + "const" + ) + : null }); + }); + } + } + + return { + "Program:exit"() { + groupByDestructuring(variables, ignoreReadBeforeAssign).forEach(checkGroup); + }, + + VariableDeclaration(node) { + if (node.kind === "let" && !isInitOfForStatement(node)) { + variables.push(...context.getDeclaredVariables(node)); } } }; diff --git a/tests/lib/rules/prefer-const.js b/tests/lib/rules/prefer-const.js index 1730e662655..d923985337b 100644 --- a/tests/lib/rules/prefer-const.js +++ b/tests/lib/rules/prefer-const.js @@ -527,15 +527,6 @@ ruleTester.run("prefer-const", rule, { { messageId: "useConst", data: { name: "b" }, type: "Identifier" }, { messageId: "useConst", data: { name: "c" }, type: "Identifier" } ] - }, - { - code: "let {a, b} = {}, c = 0; c = 1;", - output: null, - errors: [ - { messageId: "useConst", data: { name: "a" }, type: "Identifier" }, - { messageId: "useConst", data: { name: "b" }, type: "Identifier" } - ] } - ] });