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

Add support for F# Async<'T> as an awaitable type in minimal APIs #46898

Merged
merged 25 commits into from
Nov 27, 2023

Conversation

brianrourkeboll
Copy link
Contributor

@brianrourkeboll brianrourkeboll commented Feb 26, 2023

Fixes #46773.

  • Add support for F# Async<'T> as an awaitable type in minimal APIs endpoints using the same CoercedAwaitableInfo helper that is used to provide such support for controller action methods.
  • Remove extra boxing/unboxing from the ObjectMethodExecutorFSharpSupport implementation so that it can be used both for the existing controller action method scenario as well as for the minimal APIs endpoint scenario.
  • Add request delegate factory tests for F# Async<'T> wherever there were existing tests for Task<TResult> or ValueTask<TResult>.
  • Update the existing F# async coercion to turn Async<unit>1 into void-returning (non-generic) Task instead of Task<unit>.
  • Add conversion of Task<unit> to Task and ValueTask<unit> to ValueTask.
  • Add tests showing that the above conversions are supported for runtime request delegate creation but not by the compile-time source generator.

Footnotes

  1. https://learn.microsoft.com/en-us/dotnet/fsharp/language-reference/unit-type

* Use `CoercedAwaitableInfo` to convert `FSharp.Control.FSharpAsync<T>`
  values to `System.Threading.Tasks.Task<TResult>` when building request
  delegates and populate endpoint metadata accordingly.
* Add a comment to the request delegate source generator tests that
  explicitly calls out that F# async is not currently supported by the
  source generator.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 26, 2023
@ghost
Copy link

ghost commented Feb 26, 2023

Thanks for your PR, @brianrourkeboll. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@brianrourkeboll
Copy link
Contributor Author

@dotnet-policy-service agree

@mkArtakMSFT mkArtakMSFT added the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Mar 2, 2023
@ghost
Copy link

ghost commented Mar 10, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Mar 10, 2023
@brianrourkeboll
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 46898 in repo dotnet/aspnetcore

Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

There are also references to the AwaitableInfo API in the OpenAPI-related codepaths we have in OpenApiGenerator and EndpointMetadataApiDescription provider. Can we update those too for consistency across the different places?

@brianrourkeboll brianrourkeboll requested review from a team and brunolins16 as code owners March 19, 2023 19:35
@brianrourkeboll
Copy link
Contributor Author

brianrourkeboll commented Jun 12, 2023

@captainsafia @halter73 when trying to bring this branch up to date with main, it was not immediately obvious to me how best to make the changes in this PR work with the new, shared request delegate creation tests: while the runtime creation invocations pass in brianrourkeboll@13e9f30, the compile-time ones do not (as expected).

I think there are two options:

  1. I can split out the F# coercion tests and make them runtime-only (but I don't fully know what your plans are for RequestDelegateFactoryTests migration #47345 and how that might interact with them).
  2. I can just add compile-time coercion support along the lines of brianrourkeboll@413a22c. Though it is admittedly unlikely to see frequent use, it is relatively straightforward and noninvasive.

Do you have a preference?

@mitchdenny
Copy link
Member

It probably makes sense to make them runtime only.

@brianrourkeboll
Copy link
Contributor Author

All right, I added the runtime tests back to RequestDelegateFactoryTests.cs in 2111eb2. If there's a better place for me to put them, or if you'd rather I took out the (now-redundant) Task/ValueTask parts, let me know.

@danmoseley danmoseley added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jul 7, 2023
@brianrourkeboll
Copy link
Contributor Author

Let me know if there's anything else you need from me on this.

@captainsafia
Copy link
Member

/azp run

@ghost ghost removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Nov 17, 2023
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@ghost
Copy link

ghost commented Nov 25, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Nov 25, 2023
@captainsafia
Copy link
Member

/azp run

@ghost ghost removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Nov 27, 2023
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

I'll admit to having limited knowledge on the F# front here but the implementation seems sensible and doesn't introduce any breaking changes.

Copy link

Pipelines were unable to run due to time out waiting for the pull request to finish merging.

@captainsafia captainsafia merged commit 444aa9e into dotnet:main Nov 27, 2023
26 checks passed
@ghost ghost added this to the 9.0-preview1 milestone Nov 27, 2023
@brianrourkeboll
Copy link
Contributor Author

Thanks, @captainsafia!

@ghost
Copy link

ghost commented Nov 27, 2023

Hi @brianrourkeboll. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support F# Async<'T> as an awaitable type in minimal APIs
8 participants