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 all 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
126 changes: 111 additions & 15 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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;
}

Expand All @@ -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
Copy link
Member

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."

Copy link
Member

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

if (targetReturnType.flags & TypeFlags.PredicateType && (targetReturnType as PredicateType).predicate.kind === TypePredicateKind.Identifier) {
Copy link
Member

Choose a reason for hiding this comment

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

can you break the line here?

Copy link
Member

Choose a reason for hiding this comment

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

What's the other allowed kind for predicate.kind, ThisType ?

Copy link
Member Author

Choose a reason for hiding this comment

The 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)) {
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.


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;
}
}

/**
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is concerning. There is a lot of duplicated functionality here.

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 have a separate branch for that work right now, but I'd like to get it in as part of a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like it's mostly down to isRelatedTo vs isSignatureAssignableTo and Ternary vs boolean. Am I missing something?

if (source === target) {
return Ternary.True;
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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;
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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;
}
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;
}
Loading