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

Help understanding type inference issue for minimal APIs - TypedResults / DynamicallyAccessedMembers #16890

Open
drk-mtr opened this issue Mar 18, 2024 · 11 comments
Labels
Area-Diagnostics mistakes and possible improvements to diagnostics Feature Improvement
Milestone

Comments

@drk-mtr
Copy link

drk-mtr commented Mar 18, 2024

I have created an F# web project using the minimal APIs approach by running dotnet new web -lang F# using dotnet 8.0.

I'm trying to get this working using the TypesResults class, such that Open API types can be derived without me specifying them using the Produces method.

This works fine for the following scenario. Note the usage of the Results type which is required to allow the correct return types for the indexHandler:

open Microsoft.AspNetCore.Builder
open Microsoft.Extensions.Hosting
open Microsoft.AspNetCore.Http
open Microsoft.AspNetCore.Http.HttpResults
open System.Threading.Tasks

type IndexHandlerResponse = Results<Ok<string>, ProblemHttpResult>

[<EntryPoint>]
let main args =
    let builder = WebApplication.CreateBuilder(args)
    let app = builder.Build()

    let indexHandler (): Task<IndexHandlerResponse> = 
        task {
            let maybe = Some "thing"

            if maybe.IsSome
            then return TypedResults.Ok<string>(maybe.Value)
            else return TypedResults.Problem("")
        }

    app.MapGet("/", Func<Task<IndexHandlerResponse>>(indexHandler)) |> ignore

However, as soon as I convert this to a real-life example where the maybe option is wrapped in a task which gets evaluated in the body of the handler, things stop working due to "Type constraint mismatch" errors:

type IndexHandlerResponse = Results<Ok<string>, ProblemHttpResult>

[<EntryPoint>]
let main args =
    let builder = WebApplication.CreateBuilder(args)
    let app = builder.Build()

    let maybeTask = task { return Some "Hello world!"}

    let indexHandler (): Task<IndexHandlerResponse> = 
        task {
            let! maybe = maybeTask

            if maybe.IsSome
            then return TypedResults.Ok<string>(maybe.Value)
            else return TypedResults.Problem("")
        }

    app.MapGet("/", Func<Task<IndexHandlerResponse>>(indexHandler)) |> ignore

Errors are as follows:

image

image

Is this expected behaviour, or am I doing something wrong here? Be good to understand whether there's a known workaround for this.

In terms of a workaround this is the best I can come up with at the moment:

    let maybeTask = task { return Some "Hello world!"}

    let indexHandler (): Task<IndexHandlerResponse> = 
        let maybe = maybeTask.Result
        task {
            if maybe.IsSome
            then return TypedResults.Ok<string>(maybe.Value)
            else return TypedResults.Problem("")
        }
@github-actions github-actions bot added this to the Backlog milestone Mar 18, 2024
@vzarytovskii
Copy link
Member

There's clearly something is wrong with TypedResults and inference, if you change it for plain F# result, it works and binds just fine:
image

@vzarytovskii
Copy link
Member

And bind (let!) is likely highlighted because of some issue with ranges of the error, when it can't match function return type and each condition branch:
image

@brianrourkeboll
Copy link
Contributor

I think it's just because TypedResults depends on op_Implicit, which doesn't get invoked automatically in F# across the two branches of your if/else expression.

Your example compiles if you do something like this:

open System
open System.Threading.Tasks
open Microsoft.AspNetCore.Builder
open Microsoft.AspNetCore.Http
open Microsoft.AspNetCore.Http.HttpResults

type IndexHandlerResponse = Results<Ok<string>, ProblemHttpResult>

let inline (~~) x = ((^a or ^b) : (static member op_Implicit : ^a -> ^b) x)

[<EntryPoint>]
let main args =
    let builder = WebApplication.CreateBuilder(args)
    let app = builder.Build()

    let maybeTask = task { return Some "Hello world!"}

    let indexHandler (): Task<IndexHandlerResponse> = 
        task {
            let! maybe = maybeTask

            if maybe.IsSome
            then return ~~TypedResults.Ok<string>(maybe.Value)
            else return ~~TypedResults.Problem("")
        }

    app.MapGet("/", Func<Task<IndexHandlerResponse>>(indexHandler)) |> ignore
    0

