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

Promises of different types should not be compatible #9953

Closed
ghost opened this issue Jul 26, 2016 · 6 comments
Closed

Promises of different types should not be compatible #9953

ghost opened this issue Jul 26, 2016 · 6 comments
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@ghost
Copy link

ghost commented Jul 26, 2016

TypeScript Version: nightly

Code

const x: Promise<string> = Promise.resolve("");
const y: Promise<number> = x;

Expected behavior:
Compiler error on line 2.

Actual behavior:
No error.

@yortus
Copy link
Contributor

yortus commented Jul 27, 2016

In the definition of Promise<T> in lib.d.ts, if you comment out the nullary overload of then, then you get the expected behaviour. I do not understand why, I was just trying to reproduce the behaviour with a simpler case and discovered that.

// in lib.d.ts:
interface Promise<T> {
   /*...*/
    then<TResult1, TResult2>(onfulfilled: (value: T) => TResult1 | PromiseLike<TResult1>, onrejected: (reason: any) => TResult2 | PromiseLike<TResult2>): Promise<TResult1 | TResult2>;
   /*...*/
    // then(): Promise<T>;     <===== comment this one out
   /*...*/
}


// in code.ts:
const x: Promise<string> = Promise.resolve("");
const y: Promise<number> = x; // ERROR: 'Promise<string> is not assignable to 'Promise<number>'

@jeffreymorlan
Copy link
Contributor

jeffreymorlan commented Jul 27, 2016

I came up with those overloads in #9193, but under closer examination they are actually subtly wrong. TypeScript's model of function arguments assumes that extra args beyond those declared in the signature are ignored - and therefore, a function signature is assignable to a function signature with more parameters. Although you can pass 0 or 1 arguments to Promise.then, it does not ignore its first and second arguments; we do not want this rule applying to it.

I think the right way to declare Promise.then is to make all the overloads 2-arg, with optional parameters of type null (technically, any non-function can be used to get the passthrough behavior, but passing anything other than null/undefined is almost certainly a mistake)

interface Promise<T> {
    then(onfulfilled?: null, onrejected?: null): Promise<T>;
    then<TResult>(onfulfilled: (value: T) => TResult | PromiseLike<TResult>, onrejected?: null): Promise<TResult>;
    // other overloads here
}

EDIT: This only fixes the problem when --strictNullChecks is on. Changing null to never doesn't help, but changing it to string does. Not sure why...

@OliverJAsh
Copy link
Contributor

Is this still an issue?

const commits: Promise<number> = Promise.resolve('foo') // no error!

@mhegazy
Copy link
Contributor

mhegazy commented Sep 29, 2016

thanks @OliverJAsh. this should be fixed after @rbuckton's latest change to the Promise definitions.

@mhegazy mhegazy closed this as completed Sep 29, 2016
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Sep 29, 2016
@mhegazy mhegazy added this to the TypeScript 2.1 milestone Sep 29, 2016
@ghost
Copy link
Author

ghost commented Oct 14, 2016

You can still do this:

const x: Promise<number | string> = Promise.resolve("")
const y: Promise<number> = p

Here's how that causes errors:

function f(f: () => Promise<{ x: number }>) {}
f(async () => !true ? { x: 1 } : "")

(not sure why the error happens for { x: number } but not for number.)

Meaning, an async function can return a value of the wrong type if it has multiple returns (and thus returns a union type, which is a subtype of the correct type.)

There's a fix:

interface Promise<T> { __promiseBrand: T }

This enforces the Promise type as if it already had the value, which is probably how people want to view it -- if T isn't assignable to U, Promise<T> shouldn't be assignable to Promise<U>. And it makes sense, because internally you could think of promises as having a _resolvedValue: T private member, which eventually gets written to.

@ghost ghost reopened this Oct 14, 2016
@mhegazy mhegazy modified the milestones: TypeScript 2.1, TypeScript 2.1.2 Oct 27, 2016
@mhegazy mhegazy removed the Fixed A PR has been merged for this issue label Oct 31, 2016
@ghost
Copy link
Author

ghost commented Apr 19, 2017

Fixed by #15104.

@ghost ghost closed this as completed Apr 19, 2017
@mhegazy mhegazy added the Bug A bug in TypeScript label Apr 19, 2017
@mhegazy mhegazy added this to the TypeScript 2.3.1 milestone Apr 19, 2017
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Apr 19, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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