-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Inferences made when trying to match an overload are carried over to matching consecutive overloads #46312
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
Comments
I'm not sure if this is the same issue, but possibly simpler reproduction: type MaybeCbCallback<T extends any[]> = (...args: T) => void;
function maybeCallback<T extends any[]>(callback: MaybeCbCallback<T>, ...args: T): void;
function maybeCallback<T extends any[]>(callback: undefined, ...args: T): Promise<any>;
function maybeCallback<T extends any[]>(
callback: MaybeCbCallback<T> | null | undefined,
...args: T
): Promise<any> | void {
// Implementation doesn't matter
}
function testFun(cb?: () => void) {
// Error here: No overload matches this call.
if (Math.random() > 0.5) maybeCallback(cb);
// No error here:
if (cb) {
return maybeCallback(cb);
} else {
return maybeCallback(cb);
}
} |
@AlCalzone I don't believe those are the same. I've created even more simplified demo of your issue: declare function acceptMaybeNumber(arg: number): void
declare function acceptMaybeNumber(arg: undefined): void
declare const arg: number | undefined
acceptMaybeNumber(arg) // this errors The problem here is that each overload is type-checked separately and your argument that is a union doesn't satisfy any of those overloads on its own. TypeScript doesn't try to match this argument against all of the overloads at once. As to my problem... I've spent a significant amount of time investigating this in the debugger. And the root reason behind this issue is quite mundane - it's caching (at least to some extent). The problem is that we are dealing with some nested calls here and while trying to choose the first overload the inferences are made for the inner |
Smartly discarding the caches totally works and is doable - I've implemented like three different ways to do it over the last few years - it just results in two issues. One being our inferences change in many places - turns out sometimes pulling data from the first overload works in our favor. Two is that perf is across the board worse - keeping that cached data around really reduces the amount of work we do. Figuring out if those are OK, or, if not, mitigations to them, is the challenge right now. |
I can totally see the potential impact on the performance (less caching -> more work 😅 ). I would be very interested though to see the counter-example where pulling data from the first overload is preferable. This seems a little bit counter-intuitive - because the whole context changes between overloads so how persisting the information computed based on the previous (outdated/incorrect) context can yield better results? Unless perhaps it's about cases where the inference would just fail to compute anything meaningful for the second overload - but then the issue would be in the signature itself. This would just act in the same way as if this signature would be alone, without any other accompanying overloads at all. And, of course, you are way more competent here to understand the whole thing, its implications etc. I literally mean that I'm interested in this as it goes against my intuition (that might be wrong here). |
It's exactly that - but we probably can't just go "sorry, your overload list that's worked fine for a few years is actually wrong" - it'd just break too much. :( Maybe we could do something where we fall back to our current behavior if we have no inferences if we come up empty at the end of processing the signatures in isolation? But that'll just make perf twice as bad in those instances... Ungh. |
Isn't it though something that happens with each minor release of TypeScript? Such situations are not that uncommon from my experience anyway. It's, of course, hard to assess the ripple effect in advance though.
Do you perhaps have any links to specific situations where this matters? Maybe you could find some good references in some closed PRs of yours or something. I would love to get a better grasp of what we are talking about. And ye - not only the perf hit but also added complexity to the code of the proposed solution here are suboptimal 😬 OTOH maybe if we could cache the "first results" on the side and only reuse them if we fail to match something more useful for the next overloads then it wouldn't be as bad? In such a scenario the perf hit would only come from not reusing the first cache when matching subsequent overloads - but the "fallback" to the first match would be instant~ |
Another example on overloading from two different interfaces: interface F0 {
(): 1;
}
interface F1 {
(v: number): 2;
}
declare const f: F0 | F1; // f: (v: number) => 1 | 2
f() // failed, `v` is required |
This is a different issue though - this one here is specifically about inference. To "create" overloaded function using existing function signatures you need to use an intersection and not a union, see here |
Um... I can see that it works with intersection. It goes roughly like this: interface WithoutValue {
(): 1
}
interface WithValue<V> {
(v: V): 2
}
type X<V extends Record<string, any> | undefined = Record<string, any> | undefined> = V extends undefined ? WithoutValue : WithValue<V>
let x: X
x()
let y: X<undefined>
y()
let z: X<{ a: 1 }>
z({ a: 1 }) Note: this example is still not what I'm seeing. In my actual case, it is most definitely a bug. The |
The actual error is here: https://github.com/unional/events-plus/blob/fsa-error/ts/supportFSA.spec.ts#L43 The type of You can see that calling with no param is fine as in https://github.com/unional/events-plus/blob/fsa-error/ts/supportFSA.spec.ts#L32 because I have declare a the signature of But the return type of You can see the error directly in sandbox I'm trying to reduce that to a simpler example. |
Bug Report
🔎 Search Terms
overload inference generic consecutive subsequent following
the only issue that I have found that might be related to this is this one but I can't assess on my own if they are the same or not
🕗 Version & Regression Information
This is the behavior present in all 4.x versions available on the playground, including 4.5-beta.
⏯ Playground Link
Playground link with relevant code
💻 Code
🙁 Actual behavior
No overload signature matches
🙂 Expected behavior
I would expect a signature to match when it's used as one of the overloads if it matches on its own. And I would expect for inferences made in a failed attempt to match a signature to have no effect on matching the following signatures.
The text was updated successfully, but these errors were encountered: