From 1e2f57927d597a500692bb09d56c186ac7b02e96 Mon Sep 17 00:00:00 2001 From: Bryan Mishkin <698306+bmish@users.noreply.github.com> Date: Mon, 16 Dec 2019 16:15:57 -0800 Subject: [PATCH 1/4] fix: invalid autofix with destructuring assignment in no-for-loops rule Example (original): ```js for (let i = 0; i < arr.length; i++) { const { a, b } = arr[i]; console.log(a, b); } ``` Autofix (before this fix): ```js for (const element of arr) { console.log(a, b); } ``` Autofix (after this fix): ```js for (const element of arr) { const { a, b } = element; console.log(a, b); } ``` When `arr[i]` is used in a destructuring assignment, the entire assignment should not be removed (previous behavior), only `arr[i]` should be replaced with `element` (new behavior). --- rules/no-for-loop.js | 6 +++++- test/no-for-loop.js | 13 +++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/rules/no-for-loop.js b/rules/no-for-loop.js index f93503c464..e9a9dddc66 100644 --- a/rules/no-for-loop.js +++ b/rules/no-for-loop.js @@ -345,7 +345,11 @@ const create = context => { return fixer.replaceText(reference.identifier.parent, element); }), - elementNode && fixer.removeRange(getRemovalRange(elementNode, sourceCode)) + elementNode && ( + elementNode.id.type === 'ObjectPattern' ? + fixer.replaceText(elementNode.init, element) : + fixer.removeRange(getRemovalRange(elementNode, sourceCode)) + ) ].filter(Boolean); }; } diff --git a/test/no-for-loop.js b/test/no-for-loop.js index 4102b4acba..632e068a6f 100644 --- a/test/no-for-loop.js +++ b/test/no-for-loop.js @@ -356,6 +356,19 @@ ruleTester.run('no-for-loop', rule, { use(element); } } + `), + + // Destructuring assignment in usage: + testCase(outdent` + for (let i = 0; i < arr.length; i++) { + const { a, b } = arr[i]; + console.log(a, b); + } + `, outdent` + for (const element of arr) { + const { a, b } = element; + console.log(a, b); + } `) ] }); From ca742cb78d3425ad4f932b1808694d5d195e2993 Mon Sep 17 00:00:00 2001 From: fisker Date: Tue, 17 Dec 2019 14:17:18 +0800 Subject: [PATCH 2/4] fix: invalid autofix with destructuring assignment in no-for-loops rule --- rules/no-for-loop.js | 12 +++++++++--- test/no-for-loop.js | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/rules/no-for-loop.js b/rules/no-for-loop.js index f93503c464..431fd31646 100644 --- a/rules/no-for-loop.js +++ b/rules/no-for-loop.js @@ -326,12 +326,18 @@ const create = context => { const shouldGenerateIndex = isIndexVariableUsedElsewhereInTheLoopBody(indexVariable, bodyScope, arrayIdentifierName); const index = indexIdentifierName; - const element = elementIdentifierName || defaultElementName; const array = arrayIdentifierName; + let element = elementIdentifierName || defaultElementName; + let declarationType = 'const'; + if (elementNode && elementNode.id.type === 'ObjectPattern') { + declarationType = elementNode.parent.kind; + element = sourceCode.getText(elementNode.id); + } + const replacement = shouldGenerateIndex ? - `const [${index}, ${element}] of ${array}.entries()` : - `const ${element} of ${array}`; + `${declarationType} [${index}, ${element}] of ${array}.entries()` : + `${declarationType} ${element} of ${array}`; return [ fixer.replaceTextRange([ diff --git a/test/no-for-loop.js b/test/no-for-loop.js index 4102b4acba..524b7b7fe7 100644 --- a/test/no-for-loop.js +++ b/test/no-for-loop.js @@ -356,6 +356,48 @@ ruleTester.run('no-for-loop', rule, { use(element); } } + `), + + // Destructuring assignment in usage: + testCase(outdent` + for (let i = 0; i < arr.length; i++) { + const { a, b } = arr[i]; + console.log(a, b); + } + `, outdent` + for (const { a, b } of arr) { + console.log(a, b); + } + `), + testCase(outdent` + for (let i = 0; i < arr.length; i++) { + var { a, b } = arr[i]; + console.log(a, b); + } + `, outdent` + for (var { a, b } of arr) { + console.log(a, b); + } + `), + testCase(outdent` + for (let i = 0; i < arr.length; i++) { + let { a, b } = arr[i]; + console.log(a, b); + } + `, outdent` + for (let { a, b } of arr) { + console.log(a, b); + } + `), + testCase(outdent` + for (let i = 0; i < arr.length; i++) { + var { a, b } = arr[i]; + console.log(i, a, b); + } + `, outdent` + for (var [i, { a, b }] of arr.entries()) { + console.log(i, a, b); + } `) ] }); From c03479913d8696d13700b6754f0fb87053ef4444 Mon Sep 17 00:00:00 2001 From: fisker Date: Tue, 17 Dec 2019 14:42:03 +0800 Subject: [PATCH 3/4] Fix element used case --- rules/no-for-loop.js | 27 ++++++++++++++++++++------- test/no-for-loop.js | 11 +++++++++++ 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/rules/no-for-loop.js b/rules/no-for-loop.js index 431fd31646..9311cc8d4c 100644 --- a/rules/no-for-loop.js +++ b/rules/no-for-loop.js @@ -326,18 +326,27 @@ const create = context => { const shouldGenerateIndex = isIndexVariableUsedElsewhereInTheLoopBody(indexVariable, bodyScope, arrayIdentifierName); const index = indexIdentifierName; + const element = elementIdentifierName || defaultElementName; const array = arrayIdentifierName; - let element = elementIdentifierName || defaultElementName; + let declarationElement = element; let declarationType = 'const'; - if (elementNode && elementNode.id.type === 'ObjectPattern') { - declarationType = elementNode.parent.kind; - element = sourceCode.getText(elementNode.id); + let removeDeclaration = true; + if ( + elementNode && + elementNode.id.type === 'ObjectPattern' + ) { + removeDeclaration = arrayReferences.length === 1; + + if (removeDeclaration) { + declarationType = elementNode.parent.kind; + declarationElement = sourceCode.getText(elementNode.id); + } } const replacement = shouldGenerateIndex ? - `${declarationType} [${index}, ${element}] of ${array}.entries()` : - `${declarationType} ${element} of ${array}`; + `${declarationType} [${index}, ${declarationElement}] of ${array}.entries()` : + `${declarationType} ${declarationElement} of ${array}`; return [ fixer.replaceTextRange([ @@ -351,7 +360,11 @@ const create = context => { return fixer.replaceText(reference.identifier.parent, element); }), - elementNode && fixer.removeRange(getRemovalRange(elementNode, sourceCode)) + elementNode && ( + removeDeclaration ? + fixer.removeRange(getRemovalRange(elementNode, sourceCode)) : + fixer.replaceText(elementNode.init, element) + ) ].filter(Boolean); }; } diff --git a/test/no-for-loop.js b/test/no-for-loop.js index 524b7b7fe7..72bfa83b23 100644 --- a/test/no-for-loop.js +++ b/test/no-for-loop.js @@ -398,6 +398,17 @@ ruleTester.run('no-for-loop', rule, { for (var [i, { a, b }] of arr.entries()) { console.log(i, a, b); } + `), + testCase(outdent` + for (let i = 0; i < arr.length; i++) { + const { a, b } = arr[i]; + console.log(a, b, i, arr[i]); + } + `, outdent` + for (const [i, element] of arr.entries()) { + const { a, b } = element; + console.log(a, b, i, element); + } `) ] }); From 9a025f64821c2c3ae4190bbaca95a249ec6d7e79 Mon Sep 17 00:00:00 2001 From: Bryan Mishkin <698306+bmish@users.noreply.github.com> Date: Tue, 17 Dec 2019 09:22:22 -0800 Subject: [PATCH 4/4] test: add additional test case for no-for-loops with destructuring assignment --- test/no-for-loop.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/no-for-loop.js b/test/no-for-loop.js index 72bfa83b23..060ee2f9e2 100644 --- a/test/no-for-loop.js +++ b/test/no-for-loop.js @@ -409,6 +409,17 @@ ruleTester.run('no-for-loop', rule, { const { a, b } = element; console.log(a, b, i, element); } + `), + testCase(outdent` + for (let i = 0; i < arr.length; i++) { + const { a, b } = arr[i]; + console.log(a, b, arr[i]); + } + `, outdent` + for (const element of arr) { + const { a, b } = element; + console.log(a, b, element); + } `) ] });