Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Track recursive callback-covariance calls to compareSignaturesRelated #19078

Closed
wants to merge 2 commits into from
Closed

Track recursive callback-covariance calls to compareSignaturesRelated #19078

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b0a6444
Track recursive calls to compareSignaturesRelated
sandersn Oct 10, 2017
d935422
Test fix #18277:mutually recursive callbacks
sandersn Oct 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8521,7 +8521,7 @@ namespace ts {
target: Signature,
ignoreReturnTypes: boolean): boolean {
return compareSignaturesRelated(source, target, CallbackCheck.None, ignoreReturnTypes, /*reportErrors*/ false,
/*errorReporter*/ undefined, compareTypesAssignable) !== Ternary.False;
/*errorReporter*/ undefined, compareTypesAssignable, createMap<true>()) !== Ternary.False;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

type ErrorReporter = (message: DiagnosticMessage, arg0?: string, arg1?: string) => void;
Expand All @@ -8535,7 +8535,8 @@ namespace ts {
ignoreReturnTypes: boolean,
reportErrors: boolean,
errorReporter: ErrorReporter,
compareTypes: TypeComparer): Ternary {
compareTypes: TypeComparer,
previousCallbacks: Map<true>): Ternary {
// TODO (drosen): De-duplicate code between related functions.
if (source === target) {
return Ternary.True;
Expand Down Expand Up @@ -8589,10 +8590,19 @@ namespace ts {
// 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.
const callbacks = sourceSig && targetSig && !sourceSig.typePredicate && !targetSig.typePredicate &&
let callbacks = sourceSig && targetSig && !sourceSig.typePredicate && !targetSig.typePredicate &&
(getFalsyFlags(sourceType) & TypeFlags.Nullable) === (getFalsyFlags(targetType) & TypeFlags.Nullable);
if (callbacks) {
const key = getRelationKey(sourceType, targetType, /*relation*/ undefined);
if (previousCallbacks.has(key)) {
callbacks = false;
}
else {
previousCallbacks.set(key, true);
}
}
const related = callbacks ?
compareSignaturesRelated(targetSig, sourceSig, strictVariance ? CallbackCheck.Strict : CallbackCheck.Bivariant, /*ignoreReturnTypes*/ false, reportErrors, errorReporter, compareTypes) :
compareSignaturesRelated(targetSig, sourceSig, strictVariance ? CallbackCheck.Strict : CallbackCheck.Bivariant, /*ignoreReturnTypes*/ false, reportErrors, errorReporter, compareTypes, previousCallbacks) :
!callbackCheck && !strictVariance && compareTypes(sourceType, targetType, /*reportErrors*/ false) || compareTypes(targetType, sourceType, reportErrors);
if (!related) {
if (reportErrors) {
Expand Down Expand Up @@ -9736,7 +9746,7 @@ namespace ts {
*/
function signatureRelatedTo(source: Signature, target: Signature, erase: boolean, reportErrors: boolean): Ternary {
return compareSignaturesRelated(erase ? getErasedSignature(source) : source, erase ? getErasedSignature(target) : target,
CallbackCheck.None, /*ignoreReturnTypes*/ false, reportErrors, reportError, isRelatedTo);
CallbackCheck.None, /*ignoreReturnTypes*/ false, reportErrors, reportError, isRelatedTo, createMap<true>());
}

function signaturesIdenticalTo(source: Type, target: Type, kind: SignatureKind): Ternary {
Expand Down
35 changes: 35 additions & 0 deletions tests/baselines/reference/mutuallyRecursiveCallbacks.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
tests/cases/compiler/mutuallyRecursiveCallbacks.ts(6,1): error TS2322: Type '<T>(bar: Bar<T>) => void' is not assignable to type 'Bar<{}>'.
Types of parameters 'bar' and 'foo' are incompatible.
Types of parameters 'bar' and 'foo' are incompatible.
Types of parameters 'bar' and 'foo' are incompatible.
Type 'Foo<{}>' is not assignable to type 'Bar<{}>'.
Types of parameters 'bar' and 'foo' are incompatible.
Types of parameters 'bar' and 'foo' are incompatible.
Types of parameters 'bar' and 'foo' are incompatible.
Type 'Foo<{}>' is not assignable to type 'Bar<{}>'.
Types of parameters 'bar' and 'foo' are incompatible.
Types of parameters 'bar' and 'foo' are incompatible.
Type 'void' is not assignable to type 'Foo<{}>'.


==== tests/cases/compiler/mutuallyRecursiveCallbacks.ts (1 errors) ====
// #18277
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;
~~~
!!! error TS2322: Type '<T>(bar: Bar<T>) => void' is not assignable to type 'Bar<{}>'.
!!! error TS2322: Types of parameters 'bar' and 'foo' are incompatible.
!!! error TS2322: Types of parameters 'bar' and 'foo' are incompatible.
!!! error TS2322: Types of parameters 'bar' and 'foo' are incompatible.
!!! error TS2322: Type 'Foo<{}>' is not assignable to type 'Bar<{}>'.
!!! error TS2322: Types of parameters 'bar' and 'foo' are incompatible.
!!! error TS2322: Types of parameters 'bar' and 'foo' are incompatible.
!!! error TS2322: Types of parameters 'bar' and 'foo' are incompatible.
!!! error TS2322: Type 'Foo<{}>' is not assignable to type 'Bar<{}>'.
!!! error TS2322: Types of parameters 'bar' and 'foo' are incompatible.
!!! error TS2322: Types of parameters 'bar' and 'foo' are incompatible.
!!! error TS2322: Type 'void' is not assignable to type 'Foo<{}>'.

12 changes: 12 additions & 0 deletions tests/baselines/reference/mutuallyRecursiveCallbacks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
//// [mutuallyRecursiveCallbacks.ts]
// #18277
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;


//// [mutuallyRecursiveCallbacks.js]
;
bar = foo;
33 changes: 33 additions & 0 deletions tests/baselines/reference/mutuallyRecursiveCallbacks.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
=== tests/cases/compiler/mutuallyRecursiveCallbacks.ts ===
// #18277
interface Foo<T> { (bar: Bar<T>): void };
>Foo : Symbol(Foo, Decl(mutuallyRecursiveCallbacks.ts, 0, 0))
>T : Symbol(T, Decl(mutuallyRecursiveCallbacks.ts, 1, 14))
>bar : Symbol(bar, Decl(mutuallyRecursiveCallbacks.ts, 1, 20))
>Bar : Symbol(Bar, Decl(mutuallyRecursiveCallbacks.ts, 1, 41))
>T : Symbol(T, Decl(mutuallyRecursiveCallbacks.ts, 1, 14))

type Bar<T> = (foo: Foo<T>) => Foo<T>;
>Bar : Symbol(Bar, Decl(mutuallyRecursiveCallbacks.ts, 1, 41))
>T : Symbol(T, Decl(mutuallyRecursiveCallbacks.ts, 2, 9))
>foo : Symbol(foo, Decl(mutuallyRecursiveCallbacks.ts, 2, 15))
>Foo : Symbol(Foo, Decl(mutuallyRecursiveCallbacks.ts, 0, 0))
>T : Symbol(T, Decl(mutuallyRecursiveCallbacks.ts, 2, 9))
>Foo : Symbol(Foo, Decl(mutuallyRecursiveCallbacks.ts, 0, 0))
>T : Symbol(T, Decl(mutuallyRecursiveCallbacks.ts, 2, 9))

declare function foo<T>(bar: Bar<T>): void;
>foo : Symbol(foo, Decl(mutuallyRecursiveCallbacks.ts, 2, 38))
>T : Symbol(T, Decl(mutuallyRecursiveCallbacks.ts, 3, 21))
>bar : Symbol(bar, Decl(mutuallyRecursiveCallbacks.ts, 3, 24))
>Bar : Symbol(Bar, Decl(mutuallyRecursiveCallbacks.ts, 1, 41))
>T : Symbol(T, Decl(mutuallyRecursiveCallbacks.ts, 3, 21))

declare var bar: Bar<{}>;
>bar : Symbol(bar, Decl(mutuallyRecursiveCallbacks.ts, 4, 11))
>Bar : Symbol(Bar, Decl(mutuallyRecursiveCallbacks.ts, 1, 41))

bar = foo;
>bar : Symbol(bar, Decl(mutuallyRecursiveCallbacks.ts, 4, 11))
>foo : Symbol(foo, Decl(mutuallyRecursiveCallbacks.ts, 2, 38))

34 changes: 34 additions & 0 deletions tests/baselines/reference/mutuallyRecursiveCallbacks.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
=== tests/cases/compiler/mutuallyRecursiveCallbacks.ts ===
// #18277
interface Foo<T> { (bar: Bar<T>): void };
>Foo : Foo<T>
>T : T
>bar : Bar<T>
>Bar : Bar<T>
>T : T

type Bar<T> = (foo: Foo<T>) => Foo<T>;
>Bar : Bar<T>
>T : T
>foo : Foo<T>
>Foo : Foo<T>
>T : T
>Foo : Foo<T>
>T : T

declare function foo<T>(bar: Bar<T>): void;
>foo : <T>(bar: Bar<T>) => void
>T : T
>bar : Bar<T>
>Bar : Bar<T>
>T : T

declare var bar: Bar<{}>;
>bar : Bar<{}>
>Bar : Bar<T>

bar = foo;
>bar = foo : <T>(bar: Bar<T>) => void
>bar : Bar<{}>
>foo : <T>(bar: Bar<T>) => void

6 changes: 6 additions & 0 deletions tests/cases/compiler/mutuallyRecursiveCallbacks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// #18277
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;