Skip to content

Commit

Permalink
no-array-for-each: Use let if parameters are reassigned (#1139)
Browse files Browse the repository at this point in the history
  • Loading branch information
@fisker
fisker authored Mar 17, 2021
1 parent 4846167 commit a13ad3c
Show file tree
Hide file tree
Showing 5 changed files with 234 additions and 8 deletions.
43 changes: 38 additions & 5 deletions rules/no-array-for-each.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,16 @@ function getFixFunction(callExpression, sourceCode, functionInfo) {
const [callback] = callExpression.arguments;
const parameters = callback.params;
const array = callExpression.callee.object;
const {returnStatements} = functionInfo.get(callback);
const {returnStatements, scope} = functionInfo.get(callback);

const getForOfLoopHeadText = () => {
const [elementText, indexText] = parameters.map(parameter => sourceCode.getText(parameter));
const useEntries = parameters.length === 2;

let text = 'for (const ';
let text = 'for (';
text += parameters.some(parameter => isParameterReassigned(parameter, scope)) ? 'let' : 'const';
text += ' ';
text += useEntries ? `[${indexText}, ${elementText}]` : elementText;

text += ' of ';

let arrayText = sourceCode.getText(array);
Expand All @@ -83,6 +84,9 @@ function getFixFunction(callExpression, sourceCode, functionInfo) {
if (callback.body.type === 'BlockStatement') {
end = callback.body.range[0];
} else {
// In this case, parentheses are not included in body location, so we look for `=>` token
// foo.forEach(bar => ({bar}))
// ^
const arrowToken = sourceCode.getTokenBefore(callback.body, isArrowToken);
end = arrowToken.range[1];
}
Expand Down Expand Up @@ -167,32 +171,50 @@ function getFixFunction(callExpression, sourceCode, functionInfo) {
}

return function * (fixer) {
// Replace these with `for (const … of …) `
// foo.forEach(bar => bar)
// ^^^^^^^^^^^^^^^^^^ (space after `=>` didn't included)
// foo.forEach(bar => {})
// ^^^^^^^^^^^^^^^^^^^^^^
// foo.forEach(function(bar) {})
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
yield fixer.replaceTextRange(getForOfLoopHeadRange(), getForOfLoopHeadText());

// Parenthesized callback function
// foo.forEach( ((bar => {})) )
// ^^
yield * removeCallbackParentheses(fixer);

// Remove call expression trailing comma
const [
penultimateToken,
lastToken
] = sourceCode.getLastTokens(callExpression, 2);

// The possible trailing comma token of `Array#forEach()` CallExpression
// foo.forEach(bar => {},)
// ^
if (isCommaToken(penultimateToken)) {
yield fixer.remove(penultimateToken);
}

// The closing parenthesis token of `Array#forEach()` CallExpression
// foo.forEach(bar => {})
// ^
yield fixer.remove(lastToken);

for (const returnStatement of returnStatements) {
yield * replaceReturnStatement(returnStatement, fixer);
}

const expressionStatementLastToken = sourceCode.getLastToken(callExpression.parent);
// Remove semicolon if it's not needed anymore
// foo.forEach(bar => {});
// ^
if (shouldRemoveExpressionStatementLastToken(expressionStatementLastToken)) {
yield fixer.remove(expressionStatementLastToken, fixer);
}

// Prevent possible conflicts
// Prevent possible variable conflicts
yield * extendFixRange(fixer, callExpression.parent.range);
};
}
Expand Down Expand Up @@ -258,6 +280,17 @@ function isParameterSafeToFix(parameter, {scope, array, allIdentifiers}) {
return true;
}

function isParameterReassigned(parameter, scope) {
const variable = findVariable(scope, parameter);
const {references} = variable;
return references.some(reference => {
const node = reference.identifier;
const {parent} = node;
return parent.type === 'UpdateExpression' ||
(parent.type === 'AssignmentExpression' && parent.left === node);
});
}

function isFixable(callExpression, sourceCode, {scope, functionInfo, allIdentifiers}) {
// Check `CallExpression`
if (
Expand Down
2 changes: 0 additions & 2 deletions rules/utils/is-node-matches.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ function isNodeMatchesNameOrPath(node, nameOrPath) {

node = node.object;
}

return true;
}

/**
Expand Down
59 changes: 58 additions & 1 deletion test/no-array-for-each.mjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,64 @@ test.snapshot({
'NotReact.Children.forEach(bar)',
'React.NotChildren.forEach(bar)',
'React?.Children.forEach(bar)',
'NotChildren.forEach(bar)'
'NotChildren.forEach(bar)',
// Parameters are reassigned
outdent`
foo.forEach(element => {
element ++;
})
`,
outdent`
foo.forEach(element => {
const a = -- element;
})
`,
outdent`
foo.forEach((element, index) => {
index ++;
element = 2
});
`,
outdent`
foo.forEach((element, index) => {
element >>>= 2;
});
`,
outdent`
foo.forEach((element, index) => {
const a = element = 1;
});
`,
outdent`
foo.forEach((element, index) => {
let a;
a >>>= element;
});
`
]
});

test({
valid: [],
invalid: [
{
code: outdent`
foo.forEach(function(element) {
delete element;
console.log(element)
});
`,
output: outdent`
for (const element of foo) {
delete element;
console.log(element)
}
`,
errors: 1,
parserOptions: {
sourceType: 'script'
}
}
]
});

Expand Down
138 changes: 138 additions & 0 deletions test/snapshots/no-array-for-each.mjs.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1454,3 +1454,141 @@ Generated by [AVA](https://avajs.dev).
> 1 | NotChildren.forEach(bar)␊
| ^^^^^^^ Do not use `Array#forEach(…)`.␊
`

## Invalid #82
1 | foo.forEach(element => {
2 | element ++;
3 | })

> Output
`␊
1 | for (let element of foo) {␊
2 | element ++;␊
3 | }␊
`

> Error 1/1
`␊
> 1 | foo.forEach(element => {␊
| ^^^^^^^ Do not use `Array#forEach(…)`.␊
2 | element ++;␊
3 | })␊
`

## Invalid #83
1 | foo.forEach(element => {
2 | const a = -- element;
3 | })

> Output
`␊
1 | for (let element of foo) {␊
2 | const a = -- element;␊
3 | }␊
`

> Error 1/1
`␊
> 1 | foo.forEach(element => {␊
| ^^^^^^^ Do not use `Array#forEach(…)`.␊
2 | const a = -- element;␊
3 | })␊
`

## Invalid #84
1 | foo.forEach((element, index) => {
2 | index ++;
3 | element = 2
4 | });

> Output
`␊
1 | for (let [index, element] of foo.entries()) {␊
2 | index ++;␊
3 | element = 2␊
4 | }␊
`

> Error 1/1
`␊
> 1 | foo.forEach((element, index) => {␊
| ^^^^^^^ Do not use `Array#forEach(…)`.␊
2 | index ++;␊
3 | element = 2␊
4 | });␊
`

## Invalid #85
1 | foo.forEach((element, index) => {
2 | element >>>= 2;
3 | });

> Output
`␊
1 | for (let [index, element] of foo.entries()) {␊
2 | element >>>= 2;␊
3 | }␊
`

> Error 1/1
`␊
> 1 | foo.forEach((element, index) => {␊
| ^^^^^^^ Do not use `Array#forEach(…)`.␊
2 | element >>>= 2;␊
3 | });␊
`

## Invalid #86
1 | foo.forEach((element, index) => {
2 | const a = element = 1;
3 | });

> Output
`␊
1 | for (let [index, element] of foo.entries()) {␊
2 | const a = element = 1;␊
3 | }␊
`

> Error 1/1
`␊
> 1 | foo.forEach((element, index) => {␊
| ^^^^^^^ Do not use `Array#forEach(…)`.␊
2 | const a = element = 1;␊
3 | });␊
`

## Invalid #87
1 | foo.forEach((element, index) => {
2 | let a;
3 | a >>>= element;
4 | });

> Output
`␊
1 | for (const [index, element] of foo.entries()) {␊
2 | let a;␊
3 | a >>>= element;␊
4 | }␊
`

> Error 1/1
`␊
> 1 | foo.forEach((element, index) => {␊
| ^^^^^^^ Do not use `Array#forEach(…)`.␊
2 | let a;␊
3 | a >>>= element;␊
4 | });␊
`
Binary file modified test/snapshots/no-array-for-each.mjs.snap
View file
Binary file not shown.

0 comments on commit a13ad3c

Please sign in to comment.