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.

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

Implement new overload compatibility checking #6075

Merged
merged 21 commits into from
Dec 18, 2015
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
2cc6c4c
Fixed test to explicitly have return type annotation.
DanielRosenwasser Dec 8, 2015
5ef8a30
Accepted baselines.
DanielRosenwasser Dec 8, 2015
2c2b8be
Remove empty test.
DanielRosenwasser Dec 8, 2015
820a2cb
Added some tests for overload compatibility.
DanielRosenwasser Dec 8, 2015
5d94e48
Accepted baselines.
DanielRosenwasser Dec 8, 2015
dc2af2f
Use different relation for overload compatibility.
DanielRosenwasser Dec 9, 2015
e1004fa
Removed misleading comment from test.
DanielRosenwasser Dec 9, 2015
51d2409
Accepted baselines.
DanielRosenwasser Dec 9, 2015
fc170da
Merge branch 'master' into overloadCompatibility
DanielRosenwasser Dec 11, 2015
e9dc011
Fixed unnecessary error in test.
DanielRosenwasser Dec 11, 2015
8cceedd
Accepted baselines.
DanielRosenwasser Dec 11, 2015
3cdfc36
Added tests for 'void' return type compatibilty on overloads and impl…
DanielRosenwasser Dec 11, 2015
069fada
Accepted baselines.
DanielRosenwasser Dec 11, 2015
9b507b7
Specifically test for 'void' to permit implementations to return more…
DanielRosenwasser Dec 11, 2015
e012645
Accepted baselines.
DanielRosenwasser Dec 11, 2015
a236461
Reversed order of checks, since the implementation will typically be …
DanielRosenwasser Dec 11, 2015
73526cf
Do some caching so that we don't repeat the same work for the impleme…
DanielRosenwasser Dec 12, 2015
5cbf17d
Added tests.
DanielRosenwasser Dec 14, 2015
2efa697
Accepted baselines.
DanielRosenwasser Dec 14, 2015
d8db60a
Use a comparison function instead of creating a new type for each sig…
DanielRosenwasser Dec 16, 2015
731925b
Fix linter nits.
DanielRosenwasser Dec 16, 2015
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
35 changes: 34 additions & 1 deletion src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4115,6 +4115,16 @@ namespace ts {
return signature.erasedSignatureCache;
}

function getAnyReturningErasedSignature(signature: Signature): Signature {
if (!signature.anyReturningErasedSignatureCache) {
const erasedSignature = getErasedSignature(signature);
const anyReturningErasedSignature = cloneSignature(erasedSignature);
anyReturningErasedSignature.resolvedReturnType = anyType;
signature.anyReturningErasedSignatureCache = anyReturningErasedSignature;
}
return signature.anyReturningErasedSignatureCache;
}

