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

>=1.3.0 breaks type inference of unwrapped types #88

Closed
piaste opened this issue Jun 24, 2020 · 17 comments
Closed

>=1.3.0 breaks type inference of unwrapped types #88

piaste opened this issue Jun 24, 2020 · 17 comments

Comments

@piaste
Copy link

piaste commented Jun 24, 2020

Thanks again for fixing #86. After updating to 1.4.1, however, I still get errors from code which compiled with 1.2.6.

Here's a minimal repro:

type X() = 
   member _.A = 1

let f () = asyncResult {
    let! v = async { return X() }
    return v.A
}

Under 1.2.6, this compiles. Under 1.4.1, the compiler can't figure out the type of v' and errors out with a "lookup of indeterminate object type" on v'.A

Note that if you replace X() with a record, the code may compile because F# will infer types from members of records, but not from members of classes and anonymous records.

Possible workaround: unwrap v inside a regular async CE instead:

let f () = asyncResult {
    let v = async { return X() }
    return! async { let! v' = v in return v'.A }
}    

I'll still need to roll back to 1.2.6 though because we have this pattern every single time we make a database query:

asyncResult {
    use cmd = Db.Query(connStr)
    let! records = cmd.AsyncExecute(args)
    // do stuff with records
}
@piaste piaste changed the title 1.3 breaks type inference of unwrapped types >=1.3.0 breaks type inference of unwrapped types Jun 24, 2020
@TheAngryByrd
Copy link
Collaborator

TheAngryByrd commented Jun 24, 2020

Ugh this feels like a potential compiler bug. cc @cartermp @baronfel. This seems to only happen when using the Source members.


A less verbose work around is to add a type annotation to v

let f () = asyncResult {
    let! (v : X) = async { return X() }
    return v.A
}

Additionally this problem is not apparent when using the <LangVersion>preview</LangVersion> in the fsproj.

@TheAngryByrd
Copy link
Collaborator

With all these breaking changes (#84 #85) I should probably unlist these newest packages and bump this package to v2.

@baronfel
Copy link

Preview opts you into applicative support, are you thinking there's an interaction there with typechecking?

@TheAngryByrd
Copy link
Collaborator

TheAngryByrd commented Jun 24, 2020

If I expand the example to:

let f () = asyncResult {
  let! v = async { return X() }
  let! x = async { return X() }
  let! y = async { return X() }
  return v.A, x.A, y.A
}

This no longer works because it's not in applicative mode. So there's an issue with type inference with regards to Source members and Classes/Anonymous Records

@cartermp
Copy link

This looks like a compiler regression given that it doesn't repro unless you turn on preview, which actives applicatives and the associating typechecking.

@baronfel
Copy link

That's not how I read it, @cartermp. In #88 (comment) @TheAngryByrd said that turning on preview fixed the initial report, which suggest to me that the rewrite to BindReturn that happened works as expected. The first example is a pretty classic BindReturn scenario.

The example in #88 (comment) isn't something that's easily transformable to BindReturn, etc, so it surfaces the original issue even in an environment with langversion:preview enabled. I've replicated this behavior in a sample project for verification and I'm certain I don't have preview enabled.

@cartermp
Copy link

Oh, derp. I misread

@baronfel
Copy link

here's the code I'm tinkering around with to show the difference between how classes are resolved vs how records are resolved:

[<Sealed>]
type X() = 
   member _.A = 1

type Y = { A : int }

let f () = asyncResult {
  let! a = async { return X() }
  let! b = async { return X() }
  let! c = async { return X() }
  return a.A, b.A, c.A
}

let g () = asyncResult {
  let! a = async { return { A = 1 } }
  let! b = async { return { A = 1 } }
  let! c = async { return { A = 1 } }
  return a.A, b.A, c.A
}

each of the let! in the f() are throwing the following error:

error FS0041: No overloads match for method 'Bind'.

Known types of arguments: Async<Result<X,'a>> * (Y -> Async<Result<(int * int * int),'b>>)

