8000 feat: ignore IIFE's in the `no-loop-func` rule (#17528) · eslint/eslint@89a4a0a · GitHub
[go: up one dir, main page]
Skip to content

Commit 89a4a0a

Browse files
snitin315mdjermanovic
snitin315
and
mdjermanovic
authored
feat: ignore IIFE's in the no-loop-func rule (#17528)
* feat: ignore IIFE's in the `no-loop-func` rule * refactor: remove false negatives * docs: add note about IIFE * fix: correct hasUnsafeRefsInNonIIFE logic * fix: handle more cases * chore: fix lint issues * fix: report only unsafe ref names in IIFEs * fix: handle more cases * chore: update comments * refactor: remove unwanted code * refactor: remove unwanted code * refactor: update code * fix: avoid duplicate reports * chore: refactor code to avoid memory leak * chore: apply suggestions from code review Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * test: add location --------- Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
1 parent 80747d2 commit 89a4a0a

File tree

3 files changed

+454
-142
lines changed

3 files changed

+454
-142
lines changed

‎docs/src/rules/no-loop-func.md

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ In this case, each function created within the loop returns a different number a
3232

3333
This error is raised to highlight a piece of code that may not work as you expect it to and could also indicate a misunderstanding of how the language works. Your code may run without any problems if you do not fix this error, but in some situations it could behave unexpectedly.
3434

35-
This rule disallows any function within a loop that contains unsafe references (e.g. to modified variables from the outer scope).
35+
This rule disallows any function within a loop that contains unsafe references (e.g. to modified variables from the outer scope). This rule ignores IIFEs but not async or generator functions.
3636

3737
Examples of **incorrect** code for this rule:
3838

@@ -41,10 +41,6 @@ Examples of **incorrect** code for this rule:
4141
```js
4242
/*eslint no-loop-func: "error"*/
4343

44-
for (var i=10; i; i--) {
45-
(function() { return i; })();
46-
}
47-
4844
var i = 0;
4945
while(i < 5) {
5046
var a = function() { return i; };
@@ -73,6 +69,25 @@ for (let i = 0; i < 10; ++i) {
7369
setTimeout(() => console.log(foo));
7470
}
7571
foo = 100;
72+
73+
var arr = [];
74+
75+
for (var i = 0; i < 5; i++) {
76+
arr.push((f => f)(() => i));
77+
}
78+
79+
for (var i = 0; i < 5; i++) {
80+
arr.push((() => {
81+
return () => i;
82+
})());
83+
}
84+
85+
for (var i = 0; i < 5; i++) {
86+
(function fun () {
87+
if (arr.includes(fun)) return i;
88+
else arr.push(fun);
89+
})();
90+
}
7691
```
7792

