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.

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

BC break for method call on union type #10025

Closed
Strate opened this issue Jul 29, 2016 · 12 comments
Closed

BC break for method call on union type #10025

Strate opened this issue Jul 29, 2016 · 12 comments
Labels
Breaking Change Would introduce errors in existing code

Comments

@Strate
Copy link

Strate commented Jul 29, 2016

TypeScript Version: 2.1.0-dev.20160729

Code

interface A {
  focus: (force: boolean) => void
}
interface B {
  focus: () => void
}
let instance: A | B
instance.focus(true);

Expected behavior:
No compilation error at least at 2.1.0-dev.20160716

Actual behavior:
TS2349: Cannot invoke an expression whose type lacks a call signature.

@Strate
Copy link
Author

Strate commented Jul 29, 2016

Maybe similar case, also broken between 2.1.0-dev.20160716 and 2.1.0-dev.20160729:

interface List<T> {
    map<U>(mapper: (v: T, index: number) => U): List<U>
}
interface A {
    prop1: boolean
}
interface B extends A {
    prop2: number
}
type ListA = List<A>;
type ListB = List<B>;
let instance: List<A> | List<B>;
instance.map((v, index) => {
    return true;
})

@Strate Strate changed the title BC break for call object's method with possible optional argument BC break for call method on union type Jul 29, 2016
@Strate Strate changed the title BC break for call method on union type BC break for method call on union type Jul 29, 2016
@RyanCavanaugh
Copy link
Member

@ahejlsberg I believe this is due to the less-aggressive subtype reduction -- thoughts?

@ahejlsberg
Copy link
Member

@Strate I'm curious what the scenario is for having types like List<A> | List<B> when B is a subtype of A. You could equally well just write List<A> (and the issue would then go away). Similarly, you could just use type A instead of A | B in the first example.

@RyanCavanaugh Yes, this is caused by a change in #9407 to not perform subtype reduction in property accesses on objects of union types. Subtype reduction is horribly expensive on large union types and it is not clear we could ever make it perform acceptably. Also, subtype reduction only helps in those cases where we can reduce a union of function types down to a single function type.

The core issue really is that we are very conservative when it comes to call operations on a union type. We basically require all of the parameter types to be identical, with a small allowance matching omitted parameters to optional parameters.

The real fix here would be to find a better way to check calls on unions of function types. Subtype reduction isn't the solution because there are lots of cases where we have a union of function types that aren't subtypes of each other, but where there is a meaningful way to construct calls that would satisfy all of the signatures.

One possibility might be to intersect the types of parameters in corresponding positions. So, for a union of function signatures (() => T) | ((x: A) => U) | ((x: B, y: C) => V) we would produce a synthetic signature of type ((x: A & B, y: C) => T | U | V).

@RyanCavanaugh RyanCavanaugh added the Breaking Change Would introduce errors in existing code label Jul 29, 2016
@Strate
Copy link
Author

Strate commented Jul 30, 2016

@ahejlsberg seems that your suggestion could solve this issue:

type a = string;
type b = number;
interface List<T> {
    map<U>(mapper: (item: T) => U): List<U>
}
let instance: List<a> | List<b>
instance.map(v => true)

call to map produces an error "TS2349: Cannot invoke an expression whose type lacks a call signature." and "TS7006: Parameter 'v' implicitly has an 'any' type.". Second one could be fixed by explicit add a|b type for v parameter, but first one fixable only by typecasting instance to List<a|b>

@RyanCavanaugh
Copy link
Member

#10041 is a good example of how this can be problematic in the absence of a meaningless union type

@ahejlsberg
Copy link
Member

@Strate Yes, but as I think about it more, the idea of intersecting the parameter types still wouldn't work for overloaded signatures (as it wouldn't be clear which signatures to match with other signatures).

Still curious whether this shows up in a real world scenario for you, and if so, what leads to the existence of union types where one constituent is a subtype of the other?

@Strate
Copy link
Author

Strate commented Jul 30, 2016

It seems that there is no difference between

interface A { prop1: boolean }
interface B extends A { prop2: boolean }

and

interface A { prop1: boolean }
interface B { prop1: boolean, prop2: boolean }

from structural type system view.
First case is just to reduce copy-paste props from A to B.
And there is definetely big difference between Array<A> | Array<B> and Array<A|B> and, of course, Array<A>.

What about real-world scenario, I can't formalize it now, maybe our sode really could be refactored to your suggestion :)

@zpdDG4gta8XKpMCd
Copy link

I had a similar break in my code:

type Rule = LocalRule | GlobalRule;
interface LocalRule { enforce: () => void; }
interface GlobalRule { enforce: (state: GlobalState) => void; }
declare rules: Rule [];
rules.forEach (rule => rule.enforce (state)); // used to work, not anymore

@zpdDG4gta8XKpMCd
Copy link

To be frank, the only reason I wrote it this way is because of my laziness, I didn't want to pattern match to a certain case as long as the unified signature would work as well.

@yortus
Copy link
Contributor

yortus commented Jul 31, 2016

@Aleksey-Bykov laziness was smart in this case since pattern matching with LocalRule|GlobalRule also has a subtype reduction problem:

rules.forEach(rule => {
    if (isGlobalRule(rule)) {
        rule.enforce(state);
    }
    else {
        rule.enforce(); // ERROR: Property 'enforce' does not exist on type 'never'.
    }
});

@zpdDG4gta8XKpMCd
Copy link

@yortus the way i fixed is by adding a kind property to both cases that i used for discrimination, without such property i see how it can be a problem, good catch

@0815fox
Copy link

0815fox commented Oct 25, 2016

I agree with @ahejlsberg s comment:

One possibility might be to intersect the types of parameters in corresponding positions.

Please also see my comment in 9104 (which unlike this ticket is "open"):
#9104 (comment)

@Strate Yes, but as I think about it more, the idea of intersecting the parameter types still wouldn't work for overloaded signatures (as it wouldn't be clear which signatures to match with other signatures).

On my opinion overloaded methods can be handled as well:

interface I1 {
  foo(x:T1_1):R1_1;
  foo(x:T1_2):R1_2;
}
interface I2 {
  foo(x:T2_1):R2_1;
  foo(x:T2_2):R2_2;
}

type X = I1|I2;
//should become:
type X = {
  foo(X:(T1_1|T1_2)&(T2_1|T2_2)):R1_1|R1_2|R2_1|R2_2;
}

Of course it cannot "magically generate" new overloaded methods, but it can generate a function type, which allows for accurate compile time type checking.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Breaking Change Would introduce errors in existing code
Projects
None yet
Development

No branches or pull requests

7 participants