Skip to content

strictNullChecks: false positive with switch in for loop #27239

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

Closed
AlCalzone opened this issue Sep 20, 2018 · 6 comments
Closed

strictNullChecks: false positive with switch in for loop #27239

AlCalzone opened this issue Sep 20, 2018 · 6 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@AlCalzone
Copy link
Contributor

TypeScript Version: 3.1.0-dev.20180920

Search Terms: strictNullChecks

Code
This might seem weird, but is based on actual real-world code.

declare function baz(): Promise<true | "a" | "b" | "c" | Error>;

async function foo() {
    let bar: "x" | Error;
    for (let i = 0; i < 3; i++) {
        const test = await baz();
        switch (test) {
            case true: return;
            case "a": throw new Error("a");
            case "b": {
                bar = "x";
                continue;
            }
            default: {
                if (test instanceof Error) {
                    bar = new Error("foo");
                    continue;
                } else {
                    throw new Error("c");
                }
            }
        }
    }
    // false positive: bar is used before being assigned
    if (bar === "x") {

    } else {
        bar.message = "something";
    }
}

Expected behavior:
No error after the for loop. The only way to get there is if bar has the value "x" or is an instance of Error. If baz resolves to true, the function returns, "b" sets bar to "x", and "a" and everything that is not an Error (including "c" throw, and an Error sets it to an Error.

Actual behavior:
Variable "bar" is used before being assigned.

Playground Link: https://www.typescriptlang.org/play/#src=declare%20function%20baz()%3A%20Promise%3Ctrue%20%7C%20%22a%22%20%7C%20%22b%22%20%7C%20%22c%22%20%7C%20Error%3E%3B%0D%0A%0D%0Aasync%20function%20foo()%20%7B%0D%0A%20%20%20%20let%20bar%3A%20%22x%22%20%7C%20Error%3B%0D%0A%20%20%20%20for%20(let%20i%20%3D%200%3B%20i%20%3C%203%3B%20i%2B%2B)%20%7B%0D%0A%20%20%20%20%20%20%20%20const%20test%20%3D%20await%20baz()%3B%0D%0A%20%20%20%20%20%20%20%20switch%20(test)%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20case%20true%3A%20return%3B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20case%20%22a%22%3A%20throw%20new%20Error(%22a%22)%3B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20case%20%22b%22%3A%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20bar%20%3D%20%22x%22%3B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20continue%3B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20%7D%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20default%3A%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20if%20(test%20instanceof%20Error)%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20bar%20%3D%20new%20Error(%22foo%22)%3B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20continue%3B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%7D%20else%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20throw%20new%20Error(%22c%22)%3B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%7D%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20%7D%0D%0A%20%20%20%20%20%20%20%20%7D%0D%0A%20%20%20%20%7D%0D%0A%20%20%20%20if%20(bar%20%3D%3D%3D%20%22x%22)%20%7B%0D%0A%20%20%20%20%20%20%20%20%0D%0A%20%20%20%20%7D%20else%20%7B%0D%0A%20%20%20%20%20%20%20%20bar.message%20%3D%20%22something%22%3B%0D%0A%20%20%20%20%7D%0D%0A%7D

Related Issues: #24091

@ghost
Copy link

ghost commented Sep 20, 2018

We don't check that the for loop is run at least once.

function f(): number {
    let bar: number;
    for (let i = 0; i < 1; i++) {
        bar = 0;
    }
    return bar; // Error
}

When the for loop uses number literals we could check that it runs at least once, but I don't think that's a common enough case to justify special handling.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Sep 24, 2018
@AlCalzone
Copy link
Contributor Author

We don't check that the for loop is run at least once.

Would you consider adding a way to assert "I definitely know this for loop will be run at least once"?

@ghost
Copy link

ghost commented Oct 31, 2018

We have a catch-all way to assert things like this: let bar!: "x" | Error;. See also #9998

@AlCalzone
Copy link
Contributor Author

@Andy-MS It seems I forgot to mention a key point. If I change the condition after the loop to the following

    if (bar! === "x") {
        // bar is "x"
    } else {
        // bar is "x" | Error, should be Error
        bar.message = "something";
   }

the type for bar in the else branch includes "x", when it should be an Error instance. The analyzer gets it right when I inline the loop body or do this:

    bar = bar!; // <-- gross!
    if (bar === "x") {
        // bar is "x"
    } else {
        // bar is Error
        bar.message = "something";
   }

What about something like this to tell the compiler to treat the loop body as "inlined"?

for!(...) { // notice the !
    // ...
}

@ghost
Copy link

ghost commented Nov 1, 2018

That's because type narrowing doesn't work on non-null assertion expressions; see #15655

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

3 participants