-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Erase type parameters to a type which behaves as never in a union and unknown in an intersection or any otherwise #39217
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
Conversation
…own in an intersection or any otherwise
@typescript-bot user test this |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at ed49594. You can monitor the build here. |
Heya @weswigham, I've started to run the extended test suite on this PR at ed49594. You can monitor the build here. |
Heya @weswigham, I've started to run the perf test suite on this PR at ed49594. You can monitor the build here. Update: The results are in! |
Heya @weswigham, I've started to run the parallelized community code test suite on this PR at ed49594. You can monitor the build here. |
@@ -1,3 +1,6 @@ | |||
tests/cases/compiler/immutable.ts(189,20): error TS2430: Interface 'Stack<T>' incorrectly extends interface 'Indexed<T>'. | |||
The types returned by 'concat(...).map(...).filter(...)' are incompatible between these types. | |||
Type 'Set<any>' is not assignable to type 'Indexed<any>'. |
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 just a new follow-on error to the existing errors below, I believe, as the incorrect extensions below make Set
and Indexed
incompatible with one another, which is now properly reported here (as we now no longer fully erase all the data in the signature and can keep comparing until we get to that point).
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
@weswigham Here they are:Comparison Report - master..39217
System
Hosts
Scenarios
|
@typescript-bot user test this |
Heya @weswigham, I've started to run the extended test suite on this PR at c084d28. You can monitor the build here. |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at c084d28. You can monitor the build here. |
Heya @weswigham, I've started to run the parallelized community code test suite on this PR at c084d28. You can monitor the build here. |
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.
Would we need this PR if we merge #31029?
@@ -700,6 +700,7 @@ namespace ts { | |||
const anyType = createIntrinsicType(TypeFlags.Any, "any"); | |||
const autoType = createIntrinsicType(TypeFlags.Any, "any"); | |||
const wildcardType = createIntrinsicType(TypeFlags.Any, "any"); | |||
const erasedType = createIntrinsicType(TypeFlags.Any | TypeFlags.Never | TypeFlags.Unknown, "any"); |
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.
Wow, a schizophrenic type. We have an implicit assumption everywhere that only one of these flags will be set--with a few exceptions such as TypeFlags.EnumLiteral
. Are you confident there are no places we'll get confused by all three being set in this type? Tests for these now become order dependent.
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.
Yep, I'm literally abusing the order dependence of handling each of these to avoid special-casing the handling of this type in most type construction places (partially because we're essentially out of normal type flags). Generally speaking we handle Any, then Never, then Unknown (if we handle both), so it's OK, since this marker is mostly any-like (except in unions/intersections, where we handle unknown or never, and then any, which is what causes the desirable vaporization in both structures). At the start, I also wanted to add ObjectFlags.NonInferrableType
to it, but that has the side effect of cases like <T extends T[]>() => T
where we currently infer any[]
becoming unknown
. I don't think any[]
is great (it injects any
into the program without the user ever explicitly writing it, and it's not an noImplicitAny
error!), but unknown
isn't really much better. I think the ideal would be unknown[]
- but that relies on knowing that T[]
is covariant on T
... I guess since we use getVariances
in inference already, that should be safe. So In theory I could get what I want by mapping this erased type to unknown
in covariant positions and never
in contravariant positions, when it persists into the constructed type (rather than just vaporizing in a union or intersection and letting it persist beyond that as a psuedo-any
). I could also make this a special type parameter, and handle all this with a special instantiator that tracks some context of the instantiation - in fact, to do the variance bit, I probably need to.
I think #31029 would fixup some of the issues I have with marking the
|
@ahejlsberg is @weswigham's explanation of |
Unfortunately, we never finished reviewing this PR. It is pretty old now, so I'm going to close it to reduce the number of open PRs. |
This has the effect of omitting the parameters from inference calculations, without subsuming that data provided by the other members of the union/intersection.
Fixes #39080