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

Using intersection with unions #8896

Closed
blakeembrey opened this issue May 31, 2016 · 10 comments
Closed

Using intersection with unions #8896

blakeembrey opened this issue May 31, 2016 · 10 comments
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@blakeembrey
Copy link
Contributor

blakeembrey commented May 31, 2016

TypeScript Version:

1.8 / 1.9

Code

type Foo = { a: string } | { b: string }

interface Bar {
    c: string
}

type Baz = Foo & Bar

let baz: Baz

if (baz.a) {

}

Expected behavior: baz.a should be accessible, at least in 1.9 with the null checks.

Actual behavior: Property a does not exist on type

Edit: To clarify, I suspected it would have let me use it in the if condition in 1.9 since the introduction of strictNullChecks and flow control sounds like it would allow this functionality to pass.

@sandersn
Copy link
Member

I think it's easier to read if you inline and shrink the types:

let baz: ({ a } | { b }) & { c };
if (baz.a) {
  let qux: { a } & { c } = baz;
}

The problem is that, even with control flow analysis, the compiler still doesn't do much "type math". All control flow will do is narrow -- that is, throw out members of a union. What needs to happen here is for the compiler to reach inside the intersection, throw out the nested union member, then create a new intersection type.

I will go see if there are other issues related to this. I think it comes up every couple of weeks.

@ahejlsberg
Copy link
Member

The intersection type doesn't really have anything to do with the behavior here. Consider:

type Foo = { a: string } | { b: string };
let foo: Foo;
if (foo.a) {
}

We currently report an error because a is not present in all constituents of the union type. This is the reason for the error in the original example.

We might consider an alternate behavior in --strictNullChecks mode. We could say that the set of properties is every property in every constituent, with types of similarly named properties unioned together and with undefined added to their types unless the property is present in all constituents. This would make the original example work.

So, for purposes of property access, the type

type Foo = { a: string } | { a: string, b: string } | { a: number, c: number };

would behave like

type Foo = { a: string | number, b: string | undefined, c: number | undefined };

and it would require type guards to access the values in b and c.

@ahejlsberg ahejlsberg added the Suggestion An idea for TypeScript label May 31, 2016
@jeffreymorlan
Copy link
Contributor

jeffreymorlan commented May 31, 2016

That kind of type guard is very error-prone since it depends on the other types in the union not having a property with the same name as an implementation detail:

interface I { a: string; }
interface J { b: string; }

class C implements I {
    a = "foo";
    private b = 42;
}

var c: I | J = new C();

c.b && c.b.toUpperCase(); // would throw TypeError at runtime if allowed

Allowing this would mean you could no longer add or rename an object's internal properties without worrying about whether it might conflict with a union somewhere.

(EDIT: fixed method name)

@blakeembrey
Copy link
Contributor Author

@jeffreymorlan toupper would be caught as a method that does not exist by TypeScript.

@jeffreymorlan
Copy link
Contributor

@blakeembrey I meant toUpperCase

@blakeembrey
Copy link
Contributor Author

blakeembrey commented Jun 1, 2016

I know, but the comment still stands - TypeScript would error on toUpperCase.

Edit: I see your concern. I just tried out some tests.

Edit 2: I know it's probably a bit of work, but maybe it could work with existing flow control then? E.g. if (typeof c.b === 'string') and warn on other usages of it like if (c.b)?

Edit 3: Given all this, I'm not sure it's relevant now. I don't think there's anything stopping it from happening today. If you want to use one side of the union today, you'd need to rely on type coercion which removes type safety already. How are you using types like the one you mentioned above in today's code?

@RyanCavanaugh
Copy link
Member

@blakeembrey I believe we do the distributive rules correctly now - can you confirm?

@blakeembrey
Copy link
Contributor Author

I believe this is classified as unsound unless there's been a recent change to type checking for the x.a property. I understand there'd be no other way to represent this safely today, unless we get Exact types. The only other thing I can think of is having properties that aren't known be an unknown type so typeof x.a === 'string' could work with flow control but x.a would fail with unknown type.

@RyanCavanaugh
Copy link
Member

There's { a: string; b: never } | { a: never; b: string }

@RyanCavanaugh
Copy link
Member

This is allowed today with the (hopefully) clearer if ('a' in baz). We've discussed it a bunch and still don't want to allow truthy checks on possibly-unpresent properties from unions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants