-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Union signatures should exclude "uncallable" callbacks, so methods like .map
work on union containing empty array
#42558
Comments
@weswigham what do you think? |
I think this is morally equivalent to producing the union of the possibilities for the inferred parameter type, which iirc, we've declined to do in the past because it's a breaking change unless you're in |
Errr, scratch that, I'm thinking of overloads. For union's signatures our issue I'm this case is really that both signatures have generics, and those generics need to be reconciled. I actually thought I had a PR out (or already in?) that merged generics when they came from the same base signature, and thus allowed the signatures to merge... |
@weswigham it shouldn't be a breaking change for any existing code - TS 4.1 and below typically rejects anything with union signatures that are complicated enough to trigger this change. Since TS 4.2(dev) fails to infer callback parameter types in the one case this doesn't work, so you'd need type annotation to get it to compile. In that case, the type signature might be slightly refined after this change, but not enough to reject existing annotations (since e.g. It also goes beyond generic code; this is a non-generic example (in TS 4.2: const f: (ask1: (x: string, y: never) => void, otherThing: string) => void = null as any;
const g: (ask2: (z: number, w: string) => void, moreThing: number) => void = null as any;
const h = Math.random() < 1/2 ? f : g;
h((a, b) => {
return;
});
Since So Also for the record, the implementation at #42559 didn't find any breakages in existing code, and I think it would be exceedingly unlikely to find any. Union signatures aren't common, and they're much less common when they ask for callbacks that ask for arguments with type |
@weswigham I want to ping on this issue again, since it's something I have run into in a few different ways in TS. This is not about merging generic params on signatures - tsc already does this. This is just about skipping callback parameters when these signatures are merged. Effectively, it's a much-more constrained version of the following typing rule (which is sound, but TS currently does not use):
However, this is likely to break existing code in unexpected ways, since intersections show up in a lot of places. Instead, we only perform this reduction when combining callback parameters in union signatures. Which is a very uncommon place for them to show up, but when they do, they cause problems. This fixes them, while having very minimal fallout. |
Is this addressed by #42620? |
All the cases I originally cared about seem to be solved in 4.3 beta, so looks good to me! Thanks! |
Suggestion
When merging union signatures, skip "uncallable" callbacks: those that request a
never
callback argument. These functions can't be called, so they can be removed from consideration when building the function corresponding to a union signature.💻 Use Cases
TypeScript sometimes infers empty arrays as having type
never[]
. Usually, this doesn't cause problems, until it gets unioned with another type, and you try to call it as a function/method:In this example, since the
[]
is typed asnever[]
, the.map
method has a union type with two incompatible types; one possibility is astring
,number
,string[]
argument list returning a genericU
, and the other is anever
,number
,never[]
argument list returning a genericU
.In TypeScript 4.1, the two signatures were considered "incompatible", so attempting to call the function would raise an error. TypeScript 4.2 improves this situation slightly, since the function is now callable; however, it fails to infer the right types for the callback parameters, and so they get defaulted to
any
.However, there's a generic rule which can be used to "fix" the union signature so that the correct typing machinery is activated! Since "legal" code can't create values of type
never
, legal code can't call functions with any argument having typenever
. For brevity, a callback where any of its required, non-rest parameters have typenever
is called "uncallable".To avoid introducing too many new bugs or soundness holes, we can restrict this reasoning just to where union signatures are created. Specifically, when merging two callback types, if one of them is "uncallable" and the other is not, then we use the "callable" one for their intersection.
TypeScript currently handles a more-restrictive form of this check. It's able to see that (for example)
in this case,
mapSomething
has the same type asmapNums
, since it can verify that the types are "compatible". The problem happens when an "irrelevant" type appears, which often occurs for "methods" since they have an implicit or explicitthis
parameter:now, the
x
andy
parameters prevent tsc from assigning a subtyping relation between the two functions. However, sincemapNever
still can't be called, its type has effectively not changed from the previous example! So it's safe to merge them by treating it as a subtype still. That's what this change proposes, to handle some of the rough-cases wherenever
doesn't aggressively "poison" the types it's wrapped in.🔍 Search Terms
Vaguely related, but broader/different from this proposal: #40157
never
Union of subtypes should be usable as the supertype #38048I previously opened and closed #42487 since I originally didn't know about the change in behavior in 4.2.
✅ Viability Checklist
My suggestion meets these guidelines:
The text was updated successfully, but these errors were encountered: