From f6ea659693e5fde50bc3cdae730ef7ac4e65f478 Mon Sep 17 00:00:00 2001 From: Bryan Mishkin <698306+bmish@users.noreply.github.com> Date: Mon, 25 May 2020 10:17:37 -0500 Subject: [PATCH 1/6] fix: avoid naming collision with default array element variable in autofix for `no-for-loop` rule --- rules/no-for-loop.js | 10 +++++++++- test/no-for-loop.js | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/rules/no-for-loop.js b/rules/no-for-loop.js index 283ace5367..0bcceb3ac0 100644 --- a/rules/no-for-loop.js +++ b/rules/no-for-loop.js @@ -2,6 +2,7 @@ const getDocumentationUrl = require('./utils/get-documentation-url'); const isLiteralValue = require('./utils/is-literal-value'); const {flatten} = require('lodash'); +const avoidCapture = require('./utils/avoid-capture'); const defaultElementName = 'element'; const isLiteralZero = node => isLiteralValue(node, 0); @@ -261,6 +262,13 @@ const getReferencesInChildScopes = (scope, name) => { ]; }; +const getChildScopesRecursive = scope => { + return [ + scope, + ...flatten(scope.childScopes.map(s => getChildScopesRecursive(s))) + ]; +}; + const create = context => { const sourceCode = context.getSourceCode(); const {scopeManager} = sourceCode; @@ -335,7 +343,7 @@ const create = context => { const shouldGenerateIndex = isIndexVariableUsedElsewhereInTheLoopBody(indexVariable, bodyScope, arrayIdentifierName); const index = indexIdentifierName; - const element = elementIdentifierName || defaultElementName; + const element = elementIdentifierName || avoidCapture(defaultElementName, getChildScopesRecursive(bodyScope), context.parserOptions.ecmaVersion); const array = arrayIdentifierName; let declarationElement = element; diff --git a/test/no-for-loop.js b/test/no-for-loop.js index 24dfcdbfd3..bd4c2623f2 100644 --- a/test/no-for-loop.js +++ b/test/no-for-loop.js @@ -482,6 +482,40 @@ ruleTester.run('no-for-loop', rule, { const [ a, b ] = element; console.log(a, b, element); } + `), + + // Avoid naming collision when using default element name. + testCase(outdent` + for (let i = 0; i < arr.length; i += 1) { + console.log(arr[i]); + const element = foo(); + console.log(element); + } + `, outdent` + for (const element_ of arr) { + console.log(element_); + const element = foo(); + console.log(element); + } + `), + + // Avoid naming collision when using default element name (multiple collisions). + testCase(outdent` + for (let i = 0; i < arr.length; i += 1) { + console.log(arr[i]); + const element = foo(); + const element_ = foo(); + console.log(element); + console.log(element_); + } + `, outdent` + for (const element__ of arr) { + console.log(element__); + const element = foo(); + const element_ = foo(); + console.log(element); + console.log(element_); + } `) ] }); From a1c5b30b21bdb378f4c582760132b2343c811a5e Mon Sep 17 00:00:00 2001 From: fisker Date: Tue, 26 May 2020 01:33:12 +0800 Subject: [PATCH 2/6] More tests --- test/no-for-loop.js | 98 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 96 insertions(+), 2 deletions(-) diff --git a/test/no-for-loop.js b/test/no-for-loop.js index bd4c2623f2..e1d3c02adc 100644 --- a/test/no-for-loop.js +++ b/test/no-for-loop.js @@ -499,19 +499,113 @@ ruleTester.run('no-for-loop', rule, { } `), + // Avoid naming collision when using default element name (different scope). + testCase(outdent` + function element(element_) { + for (let i = 0; i < arr.length; i += 1) { + console.log(arr[i], element); + } + } + `, outdent` + function element(element_) { + for (const element__ of arr) { + console.log(element__, element); + } + } + `), + testCase(outdent` + for (let i = 0; i < arr.length; i += 1) { + function element__(element) { + console.log(arr[i], element); + } + } + `, outdent` + for (const element_ of arr) { + function element__(element) { + console.log(element_, element); + } + } + `), + testCase(outdent` + for (let i = 0; i < arr.length; i += 1) { + function element_(element) { + console.log(arr[i], element); + } + } + `, outdent` + for (const element__ of arr) { + function element_(element) { + console.log(element__, element); + } + } + `), + testCase(outdent` + for (let i = 0; i < arr.length; i += 1) { + function element() { + console.log(arr[i], element); + } + } + `, outdent` + for (const element_ of arr) { + function element() { + console.log(element_, element); + } + } + `), + testCase(outdent` + for (let i = 0; i < arr.length; i += 1) { + console.log(arr[i], element); + } + `, outdent` + for (const element_ of arr) { + console.log(element_, element); + } + `), + testCase(outdent` + for (let element = 0; element < arr.length; element += 1) { + console.log(element, arr[element]); + } + `, outdent` + for (const [element, element_] of arr.entries()) { + console.log(element, element_); + } + `), + testCase(outdent` + for (let element = 0; element < arr.length; element += 1) { + console.log(arr[element]); + } + `, outdent` + for (const element_ of arr) { + console.log(element_); + } + `), + testCase(outdent` + for (const element of arr) { + for (let j = 0; j < arr2.length; j += 1) { + console.log(element, arr2[j]); + } + } + `, outdent` + for (const element of arr) { + for (const element_ of arr2) { + console.log(element, element_); + } + } + `), + // Avoid naming collision when using default element name (multiple collisions). testCase(outdent` for (let i = 0; i < arr.length; i += 1) { - console.log(arr[i]); const element = foo(); + console.log(arr[i]); const element_ = foo(); console.log(element); console.log(element_); } `, outdent` for (const element__ of arr) { - console.log(element__); const element = foo(); + console.log(element__); const element_ = foo(); console.log(element); console.log(element_); From 2740c6208abc895bc5fcfa458d20a455c9dcc595 Mon Sep 17 00:00:00 2001 From: fisker Date: Wed, 27 May 2020 10:07:35 +0800 Subject: [PATCH 3/6] Add more tests --- test/no-for-loop.js | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/test/no-for-loop.js b/test/no-for-loop.js index e1d3c02adc..43fbf888d6 100644 --- a/test/no-for-loop.js +++ b/test/no-for-loop.js @@ -513,6 +513,21 @@ ruleTester.run('no-for-loop', rule, { } } `), + testCase(outdent` + let element; + function foo() { + for (let i = 0; i < arr.length; i += 1) { + console.log(arr[i]); + } + } + `, outdent` + let element; + function foo() { + for (const element_ of arr) { + console.log(element_); + } + } + `), testCase(outdent` for (let i = 0; i < arr.length; i += 1) { function element__(element) { @@ -561,6 +576,30 @@ ruleTester.run('no-for-loop', rule, { console.log(element_, element); } `), + testCase(outdent` + for (let i = 0; i < element.length; i += 1) { + console.log(element[i]); + } + `, outdent` + for (const element_ of element) { + console.log(element_); + } + `), + testCase(outdent` + for (let i = 0; i < arr.length; i += 1) { + console.log(arr[i]); + function foo(element) { + console.log(element); + } + } + `, outdent` + for (const element_ of arr) { + console.log(element_); + function foo(element) { + console.log(element); + } + } + `), testCase(outdent` for (let element = 0; element < arr.length; element += 1) { console.log(element, arr[element]); From 10772e7445a89565e7cdef8515ae90c1ea6012b2 Mon Sep 17 00:00:00 2001 From: fisker Date: Wed, 27 May 2020 10:08:34 +0800 Subject: [PATCH 4/6] Style --- rules/no-for-loop.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/rules/no-for-loop.js b/rules/no-for-loop.js index 0bcceb3ac0..b83e0d4572 100644 --- a/rules/no-for-loop.js +++ b/rules/no-for-loop.js @@ -262,12 +262,10 @@ const getReferencesInChildScopes = (scope, name) => { ]; }; -const getChildScopesRecursive = scope => { - return [ - scope, - ...flatten(scope.childScopes.map(s => getChildScopesRecursive(s))) - ]; -}; +const getChildScopesRecursive = scope => [ + scope, + ...flatten(scope.childScopes.map(s => getChildScopesRecursive(s))) +]; const create = context => { const sourceCode = context.getSourceCode(); From 00d4fb0de872326be315ad50d0cf7850214c211d Mon Sep 17 00:00:00 2001 From: fisker Date: Wed, 27 May 2020 10:08:50 +0800 Subject: [PATCH 5/6] rename --- rules/no-for-loop.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules/no-for-loop.js b/rules/no-for-loop.js index b83e0d4572..58bd449f11 100644 --- a/rules/no-for-loop.js +++ b/rules/no-for-loop.js @@ -264,7 +264,7 @@ const getReferencesInChildScopes = (scope, name) => { const getChildScopesRecursive = scope => [ scope, - ...flatten(scope.childScopes.map(s => getChildScopesRecursive(s))) + ...flatten(scope.childScopes.map(scope => getChildScopesRecursive(scope))) ]; const create = context => { From bf4c52bead9b60ae726f64cdf652ddc1a9dd36cc Mon Sep 17 00:00:00 2001 From: fisker Date: Wed, 27 May 2020 10:12:59 +0800 Subject: [PATCH 6/6] style --- rules/no-for-loop.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rules/no-for-loop.js b/rules/no-for-loop.js index 58bd449f11..e2f40885de 100644 --- a/rules/no-for-loop.js +++ b/rules/no-for-loop.js @@ -341,7 +341,8 @@ const create = context => { const shouldGenerateIndex = isIndexVariableUsedElsewhereInTheLoopBody(indexVariable, bodyScope, arrayIdentifierName); const index = indexIdentifierName; - const element = elementIdentifierName || avoidCapture(defaultElementName, getChildScopesRecursive(bodyScope), context.parserOptions.ecmaVersion); + const element = elementIdentifierName || + avoidCapture(defaultElementName, getChildScopesRecursive(bodyScope), context.parserOptions.ecmaVersion); const array = arrayIdentifierName; let declarationElement = element;