function getOrCreateTypeFromSignature(signature: Signature): ObjectType {
// There are two ways to declare a construct signature, one is by declaring a class constructor
// using the constructor keyword, and the other is declaring a bare construct signature in an
Expand Down Expand Up @@ -4981,6 +4991,29 @@ namespace ts {
return checkTypeRelatedTo(sourceType, targetType, assignableRelation, /*errorNode*/ undefined);
}

function isImplementationCompatibleWithOverload(implementation: Signature, overload: Signature): boolean {
const erasedSource = getErasedSignature(implementation);
const erasedTarget = getErasedSignature(overload);

// First see if the return types are compatible in either direction.
const sourceReturnType = getReturnTypeOfSignature(erasedSource);
const targetReturnType = getReturnTypeOfSignature(erasedTarget);
if (targetReturnType === voidType
|| checkTypeRelatedTo(targetReturnType, sourceReturnType, assignableRelation, /*errorNode*/ undefined)
|| checkTypeRelatedTo(sourceReturnType, targetReturnType, assignableRelation, /*errorNode*/ undefined)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In response to @weswigham's point #943 (comment), I believe the special casing of signature return types wouldn't come into play here. The return types are being compared solely as types, and not signature return types. So I think boolean would be assignable to a type predicate in this context, which is indeed desirable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly, and you can see that in stringLiteralTypesAsTags01.ts. I'll have to add more test cases though.

Copy link
Contributor

Choose a reason for hiding this comment

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

My ultimate point is that concern number 2 in #943 (comment) is not an issue, since boolean would work in the /*????*/ slot.


// The return types are compatible, so create versions of the signature with 'any' as the return type.
// We need to do this so that we can check assignability while disregarding the return type.
const anyReturningSource = getAnyReturningErasedSignature(implementation);
const anyReturningTarget = getAnyReturningErasedSignature(overload);

// Create object types to actually perform relation checks.
const anyReturningSourceType = getOrCreateTypeFromSignature(anyReturningSource);
const anyReturningTargetType = getOrCreateTypeFromSignature(anyReturningTarget);
return checkTypeRelatedTo(anyReturningSourceType, anyReturningTargetType, assignableRelation, /*errorNode*/ undefined);
Copy link
Member

Choose a reason for hiding this comment

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

We're computing and caching an awful lot of types here. They'll sit in the cache of their associated signature forever and also gum up the assignable relation cache. I think we need a function similar to compareSignatures that checks for assignability instead of identity. The function could have an ignoreReturnTypes flag and then you wouldn't need to create all of these new signatures and types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

One other possibility is to just call isSignatureAssignableTo in both directions. For the return types, doing this would yield the bivariance you are seeking. Parameter types are bivariant anyway, so each corresponding pair would just get checked in both directions twice. The only real difference is that you'd be extra permissive with the arity checks on the signatures. Namely, an overload would be allowed to be shorter (fewer parameters) than the implementation signature. You could add the necessary arity checks after the comparison to make sure the arities are only related in the direction you want. It's kind of nifty, though perhaps a bit more roundabout than the compareSignatures style approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

isSignatureAssignableTo was already doing the work I was trying to avoid, so I might as well just avoid the caching.

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've pushed out a change that does as @ahejlsberg's requested. There's no caching and no new type creation.

}
}

/**
* Checks if 'source' is related to 'target' (e.g.: is a assignable to).
* @param source The left-hand-side of the relation.
Expand Down Expand Up @@ -11778,7 +11811,7 @@ namespace ts {
//
// The implementation is completely unrelated to the specialized signature, yet we do not check this.
for (const signature of signatures) {
if (!signature.hasStringLiterals && !isSignatureAssignableTo(bodySignature, signature)) {
if (!signature.hasStringLiterals && !isImplementationCompatibleWithOverload(bodySignature, signature)) {
error(signature.declaration, Diagnostics.Overload_signature_is_not_compatible_with_function_implementation);
break;
}
Expand Down
2 changes: 2 additions & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2280,6 +2280,8 @@ namespace ts {
/* @internal */
erasedSignatureCache?: Signature; // Erased version of signature (deferred)
/* @internal */
anyReturningErasedSignatureCache?: Signature; // A version of the erased signature whose type returns 'any'
/* @internal */
isolatedSignatureType?: ObjectType; // A manufactured type that just contains the signature for purposes of signature comparison
}

Expand Down
37 changes: 0 additions & 37 deletions tests/baselines/reference/conformanceFunctionOverloads.js

This file was deleted.

25 changes: 0 additions & 25 deletions tests/baselines/reference/conformanceFunctionOverloads.symbols

This file was deleted.

25 changes: 0 additions & 25 deletions tests/baselines/reference/conformanceFunctionOverloads.types

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
tests/cases/conformance/functions/functionOverloadCompatibilityWithVoid01.ts(1,10): error TS2394: Overload signature is not compatible with function implementation.


