search

fsprojects / fantomas

FSharp source code formatter
https://fsprojects.github.io/fantomas
Other
758 stars 189 forks source link

Make Fantomas compatible with FCS #3032

Closed nojaf closed 4 months ago

nojaf commented 6 months ago

Hey folks, a quick update on the 'plain FCS vs. Fantomas.FCS fork' situation, as I promised @psfinaki. After some digging, it turns out we can work with plain FCS, but it's not all smooth sailing. Here's the deal:

  1. FSharp.Core version: Going with FCS means we're locked to FSharp.Core 8.0.100. We're trying to support older versions, so that's a bit of a snag.
  2. FCS Release Cycle - A Real Pain: The release cycle for FCS is a mess. The team isn't handling the releases directly, so what hits NuGet is pretty inconsistent. If Fantomas hits a snag, we might be waiting a month for a fix. Nightly packages could be a workaround, but they're a double-edged sword. They might work for the Fantomas tool, but for Fantomas.Core? Not so much. It could clash with other NuGet packages relying on FCS. Maybe we could use nightlies for the tool and keep Fantomas.Core on the stable releases, but is it worth the hassle?
  3. Nightly Branch Benefits: On the plus side, a nightly branch using nightly FCS packages would be great for the compiler, acting as a solid regression test for the parser.
  4. Flexibility with Source Dependency: Currently, our source dependency on the compiler is pretty handy. We can fix a parser bug in dotnet/fsharp and use it immediately. Switching to FCS, even the nightly version, means we lose that quick turnaround. A prime example is the issue at https://github.com/dotnet/fsharp/issues/16418.

So, where does that leave us? Honestly, it's a bit of a head-scratcher. This was part of a bigger exercise to understand why projects fork the F# compiler, but the answer isn't clear-cut. Thoughts?

dawedawe commented 6 months ago

Mmh, so the only real upside of this would be to catch regressions in the parser, right? Maybe adding a strong regression test suite to the parser is the better way instead of relying on a consumer to catch regressions.

nojaf commented 6 months ago

Well, not having to deal with the source dependency also just simplifies things. Right now, you need to download the files and compile Fantomas.FCS first. This takes a modest while and could be avoided.

It is also a bit more about not having another fork of FCS. There is a human problem with why we have the fork in the first place. It doesn't hurt to expose this more.

vzarytovskii commented 6 months ago
  1. FCS Release Cycle - A Real Pain: The release cycle for FCS is a mess.

Not entirely true, it's just tied to SDK releases. We had some issues with not bumping versions, but we have added a bunch of CI checks since.

The team isn't handling the releases directly, so what hits NuGet is pretty inconsistent.

It is somewhat consistent, but not to exact commit, we don't know in advance what will get in for sure. Since merge can be done after the cutoff date. The only thing we can do to make it more consistent in this sense is to pretty much cut-off in advance, so we know exactly what will get in, anything after that should go via backport/approval for SDK or to the next version. But this won't be much of the help, since it will only prolong the wait time until some fix.

Nightlies or source-based package (direct submodule?) might be the most feasible solution if latest bits are needed. But both will pretty much pin FCS and FSLib versions, it can be disruptive, especially for someone who will need to consume Fantomas as library.

nojaf commented 4 months ago

I don't think this is worth it.