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

3.7-beta regression: Promise.all wrongly adds undefined type #33752

Closed
Swatinem opened this issue Oct 2, 2019 · 25 comments · Fixed by #34501 or #45350
Closed

3.7-beta regression: Promise.all wrongly adds undefined type #33752

Swatinem opened this issue Oct 2, 2019 · 25 comments · Fixed by #34501 or #45350
Assignees
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Fix Available A PR has been opened for this issue

Comments

@Swatinem
Copy link
Contributor

Swatinem commented Oct 2, 2019

TypeScript Version: 3.7.0-beta

Search Terms:

promise all

Code

interface A {
  a: string
}
interface B {
  b: string
}
async function main(): Promise<A> {
  const [a, b] = await Promise.all([
    Promise.resolve({a: "a"} as A),
    Promise.resolve({b: "b"} as B | undefined), // <- the `| undefined` here is the problem
  ]);
  return a; // <- a has type `A | undefined`, but should be `A`
}

Expected behavior:

This is a regression from 3.6.3, where the return tuple of Promise.all is correctly inferred.

Actual behavior:

A | undefined bound of a different tuple member also adds a | undefined bound on a different unrelated return value of Promise.all.

Playground Link:

https://www.typescriptlang.org/play/?ts=3.7-Beta#code/JYOwLgpgTgZghgYwgAgILIN4ChnLgLmQGcwpQBzLAXy1ElkRQCFMdkAjQkskSmuIgE8QCZDACuIsMAD2IZAFs4oABQBKQgAUoMhcCIQAPKgB8rXAjklkAbTgAaDgF1kAXjwB3ZWGTbd+iAA6OAAbEJUbNlw-PQNAqAgiGRCANwgVDAJkACI4bKo8IjQ1eyjfHVighKTU9IxOHPZ8wuQWAB9kSQATCBhQCC6S5AB6YeRDAFpkMAALFAADDu7e-q755DmE5H1pueQABx12EIgFNic1AG42BLBxKHk4S5GxybwNgWnBfYX0JZAen0QAN5o52OIfEQZjJxCEuhxfvNqEA

Related Issues:

Maybe #33707 ?

@jack-williams
Copy link
Collaborator

I think it's caused by #33057 which is merged; #33707 is not merged so will not be in the beta.

@Swatinem
Copy link
Contributor Author

Swatinem commented Oct 2, 2019

Hm, but maybe #33707 will resolve this regression? That’s what I thought, haven’t checked though.

@jack-williams
Copy link
Collaborator

Reduced repo:

interface A {
  a: string
}
interface B {
  b: string
}

interface Foo {
  all<T1, T2>(values:  readonly [T1 | PromiseLike<T1>, T2 | PromiseLike<T2>]): Promise<[T1, T2]>;
}

declare const Foo: Foo;

function main() {
  let aValue: A;
  Foo.all([Promise.resolve({a: "a"} as A), Promise.resolve({b: "b"} as B | undefined)]).then(([a,b]) => aValue = a);
}

Error on assignment to aValue; removing readonly removes the error.

@jack-williams
Copy link
Collaborator

Yes, the typings in #33707 fix the smaller repro above.

Zarel referenced this issue in smogon/pokemon-showdown Oct 3, 2019
@RyanCavanaugh RyanCavanaugh added Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript labels Oct 17, 2019
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Oct 17, 2019
@eamodio
Copy link

eamodio commented Oct 24, 2019

Here is another case of this: #34554

@buu700
Copy link

buu700 commented Nov 10, 2019

sed -i 's|: readonly|:|g' node_modules/typescript/lib/lib.es2015.promise.d.ts seems to be an effective temporary workaround until this is fixed.

@scamden
Copy link

scamden commented Nov 23, 2019

@buu700 how do you recommend using that work around? running that command prior to compile every time?

@RyanCavanaugh this seems like a pretty major regression, is there any timeline here? I see this immediately on upgrading all around my code base.

@buu700
Copy link

buu700 commented Nov 23, 2019

You could use the attached patch with patch-package, or possibly run that sed command (or a JS equivalent) in a postinstall script.

typescript+3.7.2.patch

@scamden
Copy link

scamden commented Nov 23, 2019

@buu700 this is amazing. thank you so much for the suggestion extremely helpful!

@skylerjokiel
Copy link

I'm seeing a similar situation in Promise.all in 3.7.2 but types are being overwritten.

Ex. in the example below r1 should be of type boolean and r2 should be of type unknown but actually they are both of type unknown.

        const p1: Promise<boolean> = new Promise((resolve) => { resolve(true); });
        const p2: Promise<unknown> = new Promise((resolve) => { resolve(); });
        const [r1, r2] = await Promise.all([p1, p2]);

If p2:Primose<undefined> then r1 and r2 are of type boolean | undefined

Playground: https://www.typescriptlang.org/play/index.html#code/IYZwngdgxgBAZgV2gFwJYHsIwLbFRACgEoYBvAKBhikxGRgAcBGALhgAUAndbVEAUwA8AI3ToANv2AQAfDAC8MCPwDuHbrwEECnfiAkA3fiXlzSMXfvFGCyTgmMBuGAF8ijytVr0GAJjZcPHxCSADWEOgqsgpKqupBWjp6hsYKZhbJ1vzEzm4eVDQQdDAA2pxMADQWvgC6McAqePSBmvwAdMDi4gQlzFV+Ne7kLuTkoJCwiCgYWLj4vsRknoXFzAEawSJiktJyispqLcHalikm6adZtvZOrkMF3oz+8a2CSAAm-HD4-O97sYcNolLkZzmQMlYbO47vkvEV6GVKtU6ooGk0XsEOl0en0noMPCMxuBoPAkFA0JgcHgIABmRYUB7wxisDECLYSKTRfZxI7AzKgtLgkHZOwOaF5ZaPPzrBJCCAILr-A6s7JJSGpUxC-nZcX3OHFRFVTi1eqNVDNIHtTrdXpIgZDFxAA