Available overloads:
 - member AsyncResultBuilder.Bind : asyncComputation:Async<'T> * binder:('T -> Async<Result<'U,'TError>>) -> Async<Result<'U,'TError>> // Argument 'binder' doesn't match
 - member AsyncResultBuilder.Bind : asyncComputation:System.Threading.Tasks.Task<'T> * binder:('T -> Async<Result<'U,'TError>>) -> Async<Result<'U,'TError>> // Argument 'asyncComputation' doesn't match
 - member AsyncResultBuilder.Bind : asyncResult:Async<Result<'T,'TError>> * binder:('T -> Async<Result<'U,'TError>>) -> Async<Result<'U,'TError>> // Argument 'binder' doesn't match

which to me looks like we're definitely mis-computing the type of the binder argument for classes somehow?

@baronfel
Copy link

more findings:

@TheAngryByrd and I added some logging to the CE typecheck pass and went through an exercise of manually translating the logged SynExprs we saw into the actual builder calls.

Final Delay call emitted by the compiler
SynExpr.Lambda
   (false, false,
    SynSimplePats.SimplePats
      ([SynSimplePat.Id
          (unitVar, None, true, false, false,
           /Users/chethusk/oss/scratch/source/Library.fs (11,2--11,31) IsSynthetic=true)],
       /Users/chethusk/oss/scratch/source/Library.fs (11,2--11,31) IsSynthetic=true),
    SynExpr.App
      (NonAtomic, false,
       SynExpr.DotGet
         (SynExpr.Ident builder@, unknown (1,0--1,0) IsSynthetic=false,
          LongIdentWithDots ([Bind], []),
          /Users/chethusk/oss/scratch/source/Library.fs (11,2--11,31) IsSynthetic=true),
       SynExpr.Paren
         (SynExpr.Tuple
            (false,
             [SynExpr.App
                (NonAtomic, false,
                 SynExpr.DotGet
                   (SynExpr.Ident builder@, unknown (1,0--1,0) IsSynthetic=false,
                    LongIdentWithDots ([Source], []),
                    /Users/chethusk/oss/scratch/source/Library.fs (11,11--11,31) IsSynthetic=true),
                 SynExpr.Paren
                   (SynExpr.Paren
                      (SynExpr.App
                         (NonAtomic, false, SynExpr.Ident async,
                          SynExpr.CompExpr
                            (false, { contents = false },
                             SynExpr.YieldOrReturn
                               ((false, true),
                                SynExpr.App
                                  (Atomic, false, SynExpr.Ident X,
                                   SynExpr.Const
                                     (SynConst.Unit,
                                      /Users/chethusk/oss/scratch/source/Library.fs (11,27--11,29) IsSynthetic=false),
                                   /Users/chethusk/oss/scratch/source/Library.fs (11,26--11,29) IsSynthetic=false),
                                /Users/chethusk/oss/scratch/source/Library.fs (11,19--11,29) IsSynthetic=false),
                             /Users/chethusk/oss/scratch/source/Library.fs (11,17--11,31) IsSynthetic=false),
                          /Users/chethusk/oss/scratch/source/Library.fs (11,11--11,31) IsSynthetic=false),
                       unknown (1,0--1,0) IsSynthetic=false, None,
                       /Users/chethusk/oss/scratch/source/Library.fs (11,11--11,31) IsSynthetic=true),
                    unknown (1,0--1,0) IsSynthetic=false, None,
                    /Users/chethusk/oss/scratch/source/Library.fs (11,11--11,31) IsSynthetic=true),
                 /Users/chethusk/oss/scratch/source/Library.fs (11,11--11,31) IsSynthetic=true);
              SynExpr.MatchLambda
                (false,
                 /Users/chethusk/oss/scratch/source/Library.fs (11,7--11,8) IsSynthetic=false,
                 [Clause
                    (SynPat.Named
                       (SynPat.Wild
                          /Users/chethusk/oss/scratch/source/Library.fs (11,7--11,8) IsSynthetic=false,
                        a, false, None,
                        /Users/chethusk/oss/scratch/source/Library.fs (11,7--11,8) IsSynthetic=false),
                     None,
                     SynExpr.App
                       (NonAtomic, false,
                        SynExpr.DotGet
                          (SynExpr.Ident builder@,
                           unknown (1,0--1,0) IsSynthetic=false,
                           LongIdentWithDots ([Bind], []),
                           /Users/chethusk/oss/scratch/source/Library.fs (12,2--12,31) IsSynthetic=true),
                        SynExpr.Paren
                          (SynExpr.Tuple
                             (false,
                              [SynExpr.App
                                 (NonAtomic, false,
                                  SynExpr.DotGet
                                    (SynExpr.Ident builder@,
                                     unknown (1,0--1,0) IsSynthetic=false,
                                     LongIdentWithDots ([Source], []),
                                     /Users/chethusk/oss/scratch/source/Library.fs (12,11--12,31) IsSynthetic=true),
                                  SynExpr.Paren
                                    (SynExpr.Paren
                                       (SynExpr.App
                                          (NonAtomic, false, SynExpr.Ident async,
                                           SynExpr.CompExpr
                                             (false, { contents = false },
                                              SynExpr.YieldOrReturn
                                                ((false, true),
                                                 SynExpr.App
                                                   (Atomic, false,
                                                    SynExpr.Ident X,
                                                    SynExpr.Const
                                                      (SynConst.Unit,
                                                       /Users/chethusk/oss/scratch/source/Library.fs (12,27--12,29) IsSynthetic=false),
                                                    /Users/chethusk/oss/scratch/source/Library.fs (12,26--12,29) IsSynthetic=false),
                                                 /Users/chethusk/oss/scratch/source/Library.fs (12,19--12,29) IsSynthetic=false),
                                              /Users/chethusk/oss/scratch/source/Library.fs (12,17--12,31) IsSynthetic=false),
                                           /Users/chethusk/oss/scratch/source/Library.fs (12,11--12,31) IsSynthetic=false),
                                        unknown (1,0--1,0) IsSynthetic=false,
                                        None,
                                        /Users/chethusk/oss/scratch/source/Library.fs (12,11--12,31) IsSynthetic=true),
                                     unknown (1,0--1,0) IsSynthetic=false, None,
                                     /Users/chethusk/oss/scratch/source/Library.fs (12,11--12,31) IsSynthetic=true),
                                  /Users/chethusk/oss/scratch/source/Library.fs (12,11--12,31) IsSynthetic=true);
                               SynExpr.MatchLambda
                                 (false,
                                  /Users/chethusk/oss/scratch/source/Library.fs (12,7--12,8) IsSynthetic=false,
                                  [Clause
                                     (SynPat.Named
                                        (SynPat.Wild
                                           /Users/chethusk/oss/scratch/source/Library.fs (12,7--12,8) IsSynthetic=false,
                                         b, false, None,
                                         /Users/chethusk/oss/scratch/source/Library.fs (12,7--12,8) IsSynthetic=false),
                                      None,
                                      SynExpr.App
                                        (NonAtomic, false,
                                         SynExpr.DotGet
                                           (SynExpr.Ident builder@,
                                            unknown (1,0--1,0) IsSynthetic=false,
                                            LongIdentWithDots ([Bind], []),
                                            /Users/chethusk/oss/scratch/source/Library.fs (13,2--13,31) IsSynthetic=true),
                                         SynExpr.Paren
                                           (SynExpr.Tuple
                                              (false,
                                               [SynExpr.App
                                                  (NonAtomic, false,
                                                   SynExpr.DotGet
                                                     (SynExpr.Ident builder@,
                                                      unknown (1,0--1,0) IsSynthetic=false,
                                                      LongIdentWithDots
                                                        ([Source], []),
                                                      /Users/chethusk/oss/scratch/source/Library.fs (13,11--13,31) IsSynthetic=true),
                                                   SynExpr.Paren
                                                     (SynExpr.Paren
                                                        (SynExpr.App
                                                           (NonAtomic, false,
                                                            SynExpr.Ident async,
                                                            SynExpr.CompExpr
                                                              (false,
                                                               { contents =
                                                                           false },
                                                               SynExpr.YieldOrReturn
                                                                 ((false, true),
                                                                  SynExpr.App
                                                                    (Atomic,
                                                                     false,
                                                                     SynExpr.Ident
                                                                       X,
                                                                     SynExpr.Const
                                                                       (SynConst.Unit,
                                                                        /Users/chethusk/oss/scratch/source/Library.fs (13,27--13,29) IsSynthetic=false),
                                                                     /Users/chethusk/oss/scratch/source/Library.fs (13,26--13,29) IsSynthetic=false),
                                                                  /Users/chethusk/oss/scratch/source/Library.fs (13,19--13,29) IsSynthetic=false),
                                                               /Users/chethusk/oss/scratch/source/Library.fs (13,17--13,31) IsSynthetic=false),
                                                            /Users/chethusk/oss/scratch/source/Library.fs (13,11--13,31) IsSynthetic=false),
                                                         unknown (1,0--1,0) IsSynthetic=false,
                                                         None,
                                                         /Users/chethusk/oss/scratch/source/Library.fs (13,11--13,31) IsSynthetic=true),
                                                      unknown (1,0--1,0) IsSynthetic=false,
                                                      None,
                                                      /Users/chethusk/oss/scratch/source/Library.fs (13,11--13,31) IsSynthetic=true),
                                                   /Users/chethusk/oss/scratch/source/Library.fs (13,11--13,31) IsSynthetic=true);
                                                SynExpr.MatchLambda
                                                  (false,
                                                   /Users/chethusk/oss/scratch/source/Library.fs (13,7--13,8) IsSynthetic=false,
                                                   [Clause
                                                      (SynPat.Named
                                                         (SynPat.Wild
                                                            /Users/chethusk/oss/scratch/source/Library.fs (13,7--13,8) IsSynthetic=false,
                                                          c, false, None,
                                                          /Users/chethusk/oss/scratch/source/Library.fs (13,7--13,8) IsSynthetic=false),
                                                       None,
                                                       SynExpr.App
                                                         (NonAtomic, false,
                                                          SynExpr.DotGet
                                                            (SynExpr.Ident
                                                               builder@,
                                                             unknown (1,0--1,0) IsSynthetic=false,
                                                             LongIdentWithDots
                                                               ([Return], []),
                                                             /Users/chethusk/oss/scratch/source/Library.fs (14,2--14,22) IsSynthetic=true),
                                                          SynExpr.Paren
                                                            (SynExpr.Paren
                                                               (SynExpr.Tuple
                                                                  (false,
                                                                   [SynExpr.LongIdent
                                                                      (false,
                                                                       LongIdentWithDots
                                                                         ([a; A],
                                                                          [/Users/chethusk/oss/scratch/source/Library.fs (14,10--14,11) IsSynthetic=false]),
                                                                       None,
                                                                       /Users/chethusk/oss/scratch/source/Library.fs (14,9--14,12) IsSynthetic=false);
                                                                    SynExpr.LongIdent
                                                                      (false,
                                                                       LongIdentWithDots
                                                                         ([b; A],
                                                                          [/Users/chethusk/oss/scratch/source/Library.fs (14,15--14,16) IsSynthetic=false]),
                                                                       None,
                                                                       /Users/chethusk/oss/scratch/source/Library.fs (14,14--14,17) IsSynthetic=false);
                                                                    SynExpr.LongIdent
                                                                      (false,
                                                                       LongIdentWithDots
                                                                         ([c; A],
                                                                          [/Users/chethusk/oss/scratch/source/Library.fs (14,20--14,21) IsSynthetic=false]),
                                                                       None,
                                                                       /Users/chethusk/oss/scratch/source/Library.fs (14,19--14,22) IsSynthetic=false)],
                                                                   [/Users/chethusk/oss/scratch/source/Library.fs (14,12--14,13) IsSynthetic=false;
                                                                    /Users/chethusk/oss/scratch/source/Library.fs (14,17--14,18) IsSynthetic=false],
                                                                   /Users/chethusk/oss/scratch/source/Library.fs (14,9--14,22) IsSynthetic=false),
                                                                unknown (1,0--1,0) IsSynthetic=false,
                                                                None,
                                                                /Users/chethusk/oss/scratch/source/Library.fs (14,2--14,22) IsSynthetic=true),
                                                             unknown (1,0--1,0) IsSynthetic=false,
                                                             None,
                                                             /Users/chethusk/oss/scratch/source/Library.fs (14,2--14,22) IsSynthetic=true),
                                                          /Users/chethusk/oss/scratch/source/Library.fs (14,2--14,22) IsSynthetic=true),
                                                       /Users/chethusk/oss/scratch/source/Library.fs (14,2--14,22) IsSynthetic=false,
                                                       DebugPointForTarget.Yes)],
                                                   DebugPointAtBinding
                                                     /Users/chethusk/oss/scratch/source/Library.fs (13,2--13,31) IsSynthetic=false,
                                                   /Users/chethusk/oss/scratch/source/Library.fs (14,2--14,22) IsSynthetic=false)],
                                               [],
                                               /Users/chethusk/oss/scratch/source/Library.fs (13,2--13,31) IsSynthetic=true),
                                            unknown (1,0--1,0) IsSynthetic=false,
                                            None,
                                            /Users/chethusk/oss/scratch/source/Library.fs (13,2--13,31) IsSynthetic=true),
                                         /Users/chethusk/oss/scratch/source/Library.fs (13,2--13,31) IsSynthetic=true),
                                      /Users/chethusk/oss/scratch/source/Library.fs (13,2--14,22) IsSynthetic=false,
                                      DebugPointForTarget.Yes)],
                                  DebugPointAtBinding
                                    /Users/chethusk/oss/scratch/source/Library.fs (12,2--12,31) IsSynthetic=false,
                                  /Users/chethusk/oss/scratch/source/Library.fs (13,2--14,22) IsSynthetic=false)],
                              [],
                              /Users/chethusk/oss/scratch/source/Library.fs (12,2--12,31) IsSynthetic=true),
                           unknown (1,0--1,0) IsSynthetic=false, None,
                           /Users/chethusk/oss/scratch/source/Library.fs (12,2--12,31) IsSynthetic=true),
                        /Users/chethusk/oss/scratch/source/Library.fs (12,2--12,31) IsSynthetic=true),
                     /Users/chethusk/oss/scratch/source/Library.fs (12,2--14,22) IsSynthetic=false,
                     DebugPointForTarget.Yes)],
                 DebugPointAtBinding
                   /Users/chethusk/oss/scratch/source/Library.fs (11,2--11,31) IsSynthetic=false,
                 /Users/chethusk/oss/scratch/source/Library.fs (12,2--14,22) IsSynthetic=false)],
             [],
             /Users/chethusk/oss/scratch/source/Library.fs (11,2--11,31) IsSynthetic=true),
          unknown (1,0--1,0) IsSynthetic=false, None,
          /Users/chethusk/oss/scratch/source/Library.fs (11,2--11,31) IsSynthetic=true),
       /Users/chethusk/oss/scratch/source/Library.fs (11,2--11,31) IsSynthetic=true),
    /Users/chethusk/oss/scratch/source/Library.fs (11,2--11,31) IsSynthetic=true)]

