Avoid expensive relationship checking in mapped type member resolution #36754
Conversation
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 868b7b9. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 868b7b9. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 868b7b9. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 868b7b9. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@ahejlsberg Here they are:Comparison Report - master..36754
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. |
|
RWC and DT tests are clean (errors are preexisting conditions in master). Only positive effects on performance. I think this one is good to go. |
|
@amcasey @RyanCavanaugh Can't add you as reviewers (the usual GitHub issue), but please tak a look when you can. |
| >b : T["b"] | ||
| >this.foo : { [P in keyof T]: T[P]; } | ||
| >this : this | ||
| >foo : { [P in keyof T]: T[P]; } | ||
|
|
||
| !(a && b); | ||
| >!(a && b) : false |
There was a problem hiding this comment.
so.... this was just... wrong before? a was T["a"] which should have been constrained to number | object | undefined, which means a can be falsey, which means the new result is correct.
Do we fail to include undefined constraints in our analysis somewhere in control flow?
There was a problem hiding this comment.
I believe it's the old issue of not having a good representation for the falsy parts of a higher-order type. For example:
interface Options {
a: unknown;
b: unknown;
}
function foo<T extends Options>(a: T['a'], b: T['b']) {
const x = a && b; // Type T['b']
}The type of x really should be T['b'] unioned with the falsy parts of T['a'], but even with negated types this would be a pretty ugly type. IIRC we previously decided to leave this as a known design limitation.
There was a problem hiding this comment.
My above remark is moreso that the baseline change maybe indicates a bug elsewhere in the compiler that this change now works around. This, however, seems fine. The "worst case" is that we mix in an extra undefined that's already expressed in the property type's constraint - that's a little uglier for printback, but isn't supposed to change semantic meaning much (if any).
Maybe an inline comment on why we're making this change on the appropriate line is relevant, so someone who might come back in the future, looking to cleanup the printback in that edge case, doesn't swap it back and regress perf?
Yup, that's exactly it.
Don't think we need a comment. With this change the code is actually more consistent with what we do on the next line to shallowly remove But, for this and other reasons, it definitely would make sense to add the repro from #36564 as a performance test case. That way we'll see a 10% regression if someone messes with it. |
|
There are other text search matches for |
|
@amcasey I saw one more match, but it's fine for that to stay as is. |
This PR removes an expensive
isTypeAssignableTocheck from mapped type member resolution and replaces it with a shallowermaybeTypeOfKind. The full relationship check can generate a lot of work exploring simplifications and constraints. Replacing it with the shallow check improves total compile time of the repro in #36564 by 8-10% and also resolves circularities I'm seeing in another PR.