FSharpPlus icon indicating copy to clipboard operation
FSharpPlus copied to clipboard

F# Regressions affecting F#+

Open gusty opened this issue 10 months ago • 9 comments

With the release of F# 9, lot of regressions are being observed.

The idea is to compile a list as much detailed as possible.

  • F#+ doesn't compiler anymore:

    • Errors in Extensions/Observable.fs and Extensions/AsyncEnumerable.fs related to SeqT.lisft see below more examples of this error.
  • F#+ examples broken:

    • CEs: https://github.com/fsprojects/FSharpPlus/blob/1f0fc122269f5f0a114b8c1d19e3369daf61b0bd/docsrc/content/computation-expressions.fsx#L120-L127 Fails both when using F#+ nuget and when using F#+ compiled with F#9. More information: The for method of the monad.fx builder is compiled as follow:

      in F#9 comp F#+ ==> Monad<unit> For$W[a,T,Monad<unit>](FSharpFunc`2[FSharpFunc`2[Unit,Monad<unit>],Monad<unit>], FSharpFunc`2[Delay,FSharpFunc`2[FSharpFunc`2[Unit,Monad<unit>],FSharpFunc`2[Delay,Monad<unit>]]], FSharpFunc`2[Monad<unit>,FSharpFunc`2[Return,FSharpFunc`2[Unit,Monad<unit>]]], FSharpFunc`2[IDisposable,FSharpFunc`2[FSharpFunc`2[IDisposable,Monad<unit>],FSharpFunc`2[Using,Monad<unit>]]], FSharpFunc`2[Monad<unit>,FSharpFunc`2[FSharpFunc`2[Unit,Monad<unit>],Monad<unit>]], a, FSharpFunc`2[T,Monad<unit>])|];
      
      in existing F#+ ==> Monad<unit> For$W[a,T,Monad<unit>](FSharpFunc`2                                                          [Delay,FSharpFunc`2[FSharpFunc`2[Unit,Monad<unit>],FSharpFunc`2[Delay,Monad<unit>]]], FSharpFunc`2[Monad<unit>,FSharpFunc`2[Return,FSharpFunc`2[Unit,Monad<unit>]]], FSharpFunc`2[IDisposable,FSharpFunc`2[FSharpFunc`2[IDisposable,Monad<unit>],FSharpFunc`2[Using,Monad<unit>]]], FSharpFunc`2[Monad<unit>,FSharpFunc`2[FSharpFunc`2[Unit,Monad<unit>],Monad<unit>]], a, FSharpFunc`2[T,Monad<unit>])|];
      
    • SeqT: https://github.com/fsprojects/FSharpPlus/blob/1f0fc122269f5f0a114b8c1d19e3369daf61b0bd/docsrc/content/type-seqt.fsx#L70-L77 Fails with `type-seqt.fsx(74,61): error FS0001: 'SeqT_V2.SeqT.lift' does not support the type 'Async', because the latter lacks the required (real or built-in) member 'Map'``

      Same failure in https://github.com/fsprojects/FSharpPlus/blob/1f0fc122269f5f0a114b8c1d19e3369daf61b0bd/docsrc/content/type-seqt.fsx#L147

    • Alternatives:

      • https://github.com/fsprojects/FSharpPlus/blob/1f0fc122269f5f0a114b8c1d19e3369daf61b0bd/docsrc/content/abstraction-alternative.fsx#L114 Using F#+ compiled with F#9 -> abstraction-alternative.fsx(114,24): error FS0001: The type 'string option list' does not support the operator 'choice'
      • https://github.com/fsprojects/FSharpPlus/blob/1f0fc122269f5f0a114b8c1d19e3369daf61b0bd/docsrc/content/abstraction-alternative.fsx#L128 abstraction-alternative.fsx(128,22): error FS0071: Type constraint mismatch when applying the default type 'obj' for a type inference variable. None of the types 'Async<(string list -> string list)>, obj' support the operator '<*>' Consider adding further type constraints
    • Bitraversable: https://github.com/fsprojects/FSharpPlus/blob/1f0fc122269f5f0a114b8c1d19e3369daf61b0bd/docsrc/content/abstraction-bitraversable.fsx#L87 abstraction-bitraversable.fsx(87,37): error FS0043: No overloads match for method 'Bisequence'.

    • Misc: https://github.com/fsprojects/FSharpPlus/blob/1f0fc122269f5f0a114b8c1d19e3369daf61b0bd/docsrc/content/abstraction-misc.fsx#L47 abstraction-misc.fsx(47,22): error FS0001: '.Choice' does not support the type 'Result<int,string>', because the latter lacks the required (real or built-in) member 'IsAltLeftZero' (seems to be the same as the one in Alternatives.

    • Monad: https://github.com/fsprojects/FSharpPlus/blob/1f0fc122269f5f0a114b8c1d19e3369daf61b0bd/docsrc/content/abstraction-monad.fsx#L346 (same as above) abstraction-monad.fsx(346,16): error FS0001: The type ''a list' does not support the operator 'choice'

    • Profunctor: https://github.com/fsprojects/FSharpPlus/blob/1f0fc122269f5f0a114b8c1d19e3369daf61b0bd/docsrc/content/abstraction-traversable.fsx#L110-L113

      • https://github.com/fsprojects/FSharpPlus/blob/1f0fc122269f5f0a114b8c1d19e3369daf61b0bd/docsrc/content/abstraction-profunctor.fsx#L107

        abstraction-profunctor.fsx(107,12): error FS0071: Type constraint mismatch when applying the default type 'obj' for a type inference variable. No overloads match for method 'Map'.

      • https://github.com/fsprojects/FSharpPlus/blob/1f0fc122269f5f0a114b8c1d19e3369daf61b0bd/docsrc/content/abstraction-profunctor.fsx#L109 `abstraction-profunctor.fsx(109,12): error FS0071: Type constraint mismatch when applying the default type 'obj' for a type inference variable. No overloads match for method 'Dimap'.

    • Traversable: https://github.com/fsprojects/FSharpPlus/blob/1f0fc122269f5f0a114b8c1d19e3369daf61b0bd/docsrc/content/abstraction-traversable.fsx#L110-L113 abstraction-traversable.fsx(110,23): error FS0071: Type constraint mismatch when applying the default type 'obj' for a type inference variable. None of the types '(int list -> int list) list, obj' support the operator '<*>' Consider adding further type constraints

      abstraction-traversable.fsx(111,23): error FS0071: Type constraint mismatch when applying the default type 'obj' for a type inference variable. None of the types '(int list -> int list) option, obj' support the operator '<*>' Consider adding further type constraints

      abstraction-traversable.fsx(112,23): error FS0071: Type constraint mismatch when applying the default type 'obj' for a type inference variable. None of the types '(int list -> int list) list, obj' support the operator '<*>' Consider adding further type constraints

      abstraction-traversable.fsx(113,23): error FS0071: Type constraint mismatch when applying the default type 'obj' for a type inference variable. None of the types '(int list -> int list) list, obj' support the operator '<*>' Consider adding further type constraints

    • ZipApplicative: https://github.com/fsprojects/FSharpPlus/blob/1f0fc122269f5f0a114b8c1d19e3369daf61b0bd/docsrc/content/abstraction-zipapplicative.fsx#L139

      abstraction-zipapplicative.fsx(139,65): error FS0193: None of the types 'Async<int>, Async<char>, Async<'a>' support the operator 'Zip'

