-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Preserve substitution types in check position of conditional types #41841
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
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 542a8a2. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 542a8a2. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 542a8a2. You can monitor the build here. |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
@typescript-bot perf test this |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 542a8a2. You can monitor the build here. Update: The results are in! |
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.
Good to know we've improved our handling of substitution types elsewhere enough to not need all these special cases anymore. So long as the extended test suite and DT comes back OK (I think they're good?), this seems good.
|
DT is clean. Best I can tell so are RWC and the extended test suite, but there's a mess of unrelated issues in both suites. Looks like the perf test machine is not responding. |
|
Ah yeah, power was turned off in the office on Friday. I was planning on stopping by in a bit to turn my own workstation back on - I'll check the perf machine while I'm there. |
|
@DanielRosenwasser Here they are:Comparison Report - master..41841
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Looks like there is a bit of slow-down - I'm not sure why in the compiler-with-unions test case though. Would substitution types appear much in that version of the codebase? |
|
We only really use conditionals (which are what create substitutions) in two ways in our own codebase. The |
|
There is an almost 10% difference between the best and worst check times in compiler-with-unions. I'm going to run the perf tests again to see if that persists. |
|
@typescript-bot perf test this |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 542a8a2. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - master..41841
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Yeah, as I suspected, there was something off about that first test run. The compiler-with-unions numbers are now basically unaffected. Interestingly, material-ui numbers seem to improve a little. Anyways, certainly no adverse perf impact. |
|
This breaks styled-components on DT, although I haven't tracked down why yet. I'll file a bug when I do. |
|
Here's the message: Type 'StyledComponentInnerComponent<C>' does not satisfy the constraint 'ElementType<any>'.
Type 'string | ComponentClass<any, any> | FunctionComponent<any> | (C extends StyledComponent<infer I, any, any, never> ? I : C)' is not assignable to type 'ElementType<any>'.
Type 'string' is not assignable to type 'ElementType<any>'.
Type 'string | ComponentClass<any, any> | FunctionComponent<any>' is not assignable to type 'ElementType<any>'.
Type 'string' is not assignable to type 'ElementType<any>'.
Type 'StyledComponentInnerComponent<C>' is not assignable to type 'FunctionComponent<any>'.
Type 'string | ComponentClass<any, any> | FunctionComponent<any> | (C extends StyledComponent<infer I, any, any, never> ? I : C)' is not assignable to type 'FunctionComponent<any>'.
Type 'string' is not assignable to type 'FunctionComponent<any>'.
Type 'string | ComponentClass<any, any> | FunctionComponent<any>' is not assignable to type 'FunctionComponent<any>'.
Type 'string' is not assignable to type 'FunctionComponent<any>'. |
|
@sandersn That's odd. The DT suite run above was clean two days ago. |
|
Looking at the logs, all four shards only tested |
|
Standalone repro is up at #41886 |
With this PR we properly preserve substitution types in the check type position of conditional types. Previously we'd erase substitution types, thus losing constraints implied by containing conditional types. In #32093 this was fixed by attempting to simplify types during inference, but that can lead to infinite recursion as seen in #41756. By fixing the root issue (the erasure of substitution types) we can do away with #32093 and the infinite recursion.
BTW, the reasons we erased substitution types appear to be historical. Properly preserving them appears to have no ill effects.
Fixes #41756.