-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Changes from 19 commits
2cc6c4c
5ef8a30
2c2b8be
820a2cb
5d94e48
dc2af2f
e1004fa
51d2409
fc170da
e9dc011
8cceedd
3cdfc36
069fada
9b507b7
e012645
a236461
73526cf
5cbf17d
2efa697
d8db60a
731925b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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)) { | ||
|
||
// 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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; | ||
} | ||
|
This file was deleted.
This file was deleted.
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; | ||
} |
This file was deleted.
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)) | ||
|
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 | ||
|
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 }]); |
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)) | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.