Skip to content

Remove duplicate signatures in intersections #32049

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

Merged
merged 5 commits into from
Jul 1, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
45 changes: 29 additions & 16 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7234,8 +7234,8 @@ namespace ts {
function resolveIntersectionTypeMembers(type: IntersectionType) {
// The members and properties collections are empty for intersection types. To get all properties of an
// intersection type use getPropertiesOfType (only the language service uses this).
let callSignatures: ReadonlyArray<Signature> = emptyArray;
let constructSignatures: ReadonlyArray<Signature> = emptyArray;
let callSignatures: Signature[] | undefined;
let constructSignatures: Signature[] | undefined;
let stringIndexInfo: IndexInfo | undefined;
let numberIndexInfo: IndexInfo | undefined;
const types = type.types;
Expand All @@ -7257,13 +7257,22 @@ namespace ts {
return clone;
});
}
constructSignatures = concatenate(constructSignatures, signatures);
constructSignatures = appendSignatures(constructSignatures, signatures);
}
callSignatures = concatenate(callSignatures, getSignaturesOfType(t, SignatureKind.Call));
callSignatures = appendSignatures(callSignatures, getSignaturesOfType(t, SignatureKind.Call));
stringIndexInfo = intersectIndexInfos(stringIndexInfo, getIndexInfoOfType(t, IndexKind.String));
numberIndexInfo = intersectIndexInfos(numberIndexInfo, getIndexInfoOfType(t, IndexKind.Number));
}
setStructuredTypeMembers(type, emptySymbols, callSignatures, constructSignatures, stringIndexInfo, numberIndexInfo);
setStructuredTypeMembers(type, emptySymbols, callSignatures || emptyArray, constructSignatures || emptyArray, stringIndexInfo, numberIndexInfo);
}

function appendSignatures(signatures: Signature[] | undefined, newSignatures: readonly Signature[]) {
for (const sig of newSignatures) {
if (!signatures || every(signatures, s => !compareSignaturesIdentical(s, sig, /*partialMatch*/ false, /*ignoreThisTypes*/ false, /*ignoreReturnTypes*/ false, compareTypesIdentical))) {
Copy link
Member

Choose a reason for hiding this comment

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

Uhhhh, it's probably worth noting that compareSignaturesIdentical needs some TLC - it currently erases constraints to any like compareSignatures used to, which causes spurious false positives. Concretely, that means that if I have a:

type Foo = (<T extends number>(x: T) => T) & (<T extends string>(x: T) => T)

I don't think we'll be happy with the result here... (I looked into this around #31023 )

signatures = append(signatures, sig);
}
}
return signatures;
}

/**
Expand Down Expand Up @@ -14313,20 +14322,25 @@ namespace ts {
if (!(isMatchingSignature(source, target, partialMatch))) {
return Ternary.False;
}
// Check that the two signatures have the same number of type parameters. We might consider
// also checking that any type parameter constraints match, but that would require instantiating
// the constraints with a common set of type arguments to get relatable entities in places where
// type parameters occur in the constraints. The complexity of doing that doesn't seem worthwhile,
// particularly as we're comparing erased versions of the signatures below.
// Check that the two signatures have the same number of type parameters.
if (length(source.typeParameters) !== length(target.typeParameters)) {
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);
// Check that type parameter constraints and defaults match. If they do, instantiate the source
// signature with the type parameters of the target signature and continue the comparison.
if (target.typeParameters) {
const mapper = createTypeMapper(source.typeParameters!, target.typeParameters);
for (let i = 0; i < target.typeParameters.length; i++) {
const s = source.typeParameters![i];
const t = target.typeParameters[i];
if (!(s === t || compareTypes(instantiateType(getConstraintFromTypeParameter(s), mapper) || unknownType, getConstraintFromTypeParameter(t) || unknownType) &&
compareTypes(instantiateType(getDefaultFromTypeParameter(s), mapper) || unknownType, getDefaultFromTypeParameter(t) || unknownType))) {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't think we can consider defaults when checking identity - if we do, then two conditional extends clauses that only differ in a local type parameter default (which doesn't affect relationship checking) will fail to be seen as identical.

Copy link
Member

Choose a reason for hiding this comment

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

Eg, (A extends <T = {}>() => T ? 1 : 0) extends (A extends <T = unknown>() => T ? 1 : 0) ? true : false should resolve to true, I think, not false.

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'm not sure I understand how that example is relevant. Best I can tell at no point will we compare signatures for identity in the example.

Copy link
Member

Choose a reason for hiding this comment

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

Conditional extends clauses are compared using isTypeIdenticalTo when checking if one conditional extends the other in the above example. Apparently this is already in use in the wild as a way to observe type "identity" in complex type toolbelt libraries.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I see that logic in structuredTypeRelatedTo. Still, as far as I'm concerned, those two types aren't identical and I can't think of a meaningful definition of "identical" that would make it true for them, but not for the other cases we care about. It would just seem really odd to make an exception for local type parameter defaults, but nothing else.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, we let you assign a function with a default to a function with a different default and vice-versa - it's just the typesystem equivalent of aliasing. Generally defaults aren't observable up until the point where inference fails - this exposes them into relationship checking before inference is ever invoked, success or no.

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 still don't think that merits a special exception to identity checking. The identity relation should be concerned with accurately identifying semantically identical types, and it does a better job of that now. I think we need to get this merged.

return Ternary.False;
}
}
source = instantiateSignature(source, mapper, /*eraseTypeParameters*/ true);
}
let result = Ternary.True;

