[change] Only report first instance of unreachable code

[adamreisnz]

In ESLint 2.11.1 (bundled with Atom linter-eslint), the error for no-unreachable marks every single line of code after the return statement. Maybe this is redundant and it should only mark the first instance of unreachable code? It's pretty obvious that everything after that line will also be unreachable.

Illustration:

Perhaps we could control this via a flag/setting, in case some people do prefer to have every line marked.

[platinumazure]

I think it's worth discussing for sure. I think having it be an option will make the enhancement more palatable.

@eslint/eslint-team Thoughts?

[adamreisnz]

I find that one problem with marking every line was that it lead to "error blindness", e.g. I could not spot an actual error anymore because I was trying to ignore all the irrelevant duplicate errors.

[nzakas]

I can support this as an option, but think we should keep the current default behavior. 👍

@adambuczynski are you willing to implement this change if others agree?

[adamreisnz]

@nzakas yeah I could have a look

[mysticatea]

Hm...

At first, I'm sorry that I have not implemented #3307.
But what about that it merges warnings of continuous nodes?
If #3307 is implemented, a warning can show range. Then,

var startNode = null;
var endNode = null;

function reportIfUnreachable(node) {
    var nextNode = null;

    if (node && currentCodePath.currentSegments.every(isUnreachable)) {
        // Store this statement to distinguish consecutive statements.
        if (!startNode) {
            startNode = endNode = node;
            return;
        }

        // Skip if this statement is inside of the current range.
        if (startNode.range[0] <= node.range[0] && node.range[1] <= endNode.range[1]) {
            return;
        }

        // Skip if the previous token of this statement is inside of the current range.
        // This is meant that this statement is consecutive from the current range.
        var prev = context.getTokenBefore(node);
        if (startNode.range[0] <= prev.range[0] && prev.range[1] <= endNode.range[1]) {
            endNode = node;
            return;
        }

        nextNode = node;
    }

    // Report the current range since this node and the current range are not consecutive.
    if (startNode) {
        context.report({
            message: "Unreachable code.",
            loc: {start: startNode.loc.start, end: endNode.loc.end},
            node: startNode
        });
    }

    // Update the current range.
    startNode = endNode = nextNode;
}

[nzakas]

@mysticatea I think the problem with that is it will skip over blank lines and comments, so we will still have multiple warnings.

[mysticatea]

@nzakas It checks whether the previous token of a statement node is included in the current unreachable range, in order to distinguish that a node is consecutive from the current range. So blank lines and comments are not a barrier. e.g.

function foo() {
    return;

    a();  // ← ERROR: Unreachable code. (no-unreachable)

    b()   // ↑ ';' token is included in the unreachable code, so this statement will be merged.
    // comment
    c();  // ↑ ')' token is included in the unreachable code, so this statement will be merged.
}

This is a screenshot of a prototype.

---

If there are unreachable codes non-consecutive, there are multiple warnings:

[adamreisnz]

That looks good to me

[nzakas]

@mysticatea that looks good. Is that with a new option or by default?

[mysticatea]

I think it's good for default behavior.
CLI does not show the range of problems, but people can distinguish whether unreachable code(s) exist or not.

[nzakas]

Sounds good. We should make sure to update the docs to make it clear what this new behavior is.

@eslint/eslint-team we need a couple more 👍s here.

[markelog]

SGTM

[ilyavolodin]

👍

[nzakas]

Assuming that @markelog's "SGTM" is the same as 👍, marking as accepted.

[adamreisnz]

Thanks guys 🎉