microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.4k stars 12.4k forks source link

Inferences made when trying to match an overload are carried over to matching consecutive overloads #46312

Open Andarist opened 2 years ago

Andarist commented 2 years ago

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

interface TypegenDisabled {
  "@@xstate/typegen": false;
}
interface TypegenEnabled {
  "@@xstate/typegen": true;
}
interface EventObject {
  type: string;
}
interface ActionObject<TEvent extends EventObject> {
  _TE: TEvent;
}

interface StateMachine<
  TEvent extends EventObject,
  TTypesMeta = TypegenDisabled
> {
  _TE: TEvent;
  _TRTM: TTypesMeta;
}

interface MachineOptions<TEvent extends EventObject> {
  action?: ActionObject<TEvent>;
}

type MaybeTypegenMachineOptions<
  TEvent extends EventObject,
  TTypesMeta = TypegenDisabled
> = TTypesMeta extends TypegenEnabled
  ? {
      action?: ActionObject<{ type: "WITH_TYPEGEN" }>;
    }
  : MachineOptions<TEvent>;

declare function assign<TEvent extends EventObject>(
  assignment: (ev: TEvent) => void
): ActionObject<TEvent>;

// atm I have a single signature and it **matches**
// however, if I uncomment this additional overload then **no signature** matches
// if later on I reorder the signatures then the first one (the one that is currently not commented out) matches

// what happens here is that some inferences made when attempting to match the first overload are cached 
// and "carried over" to matching the second overload - which makes the whole thing fail

// declare function useMachine<
//   TEvent extends EventObject,
//   TTypesMeta extends TypegenEnabled
// >(
//   getMachine: StateMachine<TEvent, TTypesMeta>,
//   options: MaybeTypegenMachineOptions<TEvent, TTypesMeta>
// ): { first: true };
declare function useMachine<TEvent extends EventObject>(
  getMachine: StateMachine<TEvent>,
  options?: MachineOptions<TEvent>
): { second: true };

const machine = {} as StateMachine<{ type: "WITHOUT_TYPEGEN" }>;

const ret = useMachine(machine, {
  action: assign((_ev) => {
    ((_type: "WITHOUT_TYPEGEN") => null)(_ev.type);
  }),
});

πŸ™ 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.

AlCalzone commented 2 years ago

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);
    }
}

Playground link

Andarist commented 2 years ago

@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

Playground link

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 assign call. The first overload doesn't match but the inference made to assign persists in its .resolvedType (or a similar) - and when we get to matching the second overload this type is just being reused, causing this type error. If we could only smartly discard the cached value then it would "just work". I've even managed to bypass this prtical cache (conditionally), just as an experiment, but I've then hit another layer of caching elsewhere and I gave up.

weswigham commented 2 years ago

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.

Andarist commented 2 years ago

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).

weswigham commented 2 years ago

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.

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.

Andarist commented 2 years ago

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. :(

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.

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.

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~

unional commented 2 years ago

Another example on overloading from two different interfaces: They are merged instead of overloads.

interface F0 {
  (): 1;
}

interface F1 {
  (v: number): 2;
}

declare const f: F0 | F1; // f: (v: number) => 1 | 2
f() // failed, `v` is required

playground

Andarist commented 2 years ago

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

unional commented 2 years ago

Um... I can see that it works with intersection. The example above is probably the cause of the actual problem that I'm facing, which involves generics and inference.

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 })

playground

Note: this example is still not what I'm seeing. In my actual case, it is most definitely a bug. The x() actually works but x() and z({ a: 1 }) returns the same type.

unional commented 2 years ago

The actual error is here: https://github.com/unional/events-plus/blob/fsa-error/ts/supportFSA.spec.ts#L43

The type of event is const event: FSAEventBare<string> | FSAEventWithErrorPayload<string, Error>.

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 () => FSA<...> on both interfaces.

But the return type of event({ error: true }) is wrong (it gets the return type FSA<...>, the return type of () => ..., instead of ErrorFSABare<>)

You can see the error directly in sandbox

I'm trying to reduce that to a simpler example.