-
Notifications
You must be signed in to change notification settings - Fork 0
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
Let primitive types discriminate a union of objects #10
base: main
Are you sure you want to change the base?
Let primitive types discriminate a union of objects #10
Conversation
@@ -4989,7 +4989,7 @@ declare namespace ts { | |||
readonly modifiers?: NodeArray<Modifier>; | |||
readonly equalsGreaterThanToken: EqualsGreaterThanToken; | |||
readonly body: ConciseBody; | |||
readonly name: never; | |||
readonly name?: never; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is basically name?: undefined
without exactOptionalPropertyTypes
(and TS codebase doesnt use this flag). Is this property truly optional at runtime anyway? or is it always set to undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems that createArrowFunction
do not set it at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk if it is set to undefined somewhere else, I'd say it isn't. Even updateArrowFunction ignores it
Main goal
The ultimate goal of this PR is to allow primitive types in object unions to act as discriminants, even when there are no unit/literal types involved. Some examples of this are as follows:
If a property was not previously considered a discriminant, it will now be treated as one when the following conditions are met:
Implementation details
isDiscriminantProperty
were a cachedboolean
in thelinks
of transient symbols, so the first attempt consisted of just adding inside theisDiscriminantProperty
function(prop.links.checkFlags & CheckFlags.HasNonUniformType) && someType(propType, t => !!(t.flags & TypeFlags.Primitive)
as a fallback.Unfortunately, I found a couple of problems with this.
The first one is related with
tests/cases/conformance/types/union/contextualTypeWithUnionTypeObjectLiteral.ts
:The above assignment must fail (you can find the reason in that conformance test), but considering
prop
as a discriminant field changed the behaviour of TS, ultimately allowing the assignment. I ended up disabling the new discriminativeness ofprop
in similar situations and the simpler solution was by using a flag, which I had to take into consideration when it comes to caching the final result.I have to say this whole thing isn't particularly clean, but that test case is 10 years old, that behaviour is literally set in stone, and I don't have the guts to change it, even though the change would be more permissive so it shouldn't be a breaking change, hopefully.
The second problem is related to 60702. Enabling the discriminativness on a particular
prop
makes objects that declared that property asnever
disappear when narrowing. TLDR TS stopped compiling itself here because of howArrowFunction
is defined: itsname
property hasnever
type so it disappeared as a possible type ofnode
after the assertion. Therefore I changed its type from: never
to?: never
as suggested By Ryan, both insrc/compiler/types
, where the problematicArrowFunction
lies, and intests/baselines/reference/api/typescript.d.ts
.I don’t think this last change is problematic; rather, I think a possible indirect breaking change might lie in the fact that if a property shared between objects is typed as
never
in one or more cases and is now considered discriminant due to this additional discriminating capability of TS, then the type containing it could end up being eliminated by TS in various circumstances. I’d like to point out that this already happens when a property is considered discriminant, as can be seen in the linked issue. Simply put, enhancing TS’s capabilities in this regard increases the cases where it can occur.Final considerations
I really appreciate the simplicity of the additional fallback proposed in the PR, although the need to add a flag makes it a bit less clean. Perhaps a more experienced eye could remove it entirely, or realize that the fallback actually needs to be disabled in other call points of
isDiscriminantProperty
, even though the baseline tests didn’t highlight any other issues.I’m really afraid of creating inconsistencies by not disabling this new behaviour wherever it is needed.
One example is
discriminateContextualTypeByObjectMembers
, which indirectly callsisDiscriminantProperty
bydiscriminateContextualTypeByObjectMembers
. It is stated thatfindMatchingDiscriminantType
must be kept up-to-date withdiscriminateContextualTypeByObjectMembers
, andfindMatchingDiscriminantType
callsfindDiscriminantProperties
which is the one that disables the new behaviour. Should I disable it insidediscriminateContextualTypeByObjectMembers
too?Lastly, I wonder if
isDiscriminantWithNeverType
should take into account what’s been added by this PR. I’m quite unsure about it.Future work
From
tests/cases/compiler/narrowingDestructuring.ts
:Even if
x
has type[number, string, string]
whentypeof x[0]
is'number'
,tail
keeps having type[string, string] | [number, number]
instead of just[string, string]
.