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: prefer-const produces invalid autofix (fixes #11699) #11827

Merged
merged 3 commits into from Jul 17, 2019
Merged

Fix: prefer-const produces invalid autofix (fixes #11699) #11827

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
82a205f
Fix: prefer-const produces invalid autofix (fixes #11699)
mdjermanovic Jun 11, 2019
8e241dc
Fixed potential token prefixes and multiple autofixes
mdjermanovic Jun 11, 2019
52d18f8
Rollback, fix just the Issue and TypeScript
mdjermanovic Jun 25, 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
159 changes: 46 additions & 113 deletions lib/rules/prefer-const.js
View file
Open in desktop
Expand Up @@ -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
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -361,109 +343,60 @@ 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;
}
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);
}
}
}
}

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) {
// If at least one identifier cannot be const, the whole declaration cannot be auto-fixed to const
if (couldBeConstIdentifiers.length < identifiers.length) {
shouldFix = false;
}
});

/*
* Add nodesToReport.length to a count, then comparing the count to the length
* of the declarations in the current block.
*/
// 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));

reportCount += nodesToReport.length;
const declarationKindToken = sourceCode.getFirstToken(node, t => t.value === node.kind);

shouldFix = shouldFix && (reportCount === varDeclParent.declarations.length);
}
}
nodesToReport.forEach(nodeToReport => {
context.report({
node: nodeToReport,
messageId: "useConst",
data: nodeToReport,
fix: shouldFix ? fixer => fixer.replaceText(declarationKindToken, "const") : null
});

nodesToReport.forEach(node => {
context.report({
node,
messageId: "useConst",
data: node,
fix: shouldFix ? fixer => fixer.replaceText(sourceCode.getFirstToken(varDeclParent), "const") : null
// First report will fix the whole declaration
shouldFix = false;
});
});
}
}

return {
"Program:exit"() {
groupByDestructuring(variables, ignoreReadBeforeAssign).forEach(checkGroup);
},

VariableDeclaration(node) {
if (node.kind === "let" && !isInitOfForStatement(node)) {
variables.push(...context.getDeclaredVariables(node));
}
}
};
Expand Down
27 changes: 27 additions & 0 deletions tests/lib/rules/prefer-const.js
View file
Open in desktop
Expand Up @@ -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" }
]
}

]
Expand Down