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

Type relationships for intersections with union constraints #23672

Merged
merged 9 commits into from
Apr 27, 2018

Conversation

ahejlsberg
Copy link
Member

This PR fixes two issues:

  • Type relationships for intersection types with combined constraints that are union types are now handled properly.
  • Intersections of primitive types with mutually exclusive value domains (e.g. string & number) are now removed from union types. Previously we only removed intersections containing more than one unit type.

An example of an intersection type with a union type constraint:

function f1<T extends string | number, U extends string | number>(x: T & U) {
    let y: string | number = x;  // Ok, but was error previously
}

Fixes #23648.

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our RWC need a fix from the keyof change from the other day, but we should check if this affects RWC/DT even more.

@@ -3613,6 +3613,7 @@ namespace ts {
BooleanLike = Boolean | BooleanLiteral,
EnumLike = Enum | EnumLiteral,
ESSymbolLike = ESSymbol | UniqueESSymbol,
VoidLike = Void | Undefined,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh.... can we go with the term UndefinedLike instead? Since void is a TS-only and, conceptually, corresponds with a function return of no value, which maps to a runtime value of undefined (whereas undefined doesn't really imply anything void-y on its own).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I went with void-like because void is the supertype, but either way works.

function getUnionConstraintOfIntersection(type: IntersectionType) {
let constraints: Type[];
for (const t of type.types) {
if (t.flags & TypeFlags.Instantiable) {
Copy link
Member

@weswigham weswigham Apr 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think non-instantiable types need to contribute to the "constraint" we check against as well (as themselves). Eg,

type Something = number | object | undefined;

function foo<T extends string | number | undefined>(x: T & Something) {
    const y: number | undefined = x;
}

should typecheck, but even with this PR does not. We get the constraint of T & object as just string | number | undefined (rather than never, as we would if we carried the object through and simplified (string | number | undefined) & object), get Ternary.False, then report an error. ☹️ (Whereas never would pass, then we'd move on to T & number, which would simplify to number in this check, be true, then T & undefined, which would simplify to undefined and be true, causing the overall check to pass.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we need to handle this as well. Latest commits should get us there.

@@ -3613,6 +3613,9 @@ namespace ts {
BooleanLike = Boolean | BooleanLiteral,
EnumLike = Enum | EnumLiteral,
ESSymbolLike = ESSymbol | UniqueESSymbol,
VoidLike = Void | Undefined,
/* @internal */
DisjointDomains = NonPrimitive | StringLike | NumberLike | BooleanLike | ESSymbolLike | VoidLike | Null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Object types can also have disjoint domains, eg

type A = { x: string };
type B = { x: number };
type C = { x: boolean };
function f5<T extends A | B>(x: T & (B | C)) {
    const y: B = x; // expected to work, currently does not, doesn't work with this PR
}

(important if you just consider each of those types of x as different tags for tagged unions, eg "a", "b", and "c")

What we have here is certainly better, but definitely still not quite complete.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the intent here is simply to do better for the primitive types. I don't think it is feasible to reason about disjoint domains for arbitrary object types since they can be recursive or infinite. Also, we still allow stuff like string & { __tag__?: void } even though that's technically an impossible type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we still allow stuff like string & { tag?: void } even though that's technically an impossible type.

I feel like you can justify that by claiming that {} types indicate structure, but no top level domain (which is mostly true - { toString(): string } matches primitives AFAIK), so it's just string-augmented-with-extras. So it doesn't feel so bad.

Also, it's probably feasible for object types - we do compare them based on that structure, after all, and we're even within that comparison when this simplifier/inliner gets called. And it's not like { x: number } & { x: string } should simplify to never - it should be { x: never } (and { x: number } | { x: never } should be { x: number }) - it's exactly what we'd do when we actually compare the properties (or asked for obj.x), but not deferred until access so that the top-level union/intersection relationship works out (and this is all about doing union/intersection simplification more eagerly to make comparisons work, so it kinda fits right in).

I don't suppose we need to fix it as part of this (after all, it's already better than master), but we should at least add the test and know that the fix is incomplete, so we can either revisit it or keep its behavior stable.

@zpdDG4gta8XKpMCd
Copy link

will it fix this: #20375 ?

@mhegazy
Copy link
Contributor

mhegazy commented Apr 27, 2018

will it fix this: #20375 ?

nope, that is a different issue.

@ahejlsberg ahejlsberg merged commit 6c28da3 into master Apr 27, 2018
@ahejlsberg ahejlsberg deleted the intersectionWithUnionConstraint branch April 27, 2018 22:53
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug from #23592 - Type 'keyof U & K' does not satisfy the constraint 'string | number | symbol'
4 participants