Skip to content

Refactor signature relatability #6142

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

Merged
merged 16 commits into from
Jan 16, 2016
Merged

Refactor signature relatability #6142

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
e71b46b
Refactored most of 'isSignatureAssignableTo' to a more general functi…
DanielRosenwasser Dec 16, 2015
5e69332
Have 'signatureRelatedTo' just use 'compareSignaturesRelated'.
DanielRosenwasser Dec 16, 2015
520884c
Accepted regressive baselines.
DanielRosenwasser Dec 18, 2015
bae09d5
Merge branch 'master' into refactorSignatureRelatability
DanielRosenwasser Dec 24, 2015
7fd2a2b
Merge branch 'master' into refactorSignatureRelatability
DanielRosenwasser Jan 8, 2016
1905eac
Don't rely on truthiness.
DanielRosenwasser Jan 11, 2016
5f8f518
Merge branch 'master' into refactorSignatureRelatability
DanielRosenwasser Jan 12, 2016
c87bb37
Propagate forced re-elaboration through relation functions.
DanielRosenwasser Jan 14, 2016
51787e3
Don't report structural errors on primitive apparent types.
DanielRosenwasser Jan 14, 2016
4502213
Accepted baselines.
DanielRosenwasser Jan 14, 2016
6e0fde3
Added missing semicolon.
DanielRosenwasser Jan 14, 2016
17ff54d
Just check if the original type is a primitive instead of checking th…
DanielRosenwasser Jan 14, 2016
e8d4cf8
Documented the 'ReportErrors' enum.
DanielRosenwasser Jan 14, 2016
273cfc1
Back to booleans.
DanielRosenwasser Jan 14, 2016
e9330be
Merge branch 'master' into refactorSignatureRelatability
DanielRosenwasser Jan 14, 2016
74293a4
Merge branch 'master' into refactorSignatureRelatability
DanielRosenwasser Jan 14, 2016
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
120 changes: 40 additions & 80 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4963,6 +4963,10 @@ namespace ts {
return checkTypeRelatedTo(source, target, identityRelation, /*errorNode*/ undefined) ? Ternary.True : Ternary.False;
}

function compareTypesAssignable(source: Type, target: Type): Ternary {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this name... what is it trying to say that is different from isTypeAssignableTo?

return checkTypeRelatedTo(source, target, assignableRelation, /*errorNode*/ undefined) ? Ternary.True : Ternary.False;
}

function isTypeSubtypeOf(source: Type, target: Type): boolean {
return checkTypeSubtypeOf(source, target, /*errorNode*/ undefined);
}
Expand All @@ -4979,53 +4983,76 @@ namespace ts {
return checkTypeRelatedTo(source, target, assignableRelation, errorNode, headMessage, containingMessageChain);
}

function isSignatureAssignableTo(source: Signature,
target: Signature,
ignoreReturnTypes: boolean): boolean {
return compareSignaturesRelated(source, target, ignoreReturnTypes, /*errorReporter*/ undefined, compareTypesAssignable) !== Ternary.False;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could just pass isTypeAssignableTo here, instead of having compareTypesAssignable? Is your concern about the boolean versus Ternary return type? I think the callback can just return boolean | Ternary.

}

