Skip to content

Conversation

T-Gro
Copy link
Member

@T-Gro T-Gro commented Dec 29, 2022

See the added tests and the linked issue.
This has been introduced with the possibility to do a optimized .BindReturn (in one operation).

When used inside an if-then; without else; this started to fail with F# 5.0, but was working in F# 4.7.

After some attempts of making it go trough the .BindReturn even when conditional is used (by passing in the result of .Zero() into the 'then' expression, and letting it go via .BindReturn all the time), I did not manage to make it work.

Instead, if such situation is detected, the expression will fallback to using regular .Bind + .Return calls, just like it did in F# 4.7.

This affects code which was not compiling at F# 5.0 and later at all.

Summary of behavior depending on F# version:

  1. Until 4.7: The code works in all circumstances, but never uses the .BindReturn optimized call
  2. Before this fix: Code does not compile. If _.BindReturn is commented out, it starts working
  3. After this fix: Code compiles. If return is used outside of conditional or other control flow, _.BindReturn is indeed used and classical _.Return is not even required (see added tests). If return is used within a conditinal, the flow fallbacks to using a combination of _.Return and _.Bind, and checks that both of them are present.
type Builder () =
   
    member inline __.Return (x: 'T) = Seq.singleton x                          : seq<'T>
    member inline __.Bind (p: seq<'T>, rest: 'T->seq<'U>) = Seq.collect rest p : seq<'U>
    member inline __.Zero () = Seq.empty : seq<'T>
    member inline __.BindReturn   (x : seq<'T>, f: 'T -> 'U) : seq<'U> = Seq.map f x

let seqbuilder= new Builder ()

let _pythags = seqbuilder {
  let! z = seq [5;10]
  let! x = seq [1..z]
  let! y = seq [x..z]
  if (x*x + y*y = z*z) then return (x, y, z) }

Why I agree with @gusty that this was a bug+regression and should be fixed:

  • Libraries were offering CE to be consumed by 3rd parties, who's code inside the CE is out of control for the library authors.
  • The library, without any other changes, adds a single _.BindReturn method. This is documented to be a performance optimization when applicable.
  • Suddenly, 3rd party code is not compiling with new version of the CE/Builder under some circumstances, even though no breaking change happened (the older builder methods for regular .Bind and .Return are still there)

…hen without else. Now trying with fallback to regular Bind+Return instead
@T-Gro T-Gro linked an issue Dec 29, 2022 that may be closed by this pull request
@T-Gro T-Gro marked this pull request as ready for review January 2, 2023 08:52
@T-Gro T-Gro requested a review from a team as a code owner January 2, 2023 08:52
@T-Gro T-Gro merged commit 8ba0834 into main Jan 2, 2023
@T-Gro T-Gro deleted the 10379-problem-with-applicative-ces-and-zero branch January 2, 2023 11:15
@dsyme
Copy link
Contributor

dsyme commented Jan 10, 2023

Great work!

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

Successfully merging this pull request may close these issues.

Problem with Applicative CEs and 'Zero'
4 participants