Track recursive callback-covariance calls to compareSignaturesRelated

[sandersn]

Previously, compareSignaturesRelated would try to relate (parameters of) callback parameters covariantly without checking whether the two signatures were already being checked. This led to an infinite recursion when checking recursive types with callbacks:

interface Foo<T> { (bar: Bar<T>): void }
type Bar<T> = (foo: Foo<T>) => Foo<T>
declare function foo<T>(bar: Bar<T>): void
declare var bar: Bar<{}>
bar = foo

The fix is to track which callbacks are already being compared. The mechanism is per-call to compareSignaturesRelated, so it only stops recursions on the callback-covariance code path. However, the maybe cache check in recursiveTypeRelatedTo will catch infinite recursions on the usual code path through isRelatedTo.

Fixes #18277

[ahejlsberg]

Couldn't you just pass a type comparer function that checks the map? That way you could keep all of the logic right here and not have to add another parameter to compareSignaturesRelated.

[sandersn]

I tried this but because compareTypes isn't actually called in the tight recursive loop of compareSignaturesRelated, I have to put the whole decision inside the type comparer:

        function createRecursiveSignatureTypeComparer(compareTypes: TypeComparer, callbackCheck: CallbackCheck, target: Signature, errorReporter: ErrorReporter): TypeComparer {
            const previousCallbacks = createMap<true>();
            const kind = target.declaration ? target.declaration.kind : SyntaxKind.Unknown;
            const strictVariance = !callbackCheck && strictFunctionTypes && kind !== SyntaxKind.MethodDeclaration &&
                kind !== SyntaxKind.MethodSignature && kind !== SyntaxKind.Constructor;
            return (source, target, reportErrors) => {
                const sourceSig = getSingleCallSignature(getNonNullableType(source));
                const targetSig = getSingleCallSignature(getNonNullableType(target));
                // In order to ensure that any generic type Foo<T> is at least co-variant with respect to T no matter
                // how Foo uses T, we need to relate parameters bi-variantly (given that parameters are input positions,
                // they naturally relate only contra-variantly). However, if the source and target parameters both have
                // function types with a single call signature, we known we are relating two callback parameters. In
                // that case it is sufficient to only relate the parameters of the signatures co-variantly because,
                // similar to return values, callback parameters are output positions. This means that a Promise<T>,
                // where T is used only in callback parameter positions, will be co-variant (as opposed to bi-variant)
                // with respect to T.
                let callbacks = sourceSig && targetSig && !sourceSig.typePredicate && !targetSig.typePredicate &&
                    (getFalsyFlags(source) & TypeFlags.Nullable) === (getFalsyFlags(target) & TypeFlags.Nullable);
                if (callbacks) {
                    const key = getRelationKey(source, target, /*relation*/ undefined);
                    if (!previousCallbacks.has(key)) {
                        previousCallbacks.set(key, true);
                        return compareSignaturesRelated(targetSig, sourceSig, strictVariance ? CallbackCheck.Strict : CallbackCheck.Bivariant, /*ignoreReturnTypes*/ false, reportErrors, errorReporter, compareTypes);
                    }
                }
                return !callbackCheck && !strictVariance && compareTypes(source, target, /*reportErrors*/ false) || compareTypes(target, source, reportErrors);
            };
        }

Now the inner loop of parameter checking is clean:

            for (let i = 0; i < checkCount; i++) {
                const sourceType = i < sourceMax ? getTypeOfParameter(sourceParams[i]) : getRestTypeOfSignature(source);
                const targetType = i < targetMax ? getTypeOfParameter(targetParams[i]) : getRestTypeOfSignature(target);
                const related = compareTypes(sourceType, targetType, reportErrors);
                if (!related) {
                    if (reportErrors) {
                        errorReporter(Diagnostics.Types_of_parameters_0_and_1_are_incompatible,
                            symbolName(sourceParams[i < sourceMax ? i : sourceMax]),
                            symbolName(targetParams[i < targetMax ? i : targetMax]));
                    }
                    return Ternary.False;
                }
                result &= related;
            }

But I think this part of the algorithm reads better in situ, and the move actually requires more code churn at the call sites of compareSignaturesRelatedTo because createRecursiveSignatureTypeComparer has so many parameters.

To help the code churn at call sites, I could instead just make a compareSignaturesRelated/compareSignaturesRelatedWorker pair in order to hide the creation of the map.

[ahejlsberg]

Turns out there's an even simpler fix. I have put it up in #19107.