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

Bugfix/union excess property check #54823

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

yin
Copy link
Contributor

@yin yin commented Jun 29, 2023

Fixes #54784

We'd like to improve the behavior of error messages for Union types. Currently, the compiler does not detect that object literal {a: ..., b: ...} can't be assigned to either type in the union A|B:

type A = {a:number};
type B = {b:number};

let x: A|B  = {a:0, b:0};

There is another #12936 which aims to solve the problem in general.

Please, do not merge yet.

Matej Sadovsky added 4 commits June 28, 2023 14:06
There is a bug microsoft#54784, which causes union types to behave inconsistenly when
compared to the original types.
If the target type is an union, we'll check each type in the union.
If any of the types ar assignable without excess properties, then
that's the solution.

We'll find those properties, which are assignable in at least one
type and report those as causing the Object literal to be
unassignable.
only in some cases in the union. The check was only performed when
reporting was enbaled, which it isn;t in the first run of the funtion.

This time it's done just after truly excess proprties have been
reported.

----

TODO:

- [ ] If there are excess properties in the intersection of the
  union types, then we'll receive the usual error only. If there
  are properties which will cause the obejct literal still being
  unassignable, these will not be reported yet.

  Function `hasExcess{Properties` might need splitting. But for
  now, it's acceptable.
@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jun 29, 2023
@MartinJohns
Copy link
Contributor

Fixes #54784

Better to point to the open issue instead of the duplicate: #39050

There is another #12936 which aims to solve the problem in general.

Please don't confuse exact types with excess property checks. They solve different problems (although they're slightly overlapping).

@fatcerberus
Copy link

@MartinJohns
For what it's worth, I pointed OP to the exact types issue when they asked about making excess property checks comprehensive "in certain situations": #54784 (comment)
But yes, they are indeed distinct.

@fatcerberus
Copy link

@yin Continuing discussion from #54784 (comment)...

I'm not a TypeScript maintainer; it'd be better to discuss that kind of thing (error message wording, breaking changes, etc.) with someone on the team. As for the Flow example you gave, the behavior there seems straightforward enough, though I am concerned about performance and I'm sure the maintainers will be also. O(N*M) is effectively quadratic and the whole check itself is potentially cubic (number of types * number of properties of each * size of object literal) which could get ugly fast.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Jun 29, 2023

type A = {a:number};
type B = {b:number};

let x: A|B  = {a:0, b:0};

This being allowed is intentional behavior. Clearly an A & B is a legal A | B per the definition of both operators, and that's what's happening in this example.

Excess property checks aren't intended to be a replacement for exact types. The goal here, generally speaking, is to prevent typos in property names. Code which is legal per the type system and doesn't contain an identifiable error should not be rejected.

@fatcerberus
Copy link

fatcerberus commented Jun 29, 2023

Clearly an A & B is a legal A | B

Devil’s advocate is that this reasoning applies equally to A and B individually and yet EPC rejects a (literal) A & B in that case. It’s hard to tell where to draw the line.

@fatcerberus
Copy link

To expand on the above: if the formal line to be drawn is “the property exists on any of the union constituents” then that’s fine, but then EPC should probably reject an assignment of A & B & C to A | B to be consistent, which AFAIK it doesn’t.

@yin
Copy link
Contributor Author

yin commented Jul 1, 2023

@fatcerberus

... O(N*M) is effectively quadratic and the whole check itself is potentially cubic (number of types * number of properties of each * size of object literal) which could get ugly fast.

Before optimizing to sub-quadratic, wanted to experiment and discuss the topic. I'll keep this PR Draft until we address all issues. If we decide not to go forward, we might as well improve the baselines and/or docs based on our findings.

Clearly an A & B is a legal A | B

Devil’s advocate is that this reasoning applies equally to A and B individually and yet EPC rejects a (literal) A & B in that case. It’s hard to tell where to draw the line.

There are #20863 and #54784 so the community is confused and it's not explained.

If TypeScript would do runtime type-checking and exact-type semantics, then A & B not be a legal A | B, because it's neither a legal A, nor B.

My reasoning is that users don't expect current behavior. The reasons are not clear to them. They expect the Union type to behave as a true disjoint union and a dynamic one.

I think It's OK to define A & B to satisfy A | B. But it should be in the documentation.

@yin
Copy link
Contributor Author

yin commented Jul 1, 2023

@RyanCavanaugh I have read your comment, which motivated me to experiment with this code: #12936 (comment)

Can you elaborate on what you meant with we'd prefer to fix that by making EPC smarter. in your comment about EPC?

@yin
Copy link
Contributor Author

yin commented Jul 2, 2023

If TypeScript would do runtime type-checking and exact-type semantics, then A & B not be a legal A | B, because it's neither a legal A, nor B.

Let me re-phrase: A and B are types and A|B semantics is either A or B. A & B is neither type A, nor type B. On the other hand, if A and B were treated like say two sets of potentially overlapping (and/or conflicting) properties, then A | B can be treated as "set union"? A & B would not produce any excess properties then.

@fatcerberus
Copy link

fatcerberus commented Jul 2, 2023

IMO it’s not really useful to think of | and & in terms of their effect on object properties. If types are considered as sets of values, then A | B is precisely the set union of A and B, while A & B is precisely the set intersection. It’s true that & tends to create types which have the properties of both members of the intersection, but here’s the key: that is a side effect of the type system allowing excess properties in general (the set of legal As includes objects that also have properties of B and vice versa), creating overlap. If A and B were exact types, then intersecting them would tend to produce empty types isomorphic to never, since there’d be no overlap in their allowed values.

@yin
Copy link
Contributor Author

yin commented Jul 5, 2023 via email

@htunnicliff
Copy link

@yin would this bugfix address union EPC with tuple types, such as those in this example: #20863 (comment)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lack of expected compiler error with type declaration involving a type union
6 participants