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.

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

Fix: handle array-destructuring with obj assignment in prefer const #8994

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
139 changes: 138 additions & 1 deletion lib/rules/prefer-const.js
Expand Up @@ -126,6 +126,7 @@ function getIdentifierIfShouldBeConst(variable, ignoreReadBeforeAssign) {
if (isReadBeforeInit) {
return variable.defs[0].name;
}

return writer.identifier;
}

Expand Down Expand Up @@ -173,6 +174,7 @@ function groupByDestructuring(variables, ignoreReadBeforeAssign) {
const variable = variables[i];
const references = variable.references;
const identifier = getIdentifierIfShouldBeConst(variable, ignoreReadBeforeAssign);

let prevId = null;

for (let j = 0; j < references.length; ++j) {
Expand Down Expand Up @@ -202,6 +204,128 @@ function groupByDestructuring(variables, ignoreReadBeforeAssign) {
return identifierMap;
}

/**
* Returns a list of nodes
* that have have the type Identifier.
* This will search for nested Identifiers.
*
* @param {ASTNode} node - node to search for children and nested identifiers.
* @returns {ASTNode[]} list Identifier nodes.
*/
function getIdentifiersInArrayDestructureGroup(node) {
const identifiers = [];

if (node.elements) {
const numberOfElements = node.elements.length;

for (let i = 0; i < numberOfElements; i++) {
if (!node.elements[i].elements && node.elements[i].type === "Identifier") {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused about this line -- is the !node.elements[i].elements part redundant? It seems like an Identifier node would never have an elements property.

identifiers.push(node.elements[i]);
} else {
const innerIdentifiers = getIdentifiersInArrayDestructureGroup(node.elements[i]);

innerIdentifiers.forEach(identifierNode => identifiers.push(identifierNode));
}
}
}
return identifiers;
}

/**
* Returns a count of nodes
* This will count nested nodes in nodes elements attribute.
*
* @param {ASTNode} node - node to count elements.
* @returns {integer} count nodes.
*/
function countElementsInArrayDestructureGroup(node) {
let count = 0;

if (node.elements) {
const numberOfElements = node.elements.length;

for (let i = 0; i < numberOfElements; i++) {
if (!node.elements[i].elements) {
count += 1;
} else {
count += countElementsInArrayDestructureGroup(node.elements[i]);
}
}
}
return count;
}

/**
* Returns a list of nodes
* that have have the value type Identifier.
* This will search for nested Identifiers.
*
* @param {ASTNode} node - node to search properties that have value Identifier.
* @returns {ASTNode[]} list Identifier nodes.
*/
function getIdentifiersInObjectDestructureGroup(node) {
const identifier = [];
const propertiesLength = node.properties.length;

for (let i = 0; i < propertiesLength; i++) {
if (node.properties[i].value.type === "Identifier") {
identifier.push(node.properties[i].value);
}
}
return identifier;
}

/**
* Returns a count of nodes
* This will count the node properties length
*
* @param {ASTNode} node - node to count properties.
* @returns {integer} count properties length.
*/
function countElementsInObjectDestructureGroup(node) {
return node.properties.length;
}

/**
* Checks to see if there are less identifier nodes in a destructure group
* than the number of terms in the group. It might mean that there
* is a term in the group that cannot be converted to const.
* We want to make sure all terms can be reported
* If not, we should remove the terms that would be reported in "any" case
*
* @param {Map<ASTNode, ASTNode[]>} identifierMap - Variables to group by destructuring.
* @param {boolean} checkingMixedDestructuring - boolean to check if any value in destructuring
* should use const
* @param {integer|null} destructureCount count of destructure terms.
* @param {ASTNode[]|null} destructureIdentifier list of Identifier nodes to check
* in IdentifierMap
* @returns {Map<ASTNode, ASTNode[]>} Grouped identifier nodes.
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this description of the return type more detailed? For example, how are the nodes grouped?

*/
function verifyAllDestructuring(identifierMap, checkingMixedDestructuring, destructureCount, destructureIdentifier) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible for this function to return a new Map instead? In general I don't think it's a good idea for functions to mutate their arguments.

if (checkingMixedDestructuring) {
return identifierMap;
}
if (destructureCount === null && destructureIdentifier === null) {
return identifierMap;
}
if (destructureIdentifier.length < destructureCount) {
for (const key of identifierMap.keys()) {
const destructureGroup = identifierMap.get(key);
const destructureElement = destructureGroup[0];

if (destructureIdentifier !== null) {
for (let i = 0; i < destructureIdentifier.length; i++) {
if (destructureIdentifier[i] === destructureElement) {
identifierMap.delete(key);
}
}
}
}
}

return identifierMap;
}

/**
* Finds the nearest parent of node with a given type.
*
Expand Down Expand Up @@ -252,6 +376,8 @@ module.exports = {
const checkingMixedDestructuring = options.destructuring !== "all";
const ignoreReadBeforeAssign = options.ignoreReadBeforeAssign === true;
const variables = [];
let destructureCount = null;
let destructureIdentifier = null;

/**
* Reports given identifier nodes if all of the nodes should be declared
Expand Down Expand Up @@ -300,7 +426,18 @@ module.exports = {

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

verifyAllDestructuring(identifierMap, checkingMixedDestructuring, destructureCount, destructureIdentifier).forEach(checkGroup);
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right to me -- destructureCount is getting reassigned on every ExpressionStatement, so only the final value is getting used in the Program:exit listener. It seems like it doesn't make sense for the previous values of destructureCount to get discarded.

},
"ExpressionStatement[expression.left.type = /ArrayPattern|ObjectPattern/]"(node) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be AssignmentExpression rather than ExpressionStatement? Ostensibly we also want to catch assignments that aren't in an ExpressionStatement, e.g. foo([bar] = 1).

if (node.expression.left.type === "ArrayPattern") {
destructureIdentifier = getIdentifiersInArrayDestructureGroup(node.expression.left);
destructureCount = countElementsInArrayDestructureGroup(node.expression.left);
} else {
destructureIdentifier = getIdentifiersInObjectDestructureGroup(node.expression.left);
destructureCount = countElementsInObjectDestructureGroup(node.expression.left);
}
},

VariableDeclaration(node) {
Expand Down
68 changes: 68 additions & 0 deletions tests/lib/rules/prefer-const.js
Expand Up @@ -56,6 +56,31 @@ ruleTester.run("prefer-const", rule, {
"let a; function foo() { if (a) {} a = bar(); }",
"let a; function foo() { a = a || bar(); baz(a); }",
"let a; function foo() { bar(++a); }",
{
code: "let foo; ({ x: bar.baz, y: foo } = qux);",
options: [{ destructuring: "all" }]
},
{
code: "let foo; ({ x: bar.baz, y: foo, z: bar.bro.q } = qux);",
options: [{ destructuring: "all" }]
},
{
code: "let foo; [foo, [bar.baz]] = qux;",
options: [{ destructuring: "all" }]
},
{
code: "let foo, bar; [foo, [bar, baz.qux]] = qux;",
output: null,
options: [{ destructuring: "all" }]
},
{
code: "let predicate; [typeNode.returnType, predicate] = foo();",
options: [{ destructuring: "all" }]
},
{
code: "let predicate; let rest; [typeNode.returnType, predicate, ...rest] = foo();",
options: [{ destructuring: "all" }]
},
[
"let id;",
"function foo() {",
Expand Down Expand Up @@ -106,6 +131,11 @@ ruleTester.run("prefer-const", rule, {
}
],
invalid: [
{
code: "let foo; [foo, []] = qux;",
output: null,
errors: [{ message: "'foo' is never reassigned. Use 'const' instead.", type: "Identifier" }]
},
{
code: "let x = 1; foo(x);",
output: "const x = 1; foo(x);",
Expand Down Expand Up @@ -268,6 +298,14 @@ ruleTester.run("prefer-const", rule, {
{ message: "'a' is never reassigned. Use 'const' instead.", type: "Identifier" }
]
},
{
code: "let [ foo, bar ] = baz();",
output: "const [ foo, bar ] = baz();",
errors: [
{ message: "'foo' is never reassigned. Use 'const' instead.", type: "Identifier" },
{ message: "'bar' is never reassigned. Use 'const' instead.", type: "Identifier" }
]
},
{
code: "let {a} = obj",
output: "const {a} = obj",
Expand Down Expand Up @@ -343,6 +381,36 @@ ruleTester.run("prefer-const", rule, {
{ message: "'foo' is never reassigned. Use 'const' instead.", type: "Identifier" },
{ message: "'bar' is never reassigned. Use 'const' instead.", type: "Identifier" }
]
},
{
code: "let predicate; [predicate, typeNode.returnType] = foo();",
output: null,
errors: [{ message: "'predicate' is never reassigned. Use 'const' instead.", type: "Identifier" }]
},
{
code: "let predicate; [typeNode.returnType, predicate] = foo();",
output: null,
errors: [{ message: "'predicate' is never reassigned. Use 'const' instead.", type: "Identifier" }]
},
{
code: "let predicate; let rest; [typeNode.returnType, predicate, ...rest] = foo();",
output: null,
errors: [
{ message: "'predicate' is never reassigned. Use 'const' instead.", type: "Identifier" },
{ message: "'rest' is never reassigned. Use 'const' instead.", type: "Identifier" }
]
},
{
code: "let foo; [foo, [bar.baz]] = qux;",
output: null,
options: [{ destructuring: "any" }],
errors: [{ message: "'foo' is never reassigned. Use 'const' instead.", type: "Identifier" }]
},
{
code: "let foo; ({ x: bar.baz, y: foo } = qux);",
output: null,
options: [{ destructuring: "any" }],
errors: [{ message: "'foo' is never reassigned. Use 'const' instead.", type: "Identifier" }]
}
]
});