Compiler crashes with generics

[mohsen1]

TypeScript Version: "^3.8.3"

"No error for last overload signature" error

Code
I could narrow it down to this repository:

https://github.com/mohsen1/TS-No-error-for-last-overload-signature

https://github.com/mohsen1/TS-No-error-for-last-overload-signature/blob/fbd33f9aad6b3fd1f914b6bfe23cebc5db1dd12f/src/index.ts#L22-L23

wrapResolver( resolver(AModel) || [])

Expected behavior:
Type error, wrapResolver is not suppose to get an array
Actual behavior:
Compiler crash

Related Issues:
#33133
#33732
#35186

[andrewbranch]

Minimal repro:

// @strict: true

declare function resolver<T>(): () => void;
declare function wrapResolver<T>(resolverFunction: () => void): void;

// compiler crashes here
wrapResolver(resolver() || [])

[andrewbranch]

Here’s what’s happening.

1. In resolveCall on the wrapResolver call expression, we call chooseOverload.
   1. We need to infer type parameters, so we infer the type of resolver() || [] with the contextual type provided by wrapResolver’s signature.
      1. Because resolver() is a call expression on a generic function-returning function, we skip it and use nonInferrableType (a never type). (There’s a big comment explaining why in resolveCallExpression.) In this contrived minimal repro, it doesn’t actually matter what we infer for resolver() || [] at this point, because it doesn’t help infer the type parameter. The important thing is just that we marked the inference context as SkippedGenericFunction.
      2. We obviously infer unknown for T in the minimal repro. Doesn’t really matter.
   2. We instantiate the wrapResolver signature with the fairly useless inference we just did.
   3. Now we want to see if the argument we have is compatible with the signature we just instantiated by calling getSignatureApplicabilityError. Because the inference context was marked SkippedGenericFunction, we call it with a check mode of CheckMode.SkipGenericFunctions.
      1. We see if the argument type is assignable to the parameter type, using the check mode we were passed.
         1. Again we get the type of resolver() || [] and again, because of SkipGenericFunction, the left side is nonInferrableType, making the type of the binary expression equivalent to the type of [].
         2. This time it matters, because the contextual type of [] is not assignable to the parameter type, () => void. We return signaling that this is an error.
      2. Since we got an error on this signature, we add it to a list of overloads that aren’t going to work and move on to try other overloads.
      3. There are no other overloads, so we return undefined from chooseOverload, indicating that there are no suitable overloads.
2. Since there are allegedly no overloads, we take the list of overloads that didn’t work (just the one) and try to come up with a good error message for it.
   1. We call getSignatureApplicabilityError again, but this time pass CheckMode.Normal.
      1. Under CheckMode.Normal, the type of resolver() is () => void instead of nonInferrableType, and since that’s definitely not falsey, the whole argument type is () => void, which is assignable to the parameter type, so there’s no error.
3. Thus, we couldn’t find a signature that works, but we also couldn’t generate an error for it. Crash.

Clearly, we should have recognized that the instantiated signature is a match from the beginning, but I’m not sure how. Everything I try tweaking breaks a bunch of other tests. It seems a little weird to me that nonInferrableType disappears from a logical or expression because it’s actually never under the hood (and the TypeFacts for never are TypeFacts.All). @weswigham does this breakdown give you any ideas of what’s wrong?

[ahejlsberg]

@andrewbranch I just put up a PR that fixes this by propagating the nonInferrableType in the &&, || and ?? operators.

A bit more detail... The nonInferrableType is a never wildcard type we use in intermediate phases of overload resolution to indicate that a particular argument of a function type should be ignored for purposes of type argument inference and signature applicability checks. Since the &&, ||, and ?? operators can be used with function type arguments, and then produce function type results, we need to propagate the nonInferrableType through these operators, but we weren't doing that.

An argument could be made that any operation applied to a never operand should produce a never result (if one operand is never then the operation never happens and so itself should be never). Indeed, we do such propagation for the silentNever type used by CFA to represent incomplete types during loop analysis. However, we've chosen to instead error on never types to ease diagnosing their origins.

Aside from the fix to this particular bug, I'm somewhat concerned with the brittleness of the error reporting logic in resolveCall. We've now fixed multiple bugs that hit one or the other of the two Debug.fail calls in there, and I'm not at all convinced there aren't more latent issues. We should take a good look at revising that logic--which, by the way, I find to be very hard to follow.

[weswigham]

The Debug.fail calls were added because the failure mode is even more insidious without them - if the signature applicability error fail is triggered, it means we witnessed some kind of caching inconsistency (and so the check results may change just based on check order, which's bad), or logical inconsistency. Specifically, we always perform a first check with error reporting off (part of this is to handle overloads - we don't want to report anything on an overload that we end up not matching), optionally with some looser check mode setting - if that fails and we go to rerun in normal check mode to generate the error (which should be stricter and flag more errors, not less), and don't generate an error, then either we cached something during the first resolution that prevents us from redoing the calculation (which is bad), or the "relative strictness" of the CheckModes aren't being upheld (namely that CheckMode.Normal will always issue an error in any instance any other CheckMode would).

In this case, based on the fix and your description, it looks like it was a logical error - the CheckMode.SkipGenericFunctions flag had a logical difference from CheckMode.Normal that made it issue an error a normal check would not issue, specifically calculating too specific and expression type for an expression involving a (skipped) generic function under test.

I don't think there's a great way to rewrite the error handling to still work - what sucks and is hard to reason about is the implict invariant that CheckMode.Normal always issues an error if any other check mode would have. There's nothing enforcing that, and iirc also not even any documentation noting it (it simply arises as a requirement based on how errors are supposed to be calculated).

[JesusTheHun]

Any chance this drops into a 3.8.4 hotfix release ? this bug is really blocking me on a big project, maybe you know a workaround to wait a later release ?

[andrewbranch]

@JesusTheHun I’d have to see your specific code to be able to advise on a workaround—in the original repro, there’s a simple workaround of removing a meaningless || [] where the left side of that expression was always truthy. But, the debug message here can indicate a number of different problems, so your code may or may not even be fixed by this. Can you reproduce your crash on the TypeScript nightly release?

[JesusTheHun]

I have created a small repo to show case an issue I have with my IDE that may actually be a TS performance issue, I got lucky this small repo crashes on compilation : https://github.com/JesusTheHun/typescript-slow-case

Edit : doesn't crash with 3.9.0-dev.20200422