Testing is not complete. More errors are expected in Tests and maybe in the TypeLevel project

gusty avatar Jan 30 '25 19:01 gusty

Would it make sense to nudge the CI here and maybe look into adjustments in dotnet/fsharp that would make it easy to test the preview compiler?

smoothdeveloper avatar Jan 30 '25 19:01 smoothdeveloper

Would it make sense to nudge the CI here and maybe look into adjustments in dotnet/fsharp that would make it easy to test the preview compiler?

I think it would. Reason is, F#+ uses code in a style that is not covered by dogfooding or by other early users of previews - most of these bugs are reports unlike any other we have received. It might make sense to have a dedicated CI leg in the compiler repo that would run with preview version of the language, and run whatever is best for F#+ (just build the lib? Or build the lib + samples?).

We would need to raise familiarity with internal workings of F#+ amongst compiler contributors including myself (so that we can identify and isolate issues better), but that would be definitely for the better.

If anyone familiar with F#+ would be willing to contribute some scripting snippets to do a basic sanity test of F#+, I will be happy to integrate it as a new CI leg in the compiler.

T-Gro avatar Feb 27 '25 20:02 T-Gro

Due to the nature of the issues, even two different legs I assume:

  • download published F#+ from nuget, and run a battery of samples using preview compiler against it
  • build fresh F#+ with the preview compiler from source, and run the samples with it

WDYT?

T-Gro avatar Feb 27 '25 20:02 T-Gro

@T-Gro I think it totally makes sense. You can't imagine how hard is for me to anticipate to each new compiler and test everything by hand with F#+, then follow up on fixes. All this using free time which sometimes is quite limited.

Also important to mention, Don had the same idea, before you joined the team and there is somewhere a PR that integrates F#+ in the F# compiler test project. It just received some pushback from Phillip at that time and finally got abandoned. But we can definitely revive and improve it.

gusty avatar Feb 27 '25 23:02 gusty

The main thing I remember was Don wasn't super eager we use F#+ as motivator for evolving the compiler / language design (on areas like overload resolution, etc.), but I think for catching overload resolution, type inference regressions, and seeing if compatibility with library compiled with older compiler, F#+ is a nice stress test.

Overall, a generic infrastructure where we can add to the CI:

  • pointer to repository
  • required build command
  • sample code consuming the library

This infrastructure would provide high level reporting:

  • compile error and warnings if any versus release version of compiler
  • if there is any public signature change when compiling the library with new compiler
  • issue referencing older compiler build of the library in a small script that consumes it running through new compiler

