Unwrap substitutions both before _and_ after potential simplification#32116
Conversation
|
reping @ahejlsberg ❤️ |
1d13804 to
3b28100
Compare
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the parallelized community code test suite on this PR at e501cc9. 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 e501cc9. 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 e501cc9. 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. |
src/compiler/checker.ts
Outdated
| if (target.flags & TypeFlags.Simplifiable) { | ||
| target = getSimplifiedType(target, /*writing*/ true); | ||
| while (true) { | ||
| const s = source.flags & TypeFlags.Substitution ? source = (<SubstitutionType>source).substitute : |
There was a problem hiding this comment.
Would it ever be worth updating substitute in-place to collapse multiple links down to one?
There was a problem hiding this comment.
I don't think we can (or at least not usefully) - the associated typeVariable of nested SubstitutionTypes can (and most often does) differ. ie, a T substituted with T & U where U is substituted with U & string - while here we want to fully flatten, in other situations I think we want to keep the knowledge of what variables expanded to what. Multiple nested conditionals affecting the same type variable are already merged pre-substitute creation.
There was a problem hiding this comment.
Right; I forgot that these are more like narrowings rather than unification substitutions.
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the parallelized community code test suite on this PR at 271187f. 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 271187f. 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 271187f. 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 271187f. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
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..32116
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the extended test suite on this PR at a4fa025. 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 a4fa025. 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 a4fa025. 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 community code test suite on this PR at a4fa025. 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..32116
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 97efc37. 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 97efc37. 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 97efc37. 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 97efc37. You can monitor the build here. It should now contribute to this PR's status checks. |
|
RWC is 👍 (no diff with |
|
@weswigham Here they are:Comparison Report - master..32116
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ahejlsberg
left a comment
There was a problem hiding this comment.
Approved pending the requested change.
src/compiler/checker.ts
Outdated
| const t = getNormalizedType(target, /*writing*/ true); | ||
| if (t === target) break; | ||
| target = t; | ||
| } while (true); |
There was a problem hiding this comment.
Put the loop inside getNormalizedType. It is only called from here.
Fixes #31804
The prophecized bug from when we added conditional simplification has come to pass, thus we must decide: Do we continue being naive and just apply a second unwrap step (as I have added here), or do we complicate the unwrapping/simplifying a bit more to guarantee that we always get the final maximally unwrapped/simplified type (as was originally in the conditional simplification PR).