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.

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

Merge overloads that only differ by return type when intersecting #30520

Closed
wants to merge 5 commits into from
Closed

Merge overloads that only differ by return type when intersecting #30520

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bf5b762
Merge overloads that only differ by return type when intersecting.
kring Mar 21, 2019
ca33f40
Fix lint, other style conformance.
kring Mar 21, 2019
201c263
Update one test, add another.
kring Mar 21, 2019
35f064d
Require identical type parameters to intersect call signatures.
kring Mar 24, 2019
8896207
Merge branch 'master' into intersect-return-types
RyanCavanaugh Apr 25, 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
71 changes: 71 additions & 0 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7151,9 +7151,80 @@ namespace ts {
stringIndexInfo = intersectIndexInfos(stringIndexInfo, getIndexInfoOfType(t, IndexKind.String));
numberIndexInfo = intersectIndexInfos(numberIndexInfo, getIndexInfoOfType(t, IndexKind.Number));
}
callSignatures = intersectCallSignatures(callSignatures);
Copy link
Member

@weswigham weswigham Apr 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I wouldn't call this quite right. This has lost the nuance of which type the signatures originally came from. Say I wrote:

type WhyTho = {
  (): {x;};
  (): {y;};
} & {prop;}

with this logic, when I called something of type WhyTho, the result would be of type {x;} & {y;}, even though at no point did I actually intersect multiple callable things. Like our recent union calling infrastructure, this needs to operate pairwise over the input overload lists, and only merge between types. Eg, if I write

type Another = {
  (first: number): {x;};
  (): {y;};
  (): {q;};
} & {
  (): {x;}
} & {
  (arg: number): {z;}
}

I expect to see a resulting overload list along the lines of

  (first: number): {x;} & {z;};
  (): {y;} & {x;};
  (): {q;} & {x;};

it'd be a different story if for overload resolution we always resolved all applicable overloads "simultaneously" and merged the result, but unfortunately we always pick the first applicable overload, so retaining overload list order is important to retaining consistent behavior.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for taking a look @weswigham! I thought of that while implementing this, but I couldn't think of a way that a function could satisfy the the type WhyTho without also satisfying the intersected form produced by my change. For example:

type WhyTho = {
    (): {x;};
    (): {y;};
  } & {prop;}

function asdf() {
    return {
        x: 1
    };
}

asdf.prop = 4;

const whatever: WhyTho = asdf;

The error on the last line is:

Type 'typeof asdf' is not assignable to type 'WhyTho'.
  Type 'typeof asdf' is not assignable to type '{ (): { x: any; }; (): { y: any; }; }'.
    Property 'y' is missing in type '{ x: number; }' but required in type '{ y: any; }'.ts(2322)

How can that error by resolved other than by changing asdf to return the intersected type? I think that's why most languages I'm familiar with consider it an error in the first place to have overloads that differ only be return type.

Still, I agree it's undesirable that the signature changes just because of an unrelated intersection. Assuming it wouldn't be acceptable to always intersect return types of signatures with identical parameters (even in the absence of an explicit intersection), I'll see if I can change the code to only intersect return types of signatures from different intersected types. Any advice in that regard is greatly appreciated of course!

Copy link
Author

@kring kring Apr 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

To answer my own question:

How can that error by resolved other than by changing asdf to return the intersected type?

It can be resolved by declaring an overload, of course!

function asdf(): {x:any};
function asdf(): {y:any};
function asdf(): any {
    return 'but what should this value really be?';
}

It kind of makes my brain hurt to figure out what those two overloads with identical parameters are supposed to tell us about that function. It sort of just seems like it should be an error, but I'm sure there are good reasons that it's not.

So moving on, I'm implementing the change to only intersect return types across types, never within a type, and it seems mostly straightforward so far. I just want to clarify what should happen in a slightly more complicated situation than the one you mentioned:

