Merge overloads that only differ by return type when intersecting #30520
Conversation
|
|
|
A few notes:
Thanks for taking a look! As an open source maintainer myself, I realize this sort of PR is really difficult to accept from the community. With any luck, I'm not too far off the mark. If nothing else, perhaps this will serve as some inspiration and maybe even a starting point if the core developers decide to tackle it in the future. |
|
@typescript-bot test this |
|
Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 35f064d. You can monitor the build here. It should now contribute to this PR's status checks. |
|
@typescript-bot test this because it was out of date with master |
|
@typescript-bot perf test this |
|
Heya @RyanCavanaugh, I've started to run the perf test suite on this PR at 8896207. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@RyanCavanaugh Here they are:Comparison Report - master..30520
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @@ -7151,9 +7151,80 @@ namespace ts { | |||
| stringIndexInfo = intersectIndexInfos(stringIndexInfo, getIndexInfoOfType(t, IndexKind.String)); | |||
| numberIndexInfo = intersectIndexInfos(numberIndexInfo, getIndexInfoOfType(t, IndexKind.Number)); | |||
| } | |||
| callSignatures = intersectCallSignatures(callSignatures); | |||
There was a problem hiding this comment.
I wouldn't call this quite right. This has lost the nuance of which type the signatures originally came from. Say I wrote:
type WhyTho = {
(): {x;};
(): {y;};
} & {prop;}with this logic, when I called something of type WhyTho, the result would be of type {x;} & {y;}, even though at no point did I actually intersect multiple callable things. Like our recent union calling infrastructure, this needs to operate pairwise over the input overload lists, and only merge between types. Eg, if I write
type Another = {
(first: number): {x;};
(): {y;};
(): {q;};
} & {
(): {x;}
} & {
(arg: number): {z;}
}I expect to see a resulting overload list along the lines of
(first: number): {x;} & {z;};
(): {y;} & {x;};
(): {q;} & {x;};it'd be a different story if for overload resolution we always resolved all applicable overloads "simultaneously" and merged the result, but unfortunately we always pick the first applicable overload, so retaining overload list order is important to retaining consistent behavior.
There was a problem hiding this comment.
Thanks for taking a look @weswigham! I thought of that while implementing this, but I couldn't think of a way that a function could satisfy the the type WhyTho without also satisfying the intersected form produced by my change. For example:
type WhyTho = {
(): {x;};
(): {y;};
} & {prop;}
function asdf() {
return {
x: 1
};
}
asdf.prop = 4;
const whatever: WhyTho = asdf;
The error on the last line is:
Type 'typeof asdf' is not assignable to type 'WhyTho'.
Type 'typeof asdf' is not assignable to type '{ (): { x: any; }; (): { y: any; }; }'.
Property 'y' is missing in type '{ x: number; }' but required in type '{ y: any; }'.ts(2322)
How can that error by resolved other than by changing asdf to return the intersected type? I think that's why most languages I'm familiar with consider it an error in the first place to have overloads that differ only be return type.
Still, I agree it's undesirable that the signature changes just because of an unrelated intersection. Assuming it wouldn't be acceptable to always intersect return types of signatures with identical parameters (even in the absence of an explicit intersection), I'll see if I can change the code to only intersect return types of signatures from different intersected types. Any advice in that regard is greatly appreciated of course!
There was a problem hiding this comment.
To answer my own question:
How can that error by resolved other than by changing asdf to return the intersected type?
It can be resolved by declaring an overload, of course!
function asdf(): {x:any};
function asdf(): {y:any};
function asdf(): any {
return 'but what should this value really be?';
}It kind of makes my brain hurt to figure out what those two overloads with identical parameters are supposed to tell us about that function. It sort of just seems like it should be an error, but I'm sure there are good reasons that it's not.
So moving on, I'm implementing the change to only intersect return types across types, never within a type, and it seems mostly straightforward so far. I just want to clarify what should happen in a slightly more complicated situation than the one you mentioned:
type Another = {
(first: number): {x;};
(): {y;};
(): {q;};
} & {
(): {x;}
(): {r;} // this line added
} & {
(arg: number): {z;}
}I think in this case, it should create no-parameters signatures which are the Cartesian product of the no-parameters signatures in the intersected types. So:
(first: number): {x;} & {z;};
(): {y;} & {x;};
(): {y;} & {r;};
(): {q;} & {x;};
(): {q;} & {r;};Does that seem right to you?
There was a problem hiding this comment.
I think in this case, it should create no-parameters signatures which are the Cartesian product of the no-parameters signatures in the intersected types.
I agree with the premise, but also disagree with doing it for the same reason we don't combine multiple multi-overload sets when manufacturing union signatures: overload order matters, and the "correct" ordering of the interleaved product of signatures is ambiguous. So long as overload order matters, we can't actually combine multiple ordered lists (since the only "correct" behavior would be to resolve on each type of the union/intersection separately and combine the single selected signature).
There was a problem hiding this comment.
I've been giving this a fair bit of thought over the past few days, and, if I'm understanding you correctly, we need to make sure that any change to signature intersection follows these rules.
Upon intersection:
- the overloads within any of the constituent types of the intersection cannot be combined / have their return types intersected, even if they differ only be return type.
- the overloads within any of the constituent types of the intersection cannot change order relative to one another.
In addition, there may be one more rule:
3. The overloads of an intersection type must occur in the order of intersection of their source types.
If all three of these rules must hold, the result of an intersection will have to be pretty much as it is today, except that if two overloads that differ only by return type happen to be right next to each other in the list (as currently computed in master), and they come from different types, then they can be turned into a single overload by intersecting their return types.
So in this example:
type Another = {
(first: number): {x;}
(): {y;}
(): {q;}
} & {
(): {x;}
(): {r;}
} & {
(arg: number): {z;}
}The result would be:
(first: number): {x;}
(): {y;}
(): {q;} & {x;}
(): {r;}
(arg: number): {z;}However, if we can relax rule 3, we could achieve something like I think you were getting at with this statement:
the only "correct" behavior would be to resolve on each type of the union/intersection separately and combine the single selected signature
If I'm understanding you correctly, you're saying the only correct algorithm would be this: given a function call on an intersected type, we perform overload resolution on all of the constituent intersected types independently. Some may resolve successfully, some may not. Put all the successful ones in a list in the same order as the types they came from. Then, select the first signature from the list and intersect its return type with all other signatures in the list that differ from the first one only be return type.
That won't really uphold rule 3. Given the type above and this call site:
const f: Another = ...;
f();We select (): {y;} from the first type in the intersection and (): {x;} from the second type, so the intersected signature would be (): {y;} & {x;}. That puts precedence on (): {x;} from the second type over (): {q;} from the first type, violating rule 3.
But if that's ok (I think it is! It's less surprising than the (): {y;} we get when we strictly enforce rule 3), then we can achieve the same effect as this call-site algorithm at intersection-time with an algorithm like this:
for all types T in intersection {
for all _remaining_ signatures S in type T {
signature R = S
for all types T' in the intersection that _follow_ T {
for all _remaining_ signatures S' in type T' {
if S is identical to S' (ignoring return type) {
change the return type of R to the intersection of the return types of R and S'
remove S' from consideration in the T/T' signature set
break loop over signatures of T'
}
}
}
add overload R to result
}
}
Which for the type above would yield:
{
(first: number): {x;} & {z;}
(): {y;} & {x;}
(): {q;} & {r;}
}Which seems quite sensible to me.
Does that make sense to you?
I've been studying your recent union changes (#29011), trying to understand how that approach would apply to the intersection case. I've learned a lot about how to actually compare the signatures, but less obvious is how the intersection should behave.
By the way, I find this interesting and am happy to keep working on it even if that involves a bunch of false starts and iteration. Do let me know if I'm just wasting your time, though. There's no point in doing that. :)
RyanCavanaugh
added
the
Experiment
|
Like a lot of the Experiment-labelled PRs, this one is hard to push any further without an associated proposal that we can discuss. It looks like the best place is #9239, which is indeed missing a complete proposal. |
|
This experiment is pretty old, so I'm going to close it to reduce the number of open PRs. |
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
Consider two interfaces:
Both interfaces define a method named
footaking identical parameters. However, the return types are different. If we write a class that implements both of these interfaces, TypeScript understands that we can satisfy both interfaces by returning an object with both properties:In fact, if my imagination isn't failing me, this return type (or something that is assignable to it) is the only way we can satisfy both interfaces.
However, if we use intersection (
&) instead ofimplements:The last line fails to type check. TypeScript reports:
This is because the intersection has turned the two versions of
foointo overloads instead of intersecting their return types.In my possibly naive opinion, it makes no sense for
&to create overloads where the parameter types are identical, since we can't select an overload based solely on the return type. And the inconsistency with interface implementation seems undesirable as well. TypeScript should continue to create overloads where the parameters are different, but, for functions with identical parameters, the signatures should be collapsed to a single signature where the return type is the intersection of the return types of all the signatures with the same parameters.So that is what this pull request implements.
All previous tests passed except for
tests\cases\conformance\jsx\checkJsxGenericTagHasCorrectInferences.tsx, which now reports slightly different compiler errors. I believe that the errors are actually better in this branch (see the baseline diff).I'm certain I haven't gotten everything right here, as I only looked at the TypeScript code for the first time today. Hats off to you all for making it so easy to understand! But if you agree this PR is heading in the right direction I'm very happy to improve it as necessary.