expanded equivalent
  asyncResult.Delay(fun () -> 
      asyncResult.Bind(asyncResult.Source((async { return X() })), (fun a -> 
          asyncResult.Bind(asyncResult.Source((async { return X() })), (fun b -> 
              asyncResult.Bind(asyncResult.Source((async { return X() })), (fun c ->
                  asyncResult.Return((a.A, b.A, c.A))
              ))
          ))
      ))
  )

As best as we can tell, the emitted lambdas are correct in terms of shape. However, when we test this by manually entering this expansion into a library file with the X type defined, we see that the compiler still complains about being unable to resolve the type based on usage:

open FsToolkit.ErrorHandling

type X() = 
   member _.A = 1


let thing = 
  asyncResult.Delay(fun () -> 
      asyncResult.Bind(asyncResult.Source((async { return X() })), (fun a -> 
          asyncResult.Bind(asyncResult.Source((async { return X() })), (fun b -> 
              asyncResult.Bind(asyncResult.Source((async { return X() })), (fun c ->
                  asyncResult.Return((a.A, b.A, c.A)) // here you get ambigiuous lookup errors on each usage of `.A`
              ))
          ))
      ))
  )

we did notice that if the lambdas were explicitly typed (as is what would happen if the let! was explicitly typed (let! (a: X) = async { return X() })), the desugared form compiles well.

