-
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 all 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 |
---|---|---|
|
@@ -3501,7 +3501,7 @@ namespace ts { | |
|
||
function findMatchingSignature(signatureList: Signature[], signature: Signature, partialMatch: boolean, ignoreReturnTypes: boolean): Signature { | ||
for (const s of signatureList) { | ||
if (compareSignatures(s, signature, partialMatch, ignoreReturnTypes, compareTypes)) { | ||
if (compareSignaturesIdentical(s, signature, partialMatch, ignoreReturnTypes, compareTypesIdentical)) { | ||
return s; | ||
} | ||
} | ||
|
@@ -4955,7 +4955,7 @@ namespace ts { | |
return checkTypeRelatedTo(source, target, identityRelation, /*errorNode*/ undefined); | ||
} | ||
|
||
function compareTypes(source: Type, target: Type): Ternary { | ||
function compareTypesIdentical(source: Type, target: Type): Ternary { | ||
return checkTypeRelatedTo(source, target, identityRelation, /*errorNode*/ undefined) ? Ternary.True : Ternary.False; | ||
} | ||
|
||
|
@@ -4975,10 +4975,96 @@ namespace ts { | |
return checkTypeRelatedTo(source, target, assignableRelation, errorNode, headMessage, containingMessageChain); | ||
} | ||
|
||
function isSignatureAssignableTo(source: Signature, target: Signature): boolean { | ||
const sourceType = getOrCreateTypeFromSignature(source); | ||
const targetType = getOrCreateTypeFromSignature(target); | ||
return checkTypeRelatedTo(sourceType, targetType, assignableRelation, /*errorNode*/ undefined); | ||
/** | ||
* See signatureRelatedTo, compareSignaturesIdentical | ||
*/ | ||
function isSignatureAssignableTo(source: Signature, target: Signature, ignoreReturnTypes: boolean): boolean { | ||
// TODO (drosen): De-duplicate code between related functions. | ||
if (source === target) { | ||
return true; | ||
} | ||
if (!target.hasRestParameter && source.minArgumentCount > target.parameters.length) { | ||
return false; | ||
} | ||
|
||
// Spec 1.0 Section 3.8.3 & 3.8.4: | ||
// M and N (the signatures) are instantiated using type Any as the type argument for all type parameters declared by M and N | ||
source = getErasedSignature(source); | ||
target = getErasedSignature(target); | ||
|
||
const sourceMax = getNumNonRestParameters(source); | ||
const targetMax = getNumNonRestParameters(target); | ||
const checkCount = getNumParametersToCheckForSignatureRelatability(source, sourceMax, target, targetMax); | ||
for (let i = 0; i < checkCount; i++) { | ||
const s = i < sourceMax ? getTypeOfSymbol(source.parameters[i]) : getRestTypeOfSignature(source); | ||
const t = i < targetMax ? getTypeOfSymbol(target.parameters[i]) : getRestTypeOfSignature(target); | ||
const related = isTypeAssignableTo(t, s) || isTypeAssignableTo(s, t); | ||
if (!related) { | ||
return false; | ||
} | ||
} | ||
|
||
if (!ignoreReturnTypes) { | ||
const targetReturnType = getReturnTypeOfSignature(target); | ||
if (targetReturnType === voidType) { | ||
return true; | ||
} | ||
const sourceReturnType = getReturnTypeOfSignature(source); | ||
|
||
// The following block preserves behavior forbidding boolean returning functions from being assignable to type guard returning functions | ||
if (targetReturnType.flags & TypeFlags.PredicateType && (targetReturnType as PredicateType).predicate.kind === TypePredicateKind.Identifier) { | ||
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. can you break the line here? 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. What's the other allowed kind for 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. Yeah; actually, we might consider disallowing those too when not dealing with a get-accessor? It's an orthogonal change for now. |
||
if (!(sourceReturnType.flags & TypeFlags.PredicateType)) { | ||
return false; | ||
} | ||
} | ||
|
||
return isTypeAssignableTo(sourceReturnType, targetReturnType); | ||
} | ||
|
||
return true; | ||
} | ||
|
||
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)) { | ||
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. 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 commentThe reason will be displayed to describe this comment to others. Learn more. Exactly, and you can see that in 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. My ultimate point is that concern number 2 in #943 (comment) is not an issue, since |
||
|
||
return isSignatureAssignableTo(erasedSource, erasedTarget, /*ignoreReturnTypes*/ true); | ||
} | ||
|
||
return false; | ||
} | ||
|
||
function getNumNonRestParameters(signature: Signature) { | ||
const numParams = signature.parameters.length; | ||
return signature.hasRestParameter ? | ||
numParams - 1 : | ||
numParams; | ||
} | ||
|
||
function getNumParametersToCheckForSignatureRelatability(source: Signature, sourceNonRestParamCount: number, target: Signature, targetNonRestParamCount: number) { | ||
if (source.hasRestParameter === target.hasRestParameter) { | ||
if (source.hasRestParameter) { | ||
// If both have rest parameters, get the max and add 1 to | ||
// compensate for the rest parameter. | ||
return Math.max(sourceNonRestParamCount, targetNonRestParamCount) + 1; | ||
} | ||
else { | ||
return Math.min(sourceNonRestParamCount, targetNonRestParamCount); | ||
} | ||
} | ||
else { | ||
// Return the count for whichever signature doesn't have rest parameters. | ||
return source.hasRestParameter ? | ||
targetNonRestParamCount : | ||
sourceNonRestParamCount; | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -5565,7 +5651,7 @@ namespace ts { | |
shouldElaborateErrors = false; | ||
} | ||
} | ||
// don't elaborate the primitive apparent types (like Number) | ||
// don't elaborate the primitive apparent types (like Number) | ||
// because the actual primitives will have already been reported. | ||
if (shouldElaborateErrors && !isPrimitiveApparentType(source)) { | ||
reportError(Diagnostics.Type_0_provides_no_match_for_the_signature_1, | ||
|
@@ -5612,7 +5698,11 @@ namespace ts { | |
} | ||
} | ||
|
||
/** | ||
* See signatureAssignableTo, signatureAssignableTo | ||
*/ | ||
function signatureRelatedTo(source: Signature, target: Signature, reportErrors: boolean): Ternary { | ||
// TODO (drosen): De-duplicate code between related functions. | ||
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. Yes, this is concerning. There is a lot of duplicated functionality here. 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 have a separate branch for that work right now, but I'd like to get it in as part of a separate PR. 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. It looks like it's mostly down to |
||
if (source === target) { | ||
return Ternary.True; | ||
} | ||
|
@@ -5664,10 +5754,12 @@ namespace ts { | |
} | ||
|
||
const targetReturnType = getReturnTypeOfSignature(target); | ||
if (targetReturnType === voidType) return result; | ||
if (targetReturnType === voidType) { | ||
return result; | ||
} | ||
const sourceReturnType = getReturnTypeOfSignature(source); | ||
|
||
// The follow block preserves old behavior forbidding boolean returning functions from being assignable to type guard returning functions | ||
// The following block preserves behavior forbidding boolean returning functions from being assignable to type guard returning functions | ||
if (targetReturnType.flags & TypeFlags.PredicateType && (targetReturnType as PredicateType).predicate.kind === TypePredicateKind.Identifier) { | ||
if (!(sourceReturnType.flags & TypeFlags.PredicateType)) { | ||
if (reportErrors) { | ||
|
@@ -5688,7 +5780,7 @@ namespace ts { | |
} | ||
let result = Ternary.True; | ||
for (let i = 0, len = sourceSignatures.length; i < len; ++i) { | ||
const related = compareSignatures(sourceSignatures[i], targetSignatures[i], /*partialMatch*/ false, /*ignoreReturnTypes*/ false, isRelatedTo); | ||
const related = compareSignaturesIdentical(sourceSignatures[i], targetSignatures[i], /*partialMatch*/ false, /*ignoreReturnTypes*/ false, isRelatedTo); | ||
if (!related) { | ||
return Ternary.False; | ||
} | ||
|
@@ -5800,7 +5892,7 @@ namespace ts { | |
} | ||
|
||
function isPropertyIdenticalTo(sourceProp: Symbol, targetProp: Symbol): boolean { | ||
return compareProperties(sourceProp, targetProp, compareTypes) !== Ternary.False; | ||
return compareProperties(sourceProp, targetProp, compareTypesIdentical) !== Ternary.False; | ||
} | ||
|
||
function compareProperties(sourceProp: Symbol, targetProp: Symbol, compareTypes: (source: Type, target: Type) => Ternary): Ternary { | ||
|
@@ -5847,7 +5939,11 @@ namespace ts { | |
return false; | ||
} | ||
|
||
function compareSignatures(source: Signature, target: Signature, partialMatch: boolean, ignoreReturnTypes: boolean, compareTypes: (s: Type, t: Type) => Ternary): Ternary { | ||
/** | ||
* See signatureRelatedTo, compareSignaturesIdentical | ||
*/ | ||
function compareSignaturesIdentical(source: Signature, target: Signature, partialMatch: boolean, ignoreReturnTypes: boolean, compareTypes: (s: Type, t: Type) => Ternary): Ternary { | ||
// TODO (drosen): De-duplicate code between related functions. | ||
if (source === target) { | ||
return Ternary.True; | ||
} | ||
|
@@ -7538,7 +7634,7 @@ namespace ts { | |
// This signature will contribute to contextual union signature | ||
signatureList = [signature]; | ||
} | ||
else if (!compareSignatures(signatureList[0], signature, /*partialMatch*/ false, /*ignoreReturnTypes*/ true, compareTypes)) { | ||
else if (!compareSignaturesIdentical(signatureList[0], signature, /*partialMatch*/ false, /*ignoreReturnTypes*/ true, compareTypesIdentical)) { | ||
// Signatures aren't identical, do not use | ||
return undefined; | ||
} | ||
|
@@ -11531,7 +11627,7 @@ namespace ts { | |
} | ||
|
||
for (const otherSignature of signaturesToCheck) { | ||
if (!otherSignature.hasStringLiterals && isSignatureAssignableTo(signature, otherSignature)) { | ||
if (!otherSignature.hasStringLiterals && isSignatureAssignableTo(signature, otherSignature, /*ignoreReturnTypes*/ false)) { | ||
return; | ||
} | ||
} | ||
|
@@ -11778,7 +11874,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; | ||
} |
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.
I think you can use fewer words here: Just say "Preserve behaviour forbidding boolean functions from being assignable to type guards."
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.
Might as well change the original in
signatureRelatedTo
also