Fix inferences between alias type arguments and defaulted alias type arguments #51771
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot pack this |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 978b474. You can monitor the build here. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 978b474. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 978b474. You can monitor the build here. |
|
Heya @weswigham, I've started to run the diff-based top-repos suite on this PR at 978b474. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the diff-based user code test suite on this PR at 978b474. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at 978b474. You can monitor the build here. |
|
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@weswigham Here are the results of running the user test suite comparing Everything looks good! |
|
Heya @weswigham, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@weswigham Here they are:
CompilerComparison Report - main..51771
System
Hosts
Scenarios
TSServerComparison Report - main..51771
System
Hosts
Scenarios
StartupComparison Report - main..51771
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@weswigham Here are the results of running the top-repos suite comparing Everything looks good! |
| const minParams = getMinTypeArgumentCount(params); | ||
| const sourceTypes = fillMissingTypeArguments(source.aliasTypeArguments, params, minParams, /*isJs*/ false); | ||
| const targetTypes = fillMissingTypeArguments(target.aliasTypeArguments, params, minParams, /*isJs*/ false); | ||
| inferFromTypeArguments(sourceTypes, targetTypes!, getAliasVariances(source.aliasSymbol)); |
There was a problem hiding this comment.
Hmm, I wasn't even aware that the type argument list could be incomplete at this point, and I'm not sure that was ever intended. It's fine for type arguments to be missing at the lexical level, but once we're dealing with types we pretty much assume that the length of a type argument list matches the length of the corresponding type parameter list. That assumption is pretty pervasive throughout the compiler (modulo the this type argument that sometimes isn't present). I think it would make more sense to find the origin of the incomplete list and put the call to fillMissingTypeArguments there.
There was a problem hiding this comment.
Right, so, our old logic for giving an alias symbol instantiation an alias symbol didn't fill missing type arguments, which is why the new condition I added also didn't - it didn't seem required, since our existing usage didn't fill them. Obviously, broader usage of these unfilled lists reveals the flaw, namely inference (and probably comparison checking) not going well, since, as you said, they currently expect filled lists. Thing is, I think trafficking the unfilled lists has an advantage - it may be worth handling incomplete lists in inference/comparison checking. If we start filling them eagerly, rather than getting the user-supplied list, when we printback, we don't printback the user supplied list anymore. Meaning, even though you wrote Component, we print back and serialize Component<{}, any, any>. For complex defaults, this means we can muddy the printback unnecessarily quite a bit. So while I could fix this by passing in filled lists at the now 2 locations we supply an alias for an alias symbol instantiation, the UX is much nicer if instead we update inference and comparison checking to handle incomplete type alias argument lists, imo.
…er than within inference" This reverts commit 9f806ef.
|
@ahejlsberg Rather than ensuring we pass filled lists at every callsite, after an audit, I've ensured we handled unfilled lists everywhere we did comparisons of type alias type argument lists. This is better UX, since then when we printback the aliases, we print them back in the same way the user wrote them (ie, relying on defaults). In either case, there were only 2 places that needing updating. There were 2 places we made lists that needed filling (at a minimum; we make some impromptu aliases out of type references that probably also need filling, but I didn't investigate those too much), but there's also only 2 places we compare alias type argument lists where the list needs to be filled (at most) - inference and variance comparison checking. |
1. lambda-tester until microsoft/TypeScript#51771 is merged. 2. aws dependents -- these frequently run out of memory and I want to be notified when that changes without being told that they're failing. They're not failing because of Typescript changes, but because AWS' memory usage is high enough to cause dtslint-runner to cause them to run out of memory.
1. lambda-tester until microsoft/TypeScript#51771 is merged. 2. aws dependents -- these frequently run out of memory and I want to be notified when that changes without being told that they're failing. They're not failing because of Typescript changes, but because AWS' memory usage is high enough to cause dtslint-runner to cause them to run out of memory.
There was a problem hiding this comment.
I think trafficking the unfilled lists has an advantage
I tend to agree with this, though could be persuaded if we forget to do this in so many places that it's a problem. If we want, we can do a follow-up PR that pre-emptively fills type argument defaults, but this seems like a very straightforward fix for now.
|
Turns out that in most cases we do fill the type parameter defaults early and print them back even if they weren't written explicitly. So, we're basically in an inconsistent state. I think it would make sense to create a separate PR to see what the effect would be of always preserving type references as written. |
Fixes #51698
As I mentioned in the related issue, contrary to speculation during post-standup, the issue wasn't that we were missing the alias identity (actually, to ensure that isn't an issue, we use the target symbol on construction of the alias already), but rather was that we ignored alias type argument defaults during inference. With this, we actually apply the defaults to the alias type argument lists we compare during inference, fixing the issue~