Add support for Fourmolu 0.16

[brandonchinn178]

No description provided.

[jhrcek]

Hey @brandonchinn178
thanks for working on the upgrade!
It seems though (from my trying to cherry-pick this update to wip/9.10 branch) that hls fourmolu plugin code no longer compiles with 0.16, because this new version no longer exports loadConfigFile (used here and exported by fourmolu<0.16): https://hackage.haskell.org/package/fourmolu-0.15.0.0/docs/Ormolu-Config.html#v:loadConfigFile

Could you advise how to deal with that? It seems that fourmolu-0.16's Ormolu.Config module doesn't export any function for loading FourmoluConfig..

[jhrcek]

For reference: ci/circleci: stackage-lts21 job is failing with Could not find module ‘Paths_fourmolu’ due to it using older version of fourmolu.

We might as well update to more recent stack snapshots, but for the purposes of this PR I'd be ok adding more recent version of fourmolu to extra-deps in stack-lts21.yaml

[michaelpj]

Does that suggest this doesn't actually compile with older fourmolus? Should we raise the lower bound?

[jhrcek]

Should we raise the lower bound?

It seems that Paths_fourmolu has been exposed by fourmolu lib since 0.12.0.0, older versions don't expose it.

lts-21 resolver we use in CI has fourmolu-0.11.0.0

The bounds in cabal file look fine, it's just that the stack.yaml is ignoring them. Who would have thought that allow-newer actually also allows older 😄

[jhrcek]

FYI: this PR should solve the "stack CI fails because it's using old formolu" issue.

[brandonchinn178]

@jhrcek Is it important to support multiple versions of Fourmolu? Don't most users of HLS download a prebuilt executable? I don't see why HLS can't just depend on one version of Fourmolu at a time

because this new version no longer exports loadConfigFile

Yes, I'll push a change. Basically, we broke up the function into much better composable pieces now. loadConfigFile is now findConfigFile that just looks for the config, and then you can parse the config with the normal Yaml.decode* functions

[jhrcek]

Since we support formatting with external fourmolu (do you know which external fourmolu versions the plugin should be compatible with?) I'd be ok with supporting only 1 version of build-in fourmolu library.
As long as it passes CI for all supported ghc versions.

But lets hear from more active maintainers whether they'd agree with that too, @fendor ? @michaelpj ?

[brandonchinn178]

do you know which external fourmolu versions should be supported?

Any external fourmolu version should work. I think the plugin just shells out, so there shouldn't really be breaking changes

But lets hear from more active maintainers whether they'd agree with that too

Sure, the last commit I pushed dropped support for < 0.16, but I can revert if necessary (and handle compat with older versions)

[michaelpj]

Is it important to support multiple versions of Fourmolu?

It's important to support multiple versions of GHC, but I think we only need a range of fourmolu versions that handles that, so it should be fine to raise the lower bound.

[michaelpj]

Needs an index-state bump.

[fendor]

LGTM!

Still more discussion necessary, can't be merged as is.

[fendor]

I misread the discussion and thought the only change necessary to get this merged is an index-state bump. Now I realise the discussion is rather whether it is ok to only support a fourmolu version that can only be compiled with GHC 9.10. I apologise for pushing to this PR without asking.

We want to support the fourmolu code formatter for HLS binaries that have been compiled with older GHC versions. As such, I think we still need the CPP conditionals.

[brandonchinn178]

Fourmolu uses ghc-src-meta, so any given version of Fourmolu works for the last 3 major GHC versions. Specifically, 0.16 works for GHC 9.6, 9.8, and 9.10

Does HLS really need to build against 9.2 and 9.4? I'm surprised that Fourmolu was building against those

[fendor]

Does HLS really need to build against 9.2 and 9.4?

According to our current deprecation policy, yes. GHC 9.8 is not yet part of a stackage LTS, so we need to support at least GHC 9.4.

I'm surprised that Fourmolu was building against those

Perhaps it worked because we were using old fourmolu versions as well?