@vzarytovskii
Copy link
Member

vzarytovskii commented Mar 18, 2024

In other words, the real problem is that types Results<Ok<string>, ProblemHttpResult> don't match (don't implicitly cast into) TypedResults.Problem and TypedResults.Ok, they also both don't match each other.

The error message can get some improvements.

Update: Ah, @brianrourkeboll beat me to it.

@drk-mtr
Copy link
Author

drk-mtr commented Mar 18, 2024

I think it's just because TypedResults depends on op_Implicit, which doesn't get invoked automatically in F# across the two branches of your if/else expression.

I assumed this was what was going on. Your "double-tilde" workaround was exactly the sort of thing I was looking for but lacked the knowledge to define myself - thanks v much :)

What I didn't want to do was upcast to IResult, because that prevents Open API gen.

I think the "bug" here (if there is one) is around the unanticipated behaviour i.e. it works fine if the maybe option in this code is simply an option type, but not when it's a Task<string option> that gets evaluated to an option inside the task computation expression.

If anyone wants to suggest a more apt title for this I'll update the issue :)

@brianrourkeboll
Copy link
Contributor

I think the "bug" here (if there is one) is around the unanticipated behaviour i.e. it works fine if the maybe option in this code is simply an option type, but not when it's a Task<string option> that gets evaluated to an option inside the task computation expression.

If anyone wants to suggest a more apt title for this I'll update the issue :)

My guess is that the elaboration of the task expression into a state machine might be interfering with (happening before?) op_Implicit can be sought out to unify the conditional branches.

Compare the contents of the generated MoveNext() here and here (bearing in mind that SharpLab is using a pretty old compiler).

@abonie abonie added Area-Diagnostics mistakes and possible improvements to diagnostics Feature Improvement and removed Needs-Triage labels Mar 18, 2024
@abonie
Copy link
Member

abonie commented Mar 18, 2024

We should improve the error message here, but otherwise this is not a bug, @brianrourkeboll explained it well (thanks!)

@drk-mtr
Copy link
Author

drk-mtr commented Mar 18, 2024

We should improve the error message here, but otherwise this is not a bug, @brianrourkeboll explained it well (thanks!)

Why is it that the first scenario works as I'm expecting? I'm not protesting the idea that this isn't a bug, as I don't have the knowledge to make a claim either way, just interested to understand as it seems inconsistent to the lay person. Would the suggested guidance for people making a minimal API be to use the method brianrourkeboll suggested?

@brianrourkeboll
Copy link
Contributor

brianrourkeboll commented Mar 18, 2024

Why is it that the first scenario works as I'm expecting?

See https://github.com/fsharp/fslang-design/blob/main/FSharp-6.0/FS-1093-additional-conversions.md#detailed-design

let f () : A = if true then B() else C()

is elaborated to

let f () : A = if true then (B() :> A) else (C() :> A)

I would guess that the compiler automatically inserts calls to op_Implicit (analogous to :> in this example) in your first scenario.

In the second scenario, after the let! bind, it seems like the compiler has already begun shuffling things around into state machine code, so the fact that both branches have a type implicitly convertible to the target type is no longer apparent, and the automatic calls to op_Implicit are no longer inserted.

(Disclaimer: this is just a guess; I haven't looked at the compiler to see what it actually does.)

@smoothdeveloper
Copy link
Contributor

We should improve the error message here

What would it do? leverage the fact that the type defines op_Implicit for all branches, and guides the user to use it?

@vzarytovskii
Copy link
Member

We should improve the error message here

What would it do? leverage the fact that the type defines op_Implicit for all branches, and guides the user to use it?

This, as well as choose ranges better for state machines based builders. We probably don't want to include any binds when we can't infer return type

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Diagnostics mistakes and possible improvements to diagnostics Feature Improvement
Projects
Status: New
Development

No branches or pull requests

5 participants