7893
:::
@@ -106,6 +121,22 @@ for (let i=10; i; i--) {
106121
a();
107122
}
108123
//... no modifications of foo after this loop ...
124+
125+
var arr = [];
126+
127+
for (var i=10; i; i--) {
128+
(function() { return i; })();
129+
}
130+
131+
for (var i = 0; i < 5; i++) {
132+
arr.push((f => f)((() => i)()));
133+
}
134+
135+
for (var i = 0; i < 5; i++) {
136+
arr.push((() => {
137+
return (() => i)();
138+
})());
139+
}
109140
```
110141

111142
:::

‎lib/rules/no-loop-func.js

Lines changed: 161 additions & 129 deletions
Original file line numberDiff line numberDiff line change
@@ -9,140 +9,16 @@
99
// Helpers
1010
//------------------------------------------------------------------------------
1111

12-
/**
13-
* Gets the containing loop node of a specified node.
14-
*
15-
* We don't need to check nested functions, so this ignores those.
16-
* `Scope.through` contains references of nested functions.
17-
* @param {ASTNode} node An AST node to get.
18-
* @returns {ASTNode|null} The containing loop node of the specified node, or
19-
* `null`.
20-
*/
21-
function getContainingLoopNode(node) {
22-
for (let currentNode = node; currentNode.parent; currentNode = currentNode.parent) {
23-
const parent = currentNode.parent;
24-
25-
switch (parent.type) {
26-
case "WhileStatement":
27-
case "DoWhileStatement":
28-
return parent;
29-
30-
case "ForStatement":
31-
32-
// `init` is outside of the loop.
33-
if (parent.init !== currentNode) {
34-
return parent;
35-
}
36-
break;
37-
38-
case "ForInStatement":
39-
case "ForOfStatement":
40-
41-
// `right` is outside of the loop.
42-
if (parent.right !== currentNode) {
43-
return parent;
44-
}
45-
break;
46-
47-
case "ArrowFunctionExpression":
48-
case "FunctionExpression":
49-
case "FunctionDeclaration":
50-
51-
// We don't need to check nested functions.
52-
return null;
53-
54-
default:
55-
break;
56-
}
57-
}
58-
59-
return null;
60-
}
6112

6213
/**
63-
* Gets the containing loop node of a given node.
64-
* If the loop was nested, this returns the most outer loop.
65-
* @param {ASTNode} node A node to get. This is a loop node.
66-
* @param {ASTNode|null} excludedNode A node that the result node should not
67-
* include.
68-
* @returns {ASTNode} The most outer loop node.
14+
* Identifies is a node is a FunctionExpression which is part of an IIFE
15+
* @param {ASTNode} node Node to test
16+
* @returns {boolean} True if it's an IIFE
6917
*/
70-
function getTopLoopNode(node, excludedNode) {
71-
const border = excludedNode ? excludedNode.range[1] : 0;
72-
let retv = node;
73-
let containingLoopNode = node;
74-
75-
while (containingLoopNode && containingLoopNode.range[0] >= border) {
76-
retv = containingLoopNode;
77-
containingLoopNode = getContainingLoopNode(containingLoopNode);
78-
}
79-
80-
return retv;
18+
function isIIFE(node) {
19+
return (node.type === "FunctionExpression" || node.type === "ArrowFunctionExpression") && node.parent && node.parent.type === "CallExpression" && node.parent.callee === node;
8120
}
8221

83-
/**
84-
* Checks whether a given reference which refers to an upper scope's variable is
85-
* safe or not.
86-
* @param {ASTNode} loopNode A containing loop node.
87-
* @param {eslint-scope.Reference} reference A reference to check.
88-
* @returns {boolean} `true` if the reference is safe or not.
89-
*/
90-
function isSafe(loopNode, reference) {
91-
const variable = reference.resolved;
92-
const definition = variable && variable.defs[0];
93-
const declaration = definition && definition.parent;
94-
const kind = (declaration && declaration.type === "VariableDeclaration")
95-
? declaration.kind
96-
: "";
97-
98-
// Variables which are declared by `const` is safe.
99-
if (kind === "const") {
100-
return true;
101-
}
102-
103-
/*
104-
* Variables which are declared by `let` in the loop is safe.
105-
* It's a different instance from the next loop step's.
106-
*/
107-
if (kind === "let" &&
108-
declaration.range[0] > loopNode.range[0] &&
109-
declaration.range[1] < loopNode.range[1]
110-
) {
111-
return true;
112-
}
113-
114-
/*
115-
* WriteReferences which exist after this border are unsafe because those
116-
* can modify the variable.
117-
*/
118-
const border = getTopLoopNode(
119-
loopNode,
120-
(kind === "let") ? declaration : null
121-
).range[0];
122-
123-
/**
124-
* Checks whether a given reference is safe or not.
125-
* The reference is every reference of the upper scope's variable we are
126-
* looking now.
127-
*
128-
* It's safe if the reference matches one of the following condition.
129-
* - is readonly.
130-
* - doesn't exist inside a local function and after the border.
131-
* @param {eslint-scope.Reference} upperRef A reference to check.
132-
* @returns {boolean} `true` if the reference is safe.
133-
*/
134-
function isSafeReference(upperRef) {
135-
const id = upperRef.identifier;
136-
137-
return (
138-
179B !upperRef.isWrite() ||
139-
variable.scope.variableScope === upperRef.from.variableScope &&
140-
id.range[0] < border
141-
);
142-
}
143-
144-
return Boolean(variable) && variable.references.every(isSafeReference);
145-
}
14622

14723
//------------------------------------------------------------------------------
14824
// Rule Definition
@@ -168,8 +44,147 @@ module.exports = {
16844

16945
create(context) {
17046

47+
const SKIPPED_IIFE_NODES = new Set();
17148
const sourceCode = context.sourceCode;
17249

50+
/**
51+
* Gets the containing loop node of a specified node.
52+
*
53+
* We don't need to check nested functions, so this ignores those, with the exception of IIFE.
54+
* `Scope.through` contains references of nested functions.
55+
* @param {ASTNode} node An AST node to get.
56+
* @returns {ASTNode|null} The containing loop node of the specified node, or
57+
* `null`.
58+
*/
59+
function getContainingLoopNode(node) {
60+
for (let currentNode = node; currentNode.parent; currentNode = currentNode.parent) {
61+
const parent = currentNode.parent;
62+
63+
switch (parent.type) {
64+
case "WhileStatement":
65+
case "DoWhileStatement":
66+
return parent;
67+
68+
case "ForStatement":
69+
70+
// `init` is outside of the loop.
71+
if (parent.init !== currentNode) {
72+
return parent;
73+
}
74+
break;
75+
76+
case "ForInStatement":
77+
case "ForOfStatement":
78+
79+
// `right` is outside of the loop.
80+
if (parent.right !== currentNode) {
81+
return parent;
82+
}
83+
break;
84+
85+
case "ArrowFunctionExpression":
86+
case "FunctionExpression":
87+
case "FunctionDeclaration":
88+
89+
// We need to check nested functions only in case of IIFE.
90+
if (SKIPPED_IIFE_NODES.has(parent)) {
91+
break;
92+
}
93+
94+
return null;
95+
default:
96+
break;
97+
}
98+
}
99+
100+
return null;
101+
}
102+
103+
/**
104+
* Gets the containing loop node of a given node.
105+
* If the loop was nested, this returns the most outer loop.
106+
* @param {ASTNode} node A node to get. This is a loop node.
107+
* @param {ASTNode|null} excludedNode A node that the result node should not
108+
* include.
109+
* @returns {ASTNode} The most outer loop node.
110+
*/
111+
function getTopLoopNode(node, excludedNode) {
112+
const border = excludedNode ? excludedNode.range[1] : 0;
113+
let retv = node;
114+
let containingLoopNode = node;
115+
116+
while (containingLoopNode && containingLoopNode.range[0] >= border) {
117+
retv = containingLoopNode;
118+
containingLoopNode = getContainingLoopNode(containingLoopNode);
119+
}
120+
121+
return retv;
122+
}
123+
124+
/**
125+
* Checks whether a given reference which refers to an upper scope's variable is
126+
* safe or not.
127+
* @param {ASTNode} loopNode A containing loop node.
128+
* @param {eslint-scope.Reference} reference A reference to check.
129+
* @returns {boolean} `true` if the reference is safe or not.
130+
*/
131+
function isSafe(loopNode, reference) {
132+
const variable = reference.resolved;
133+
const definition = variable && variable.defs[0];
134+
const declaration = definition && definition.parent;
135+
const kind = (declaration && declaration.type === "VariableDeclaration")
136+
? declaration.kind
137+
: "";
138+
139+
// Variables which are declared by `const` is safe.
140+
if (kind === "const") {
141+
return true;
142+
}
143+
144+
/*
145+
* Variables which are declared by `let` in the loop is safe.
146+
* It's a different instance from the next loop step's.
147+
*/
148+
if (kind === "let" &&
149+
declaration.range[0] > loopNode.range[0] &&
150+
declaration.range[1] < loopNode.range[1]
151+
) {
152+
return true;
153+
}
154+
155+
/*
156+
* WriteReferences which exist after this border are unsafe because those
157+
* can modify the variable.
158+
*/
159+
const border = getTopLoopNode(
160+
loopNode,
161+
(kind === "let") ? declaration : null
162+
).range[0];
163+
164+
/**
165+
* Checks whether a given reference is safe or not.
166+
* The reference is every reference of the upper scope's variable we are
167+
* looking now.
168+
*
169+
* It's safe if the reference matches one of the following condition.
170+
* - is readonly.
171+
* - doesn't exist inside a local function and after the border.
172+
* @param {eslint-scope.Reference} upperRef A reference to check.
173+
* @returns {boolean} `true` if the reference is safe.
174+
*/
175+
function isSafeReference(upperRef) {
176+
const id = upperRef.identifier;
177+
178+
return (
179+
!upperRef.isWrite() ||
180+
variable.scope.variableScope === upperRef.from.variableScope &&
181+
id.range[0] < border
182+
);
183+
}
184+
185+
return Boolean(variable) && variable.references.every(isSafeReference);
186+
}
187+
173188
/**
174189
* Reports functions which match the following condition:
175190
*
@@ -186,6 +201,23 @@ module.exports = {
186201
}
187202

188203
const references = sourceCode.getScope(node).through;
204+
205+
// Check if the function is not asynchronous or a generator function
206+
if (!(node.async || node.generator)) {
207+
if (isIIFE(node)) {
208+
209+
const isFunctionExpression = node.type === "FunctionExpression";
210+
211+
// Check if the function is referenced elsewhere in the code
212+
const isFunctionReferenced = isFunctionExpression && node.id ? references.some(r => r.identifier.name === node.id.name) : false;
213+
214+
if (!isFunctionReferenced) {
215+
SKIPPED_IIFE_NODES.add(node);
216+
return;
217+
}
218+
}
219+
}
220+
189221
const unsafeRefs = references.filter(r => r.resolved && !isSafe(loopNode, r)).map(r => r.identifier.name);
190222

191223
if (unsafeRefs.length > 0) {

0 commit comments

Comments
 (0)
0