type Another = {
  (first: number): {x;};
  (): {y;};
  (): {q;};
} & {
  (): {x;}
  (): {r;} // this line added
} & {
  (arg: number): {z;}
}

I think in this case, it should create no-parameters signatures which are the Cartesian product of the no-parameters signatures in the intersected types. So:

  (first: number): {x;} & {z;};
  (): {y;} & {x;};
  (): {y;} & {r;};
  (): {q;} & {x;};
  (): {q;} & {r;};

Does that seem right to you?

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 in this case, it should create no-parameters signatures which are the Cartesian product of the no-parameters signatures in the intersected types.

I agree with the premise, but also disagree with doing it for the same reason we don't combine multiple multi-overload sets when manufacturing union signatures: overload order matters, and the "correct" ordering of the interleaved product of signatures is ambiguous. So long as overload order matters, we can't actually combine multiple ordered lists (since the only "correct" behavior would be to resolve on each type of the union/intersection separately and combine the single selected signature).

Copy link
Author

@kring kring May 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I've been giving this a fair bit of thought over the past few days, and, if I'm understanding you correctly, we need to make sure that any change to signature intersection follows these rules.

Upon intersection:

  1. the overloads within any of the constituent types of the intersection cannot be combined / have their return types intersected, even if they differ only be return type.
  2. the overloads within any of the constituent types of the intersection cannot change order relative to one another.

In addition, there may be one more rule:
3. The overloads of an intersection type must occur in the order of intersection of their source types.

If all three of these rules must hold, the result of an intersection will have to be pretty much as it is today, except that if two overloads that differ only by return type happen to be right next to each other in the list (as currently computed in master), and they come from different types, then they can be turned into a single overload by intersecting their return types.

So in this example:

type Another = {
  (first: number): {x;}
  (): {y;}
  (): {q;}
} & {
  (): {x;}
  (): {r;}
} & {
  (arg: number): {z;}
}

The result would be:

(first: number): {x;}
(): {y;}
(): {q;} & {x;}
(): {r;}
(arg: number): {z;}

However, if we can relax rule 3, we could achieve something like I think you were getting at with this statement:

the only "correct" behavior would be to resolve on each type of the union/intersection separately and combine the single selected signature

If I'm understanding you correctly, you're saying the only correct algorithm would be this: given a function call on an intersected type, we perform overload resolution on all of the constituent intersected types independently. Some may resolve successfully, some may not. Put all the successful ones in a list in the same order as the types they came from. Then, select the first signature from the list and intersect its return type with all other signatures in the list that differ from the first one only be return type.

That won't really uphold rule 3. Given the type above and this call site:

const f: Another = ...;
f();

We select (): {y;} from the first type in the intersection and (): {x;} from the second type, so the intersected signature would be (): {y;} & {x;}. That puts precedence on (): {x;} from the second type over (): {q;} from the first type, violating rule 3.

But if that's ok (I think it is! It's less surprising than the (): {y;} we get when we strictly enforce rule 3), then we can achieve the same effect as this call-site algorithm at intersection-time with an algorithm like this:

for all types T in intersection {
  for all _remaining_ signatures S in type T {
    signature R = S
    for all types T' in the intersection that _follow_ T {
      for all _remaining_ signatures S' in type T' {
        if S is identical to S' (ignoring return type) {
          change the return type of R to the intersection of the return types of R and S'
          remove S' from consideration in the T/T' signature set
          break loop over signatures of T'
        }
      }
    }
    add overload R to result
  }
}

Which for the type above would yield:

{
  (first: number): {x;} & {z;}
  (): {y;} & {x;}
  (): {q;} & {r;}
}

Which seems quite sensible to me.

Does that make sense to you?

I've been studying your recent union changes (#29011), trying to understand how that approach would apply to the intersection case. I've learned a lot about how to actually compare the signatures, but less obvious is how the intersection should behave.

By the way, I find this interesting and am happy to keep working on it even if that involves a bunch of false starts and iteration. Do let me know if I'm just wasting your time, though. There's no point in doing that. :)