/**
* See signatureRelatedTo, compareSignaturesIdentical
Copy link
Member

Choose a reason for hiding this comment

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

This comment needs to be updated now that everybody calls this function, right?

*/
function isSignatureAssignableTo(source: Signature, target: Signature, ignoreReturnTypes: boolean): boolean {
function compareSignaturesRelated(source: Signature,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think compareSignatures is fine, or checkSignatureRelatedTo. I'm having trouble figuring out the pattern with this naming scheme.

Copy link
Member

Choose a reason for hiding this comment

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

I've got compareX: Ternary and isX: boolean. Not sure about the suffixes yet though.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that there's a compareSignaturesIdentical (previously called compareSignatures!) and so I'm trying to differentiate the two here.

Copy link
Contributor

Choose a reason for hiding this comment

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

But all the isX functions nested in checkTypeRelatedTo return Ternary. I would just call them isSignatureIdenticalTo, isSignatureRelatedTo, and isSignatureAssignableTo. I don't think the prefix compare is illuminating in any way. Then if there are any further distinctions to be made, they can be encoded more explicitly into the name, and consistently across all of these functions.

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 suppose; I'm mostly trying to expose an easier interface with the is* functions by making them return boolean instead of Ternary.

target: Signature,
ignoreReturnTypes: boolean,
errorReporter: (d: DiagnosticMessage, arg0?: string, arg1?: string) => void,
compareTypes: (s: Type, t: Type, reportErrors?: boolean) => Ternary): Ternary {
// TODO (drosen): De-duplicate code between related functions.
if (source === target) {
return true;
return Ternary.True;
}
if (!target.hasRestParameter && source.minArgumentCount > target.parameters.length) {
return false;
return Ternary.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);

let result = Ternary.True;

const sourceMax = getNumNonRestParameters(source);
const targetMax = getNumNonRestParameters(target);
const checkCount = getNumParametersToCheckForSignatureRelatability(source, sourceMax, target, targetMax);
const sourceParams = source.parameters;
const targetParams = target.parameters;
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);
const s = i < sourceMax ? getTypeOfSymbol(sourceParams[i]) : getRestTypeOfSignature(source);
const t = i < targetMax ? getTypeOfSymbol(targetParams[i]) : getRestTypeOfSignature(target);
const related = compareTypes(t, s, /*reportErrors*/ false) || compareTypes(s, t, !!errorReporter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, you incorporated the reversal here too.

if (!related) {
return false;
if (errorReporter) {
errorReporter(Diagnostics.Types_of_parameters_0_and_1_are_incompatible,
sourceParams[i < sourceMax ? i : sourceMax].name,
targetParams[i < targetMax ? i : targetMax].name);
}
return Ternary.False;
}
result &= related;
}

if (!ignoreReturnTypes) {
const targetReturnType = getReturnTypeOfSignature(target);
if (targetReturnType === voidType) {
return true;
return result;
}
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) {
if (!(sourceReturnType.flags & TypeFlags.PredicateType)) {
return false;
if (errorReporter) {
errorReporter(Diagnostics.Signature_0_must_have_a_type_predicate, signatureToString(source));
}
return Ternary.False;
}
}

return isTypeAssignableTo(sourceReturnType, targetReturnType);
result &= compareTypes(sourceReturnType, targetReturnType, !!errorReporter);
}

return true;
return result;
}

function isImplementationCompatibleWithOverload(implementation: Signature, overload: Signature): boolean {
Expand Down Expand Up @@ -5708,77 +5735,10 @@ namespace ts {
}

/**
* See signatureAssignableTo, signatureAssignableTo
* See signatureAssignableTo, compareSignaturesIdentical
*/
function signatureRelatedTo(source: Signature, target: Signature, reportErrors: boolean): Ternary {
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 move all three pieces of code together?
Also, wasn't there a third nearly identical piece of code?

Copy link
Contributor

Choose a reason for hiding this comment

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

signatureIsRelatedTo will have to stay inside checkTypeRelatedTo though, because it references isRelatedTo.

// TODO (drosen): De-duplicate code between related functions.
if (source === target) {
return Ternary.True;
}
if (!target.hasRestParameter && source.minArgumentCount > target.parameters.length) {
return Ternary.False;
}
let sourceMax = source.parameters.length;
let targetMax = target.parameters.length;
let checkCount: number;
if (source.hasRestParameter && target.hasRestParameter) {
checkCount = sourceMax > targetMax ? sourceMax : targetMax;
sourceMax--;
targetMax--;
}
else if (source.hasRestParameter) {
sourceMax--;
checkCount = targetMax;
}
else if (target.hasRestParameter) {
targetMax--;
checkCount = sourceMax;
}
else {
checkCount = sourceMax < targetMax ? sourceMax : targetMax;
}
// 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);
let result = Ternary.True;
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 saveErrorInfo = errorInfo;
let related = isRelatedTo(s, t, reportErrors);
if (!related) {
related = isRelatedTo(t, s, /*reportErrors*/ false);
if (!related) {
if (reportErrors) {
reportError(Diagnostics.Types_of_parameters_0_and_1_are_incompatible,
source.parameters[i < sourceMax ? i : sourceMax].name,
target.parameters[i < targetMax ? i : targetMax].name);
}
return Ternary.False;
}
errorInfo = saveErrorInfo;
}
result &= related;
}

const targetReturnType = getReturnTypeOfSignature(target);
if (targetReturnType === voidType) {
return result;
}
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) {
if (!(sourceReturnType.flags & TypeFlags.PredicateType)) {
if (reportErrors) {
reportError(Diagnostics.Signature_0_must_have_a_type_predicate, signatureToString(source));
}
return Ternary.False;
}
}

