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

Poor narrowing of contextual unions that don't share properties #48460

Closed
erikbrinkman opened this issue Mar 28, 2022 · 4 comments
Closed

Poor narrowing of contextual unions that don't share properties #48460

erikbrinkman opened this issue Mar 28, 2022 · 4 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@erikbrinkman
Copy link
Contributor

Bug Report

In cases with discriminant unions that don't fit the conventional form (e.g. one property that's constant among all union members) type discrimination doesn't happen as one might expect or desire, see the code example.

I tried to search to see if this had been talked about before, but couldn't find anything. I imagine the current design was around capturing a common pattern in js while not spending too much time trying to narrow the types.

To address this in the same framework: discriminateTypeByDiscriminableItems and isDiscriminantProperty would have to be tweaked to handle missing properties and it's not clear how that would work with exactOptionalPropertyTypes. Alternatively, the existing code may work by first filtering the union down to feasible assignments based on the exitance of properties, e.g. only consider types that the object literal has every non-optional property. However, doing that correctly would require constructing a filtered union type and I'm not sure if there's precedent for that.

Doing any of this will likely cause a performance penalty, so I think it's also fair to say this pattern is infrequent enough to not care about getting correct.

🔎 Search Terms

discriminant unions narrowing

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about 3.3

⏯ Playground Link

Playground link with relevant code

💻 Code

type DiscriminatorA = {
    a: true;
    cb: (x: string) => void;
}

type DiscriminatorB = {
    b: true;
    cb: (x: number) => void;
}

declare function f(options: DiscriminatorA | DiscriminatorB): void;

f({
    b: true,
    cb: n => n.toFixed()
});

🙁 Actual behavior

Parameter 'n' implicitly has an 'any' type.(7006)

🙂 Expected behavior

type checks successfully

@RyanCavanaugh
Copy link
Member

This is fully intentional because object types aren't sealed - it's perfectly legal to have

const m = {
  a: true,
  b: true,
  cb: (x: string) => null
};
const s: DiscriminatorA = m;

Whether f invokes this function with a string or number is not predictable based on the types. If you "block out" the other discriminants to prevent that from happening, this works as hoped:

type DiscriminatorA = {
    a: true;
    b?: never;
    cb: (x: string) => void;
}

type DiscriminatorB = {
    a?: never;
    b: true;
    cb: (x: number) => void;
}

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Mar 28, 2022
@erikbrinkman
Copy link
Contributor Author

I understand they're not sealed, but as written, the object literal

{
    b: true,
    cb: n => n.toFixed()
}

is not assignable to DiscriminatorA. I'm not saying it should generally reject one or the other, I'm saying when called with a literal, it should be able to infer the function type in these cases.

@RyanCavanaugh
Copy link
Member

It could be made to work for that case, but raises other questions like what to do with this

f({
    a: true,
    b: true,
    cb: n => n // string | number? string & number?
});

Overall I think your assessment of trade-offs is correct -- this pattern doesn't really work outside this immediate example and isn't worth spending cycles on

@erikbrinkman
Copy link
Contributor Author

In the example above, the current behavior seems correct, you can't narrow the type in the union. Whether you would then infer something about the unknown property seems like a different issue.

There are some potential cases where this comes up due to not having a discriminable member e.g. this playground, and more so came up in picking optional properties in the same context (#48448). But given your assessment, I'm cool with closing until/if there's an actual use-case that justifies it.

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

2 participants