Skip to content

Don't use a cached union/intersection type if there is an aliasSymbol #17349

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

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 21, 2017

It's not safe to cache the creation of a value based on only some of the inputs to that creation. This updates the code to not cache the creation of a type alias with aliasSymbol defined. (Presumably we will never create a type for the same symbol twice?)

@ghost ghost requested a review from aozgaa July 21, 2017 18:12
@ghost ghost force-pushed the isSymbolAccessible branch 3 times, most recently from 86ea47f to af108c0 Compare July 21, 2017 20:54
@ghost ghost changed the title Remove 'isSymbolAccessible' checks when writing type of type literal Don't use a cached union/intersection type if there is an aliasSymbol Jul 21, 2017
@ghost ghost requested a review from sandersn July 21, 2017 20:56
@ghost ghost force-pushed the isSymbolAccessible branch from af108c0 to 82d68fc Compare July 21, 2017 20:56
@@ -1,32 +1,32 @@
=== tests/cases/conformance/types/literal/booleanLiteralTypes1.ts ===
type A1 = true | false;
>A1 : boolean
Copy link
Member

Choose a reason for hiding this comment

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

This behaviour is intended. I don't know that it's super important. It's probably even less important that

type A = { a }
type B = { b }
type T = A | B
type U = A | B
declare var t: T
declare var u: U

should also print either T or U (or A | B) for both t and u.

It's more important that:

declare var x: true | false

prints 'boolean' for the type of x

Copy link
Member

Choose a reason for hiding this comment

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

@ahejlsberg can you weigh in on true | false and boolean being equivalent to type B = true | false ?

>true : true
>false : false

var a: false | true;
>a : boolean
>a : A1
Copy link
Author

@ghost ghost Jul 25, 2017

Choose a reason for hiding this comment

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

This change looked wrong until I realized that this is the fourth declaration of a, and the first declaration has type A1, which is the type shown here.
There is a test below showing that a variable whose only declaration is of type true | false will still print as boolean.

@ahejlsberg
Copy link
Member

ahejlsberg commented Jul 25, 2017

@Andy-MS @sandersn This PR doesn't look right to me. In numerous places we rely on the fact that a union of a given set of types is represented by a single object identity. Why are we trying to change this? I don't see an issue referenced here so I'm not clear on the motivation.

@ghost
Copy link
Author

ghost commented Jul 26, 2017

Closing in favor of #17434.

@ghost ghost closed this Jul 26, 2017
@ghost ghost deleted the isSymbolAccessible branch July 26, 2017 20:34
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
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.

3 participants