return result & isRelatedTo(sourceReturnType, targetReturnType, reportErrors);
return compareSignaturesRelated(source, target, /*ignoreReturnTypes*/ false, reportErrors && reportError, isRelatedTo);
}

function signaturesIdenticalTo(source: Type, target: Type, kind: SignatureKind): Ternary {
Expand Down
4 changes: 0 additions & 4 deletions tests/baselines/reference/assignmentCompatWithCallSignatures4.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignme
Type '(arg2: { foo: number; }) => any' is not assignable to type '(arg2: Base) => Derived'.
Types of parameters 'arg2' and 'arg2' are incompatible.
Type '{ foo: number; }' is not assignable to type 'Base'.
Types of property 'foo' are incompatible.
Type 'number' is not assignable to type 'string'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithCallSignatures4.ts(53,9): error TS2322: Type '(x: (arg: Base) => Derived, y: (arg2: Base) => Derived) => (r: Base) => Derived' is not assignable to type '<T extends Base, U extends Derived>(x: (arg: T) => U, y: (arg2: { foo: number; }) => U) => (r: T) => U'.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it a regression to drop these lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. I've noted that elsewhere in the PR. Mostly I'm trying to see whether this is an appropriate direction to move in. I need to figure out how to maintain this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these losses primarily because of the covariant / contravariant reversal, or something else in the change? It might be good to swap them back, just to see if it makes the errors go away.

Types of parameters 'y' and 'y' are incompatible.
Type '(arg2: Base) => Derived' is not assignable to type '(arg2: { foo: number; }) => any'.
Expand Down Expand Up @@ -69,8 +67,6 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignme
!!! error TS2322: Type '(arg2: { foo: number; }) => any' is not assignable to type '(arg2: Base) => Derived'.
!!! error TS2322: Types of parameters 'arg2' and 'arg2' are incompatible.
!!! error TS2322: Type '{ foo: number; }' is not assignable to type 'Base'.
!!! error TS2322: Types of property 'foo' are incompatible.
!!! error TS2322: Type 'number' is not assignable to type 'string'.
b8 = a8; // error, { foo: number } and Base are incompatible
~~
!!! error TS2322: Type '(x: (arg: Base) => Derived, y: (arg2: Base) => Derived) => (r: Base) => Derived' is not assignable to type '<T extends Base, U extends Derived>(x: (arg: T) => U, y: (arg2: { foo: number; }) => U) => (r: T) => U'.
Expand Down
4 changes: 0 additions & 4 deletions tests/baselines/reference/assignmentCompatWithConstructSignatures4.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignme
Type '(arg2: { foo: number; }) => any' is not assignable to type '(arg2: Base) => Derived'.
Types of parameters 'arg2' and 'arg2' are incompatible.
Type '{ foo: number; }' is not assignable to type 'Base'.
Types of property 'foo' are incompatible.
Type 'number' is not assignable to type 'string'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithConstructSignatures4.ts(53,9): error TS2322: Type 'new (x: (arg: Base) => Derived, y: (arg2: Base) => Derived) => (r: Base) => Derived' is not assignable to type 'new <T extends Base, U extends Derived>(x: (arg: T) => U, y: (arg2: { foo: number; }) => U) => (r: T) => U'.
Types of parameters 'y' and 'y' are incompatible.
Type '(arg2: Base) => Derived' is not assignable to type '(arg2: { foo: number; }) => any'.
Expand Down Expand Up @@ -83,8 +81,6 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignme
!!! error TS2322: Type '(arg2: { foo: number; }) => any' is not assignable to type '(arg2: Base) => Derived'.
!!! error TS2322: Types of parameters 'arg2' and 'arg2' are incompatible.
!!! error TS2322: Type '{ foo: number; }' is not assignable to type 'Base'.
!!! error TS2322: Types of property 'foo' are incompatible.
!!! error TS2322: Type 'number' is not assignable to type 'string'.
b8 = a8; // error
~~
!!! error TS2322: Type 'new (x: (arg: Base) => Derived, y: (arg2: Base) => Derived) => (r: Base) => Derived' is not assignable to type 'new <T extends Base, U extends Derived>(x: (arg: T) => U, y: (arg2: { foo: number; }) => U) => (r: T) => U'.
Expand Down
4 changes: 0 additions & 4 deletions tests/baselines/reference/callSignatureAssignabilityInInheritance3.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/callSign
Type '(arg2: { foo: number; }) => any' is not assignable to type '(arg2: Base) => Derived'.
Types of parameters 'arg2' and 'arg2' are incompatible.
Type '{ foo: number; }' is not assignable to type 'Base'.
Types of property 'foo' are incompatible.
Type 'number' is not assignable to type 'string'.


==== tests/cases/conformance/types/typeRelationships/assignmentCompatibility/callSignatureAssignabilityInInheritance3.ts (2 errors) ====
Expand Down Expand Up @@ -89,8 +87,6 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/callSign
!!! error TS2430: Type '(arg2: { foo: number; }) => any' is not assignable to type '(arg2: Base) => Derived'.
!!! error TS2430: Types of parameters 'arg2' and 'arg2' are incompatible.
!!! error TS2430: Type '{ foo: number; }' is not assignable to type 'Base'.
!!! error TS2430: Types of property 'foo' are incompatible.
!!! error TS2430: Type 'number' is not assignable to type 'string'.
a8: <T extends Base, U extends Derived>(x: (arg: T) => U, y: (arg2: { foo: number; }) => U) => (r: T) => U; // error, type mismatch
}