setStructuredTypeMembers(type, emptySymbols, callSignatures, constructSignatures, stringIndexInfo, numberIndexInfo);
}

function callSignaturesAreIdenticalExceptReturnType(a: Signature, b: Signature): boolean {
const parametersA = a.parameters;
const parametersB = b.parameters;

if (parametersA.length !== parametersB.length) {
return false;
}

if (!every(parametersA, (parameter, i) => isTypeIdenticalTo(getTypeOfParameter(parameter), getTypeOfParameter(parametersB[i])))) {
return false;
}

// Parameter types are identical, now check type parameters.
const typesA = a.typeParameters;
const typesB = b.typeParameters;

if (typesA === undefined || typesB === undefined) {
return typesA === typesB;
}

if (typesA.length !== typesB.length) {
return false;
}

return every(typesA, (parameter, i) => {
const constraintA = getConstraintOfTypeParameter(parameter);
const constraintB = getConstraintOfTypeParameter(typesB[i]);
const sameConstraints = (constraintA === undefined && constraintB === undefined) ||
(constraintA !== undefined && constraintB !== undefined && isTypeIdenticalTo(constraintA, constraintB));

const defaultA = parameter.default;
const defaultB = typesB[i].default;
const sameDefaults = (defaultA === undefined && defaultB === undefined) ||
(defaultA !== undefined && defaultB !== undefined && isTypeIdenticalTo(defaultA, defaultB));

return sameConstraints && sameDefaults;
});
}

function intersectCallSignatures(callSignatures: ReadonlyArray<Signature>): ReadonlyArray<Signature> {
if (callSignatures.length === 0) {
return callSignatures;
}

// Overloads that differ only by return type are not usable, so:
// 1. Group all the call signatures by their type parameters + parameter types. Each group is an overload.
// 2. Create a new signature for each group where the return type is the intersection
// of the return types of the signatures in that group.
const groups: Signature[][] = [];

callSignatures.forEach((callSignature) => {
const matchingGroup = find(groups, (group) => callSignaturesAreIdenticalExceptReturnType(callSignature, group[0]));
if (matchingGroup === undefined) {
groups.push([callSignature]);
}
else {
matchingGroup.push(callSignature);
}
});

return groups.map((signatures) => {
if (signatures.length === 1) {
return signatures[0];
}
const clone = cloneSignature(signatures[0]);
clone.resolvedReturnType = getIntersectionType(signatures.map(signature => getReturnTypeOfSignature(signature)));
return clone;
});
}

/**
* Converts an AnonymousType to a ResolvedType.
*/
Expand Down
13 changes: 5 additions & 8 deletions tests/baselines/reference/checkJsxGenericTagHasCorrectInferences.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
tests/cases/conformance/jsx/file.tsx(13,54): error TS2322: Type '(a: { x: string; }) => string' is not assignable to type '((a: { x: string; }) => string) & ((cur: { x: string; }) => { x: string; })'.
Type '(a: { x: string; }) => string' is not assignable to type '(cur: { x: string; }) => { x: string; }'.
Type 'string' is not assignable to type '{ x: string; }'.
tests/cases/conformance/jsx/file.tsx(13,71): error TS2322: Type 'string' is not assignable to type 'string & { x: string; }'.
Type 'string' is not assignable to type '{ x: string; }'.