So perhaps there's some more general type-inference problem at play here @cartermp?

@baronfel
Copy link

This seems to be because of the presence of the Bind(Async<'t>, 't -> Async<Result<'t, 'err>>) overload. This was just introduced in version 1.4.1 of this library, to allow users to unwrap only one level of the Async<Result<'t, 'err>> wrapped type. However it seems that adding these bind overloads makes the compiler unhappy. The ambiguity can be solved by forcing explicit type patterns (let! (a: X)) at each callsite, giving users the choice of which 'layers' they want to unwrap, but that's a huge burden to add.

This seems like a huge use case for the 'restraint' concept that @TheAngryByrd likes to talk about:

member _.Bind(a: Async<'t>, binder: 't -> Async<Result<'t, 'err>>) when 't is not Result<blah, blah> = ...

@sandeep-eroad
Copy link

I am also seeing similar behaviour with Postgres Type provider https://github.com/demetrixbio/FSharp.Data.Npgsql with latest version of FsToolkit but this works fine with version 1.2.2.

@TheAngryByrd
Copy link
Collaborator

TheAngryByrd commented Jun 25, 2020

Well the current options are:

  • See if there's a way to get the F# compiler to fix this problem (seems unlikely)
  • Remove the Bind introduced in Fixed #86 #87 to allow for type inference to work properly and list work arounds for handling the other wrapper type.
  • Keep the Bind from Fixed #86 #87 and have this as a known issue in documentation where you have to add a type annotation for classes.