Maybe this would work better if this can be external to the compiler repository upfront, so ultimately it won't be core compiler team owning it, as I know, each such request expands the maintenance area significantly.

I think it is still a lot of work to make something like this though.

But in meantime, a small thing that checks F#+ in this repository is still good way to know upfront about regressions.

smoothdeveloper avatar Feb 27 '25 23:02 smoothdeveloper

This one I assume? https://github.com/dotnet/fsharp/pull/5878/files

Understandably, there has been pushback on the consequences of such CI leg failing, especially if caused by a F#+ change added since last run(even when hypothetical) and not by the compiler-contributing PR executing the test. I think this is still worth it, and we could do a backdoor where a label "Accept-F#+-Break" would allow merging a PR despite seeing some breaks. IMO still worth a try and we will be wiser after running this for some period of time.

Another angle is the one of security and stability implied by running code from a variable location. i.e. choosing a compromise between the various options of what code to build & run exactly:

  • Always clone fresh fsprojects/FSharpPlus/main
  • Clone a well known stable commit/tag, allow regular PRs for F#+ maintainers to change that commit inside dotnet/fsharp
  • Maintain a mirror of the code and tests within dotnet/fsharp (similarly to how fslex/yacc are being mirrored)

The same question then applies to the scripts orchestrating it.

Thinking this trough, the option I currently like the most:

  • having a well known stable commit/tag pointer to this repo
  • keeping all the lib code, test code and orchestration scripts within this repo, not maintain a mirror in dotnet/fsharp
  • dotnet/fsharp would then only maintain the information about which commit/tag to clone and which script(s) to execute (the CI leg would only do something along the lines of git clone && ./compiler-regression-tests.ps1 )

Upside: Risks and stability can be safely accepted by dotnet/fsharp owners, the code being built is no longer a moving target Downside: The well known commit/tag would need to be regularly updated depending on the size and frequency of changes in fsprojects/FSharpPlus Downside: The scripts would need to be aware of where they are running and replug project settings in order to use the freshly built-compiler from a sibling folder, remove global.json file etc.

T-Gro avatar Feb 28 '25 11:02 T-Gro

Yes, that's the PR.

Actually if you read through, the idea was to always use a specific commit, so changes in the library shouldn't affect the compiler.

Don explains in the comments how to achieve that.

gusty avatar Feb 28 '25 15:02 gusty

I agree to that. The compiler codebase would only need to know your repo URL, specific commit, and which script/command to execute.

That way, if F#+ pivots this, same mechanism can be also used for other critical parts of the F# library ecosystem (where F#+ still has a special place due to the way it stressed SRTP features).

I will have a think on how to smoothly plug in the necessary overrides ( they should be covered here https://github.com/dotnet/fsharp/blob/main/UseLocalCompiler.Directory.Build.props + cleaning global.json references). This set of overrides gives a hint - perharps to CI job being executed could have environment variables pointing to the freshly built compiler artifacts ($LocalFSharpCompilerPath) and likely one more parameter to include the current SDK/tfm )

T-Gro avatar Feb 28 '25 16:02 T-Gro

Here are some regressions tested with docker images:

I will keep updating, but so far these regressions happened in F#9 and in F#10 preview they still repro.

#r "nuget: FSharpPlus"

open FSharpPlus

let y: seq<_> = monad.plus {
    for x in seq [1..3] do
        for y in seq [10; 20] do
            return (x, y)
}

here's another one

#r "nuget: FSharpPlus"

open FSharpPlus
open FSharpPlus.Data

type AsyncResult<'T, 'E> = ResultT<Async<Result<'T, 'E>>>

type ResultTBuilder<'``monad<Result<'t, 'e>>``>() =
  inherit Builder<ResultT<'``monad<Result<'t, 'e>>``>>()

  member inline _.For (x: ResultT<'``Monad<Result<'T, 'E>>``>, f: 'T -> ResultT<'``Monad<Result<'U, 'E>>``>) = x >>= f : ResultT<'``Monad<Result<'U, 'E>>``>

  [<CustomOperation("lift", IsLikeZip=true)>]
  member inline _.Lift (x: ResultT<'``Monad<Result<'T, 'E>>``>, m: '``Monad<'U>``, f: 'T -> 'U -> 'V) =
    x >>= fun a ->
      lift m |> ResultT.bind (fun b ->
        result (f a b) : ResultT<'``Monad<Result<'V, 'E>>``>)

let resultT<'``Monad<Result<'T, 'E>>``> = new ResultTBuilder<'``Monad<Result<'T, 'E>>``>()

let sampleWorkflow2 =
  monad {
    let! x = Some 1
    let! y = Some 2
    return x + y
  }

let test2 () =
  resultT {
    let! x = ResultT.hoist (Ok 1)
    lift y in sampleWorkflow2
    return x + y
  }

gusty avatar Sep 21 '25 08:09 gusty