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

4.8.2 regression: type narrowing bug #50706

Closed
lll000111 opened this issue Sep 9, 2022 · 6 comments · Fixed by #50735
Closed

4.8.2 regression: type narrowing bug #50706

lll000111 opened this issue Sep 9, 2022 · 6 comments · Fixed by #50735
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@lll000111
Copy link

lll000111 commented Sep 9, 2022

Bug Report

Instead of words, just the code. The comments say what is wrong and what used to work in 4.7.4.

// Arguments are set to "unknown" because they receive data from network requests
// so runtime checks are mandatory to narrow type before use.
function SendBlob(data: unknown, encoding: unknown) {
    if (encoding !== undefined && encoding !== 'utf8') {
        throw new Error('encoding');
    }

    // Mouse hover to see detected type 
    // When using TS v4.8.2 it is {} | undefined
    // When using TS v4.7.4 it is "utf8" | undefined
    encoding;
};

🔎 Search Terms

"4.8.2" and I found a few but was told to file a new issue because I'm told it wasn't similar to the ones that I found and fought were similar.

I thought e.g. #50671 looks similar, not for the code but possibly for the deeper cause, since I don't think that many things changed in exactly this part of TS since 4.7.4.

🕗 Version & Regression Information

4.7.4 works

4.8.2 doesn't work

⏯ Playground Link

https://www.typescriptlang.org/play?ts=4.8.2#code/GYVwdgxgLglg9mABAZQKZgCYCEA2cBGAFBgIZQkBci4A1mHAO5gA0i6EcGMYA5lbfSYBKRAG8AsAChEMxDGCJC7Ttx6IAhAF5N1TKmDdUGRADITbSCt4btiAOQgowABx2RE6bK9QAFgCdGRDBUBkQAUT8Av0I7ZS5eNwBuKS8AXykU2QB6LMQAWTgQAGdURB84ADdUP0QoOEQS0oxUKFRoI1qATwAHUsyZHMQAdR90aiLVRAAVZEQKgBYAOmdFgCY5KDkisVTEAB9dZoNgjH7EQZGx4smZuaWAdkX5ja3EACJHFzf9w-1DU88MjiqmSklSoKAA

@andrewbranch
Copy link
Member

Hah, this is a fun one. Switch the order of your checks:

if (encoding !== 'utf8' && encoding !== undefined) {

Narrowing unknown has several special rules. A new one in 4.8 is that unknown can break down into a union of {} | null | undefined. That’s triggered as soon as you filter undefined out of it in your first check, and at that point, you’re left with {} | null, and so the next narrowing step can only discriminate between those two things. But if you reverse the order, a different set of special rules around strict equality in general takes effect first.

@andrewbranch andrewbranch added the Bug A bug in TypeScript label Sep 9, 2022
@andrewbranch andrewbranch added this to the TypeScript 4.9.0 milestone Sep 9, 2022
@lll000111
Copy link
Author

lll000111 commented Sep 9, 2022

Ah I have another one, single check for undefined condition:

Playground

function doSomething<T extends unknown>(value: T): T {
    if (value === undefined) {
        return value;
    }

    // ERROR: This condition will always return false because...
    if (value === 42) {
        throw Error('Meaning of life value');
    }

    return value;
}

Or, without generics, without error but the type is wrong regardless, simply

Playground

function doSomething(value: unknown): void {
    if (value === undefined) {
        return;
    }

    if (value === 42) {
        // 4.8.2: Type is {}
        // 4.7.4: Type is 42
        value;
    }
}

@andrewbranch
Copy link
Member

Yeah, that’s the exact same issue.

the type is wrong regardless

Not wrong, just not as specific as it could be. 42 is assignable to {}. {} just means not null or undefined. 42 is not null or undefined.

@ahejlsberg
Copy link
Member

Actually, the issue reported here is a separate and unrelated issue. In 4.8, the check for undefined narrows the type of T to T & ({} | null), but that should still be comparable to 42.

@lll000111
Copy link
Author

lll000111 commented Oct 6, 2022

@ahejlsberg This has not been fixed in the new 4.8.4. If I follow the "closed as completed" link to a merge it shows 4.8.4 as the target release for that merge though.

There is 4.9.0 as "Milestone" on this issue, but the link to PR #50735 as the latest log entry just above has 4.8.4?

@ahejlsberg
Copy link
Member

I think the intent was to fix it in 4.8.4, but you're right, it isn't in that release. It's definitely fixed in 4.9.

@andrewbranch Looks like there was a failed cherry pick into 4.8.4, but not sure if that was the reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants