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

snitin315 · 2023-09-02T12:25:16Z

Prerequisites checklist

I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Fixes #16902

var arr = [];

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

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

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

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

for (var i = 0; i < 5; i++) {
    (function fun () {
        if (arr.includes(fun)) return i; // Error
        else arr.push(fun);
    })();
}

Is there anything you'd like reviewers to focus on?

netlify · 2023-09-02T12:25:21Z

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	`18cddef`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/6594d4c95fae9b0008630cbb

nzakas

Code LGTM. Can you also add a note in the rule details that explains that IIFEs are ignored while generators/async are not?

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

lib/rules/no-loop-func.js

nzakas

LGTM. Just a suggestion clean up the docs.

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

lib/rules/no-loop-func.js

mdjermanovic · 2023-09-16T22:59:05Z

I think we could simplify this a lot by taking a different approach, that is to skip IIFEs (that are not async, generators, or self-referencing) here and thus let the rule check and report their inner functions directly.

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

Lines 47 to 52 in bd7a71f

 case "ArrowFunctionExpression": 

 case "FunctionExpression": 

 case "FunctionDeclaration": 

 // We don't need to check nested functions. 

 return null;

For example, the current version of this rule reports error for unsafe reference to i on the IIFE:

/* eslint no-loop-func: "error" */

var arr = [];

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

After this change, it would report the same error directly on the inner function:

/* eslint no-loop-func: "error" */

var arr = [];

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

function checkForLoops could just skip IIFEs without checking anything about their inner functions since they'll be checked directly later in the traversal.

The only downside is that this can change error locations (and thus existing eslint-disable-next?-line comments would have to be reallocated) and the total number of errors if there are multiple inner functions, but I think it's okay for a semver-minor release.

github-actions · 2023-09-27T22:31:18Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

Rec0iL99 · 2023-09-28T11:47:13Z

Not Stale. @snitin315 is less active at the moment. Will be back soon.

github-actions · 2023-10-08T22:31:21Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

github-actions · 2023-10-20T22:31:20Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

snitin315 · 2023-10-21T02:30:36Z

The only downside is that this can change error locations (and thus existing eslint-disable-next?-line comments would have to be reallocated) and the total number of errors if there are multiple inner functions, but I think it's okay for a semver-minor release.

@mdjermanovic Once we changed error locations for max-lines-per-function, we had to revert it later in #15397. So, I'm not sure if we should do it in a minor release here.

mdjermanovic · 2023-10-23T09:37:39Z

The only downside is that this can change error locations (and thus existing eslint-disable-next?-line comments would have to be reallocated) and the total number of errors if there are multiple inner functions, but I think it's okay for a semver-minor release.

@mdjermanovic Once we changed error locations for max-lines-per-function, we had to revert it later in #15397. So, I'm not sure if we should do it in a minor release here.

I think it's okay in this case. For max-lines-per-function, the change wasn't a bug fix and it was affecting every lint problem reported by this rule, while this change is considered a bug fix and affects only problems on IIFEs, and most eslint-disable comments on IIFEs should be just removed so I wouldn't expect many cases where eslint-disable comments need to be reallocated (Loop > IIFE with inner functions seems like a rare case).

github-actions · 2023-11-02T22:31:25Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

snitin315 · 2023-11-07T05:33:29Z

Makes sense, I'll update the PR 👍🏻

github-actions · 2023-11-17T22:31:31Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

Rec0iL99 · 2023-11-18T16:07:53Z

Ping @snitin315

github-actions · 2023-11-28T22:31:21Z

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

snitin315 · 2024-01-03T03:35:12Z

@mdjermanovic Thanks for the detailed review, I've resolved and added corresponding test cases to your comments.

mdjermanovic · 2024-03-13T13:11:17Z

lib/rules/no-loop-func.js

