Skip to content

Commit a87c296

Browse files
yeonjuanJoshuaKGoldberg
yeonjuan
and
JoshuaKGoldberg
authoredOct 10, 2024
fix(eslint-plugin): [no-loop-func] sync from upstream base rule (#10103)
* fix(eslint-plugin): [no-loop-func] sync from upstream base rule * update commit --------- Co-authored-by: Josh Goldberg <[email protected]>
·
v8.32.1v8.9.0
1 parent 656a36e commit a87c296

File tree

2 files changed

+384
-218
lines changed

2 files changed

+384
-218
lines changed
 

‎packages/eslint-plugin/src/rules/no-loop-func.ts

Lines changed: 189 additions & 147 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,168 @@ export default createRule<Options, MessageIds>({
3030
},
3131
defaultOptions: [],
3232
create(context) {
33+
const SKIPPED_IIFE_NODES = new Set<
34+
| TSESTree.ArrowFunctionExpression
35+
| TSESTree.FunctionDeclaration
36+
| TSESTree.FunctionExpression
37+
>();
38+
39+
/**
40+
* Gets the containing loop node of a specified node.
41+
*
42+
* We don't need to check nested functions, so this ignores those.
43+
* `Scope.through` contains references of nested functions.
44+
*
45+
* @param node An AST node to get.
46+
* @returns The containing loop node of the specified node, or `null`.
47+
*/
48+
function getContainingLoopNode(node: TSESTree.Node): TSESTree.Node | null {
49+
for (
50+
let currentNode = node;
51+
currentNode.parent;
52+
currentNode = currentNode.parent
53+
) {
54+
const parent = currentNode.parent;
55+
56+
switch (parent.type) {
57+
case AST_NODE_TYPES.WhileStatement:
58+
case AST_NODE_TYPES.DoWhileStatement:
59+
return parent;
60+
61+
case AST_NODE_TYPES.ForStatement:
62+
// `init` is outside of the loop.
63+
if (parent.init !== currentNode) {
64+
return parent;
65+
}
66+
break;
67+
68+
case AST_NODE_TYPES.ForInStatement:
69+
case AST_NODE_TYPES.ForOfStatement:
70+
// `right` is outside of the loop.
71+
if (parent.right !== currentNode) {
72+
return parent;
73+
}
74+
break;
75+
76+
case AST_NODE_TYPES.ArrowFunctionExpression:
77+
case AST_NODE_TYPES.FunctionExpression:
78+
case AST_NODE_TYPES.FunctionDeclaration:
79+
// We don't need to check nested functions.
80+
81+
// We need to check nested functions only in case of IIFE.
82+
if (SKIPPED_IIFE_NODES.has(parent)) {
83+
break;
84+
}
85+
return null;
86+
87+
default:
88+
break;
89+
}
90+
}
91+
92+
return null;
93+
}
94+
95+
/**
96+
* Gets the containing loop node of a given node.
97+
* If the loop was nested, this returns the most outer loop.
98+
* @param node A node to get. This is a loop node.
99+
* @param excludedNode A node that the result node should not include.
100+
* @returns The most outer loop node.
101+
*/
102+
function getTopLoopNode(
103+
node: TSESTree.Node,
104+
excludedNode: TSESTree.Node | null | undefined,
105+
): TSESTree.Node {
106+
const border = excludedNode ? excludedNode.range[1] : 0;
107+
let retv = node;
108+
let containingLoopNode: TSESTree.Node | null = node;
109+
110+
while (containingLoopNode && containingLoopNode.range[0] >= border) {
111+
retv = containingLoopNode;
112+
containingLoopNode = getContainingLoopNode(containingLoopNode);
113+
}
114+
115+
return retv;
116+
}
117+
118+
/**
119+
* Checks whether a given reference which refers to an upper scope's variable is
120+
* safe or not.
121+
* @param loopNode A containing loop node.
122+
* @param reference A reference to check.
123+
* @returns `true` if the reference is safe or not.
124+
*/
125+
function isSafe(
126+
loopNode: TSESTree.Node,
127+
reference: TSESLint.Scope.Reference,
128+
): boolean {
129+
const variable = reference.resolved;
130+
const definition = variable?.defs[0];
131+
const declaration = definition?.parent;
132+
const kind =
133+
declaration?.type === AST_NODE_TYPES.VariableDeclaration
134+
? declaration.kind
135+
: '';
136+
137+
// type references are all safe
138+
// this only really matters for global types that haven't been configured
139+
if (reference.isTypeReference) {
140+
return true;
141+
}
142+
143+
// Variables which are declared by `const` is safe.
144+
if (kind === 'const') {
145+
return true;
146+
}
147+
148+
/*
149+
* Variables which are declared by `let` in the loop is safe.
150+
* It's a different instance from the next loop step's.
151+
*/
152+
if (
153+
kind === 'let' &&
154+
declaration &&
155+
declaration.range[0] > loopNode.range[0] &&
156+
declaration.range[1] < loopNode.range[1]
157+
) {
158+
return true;
159+
}
160+
161+
/*
162+
* WriteReferences which exist after this border are unsafe because those
163+
* can modify the variable.
164+
*/
165+
const border = getTopLoopNode(
166+
loopNode,
167+
kind === 'let' ? declaration : null,
168+
).range[0];
169+
170+
/**
171+
* Checks whether a given reference is safe or not.
172+
* The reference is every reference of the upper scope's variable we are
173+
* looking now.
174+
*
175+
* It's safe if the reference matches one of the following condition.
176+
* - is readonly.
177+
* - doesn't exist inside a local function and after the border.
178+
*
179+
* @param upperRef A reference to check.
180+
* @returns `true` if the reference is safe.
181+
*/
182+
function isSafeReference(upperRef: TSESLint.Scope.Reference): boolean {
183+
const id = upperRef.identifier;
184+
185+
return (
186+
!upperRef.isWrite() ||
187+
(variable?.scope.variableScope === upperRef.from.variableScope &&
188+
id.range[0] < border)
189+
);
190+
}
191+
192+
return variable?.references.every(isSafeReference) ?? false;
193+
}
194+
33195
/**
34196
* Reports functions which match the following condition:
35197
* - has a loop node in ancestors.
@@ -50,8 +212,25 @@ export default createRule<Options, MessageIds>({
50212
}
51213

52214
const references = context.sourceCode.getScope(node).through;
215+
216+
if (!(node.async || node.generator) && isIIFE(node)) {
217+
const isFunctionExpression =
218+
node.type === AST_NODE_TYPES.FunctionExpression;
219+
220+
// Check if the function is referenced elsewhere in the code
221+
const isFunctionReferenced =
222+
isFunctionExpression && node.id
223+
? references.some(r => r.identifier.name === node.id?.name)
224+
: false;
225+
226+
if (!isFunctionReferenced) {
227+
SKIPPED_IIFE_NODES.add(node);
228+
return;
229+
}
230+
}
231+
53232
const unsafeRefs = references
54-
.filter(r => !isSafe(loopNode, r))
233+
.filter(r => r.resolved && !isSafe(loopNode, r))
55234
.map(r => r.identifier.name);
56235

57236
if (unsafeRefs.length > 0) {
@@ -71,151 +250,14 @@ export default createRule<Options, MessageIds>({
71250
},
72251
});
73252

74-
/**
75-
* Gets the containing loop node of a specified node.
76-
*
77-
* We don't need to check nested functions, so this ignores those.
78-
* `Scope.through` contains references of nested functions.
79-
*
80-
* @param node An AST node to get.
81-
* @returns The containing loop node of the specified node, or `null`.
82-
*/
83-
function getContainingLoopNode(node: TSESTree.Node): TSESTree.Node | null {
84-
for (
85-
let currentNode = node;
86-
currentNode.parent;
87-
currentNode = currentNode.parent
88-
) {
89-
const parent = currentNode.parent;
90-
91-
switch (parent.type) {
92-
case AST_NODE_TYPES.WhileStatement:
93-
case AST_NODE_TYPES.DoWhileStatement:
94-
return parent;
95-
96-
case AST_NODE_TYPES.ForStatement:
97-
// `init` is outside of the loop.
98-
if (parent.init !== currentNode) {
99-
return parent;
100-
}
101-
break;
102-
103-
case AST_NODE_TYPES.ForInStatement:
104-
case AST_NODE_TYPES.ForOfStatement:
105-
// `right` is outside of the loop.
106-
if (parent.right !== currentNode) {
107-
return parent;
108-
}
109-
break;
110-
111-
case AST_NODE_TYPES.ArrowFunctionExpression:
112-
case AST_NODE_TYPES.FunctionExpression:
113-
case AST_NODE_TYPES.FunctionDeclaration:
114-
// We don't need to check nested functions.
115-
return null;
116-
117-
default:
118-
break;
119-
}
120-
}
121-
122-
return null;
123-
}
124-
125-
/**
126-
* Gets the containing loop node of a given node.
127-
* If the loop was nested, this returns the most outer loop.
128-
* @param node A node to get. This is a loop node.
129-
* @param excludedNode A node that the result node should not include.
130-
* @returns The most outer loop node.
131-
*/
132-
function getTopLoopNode(
133-
node: TSESTree.Node,
134-
excludedNode: TSESTree.Node | null | undefined,
135-
): TSESTree.Node {
136-
const border = excludedNode ? excludedNode.range[1] : 0;
137-
let retv = node;
138-
let containingLoopNode: TSESTree.Node | null = node;
139-
140-
while (containingLoopNode && containingLoopNode.range[0] >= border) {
141-
retv = containingLoopNode;
142-
containingLoopNode = getContainingLoopNode(containingLoopNode);
143-
}
144-
145-
return retv;
146-
}
147-
148-
/**
149-
* Checks whether a given reference which refers to an upper scope's variable is
150-
* safe or not.
151-
* @param loopNode A containing loop node.
152-
* @param reference A reference to check.
153-
* @returns `true` if the reference is safe or not.
154-
*/
155-
function isSafe(
156-
loopNode: TSESTree.Node,
157-
reference: TSESLint.Scope.Reference,
253+
function isIIFE(
254+
node:
255+
| TSESTree.ArrowFunctionExpression
256+
| TSESTree.FunctionDeclaration
257+
| TSESTree.FunctionExpression,
158258
): boolean {
159-
const variable = reference.resolved;
160-
const definition = variable?.defs[0];
161-
const declaration = definition?.parent;
162-
const kind =
163-
declaration?.type === AST_NODE_TYPES.VariableDeclaration
164-
? declaration.kind
165-
: '';
166-
167-
// type references are all safe
168-
// this only really matters for global types that haven't been configured
169-
if (reference.isTypeReference) {
170-
return true;
171-
}
172-
173-
// Variables which are declared by `const` is safe.
174-
if (kind === 'const') {
175-
return true;
176-
}
177-
178-
/*
179-
* Variables which are declared by `let` in the loop is safe.
180-
* It's a different instance from the next loop step's.
181-
*/
182-
if (
183-
kind === 'let' &&
184-
declaration &&
185-
declaration.range[0] > loopNode.range[0] &&
186-
declaration.range[1] < loopNode.range[1]
187-
) {
188-
return true;
189-
}
190-
191-
/*
192-
* WriteReferences which exist after this border are unsafe because those
193-
* can modify the variable.
194-
*/
195-
const border = getTopLoopNode(loopNode, kind === 'let' ? declaration : null)
196-
.range[0];
197-
198-
/**
199-
* Checks whether a given reference is safe or not.
200-
* The reference is every reference of the upper scope's variable we are
201-
* looking now.
202-
*
203-
* It's safe if the reference matches one of the following condition.
204-
* - is readonly.
205-
* - doesn't exist inside a local function and after the border.
206-
*
207-
* @param upperRef A reference to check.
208-
* @returns `true` if the reference is safe.
209-
*/
210-
function isSafeReference(upperRef: TSESLint.Scope.Reference): boolean {
211-
const id = upperRef.identifier;
212-
213-
return (
214-
!upperRef.isWrite() ||
215-
(variable?.scope.variableScope === upperRef.from.variableScope &&
216-
id.range[0] < border)
217-
);
218-
}
219-
220-
return variable?.references.every(isSafeReference) ?? false;
259+
return (
260+
node.parent.type === AST_NODE_TYPES.CallExpression &&
261+
node.parent.callee === node
262+
);
221263
}

‎packages/eslint-plugin/tests/rules/no-loop-func.test.ts

Lines changed: 195 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,13 @@ const ruleTester = new RuleTester();
88
ruleTester.run('no-loop-func', rule, {
99
valid: [
1010
`
11+
for (let i = 0; i < 10; i++) {
12+
function foo() {
13+
console.log('A');
14+
}
15+
}
16+
`,
17+
`
1118
let someArray: MyType[] = [];
1219
for (let i = 0; i < 10; i += 1) {
1320
someArray = someArray.filter((item: MyType) => !!item);
@@ -50,7 +57,7 @@ for (let i = 0; i < 10; i += 1) {
5057
invalid: [],
5158
});
5259

53-
// Forked from https://github.com/eslint/eslint/blob/bf2e367bf4f6fde9930af9de8b8d8bc3d8b5782f/tests/lib/rules/no-loop-func.js
60+
// Forked from https://github.com/eslint/eslint/blob/89a4a0a260b8eb11487fe3d5d4d80f4630933eb3/tests/lib/rules/no-loop-func.js
5461
ruleTester.run('no-loop-func ESLint tests', rule, {
5562
valid: [
5663
"string = 'function a() {}';",
@@ -271,6 +278,193 @@ for (let i of {}) {
271278
),
272279
languageOptions: { parserOptions: { ecmaVersion: 6 } },
273280
},
281+
/*
282+
* These loops _look_ like they might be unsafe, but because i is undeclared, they're fine
283+
* at least as far as this rule is concerned - the loop doesn't declare/generate the variable.
284+
*/
285+
`
286+
while (i) {
287+
(function () {
288+
i;
289+
});
290+
}
291+
`,
292+
`
293+
do {
294+
(function () {
295+
i;
296+
});
297+
} while (i);
298+
`,
299+
300+
/**
301+
* These loops _look_ like they might be unsafe, but because i is declared outside the loop
302+
* and is not updated in or after the loop, they're fine as far as this rule is concerned.
303+
* The variable that's captured is just the one variable shared by all the loops, but that's
304+
* explicitly expected in these cases.
305+
*/
306+
`
307+
var i;
308+
while (i) {
309+
(function () {
310+
i;
311+
});
312+
}
313+
`,
314+
`
315+
var i;
316+
do {
317+
(function () {
318+
i;
319+
});
320+
} while (i);
321+
`,
322+
323+
/**
324+
* These loops use an undeclared variable, and so shouldn't be flagged by this rule,
325+
* they'll be picked up by no-undef.
326+
*/
327+
{
328+
code: `
329+
for (var i = 0; i < l; i++) {
330+
(function () {
331+
undeclared;
332+
});
333+
}
334+
`,
335+
languageOptions: { parserOptions: { ecmaVersion: 6 } },
336+
},
337+
{
338+
code: `
339+
for (let i = 0; i < l; i++) {
340+
(function () {
341+
undeclared;
342+
});
343+
}
344+
`,
345+
languageOptions: { parserOptions: { ecmaVersion: 6 } },
346+
},
347+
{
348+
code: `
349+
for (var i in {}) {
350+
i = 7;
351+
(function () {
352+
undeclared;
353+
});
354+
}
355+
`,
356+
languageOptions: { parserOptions: { ecmaVersion: 6 } },
357+
},
358+
{
359+
code: `
360+
for (let i in {}) {
361+
i = 7;
362+
(function () {
363+
undeclared;
364+
});
365+
}
366+
`,
367+
languageOptions: { parserOptions: { ecmaVersion: 6 } },
368+
},
369+
{
370+
code: `
371+
for (const i of {}) {
372+
(function () {
373+
undeclared;
374+
});
375+
}
376+
`,
377+
languageOptions: { parserOptions: { ecmaVersion: 6 } },
378+
},
379+
{
380+
code: `
381+
for (let i = 0; i < 10; ++i) {
382+
for (let x in xs.filter(x => x != undeclared)) {
383+
}
384+
}
385+
`,
386+
languageOptions: { parserOptions: { ecmaVersion: 6 } },
387+
},
388+
// IIFE
389+
{
390+
code: `
391+
let current = getStart();
392+
while (current) {
393+
(() => {
394+
current;
395+
current.a;
396+
current.b;
397+
current.c;
398+
current.d;
399+
})();
400+
401+
current = current.upper;
402+
}
403+
`,
404+
languageOptions: { parserOptions: { ecmaVersion: 6 } },
405+
},
406+
`
407+
for (
408+
var i = 0;
409+
(function () {
410+
i;
411+
})(),
412+
i < l;
413+
i++
414+
) {}
415+
`,
416+
`
417+
for (
418+
var i = 0;
419+
i < l;
420+
(function () {
421+
i;
422+
})(),
423+
i++
424+
) {}
425+
`,
426+
{
427+
code: `
428+
for (var i = 0; i < 10; ++i) {
429+
(() => {
430+
i;
431+
})();
432+
}
433+
`,
434+
languageOptions: { parserOptions: { ecmaVersion: 6 } },
435+
},
436+
{
437+
code: `
438+
for (var i = 0; i < 10; ++i) {
439+
(function a() {
440+
i;
441+
})();
442+
}
443+
`,
444+
languageOptions: { parserOptions: { ecmaVersion: 6 } },
445+
},
446+
{
447+
code: `
448+
var arr = [];
449+
for (var i = 0; i < 5; i++) {
450+
arr.push((f => f)((() => i)()));
451+
}
452+
`,
453+
languageOptions: { parserOptions: { ecmaVersion: 6 } },
454+
},
455+
{
456+
code: `
457+
var arr = [];
458+
for (var i = 0; i < 5; i++) {
459+
arr.push(
460+
(() => {
461+
return (() => i)();
462+
})(),
463+
);
464+
}
465+
`,
466+
languageOptions: { parserOptions: { ecmaVersion: 6 } },
467+
},
274468
],
275469
invalid: [
276470
{
@@ -390,76 +584,6 @@ for (var i = 0; i < l; i++) {
390584
},
391585
],
392586
},
393-
{
394-
code: `
395-
for (
396-
var i = 0;
397-
(function () {
398-
i;
399-
})(),
400-
i < l;
401-
i++
402-
) {}
403-
`,
404-
errors: [
405-
{
406-
data: { varNames: "'i'" },
407-
messageId: 'unsafeRefs',
408-
type: AST_NODE_TYPES.FunctionExpression,
409-
},
410-
],
411-
},
412-
{
413-
code: `
414-
for (
415-
var i = 0;
416-
i < l;
417-
(function () {
418-
i;
419-
})(),
420-
i++
421-
) {}
422-
`,
423-
errors: [
424-
{
425-
data: { varNames: "'i'" },
426-
messageId: 'unsafeRefs',
427-
type: AST_NODE_TYPES.FunctionExpression,
428-
},
429-
],
430-
},
431-
{
432-
code: `
433-
while (i) {
434-
(function () {
435-
i;
436-
});
437-
}
438-
`,
439-
errors: [
440-
{
441-
data: { varNames: "'i'" },
442-
messageId: 'unsafeRefs',
443-
type: AST_NODE_TYPES.FunctionExpression,
444-
},
445-
],
446-
},
447-
{
448-
code: `
449-
do {
450-
(function () {
451-
i;
452-
});
453-
} while (i);
454-
`,
455-
errors: [
456-
{
457-
data: { varNames: "'i'" },
458-
messageId: 'unsafeRefs',
459-
type: AST_NODE_TYPES.FunctionExpression,
460-
},
461-
],
462-
},
463587

464588
// Warns functions which are using modified variables.
465589
{

0 commit comments

Comments
 (0)
Please sign in to comment.