imhoffd added a commit to ionic-team/ionic-cli that referenced this issue Dec 5, 2019
@JasonKleban
Copy link

Is this not a real issue? I'm getting it, and it seems to me that it is generally pretty disruptive, but I'm not seeing enough MS chatter about it to confirm that it's not just a legit new type error that I don't understand.

@nickzelei
Copy link

@JasonKleban This is a legit regression. Just updated a codebase and am seeing this.

Easily reproducible:

const [num1, num2] = await Promise.all([Promise.resolve(1), Promise.resolve(null)]);
Causes the type for num1 to be number | null.

dhleong added a commit to dhleong/shougun that referenced this issue Dec 14, 2019
Note that Typescript 3.7 seems to have a type inference bug that makes
Promise.all() use annoying/breaks existing code compilation (see
microsoft/TypeScript#33752, for example)
so we're sticking with 3.6 for now
@anton-bot
Copy link

anton-bot commented Dec 18, 2019

still reproducible in 3.7.3 and nightly

@jmalins
Copy link

jmalins commented Dec 18, 2019

We are having this issue in 3.7.3 as well. It is blocking our upgrade off of 3.6.x (hundreds of occurrences across the code base).

@thomas-darling
Copy link

@RyanCavanaugh What's the timeline for getting this fixed?
It's been around for a while now, and it's a pretty major regression, blocking us from upgrading...

@RyanCavanaugh RyanCavanaugh removed this from the Backlog milestone Jan 10, 2020
@buu700
Copy link

buu700 commented Feb 6, 2020

sed -i 's|: readonly|:|g' node_modules/typescript/lib/lib.es2015.promise.d.ts seems to be an effective temporary workaround until this is fixed.

Is there any reason my fix from November (quoted above) can't or shouldn't be used?

I don't mind continuing to patch TypeScript in my own projects as a temporary workaround, but it just seems odd that such a high-impact regression with a simple quick fix available is still an issue three months later.

@Swatinem
Copy link
Contributor Author

Swatinem commented Feb 7, 2020

tbh, I’m a bit disappointed by the lack of communication here. I recently read the already month old Design notes here: #36138 which mention ideas to create a dedicated awaited or promised type, which would maybe solve this, but is probably a very invasive change.
However, this is just speculation on my part. Some official clarification would be nice.

@joseluisq
Copy link

v3.7.5 02.20 and I still facing the same issue on PromiseLike types via lib.es5.d.ts

@rbuckton
Copy link
Member

Assuming we merge the awaited type PR, I plan to take a renewed look at all of the Promise-related issues in light of that change.

Shesekino added a commit to snyk/snyk-docker-plugin that referenced this issue Feb 19, 2020
to slightly clarify the API between this library and its users, add a bit of types where easy.
many of the types I've added rely on some "any"s because I'm trying to provide some initial value to make future work easier.

a single place uses the non-null assertion operator: apparently there's an open bug in typescript where Promise.all() wrongly adds " | undefined"
microsoft/TypeScript#33752
we'll try to keep an eye open here to see when it can be removed.
Shesekino added a commit to snyk/snyk-docker-plugin that referenced this issue Feb 20, 2020
to slightly clarify the API between this library and its users, add a bit of types where easy.
many of the types I've added rely on some "any"s because I'm trying to provide some initial value to make future work easier.

a single place uses the non-null assertion operator: apparently there's an open bug in typescript where Promise.all() wrongly adds " | undefined"
microsoft/TypeScript#33752
we'll try to keep an eye open here to see when it can be removed.
@phiresky
Copy link

phiresky commented Feb 29, 2020

This issue can be worked around by adding this somewhere:

declare global {
	interface PromiseConstructor {
		all<T extends readonly any[] | readonly [any]>(values: T): Promise<{ -readonly [P in keyof T]: Awaited<T[P]> }>;
	}
}

but I'd be really happy having this fixed, my devs are using this as an excuse to plaster Bluebird everywhere instead of using native promises.

Still exists in 3.9.0-dev.20200229

@anton-bot
Copy link

We anticipate that our next version, TypeScript 3.9, will be coming mid-May of 2020, and will mostly focus on performance, polish, and potentially smarter type-checking for Promises.

Hopefully this issue will be fixed in 3.9

@anton-bot
Copy link

Seems this example still does not work on nightly.

(async () => {
    const [num1, num2] = await Promise.all([Promise.resolve(1), Promise.resolve(null)]);
    num1.toString();
})

Is it really fixed?

@nickzelei
Copy link

Just checked on 3.9.0-dev.20200317 and it appears to be fixed. woohoo!

@anton-bot
Copy link

@DanielRosenwasser
Copy link
Member

The playground seems to have a lot of weirdness there, specifically with Promise.all. Things like signature help don't even seem to work correctly. I'm not sure exactly what the problem is, but if you do try that code out locally with a nightly version of TypeScript, it should all work fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment