Skip to content

Narrowed variable types become widened in functions defined after narrowing #50580

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
mattrunyon opened this issue Sep 1, 2022 · 5 comments · Fixed by #56908
Closed

Narrowed variable types become widened in functions defined after narrowing #50580

mattrunyon opened this issue Sep 1, 2022 · 5 comments · Fixed by #56908
Labels
Duplicate An existing issue was already created

Comments

@mattrunyon
Copy link

Bug Report

🔎 Search Terms

possibly undefined array methods predicate mutable variables narrowing

🕗 Version & Regression Information

4.5.4

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about functions and method "bugs" that aren't bugs

⏯ Playground Link

Playground link with relevant code

💻 Code

function test(param?: number): number {
    let a: number | undefined;

    // Some code that could make a defined goes here

    if (a === undefined || param === undefined) {
        return 0;
    }

    const b = () => a + param; // a is not narrowed. param is narrowed

    function c() {
        // Neither is narrowed
        return a + param;
    }

    [1, 2, 3].map(elem => elem + a + param); // a is not narrowed

    return a + param; // Both narrowed
}

🙁 Actual behavior

The variables are not narrowed when used in functions. param is narrowed when using an arrow function. This causes friction when trying to use an array method that takes a function as an argument

🙂 Expected behavior

Both variables are narrowed to number in all cases

@guillaumebrunerie
Copy link

This is by design, see #9998 and the template "Types not correct in/with callbacks" when creating a new issue. The basic reason is that the narrowing could have become invalid by the time the function gets invoked.

@mattrunyon
Copy link
Author

Ahh that makes sense. The behavior still seems inconsistent with parameters and variables in arrow functions. Why is param narrowed in the arrow function definition? Shouldn't it also be its original type? They're both possibly undefined in the regular function definition.

@guillaumebrunerie
Copy link

Indeed, not sure why it behaves differently for parameters and variables.

@andrewbranch
Copy link
Member

For const variables, we know the value can never change, so if narrowing applies in one place, it’s safe to keep that narrowing inside functions too. If you replace let a with const a = undefined as number | undefined, you’ll see that a keeps its narrowing in the arrow function. We assume that let variables will change, so that narrowing gets undone. Because parameters are not annotated with let or const, we do some quick syntactic analysis to determine whether a parameter is ever reassigned, and if not, we are safe to treat it like a const.

Since function declarations get hoisted, we defer checking them, so I think it may just be challenging to figure out where to put the body of c into the control flow graph of the function it’s in. I’m not 100% sure why the narrowing doesn’t or can’t apply in there, but given that we don’t protect you from TDZ issues either (try adding a call to c() as the first line of test), the limitation seems kind of reasonable. I’m sure there’s an issue somewhere digging more into that particular case.

@andrewbranch andrewbranch added the Duplicate An existing issue was already created label Sep 7, 2022
@typescript-bot
Copy link
Collaborator

This issue has been marked as a 'Duplicate' 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
Duplicate An existing issue was already created
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants