Skip to content

Increase selectivity of subtype relationship for signatures #35659

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 12 commits into from
Dec 20, 2019
Merged

Increase selectivity of subtype relationship for signatures #35659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b457de4
Increase selectivity of subtype relationship for signatures
ahejlsberg Dec 12, 2019
5524a61
Add regression test
ahejlsberg Dec 13, 2019
6d67054
Accept new baselines
ahejlsberg Dec 13, 2019
47f26e6
Use strictSubtypeRelation for union subtype reduction
ahejlsberg Dec 14, 2019
42c0451
(x: number | undefined) -> void is subtype of (x?: number | undefined…
ahejlsberg Dec 14, 2019
e60d7ff
Accept new baselines
ahejlsberg Dec 14, 2019
7df76b6
Merge branch 'master' into fix35414-2
ahejlsberg Dec 16, 2019
baac083
Add tests
ahejlsberg Dec 17, 2019
e0e45ec
Accept new baselines
ahejlsberg Dec 17, 2019
8228888
Address CR feedback
ahejlsberg Dec 20, 2019
273bb3e
Fix parameter list length check
ahejlsberg Dec 20, 2019
4154dcb
Accept API baseline changes
ahejlsberg Dec 20, 2019
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
58 changes: 33 additions & 25 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,12 @@ namespace ts {
NoTupleBoundsCheck = 1 << 3,
}

const enum CallbackCheck {
None,
Bivariant,
Strict,
const enum SignatureCheckMode {
BivariantCallback = 1 << 0,
StrictCallback = 1 << 1,
IgnoreReturnTypes = 1 << 2,
StrictArity = 1 << 3,
Callback = BivariantCallback | StrictCallback,
}

const enum MappedTypeModifiers {
Expand Down Expand Up @@ -343,6 +345,7 @@ namespace ts {
assignable: assignableRelation.size,
identity: identityRelation.size,
subtype: subtypeRelation.size,
strictSubtype: strictSubtypeRelation.size,
}),
isUndefinedSymbol: symbol => symbol === undefinedSymbol,
isArgumentsSymbol: symbol => symbol === argumentsSymbol,
Expand Down Expand Up @@ -869,6 +872,7 @@ namespace ts {
let outofbandVarianceMarkerHandler: ((onlyUnreliable: boolean) => void) | undefined;

const subtypeRelation = createMap<RelationComparisonResult>();
const strictSubtypeRelation = createMap<RelationComparisonResult>();
const assignableRelation = createMap<RelationComparisonResult>();
const comparableRelation = createMap<RelationComparisonResult>();
const identityRelation = createMap<RelationComparisonResult>();
Expand Down Expand Up @@ -11400,7 +11404,7 @@ namespace ts {
}
}
count++;
if (isTypeSubtypeOf(source, target) && (
if (isTypeRelatedTo(source, target, strictSubtypeRelation) && (
!(getObjectFlags(getTargetType(source)) & ObjectFlags.Class) ||
!(getObjectFlags(getTargetType(target)) & ObjectFlags.Class) ||
isTypeDerivedFrom(source, target))) {
Expand Down Expand Up @@ -13988,7 +13992,7 @@ namespace ts {
function isSignatureAssignableTo(source: Signature,
target: Signature,
ignoreReturnTypes: boolean): boolean {
return compareSignaturesRelated(source, target, CallbackCheck.None, ignoreReturnTypes, /*reportErrors*/ false,
return compareSignaturesRelated(source, target, ignoreReturnTypes ? SignatureCheckMode.IgnoreReturnTypes : 0, /*reportErrors*/ false,
/*errorReporter*/ undefined, /*errorReporter*/ undefined, compareTypesAssignable, /*reportUnreliableMarkers*/ undefined) !== Ternary.False;
}

Expand All @@ -14008,8 +14012,7 @@ namespace ts {
*/
function compareSignaturesRelated(source: Signature,
target: Signature,
callbackCheck: CallbackCheck,
ignoreReturnTypes: boolean,
checkMode: SignatureCheckMode,
reportErrors: boolean,
errorReporter: ErrorReporter | undefined,
incompatibleErrorReporter: ((source: Type, target: Type) => void) | undefined,
Expand All @@ -14025,7 +14028,9 @@ namespace ts {
}

const targetCount = getParameterCount(target);
if (!hasEffectiveRestParameter(target) && getMinArgumentCount(source) > targetCount) {
const sourceHasMoreParameters = !hasEffectiveRestParameter(target) &&
(checkMode & SignatureCheckMode.StrictArity ? hasEffectiveRestParameter(source) || getParameterCount(source) > targetCount : getMinArgumentCount(source) > targetCount);
if (sourceHasMoreParameters) {
return Ternary.False;
}

Expand All @@ -14046,7 +14051,7 @@ namespace ts {
}

const kind = target.declaration ? target.declaration.kind : SyntaxKind.Unknown;
const strictVariance = !callbackCheck && strictFunctionTypes && kind !== SyntaxKind.MethodDeclaration &&
const strictVariance = !(checkMode & SignatureCheckMode.Callback) && strictFunctionTypes && kind !== SyntaxKind.MethodDeclaration &&
kind !== SyntaxKind.MethodSignature && kind !== SyntaxKind.Constructor;
let result = Ternary.True;

Expand Down Expand Up @@ -14081,14 +14086,17 @@ namespace ts {
// similar to return values, callback parameters are output positions. This means that a Promise<T>,
// where T is used only in callback parameter positions, will be co-variant (as opposed to bi-variant)
// with respect to T.
const sourceSig = callbackCheck ? undefined : getSingleCallSignature(getNonNullableType(sourceType));
const targetSig = callbackCheck ? undefined : getSingleCallSignature(getNonNullableType(targetType));
const sourceSig = checkMode & SignatureCheckMode.Callback ? undefined : getSingleCallSignature(getNonNullableType(sourceType));
const targetSig = checkMode & SignatureCheckMode.Callback ? undefined : getSingleCallSignature(getNonNullableType(targetType));
const callbacks = sourceSig && targetSig && !getTypePredicateOfSignature(sourceSig) && !getTypePredicateOfSignature(targetSig) &&
(getFalsyFlags(sourceType) & TypeFlags.Nullable) === (getFalsyFlags(targetType) & TypeFlags.Nullable);
const related = callbacks ?
// TODO: GH#18217 It will work if they're both `undefined`, but not if only one is
compareSignaturesRelated(targetSig!, sourceSig!, strictVariance ? CallbackCheck.Strict : CallbackCheck.Bivariant, /*ignoreReturnTypes*/ false, reportErrors, errorReporter, incompatibleErrorReporter, compareTypes, reportUnreliableMarkers) :
!callbackCheck && !strictVariance && compareTypes(sourceType, targetType, /*reportErrors*/ false) || compareTypes(targetType, sourceType, reportErrors);
let related = callbacks ?
compareSignaturesRelated(targetSig!, sourceSig!, (checkMode & SignatureCheckMode.StrictArity) | (strictVariance ? SignatureCheckMode.StrictCallback : SignatureCheckMode.BivariantCallback), reportErrors, errorReporter, incompatibleErrorReporter, compareTypes, reportUnreliableMarkers) :
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 isTypeIdenticalTo check here for?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without it a signature like (x?: string | undefined) => void wouldn't be a subtype of (x: string) => void. We want it to be such that we reduce a union to just (x: string) => void. It's basically a tiebreaker for positions with identical types but one with and one without the ? modifier.

Copy link
Member

@weswigham weswigham Dec 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

...Mmm, but isTypeIdenticalTo doesn't seem quite right...? Shouldn't it be compareTypes(sourceType, targetType, /*reportErrors*/ false)? The difference, I think, should really only be visible, but from what I gather is important when callbacks with optional parameters are the parameter type of callbacks with optional parameters, eg (x?: (x?: string | undefined) => void) => void and (x: (x: string | undefined) => void) => void The first should be a subtype of the second because the second's parameter is a subtype of the first's.

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, probably better to use compareTypes.

!(checkMode & SignatureCheckMode.Callback) && !strictVariance && compareTypes(sourceType, targetType, /*reportErrors*/ false) || compareTypes(targetType, sourceType, reportErrors);
// With strict arity, (x: number | undefined) => void is a subtype of (x?: number | undefined) => void
if (related && checkMode & SignatureCheckMode.StrictArity && i >= getMinArgumentCount(source) && i < getMinArgumentCount(target) && compareTypes(sourceType, targetType, /*reportErrors*/ false)) {
related = Ternary.False;
}
if (!related) {
if (reportErrors) {
errorReporter!(Diagnostics.Types_of_parameters_0_and_1_are_incompatible,
Expand All @@ -14100,7 +14108,7 @@ namespace ts {
result &= related;
}

if (!ignoreReturnTypes) {
if (!(checkMode & SignatureCheckMode.IgnoreReturnTypes)) {
// If a signature resolution is already in-flight, skip issuing a circularity error
// here and just use the `any` type directly
const targetReturnType = isResolvingReturnTypeOfSignature(target) ? anyType
Expand Down Expand Up @@ -14131,7 +14139,7 @@ namespace ts {
// When relating callback signatures, we still need to relate return types bi-variantly as otherwise
// the containing type wouldn't be co-variant. For example, interface Foo<T> { add(cb: () => T): void }
// wouldn't be co-variant for T without this rule.
result &= callbackCheck === CallbackCheck.Bivariant && compareTypes(targetReturnType, sourceReturnType, /*reportErrors*/ false) ||
result &= checkMode & SignatureCheckMode.BivariantCallback && compareTypes(targetReturnType, sourceReturnType, /*reportErrors*/ false) ||
compareTypes(sourceReturnType, targetReturnType, reportErrors);
if (!result && reportErrors && incompatibleErrorReporter) {
incompatibleErrorReporter(sourceReturnType, targetReturnType);
Expand Down Expand Up @@ -15439,7 +15447,7 @@ namespace ts {
}
else {
// An empty object type is related to any mapped type that includes a '?' modifier.
if (relation !== subtypeRelation && isPartialMappedType(target) && isEmptyObjectType(source)) {
if (relation !== subtypeRelation && relation !== strictSubtypeRelation && isPartialMappedType(target) && isEmptyObjectType(source)) {
return Ternary.True;
}
if (isGenericMappedType(target)) {
Expand Down Expand Up @@ -15481,7 +15489,7 @@ namespace ts {
}
// Consider a fresh empty object literal type "closed" under the subtype relationship - this way `{} <- {[idx: string]: any} <- fresh({})`
// and not `{} <- fresh({}) <- {[idx: string]: any}`
else if (relation === subtypeRelation && isEmptyObjectType(target) && getObjectFlags(target) & ObjectFlags.FreshLiteral && !isEmptyObjectType(source)) {
else if ((relation === subtypeRelation || relation === strictSubtypeRelation) && isEmptyObjectType(target) && getObjectFlags(target) & ObjectFlags.FreshLiteral && !isEmptyObjectType(source)) {
return Ternary.False;
}
// Even if relationship doesn't hold for unions, intersections, or generic type references,
Expand Down Expand Up @@ -15826,7 +15834,7 @@ namespace ts {
if (relation === identityRelation) {
return propertiesIdenticalTo(source, target, excludedProperties);
}
const requireOptionalProperties = relation === subtypeRelation && !isObjectLiteralType(source) && !isEmptyArrayLiteralType(source) && !isTupleType(source);
const requireOptionalProperties = (relation === subtypeRelation || relation === strictSubtypeRelation) && !isObjectLiteralType(source) && !isEmptyArrayLiteralType(source) && !isTupleType(source);
const unmatchedProperty = getUnmatchedProperty(source, target, requireOptionalProperties, /*matchDiscriminantProperties*/ false);
if (unmatchedProperty) {
if (reportErrors) {
Expand Down Expand Up @@ -16049,7 +16057,7 @@ namespace ts {
*/
function signatureRelatedTo(source: Signature, target: Signature, erase: boolean, reportErrors: boolean, incompatibleReporter: (source: Type, target: Type) => void): Ternary {
return compareSignaturesRelated(erase ? getErasedSignature(source) : source, erase ? getErasedSignature(target) : target,
CallbackCheck.None, /*ignoreReturnTypes*/ false, reportErrors, reportError, incompatibleReporter, isRelatedTo, reportUnreliableMarkers);
relation === strictSubtypeRelation ? SignatureCheckMode.StrictArity : 0, reportErrors, reportError, incompatibleReporter, isRelatedTo, reportUnreliableMarkers);
}

function signaturesIdenticalTo(source: Type, target: Type, kind: SignatureKind): Ternary {
Expand Down Expand Up @@ -16363,12 +16371,12 @@ namespace ts {
source = target;
target = temp;
}
const intersection = isIntersectionConstituent ? "&" : "";
const delimiter = isIntersectionConstituent ? ";" : ",";
if (isTypeReferenceWithGenericArguments(source) && isTypeReferenceWithGenericArguments(target)) {
const typeParameters: Type[] = [];
return getTypeReferenceId(<TypeReference>source, typeParameters) + "," + getTypeReferenceId(<TypeReference>target, typeParameters) + intersection;
return getTypeReferenceId(<TypeReference>source, typeParameters) + delimiter + getTypeReferenceId(<TypeReference>target, typeParameters);
}
return source.id + "," + target.id + intersection;
return source.id + delimiter + target.id;
}

// Invoke the callback for each underlying property symbol of the given symbol and return the first
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3161,7 +3161,7 @@ namespace ts {
getIdentifierCount(): number;
getSymbolCount(): number;
getTypeCount(): number;
getRelationCacheSizes(): { assignable: number, identity: number, subtype: number };
getRelationCacheSizes(): { assignable: number, identity: number, subtype: number, strictSubtype: number };

/* @internal */ getFileProcessingDiagnostics(): DiagnosticCollection;
/* @internal */ getResolvedTypeReferenceDirectives(): Map<ResolvedTypeReferenceDirective | undefined>;
Expand Down Expand Up @@ -3479,7 +3479,7 @@ namespace ts {
/* @internal */ getIdentifierCount(): number;
/* @internal */ getSymbolCount(): number;
/* @internal */ getTypeCount(): number;
/* @internal */ getRelationCacheSizes(): { assignable: number, identity: number, subtype: number };
/* @internal */ getRelationCacheSizes(): { assignable: number, identity: number, subtype: number, strictSubtype: number };

/* @internal */ isArrayType(type: Type): boolean;
/* @internal */ isTupleType(type: Type): boolean;
Expand Down
1 change: 1 addition & 0 deletions src/executeCommandLine/executeCommandLine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,6 +654,7 @@ namespace ts {
reportCountStatistic("Assignability cache size", caches.assignable);
reportCountStatistic("Identity cache size", caches.identity);
reportCountStatistic("Subtype cache size", caches.subtype);
reportCountStatistic("Strict subtype cache size", caches.strictSubtype);
performance.forEachMeasure((name, duration) => reportTimeStatistic(`${name} time`, duration));
}
else {
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/api/tsserverlibrary.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1919,6 +1919,7 @@ declare namespace ts {
assignable: number;
identity: number;
subtype: number;
strictSubtype: number;
};
isSourceFileFromExternalLibrary(file: SourceFile): boolean;
isSourceFileDefaultLibrary(file: SourceFile): boolean;
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/api/typescript.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1919,6 +1919,7 @@ declare namespace ts {
assignable: number;
identity: number;
subtype: number;
strictSubtype: number;
};
isSourceFileFromExternalLibrary(file: SourceFile): boolean;
isSourceFileDefaultLibrary(file: SourceFile): boolean;
Expand Down
10 changes: 5 additions & 5 deletions tests/baselines/reference/bestCommonTypeOfConditionalExpressions.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ var r5 = true ? b : a; // typeof b
>a : { x: number; y?: number; }

var r6 = true ? (x: number) => { } : (x: Object) => { }; // returns number => void
>r6 : (x: number) => void
>true ? (x: number) => { } : (x: Object) => { } : (x: number) => void
>r6 : ((x: number) => void) | ((x: Object) => void)
>true ? (x: number) => { } : (x: Object) => { } : ((x: number) => void) | ((x: Object) => void)
>true : true
>(x: number) => { } : (x: number) => void
>x : number
Expand All @@ -75,16 +75,16 @@ var r6 = true ? (x: number) => { } : (x: Object) => { }; // returns number => vo
var r7: (x: Object) => void = true ? (x: number) => { } : (x: Object) => { };
>r7 : (x: Object) => void
>x : Object
>true ? (x: number) => { } : (x: Object) => { } : (x: number) => void
>true ? (x: number) => { } : (x: Object) => { } : ((x: number) => void) | ((x: Object) => void)
>true : true
>(x: number) => { } : (x: number) => void
>x : number
>(x: Object) => { } : (x: Object) => void
>x : Object

var r8 = true ? (x: Object) => { } : (x: number) => { }; // returns Object => void
>r8 : (x: Object) => void
>true ? (x: Object) => { } : (x: number) => { } : (x: Object) => void
>r8 : ((x: Object) => void) | ((x: number) => void)
>true ? (x: Object) => { } : (x: number) => { } : ((x: Object) => void) | ((x: number) => void)
>true : true
>(x: Object) => { } : (x: Object) => void
>x : Object
Expand Down
6 changes: 3 additions & 3 deletions tests/baselines/reference/functionWithMultipleReturnStatements2.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ function f4() {
}

function f5() {
>f5 : () => Object
>f5 : () => Object | 1

return 1;
>1 : 1
Expand Down Expand Up @@ -136,7 +136,7 @@ function f10() {

// returns number => void
function f11() {
>f11 : () => (x: number) => void
>f11 : () => ((x: number) => void) | ((x: Object) => void)

if (true) {
>true : true
Expand All @@ -154,7 +154,7 @@ function f11() {

// returns Object => void
function f12() {
>f12 : () => (x: Object) => void
>f12 : () => ((x: Object) => void) | ((x: number) => void)

if (true) {
>true : true
Expand Down
8 changes: 4 additions & 4 deletions tests/baselines/reference/subtypesOfTypeParameterWithConstraints2.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,16 +566,16 @@ function f20<T extends Number>(x: T) {
>x : T

var r19 = true ? new Object() : x; // ok
>r19 : Object
>true ? new Object() : x : Object
>r19 : Object | T
>true ? new Object() : x : Object | T
>true : true
>new Object() : Object
>Object : ObjectConstructor
>x : T

var r19 = true ? x : new Object(); // ok
>r19 : Object
>true ? x : new Object() : Object
>r19 : Object | T
>true ? x : new Object() : Object | T
>true : true
>x : T
>new Object() : Object
Expand Down
Loading