-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Fix getRecursionIdentity function to always return some identity #43527
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
This PR implements the fix I describe here. Basically, The PR reverts the changes from #43435 since they're no longer required and don't fix the core issue. However, I'm keeping the tests from that PR. Regarding the baseline changes, two tests that previously failed now succeed--as they did when they were originally created. The |
@typescript-bot test this |
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 90382b2. You can monitor the build here. Update: The results are in! |
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 90382b2. You can monitor the build here. |
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 90382b2. You can monitor the build here. Update: The results are in! |
Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 90382b2. You can monitor the build here. |
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 90382b2. You can monitor the build here. |
@ahejlsberg Here they are:Comparison Report - master..43527
System
Hosts
Scenarios
Developer Information: |
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
@ahejlsberg Here they are:Comparison Report - master..43527
System
Hosts
Scenarios
Developer Information: |
Tests all look good, performance is unaffected. |
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.
Makes sense 👍
That doesn't seem like a good outcome - a reverse mapped type over something with a pointer to a deep type hierarchy adding 40s to a compilation seems unacceptable to me? I just tested it out - #42485 can bring the runtime of that test down from 27s to 3s. Still not great (3s for a single call on a beefy machine?), but fixes it by an order of magnitude and definitely makes it keepable as a test. |
I'm not sure I understand how #42485 helps. Before the fix in this PR we'd end up hitting the panic limit at 100 recursive invocations and output an With the PR we keep successfully relating types until we get to 50 levels of instantiations and then stop--but without printing any types, so #42485 wouldn't affect anything. It takes longer for this to occur but that isn't surprising given the very large number of types |
This test (from 7bd6eaf): let xhr: XMLHttpRequest;
const out2 = foo(xhr);
out2.responseXML
out2.responseXML.activeElement.className.length you said you removed from this PR because after this PR, it takes an excessive time to typecheck. I can see that - on my machine it typechecks in 27s and issues a (bogus! The tree of possible reverse-mapped types is in fact finite!) |
@weswigham I thought #42485 was just about fixing type printback, but great that it also improves performance. Let's get it merged before this PR and then we can keep the test. |
Fixes #43493.