if (!ignoreThisTypes) {
const sourceThisType = getThisTypeOfSignature(source);
if (sourceThisType) {
Expand All @@ -14340,7 +14354,6 @@ namespace ts {
}
}
}

const targetLen = getParameterCount(target);
for (let i = 0; i < targetLen; i++) {
const s = getTypeAtPosition(source, i);
Expand Down
36 changes: 36 additions & 0 deletions tests/baselines/reference/genericSignatureIdentity.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
tests/cases/compiler/genericSignatureIdentity.ts(10,5): error TS2403: Subsequent variable declarations must have the same type. Variable 'x' must be of type '<T extends Date>(x: T) => T', but here has type '<T extends number>(x: T) => T'.
tests/cases/compiler/genericSignatureIdentity.ts(14,5): error TS2403: Subsequent variable declarations must have the same type. Variable 'x' must be of type '<T extends Date>(x: T) => T', but here has type '<T>(x: T) => T'.
tests/cases/compiler/genericSignatureIdentity.ts(18,5): error TS2403: Subsequent variable declarations must have the same type. Variable 'x' must be of type '<T extends Date>(x: T) => T', but here has type '<T>(x: any) => any'.


==== tests/cases/compiler/genericSignatureIdentity.ts (3 errors) ====
// This test is here to remind us of our current limits of type identity checking.
// Ideally all of the below declarations would be considered different (and thus errors)
// but they aren't because we erase type parameters to type any and don't check that
// constraints are identical.

var x: {
<T extends Date>(x: T): T;
};

var x: {
~
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'x' must be of type '<T extends Date>(x: T) => T', but here has type '<T extends number>(x: T) => T'.
!!! related TS6203 tests/cases/compiler/genericSignatureIdentity.ts:6:5: 'x' was also declared here.
<T extends number>(x: T): T;
};

var x: {
~
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'x' must be of type '<T extends Date>(x: T) => T', but here has type '<T>(x: T) => T'.
!!! related TS6203 tests/cases/compiler/genericSignatureIdentity.ts:6:5: 'x' was also declared here.
<T>(x: T): T;
};

var x: {
~
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'x' must be of type '<T extends Date>(x: T) => T', but here has type '<T>(x: any) => any'.
!!! related TS6203 tests/cases/compiler/genericSignatureIdentity.ts:6:5: 'x' was also declared here.
<T>(x: any): any;
};

Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
tests/cases/compiler/identityForSignaturesWithTypeParametersAndAny.ts(2,5): error TS2403: Subsequent variable declarations must have the same type. Variable 'f' must be of type '<T, U>(x: T, y: U) => T', but here has type '<T, U>(x: any, y: any) => any'.
tests/cases/compiler/identityForSignaturesWithTypeParametersAndAny.ts(5,5): error TS2403: Subsequent variable declarations must have the same type. Variable 'g' must be of type '<T, U>(x: T, y: U) => T', but here has type '<T>(x: any, y: any) => any'.
tests/cases/compiler/identityForSignaturesWithTypeParametersAndAny.ts(8,5): error TS2403: Subsequent variable declarations must have the same type. Variable 'h' must be of type '<T, U>(x: T, y: U) => T', but here has type '(x: any, y: any) => any'.
tests/cases/compiler/identityForSignaturesWithTypeParametersAndAny.ts(11,5): error TS2403: Subsequent variable declarations must have the same type. Variable 'i' must be of type '<T, U>(x: T, y: U) => T', but here has type '<T, U>(x: any, y: string) => any'.
tests/cases/compiler/identityForSignaturesWithTypeParametersAndAny.ts(14,5): error TS2403: Subsequent variable declarations must have the same type. Variable 'j' must be of type '<T, U>(x: T, y: U) => T', but here has type '<T, U>(x: any, y: any) => string'.


==== tests/cases/compiler/identityForSignaturesWithTypeParametersAndAny.ts (4 errors) ====
==== tests/cases/compiler/identityForSignaturesWithTypeParametersAndAny.ts (5 errors) ====
var f: <T, U>(x: T, y: U) => T;
var f: <T, U>(x: any, y: any) => any;
~
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'f' must be of type '<T, U>(x: T, y: U) => T', but here has type '<T, U>(x: any, y: any) => any'.
!!! related TS6203 tests/cases/compiler/identityForSignaturesWithTypeParametersAndAny.ts:1:5: 'f' was also declared here.

var g: <T, U>(x: T, y: U) => T;
var g: <T>(x: any, y: any) => any;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
tests/cases/compiler/identityForSignaturesWithTypeParametersSwitched.ts(2,5): error TS2403: Subsequent variable declarations must have the same type. Variable 'f' must be of type '<T, U>(x: T, y: U) => T', but here has type '<T, U>(x: U, y: T) => U'.


==== tests/cases/compiler/identityForSignaturesWithTypeParametersSwitched.ts (1 errors) ====
var f: <T, U>(x: T, y: U) => T;
var f: <T, U>(x: U, y: T) => U;
~
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'f' must be of type '<T, U>(x: T, y: U) => T', but here has type '<T, U>(x: U, y: T) => U'.
!!! related TS6203 tests/cases/compiler/identityForSignaturesWithTypeParametersSwitched.ts:1:5: 'f' was also declared here.
9 changes: 9 additions & 0 deletions tests/baselines/reference/keyofAndIndexedAccess2.errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -237,4 +237,13 @@ tests/cases/conformance/types/keyof/keyofAndIndexedAccess2.ts(108,5): error TS23
type Baz<T, Q extends Foo<T>> = { [K in keyof Q]: T[Q[K]] };

type Qux<T, Q extends Bar<T>> = { [K in keyof Q]: T[Q[K]["0"]] };

// Repro from #32038

const actions = ['resizeTo', 'resizeBy'] as const;
for (const action of actions) {
window[action] = (x, y) => {
window[action](x, y);
}
}

16 changes: 16 additions & 0 deletions tests/baselines/reference/keyofAndIndexedAccess2.js
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,15 @@ type Bar<T> = { [key: string]: { [K in keyof T]: [K] }[keyof T] };
type Baz<T, Q extends Foo<T>> = { [K in keyof Q]: T[Q[K]] };

type Qux<T, Q extends Bar<T>> = { [K in keyof Q]: T[Q[K]["0"]] };

// Repro from #32038

const actions = ['resizeTo', 'resizeBy'] as const;
for (const action of actions) {
window[action] = (x, y) => {
window[action](x, y);
}
}


//// [keyofAndIndexedAccess2.js]
Expand Down Expand Up @@ -253,3 +262,10 @@ export class c {
this["a"] = "b";
}
}
// Repro from #32038
const actions = ['resizeTo', 'resizeBy'];
for (const action of actions) {
window[action] = (x, y) => {
window[action](x, y);
};
}
23 changes: 23 additions & 0 deletions tests/baselines/reference/keyofAndIndexedAccess2.symbols
Original file line number Diff line number Diff line change
Expand Up @@ -564,3 +564,26 @@ type Qux<T, Q extends Bar<T>> = { [K in keyof Q]: T[Q[K]["0"]] };
>Q : Symbol(Q, Decl(keyofAndIndexedAccess2.ts, 156, 11))
>K : Symbol(K, Decl(keyofAndIndexedAccess2.ts, 156, 35))

// Repro from #32038

const actions = ['resizeTo', 'resizeBy'] as const;
>actions : Symbol(actions, Decl(keyofAndIndexedAccess2.ts, 160, 5))

for (const action of actions) {
>action : Symbol(action, Decl(keyofAndIndexedAccess2.ts, 161, 10))
>actions : Symbol(actions, Decl(keyofAndIndexedAccess2.ts, 160, 5))

window[action] = (x, y) => {
>window : Symbol(window, Decl(lib.dom.d.ts, --, --))
>action : Symbol(action, Decl(keyofAndIndexedAccess2.ts, 161, 10))
>x : Symbol(x, Decl(keyofAndIndexedAccess2.ts, 162, 19))
>y : Symbol(y, Decl(keyofAndIndexedAccess2.ts, 162, 21))

window[action](x, y);
>window : Symbol(window, Decl(lib.dom.d.ts, --, --))
>action : Symbol(action, Decl(keyofAndIndexedAccess2.ts, 161, 10))
>x : Symbol(x, Decl(keyofAndIndexedAccess2.ts, 162, 19))
>y : Symbol(y, Decl(keyofAndIndexedAccess2.ts, 162, 21))
}
}

32 changes: 32 additions & 0 deletions tests/baselines/reference/keyofAndIndexedAccess2.types
Original file line number Diff line number Diff line change
Expand Up @@ -540,3 +540,35 @@ type Baz<T, Q extends Foo<T>> = { [K in keyof Q]: T[Q[K]] };
type Qux<T, Q extends Bar<T>> = { [K in keyof Q]: T[Q[K]["0"]] };
>Qux : Qux<T, Q>

