-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Add Awaited<T> to core library #21613
Conversation
This only solves the "Incorrect return types or errors with complex Promise chains" issue in #17077. This does not solve the "Incorrect eager resolution of the "awaited type" for a type variable" issue. |
There's a difference to |
src/lib/es5.d.ts
Outdated
|
||
/** Gets the type resulting from awaiting `T`. This does **not** recursively unwrap nested promises. */ | ||
declare type Awaited<T> = | ||
T extends Awaitable<Awaitable<infer U>> ? U : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we figure that two levels deep is enough for most uses, and if anyone complains, we can always add more later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if we currently don't do better than two levels, this will do for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are using the Promise
definition from our libs then you won't need anything deeper. Once this is shipping we can modify the definitions in DT to use it as well. Per my examples above: type T3 = Awaited<Promise<Promise<number>>>; // number
for any level of nesting using either Promise
or PromiseLike
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually only need:
declare type Awaited<T> =
T extends Awaitable<infer U> ? U :
T extends { then(...args: any[]): any; } ? never :
T;
I seemed to have forgotten to stage an additional change to es5.d.ts.
src/lib/es5.d.ts
Outdated
/** An object type that can be definitely be awaited. Do not inherit from this type. */ | ||
declare type Awaitable<T> = { then(onfulfilled: (value: T) => any): any; }; | ||
|
||
/** An object type that may not be awaitable. Do not inherit from this type. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this type isn't supposed to be extended/used, why not just inline the object type inside Awaited
, below, since it's only used one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've considered that, but this is more readable when getting Quick Info on Awaited<T>
. I don't even need Awaitable<T>
if I inline it in the 2-3 places its used but that again reduces readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed NonAwaitable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to also remove Awaitable<T>
and inline the definition into Awaited<T>
. The two other places where it was referenced were fine as PromiseLike<T>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It definitely seems better than the union with PromiseLike
most positions took before from before, and we can definitely improve it in the future if we can.
We should audit how this affects definitelytyped, though.
src/lib/es5.d.ts
Outdated
@@ -1273,7 +1273,20 @@ declare type PropertyDecorator = (target: Object, propertyKey: string | symbol) | |||
declare type MethodDecorator = <T>(target: Object, propertyKey: string | symbol, descriptor: TypedPropertyDescriptor<T>) => TypedPropertyDescriptor<T> | void; | |||
declare type ParameterDecorator = (target: Object, propertyKey: string | symbol, parameterIndex: number) => void; | |||
|
|||
declare type PromiseConstructorLike = new <T>(executor: (resolve: (value?: T | PromiseLike<T>) => void, reject: (reason?: any) => void) => void) => PromiseLike<T>; | |||
/** An object type that can be definitely be awaited. Do not inherit from this type. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"be definitely be"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/lib/es5.d.ts
Outdated
|
||
/** Gets the type resulting from awaiting `T`. This does **not** recursively unwrap nested promises. */ | ||
declare type Awaited<T> = | ||
T extends Awaitable<Awaitable<infer U>> ? U : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if we currently don't do better than two levels, this will do for now.
@ajafff I'll see if there's anything worth changing here. Despite the naming |
Sorry, this was incorrect. If the async function foo() {
let y1: { then(): any; };
let x1 = await y1; // Type of 'await' operand must either be a valid promise or must not contain a callable 'then' member.
let y2: { then(onfulfilled: () => any): any; };
let x2 = await y2; // never
} The best approximation of an error we can provide here is to resolve the type to |
Can you run this on DefinitelyTyped before checking in? Any change to inference is likely to disturb things there and it would be nice to know how big the break is. |
Please also run the RWC tests before submitting and ensure we have not introduced any regressions there either. |
and user tests. |
@mhegazy, I plan to run this against RWC and DefinitelyTyped today. If all goes well, should we discuss this in the design meeting before taking the change? |
sure. We should discuss the |
@mhegazy There are a few failing RWC tests after this that may actually be more correct. Most of them seem to be related to assignability, i.e.: |
An example of one of the RWC errors is this:
I'd expect @ahejlsberg, any suggestions? Is there something I can change in my definition to address this issue, or is there a change we can make in the checker that makes sense? Is this something that |
I got it to recursively unwrap under the present definition of type Awaited<T> = {
'0': T;
'1': Awaited<T extends { then(onfulfilled: (value: infer U) => any): any; } ? U : 'wat'>;
}[T extends { then(onfulfilled: (value: any) => any): any; } ? '1' : '0'];
let one: 1;
one = null! as Awaited<1>>; // ok
one = null! as Awaited<Promise<1>>>; // ok
one = null! as Awaited<Promise<Promise<1>>>>; // ok I'll admit it's clunky and silly -- I just used an object for the conditional to enable type recursion. But it works on master already, fwiw. |
@tycho01 @ahejlsberg on type Awaited<T> = {
'0': T;
'1': Awaited<T extends { then(onfulfilled: (value: infer U) => any): any; } ? U : never>;
}[T extends { then(onfulfilled: (value: any) => any): any; } ? '1' : '0'];
function foo<T>(x: T) {
let one: T;
let n1: Awaited<T>;
let n2: Awaited<Awaited<T>>;
let n3: Awaited<Awaited<Awaited<T>>>;
one = n1;
one = n2;
one = n3;
} spin forever. Looks like an infinite loop in |
@weswigham, I think that is the same issue as in #21611 |
Sorry, just remembered I'd yet to test for the original core issue, |
I played around a bit more to get the union cases to work properly. Conditional types already distributed over union types properly, so this came down to finding a way to make recursive types based on this, i.e. somehow wrapping the recursion so it'd allow it, using some useless no-op wrapper delaying execution to trick it. I found a silly approach that appears to work: Using that to unwrap promises or to flatten nested array types: // unwrap a Promise to obtain its return value
export type Awaited<T> = {
'1': T extends { then(onfulfilled: (value: infer U) => any): any; } ? Awaited<U> : T;
}[T extends number ? '1' : '1'];
// Awaited<Promise<1 | Promise<2>>> // -> 1 | 2
// flatten a structure of nested tuples/arrays into a flat array type
export type Flatten<T> = {
'1': Flatten<T extends Array<infer U> ? U : T>;
}[T extends number ? '1' : '1'];
// Flatten<Array<1 | Array<2>>> // -> 1 | 2 |
@tycho01 you rock my friend |
Sorry, so far I'd tested through the compiler API, but through |
Does not look like this is something we can proceed on at the time being.closing the PR, but leaving the branch for further investigations. |
This adds
Awaited<T>
as a mechanism to unwrap a type to its "awaited" type:Unlike the
await
expression,Awaited<T>
does not recursively unwrapT
as we do not support circular references in type aliases. It turns out recursive unwrap ofT
is mostly unnecessary if we modify the definition ofPromise
andPromiseLike
to return aPromise<Awaited<TResult>>
:This effectively flattens
Promise<Promise<number>>
as well as other nested definitions ofPromise
:The
Awaited<T>
type is a conditional type that provides the following conditions:T
has athen()
method with a callback, infer the callback's first argument asU
and use that as the result type.T
has athen()
method without a callback, this is a non-promise "thenable" that can never be resolved, so the result type isnever
.T
.The benefit of this approach is more effective type inference from the
onfulfilled
callback passed to thethen
method of aPromise
: