Skip to content

Ignore bivariance in subtype relationship#41979

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

Closed
Ryan Cavanaugh (RyanCavanaugh) wants to merge 10 commits into from
Closed

Ignore bivariance in subtype relationship #41979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6df05fa
Have signature identity checks look at the base constraint of type pa…
weswigham Apr 18, 2019
d36dfcb
Update text in fourslash test
weswigham Apr 18, 2019
0580cca
Add whitespace to fix lint, remove duplicate impl
weswigham Apr 18, 2019
01dea4b
Consolidate names
weswigham Apr 18, 2019
40d4f30
Merge branch 'master' into better-signature-identity-checks
weswigham Mar 4, 2020
e14c58e
Remove comparisons of type parameter defaults, add more test cases
weswigham Mar 4, 2020
3037443
Merge branch 'master' into better-signature-identity-checks
weswigham Dec 15, 2020
376a03b
Ignore method bivariance in the subtype and strict subtype relationships
RyanCavanaugh Dec 15, 2020
767cf7d
Merge remote-tracking branch 'upstream/master' into fix41977
RyanCavanaugh Dec 16, 2020
7479b8a
Fix build break
RyanCavanaugh Dec 16, 2020
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
70 changes: 57 additions & 13 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -194,6 +194,7 @@ namespace ts {
StrictCallback = 1 << 1,
IgnoreReturnTypes = 1 << 2,
StrictArity = 1 << 3,
IgnoreBivariance = 1 << 4,
Callback = BivariantCallback | StrictCallback,
}

