Filter possible contextual return types from unions for async functions and generators #51196
Conversation
…ns and generators
typescript-bot
added
the
For Milestone Bug
src/compiler/checker.ts
Outdated
| if (functionFlags & FunctionFlags.Async) { | ||
| return !!getAwaitedTypeOfPromise(returnType); | ||
| } |
There was a problem hiding this comment.
This block supersedes my other attempt from a couple of months back at fixing #47682 . I think that the fix here is better but perhaps that older one might be still interesting for reviewers
src/compiler/checker.ts
Outdated
| return checkGeneratorInstantiationAssignabilityToReturnType(returnType, functionFlags, /*errorNode*/ undefined); | ||
| } | ||
| if (functionFlags & FunctionFlags.Async) { | ||
| return !!getAwaitedTypeOfPromise(returnType); |
There was a problem hiding this comment.
This is a isPromise check based on the existing one in the codebase:
https://github.dev/microsoft/TypeScript/blob/98c19cbbbe83c2ae3c89a4e08317a4b9ccbcb206/src/compiler/checker.ts#L38691
| @@ -5,7 +5,7 @@ type OrGen = () => (number | Generator<string, (arg: number) => void, undefined> | |||
|
|
|||
| const g: OrGen = function* () { | |||
| >g : OrGen | |||
| >function* () { return (num) => console.log(num);} : () => Generator<never, (num: number) => void, unknown> | |||
| >function* () { return (num) => console.log(num);} : () => Generator<never, (num: number) => void, undefined> | |||
There was a problem hiding this comment.
All 3 affected baselines are directly related to the change and they are improvements/fixes
…-type-asyncs-and-generators
…-type-asyncs-and-generators
|
@jakebailey what would you say about being nerd-sniped into reviewing this one? 😅 |
|
I can at least be nerd sniped into running the tests. @typescript-bot test this |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at dfe955a. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at dfe955a. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at dfe955a. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at dfe955a. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at dfe955a. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at dfe955a. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
I'll take it, thanks! |
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
@jakebailey Here they are:Comparison Report - main..51196
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @jakebailey, it looks like the DT test run failed. Please check the log for more details. |
|
Yay npm timeouts @typescript-bot run dt |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at dfe955a. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
Hey @jakebailey, the results of running the DT tests are ready. |
|
Thanks for that test run! It seems that all of the reported failures might be related to promise-likes/thenables, I will dig into this. I prepared minimal repro cases for all 3: TS playground |
src/compiler/checker.ts
Outdated
| if (returnType && returnType.flags & TypeFlags.Union) { | ||
| returnType = filterType(returnType, t => checkGeneratorInstantiationAssignabilityToReturnType(t, functionFlags, /*errorNode*/ undefined)); | ||
| } |
There was a problem hiding this comment.
Is this necessary? I think it makes sense to be more forgiving for contextual types, but if you write:
function * f(): Generator<A, B, C> | string {}then you'll probably have other issues with assignability. I'm not sure we should filter the return type of an explicit type annotation.
Is this to support #41428? If so, I'm not sure we should try to handle a case of
function * f(): Generator<A> | AsyncGenerator<A> {}since a normal generator is incapable of producing an async generator.
There was a problem hiding this comment.
Is this to support #41428? If so, I'm not sure we should try to handle a case of
Yes, it was added in this commit: "Filter annotated return type within checkYieldExpression"
since a normal generator is incapable of producing an async generator.
That is true, I just assumed that this might be wanted to handle mixed unions. The function* is just sugar and with regular functions, we are free to assign wider return types than what is required to be covered. So, from my PoV, this was meant as a convenience - it's more about type reusability than about actually wanting to write a union like this directly as the return type annotation, usually it would come from an alias.
Andarist
force-pushed
the
fix/contextual-return-type-asyncs-and-generators
branch
from
1b98fac to
3f1ee89
Compare
| thisArg?: any | ||
| ): void; | ||
|
|
||
| registerCommand("_references-view.showHistoryItem", async (item) => { // Error, contextual return type of Promise<unknown> |
There was a problem hiding this comment.
When I created this PR, this code wasn't erroring. My initial changes created a regression with this code and it started to error (problem caught by the extended test suite). However, right now it is supposed to error as per the change here. I decided to add this as an extra regression test.
src/compiler/checker.ts
Outdated
| const functionFlags = getFunctionFlags(functionDecl); | ||
| if (functionFlags & FunctionFlags.Generator) { | ||
| return filterType(returnType, t => { | ||
| return !!(t.flags & (TypeFlags.AnyOrUnknown | TypeFlags.Void | TypeFlags.InstantiableNonPrimitive)) || checkGeneratorInstantiationAssignabilityToReturnType(t, functionFlags, /*errorNode*/ undefined); |
There was a problem hiding this comment.
I'm not super fond of this check here (talking mainly about TypeFlags.InstantiableNonPrimitive check, the rest is OK).
I analyzed all of the core logic behind this change and I think it's sound. This particular check might be slightly off though. In addition to that, I think that this logic could be improved further for instantiable types but I don't know how to do it right now. I think that this PR is an improvement already though and that it could be merged without fixing it right now.
I only have a single idea to fix this: create copies of instantiable types here with the same~ filtering logic applied to their constraints (perhaps the filtering could stay deferred so we wouldn't have to compute it until requested). I'm not sure how feasible this is and if it's the right approach so I'm not even taking a stab at this right now.
The overall idea behind this part of the fix is that we want to reject contextual types that are guaranteed to not match the given function's criteria (non-promises/non-generators). We also need to keep the types that poison the type (like any/unknown/void).
We need to do this here, for example, because getContextualTypeForReturnExpression uses the result of this function (getContextualReturnType). Let's consider this example:
declare function load(): Promise<boolean>;
type LoadCallback = () => Promise<boolean> | string;
const cb1: LoadCallback = async () => load().then(m => m);In a case like this, we shouldn't allow string to be used as the contextual type for this return expression (of the inner arrow function). If we don't reject it soon enough then we end up with Promise<string | boolean> as the result of getContextualTypeForReturnExpression and thus we end up with an assignability problem as that's not assignable to Promise<boolean>.
Initially, I was trying to fix this by filtering in the getContextualTypeForReturnExpression here but I found out that filtering in getContextualReturnType gives better results for some of the other added test cases in this PR here.
…-type-asyncs-and-generators # Conflicts: # tests/baselines/reference/noImplicitReturnsExclusions.errors.txt
…-type-asyncs-and-generators # Conflicts: # src/compiler/checker.ts
|
@rbuckton friendly 🏓 |
…-type-asyncs-and-generators
fixes #51187
fixes #47682
fixes #41428
supersedes #47683
fixes #42439