// Repro from #32038

const actions = ['resizeTo', 'resizeBy'] as const;
>actions : readonly ["resizeTo", "resizeBy"]
>['resizeTo', 'resizeBy'] as const : readonly ["resizeTo", "resizeBy"]
>['resizeTo', 'resizeBy'] : readonly ["resizeTo", "resizeBy"]
>'resizeTo' : "resizeTo"
>'resizeBy' : "resizeBy"

for (const action of actions) {
>action : "resizeTo" | "resizeBy"
>actions : readonly ["resizeTo", "resizeBy"]

window[action] = (x, y) => {
>window[action] = (x, y) => { window[action](x, y); } : (x: number, y: number) => void
>window[action] : ((x: number, y: number) => void) & ((x: number, y: number) => void)
>window : Window
>action : "resizeTo" | "resizeBy"
>(x, y) => { window[action](x, y); } : (x: number, y: number) => void
>x : number
>y : number

window[action](x, y);
>window[action](x, y) : void
>window[action] : ((x: number, y: number) => void) | ((x: number, y: number) => void)
>window : Window
>action : "resizeTo" | "resizeBy"
>x : number
>y : number
}
}

28 changes: 28 additions & 0 deletions tests/baselines/reference/promiseIdentity.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
tests/cases/compiler/promiseIdentity.ts(21,5): error TS2403: Subsequent variable declarations must have the same type. Variable 'y' must be of type 'IPromise2<string, number>', but here has type 'Promise2<any, string>'.


