Skip to content

Excess discriminated types match all discriminable properties #32755

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

Merged

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Aug 7, 2019

Excess property checks only use discriminated types when exactly 1 type's properties from the target are related to all of the discriminable properties of the source. However, if more than 1 type from the target matches any property, we give up because there might be 2 discriminated types from the target union.

This change actually checks whether all properties of all types in the target union are related to the discriminable properties of the source. This makes the list of discriminated type === 1 more often, which in turn makes excess property checks use a discriminated type more often.

I'm not sold on the value of this change vs its complexity. Opinions are welcome.

Fixes #31655
Fixes #32657

… properties

This allows fewer types to be discriminated in excess properties, which
fixes some examples.
@sandersn sandersn requested review from weswigham and orta August 7, 2019 20:28
@weswigham
Copy link
Member

#32711 ?

@orta
Copy link
Contributor

orta commented Aug 8, 2019

I can't attest too much to the complexity, it doesn't seem too complex to me but this feels like a useful error message 👍

@sandersn
Copy link
Member Author

sandersn commented Aug 8, 2019

@weswigham Yep! #32711 is the same fix! Thanks for pointing that out. I still like this fix better, so I asked @jack-williams to take a look and see what he thinks.

@jack-williams
Copy link
Collaborator

jack-williams commented Aug 8, 2019

@sandersn In short I think this is better and should be merged over mine.

If I understood the old code correctly: the requirement to discriminate to a type wasn't to match all discriminant properties, only that any matched properties were always from the same union constituent. I tried replicating this behaviour so that you didn't need to match all properties, just that all matched properties had an unambiguous candidate.

I'm not actually sure this requirement is necessary; instead, requiring that all properties are matched (by a single type) seems to deliver the same results with cleaner code. Presumably if no candidate matches all discriminants then an error will get flagged later on because the union is unassignable to.

@sandersn
Copy link
Member Author

sandersn commented Aug 8, 2019

Ah, I see. Yeah, I think my simpler definition is fine even if it's a bit stricter. And you are right that we'll catch any real assignability errors later on.

@sandersn sandersn merged commit b24050a into master Aug 8, 2019
@sandersn sandersn deleted the excess-discriminated-types-match-all-discriminable-properties branch August 8, 2019 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants