Change Request: no-loop-func should ignore variables declared outside the loop

[matwilko]

ESLint version

8.45.0

What problem do you want to solve?

At the moment, no-loop-func will highlight any variable reference that is declared outside the loop, but is mutated inside or after the loop as being "unsafe".

e.g. the test cases:

{
  code: "let a; for (let i=0; i<l; i++) { a = 1; (function() { a; });}",
  parserOptions: { ecmaVersion: 6 },
  errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionExpression" }]
},
{
  code: "let a; for (let i in {}) { (function() { a; }); a = 1; }",
  parserOptions: { ecmaVersion: 6 },
  errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionExpression" }]
},
{
  code: "let a; for (let i of {}) { (function() { a; }); } a = 1; ",
  parserOptions: { ecmaVersion: 6 },
  errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionExpression" }]
},
{
  code: "let a; for (let i=0; i<l; i++) { (function() { (function() { a; }); }); a = 1; }",
  parserOptions: { ecmaVersion: 6 },
  errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionExpression" }]
},
{
  code: "let a; for (let i in {}) { a = 1; function foo() { (function() { a; }); } }",
  parserOptions: { ecmaVersion: 6 },
  errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionDeclaration" }]
},
{
  code: "let a; for (let i of {}) { (() => { (function() { a; }); }); } a = 1;",
  parserOptions: { ecmaVersion: 6 },
  errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "ArrowFunctionExpression" }]
},
{
  code: "var a; for (let x of xs) { a = 1; (function() { a; }); }",
  parserOptions: { ecmaVersion: 6 },
  errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionExpression" }]
},
{
  code: "var a; for (let x of xs) { (function() { a; }); a = 1; }",
  parserOptions: { ecmaVersion: 6 },
  errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionExpression" }]
},
{
  code: "let a; function foo() { a = 10; } for (let x of xs) { (function() { a; }); } foo();",
  parserOptions: { ecmaVersion: 6 },
  errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionExpression" }]
},
{
  code: "let a; function foo() { a = 10; for (let x of xs) { (function() { a; }); } } foo();",
  parserOptions: { ecmaVersion: 6 },
  errors: [{ messageId: "unsafeRefs", data: { varNames: "'a'" }, type: "FunctionExpression" }]
}

Reading the docs for the rule, my understanding is that the rule is trying to prevent the very specific scenario of a function creating a closure over a loop variable that has a wider scope than the user might expect, and therefore that that loop variable is shared across all iterations.

i.e. it is trying to point out to the user that for (var a in {}) { ... } is equivalent to var a; for(a in {}) { ... }, and that capturing a is probably not going to do what they expect.

But in the case that the user has already declared a variable outside of the loop, I don't think the scope of that variable is at all unclear, at least in regards to loop iterations.

Unless the user fundamentally misunderstands scoping, I don't think any reasonable user would think the variable isn't shared by all iterations of the loop already, and therefore mutating it in/after the loop and capturing it isn't a problem.

What do you think is the correct solution?

I think the rule needs to ignore any variable references to variables that are declared outside the loop.

The rule should solely be detecting the case where the scope of a variable declared by the loop statement is wider than a single loop iteration, since that is the case when capturing it is counter-intuitive/"unsafe".

Participation

• I am willing to submit a pull request for this change.

Additional comments

Previous context on #17358

[mdjermanovic]

Unless the user fundamentally misunderstands scoping, I don't think any reasonable user would think the variable isn't shared by all iterations of the loop already, and therefore mutating it in/after the loop and capturing it isn't a problem.

This rule is about possible misunderstanding of closures, for example:

var i, arr = [];

for (i = 1; i <= 3; i++) {
    arr.push(() => i);
}

arr.map(f => f()); // expected: [1, 2, 3], actual: [4, 4, 4]

I think the current behavior is correct if the variable is modified in the loop, regardless of where it is declared, but I'm not sure if it should report variables modified after the loop but not in the loop where the functions are created.

[matwilko]

Yup, I understand what the current implementation is trying to call out, but in the example you gave, I don't agree that [1,2,3] is the expected outcome even from a quick glance - i has been specifically scoped outside of the loop, so I would expect it to be captured from that higher scope.

I'd only expect the rule to trigger when the actual scope of the variable doesn't match the expected scope of the variable. I get that we probably disagree on what the average user might consider the "expected" scope however.

Based on your example, I can definitely see an argument that a different rule should warn that you shouldn't re-use existing variables in loop statements - that's definitely likely to cause headaches 😄

As a counter-example, here's a small change to your example to create a valid scenario that gets flagged:

let total = 0, loaded = 0, arr = [];
for (const someFunc of someIterator) {
   arr.push(() => { someFunc(); console.log(`Loaded func ${loaded++} of ${total}`); });
   total++;
}

return arr.map(f => f());

Unlike the scenario where the loop statement declares the variable, where the fix is to change the declaration to let or const to trigger the proper scoping, there's no way to change this code to satisfy the rule without either disabling it or changing the semantics of the code.

If we can't agree on the "expected" scope, could this perhaps be implemented as an option rather than a change to the default behaviour, so that users could choose which behaviour applies?

[mdjermanovic]

Referencing a variable that changes in the loop looks like intent to capture the current value at the moment where the function is created. I think that's what this rule targets, not potential scoping issues.

If we can't agree on the "expected" scope, could this perhaps be implemented as an option rather than a change to the default behaviour, so that users could choose which behaviour applies?

Which cases would the rule report with the new behavior? It seems the only potential problems would be var declarations, which are rarely used at this point.

[github-actions]

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

[github-actions]

This issue was auto-closed due to inactivity. While we wish we could keep responding to every issue, we unfortunately don't have the bandwidth and need to focus on high-value issues.