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

False positive on `no-loop-func` #9527

Closed
isiahmeadows opened this Issue Oct 28, 2017 · 4 comments

Comments

Projects
None yet
5 participants
@isiahmeadows

isiahmeadows commented Oct 28, 2017

Tell us about your environment

  • ESLint Version: Online
  • Node Version: N/A
  • npm Version: N/A

What parser (default, Babel-ESLint, etc.) are you using? Default

Please show your full configuration:

Configuration
exports.rules = {
    "no-loop-func": "error"
}

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

See this demo snippet

What did you expect to happen? No error

What actually happened? Please include the actual, raw output from ESLint.

It complained in the closure.

When you change ready to a loop-invariant variable (like i), the warning disappears. Note that in this reduced case (as well as my actual code), the closed-over variable being referenced outside the scope is provably loop-invariant outside the nested callback itself (and thus avoids the usual issues), even though it is modified in child scopes, and this can be discerned without the use of abstract evaluation. (Technically, abstract evaluation would just obliterate it entirely.)


Note: it is out of the scope of this bug whether to allow code like this, where ready is not loop-invariant:

"use strict";
function foo() {
	let create = () => {}
	let values = []
	let ready
	
	for (let i = 0; i < 10; i++) {
		create(() => {
			values = ready
		})
		ready = i
	}

	return () => {
		ready = 10
	}
}

@eslint eslint bot added the triage label Oct 28, 2017

@mysticatea mysticatea added question and removed triage labels Oct 28, 2017

@mysticatea

This comment has been minimized.

Member

mysticatea commented Oct 28, 2017

Thank you for this issue.

However, this is working as intended.

Originally, the rule no-loop-func is literally the rule which disallows all function declarations in loop. The purpose of the rule is avoidance of confusion by the fact that loop-blocks don't make any scope for variables.

for (var i = 0; i < 3; ++i) {
    setTimeout(() => console.log(i)) // 3, 3, 3
}

Online demo

Since ES2015, loop-blocks can make the scope for let and const variables. So the rule got some relaxing. The rule allows functions which don't use variables of outer scopes than the loop.

for (let i = 0; i < 3; ++i) {
    setTimeout(() => console.log(i)) // 1,2,3; OK because the `i` is in the loop-block's scope.
}

let outer = 0
for (let i = 0; i < 3; ++i) {
    setTimeout(() => console.log(outer)) // 3,3,3; Disallowed because the `outer` is not in the loop-block's scope.
    outer += 1
}

for (let i = 0; i < 3; ++i) {
    setTimeout(() => console.log(outer)) // 3,3,3; OK because the `outer` is not modified after this loop. The rule considers the `outer` is a constant.
}

Online demo

In your example, the function is using variables which are in outer scopes than the loop.

@isiahmeadows

This comment has been minimized.

isiahmeadows commented Oct 28, 2017

Oh okay. So I guess I'll keep my workaround of a separate function outside the loop. Could the last two cases and how they differ (where outer is unmodified outside the loop) be clarified in the docs, then?

@mayankraipure

This comment has been minimized.

mayankraipure commented Feb 13, 2018

Use this
/eslint no-loop-func: "off"/
Instead of
/eslint no-loop-func: "error"/

@rachx

This comment has been minimized.

Contributor

rachx commented Mar 19, 2018

Working on this

rachx added a commit to rachx/eslint that referenced this issue Mar 19, 2018

@eslint eslint bot locked and limited conversation to collaborators Sep 21, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.