From 08616bfdafc808b3fb5d86a5e4a8c5664b9cead8 Mon Sep 17 00:00:00 2001 From: Victor Hom Date: Sun, 23 Jul 2017 21:34:24 -0400 Subject: [PATCH 1/6] Fix: handle array-destructuring with obj assignment in prefer const (fixes #8308) --- lib/rules/prefer-const.js | 11 ++++++++++ tests/lib/rules/prefer-const.js | 36 ++++++++++++++++++++++++--------- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/lib/rules/prefer-const.js b/lib/rules/prefer-const.js index 1395e0a8a08..de98b022da4 100644 --- a/lib/rules/prefer-const.js +++ b/lib/rules/prefer-const.js @@ -199,6 +199,17 @@ function groupByDestructuring(variables, ignoreReadBeforeAssign) { } } + for (const key of identifierMap.keys()) { + const destructureGroup = identifierMap.get(key); + const destructureElement = destructureGroup[0]; + + if (destructureElement && destructureElement.parent && destructureElement.parent.elements) { + if (destructureElement.parent.elements.length !== destructureGroup.length) { + identifierMap.delete(key); + } + } + } + return identifierMap; } diff --git a/tests/lib/rules/prefer-const.js b/tests/lib/rules/prefer-const.js index f389dcd3026..8cbe2148ff2 100644 --- a/tests/lib/rules/prefer-const.js +++ b/tests/lib/rules/prefer-const.js @@ -56,16 +56,24 @@ 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); }", - [ - "let id;", - "function foo() {", - " if (typeof id !== 'undefined') {", - " return;", - " }", - " id = setInterval(() => {}, 250);", - "}", - "foo();" - ].join("\n"), + "let predicate; [typeNode.returnType, predicate] = foo();", + "let predicate; let rest; [typeNode.returnType, predicate, ...rest] = foo();", + { + code: "let predicate; [typeNode.returnType, predicate] = foo();", + options: [{ destructuring: "all" }] + }, + { + code: [ + "let id;", + "function foo() {", + " if (typeof id !== 'undefined') {", + " return;", + " }", + " id = setInterval(() => {}, 250);", + "}", + "foo();" + ].join("\n") + }, "/*exported a*/ let a; function init() { a = foo(); }", "/*exported a*/ let a = 1", "let a; if (true) a = 0; foo(a);", @@ -268,6 +276,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", From 0048eb26a86af6a25a4a218f2f048ccea53168a9 Mon Sep 17 00:00:00 2001 From: Victor Hom Date: Sun, 23 Jul 2017 21:58:45 -0400 Subject: [PATCH 2/6] add a blank space to trigger github tests --- tests/lib/rules/prefer-const.js | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/lib/rules/prefer-const.js b/tests/lib/rules/prefer-const.js index 8cbe2148ff2..b6552454bda 100644 --- a/tests/lib/rules/prefer-const.js +++ b/tests/lib/rules/prefer-const.js @@ -95,6 +95,7 @@ ruleTester.run("prefer-const", rule, { options: [{ destructuring: "all" }] }, + // https://github.com/eslint/eslint/issues/8187 { code: "let { name, ...otherStuff } = obj; otherStuff = {};", From c21dad6a211e035945a1c645dcdc7e260f9ad620 Mon Sep 17 00:00:00 2001 From: Victor Hom Date: Sun, 23 Jul 2017 21:59:08 -0400 Subject: [PATCH 3/6] remove a blank line --- tests/lib/rules/prefer-const.js | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/lib/rules/prefer-const.js b/tests/lib/rules/prefer-const.js index b6552454bda..8cbe2148ff2 100644 --- a/tests/lib/rules/prefer-const.js +++ b/tests/lib/rules/prefer-const.js @@ -95,7 +95,6 @@ ruleTester.run("prefer-const", rule, { options: [{ destructuring: "all" }] }, - // https://github.com/eslint/eslint/issues/8187 { code: "let { name, ...otherStuff } = obj; otherStuff = {};", From bd70c1c873399eef3ea6b9243a2a97366603215a Mon Sep 17 00:00:00 2001 From: Victor Hom Date: Tue, 25 Jul 2017 22:50:29 -0400 Subject: [PATCH 4/6] add tests; one still failing --- lib/rules/prefer-const.js | 19 ++++++++------ tests/lib/rules/prefer-const.js | 44 +++++++++++++++++++++++++++++++-- 2 files changed, 53 insertions(+), 10 deletions(-) diff --git a/lib/rules/prefer-const.js b/lib/rules/prefer-const.js index de98b022da4..bdda581b1ae 100644 --- a/lib/rules/prefer-const.js +++ b/lib/rules/prefer-const.js @@ -164,9 +164,10 @@ function getDestructuringHost(reference) { * @param {eslint-scope.Variable[]} variables - Variables to group by destructuring. * @param {boolean} ignoreReadBeforeAssign - * The value of `ignoreReadBeforeAssign` option. + * @param {boolean} checkingMixedDestructuring - value on how to report destructuring (any/all) * @returns {Map} Grouped identifier nodes. */ -function groupByDestructuring(variables, ignoreReadBeforeAssign) { +function groupByDestructuring(variables, ignoreReadBeforeAssign, checkingMixedDestructuring) { const identifierMap = new Map(); for (let i = 0; i < variables.length; ++i) { @@ -199,13 +200,15 @@ function groupByDestructuring(variables, ignoreReadBeforeAssign) { } } - for (const key of identifierMap.keys()) { - const destructureGroup = identifierMap.get(key); - const destructureElement = destructureGroup[0]; + if (!checkingMixedDestructuring) { + for (const key of identifierMap.keys()) { + const destructureGroup = identifierMap.get(key); + const destructureElement = destructureGroup[0]; - if (destructureElement && destructureElement.parent && destructureElement.parent.elements) { - if (destructureElement.parent.elements.length !== destructureGroup.length) { - identifierMap.delete(key); + if (destructureElement && destructureElement.parent && destructureElement.parent.elements) { + if (destructureElement.parent.elements.length !== destructureGroup.length) { + identifierMap.delete(key); + } } } } @@ -311,7 +314,7 @@ module.exports = { return { "Program:exit"() { - groupByDestructuring(variables, ignoreReadBeforeAssign).forEach(checkGroup); + groupByDestructuring(variables, ignoreReadBeforeAssign, checkingMixedDestructuring).forEach(checkGroup); }, VariableDeclaration(node) { diff --git a/tests/lib/rules/prefer-const.js b/tests/lib/rules/prefer-const.js index 8cbe2148ff2..4377cd70bd1 100644 --- a/tests/lib/rules/prefer-const.js +++ b/tests/lib/rules/prefer-const.js @@ -56,12 +56,22 @@ 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); }", - "let predicate; [typeNode.returnType, predicate] = foo();", - "let predicate; let rest; [typeNode.returnType, predicate, ...rest] = foo();", + { + code: "let foo; ({ x: bar.baz, y: foo } = qux);", + options: [{ destructuring: "all" }] + }, + { + code: "let foo; [foo, [bar.baz]] = qux;", + 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" }] + }, { code: [ "let id;", @@ -359,6 +369,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" }] } ] }); From da70054ed496387a802e2490ae9a9a443eb82a84 Mon Sep 17 00:00:00 2001 From: Victor Hom Date: Sat, 29 Jul 2017 12:33:19 -0400 Subject: [PATCH 5/6] handle object destructuring --- lib/rules/prefer-const.js | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/lib/rules/prefer-const.js b/lib/rules/prefer-const.js index bdda581b1ae..c24ee328412 100644 --- a/lib/rules/prefer-const.js +++ b/lib/rules/prefer-const.js @@ -204,11 +204,20 @@ function groupByDestructuring(variables, ignoreReadBeforeAssign, checkingMixedDe for (const key of identifierMap.keys()) { const destructureGroup = identifierMap.get(key); const destructureElement = destructureGroup[0]; + let elementsInHost; - if (destructureElement && destructureElement.parent && destructureElement.parent.elements) { - if (destructureElement.parent.elements.length !== destructureGroup.length) { - identifierMap.delete(key); + if (destructureElement && destructureElement.parent) { + if (destructureElement.parent.elements) { + elementsInHost = destructureElement.parent.elements; } + + if (destructureElement.parent.parent.type === "ObjectPattern") { + elementsInHost = destructureGroup[0].parent.parent.properties; + } + } + + if (elementsInHost && elementsInHost.length && elementsInHost.length !== destructureGroup.length) { + identifierMap.delete(key); } } } From 695b0068b2a018e1672e6e6d84ea099fd6eacd1d Mon Sep 17 00:00:00 2001 From: Victor Hom Date: Sat, 23 Sep 2017 23:23:18 -0400 Subject: [PATCH 6/6] update for destructuring cases in prefer-const --- lib/rules/prefer-const.js | 146 ++++++++++++++++++++++++++++---- tests/lib/rules/prefer-const.js | 36 +++++--- 2 files changed, 154 insertions(+), 28 deletions(-) diff --git a/lib/rules/prefer-const.js b/lib/rules/prefer-const.js index c24ee328412..e55b57ccd10 100644 --- a/lib/rules/prefer-const.js +++ b/lib/rules/prefer-const.js @@ -126,6 +126,7 @@ function getIdentifierIfShouldBeConst(variable, ignoreReadBeforeAssign) { if (isReadBeforeInit) { return variable.defs[0].name; } + return writer.identifier; } @@ -164,16 +165,16 @@ function getDestructuringHost(reference) { * @param {eslint-scope.Variable[]} variables - Variables to group by destructuring. * @param {boolean} ignoreReadBeforeAssign - * The value of `ignoreReadBeforeAssign` option. - * @param {boolean} checkingMixedDestructuring - value on how to report destructuring (any/all) * @returns {Map} Grouped identifier nodes. */ -function groupByDestructuring(variables, ignoreReadBeforeAssign, checkingMixedDestructuring) { +function groupByDestructuring(variables, ignoreReadBeforeAssign) { const identifierMap = new Map(); for (let i = 0; i < variables.length; ++i) { const variable = variables[i]; const references = variable.references; const identifier = getIdentifierIfShouldBeConst(variable, ignoreReadBeforeAssign); + let prevId = null; for (let j = 0; j < references.length; ++j) { @@ -200,25 +201,125 @@ function groupByDestructuring(variables, ignoreReadBeforeAssign, checkingMixedDe } } - if (!checkingMixedDestructuring) { + 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") { + 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} 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} Grouped identifier nodes. + */ +function verifyAllDestructuring(identifierMap, checkingMixedDestructuring, destructureCount, destructureIdentifier) { + 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]; - let elementsInHost; - - if (destructureElement && destructureElement.parent) { - if (destructureElement.parent.elements) { - elementsInHost = destructureElement.parent.elements; - } - if (destructureElement.parent.parent.type === "ObjectPattern") { - elementsInHost = destructureGroup[0].parent.parent.properties; + if (destructureIdentifier !== null) { + for (let i = 0; i < destructureIdentifier.length; i++) { + if (destructureIdentifier[i] === destructureElement) { + identifierMap.delete(key); + } } } - - if (elementsInHost && elementsInHost.length && elementsInHost.length !== destructureGroup.length) { - identifierMap.delete(key); - } } } @@ -275,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 @@ -323,7 +426,18 @@ module.exports = { return { "Program:exit"() { - groupByDestructuring(variables, ignoreReadBeforeAssign, checkingMixedDestructuring).forEach(checkGroup); + const identifierMap = groupByDestructuring(variables, ignoreReadBeforeAssign); + + verifyAllDestructuring(identifierMap, checkingMixedDestructuring, destructureCount, destructureIdentifier).forEach(checkGroup); + }, + "ExpressionStatement[expression.left.type = /ArrayPattern|ObjectPattern/]"(node) { + 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) { diff --git a/tests/lib/rules/prefer-const.js b/tests/lib/rules/prefer-const.js index 4377cd70bd1..aa1e856deab 100644 --- a/tests/lib/rules/prefer-const.js +++ b/tests/lib/rules/prefer-const.js @@ -60,30 +60,37 @@ ruleTester.run("prefer-const", rule, { 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 predicate; [typeNode.returnType, predicate] = foo();", + code: "let foo, bar; [foo, [bar, baz.qux]] = qux;", + output: null, options: [{ destructuring: "all" }] }, { - code: "let predicate; let rest; [typeNode.returnType, predicate, ...rest] = foo();", + code: "let predicate; [typeNode.returnType, predicate] = foo();", options: [{ destructuring: "all" }] }, { - code: [ - "let id;", - "function foo() {", - " if (typeof id !== 'undefined') {", - " return;", - " }", - " id = setInterval(() => {}, 250);", - "}", - "foo();" - ].join("\n") + code: "let predicate; let rest; [typeNode.returnType, predicate, ...rest] = foo();", + options: [{ destructuring: "all" }] }, + [ + "let id;", + "function foo() {", + " if (typeof id !== 'undefined') {", + " return;", + " }", + " id = setInterval(() => {}, 250);", + "}", + "foo();" + ].join("\n"), "/*exported a*/ let a; function init() { a = foo(); }", "/*exported a*/ let a = 1", "let a; if (true) a = 0; foo(a);", @@ -124,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);",