Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: ignore IIFE's in the no-loop-func rule #17528

Merged
merged 16 commits into from
May 29, 2024
Merged
41 changes: 36 additions & 5 deletions docs/src/rules/no-loop-func.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ In this case, each function created within the loop returns a different number a

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.

This rule disallows any function within a loop that contains unsafe references (e.g. to modified variables from the outer scope).
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.
snitin315 marked this conversation as resolved.
Show resolved Hide resolved

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

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

for (var i=10; i; i--) {
(function() { return i; })();
}

var i = 0;
while(i < 5) {
var a = function() { return i; };
Expand Down Expand Up @@ -73,6 +69,25 @@ for (let i = 0; i < 10; ++i) {
setTimeout(() => console.log(foo));
}
foo = 100;

var arr = [];

for (var i = 0; i < 5; i++) {
arr.push((f => f)(() => i));
}

for (var i = 0; i < 5; i++) {
arr.push((() => {
return () => i;
})());
}

for (var i = 0; i < 5; i++) {
(function fun () {
if (arr.includes(fun)) return i;
else arr.push(fun);
})();
}
snitin315 marked this conversation as resolved.
Show resolved Hide resolved
```

:::
Expand Down Expand Up @@ -106,6 +121,22 @@ for (let i=10; i; i--) {
a();
}
//... no modifications of foo after this loop ...

var arr = [];

for (var i=10; i; i--) {
(function() { return i; })();
}

for (var i = 0; i < 5; i++) {
arr.push((f => f)((() => i)()));
}

for (var i = 0; i < 5; i++) {
arr.push((() => {
return (() => i)();
})());
}
```

:::
290 changes: 161 additions & 129 deletions lib/rules/no-loop-func.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,140 +9,16 @@
// Helpers
//------------------------------------------------------------------------------

/**
* Gets the containing loop node of a specified node.
*
* We don't need to check nested functions, so this ignores those.
* `Scope.through` contains references of nested functions.
* @param {ASTNode} node An AST node to get.
* @returns {ASTNode|null} The containing loop node of the specified node, or
* `null`.
*/
function getContainingLoopNode(node) {
for (let currentNode = node; currentNode.parent; currentNode = currentNode.parent) {
const parent = currentNode.parent;

switch (parent.type) {
case "WhileStatement":
case "DoWhileStatement":
return parent;

case "ForStatement":

// `init` is outside of the loop.
if (parent.init !== currentNode) {
return parent;
}
break;

case "ForInStatement":
case "ForOfStatement":

// `right` is outside of the loop.
if (parent.right !== currentNode) {
return parent;
}
break;

case "ArrowFunctionExpression":
case "FunctionExpression":
case "FunctionDeclaration":

// We don't need to check nested functions.
return null;

default:
break;
}
}

return null;
}

/**
* Gets the containing loop node of a given node.
* If the loop was nested, this returns the most outer loop.
* @param {ASTNode} node A node to get. This is a loop node.
* @param {ASTNode|null} excludedNode A node that the result node should not
* include.
* @returns {ASTNode} The most outer loop node.
* Identifies is a node is a FunctionExpression which is part of an IIFE
* @param {ASTNode} node Node to test
* @returns {boolean} True if it's an IIFE
*/
function getTopLoopNode(node, excludedNode) {
const border = excludedNode ? excludedNode.range[1] : 0;
let retv = node;
let containingLoopNode = node;

while (containingLoopNode && containingLoopNode.range[0] >= border) {
retv = containingLoopNode;
containingLoopNode = getContainingLoopNode(containingLoopNode);
}

return retv;
function isIIFE(node) {
return (node.type === "FunctionExpression" || node.type === "ArrowFunctionExpression") && node.parent && node.parent.type === "CallExpression" && node.parent.callee === node;
}

/**
* Checks whether a given reference which refers to an upper scope's variable is
* safe or not.
* @param {ASTNode} loopNode A containing loop node.
* @param {eslint-scope.Reference} reference A reference to check.
* @returns {boolean} `true` if the reference is safe or not.
*/
function isSafe(loopNode, reference) {
const variable = reference.resolved;
const definition = variable && variable.defs[0];
const declaration = definition && definition.parent;
const kind = (declaration && declaration.type === "VariableDeclaration")
? declaration.kind
: "";

// Variables which are declared by `const` is safe.
if (kind === "const") {
return true;
}

/*
* Variables which are declared by `let` in the loop is safe.
* It's a different instance from the next loop step's.
*/
if (kind === "let" &&
declaration.range[0] > loopNode.range[0] &&
declaration.range[1] < loopNode.range[1]
) {
return true;
}

/*
* WriteReferences which exist after this border are unsafe because those
* can modify the variable.
*/
const border = getTopLoopNode(
loopNode,
(kind === "let") ? declaration : null
).range[0];

/**
* Checks whether a given reference is safe or not.
* The reference is every reference of the upper scope's variable we are
* looking now.
*
* It's safe if the reference matches one of the following condition.
* - is readonly.
* - doesn't exist inside a local function and after the border.
* @param {eslint-scope.Reference} upperRef A reference to check.
* @returns {boolean} `true` if the reference is safe.
*/
function isSafeReference(upperRef) {
const id = upperRef.identifier;

return (
!upperRef.isWrite() ||
variable.scope.variableScope === upperRef.from.variableScope &&
id.range[0] < border
);
}

return Boolean(variable) && variable.references.every(isSafeReference);
}

//------------------------------------------------------------------------------
// Rule Definition
Expand All @@ -168,8 +44,147 @@ module.exports = {

create(context) {

const SKIPPED_IIFE_NODES = new Set();
const sourceCode = context.sourceCode;

/**
* Gets the containing loop node of a specified node.
*
* We don't need to check nested functions, so this ignores those, with the exception of IIFE.
* `Scope.through` contains references of nested functions.
* @param {ASTNode} node An AST node to get.
* @returns {ASTNode|null} The containing loop node of the specified node, or
* `null`.
*/
function getContainingLoopNode(node) {
for (let currentNode = node; currentNode.parent; currentNode = currentNode.parent) {
const parent = currentNode.parent;

switch (parent.type) {
case "WhileStatement":
case "DoWhileStatement":
return parent;

case "ForStatement":

// `init` is outside of the loop.
if (parent.init !== currentNode) {
return parent;
}
break;

case "ForInStatement":
case "ForOfStatement":

// `right` is outside of the loop.
if (parent.right !== currentNode) {
return parent;
}
break;

case "ArrowFunctionExpression":
case "FunctionExpression":
case "FunctionDeclaration":

// We need to check nested functions only in case of IIFE.
if (SKIPPED_IIFE_NODES.has(parent)) {
break;
}

return null;
default:
break;
}
}

return null;
}

/**
* Gets the containing loop node of a given node.
* If the loop was nested, this returns the most outer loop.
* @param {ASTNode} node A node to get. This is a loop node.
* @param {ASTNode|null} excludedNode A node that the result node should not
* include.
* @returns {ASTNode} The most outer loop node.
*/
function getTopLoopNode(node, excludedNode) {
const border = excludedNode ? excludedNode.range[1] : 0;
let retv = node;
let containingLoopNode = node;

while (containingLoopNode && containingLoopNode.range[0] >= border) {
retv = containingLoopNode;
containingLoopNode = getContainingLoopNode(containingLoopNode);
}

return retv;
}

/**
* Checks whether a given reference which refers to an upper scope's variable is
* safe or not.
* @param {ASTNode} loopNode A containing loop node.
* @param {eslint-scope.Reference} reference A reference to check.
* @returns {boolean} `true` if the reference is safe or not.
*/
function isSafe(loopNode, reference) {
const variable = reference.resolved;
const definition = variable && variable.defs[0];
const declaration = definition && definition.parent;
const kind = (declaration && declaration.type === "VariableDeclaration")
? declaration.kind
: "";

// Variables which are declared by `const` is safe.
if (kind === "const") {
return true;
}

/*
* Variables which are declared by `let` in the loop is safe.
* It's a different instance from the next loop step's.
*/
if (kind === "let" &&
declaration.range[0] > loopNode.range[0] &&
declaration.range[1] < loopNode.range[1]
) {
return true;
}

/*
* WriteReferences which exist after this border are unsafe because those
* can modify the variable.
*/
const border = getTopLoopNode(
loopNode,
(kind === "let") ? declaration : null
).range[0];

/**
* Checks whether a given reference is safe or not.
* The reference is every reference of the upper scope's variable we are
* looking now.
*
* It's safe if the reference matches one of the following condition.
* - is readonly.
* - doesn't exist inside a local function and after the border.
* @param {eslint-scope.Reference} upperRef A reference to check.
* @returns {boolean} `true` if the reference is safe.
*/
function isSafeReference(upperRef) {
const id = upperRef.identifier;

return (
!upperRef.isWrite() ||
variable.scope.variableScope === upperRef.from.variableScope &&
id.range[0] < border
);
}

return Boolean(variable) && variable.references.every(isSafeReference);
}

/**
* Reports functions which match the following condition:
*
Expand All @@ -186,6 +201,23 @@ module.exports = {
}

const references = sourceCode.getScope(node).through;

// Check if the function is not asynchronous or a generator function
if (!(node.async || node.generator)) {
snitin315 marked this conversation as resolved.
Show resolved Hide resolved
if (isIIFE(node)) {

const isFunctionExpression = node.type === "FunctionExpression";

// Check if the function is referenced elsewhere in the code
const isFunctionReferenced = isFunctionExpression && node.id ? references.some(r => r.identifier.name === node.id.name) : false;
snitin315 marked this conversation as resolved.
Show resolved Hide resolved

if (!isFunctionReferenced) {
SKIPPED_IIFE_NODES.add(node);
return;
}
}
}

const unsafeRefs = references.filter(r => r.resolved && !isSafe(loopNode, r)).map(r => r.identifier.name);

if (unsafeRefs.length > 0) {
Expand Down