Include type parameter defaults in contextual typing #50994
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
|
@mxsdev The simplification in this PR is simply to perform contextual type instantiation for all type parameters that declare default types, even if no inferences have been made for those type parameters. |
|
(slight typo - @mxsdev) |
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at 58d90d4. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 58d90d4. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at 58d90d4. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 58d90d4. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 58d90d4. You can monitor the build here. |
|
@ahejlsberg Here are the results of running the user test suite comparing Everything looks good! |
| @@ -27534,7 +27534,7 @@ namespace ts { | |||
| const inferenceContext = getInferenceContext(node); | |||
| // If no inferences have been made, nothing is gained from instantiating as type parameters | |||
| // would just be replaced with their defaults similar to the apparent type. | |||
There was a problem hiding this comment.
is this part of the comment here still relevant? with the code change in this PR this now reads weirdly to me, or there is some additional context missing here - like what kind of default type parameters should be ignored and what kind of default parameters might be useful. Or perhaps this comment was never about defaults and it should mention base constraints?
There was a problem hiding this comment.
You're right, that should say constraints, not defaults.
|
@ahejlsberg I've created a PR targeting this branch, with an additional test case from this other issue, the PR can be found here: #51002 This PR here should also add #46310 to the issues that it's going to fix |
|
@ahejlsberg Here they are:Comparison Report - main..50994
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here are the results of running the top-repos suite comparing Everything looks good! |
There was a problem hiding this comment.
I'd think this'd make us pick the default in more ambiguous contextually typed inference situations (I'm thinking something that's circularly constrained with a default could change to use the default from an any maybe), but it looks like nothing in the extended suites is obviously broken, so that's good~
Ah I see, much simpler/more direct indeed. I was concerned originally about doing that since I assumed like @weswigham that it might infer in too many situations. Good stuff! |
| // If no inferences have been made, and none of the type parameters for which we are inferring | ||
| // specify default types, nothing is gained from instantiating as type parameters would just be | ||
| // replaced with their constraints similar to the apparent type. | ||
| if (inferenceContext && contextFlags! & ContextFlags.Signature && some(inferenceContext.inferences, hasInferenceCandidatesOrDefault)) { |
There was a problem hiding this comment.
I also have an additional fix touching this very line. Since this is being touched now, perhaps you could take a look if it looks any good? :p
There was a problem hiding this comment.
I'm not quite sure what that fix is trying to accomplish. Either way I don't think it is related to the issue being solved in this PR, so I think it's better to keep it separate.
|
Tests and performance all look good. I'll merge this. |
This PR supersedes #50960 and fixes #50858.
Thanks @mxsdev for calling attention to this issue and proposing a fix. This PR effectively implements the same idea as #50960 in a simpler manner. Also, this PR borrows the tests from #50960.
Fixes #46310.
Fixes #50858.