Avoid runtime dependency involving brittany

[jneira]

• Last step to being able to provide hie binaries (see Package hie binaries #1068)
• From that issue:
  • brittany did the necessary changes (thanks to @lspitzner) to let us avoid that dependency
  • @fendor tried to implement the change (see Package hie binaries #1068 (comment))
  • As @alanz commented the change should be

Then all we have to do is pass in the already-loaded typechecked (or parsed) module and we are good to go. We can ignore all the stuff about context, libdirs, etc

As a temporary alternative we could provide the binaries with a warning about brittany (we have other two formatters providers) or, if it exists, provide a manual workaround to make it work

[lspitzner]

The new items in the brittany API are so far only on a feature branch, as I was hoping on confirmation that the new API works for this. With some more comments, the new exposed function is:

pPrintModule
  :: Config -- ^ brittany config, same as before.
            -- The previous brittany plugin code should know how to
            -- obtain this.
  -> ExactPrint.Anns
  -> GHC.ParsedSource -- ^ these two basically come out of exactprint.
       -- see https://hackage.haskell.org/package/ghc-exactprint-0.6.2/docs/Language-Haskell-GHC-ExactPrint-Parsers.html
       -- brittany has some glue code around this, but HIE can and should rely on
       -- its own methods to make this work.
  -> ([BrittanyError], TextL.Text)
       -- ^ This again is the same as for the "old" API. The existing brittany plugin
       -- knows what to do with it.

I have not looked at the HIE sources in a while, so I have no idea how things are set up. Judging from

pass in the already-loaded typechecked (or parsed) module

you should have a GHC.ParsedSource value available (per module). The Anns could be harder, depending on whether you grab/save that while parsing already. If not, you might have to carry that around in addition to the ParsedSource.

(The additional glue code in brittany is mostly about setting up extensions to even allow parsing. If HIE already has parsing, you should be good to go.)

I currently won't try building HIE, but if you run into specific errors in exactprint/ghc/brittany API usage I might be able to help.

[fendor]

@lspitzner thank you for this function!
I attempted to implement this but failed due to the ExactPrint.Anns parameter which I could not obtain.

Moreover, I was struggling to see how a different feature can be implemented with this: format only a source code selection.
Should we extract the AST nodes ourselves and pass this only to pPrintModule? Or would that feature not work in combination with this API?

[lspitzner]

due to the ExactPrint.Anns parameter which I could not obtain.

Ah yeah, I feared that would be the case. I'd have to start digging into HIE source too to figure this out. Maybe @alanz can give a hint?

[lspitzner]

Should we extract the AST nodes ourselves and pass this only to pPrintModule? Or would that feature not work in combination with this API?

The simple answer is: Yeah, you could just do that. In my current editor I can select one declaration and format it, which just treats a selection as an (anonymous, implicit) module. However this breaks in two fashions: 1) per-module extensions are not respected 2) inline brittany config is not respected.

The first item is not a problem here, because you select parts from an already-parsed syntax tree. The other item is a bit more complex. An example:

-- brittany-next-binding --columns 120
foo :: Int -> IO ()
foo x = do
  lots of code here

now if the user wants to format just the foo x = do {..} bit, can we manage to still have the "120 columns" config apply (as it would when you'd format the whole module)?

---

So the proper solution definitely requires one more function in the brittany API. Something like "here is the full module AST, all the ANNs, but please only return the formatted version of this node in the syntax tree/this binding/this declaration". Should make it a bit less fiddly for you to implement, too.

This will still be tricky around empty lines and comments before and after the part-to-be-formatted, but that should be manageable.

[lspitzner]

For the last point, consider

abc = 13

-- some comment
def = print True

-- final comment

if you'd give brittany the whole module, but "please only format the def binding", what should it return? It could be

def = print True

or

-- some comment
def = print True

or

-- some comment
def = print True

or even

-- some comment
def = print True

-- final comment

because the final comment is attached to the True (is it? Would need to check. Only know that there is no next element that it could attach to, but it might be attached to the whole module.)

[lspitzner]

And in the end, HIE would need to figure out what part of the textual buffer to replace, too. Feels a bit like the proper return value is a diff/patch. Then the whitespace/comment question is easy to resolve. Or at least, HIE would not need to care, because it just applies the patch, and brittany could implement it however it wanted, presuming it produced a consistent patch.

[lspitzner]

@fendor One small follow-up question: Would it make sense to implement the "format just this function" functionality in the following manner: pass the whole module, together with a restriction "only format myUnformattedFunction", which returns the whole module with only the specified functioned changed? This trusts the formatter not to touch more (but then ensuring that on HIEs end seems to be hard/annoying (looking at source spans, replacing based on that?)

This depends on the API/protocol to the editor - not sure if "replace the whole file buffer with this" has downsides.

[Avi-D-coder]

Replace whole buffer does indeed have downsides (it breaks recorded type info and vim undo chains). We should really be generating granular changes by diffing the output of all formatters. I have not had time to look in to doing this.

[fendor]

@lspitzner I think this sounds like a very reasonable API!
I also think that we can obtain granular diffs between the original and new changes.

[alanz]

I would have to check, but I think the change gets turned into a diff for the applyedit. So if it only changes certain parts, only those will show up as the edit.