Expand DownExpand Up@@ -10274,7 +10275,7 @@ namespace ts {
if (signatures !== masterList) {
const signature = signatures[0];
Debug.assert(!!signature, "getUnionSignatures bails early on empty signature lists and should not have empty lists on second pass");
results = signature.typeParameters && some(results, s => !!s.typeParameters) ? undefined : map(results, sig => combineSignaturesOfUnionMembers(sig, signature));
results = signature.typeParameters && some(results, s => !!s.typeParameters && !compareTypeParametersIdentical(signature.typeParameters!, s.typeParameters)) ? undefined : map(results, sig => combineSignaturesOfUnionMembers(sig, signature));
if (!results) {
break;
}
Expand All@@ -10285,18 +10286,39 @@ namespace ts {
return result || emptyArray;
}

function combineUnionThisParam(left: Symbol | undefined, right: Symbol | undefined): Symbol | undefined {
function compareTypeParametersIdentical(sourceParams: readonly TypeParameter[], targetParams: readonly TypeParameter[]): boolean {
if (sourceParams.length !== targetParams.length) {
return false;
}

const mapper = createTypeMapper(targetParams, sourceParams);
for (let i = 0; i < sourceParams.length; i++) {
const source = sourceParams[i];
const target = targetParams[i];
if (source === target) continue;
// We instantiate the target type parameter constraints into the source types so we can recognize `<T, U extends T>` as the same as `<A, B extends A>`
if (!isTypeIdenticalTo(getConstraintFromTypeParameter(source) || unknownType, instantiateType(getConstraintFromTypeParameter(target) || unknownType, mapper))) return false;
// We don't compare defaults - we just use the type parameter defaults from the first signature that seems to match.
// It might make sense to combine these defaults in the future, but doing so intelligently requires knowing
// if the parameter is used covariantly or contravariantly (so we intersect if it's used like a parameter or union if used like a return type)
// and, since it's just an inference _default_, just picking one arbitrarily works OK.
}

return true;
}

function combineUnionThisParam(left: Symbol | undefined, right: Symbol | undefined, mapper: TypeMapper | undefined): Symbol | undefined {
if (!left || !right) {
return left || right;
}
// A signature `this` type might be a read or a write position... It's very possible that it should be invariant
// and we should refuse to merge signatures if there are `this` types and they do not match. However, so as to be
// permissive when calling, for now, we'll intersect the `this` types just like we do for param types in union signatures.
const thisType = getIntersectionType([getTypeOfSymbol(left), getTypeOfSymbol(right)]);
const thisType = getIntersectionType([getTypeOfSymbol(left), instantiateType(getTypeOfSymbol(right), mapper)]);
return createSymbolWithType(left, thisType);
}

function combineUnionParameters(left: Signature, right: Signature) {
function combineUnionParameters(left: Signature, right: Signature, mapper: TypeMapper | undefined) {
const leftCount = getParameterCount(left);
const rightCount = getParameterCount(right);
const longest = leftCount >= rightCount ? left : right;
Expand All@@ -10306,8 +10328,14 @@ namespace ts {
const needsExtraRestElement = eitherHasEffectiveRest && !hasEffectiveRestParameter(longest);
const params = new Array<Symbol>(longestCount + (needsExtraRestElement ? 1 : 0));
for (let i = 0; i < longestCount; i++) {
const longestParamType = tryGetTypeAtPosition(longest, i)!;
const shorterParamType = tryGetTypeAtPosition(shorter, i) || unknownType;
let longestParamType = tryGetTypeAtPosition(longest, i)!;
if (longest === right) {
longestParamType = instantiateType(longestParamType, mapper);
}
let shorterParamType = tryGetTypeAtPosition(shorter, i) || unknownType;
if (shorter === right) {
shorterParamType = instantiateType(shorterParamType, mapper);
}
const unionParamType = getIntersectionType([longestParamType, shorterParamType]);
const isRestParam = eitherHasEffectiveRest && !needsExtraRestElement && i === (longestCount - 1);
const isOptional = i >= getMinArgumentCount(longest) && i >= getMinArgumentCount(shorter);
Expand All@@ -10328,19 +10356,28 @@ namespace ts {
if (needsExtraRestElement) {
const restParamSymbol = createSymbol(SymbolFlags.FunctionScopedVariable, "args" as __String);
restParamSymbol.type = createArrayType(getTypeAtPosition(shorter, longestCount));
if (shorter === right) {
restParamSymbol.type = instantiateType(restParamSymbol.type, mapper);
}
params[longestCount] = restParamSymbol;
}
return params;
}

function combineSignaturesOfUnionMembers(left: Signature, right: Signature): Signature {
const typeParams = left.typeParameters || right.typeParameters;
let paramMapper: TypeMapper | undefined;
if (left.typeParameters && right.typeParameters) {
paramMapper = createTypeMapper(right.typeParameters, left.typeParameters);
// We just use the type parameter defaults from the first signature
}
const declaration = left.declaration;
const params = combineUnionParameters(left, right);
const thisParam = combineUnionThisParam(left.thisParameter, right.thisParameter);
const params = combineUnionParameters(left, right, paramMapper);
const thisParam = combineUnionThisParam(left.thisParameter, right.thisParameter, paramMapper);
const minArgCount = Math.max(left.minArgumentCount, right.minArgumentCount);
const result = createSignature(
declaration,
left.typeParameters || right.typeParameters,
typeParams,
thisParam,
params,
/*resolvedReturnType*/ undefined,
Expand All@@ -10349,6 +10386,9 @@ namespace ts {
(left.flags | right.flags) & SignatureFlags.PropagatingFlags
);
result.unionSignatures = concatenate(left.unionSignatures || [left], [right]);
if (paramMapper) {
result.mapper = left.mapper && left.unionSignatures ? combineTypeMappers(left.mapper, paramMapper) : paramMapper;
}
return result;
}

Expand DownExpand Up@@ -11882,7 +11922,7 @@ namespace ts {
return errorType;
}
let type = signature.target ? instantiateType(getReturnTypeOfSignature(signature.target), signature.mapper) :
signature.unionSignatures ? getUnionType(map(signature.unionSignatures, getReturnTypeOfSignature), UnionReduction.Subtype) :
signature.unionSignatures ? instantiateType(getUnionType(map(signature.unionSignatures, getReturnTypeOfSignature), UnionReduction.Subtype), signature.mapper) :
getReturnTypeFromAnnotation(signature.declaration!) ||
(nodeIsMissing((<FunctionLikeDeclaration>signature.declaration).body) ? anyType : getReturnTypeFromBody(<FunctionLikeDeclaration>signature.declaration));
if (signature.flags & SignatureFlags.IsInnerCallChain) {
Expand DownExpand Up@@ -16227,8 +16267,12 @@ namespace ts {
}

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

const sourceThisType = getThisTypeOfSignature(source);
Expand DownExpand Up@@ -18485,7 +18529,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,
relation === strictSubtypeRelation ? SignatureCheckMode.StrictArity : 0, reportErrors, reportError, incompatibleReporter, isRelatedTo, makeFunctionTypeMapper(reportUnreliableMarkers));
relation === strictSubtypeRelation ? (SignatureCheckMode.StrictArity | SignatureCheckMode.IgnoreBivariance) : 0, reportErrors, reportError, incompatibleReporter, isRelatedTo, makeFunctionTypeMapper(reportUnreliableMarkers));
}

function signaturesIdenticalTo(source: Type, target: Type, kind: SignatureKind): Ternary {
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/visitorPublic.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -36,7 +36,7 @@ namespace ts {
return undefined;
}
else if (isArray(visited)) {
visitedNode = (lift || extractSingleNode)(visited);
visitedNode = (lift || extractSingleNode)(visited as unknown as NodeArray<Node>);

@RyanCavanaughRyan Cavanaugh (RyanCavanaugh)Dec 16, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

cc Wesley Wigham (@weswigham)Anders Hejlsberg (@ahejlsberg) interesting break here. visited is narrowed to Node[] here, but lift is undefined | (arg: NodeArray<Node>) => Node, so we were passing what we only proved to be a Node[] to something that needed a subtype of Node[]. It's not immediately clear to me if this is a bug-in-fact but it's definitely not correct per our normal definitions, i.e. you could not have written

if(lift)lift(visited);

on this line

}
else {
visitedNode = visited;
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/arrayLiteralWithMultipleBestCommonTypes.types
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -36,8 +36,8 @@ var cs = [a, b, c]; // { x: number; y?: number };[]
>c : { x: number; a?: number; }

var ds = [(x: Object) => 1, (x: string) => 2]; // { (x:Object) => number }[]
>ds : ((x: Object) => number)[]
>[(x: Object) => 1, (x: string) => 2] : ((x: Object) => number)[]
>ds : ((x: string) => number)[]
>[(x: Object) => 1, (x: string) => 2] : ((x: string) => number)[]
>(x: Object) => 1 : (x: Object) => number
>x : Object
>1 : 1
Expand Down
14 changes: 7 additions & 7 deletions tests/baselines/reference/arrayOfFunctionTypes3.types
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -50,28 +50,28 @@ var c: { (x: number): number; (x: any): any; };
>x : any

var z = [a, b, c];
>z : { (x: number): number; (x: any): any; }[]
>[a, b, c] : { (x: number): number; (x: any): any; }[]
>z : ({ (x: number): number; (x: string): string; } | { (x: number): number; (x: any): any; })[]
>[a, b, c] : ({ (x: number): number; (x: string): string; } | { (x: number): number; (x: any): any; })[]
>a : { (x: number): number; (x: string): string; }
>b : { (x: number): number; (x: string): string; }
>c : { (x: number): number; (x: any): any; }

var r4 = z[0];
>r4 : { (x: number): number; (x: any): any; }
>z[0] : { (x: number): number; (x: any): any; }
>z : { (x: number): number; (x: any): any; }[]
>r4 : { (x: number): number; (x: string): string; } | { (x: number): number; (x: any): any; }
>z[0] : { (x: number): number; (x: string): string; } | { (x: number): number; (x: any): any; }
>z : ({ (x: number): number; (x: string): string; } | { (x: number): number; (x: any): any; })[]
>0 : 0

var r5 = r4(''); // any not string
>r5 : any
>r4('') : any
>r4 : { (x: number): number; (x: any): any; }
>r4 : { (x: number): number; (x: string): string; } | { (x: number): number; (x: any): any; }
>'' : ""

var r5b = r4(1);
>r5b : number
>r4(1) : number
>r4 : { (x: number): number; (x: any): any; }
>r4 : { (x: number): number; (x: string): string; } | { (x: number): number; (x: any): any; }
>1 : 1

var a2: { <T>(x: T): number; (x: string): string;};
Expand Down
18 changes: 6 additions & 12 deletions tests/baselines/reference/bestChoiceType.symbols
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2,14 +2,12 @@
// Repro from #10041

(''.match(/ /) || []).map(s => s.toLowerCase());
>(''.match(/ /) || []).map : Symbol(Array.map, Decl(lib.es5.d.ts, --, --))
>(''.match(/ /) || []).map : Symbol(Array.map, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>''.match : Symbol(String.match, Decl(lib.es5.d.ts, --, --))
>match : Symbol(String.match, Decl(lib.es5.d.ts, --, --))
>map : Symbol(Array.map, Decl(lib.es5.d.ts, --, --))
>map : Symbol(Array.map, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>s : Symbol(s, Decl(bestChoiceType.ts, 2, 26))
>s.toLowerCase : Symbol(String.toLowerCase, Decl(lib.es5.d.ts, --, --))
>s : Symbol(s, Decl(bestChoiceType.ts, 2, 26))
>toLowerCase : Symbol(String.toLowerCase, Decl(lib.es5.d.ts, --, --))

// Similar cases

Expand All@@ -27,13 +25,11 @@ function f1() {

let z = y.map(s => s.toLowerCase());
>z : Symbol(z, Decl(bestChoiceType.ts, 9, 7))
>y.map : Symbol(Array.map, Decl(lib.es5.d.ts, --, --))
>y.map : Symbol(Array.map, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>y : Symbol(y, Decl(bestChoiceType.ts, 8, 7))
>map : Symbol(Array.map, Decl(lib.es5.d.ts, --, --))
>map : Symbol(Array.map, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>s : Symbol(s, Decl(bestChoiceType.ts, 9, 18))
>s.toLowerCase : Symbol(String.toLowerCase, Decl(lib.es5.d.ts, --, --))
>s : Symbol(s, Decl(bestChoiceType.ts, 9, 18))
>toLowerCase : Symbol(String.toLowerCase, Decl(lib.es5.d.ts, --, --))
}

function f2() {
Expand All@@ -51,12 +47,10 @@ function f2() {

let z = y.map(s => s.toLowerCase());
>z : Symbol(z, Decl(bestChoiceType.ts, 15, 7))
>y.map : Symbol(Array.map, Decl(lib.es5.d.ts, --, --))
>y.map : Symbol(Array.map, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>y : Symbol(y, Decl(bestChoiceType.ts, 14, 7))
>map : Symbol(Array.map, Decl(lib.es5.d.ts, --, --))
>map : Symbol(Array.map, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
>s : Symbol(s, Decl(bestChoiceType.ts, 15, 18))
>s.toLowerCase : Symbol(String.toLowerCase, Decl(lib.es5.d.ts, --, --))
>s : Symbol(s, Decl(bestChoiceType.ts, 15, 18))
>toLowerCase : Symbol(String.toLowerCase, Decl(lib.es5.d.ts, --, --))
}

74 changes: 37 additions & 37 deletions tests/baselines/reference/bestChoiceType.types
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2,23 +2,23 @@
// Repro from #10041

(''.match(/ /) || []).map(s => s.toLowerCase());
>(''.match(/ /) || []).map(s => s.toLowerCase()) : string[]
>(''.match(/ /) || []).map : <U>(callbackfn: (value: string, index: number, array: string[]) => U, thisArg?: any) => U[]
>(''.match(/ /) || []) : RegExpMatchArray
>''.match(/ /) || [] : RegExpMatchArray
>(''.match(/ /) || []).map(s => s.toLowerCase()) : any[]
>(''.match(/ /) || []).map : (<U>(callbackfn: (value: string, index: number, array: string[]) => U, thisArg?: any) => U[]) | (<U>(callbackfn: (value: never, index: number, array: never[]) => U, thisArg?: any) => U[])
>(''.match(/ /) || []) : RegExpMatchArray | never[]
>''.match(/ /) || [] : RegExpMatchArray | never[]
>''.match(/ /) : RegExpMatchArray | null
>''.match : (regexp: string | RegExp) => RegExpMatchArray | null
>'' : ""
>match : (regexp: string | RegExp) => RegExpMatchArray | null
>/ / : RegExp
>[] : never[]
>map : <U>(callbackfn: (value: string, index: number, array: string[]) => U, thisArg?: any) => U[]
>s => s.toLowerCase() : (s: string) => string
>s : string
>s.toLowerCase() : string
>s.toLowerCase : () => string
>s : string
>toLowerCase : () => string
>map : (<U>(callbackfn: (value: string, index: number, array: string[]) => U, thisArg?: any) => U[]) | (<U>(callbackfn: (value: never, index: number, array: never[]) => U, thisArg?: any) => U[])
>s => s.toLowerCase() : (s: any) => any
>s : any
>s.toLowerCase() : any
>s.toLowerCase : any
>s : any
>toLowerCase : any

// Similar cases

Expand All@@ -34,23 +34,23 @@ function f1() {
>/ / : RegExp

let y = x || [];
>y : RegExpMatchArray
>x || [] : RegExpMatchArray
>y : RegExpMatchArray | never[]
>x || [] : RegExpMatchArray | never[]
>x : RegExpMatchArray | null
>[] : never[]

let z = y.map(s => s.toLowerCase());
>z : string[]
>y.map(s => s.toLowerCase()) : string[]
>y.map : <U>(callbackfn: (value: string, index: number, array: string[]) => U, thisArg?: any) => U[]
>y : RegExpMatchArray
>map : <U>(callbackfn: (value: string, index: number, array: string[]) => U, thisArg?: any) => U[]
>s => s.toLowerCase() : (s: string) => string
>s : string
>s.toLowerCase() : string
>s.toLowerCase : () => string
>s : string
>toLowerCase : () => string
>z : any[]
>y.map(s => s.toLowerCase()) : any[]
>y.map : (<U>(callbackfn: (value: string, index: number, array: string[]) => U, thisArg?: any) => U[]) | (<U>(callbackfn: (value: never, index: number, array: never[]) => U, thisArg?: any) => U[])
>y : RegExpMatchArray | never[]
>map : (<U>(callbackfn: (value: string, index: number, array: string[]) => U, thisArg?: any) => U[]) | (<U>(callbackfn: (value: never, index: number, array: never[]) => U, thisArg?: any) => U[])
>s => s.toLowerCase() : (s: any) => any
>s : any
>s.toLowerCase() : any
>s.toLowerCase : any
>s : any
>toLowerCase : any
}

function f2() {
Expand All@@ -65,23 +65,23 @@ function f2() {
>/ / : RegExp

let y = x ? x : [];
>y : RegExpMatchArray
>x ? x : [] : RegExpMatchArray
>y : RegExpMatchArray | never[]
>x ? x : [] : RegExpMatchArray | never[]
>x : RegExpMatchArray | null
>x : RegExpMatchArray
>[] : never[]

let z = y.map(s => s.toLowerCase());
>z : string[]
>y.map(s => s.toLowerCase()) : string[]
>y.map : <U>(callbackfn: (value: string, index: number, array: string[]) => U, thisArg?: any) => U[]
>y : RegExpMatchArray
>map : <U>(callbackfn: (value: string, index: number, array: string[]) => U, thisArg?: any) => U[]
>s => s.toLowerCase() : (s: string) => string
>s : string
>s.toLowerCase() : string
>s.toLowerCase : () => string
>s : string
>toLowerCase : () => string
>z : any[]
>y.map(s => s.toLowerCase()) : any[]
>y.map : (<U>(callbackfn: (value: string, index: number, array: string[]) => U, thisArg?: any) => U[]) | (<U>(callbackfn: (value: never, index: number, array: never[]) => U, thisArg?: any) => U[])
>y : RegExpMatchArray | never[]
>map : (<U>(callbackfn: (value: string, index: number, array: string[]) => U, thisArg?: any) => U[]) | (<U>(callbackfn: (value: never, index: number, array: never[]) => U, thisArg?: any) => U[])
>s => s.toLowerCase() : (s: any) => any
>s : any
>s.toLowerCase() : any
>s.toLowerCase : any
>s : any
>toLowerCase : any
}

4 changes: 2 additions & 2 deletions tests/baselines/reference/contextualTypingArrayOfLambdas.types
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -23,8 +23,8 @@ class C extends A {
}

var xs = [(x: A) => { }, (x: B) => { }, (x: C) => { }];
>xs : ((x: A) => void)[]
>[(x: A) => { }, (x: B) => { }, (x: C) => { }] : ((x: A) => void)[]
>xs : (((x: B) => void) | ((x: C) => void))[]
>[(x: A) => { }, (x: B) => { }, (x: C) => { }] : (((x: B) => void) | ((x: C) => void))[]
>(x: A) => { } : (x: A) => void
>x : A
>(x: B) => { } : (x: B) => void
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/initializedDestructuringAssignmentTypes.types
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -3,7 +3,7 @@ const [, a = ''] = ''.match('') || [];
> : undefined
>a : string
>'' : ""
>''.match('') || [] : RegExpMatchArray
>''.match('') || [] : RegExpMatchArray | undefined[]
>''.match('') : RegExpMatchArray
>''.match : (regexp: string | RegExp) => RegExpMatchArray
>'' : ""
Expand Down
Loading