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

Fixed contextual async return type with unions in context that contain a mix of promise and non-promise types #47683

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Feb 1, 2022

Please verify that:

  • There is an associated issue in the Backlog milestone (required)
  • Code is up-to-date with the main branch
  • You've successfully run gulp runtests locally
  • There are new or updated unit tests validating the change

This potentially fixes #47682

Commits:

  • first commit just add a test case to showcase the weird behavior
  • second commit fixes the thing in one-line change
  • third commit just refactors the code to avoid fallthrough and to make all paths more distinct and easier to follow (in my opinion), this is a subjective matter and I can revert this if needed

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Feb 1, 2022
const contextualAwaitedType = mapType(contextualReturnType, getAwaitedTypeNoAlias);
return contextualAwaitedType && getUnionType([contextualAwaitedType, createPromiseLikeType(contextualAwaitedType)]);
if (functionFlags & FunctionFlags.Async) {
const contextualTypeOfPromise = mapType(contextualReturnType, getAwaitedTypeOfPromise);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main and only change here is that for async non-generator functions I use mapType with getAwaitedTypeOfPromise. This ensures that I only unwrap promise types instead of unwrapping any types with Awaited<T>.

It IMHO conceptually makes sense because if we deal with unions we can only return a Promise type that includes Promise-wrapped types. If a member of a union is of a different type then async method just won't be able to match it as async always returns a Promise.

@sandersn sandersn added the For Backlog Bug PRs that fix a backlog bug label Feb 11, 2022
@typescript-bot typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Backlog Bug PRs that fix a backlog bug For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Feb 11, 2022
@sandersn sandersn added For Backlog Bug PRs that fix a backlog bug and removed For Milestone Bug PRs that fix a bug with a specific milestone labels Feb 11, 2022
@typescript-bot typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Backlog Bug PRs that fix a backlog bug labels Feb 11, 2022
@Andarist
Copy link
Contributor Author

Andarist commented Apr 4, 2022

@rbuckton friendly 🏓 would love to hear out your thoughts about this PR :)

@Azarattum
Copy link

@Andarist would this fix #48927?

@Andarist
Copy link
Contributor Author

Andarist commented May 3, 2022

@Azarattum I’ve checked and those are unrelated

@Andarist
Copy link
Contributor Author

Andarist commented Jul 3, 2022

@rbuckton friendly 🏓

@Andarist Andarist force-pushed the contextual-async-return-type-with-unions branch from 0ea6851 to f4b2ec7 Compare July 5, 2022 08:17
@sandersn
Copy link
Member

since #51196 supersedes this PR, do you want to close it?

@Andarist
Copy link
Contributor Author

Ye, I think that this other PR is a better one. Let's close this one.

@Andarist Andarist closed this Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Contextual generic return type of an async function includes union members from the context that it shouldn't
5 participants