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

New rule proposal: no-unreachable-loop #12381

Closed
mdjermanovic opened this issue Oct 5, 2019 · 9 comments · Fixed by #12660
Closed

New rule proposal: no-unreachable-loop #12381

mdjermanovic opened this issue Oct 5, 2019 · 9 comments · Fixed by #12660
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules

Comments

@mdjermanovic
Copy link
Member

Please describe what the rule should do:

Disallows loops which body exits the loop in all paths and therefore can never even reach the test condition for the second time (or first time if it's do-while), meaning that it isn't actually a loop.

Targets while, do-while, for, for-in and for-of loops.

What category of rule is this? (place an "X" next to just one item)

[X] Warns about a potential error (problem)
[ ] Suggests an alternate way of doing something (suggestion)
[ ] Enforces code style (layout)
[ ] Other (please specify:)

Provide 2-3 code examples that this rule will warn about:

while (foo) {
    doSomething(foo);
    foo = foo.parent;
    break;
}

for (i = 0; i < arr.length; i++) {
    if (isSomething(arr[i])) {
        break;
    } else {
        return false;
    }
}

do {
    if (foo.type === "bar") {
        return foo;
    } else {
        throw new Error("invalid");
    }
} while (foo);

for (foo of bar) {
    if (foo.type === "bar") {
        doSomething(foo);
    }
    break;
}

for (foo in bar) break;

The code is correct only if the end of the body is reachable (i.e. loops from the end) or the loop has a continue statement.

Why should this rule be included in ESLint (instead of a plugin)?

It's a very possible error in the code, something is missing or something else is at the wrong place.

Even if it isn't an error the code should be refactored to avoid unnecessary loop syntax.

This is a similar type of error as no-unreachable, but I think this should be a separate rule because there is no particular unreachable code in these cases (except for the code in do-while test and for-loop update, but I think that no-unreachable doesn't report that).

In some cases both no-unreachable and this rule can report warnings (e.g. no-unreachable for code after the last break, this rule for the whole loop), but I think that isn't overlapping.

Are you willing to submit a pull request to implement this rule?

Yes, I'd love to work on this.

@mdjermanovic mdjermanovic added triage An ESLint team member will look at this issue soon rule Relates to ESLint's core rules feature This change adds a new feature to ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Oct 5, 2019
@mdjermanovic mdjermanovic self-assigned this Oct 5, 2019
@ljharb
Copy link
Sponsor Contributor

ljharb commented Oct 5, 2019

For while or do or for, this makes sense to me - but for..in is occasionally (much more often historically) used like this:

for (var k in obj) {
  k = true;
  break;
}
if (k) {
  // object has enumerable properties
}

The same could be true of for..of - both for..in and for..of are not inherently unnecessary here since they perform iterations.

@mdjermanovic
Copy link
Member Author

That's correct, I think there should be options to turn off for-in and for-of checks for this reason.

An alternative for an user is to extract that code to a function (hasEnumerableProperties or getFirstEnumerableName) and eslint-disable just that one line, or instead use Object.keys over prototype chain in the function. Similar for for-of, an alternative could be to access the iterator directly, or do [first] = foo if undefined isn't an expected value. But, all of this would be probably inconvenient for an existing codebase.

I'd still vote that the rule checks for-in and for-of as it could catch some real errors, but behind an option (or options).

@ljharb
Copy link
Sponsor Contributor

ljharb commented Oct 6, 2019

Most configurable would be an option for each construct, regardless of the defaults.

@mdjermanovic
Copy link
Member Author

I agree, one option for for-in and another for for-of.

Or did you mean all 5 options, for, while and do as well?

@ljharb
Copy link
Sponsor Contributor

ljharb commented Oct 16, 2019

Each option separately.

@mdjermanovic
Copy link
Member Author

Any thoughts from the team about this rule? :-)

It looks to me that using the already existing code path analysis features this rule could reliably (without false negatives or false positives) detect these bugs in the code, and might be a nice complement to the no-unreachable rule.

@kaicataldo
Copy link
Member

@ilyavolodin Mind clarifying if your thumb on the comment above is in support of this proposal? If so, we can mark this as accepted :)

@mdjermanovic
Copy link
Member Author

One more 👍 to accept, if thumb on the comment doesn't count :)

@ilyavolodin ilyavolodin added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Dec 10, 2019
@mdjermanovic
Copy link
Member Author

I'm working on this.

nzakas pushed a commit that referenced this issue Jun 13, 2020
* New: Add no-unreachable-loop rule (fixes #12381)

* Add fallthrough switch test
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Dec 11, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants