Unreachable 'for' clause doesn't correctly narrow

[zpdDG4gta8XKpMCd]

nightly build Oct 24, 2016

// works as expected
function shouldNodeBeConsidered(ruleKey: string, node: ts.Node): boolean {
    for (let current: ts.Node | undefined = node; current !== undefined; current = current.parent) { // works
    }
}

// weirdly doesn't work
function shouldNodeBeConsidered(ruleKey: string, node: ts.Node): boolean {
    for (
        let current: ts.Node | undefined = node;
        current !== undefined;
        current = current.parent // <-- [ts] Object is possibly 'undefined'.
                                 //          let current: ts.Node | undefined

     ) {
        if (shouldImmediateNodeBeConsidered(ruleKey, current)) {
            return true;
        } else {
            continue;
        }
    }
    return false;
}

[masaeedu]

@Aleksey-Bykov Not super familiar with the flow based type checking, but did you mean to have an if statement there?

        let current: ts.Node | undefined = node;
        if (current !== undefined) {
            current = current.parent;
        }

[RyanCavanaugh]

@masaeedu the comparison is the middle clause of a for loop, not an expression statement

[RyanCavanaugh]

@Aleksey-Bykov I can't reproduce this. As usual, we'd really prefer a self-contained repro and a descriptive title on bugs

[masaeedu]

@RyanCavanaugh My bad. I can reproduce this, you need to enable strictNullChecks. I'll set up a repo.

[masaeedu]

@RyanCavanaugh https://github.com/masaeedu/typescript11821

[zpdDG4gta8XKpMCd]

@RyanCavanaugh updated the title,
what is a self contained repro? is it something git issue templates are for? didn't i follow it?

[zpdDG4gta8XKpMCd]

it's like you are giving me a favor of considering bugs that i found in your product, sorry for doing it and making you upset

[RyanCavanaugh]

function foo() {
    for (let current: string | undefined = 'oops'; true; current = current.substr(1)) {
    // Incorrect error in third clause
        return;
    }
}

[RyanCavanaugh]

@Aleksey-Bykov apology accepted

[masaeedu]

@RyanCavanaugh Isn't the error only incorrect in the case where the middle clause narrows the loop variable?

[RyanCavanaugh]

The narrowing of the middle clause is irrelevant because we never assign anything possibly-undefined to current in the first place

[masaeedu]

@RyanCavanaugh Well in the original case current.parent, and in your case current.substr (I assume), can return undefined. It is true that in your case you have a return that prevents the final loop clause, but this isn't just a problem with returns, you get the problem if you use continue as well, in which case the final loop clause will be evaluated. I added some more scenarios to the repo.

[zpdDG4gta8XKpMCd]

@RyanCavanaugh, you made a repro that doens't really cover the reported case: notice that the first snippet works, the problem reveals itself in the presence of a body

[RyanCavanaugh]

substr can't return undefined.

More accurately, this seems to happen when the end flow point of the for loop body is unreachable.

[zpdDG4gta8XKpMCd]

oh, you used the return for a body, nevermind

[masaeedu]

@RyanCavanaugh Here's what I'm trying to say

interface Node {
    parent: Node | undefined;
}
// This currently fails to compile. After the fix it SHOULD compile
function foo1(node: Node): boolean {
    for (let current: Node | undefined = node; current !== undefined; current = current.parent) {
        continue;
    }
    return false;
}
// This currently fails to compile. After the fix it SHOULD NOT compile
function foo2(node: Node): boolean {
    for (let current: Node | undefined = node; 1 === 1; current = current.parent) {
        continue;
    }
    return false;
}

The second case should fail because the conditional clause does not do the appropriate narrowing, regardless of whether the bottom of the loop body is reachable.

[RyanCavanaugh]

The second case shouldn't fail. There are no assignments to current which don't have an RHS of a non-undefined type. See how this does compile without error:

interface Node {
    parent: Node;
}
function foo2(node: Node): boolean {
    for (let current: Node | undefined = node; 1 === 1; current = current.parent) {
        // continue;
    }
    return false;
}

Note that there should never be a difference between a for loop like this

for(a; ; c) {
  // code
  // block-terminal continue
  continue;
}

and a for loop like this (all other expressions being equal)

for(a; ; c) {
  // code
}

[masaeedu]

There are no assignments to current which don't have an RHS of a non-undefined type.

current.parent is Node | undefined in the example I gave you (not Node), so there is one assignment to current which does not have an RHS of a non-undefined type.

We're travelling up the parent chain of a Node: eventually you'll run out of parents and get undefined. If you don't have a condition that ensures you stop iterating at that point, there is no guarantee that current is not undefined when the final clause is evaluated.

[masaeedu]

@RyanCavanaugh You can verify that the latter fails at runtime where the former does not:

interface N {
    parent?: N;
}

let n1: N = {}, n2: N = {};
n1.parent = n2;

// This currently fails to compile. After the fix it SHOULD compile
function foo1(node: N): boolean {
    for (let current: N | undefined = node; current !== undefined; current = current.parent) {
        continue;
    }
    return false;
}

// This currently fails to compile. After the fix it SHOULD NOT compile
function foo2(node: N): boolean {
    for (let current: N | undefined = node; 1 === 1; current = current.parent) {
        continue;
    }
    return false;
}

console.log('foo1: ', foo1(n1)); // false
console.log('foo2: ', foo2(n1)); // Uncaught TypeError: Cannot read property 'parent' of undefined

[RyanCavanaugh]

Forward-duping this to #26914