Ormolu formatter support

[DavSanchez]

Hi! This addresses #1410 for the Ormolu formatter

I still have some work to do, that is sending a warning if the Format Selection option is used (in lines 31-33 of the committed Ormolu.hs), as this function is not yet supported by Ormolu, but I don't know exactly the best way to proceed. I was pointed to LspStdio.hs and about throwing a warning on the right thread, but I'm still interpreting the file and how to make use of the functions in it.

Feel free to provide any suggestions.

Also, I think I should put up some tests in FormatSpec.hs when Ormolu.hs is done, to let everyone check that everything works 😃

[fendor]

First, thank you for this PR! It looks very promising.

Is it possible that ormolu does not work with GHC versions below 8.6?

[DavSanchez]

Thanks @fendor! Mm yes it’s possible, currently I have only tested it in GHC 8.6.5... I’ll take a look on the Ormolu repo and the failing tests to see if I can make it work there.

EDIT: Yes it’s an issue with base versions... I think that makes it incompatible with 8.4.x.

[fendor]

Yeah, with the base restriction, I dont think it is possible with GHC versions below 8.6.
Which is fine for now, rather focus on the test-suite, if possible!

[lukel97]

@DavSanchez looks like you'll probably need to add ormolu to the extra-deps fields in the stack.yaml's of those failing CI runs

[DavSanchez]

@DavSanchez looks like you'll probably need to add ormolu to the extra-deps fields in the stack.yaml's of those failing CI runs

I removed it because ormolu is not compatible with these versions of GHC, due to the base versions used. Is there any course of action for these stack-8.4.x.yaml builds to ignore Ormolu for the CI?

[fendor]

No, you can not disable the CI for certain tests.
I think you have to add feature flags, e.g. guard the package import to only happen when the ghc version is greater than 8.4 and guard the plugin implementation, too, with CPP statements. My suggestion is to use a noop formatter if ormolu is not available and Log.errorm messages.

[fendor]

Tests are still missing.

I suggest to use CPP guards to guard the implementation and provide noop formatters if ormolu is not available for the ghc version.

[DavSanchez]

No, you can not disable the CI for certain tests.
I think you have to add feature flags, e.g. guard the package import to only happen when the ghc version is greater than 8.4 and guard the plugin implementation, too, with CPP statements. My suggestion is to use a noop formatter if ormolu is not available and Log.errorm messages.

Ah, I didn't know you could insert C-like guards simply like that in Haskell code. I'll take a look and update the code accordingly. I'll also check Log.errorm.

As for the tests, yeah I don't have them at the moment, I'll add them during the holidays.

[DavSanchez]

I think you have to add feature flags, e.g. guard the package import to only happen when the ghc version is greater than 8.4 and guard the plugin implementation, too, with CPP statements.

Hi @fendor, if I guard the package imports in haskell-ide-engine.cabal is it really needed to also guard Ormolu.hs itself? If I guard just the module import at MainHie.hs does the file get compiled anyway, even if it's not used by any target file in haskell-ide-engine.cabal?

My suggestion is to use a noop formatter if ormolu is not available and Log.errorm messages.

At the moment my attempt just does return $ IdeResultOk [TextEdit (fullRange contents) contents]. Since this does an actual replacement (with the same original file content), is there any other way of doing a true noop without messing with the types or is that the correct way to go?

[fendor]

@DavSanchez I think it would be better if we guard the functions in Ormolu, since we have to add the available formatters to the lsp-client, seemingly. iirc https://github.com/alanz/vscode-hie-server/blob/954e0a9ecc7be4d96bc71d05c65a730e642b65a6/package.json#L87.
We probably have to add an entry to this list of formatters, but we dont know if the ormolu formatter is really available since this depends on the ghc version the lsp-server has been compiled with. That is why I think, guarding just the functions and the imports (also the dependency like you already did) in Ormolu.hs is better. We still want to inform the user somehow that this doesnt work.

Maybe return $ IdeResultOk [] is already enough? Then there is no text-edit.

[DavSanchez]

Hi again! A few questions.

In the test suite (FormatSpec.hs file), how do I control what formatter is selected for the appropriate test? I see that it tests whether the formatting provider changes and also see the brittany-specific test suite, but for this last test I can't see where brittany is selected.

How can I assert that a warning or error was emitted? This is for checking that a format selection action triggers the correct message.

Also, should I make any tests for the noop formatter used when GHC < 8.6?

[fendor]

@DavSanchez Sorry for the late response, I overlooked the notification.
To switch the formatter the following can be used:

sendNotification WorkspaceDidChangeConfiguration (DidChangeConfigurationParams (formatLspConfig "ormolu"))
formatDoc doc (FormattingOptions 2 True)
documentContents doc >>= liftIO . (`shouldBe` ormolu)

For example, see the test-case in https://github.com/haskell/haskell-ide-engine/blob/master/test/functional/FormatSpec.hs#L49.

You will test the noop formatter anyways, since the test-cases need to work across all CI jobs.
For example, you can check the ghc version via https://github.com/haskell/haskell-ide-engine/blob/master/test/utils/TestUtils.hs#L146 (this avoids using CPP instructions).
then just do:

