Skip to content

Commit d84143f

Browse files
fiskerfisker
authored
no-array-for-each: Support autofixing complicated parameters (sindresorhus#1228)
1 parent 6443ee3 commit d84143f

File tree

4 files changed

+243
-42
lines changed

4 files changed

+243
-42
lines changed

‎rules/no-array-for-each.js

Lines changed: 47 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -47,18 +47,19 @@ function isReturnStatementInContinueAbleNodes(returnStatement, callbackFunction)
4747
return false;
4848
}
4949

50-
function getFixFunction(callExpression, sourceCode, functionInfo) {
50+
function getFixFunction(callExpression, functionInfo, context) {
51+
const sourceCode = context.getSourceCode();
5152
const [callback] = callExpression.arguments;
5253
const parameters = callback.params;
5354
const array = callExpression.callee.object;
54-
const {returnStatements, scope} = functionInfo.get(callback);
55+
const {returnStatements} = functionInfo.get(callback);
5556

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

6061
let text = 'for (';
61-
text += parameters.some(parameter => isParameterReassigned(parameter, scope)) ? 'let' : 'const';
62+
text += isFunctionParameterVariableReassigned(callback, context) ? 'let' : 'const';
6263
text += ' ';
6364
text += useEntries ? `[${indexText}, ${elementText}]` : elementText;
6465
text += ' of ';
@@ -224,44 +225,57 @@ const isChildScope = (child, parent) => {
224225
return false;
225226
};
226227

227-
function isParameterSafeToFix(parameter, {scope, array, allIdentifiers}) {
228-
const {type, name: parameterName} = parameter;
229-
if (type !== 'Identifier') {
230-
return false;
231-
}
228+
function isFunctionParametersSafeToFix(callbackFunction, {context, scope, array, allIdentifiers}) {
229+
const variables = context.getDeclaredVariables(callbackFunction);
230+
231+
for (const variable of variables) {
232+
if (variable.defs.length !== 1) {
233+
return false;
234+
}
232235

233-
const [arrayStart, arrayEnd] = array.range;
234-
for (const identifier of allIdentifiers) {
235-
const {name, range: [start, end]} = identifier;
236-
if (
237-
name !== parameterName ||
238-
start < arrayStart ||
239-
end > arrayEnd
240-
) {
236+
const [definition] = variable.defs;
237+
if (definition.type !== 'Parameter') {
241238
continue;
242239
}
243240

244-
const variable = findVariable(scope, identifier);
245-
if (!variable || variable.scope === scope || isChildScope(scope, variable.scope)) {
246-
return false;
241+
const variableName = definition.name.name;
242+
const [arrayStart, arrayEnd] = array.range;
243+
for (const identifier of allIdentifiers) {
244+
const {name, range: [start, end]} = identifier;
245+
if (
246+
name !== variableName ||
247+
start < arrayStart ||
248+
end > arrayEnd
249+
) {
250+
continue;
251+
}
252+
253+
const variable = findVariable(scope, identifier);
254+
if (!variable || variable.scope === scope || isChildScope(scope, variable.scope)) {
255+
return false;
256+
}
247257
}
248258
}
249259

250260
return true;
251261
}
252262

253-
function isParameterReassigned(parameter, scope) {
254-
const variable = findVariable(scope, parameter);
255-
const {references} = variable;
256-
return references.some(reference => {
257-
const node = reference.identifier;
258-
const {parent} = node;
259-
return parent.type === 'UpdateExpression' ||
260-
(parent.type === 'AssignmentExpression' && parent.left === node);
261-
});
263+
function isFunctionParameterVariableReassigned(callbackFunction, context) {
264+
return context.getDeclaredVariables(callbackFunction)
265+
.filter(variable => variable.defs[0].type === 'Parameter')
266+
.some(variable => {
267+
const {references} = variable;
268+
return references.some(reference => {
269+
const node = reference.identifier;
270+
const {parent} = node;
271+
return parent.type === 'UpdateExpression' ||
272+
(parent.type === 'AssignmentExpression' && parent.left === node);
273+
});
274+
});
262275
}
263276

264-
function isFixable(callExpression, sourceCode, {scope, functionInfo, allIdentifiers}) {
277+
function isFixable(callExpression, {scope, functionInfo, allIdentifiers, context}) {
278+
const sourceCode = context.getSourceCode();
265279
// Check `CallExpression`
266280
if (
267281
callExpression.optional ||
@@ -297,10 +311,8 @@ function isFixable(callExpression, sourceCode, {scope, functionInfo, allIdentifi
297311
const parameters = callback.params;
298312
if (
299313
!(parameters.length === 1 || parameters.length === 2) ||
300-
parameters.some(parameter =>
301-
parameter.typeAnnotation ||
302-
!isParameterSafeToFix(parameter, {scope, array: callExpression, allIdentifiers})
303-
)
314+
parameters.some(({type, typeAnnotation}) => type === 'RestElement' || typeAnnotation) ||
315+
!isFunctionParametersSafeToFix(callback, {scope, array: callExpression, allIdentifiers, context})
304316
) {
305317
return false;
306318
}
@@ -329,8 +341,6 @@ const create = context => {
329341
const allIdentifiers = [];
330342
const functionInfo = new Map();
331343

332-
const sourceCode = context.getSourceCode();
333-
334344
return {
335345
':function'(node) {
336346
functionStack.push(node);
@@ -373,8 +383,8 @@ const create = context => {
373383
messageId: MESSAGE_ID
374384
};
375385

376-
if (isFixable(node, sourceCode, {scope, allIdentifiers, functionInfo})) {
377-
problem.fix = getFixFunction(node, sourceCode, functionInfo);
386+
if (isFixable(node, {scope, allIdentifiers, functionInfo, context})) {
387+
problem.fix = getFixFunction(node, functionInfo, context);
378388
}
379389

380390
context.report(problem);

‎test/no-array-for-each.mjs

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,7 @@ test.snapshot({
2525
'foo.forEach(function * (element) {})',
2626
'foo.forEach(() => bar())',
2727
'foo.forEach((element, index, array) => bar())',
28-
// Ideally this should be fixable, but hard to know variable conflicts
29-
'foo.forEach(({property}) => bar(property))',
28+
'property.forEach(({property}) => bar(property))',
3029

3130
// Can't turn `return` to `continue`
3231
outdent`
@@ -377,6 +376,22 @@ test.snapshot({
377376
let a;
378377
a >>>= element;
379378
});
379+
`,
380+
381+
// Complicated parameters
382+
'foo.forEach(({property}) => {bar(property)})',
383+
'foo.forEach(({foo: {foo: [property]}}) => {bar(property, index)})',
384+
'foo.forEach((element, {bar: {bar: [index]}}) => {bar(element, index)})',
385+
'foo.forEach((element = elementDefaultValue, index = indexDefaultValue) => {})',
386+
'foo.forEach(({}) => {})',
387+
'foo.forEach(function foo({a, b, c, d}) {})',
388+
'foo.forEach(function foo({a, b, c, d, foo}) {})',
389+
'foo.forEach(({foo: property}) => {bar(property)})',
390+
'foo.forEach(({[foo]: property}) => {bar(property)})',
391+
outdent`
392+
foo.forEach(({element}, index) => {
393+
element &&= 2;
394+
});
380395
`
381396
]
382397
});
@@ -401,6 +416,22 @@ test({
401416
parserOptions: {
402417
sourceType: 'script'
403418
}
419+
},
420+
{
421+
code: 'foo.forEach(function(element, element) {})',
422+
output: 'foo.forEach(function(element, element) {})',
423+
errors: 1,
424+
parserOptions: {
425+
sourceType: 'script'
426+
}
427+
},
428+
{
429+
code: 'foo.forEach(function element(element, element) {})',
430+
output: 'foo.forEach(function element(element, element) {})',
431+
errors: 1,
432+
parserOptions: {
433+
sourceType: 'script'
434+
}
404435
}
405436
]
406437
});

‎test/snapshots/no-array-for-each.mjs.md

Lines changed: 163 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,13 +115,13 @@ Generated by [AVA](https://avajs.dev).
115115
`
116116

117117
## Invalid #12
118-
1 | foo.forEach(({property}) => bar(property))
118+
1 | property.forEach(({property}) => bar(property))
119119

120120
> Error 1/1
121121
122122
`␊
123-
> 1 | foo.forEach(({property}) => bar(property))␊
124-
| ^^^^^^^ Do not use \`Array#forEach(…)\`.␊
123+
> 1 | property.forEach(({property}) => bar(property))␊
124+
| ^^^^^^^ Do not use \`Array#forEach(…)\`.␊
125125
`
126126

127127
## Invalid #13
@@ -1602,3 +1602,163 @@ Generated by [AVA](https://avajs.dev).
16021602
3 | a >>>= element;␊
16031603
4 | });␊
16041604
`
1605+
1606+
## Invalid #88
1607+
1 | foo.forEach(({property}) => {bar(property)})
1608+
1609+
> Output
1610+
1611+
`␊
1612+
1 | for (const {property} of foo) {bar(property)}␊
1613+
`
1614+
1615+
> Error 1/1
1616+
1617+
`␊
1618+
> 1 | foo.forEach(({property}) => {bar(property)})␊
1619+
| ^^^^^^^ Do not use \`Array#forEach(…)\`.␊
1620+
`
1621+
1622+
## Invalid #89
1623+
1 | foo.forEach(({foo: {foo: [property]}}) => {bar(property, index)})
1624+
1625+
> Output
1626+
1627+
`␊
1628+
1 | for (const {foo: {foo: [property]}} of foo) {bar(property, index)}␊
1629+
`
1630+
1631+
> Error 1/1
1632+
1633+
`␊
1634+
> 1 | foo.forEach(({foo: {foo: [property]}}) => {bar(property, index)})␊
1635+
| ^^^^^^^ Do not use \`Array#forEach(…)\`.␊
1636+
`
1637+
1638+
## Invalid #90
1639+
1 | foo.forEach((element, {bar: {bar: [index]}}) => {bar(element, index)})
1640+
1641+
> Output
1642+
1643+
`␊
1644+
1 | for (const [{bar: {bar: [index]}}, element] of foo.entries()) {bar(element, index)}␊
1645+
`
1646+
1647+
> Error 1/1
1648+
1649+
`␊
1650+
> 1 | foo.forEach((element, {bar: {bar: [index]}}) => {bar(element, index)})␊
1651+
| ^^^^^^^ Do not use \`Array#forEach(…)\`.␊
1652+
`
1653+
1654+
## Invalid #91
1655+
1 | foo.forEach((element = elementDefaultValue, index = indexDefaultValue) => {})
1656+
1657+
> Output
1658+
1659+
`␊
1660+
1 | for (const [index = indexDefaultValue, element = elementDefaultValue] of foo.entries()) {}␊
1661+
`
1662+
1663+
> Error 1/1
1664+
1665+
`␊
1666+
> 1 | foo.forEach((element = elementDefaultValue, index = indexDefaultValue) => {})␊
1667+
| ^^^^^^^ Do not use \`Array#forEach(…)\`.␊
1668+
`
1669+
1670+
## Invalid #92
1671+
1 | foo.forEach(({}) => {})
1672+
1673+
> Output
1674+
1675+
`␊
1676+
1 | for (const {} of foo) {}␊
1677+
`
1678+
1679+
> Error 1/1
1680+
1681+
`␊
1682+
> 1 | foo.forEach(({}) => {})␊
1683+
| ^^^^^^^ Do not use \`Array#forEach(…)\`.␊
1684+
`
1685+
1686+
## Invalid #93
1687+
1 | foo.forEach(function foo({a, b, c, d}) {})
1688+
1689+
> Output
1690+
1691+
`␊
1692+
1 | for (const {a, b, c, d} of foo) {}␊
1693+
`
1694+
1695+
> Error 1/1
1696+
1697+
`␊
1698+
> 1 | foo.forEach(function foo({a, b, c, d}) {})␊
1699+
| ^^^^^^^ Do not use \`Array#forEach(…)\`.␊
1700+
`
1701+
1702+
## Invalid #94
1703+
1 | foo.forEach(function foo({a, b, c, d, foo}) {})
1704+
1705+
> Error 1/1
1706+
1707+
`␊
1708+
> 1 | foo.forEach(function foo({a, b, c, d, foo}) {})␊
1709+
| ^^^^^^^ Do not use \`Array#forEach(…)\`.␊
1710+
`
1711+
1712+
## Invalid #95
1713+
1 | foo.forEach(({foo: property}) => {bar(property)})
1714+
1715+
> Output
1716+
1717+
`␊
1718+
1 | for (const {foo: property} of foo) {bar(property)}␊
1719+
`
1720+
1721+
> Error 1/1
1722+
1723+
`␊
1724+
> 1 | foo.forEach(({foo: property}) => {bar(property)})␊
1725+
| ^^^^^^^ Do not use \`Array#forEach(…)\`.␊
1726+
`
1727+
1728+
## Invalid #96
1729+
1 | foo.forEach(({[foo]: property}) => {bar(property)})
1730+
1731+
> Output
1732+
1733+
`␊
1734+
1 | for (const {[foo]: property} of foo) {bar(property)}␊
1735+
`
1736+
1737+
> Error 1/1
1738+
1739+
`␊
1740+
> 1 | foo.forEach(({[foo]: property}) => {bar(property)})␊
1741+
| ^^^^^^^ Do not use \`Array#forEach(…)\`.␊
1742+
`
1743+
1744+
## Invalid #97
1745+
1 | foo.forEach(({element}, index) => {
1746+
2 | element &&= 2;
1747+
3 | });
1748+
1749+
> Output
1750+
1751+
`␊
1752+
1 | for (let [index, {element}] of foo.entries()) {␊
1753+
2 | element &&= 2;␊
1754+
3 | }␊
1755+
`
1756+
1757+
> Error 1/1
1758+
1759+
`␊
1760+
> 1 | foo.forEach(({element}, index) => {␊
1761+
| ^^^^^^^ Do not use \`Array#forEach(…)\`.␊
1762+
2 | element &&= 2;␊
1763+
3 | });␊
1764+
`

‎test/snapshots/no-array-for-each.mjs.snap

525 Bytes
Binary file not shown.

0 commit comments

Comments
 (0)