- // We don't need to check nested functions.
- return null;
+ // We need to check nested functions only in case of IIFE.
+ if (isIIFE(node)) {


Suggested change

if (isIIFE(node)) {

if (isIIFE(parent)) {

mdjermanovic · 2024-03-13T13:14:21Z

lib/rules/no-loop-func.js

+ if (!(node.async || node.generator)) {
+ const isFunctionExpression = node.type === "FunctionExpression";
+
+ if (isIIFE(node)) {
+
+ // 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;
+
+ // Check if there are unsafe references used within non-immediately-invoked nested functions
+ const unsafeRefsInNonIIFE = unsafeRefs.filter(r => {
+ let refScope = r.from.variableScope;
+
+ if (node === refScope.block) {
+ return !isIIFE(refScope.block);
+ }
+
+ while (node !== refScope.block) {
+ if (!isIIFE(refScope.block)) {
+ return true;
+ }
+ refScope = refScope.upper.variableScope;
+ }
+
+ return false;
+ });
+
+ const unsafeRefsInNonIIFEName = unsafeRefsInNonIIFE.map(r => r.identifier.name);
+
+ if (!isFunctionReferenced && unsafeRefsInNonIIFEName.length === 0) {
+ return;
+ }
+
+ if (unsafeRefsInNonIIFEName.length > 0) {
+ context.report({
+ node,
+ messageId: "unsafeRefs",
+ data: { varNames: `'${unsafeRefsInNonIIFEName.join("', '")}'` }
+ });
+ return;
+ }
+ }
+ }


There is no need to check inner functions because they'll be checked separately.

snitin315 requested a review from a team as a code owner September 2, 2023 12:25

eslint-github-bot bot added the feature This change adds a new feature to ESLint label Sep 2, 2023

snitin315 added rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion labels Sep 2, 2023

snitin315 mentioned this pull request Sep 2, 2023

feat: ignore IIFE's for no-loop-func rule #17426

Closed

1 task

nzakas requested changes Sep 6, 2023

View reviewed changes

docs/src/rules/no-loop-func.md Show resolved Hide resolved

mdjermanovic reviewed Sep 6, 2023

View reviewed changes

lib/rules/no-loop-func.js Outdated Show resolved Hide resolved

mdjermanovic reviewed Sep 7, 2023

View reviewed changes

lib/rules/no-loop-func.js Outdated Show resolved Hide resolved

mdjermanovic reviewed Sep 9, 2023

View reviewed changes

lib/rules/no-loop-func.js Outdated Show resolved Hide resolved

snitin315 requested a review from mdjermanovic September 12, 2023 11:47

nzakas previously approved these changes Sep 12, 2023

View reviewed changes

docs/src/rules/no-loop-func.md Show resolved Hide resolved

mdjermanovic requested changes Sep 16, 2023

View reviewed changes

lib/rules/no-loop-func.js Show resolved Hide resolved

lib/rules/no-loop-func.js Outdated Show resolved Hide resolved

lib/rules/no-loop-func.js Show resolved Hide resolved

lib/rules/no-loop-func.js Show resolved Hide resolved

lib/rules/no-loop-func.js Show resolved Hide resolved

github-actions bot added the Stale label Sep 27, 2023

Rec0iL99 removed the Stale label Sep 28, 2023

github-actions bot added the Stale label Oct 8, 2023

Rec0iL99 removed the Stale label Oct 10, 2023

github-actions bot added the Stale label Oct 20, 2023

snitin315 removed the Stale label Oct 21, 2023

github-actions bot added the Stale label Nov 2, 2023

Rec0iL99 removed the Stale label Nov 3, 2023

github-actions bot added the Stale label Nov 17, 2023

Rec0iL99 removed the Stale label Nov 18, 2023

github-actions bot added the Stale label Nov 28, 2023

snitin315 self-assigned this Nov 29, 2023

snitin315 removed the Stale label Nov 29, 2023

snitin315 added 5 commits January 1, 2024 11:03

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

e0b962d

refactor: remove false negatives

0c1b35e

docs: add note about IIFE

36814ac

fix: correct hasUnsafeRefsInNonIIFE logic

0fd38e9

fix: handle more cases

7a9f721

snitin315 force-pushed the fix/ignore-iife-no-loop-func branch from 5dd9f24 to 7a9f721 Compare January 1, 2024 11:47

snitin315 added 4 commits January 2, 2024 09:30

chore: fix lint issues

0a349b3

fix: report only unsafe ref names in IIFEs

0db7246

fix: handle more cases

18f4a6a

chore: update comments

18cddef

amareshsm requested a review from mdjermanovic February 2, 2024 05:28

snitin315 dismissed nzakas’s stale review via 18cddef March 12, 2024 12:05

mdjermanovic reviewed Mar 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

snitin315 commented Sep 2, 2023

netlify bot commented Sep 2, 2023 •

edited

nzakas left a comment

nzakas left a comment

mdjermanovic commented Sep 16, 2023

github-actions bot commented Sep 27, 2023

Rec0iL99 commented Sep 28, 2023

github-actions bot commented Oct 8, 2023

github-actions bot commented Oct 20, 2023

snitin315 commented Oct 21, 2023

mdjermanovic commented Oct 23, 2023

github-actions bot commented Nov 2, 2023

snitin315 commented Nov 7, 2023

github-actions bot commented Nov 17, 2023

Rec0iL99 commented Nov 18, 2023

github-actions bot commented Nov 28, 2023

snitin315 commented Jan 3, 2024

mdjermanovic Mar 13, 2024

mdjermanovic Mar 13, 2024

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

Are you sure you want to change the base?

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

Conversation

snitin315 commented Sep 2, 2023

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

netlify bot commented Sep 2, 2023 • edited

✅ Deploy Preview for docs-eslint canceled.

nzakas left a comment

Choose a reason for hiding this comment

nzakas left a comment

Choose a reason for hiding this comment

mdjermanovic commented Sep 16, 2023

github-actions bot commented Sep 27, 2023

Rec0iL99 commented Sep 28, 2023

github-actions bot commented Oct 8, 2023

github-actions bot commented Oct 20, 2023

snitin315 commented Oct 21, 2023

mdjermanovic commented Oct 23, 2023

github-actions bot commented Nov 2, 2023

snitin315 commented Nov 7, 2023

github-actions bot commented Nov 17, 2023

Rec0iL99 commented Nov 18, 2023

github-actions bot commented Nov 28, 2023

snitin315 commented Jan 3, 2024

mdjermanovic Mar 13, 2024

Choose a reason for hiding this comment

mdjermanovic Mar 13, 2024

Choose a reason for hiding this comment

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

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

netlify bot commented Sep 2, 2023 •

edited