formatDoc doc (FormattingOptions 2 True)
docContent <- documentContents doc
case ghcVersion of 
  GHC86 -> liftIO $ docContent  `shouldBe` formattedOrmolu
  _ -> liftIO $ docContent  `shouldBe` unchangedDoc

Tests relevant for hsimport are here: https://github.com/haskell/haskell-ide-engine/blob/master/test/functional/FunctionalCodeActionsSpec.hs#L529
You can see an example call of this function here: https://github.com/haskell/haskell-ide-engine/blob/master/test/functional/FunctionalCodeActionsSpec.hs#L136
In this case, you wont need ghcVersion since we do not support that feature, anyways.
Calling the function hsImportSpec correctly, should be enough to test and document the hsimport interaction with ormolu we mentioned above.

hope this helps! (btw, did not try to compile any of the code snippets, might contain small errors but the idea is hopefully right)

EDIT:

Sending an error should in my opinion be done in Server.hs. There we can intercept NotFormatDocument and NotFormatDocumentSelection and throw an error if ormolu is selected which is presented to the user which you can actually check for. However, I would postpone that after the tests are finished, maybe @bubba has a better idea than that?

[lukel97]

@fendor

Sending an error should in my opinion be done in Server.hs. There we can intercept NotFormatDocument and NotFormatDocumentSelection and throw an error if ormolu is selected which is presented to the user which you can actually check for. However, I would postpone that after the tests are finished, maybe @bubba has a better idea than that?

I believe IdeErrors get presented to the users as a showmessagerequest anyway at the end of the day, so it should be fine + should be possible to test for it. I agree though, we can prettify this up after the PR!

[DavSanchez]

Looks like the tests for FormatSpec.hs are passing. Just on time for 2020 at my place!

Happy new year everyone! 🎉

[fendor]

The tests for hsimport https://github.com/haskell/haskell-ide-engine/blob/master/test/functional/FunctionalCodeActionsSpec.hs#L529 are missing.

[DavSanchez]

Yeah, I’ll implement them in the next hours.

[fendor]

Note, you will have problems with your implementation for format selection. I suggest, until @bubba disagrees, that format selection is a noop and that a warning is added to Server.hs when the explicit request for formatting a selection is received

[DavSanchez]

Yeah but, what is exactly happening here? I would assume that the hsImport test would fail on the first two cases, since the expected results are formatted and Ormolu in GHC < 8.6 is a NOP, but why the timeout instead? What am I missing?

[Avi-D-coder]

We should pass default-extensions and ghc-options in ormolu Config.cfgDynOptions

[DavSanchez]

We should pass default-extensions and ghc-options in ormolu Config.cfgDynOptions

You mean adding them to the Cabal and/or Stack files for HIE or loading Ormolu as a submodule and doing it there? Why is it necessary?

[fendor]

@DavSanchez while @Avi-D-coder is right, we will postpone this for now. None of the other formatters is doing this correctly at the moment, anyways, as far as I know. We can improve it iteratively.
I will do a final review tomorrow and after addressing found issues (if there are any), I think we are ready for a merge.

[Avi-D-coder]

@DavSanchez Ormolu formats code differently if certain extensions or ghc options are enabled. E.g TypeApplications. They can be enabled cradle wide in cabal and other build files, the project wide options need to be passed to the formatter so formatting does not change the codes semantics.

[Avi-D-coder]

@fendor do we cache ghc flags anywhere or should we just call whatever function hie-bios flags $filePath does?

[fendor]

@Avi-D-coder We are currently not caching the results of hie-bios flags $filepath and just calling this function would yield very slow performance.
But we are caching the HscEnv for each cradle and transitively, we have access to DynFlags which contain the exact flags used to compile the file. However, the FormattingProvider does not permit us to look up the cache at the moment, if I read that right. We need to refactor that, so that we have access to the DynFlags.

[Avi-D-coder]

@fendor I'm not aware of a DynFlags to [String] conversion. Ormolu could be modified to provide a version that takes DynFlags instead of [String].

At Avi-D-coder@baa12ac I call findLocalCradle; it's a bit slow, but it does the job.

[fendor]

Using findLocalCradle is horribly slow since you are using either cabal repl or stack repl to load the project. I dont know how Ormolu works, but I can not imagine just passing in all the ghc-options makes sense, but rather parse out the Language Extensions we care about and passing them to Ormolu.

[Avi-D-coder]

@fendor currently I'm filtering everything except plugins, preprocessor and extensions.

Unless I missed something last time I took a look at getting strings out of DynFlags it would require abunch of manually mappings. Perhaps we could cache the GHC option strings directly?

[fendor]

Last issue from my point of view, other than that LGTM!

[fendor]

@Avi-D-coder Caching the ghc options directly is possible, too.

[Avi-D-coder]

@alanz ping

[DavSanchez]

Thanks for all the help guys! It’s been a great ride contributing with Haskell. A lot to learn yet...

[jneira]

@DavSanchez thank you for your contribution 👌