Frankly, perhaps it is time to no longer link formatters directly into HLS. Perhaps we should build them and ship formatter binaries with HLS? But this discussion shouldn't happen on a PR, but on the issue #411.

[brandonchinn178]

Ah I didn't realize that Fourmolu 0.14 supported back to GHC 9.2. Ok, I'll add those back in

[jhrcek]

Thank you! LGTM

[jhrcek]

Oh, I forgot, we should enable fourmolu for ghc 9.10.
I pushed a commit to that effect, fixing compilation errors with 9.10 along the way.

[jhrcek]

I forgot how to push to PR branches coming from external fork and am too lazy to open a PR targetting @brandonchinn178 's fork. Brandon, could you please apply this patch in your PR?

diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index fa851b03..84e75963 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -177,8 +177,7 @@ jobs:
         name: Test hls-ormolu-plugin
         run: cabal test hls-ormolu-plugin-tests || cabal test hls-ormolu-plugin-tests
 
-        # TODO enable when it supports 9.10
-      - if: matrix.test && matrix.ghc != '9.10'
+      - if: matrix.test
         name: Test hls-fourmolu-plugin
         run: cabal test hls-fourmolu-plugin-tests || cabal test hls-fourmolu-plugin-tests
 
diff --git a/cabal.project b/cabal.project
index 1e8a7640..d406a40c 100644
--- a/cabal.project
+++ b/cabal.project
@@ -49,10 +49,9 @@ if impl(ghc >= 9.9)
     lens >= 5.3.2,
     -- See
     -- https://github.com/haskell/stylish-haskell/issues/479
-    -- https://github.com/fourmolu/fourmolu/issues/412
     -- https://github.com/ennocramer/floskell/pull/82
     -- https://github.com/ndmitchell/hlint/pull/1594
-    haskell-language-server -stylishHaskell -fourmolu -hlint -retrie -splice -floskell,
+    haskell-language-server -stylishHaskell -hlint -retrie -splice -floskell,
   allow-newer:
     entropy:base,
     entropy:directory,
diff --git a/haskell-language-server.cabal b/haskell-language-server.cabal
index e58ea696..acede2ec 100644
--- a/haskell-language-server.cabal
+++ b/haskell-language-server.cabal
@@ -1470,7 +1470,7 @@ library hls-fourmolu-plugin
     , process-extras  >= 0.7.1
     , text
     , transformers
-
+    , yaml
 
 test-suite hls-fourmolu-plugin-tests
   import:           defaults, pedantic, test-defaults, warnings
diff --git a/plugins/hls-fourmolu-plugin/src/Ide/Plugin/Fourmolu.hs b/plugins/hls-fourmolu-plugin/src/Ide/Plugin/Fourmolu.hs
index ca0911a6..7615b7d2 100644
--- a/plugins/hls-fourmolu-plugin/src/Ide/Plugin/Fourmolu.hs
+++ b/plugins/hls-fourmolu-plugin/src/Ide/Plugin/Fourmolu.hs
@@ -46,6 +46,10 @@ import           System.Process.Run              (cwd, proc)
 import           System.Process.Text             (readCreateProcessWithExitCode)
 import           Text.Read                       (readMaybe)
 
+#if MIN_VERSION_fourmolu(0,16,0)
+import qualified Data.Yaml                       as Yaml
+#endif
+
 descriptor :: Recorder (WithPriority LogEvent) -> PluginId -> PluginDescriptor IdeState
 descriptor recorder plId =
     (defaultPluginDescriptor plId desc)
@@ -153,8 +157,8 @@ loadConfig recorder fp = do
             pure emptyConfig
         Right file -> do
             logWith recorder Info $ ConfigPath file
-            Yaml.decodeFileEither file >>= \case
-                Left e -> do
+            liftIO (Yaml.decodeFileEither file) >>= \case
+                Left err -> do
                     let errorMessage = "Failed to load " <> T.pack file <> ": " <> T.pack (show err)
                     lift $ pluginSendNotification SMethod_WindowShowMessage $
                         ShowMessageParams