@sandeep-eroad
Copy link

I vote for 2 option.

@piaste
Copy link
Author

piaste commented Jul 2, 2020

I too vote 2. As I mentioned, we use this type inference every single time we make a database query, and we cannot add a type annotation because the type is generated by the type provider and doesn't have a usable name.

On the other side, the issue #86 (that the Bind in #87 resolved) only appears in a couple of spots through our codebase, and a workaround should be possible, since all CEs are just sugar, although it seems a bit tricky.

For reference, take this piece of v1.2.6 code:

    let f () = asyncResult { return ""  }

    let g () = 

        asyncResult {        
            let! r1 = f ()   
            let! (r2 : Result<string, _>)  = f ()
            return (r2 = Ok r1)        
        }

Not sure how this could be done in v1.4.0 without forsaking the use of asyncResult entirely.

@TheAngryByrd
Copy link
Collaborator

On the other side, the issue #86 (that the Bind in #87 resolved) only appears in a couple of spots through our codebase, and a workaround should be possible, since all CEs are just sugar, although it seems a bit tricky.

Yeah this was the same for one of my work projects, but there was no real good reason for us to be doing it.

Ok I'm going to revert #87 and yank the release of 1.4.1 for now.

@TheAngryByrd
Copy link
Collaborator

TheAngryByrd commented Jul 2, 2020

@piaste so typically if I needed the Result directly for some reason, I would just use an async CE.

We also might be missing other function helpers but the most basic to just handle either Ok or Error cases,

let foldResult onSuccess onError ar =
Async.map (Result.fold onSuccess onError) ar

tends to be able to handle all cases I've come across.

@piaste
Copy link
Author

piaste commented Jul 3, 2020

@TheAngryByrd the issue happened when we had a long taskResult computation, as usually every API endpoint is a single taskResult CE, but at one particular point in the flow we had an error return that we were able to handle on the spot instead of returning a HTTP failure. Restructuring the entire CE to be a plain async with manual error handling would not be worth it.

For now, I've solved it somewhat clumsily by running the right side of the unwrapping throgh Task.map (function | Ok x -> Choice1Of2 x | Error y -> Choice2Of2 y), so the Choice doesn't get unwrapped and it can be handled manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants