Allow inference to explore multiple instances of the same symbol#31633
Allow inference to explore multiple instances of the same symbol#31633weswigham wants to merge 7 commits intomicrosoft:mainfrom
Conversation
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 58b71fe. You can monitor the build here. It should now contribute to this PR's status checks. |
|
So, looking at I have an idea for optimizing the patterns that cause the type blowup in #15443, but the first step is to actually get a test into the compiler suite of the issue. I guess I'll do that work here~ |
|
I've isolated the OOM issue from |
…already beign related
|
I've re-fixed #15443 in the PR (now with a test in our suite), this time by adding an optimization into assignment checking that looks for identical types among the type parameters of the @typescript-bot perf test this since this touches assignment checking now |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 37fce9f. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 37fce9f. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 37fce9f. You can monitor the build here. It should now contribute to this PR's status checks. |
|
@weswigham Here they are:Comparison Report - master..31633
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…und yet, use more complete depth checking machinery
|
@typescript-bot run dt again - this update should fix those OOMs in DT with further improvements to how we calculate our max recursion depth in inference. |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at a7a27e3. You can monitor the build here. It should now contribute to this PR's status checks. |
It's just like |
|
Since @ahejlsberg was slightly interested in this, I've synced this PR with @typescript-bot perf test this |
|
Heya @weswigham, I've started to run the parallelized community code test suite on this PR at f4d1289. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at f4d1289. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at f4d1289. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
Heya @weswigham, I've started to run the extended test suite on this PR at f4d1289. You can monitor the build here. It should now contribute to this PR's status checks. |
|
@weswigham Here they are:Comparison Report - master..31633
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
Perf looks good, and the changes seem good in the places with changes (sometimes it's less errors from better matching recursive structures, but in some places we issue a new error from improved inference around |
|
@typescript-bot perf test this
|
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at c79e70d. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the extended test suite on this PR at c79e70d. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at c79e70d. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
Heya @weswigham, I've started to run the parallelized community code test suite on this PR at c79e70d. You can monitor the build here. It should now contribute to this PR's status checks. |
|
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..31633
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg said to punt this from 3.8 because of it's complexity, but it's still going to need a timely review to get into 3.9; so that'd be much appreciated ❤️ |
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
The high-level description sounds plausible, but the implementation reminds me of ad-hoc extensions to assignability that I've tried before. I'll defer to @ahejlsberg for whether we want to add it, since it seems like it mostly helps just fp-ts.
| // This is difficult to detect generally, so we scan for prior comparisons of the same instantiated type, and match up matching | ||
| // type arguments into sets to create a canonicalization based on those matches | ||
| if (relation !== identityRelation && ((source.aliasSymbol && !source.aliasTypeArgumentsContainsMarker && source.aliasTypeArguments) || (getObjectFlags(source) & ObjectFlags.Reference && !!getTypeArguments(<TypeReference>source).length && !(getObjectFlags(source) & ObjectFlags.MarkerType))) && | ||
| ((target.aliasSymbol && !target.aliasTypeArgumentsContainsMarker && target.aliasTypeArguments) || (getObjectFlags(target) & ObjectFlags.Reference && !!getTypeArguments(<TypeReference>target).length && !(getObjectFlags(target) & ObjectFlags.MarkerType)))) { |
There was a problem hiding this comment.
might be worth it to extract a function for this predicate, if only to make the name obvious.
(the line length is also too long)
| declare function load(name: string): Promise<string>; | ||
| declare function convert(s: string): IPromise<number>; | ||
|
|
||
| var $$x = load("something").then(s => convert(s)); |
There was a problem hiding this comment.
why is this error gone now? does overload resolution follow the new code path in assignability now?
There was a problem hiding this comment.
We now successfully identify that Promise<T> and IPromise<T> are identical, and so no longer fail inference when relating their then signatures~
|
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. |
7 participants
Fixes #31616
Different instances of a type with a symbol are different instances, ergo can have different underlying structure and produce differing inferences. Stopping after we've seen a symbol once as we recur downward (but not once globally) is actually strange - it meant we'd infer to both instances in
{a: Foo<A, M>, b: Foo<B, N>}, but to only one while checking{a: Foo<A, Foo<B, M>>}. The need for a limiter here makes sense (since we do produce types that can infinitely expand), but this one was excessively restrictive.Additionally, for consistency (since it needs a similar cutoff), I've unified the depth limit constant between inference and type printout.