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

Make Fantomas compatible with FCS #3032

Closed
wants to merge 3 commits into from

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Dec 29, 2023

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 Incorrect range for SynInterpolatedStringPart.String in extended interpolated string dotnet/fsharp#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
Copy link
Member

dawedawe commented Jan 2, 2024

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

nojaf commented Jan 2, 2024

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

vzarytovskii commented Jan 3, 2024

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

nojaf commented Feb 8, 2024

I don't think this is worth it.

@nojaf nojaf closed this Feb 8, 2024
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

Successfully merging this pull request may close these issues.

3 participants