Limit type argument inference from binding patterns #49086
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot test this |
|
Heya @andrewbranch, I've started to run the extended test suite on this PR at 7e8c56c. You can monitor the build here. |
|
Heya @andrewbranch, I've started to run the diff-based community code test suite on this PR at 7e8c56c. You can monitor the build here. Update: The results are in! |
|
Heya @andrewbranch, I've started to run the parallelized Definitely Typed test suite on this PR at 7e8c56c. You can monitor the build here. |
|
@typescript-bot perf test this |
|
Heya @andrewbranch, I've started to run the perf test suite on this PR at 7e8c56c. You can monitor the build here. Update: The results are in! |
|
@andrewbranch |
|
@andrewbranch Here they are:Comparison Report - main..49086
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot pack this |
|
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 7e8c56c. You can monitor the build here. |
|
Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
| @@ -0,0 +1,3 @@ | |||
| declare function pick<O, T extends keyof O>(keys: T[], obj?: O): Pick<O, T>; | |||
| const _ = pick(['b'], { a: 'a', b: 'b' }); // T: "b" | |||
| const { } = pick(['b'], { a: 'a', b: 'b' }); // T: "b" | "a" ??? (before fix) | |||
There was a problem hiding this comment.
Since this repro in theory involves completions, should it have a fourslash variant?
There was a problem hiding this comment.
I'm curious if other callers of getWidenedLiteralLikeTypeForContextualType need to be passing node (checkPropertyAssignment? AFAIK that maybe handles the assignment pattern case ({x} = f())...?), and some fourslash examples for the completions-relevant examples (to verify those still work using the LS overrides for signature resolution despite these changes), but the core changes themselves look good to me.
|
An example mentioned in the design meeting is broken by this: declare function id<T>(x: T): T;
const { f = (x: string) => x.length } = id({ f: x => x.charAt });When getting the type of the function expression inside the call to draw inferences from, we have the contextual type from the binding pattern available, but we don’t use it because we’re skipping context-sensitive functions and return |
|
Usually that just means we'll pick up the better type in the next inference pass - the |
|
Doh, of course. That was a red herring. Still debugging. |
|
Ah, I see what the problem is. I mistook how the |
|
Hm, ok. I’m actually still having trouble combining these type mappers in a way that works for every case, and I’m not sure if it’s possible. The issue is that for instantiating contextual signatures, we actually do use those low-priority return type inferences as well as other inferences made so far. So when I completely separated those inferences, I attempted recombine them from the two contexts, but I don’t think that’s going to work. An earlier iteration of this PR, rather than removing binding pattern return type inferences from the primary context, flagged them with another priority flag and then avoided returning those from |
|
@ahejlsberg do you want to take a look at what I have so far before I go down a rabbit hole trying to solve ☝️? Maybe you’ll have an idea for that. |
|
@ahejlsberg the changes we discussed yesterday are up. That approach actually fixed this test case: declare function id<T>(x: T): T;
const { f = (x: string) => x.length } = id({ f: x => x.charAt });Seems like the best outcome we could have hoped for! |
|
Is this still waiting on a review or is it ready to merge? |
|
I think @ahejlsberg may have wanted a final look at it. |
| } | ||
| return instantiateInstantiableTypes(contextualType, inferenceContext.nonFixingMapper); | ||
| } | ||
| if (inferenceContext?.returnMapper) { |
There was a problem hiding this comment.
This if statement now runs even when there are no inference candidates. Is that intended?
There was a problem hiding this comment.
Yes, because the returnMapper pulls from a different inference context that may have candidates not present in inferenceContext. (Also, returnMapper is guaranteed to have candidates if it exists.)
Second attempt of #45719 (and #46009)
Fixes #43605
Fixes #45074
Fixes #45663
In the past, when we made inferences to type arguments in return types, we did two things with those inferences: (1) put them in the primary inference context with a low priority, and (2) put them in a secondary inference context that gets used solely for instantiating contextual types of arguments in the same call expression. The consequence of (1) is easily observable when no other inference sources are present to overwrite the low-priority return type inference made from a binding pattern:
#45719 fixed this by preventing return type inferences from binding patterns altogether, i.e., it stopped both (1) and (2) above. This turned out to be a problem, because (2) produces very desirable behavior with array binding patterns:
The binding pattern provides a tuple contextual type for the array literal in the return expression, allowing
e1,e2, ande3to have separate types rather than all havingstring | number | boolean. To preserve this desirable behavior, I tried #46009 which only prevented inferences from object binding patterns. However, it was later discovered that this still wasn’t a complete fix, because it still allowed:I ended up reverting all these attempts to reevaluate, with the conclusion
So that’s what this PR does. Rather than make any distinctions between object and array binding patterns, I make a distinction between inference passes into separate contexts (1) and (2) above. If a contextual type for the return type came from a binding pattern, we make no inferences into (1), but we always make inferences into (2) so that other arguments can benefit from that contextual type (primarily for the array → tuple case—it does happen in other cases but I haven’t found any that are useful).