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

F# Regressions affecting F#+ #613

Open
gusty opened this issue Jan 30, 2025 · 8 comments
Open

F# Regressions affecting F#+ #613

gusty opened this issue Jan 30, 2025 · 8 comments

Comments

@gusty
Copy link
Member

gusty commented Jan 30, 2025

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:

      let (asnNumber: Async<_>) = monad.fx {
      let mutable m = ResizeArray ()
      try
      for i = 1 to 10 do
      m.Add i
      return m.[-1]
      with e ->
      return -3 }

      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:

      let pages: SeqT<_, _> = monad.plus {
      use wc = new WebClient ()
      for url in urls do
      try
      let! html = wc.AsyncDownloadString (Uri url) |> SeqT.lift
      yield url, html.Length
      with _ ->
      yield url, -1 }

      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

      let! doc = downloadDocument url |> SeqT.lift

    • Alternatives:

      • let firstGood = choice alternatives //Some "Result is OK"

        Using F#+ compiled with F#9 -> abstraction-alternative.fsx(114,24): error FS0001: The type 'string option list' does not support the operator 'choice'
      • let nameAndAddress = traverse (fun x -> putStrLn x >>= fun _ -> getLine) ["name";"address"]

        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:

      let res42 = asyncSquareRoot 1764 |> bisequence |> Async.RunSynchronously
      abstraction-bitraversable.fsx(87,37): error FS0043: No overloads match for method 'Bisequence'.

    • Misc:

      let firstOk = choice resultList

      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:

      (same as above)
      abstraction-monad.fsx(346,16): error FS0001: The type ''a list' does not support the operator 'choice'

    • Profunctor:

      let res210 = traverse f [1; 2; 3]
      let resSome210 = traverse g [1; 2; 3]
      let resEmptyList = traverse f [1000; 2000; 3000]
      let resEListOfElist = traverse f []

    • Traversable:

      let res210 = traverse f [1; 2; 3]
      let resSome210 = traverse g [1; 2; 3]
      let resEmptyList = traverse f [1000; 2000; 3000]
      let resEListOfElist = traverse f []

      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:

      let (x, y) = zip (async { return 1 }) (async { return '2' }) |> Async.RunSynchronously

      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

@smoothdeveloper
Copy link
Contributor

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?

@T-Gro
Copy link

T-Gro commented Feb 27, 2025

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
Copy link

T-Gro commented Feb 27, 2025

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?

@gusty
Copy link
Member Author

gusty commented Feb 27, 2025

@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.

@smoothdeveloper
Copy link
Contributor

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.

@T-Gro
Copy link

T-Gro commented Feb 28, 2025

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.

@gusty
Copy link
Member Author

gusty commented Feb 28, 2025

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.

@T-Gro
Copy link

T-Gro commented Feb 28, 2025

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 )

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

3 participants