==== tests/cases/conformance/jsx/file.tsx (1 errors) ====
Expand All @@ -17,8 +16,6 @@ tests/cases/conformance/jsx/file.tsx(13,54): error TS2322: Type '(a: { x: string
let b = <GenericComponent initialValues={12} nextValues={a => a} />; // No error - Values should be reinstantiated with `number` (since `object` is a default, not a constraint)
let c = <GenericComponent initialValues={{ x: "y" }} nextValues={a => ({ x: a.x })} />; // No Error
let d = <GenericComponent initialValues={{ x: "y" }} nextValues={a => a.x} />; // Error - `string` is not assignable to `{x: string}`
~~~~~~~~~~
!!! error TS2322: Type '(a: { x: string; }) => string' is not assignable to type '((a: { x: string; }) => string) & ((cur: { x: string; }) => { x: string; })'.
!!! error TS2322: Type '(a: { x: string; }) => string' is not assignable to type '(cur: { x: string; }) => { x: string; }'.
!!! error TS2322: Type 'string' is not assignable to type '{ x: string; }'.
!!! related TS6500 tests/cases/conformance/jsx/file.tsx:13:54: The expected type comes from property 'nextValues' which is declared here on type 'IntrinsicAttributes & IntrinsicClassAttributes<GenericComponent<{ initialValues: { x: string; }; nextValues: (a: { x: string; }) => string; }, { x: string; }>> & { initialValues: { x: string; }; nextValues: (a: { x: string; }) => string; } & BaseProps<{ x: string; }> & { children?: ReactNode; }'
~~~
!!! error TS2322: Type 'string' is not assignable to type 'string & { x: string; }'.
!!! error TS2322: Type 'string' is not assignable to type '{ x: string; }'.
61 changes: 61 additions & 0 deletions tests/baselines/reference/intersectionOfCallsWithSameParameters.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
tests/cases/compiler/intersectionOfCallsWithSameParameters.ts(38,7): error TS2741: Property 'two' is missing in type '{ one: number; }' but required in type '{ one: number; two: number; }'.
tests/cases/compiler/intersectionOfCallsWithSameParameters.ts(39,7): error TS2741: Property 'two' is missing in type '{ one: number; }' but required in type '{ one: number; two: number; }'.
tests/cases/compiler/intersectionOfCallsWithSameParameters.ts(40,7): error TS2741: Property 'two' is missing in type '{ one: number; }' but required in type '{ one: number; two: number; }'.
tests/cases/compiler/intersectionOfCallsWithSameParameters.ts(41,7): error TS2741: Property 'two' is missing in type '{ one: number; }' but required in type '{ one: number; two: number; }'.


==== tests/cases/compiler/intersectionOfCallsWithSameParameters.ts (4 errors) ====
interface One {
differentParameterType(id: string): { one: number };
differentNumberOfParameters(id: string): { one: number };
differentTypeParameterDefault<T = number>(id: string): { one: number };
differentTypeParameterConstraint<T extends { one: number }>(id: string): { one: number };

same1(id: string): { one: number };
same2<T>(id: string): { one: number };
same3<T extends { one: number }>(id: string): { one: number };
same4<T = number>(id: string): { one: number };
same5<T1 extends { one: number }, T2 = number>(id: string): { one: number };
}

interface Two {
differentParameterType(id: number): { two: number };
differentNumberOfParameters(id: string, second: string): { two: number };
differentTypeParameterDefault<T = string>(id: string): { two: number };
differentTypeParameterConstraint<T extends { two: number }>(id: string): { two: number };

same1(id: string): { two: number };
same2<T>(id: string): { two: number };
same3<T extends { one: number }>(id: string): { two: number };
same4<T = number>(id: string): { two: number };
same5<T1 extends { one: number }, T2 = number>(id: string): { two: number };
}

const i: One & Two = <any>{};

// These lines should type check; the return type should be intersected.
const same1: { one: number, two: number } = i.same1('test');
const same2: { one: number, two: number } = i.same2<number>('test');
const same3: { one: number, two: number } = i.same3<{ one:number }>('test');
const same4: { one: number, two: number } = i.same4('test');
const same5: { one: number, two: number } = i.same5<{ one:number }, string>('test');

