Intersect 'this' types in union signatures #32538
Conversation
| declare const fn: A | B | C; | ||
| fn(0); | ||
| ~~~~~ | ||
| !!! error TS2684: The 'this' context of type 'void' is not assignable to method's 'this' of type 'never'. |
There was a problem hiding this comment.
I’m not sure what to do with this: void. It doesn’t have any special handling right now, so it intersects with number to produce never. But it feels fairly synonymous with completely omitting a this type, in which case I would drop it out of the intersection. I’m a bit fuzzy on how to interpret the void type outside of return types.
There was a problem hiding this comment.
void is usually best treated as synonymous with undefined in all non-return positions - at least that's what we've done thus far (despite the fact that any return type is assignable to a void returning function, thus you cannot guarantee that a void returning function returns undefined). It's a bit inconsistent.
There was a problem hiding this comment.
Ok; by that logic I think the intersection stands.
There was a problem hiding this comment.
In this parameters, void is definitely a synonym of undefined. I wrote the feature before undefined existed. this: void is (was?) the idiomatic way to specify that there will be no this provided.
never seems right because it arises from an union of signatures that require both (1) no this provided (2) some this provided.
|
If you really want to get it in, I'd run RWC and user tests first and see what breaks. If we see unnecessary breaks, I'd hold off until 3.7. |
|
@typescript-bot test this |
|
Heya @andrewbranch, I've started to run the extended test suite on this PR at 4f2a2e5. You can monitor the build here. It should now contribute to this PR's status checks. |
|
@typescript-bot run dt |
|
Heya @andrewbranch, I've started to run the parallelized Definitely Typed test suite on this PR at 4f2a2e5. You can monitor the build here. It should now contribute to this PR's status checks. |
|
@DanielRosenwasser RWC and DT are clean. This is a real edge case of a breaking change (as is my specialty of late, a la #31949). |
There was a problem hiding this comment.
I'm kind of torn on this one because I think (1) it's correct but (2) it's a low-value change, mainly because it's likely to confuse people when it's issued.
On the other hand, (3) not many people use this parameters anyway so (4) RWC didn't even break.
I guess if there were a specific error for this to mitigate (2) I'd be happier, but because of (3) and (4) I don't think that's worth the effort.
All told, (1) + (2) + (3) + (4) = 
| data: number; | ||
| } | ||
| function test(r: Real | Fake) { | ||
| r.method(12); // error |
There was a problem hiding this comment.
Not a big fan of this, since it's prima facie Obviously Legal.
There was a problem hiding this comment.
I mean, this is the crux of the change 😅
There was a problem hiding this comment.
Well, it's Obviously Legal and Technically Incorrect, and in this case I think Technically Incorrect should win.
|
I also think it’s a low value change because of (3)—it requires some suspiciously unconventional code to reproduce an unsafe behavior at runtime. I’m not sure where a custom error message would even go, because computing the union signature(s) happens totally separately from checking that the call target satisfies the signature(s). All told, I agree it’s a little confusing, but maybe not more confusing than regular parameters: declare const fn: ((x: string) => void) | ((x: number) => void);
fn('hello'); // what? why isn’t the parameter type 'string | number'? no special error here |
Fixes #31547
Since the GitHub reviewer dropdown isn’t going to work: @weswigham @sandersn 👋
P.S. @DanielRosenwasser this is a bug fix but it’s also a breaking change that we didn’t mention for the 3.6 beta.