==== tests/cases/conformance/functions/functionOverloadCompatibilityWithVoid01.ts (1 errors) ====
function f(x: string): number;
~
!!! error TS2394: Overload signature is not compatible with function implementation.
function f(x: string): void {
return;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
//// [functionOverloadCompatibilityWithVoid01.ts]
function f(x: string): number;
function f(x: string): void {
return;
}

//// [functionOverloadCompatibilityWithVoid01.js]
function f(x) {
return;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
//// [functionOverloadCompatibilityWithVoid02.ts]
function f(x: string): void;
function f(x: string): number {
return 0;
}

//// [functionOverloadCompatibilityWithVoid02.js]
function f(x) {
return 0;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
=== tests/cases/conformance/functions/functionOverloadCompatibilityWithVoid02.ts ===
function f(x: string): void;
>f : Symbol(f, Decl(functionOverloadCompatibilityWithVoid02.ts, 0, 0), Decl(functionOverloadCompatibilityWithVoid02.ts, 0, 28))
>x : Symbol(x, Decl(functionOverloadCompatibilityWithVoid02.ts, 0, 11))

function f(x: string): number {
>f : Symbol(f, Decl(functionOverloadCompatibilityWithVoid02.ts, 0, 0), Decl(functionOverloadCompatibilityWithVoid02.ts, 0, 28))
>x : Symbol(x, Decl(functionOverloadCompatibilityWithVoid02.ts, 1, 11))

return 0;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
=== tests/cases/conformance/functions/functionOverloadCompatibilityWithVoid02.ts ===
function f(x: string): void;
>f : (x: string) => void
>x : string

function f(x: string): number {
>f : (x: string) => void
>x : string

return 0;
>0 : number
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
//// [functionOverloadCompatibilityWithVoid03.ts]
function f(x: string): void;
function f(x: string): void {
return;
}

//// [functionOverloadCompatibilityWithVoid03.js]
function f(x) {
return;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
=== tests/cases/conformance/functions/functionOverloadCompatibilityWithVoid03.ts ===
function f(x: string): void;
>f : Symbol(f, Decl(functionOverloadCompatibilityWithVoid03.ts, 0, 0), Decl(functionOverloadCompatibilityWithVoid03.ts, 0, 28))
>x : Symbol(x, Decl(functionOverloadCompatibilityWithVoid03.ts, 0, 11))

function f(x: string): void {
>f : Symbol(f, Decl(functionOverloadCompatibilityWithVoid03.ts, 0, 0), Decl(functionOverloadCompatibilityWithVoid03.ts, 0, 28))
>x : Symbol(x, Decl(functionOverloadCompatibilityWithVoid03.ts, 1, 11))

return;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
=== tests/cases/conformance/functions/functionOverloadCompatibilityWithVoid03.ts ===
function f(x: string): void;
>f : (x: string) => void
>x : string

function f(x: string): void {
>f : (x: string) => void
>x : string

return;
}
10 changes: 0 additions & 10 deletions tests/baselines/reference/functionOverloads22.errors.txt

This file was deleted.

19 changes: 19 additions & 0 deletions tests/baselines/reference/functionOverloads22.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
=== tests/cases/compiler/functionOverloads22.ts ===
function foo(bar:number):{a:number;}[];
>foo : Symbol(foo, Decl(functionOverloads22.ts, 0, 0), Decl(functionOverloads22.ts, 0, 39), Decl(functionOverloads22.ts, 1, 49))
>bar : Symbol(bar, Decl(functionOverloads22.ts, 0, 13))
>a : Symbol(a, Decl(functionOverloads22.ts, 0, 26))

function foo(bar:string):{a:number; b:string;}[];
>foo : Symbol(foo, Decl(functionOverloads22.ts, 0, 0), Decl(functionOverloads22.ts, 0, 39), Decl(functionOverloads22.ts, 1, 49))
>bar : Symbol(bar, Decl(functionOverloads22.ts, 1, 13))
>a : Symbol(a, Decl(functionOverloads22.ts, 1, 26))
>b : Symbol(b, Decl(functionOverloads22.ts, 1, 35))

function foo(bar:any):{a:any;b?:any;}[] { return [{a:""}] }
>foo : Symbol(foo, Decl(functionOverloads22.ts, 0, 0), Decl(functionOverloads22.ts, 0, 39), Decl(functionOverloads22.ts, 1, 49))
>bar : Symbol(bar, Decl(functionOverloads22.ts, 2, 13))
>a : Symbol(a, Decl(functionOverloads22.ts, 2, 23))
>b : Symbol(b, Decl(functionOverloads22.ts, 2, 29))
>a : Symbol(a, Decl(functionOverloads22.ts, 2, 51))

22 changes: 22 additions & 0 deletions tests/baselines/reference/functionOverloads22.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
=== tests/cases/compiler/functionOverloads22.ts ===
function foo(bar:number):{a:number;}[];
>foo : { (bar: number): { a: number; }[]; (bar: string): { a: number; b: string; }[]; }
>bar : number
>a : number

function foo(bar:string):{a:number; b:string;}[];
>foo : { (bar: number): { a: number; }[]; (bar: string): { a: number; b: string; }[]; }
>bar : string
>a : number
>b : string

function foo(bar:any):{a:any;b?:any;}[] { return [{a:""}] }
>foo : { (bar: number): { a: number; }[]; (bar: string): { a: number; b: string; }[]; }
>bar : any
>a : any
>b : any
>[{a:""}] : { a: string; }[]
>{a:""} : { a: string; }
>a : string
>"" : string

24 changes: 24 additions & 0 deletions tests/baselines/reference/functionOverloads43.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
//// [functionOverloads43.ts]
function foo(bar: { a:number }[]): number;
function foo(bar: { a:string }[]): string;
function foo([x]: { a:number | string }[]): string | number {
if (x) {
return x.a;
}

return undefined;
}

var x = foo([{a: "str"}]);
var y = foo([{a: 100}]);

//// [functionOverloads43.js]
function foo(_a) {
var x = _a[0];
if (x) {
return x.a;
}
return undefined;
}
var x = foo([{ a: "str" }]);
var y = foo([{ a: 100 }]);
39 changes: 39 additions & 0 deletions tests/baselines/reference/functionOverloads43.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
=== tests/cases/compiler/functionOverloads43.ts ===
function foo(bar: { a:number }[]): number;
>foo : Symbol(foo, Decl(functionOverloads43.ts, 0, 0), Decl(functionOverloads43.ts, 0, 42), Decl(functionOverloads43.ts, 1, 42))
>bar : Symbol(bar, Decl(functionOverloads43.ts, 0, 13))
>a : Symbol(a, Decl(functionOverloads43.ts, 0, 19))

function foo(bar: { a:string }[]): string;
>foo : Symbol(foo, Decl(functionOverloads43.ts, 0, 0), Decl(functionOverloads43.ts, 0, 42), Decl(functionOverloads43.ts, 1, 42))
>bar : Symbol(bar, Decl(functionOverloads43.ts, 1, 13))
>a : Symbol(a, Decl(functionOverloads43.ts, 1, 19))

function foo([x]: { a:number | string }[]): string | number {
>foo : Symbol(foo, Decl(functionOverloads43.ts, 0, 0), Decl(functionOverloads43.ts, 0, 42), Decl(functionOverloads43.ts, 1, 42))
>x : Symbol(x, Decl(functionOverloads43.ts, 2, 14))
>a : Symbol(a, Decl(functionOverloads43.ts, 2, 19))

if (x) {
>x : Symbol(x, Decl(functionOverloads43.ts, 2, 14))

return x.a;
>x.a : Symbol(a, Decl(functionOverloads43.ts, 2, 19))
>x : Symbol(x, Decl(functionOverloads43.ts, 2, 14))
>a : Symbol(a, Decl(functionOverloads43.ts, 2, 19))
}

return undefined;
>undefined : Symbol(undefined)
}

var x = foo([{a: "str"}]);
>x : Symbol(x, Decl(functionOverloads43.ts, 10, 3))
>foo : Symbol(foo, Decl(functionOverloads43.ts, 0, 0), Decl(functionOverloads43.ts, 0, 42), Decl(functionOverloads43.ts, 1, 42))
>a : Symbol(a, Decl(functionOverloads43.ts, 10, 14))

var y = foo([{a: 100}]);
>y : Symbol(y, Decl(functionOverloads43.ts, 11, 3))
>foo : Symbol(foo, Decl(functionOverloads43.ts, 0, 0), Decl(functionOverloads43.ts, 0, 42), Decl(functionOverloads43.ts, 1, 42))
>a : Symbol(a, Decl(functionOverloads43.ts, 11, 14))

Loading