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

Generic parameter inference for Promise.then oddity #11022

Closed
rkirov opened this issue Sep 20, 2016 · 2 comments
Closed

Generic parameter inference for Promise.then oddity #11022

rkirov opened this issue Sep 20, 2016 · 2 comments
Assignees
Labels
Breaking Change Would introduce errors in existing code Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@rkirov
Copy link
Contributor

rkirov commented Sep 20, 2016

TypeScript Version: 1.8.0 / nightly (2.0.5-dev.20160919)

Code

interface Thenable<T> {
  then<U>(then: (value: T) => U | Thenable<U>): Thenable<U>;
}

declare let x: Thenable<string>;
declare function f(): Thenable<string|null>|null;
declare function g(): Thenable<string>;

let y = x.then((_) => {
  if (Math.random() < 0.5) {
    return f();
  } else {
    return g();
  }
});

Expected behavior:
U should be inferred to be string | null as that solves the type constraints. It can be verified by explicitly adding then<string | null>.(...) which type checks.

Actual behavior:
TS errors with Argument of type '(_: string) => Thenable<string> | null' is not assignable to parameter of type '(value: string) => string | Thenable<string>'.

Oddities:
The following changes make the error disappear, while I expect them to have no relevance to the type of the closure:

  • moving line 5 to line 8 - declare let x below declare function f and g.
  • change of the type of x to Thenable<number>.
  • add a field of type T to Thenable - a: T, that would make Thenable strictly covariant.
  • replace with ternary expression - (_) => (Math.random() < 0.5) ? f() : g()
@ahejlsberg
Copy link
Member

The core issue here is that Thenable<string> and Thenable<string | null> are both subtypes of each other because within Thenable<T>, T is only used in a bi-variant position (i.e. a parameter position). When two distinct types are both subtypes of each other and the compiler performs subtype reduction (as it does when inferring a return type from multiple return statements), the compiler picks "the first" of the types based on an internal ordering that appears somewhat arbitrary to the external observer. That's why seemingly irrelevant changes in the ordering of declarations produces different outcomes.

This behavior is obviously not desirable. We'll think about possible ways we can introduce tie breaker rules to disambiguate in cases like this.

@rkirov
Copy link
Contributor Author

rkirov commented Sep 21, 2016

Ah, that makes total sense, and explains why moving type declarations have effects on the type inference.

Apart from changing the choices in the subtype reduction, I wonder if there is a way to have the algorithm picking U to consider U = string | null even if the type of passed call back is explicitly written as Thenable<string> | null. At least conceptually, such U is a valid solution because string | null | Thenable<string | null> is a supertype of Thenable<string> | null.

@ahejlsberg ahejlsberg self-assigned this Apr 10, 2017
@ahejlsberg ahejlsberg added Design Limitation Constraints of the existing architecture prevent this from being fixed Fixed A PR has been merged for this issue labels Apr 10, 2017
@mhegazy mhegazy added Bug A bug in TypeScript and removed Design Limitation Constraints of the existing architecture prevent this from being fixed labels Apr 18, 2017
@mhegazy mhegazy added this to the TypeScript 2.3.1 milestone Apr 18, 2017
@yuit yuit added the Breaking Change Would introduce errors in existing code label Jun 7, 2017
@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 Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

4 participants