==== tests/cases/compiler/promiseIdentity.ts (1 errors) ====
export interface IPromise<T> {
then<U>(callback: (x: T) => IPromise<U>): IPromise<U>;
}
interface Promise<T> {
then<U>(callback: (x: T) => Promise<U>): Promise<U>;
}
var x: IPromise<string>;
var x: Promise<string>;


interface IPromise2<T, V> {
then<U, W>(callback: (x: T) => IPromise2<U, W>): IPromise2<W, U>;
}
interface Promise2<T, V> {
then<U, W>(callback: (x: V) => Promise2<U, T>): Promise2<T, W>; // Uses V instead of T in callback's parameter
}

// Ok because T in this particular Promise2 is any, as are all the U and W references.
// Also, the V of Promise2 happens to coincide with the T of IPromise2 (they are both string).
var y: IPromise2<string, number>;
var y: Promise2<any, string>;
~
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'y' must be of type 'IPromise2<string, number>', but here has type 'Promise2<any, string>'.
!!! related TS6203 tests/cases/compiler/promiseIdentity.ts:20:5: 'y' was also declared here.
17 changes: 17 additions & 0 deletions tests/baselines/reference/promiseIdentityWithAny.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
tests/cases/compiler/promiseIdentityWithAny.ts(10,5): error TS2403: Subsequent variable declarations must have the same type. Variable 'x' must be of type 'IPromise<string, number>', but here has type 'Promise<string, boolean>'.


==== tests/cases/compiler/promiseIdentityWithAny.ts (1 errors) ====
export interface IPromise<T, V> {
then<U, W>(callback: (x: T) => IPromise<U, W>): IPromise<U, W>;
}
export interface Promise<T, V> {
then<U, W>(callback: (x: T) => Promise<any, any>): Promise<any, any>;
}

// Should be ok because signature type parameters get erased to any
var x: IPromise<string, number>;
var x: Promise<string, boolean>;
~
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'x' must be of type 'IPromise<string, number>', but here has type 'Promise<string, boolean>'.
!!! related TS6203 tests/cases/compiler/promiseIdentityWithAny.ts:9:5: 'x' was also declared here.
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
tests/cases/compiler/promiseIdentityWithConstraints.ts(10,5): error TS2403: Subsequent variable declarations must have the same type. Variable 'x' must be of type 'IPromise<string, number>', but here has type 'Promise<string, boolean>'.


==== tests/cases/compiler/promiseIdentityWithConstraints.ts (1 errors) ====
export interface IPromise<T, V> {
then<U extends T, W extends V>(callback: (x: T) => IPromise<U, W>): IPromise<U, W>;
}
export interface Promise<T, V> {
then<U extends T, W extends V>(callback: (x: T) => Promise<U, W>): Promise<U, W>;
}

// Error because constraint V doesn't match
var x: IPromise<string, number>;
var x: Promise<string, boolean>;
~
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'x' must be of type 'IPromise<string, number>', but here has type 'Promise<string, boolean>'.
!!! related TS6203 tests/cases/compiler/promiseIdentityWithConstraints.ts:9:5: 'x' was also declared here.
9 changes: 9 additions & 0 deletions tests/cases/conformance/types/keyof/keyofAndIndexedAccess2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,3 +158,12 @@ type Bar<T> = { [key: string]: { [K in keyof T]: [K] }[keyof T] };
type Baz<T, Q extends Foo<T>> = { [K in keyof Q]: T[Q[K]] };

type Qux<T, Q extends Bar<T>> = { [K in keyof Q]: T[Q[K]["0"]] };

// Repro from #32038

const actions = ['resizeTo', 'resizeBy'] as const;
for (const action of actions) {
window[action] = (x, y) => {
window[action](x, y);
}
}