Expand Down
4 changes: 0 additions & 4 deletions tests/baselines/reference/constructSignatureAssignabilityInInheritance3.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/construc
Type '(arg2: { foo: number; }) => any' is not assignable to type '(arg2: Base) => Derived'.
Types of parameters 'arg2' and 'arg2' are incompatible.
Type '{ foo: number; }' is not assignable to type 'Base'.
Types of property 'foo' are incompatible.
Type 'number' is not assignable to type 'string'.


==== tests/cases/conformance/types/typeRelationships/assignmentCompatibility/constructSignatureAssignabilityInInheritance3.ts (2 errors) ====
Expand Down Expand Up @@ -79,8 +77,6 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/construc
!!! error TS2430: Type '(arg2: { foo: number; }) => any' is not assignable to type '(arg2: Base) => Derived'.
!!! error TS2430: Types of parameters 'arg2' and 'arg2' are incompatible.
!!! error TS2430: Type '{ foo: number; }' is not assignable to type 'Base'.
!!! error TS2430: Types of property 'foo' are incompatible.
!!! error TS2430: Type 'number' is not assignable to type 'string'.
a8: new <T extends Base, U extends Derived>(x: (arg: T) => U, y: (arg2: { foo: number; }) => U) => (r: T) => U; // error, type mismatch
}

Expand Down