// These lines should not, because the functions should become overloads rather
// than the return types intersected.
const differentParameterType: { one: number, two: number } = i.differentParameterType('test');
~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2741: Property 'two' is missing in type '{ one: number; }' but required in type '{ one: number; two: number; }'.
!!! related TS2728 tests/cases/compiler/intersectionOfCallsWithSameParameters.ts:38:46: 'two' is declared here.
const differentNumberOfParameters: { one: number, two: number } = i.differentNumberOfParameters('test');
~~~~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2741: Property 'two' is missing in type '{ one: number; }' but required in type '{ one: number; two: number; }'.
!!! related TS2728 tests/cases/compiler/intersectionOfCallsWithSameParameters.ts:39:51: 'two' is declared here.
const differentTypeParameterDefault: { one: number, two: number } = i.differentTypeParameterDefault('test');
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2741: Property 'two' is missing in type '{ one: number; }' but required in type '{ one: number; two: number; }'.
!!! related TS2728 tests/cases/compiler/intersectionOfCallsWithSameParameters.ts:40:53: 'two' is declared here.
const differentTypeParameterConstraint: { one: number, two: number } = i.differentTypeParameterConstraint<{ one: number }>('test');
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2741: Property 'two' is missing in type '{ one: number; }' but required in type '{ one: number; two: number; }'.
!!! related TS2728 tests/cases/compiler/intersectionOfCallsWithSameParameters.ts:41:56: 'two' is declared here.

59 changes: 59 additions & 0 deletions tests/baselines/reference/intersectionOfCallsWithSameParameters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
//// [intersectionOfCallsWithSameParameters.ts]
interface One {
differentParameterType(id: string): { one: number };
differentNumberOfParameters(id: string): { one: number };
differentTypeParameterDefault<T = number>(id: string): { one: number };
differentTypeParameterConstraint<T extends { one: number }>(id: string): { one: number };

same1(id: string): { one: number };
same2<T>(id: string): { one: number };
same3<T extends { one: number }>(id: string): { one: number };
same4<T = number>(id: string): { one: number };
same5<T1 extends { one: number }, T2 = number>(id: string): { one: number };
}

interface Two {
differentParameterType(id: number): { two: number };
differentNumberOfParameters(id: string, second: string): { two: number };
differentTypeParameterDefault<T = string>(id: string): { two: number };
differentTypeParameterConstraint<T extends { two: number }>(id: string): { two: number };

same1(id: string): { two: number };
same2<T>(id: string): { two: number };
same3<T extends { one: number }>(id: string): { two: number };
same4<T = number>(id: string): { two: number };
same5<T1 extends { one: number }, T2 = number>(id: string): { two: number };
}

const i: One & Two = <any>{};

// These lines should type check; the return type should be intersected.
const same1: { one: number, two: number } = i.same1('test');
const same2: { one: number, two: number } = i.same2<number>('test');
const same3: { one: number, two: number } = i.same3<{ one:number }>('test');
const same4: { one: number, two: number } = i.same4('test');
const same5: { one: number, two: number } = i.same5<{ one:number }, string>('test');

// These lines should not, because the functions should become overloads rather
// than the return types intersected.
const differentParameterType: { one: number, two: number } = i.differentParameterType('test');
const differentNumberOfParameters: { one: number, two: number } = i.differentNumberOfParameters('test');
const differentTypeParameterDefault: { one: number, two: number } = i.differentTypeParameterDefault('test');
const differentTypeParameterConstraint: { one: number, two: number } = i.differentTypeParameterConstraint<{ one: number }>('test');


//// [intersectionOfCallsWithSameParameters.js]
"use strict";
var i = {};
// These lines should type check; the return type should be intersected.
var same1 = i.same1('test');
var same2 = i.same2('test');
var same3 = i.same3('test');
var same4 = i.same4('test');
var same5 = i.same5('test');
// These lines should not, because the functions should become overloads rather
// than the return types intersected.
var differentParameterType = i.differentParameterType('test');
var differentNumberOfParameters = i.differentNumberOfParameters('test');
var differentTypeParameterDefault = i.differentTypeParameterDefault('test');
var differentTypeParameterConstraint = i.differentTypeParameterConstraint('test');
Loading