prepend decl tests

[santiweight]

Hey @alanz :)

I tried out adding something that I would want (not need) for HLS plugins: prepending a declaration onto a list of declarations. I also wanted to try this out to see if I'm being sane in general when using the API.

Let me know if this feature seems valuable, in which case I will add it to the high-level API in the Transform module. The goal is simply to allow adding a declaration at the start of a where clause, which is a nice middle-ground of simple but non-trivial.

Hopefully, my using the API can be helpful to you as feedback - so please do ask lots of questions :)

Some things I bumped into:

• I couldn't see a way to run a single test: I instead had to run the entire test suite every time
• There appears to be no formatter configured on the repo. It would help me produce nicer code and reduce conflicts long-term
• The repo has a lot of large files - would it be okay to chop them up?

[alanz]

Hi @santiweight

I am glad to see you jumping in on this stuff.

Firstly, adding a decl and managing the comments around it is intended to be something that "just works", and probably has the most, terribly hairy code devoted to it. As such, you should be modelling your tests on the transformHighLevelTests, the various addLocalDeclXX ones.

In essence, hsDecls and replaceDecls is intended to seamlessly manage the top level decls, and modifyValD to manage the local binds, where is is given a list of decls (converted automatically to/from binds), and returns the modified ones. Apart from setting the entry DP, it should not be necessary to do anything else, and if it is wrong then the ghc-exactprint Transform module should be fixed. Which is a terribly arduous task, full of ad-hoc cases.

I couldn't see a way to run a single test: I instead had to run the entire test suite every time

I generally run cabal repl test, load tests/Test.hs and then run a function called tt', which I edit to have the (single) test I am focusing at a time.

If you look at it you will see the commented out debris from prior tests. The short form

• mkTestModChange libdir rmDecl1 "RmDecl1.hs" calls the rmDecl changer function on RmDecl1.hs in the tests/examples/transform directory
• mkParserTest libdir "ghc92" "TopLevelSemis1.hs" does a simple roundtrip test on tests/examples/ghc92/TopLevelSemis1.hs
• mkParserTestBC libdir "ghc92" "TopLevelSemis1.hs" does a roundtrip, but calls balanceComments on the parsed AST first, to make sure it is idempotent
• mkParserTestMD libdir "ghc92" "TopLevelSemis1.hs" does a roundtrip, but calls makeDelta on the parsed AST first, to make sure it is idempotent

All of these generate a file next to the original source file with a suffix of .out, e,g, tests/examples/ghc92/TopLevelSemis1.hs.out. The first part is the output of exact printing, the second part is the AST that was printed. I generally diff them to see where the problem lies, then look up that row/col in the following AST to see exactly what AST item is the problem.

There appears to be no formatter configured on the repo. It would help me produce nicer code and reduce conflicts long-term

I am currently writing a lot of rust code, and autoformatting. It is a liberating experience, and I feel frustrated not being able to do it on the haskell code. So in principle I am not against it. We just need to be careful about timing, as I currently have the master branch tracking the current release, and ghc-head which was synced against GHC utils/check-exact in !8611. At the very least the latter two should be kept aligned.

The repo has a lot of large files - would it be okay to chop them up?

As per my other email, in principle fine, so long as we sync with GHC check-exact too.

[santiweight]

I'm excited to be involved :) There's a lot of potential here for HLS and I'm excited to be a guinea pig!

In essence, hsDecls and replaceDecls is intended to seamlessly manage the top level decls, and modifyValD to manage the local binds, where is is given a list of decls (converted automatically to/from binds), and returns the modified ones. Apart from setting the entry DP, it should not be necessary to do anything else, and if it is wrong then the ghc-exactprint Transform module should be fixed. Which is a terribly arduous task, full of ad-hoc cases.

I think I have some confusion here. For example, in my current case, I am attempting to prepend an LHsDecl GhcPs onto a bindings where clauses.

I tried a few things, and I came up with something that works-ish (this MR). But it's helpful for me to understand the things that don't work. If you could help me understand, for example, why this doesn't work:

addLocaLDecl7 :: Changer
addLocaLDecl7 libdir top = do
  Right (L ld (ValD _ decl)) <- withDynFlags libdir (\df -> parseDecl df "decl" "nn = 2")
  let decl' = setEntryDP (L ld decl) (DifferentLine 1 5)
      doAddLocal = do
        let lp = makeDeltaAst top
        ds <- balanceCommentsList =<< hsDecls lp
        (unzip -> (ds', _)) <- flip mapM ds $ \d -> do
          modifyValD (getLocA d) d $ \ _m ds -> pure (wrapDecl decl': ds, Just ())
        replaceDecls lp $ ds'
  let (lp',_,w) = runTransform doAddLocal
  debugM $ "addLocaLDecl7:" ++ intercalate "\n" w
  return lp'
==============
d1 = 1
  where -- c1
        w1 = 1
------>
d1 = 1
  where
     nn = 2 -- c1
             w1 = 1

d2 = 1
  where w2 = 1
------>
d2 = 1
  where
     nn = 2 w2 = 1

I'm being a little obtuse, since in this example I've put no effort into fixing the anchors. But if we start here, what exactly is the right approach to make it work? From my perspective, I feel like I have to implement what I did in this MR (prependDecl :: LHsDecl GhcPs -> [LHsDecl GhcPs] -> [LHsDecl GhcPs]) so I can set the right anchors based on various difference situations...

At face value, it appears that modifyValD is not handling things correctly, since the original declaration is using the same anchor as before. I'm also thinking that if I add a new decl to a list of original decls, the anchor on the new decl is somewhat irrelevant, and it should instead conform to how the original decls were anchored, but that appears to not be the case.

Is the right approach to alter the anchors prior to returning the new list of decls in modifyValD, or to alter the machinery within modifyValD, which appears to be replaceDeclsValbinds, to handle the cases gracefully as I have tried to do in this MR?

So in principle I am not against it. We just need to be careful about timing

Thank you for explaining the repo wrt tests and formatting etc. I'll let you tell me when the right time is to add formatting. It has been helpful to understand better what your current approach is, and I feel much more ready to contribute.

[alanz]

@santiweight I took a quick look at this.

The heart of the modifyValD function is

https://github.com/alanz/ghc-exactprint/blob/3b36f5d0a498e31d882fe111304b2cf5ca6cad22/src/Language/Haskell/GHC/ExactPrint/Transform.hs#L1199-L1208

and in turn the key working parts of that is

• a call to hsDecls on the Match statement to get the binds, converted from a list of binds and signatures into an ordered list ofHsDecls. Remember that a HsDecl is just a wrapper around all the things that can occur at the top level, and carries no other information except to tag them.
• calling the user-supplied transformation (f) on these decls
• calling replaceDecls to undo the operation from hsDecls, but also to manage the surrounding context when returning them.

So the work done in your prependDecls function actually belongs somewhere in the replaceDecls implementation for a Match, ie https://github.com/alanz/ghc-exactprint/blob/3b36f5d0a498e31d882fe111304b2cf5ca6cad22/src/Language/Haskell/GHC/ExactPrint/Transform.hs#L932-L948

And in fact probably somewhere in the implementation of replaceDeclsValbinds.

Some other hints

• I always configure with cabal configure -fdev --enable-tests, which means loading tests/Test.hs in GHCI reloads the lib if it has been modified
• Setting Language.Haskell.GHC.ExactPrint.Utils.debugEnabled to true (by editing the file, comment out the appropriate line) enables some detailed info while it runs. Only really useful if running one test with tt'

[santiweight]

Thanks - that's exactly what my impression was, but I didn't want to put the time in before you could confirm my approach. I will put the time in now :)

[santiweight]

I've been thinking about this for a bit, but I'm worried about adding the logic to replaceDecls as you suggest.

When we receive the new list of decls, it makes a lot of sense to automatically handle the anchor for adding a new where if needed, and to be faithful to the indentation provided by the new decls, which is what the name replaceDecls suggests to me.

---

But let's look at the follow two functions:

• prependDecl has quite a clear goal: take a list decls, and prepend a new decl while maintaining the user's indentation.

• appendDecl, which adds a new decl to the end of a group of decls while still maintaining the user's indentation.

I don't see how both of these functions could be supported by a reasonable implementation of replaceDecls. For example, it's unclear how the following two implementations could do what I expect:

prependDecl newDecl foo = do
    [d] <- hsDecls
    replaceDecls [newDecl, d] foo
----------
appendDecl foo newDecl = do
      [d] <- hsDecls foo
      replaceDecls [d, newDecl] foo

For my prependDecls, the new decls' indentation conforms to the old decl d. But my intention for the second would be to follow the indentation of the old d. It seems like interpreting each of these cases differently is asking for overly-complicated code.

The only way to do this is to do would be to perform RealSrcSpan comparisons and check which are the old and new decl, but that is no different from exposing a collection of new functions prependDecl and appendDecl, and I think the latter API is much clearer and easier to test and maintain.

Finally, what if I want to implement prependDeclIgnoringOldIndentation, it's unclear how I could do this if replaceDecls is faithful to the old decls' indentation by default!

---

My current thinking is to keep replaceDecls as it is right now: a helper function that removes the need to manually insert where and let keywords into an expr's/match's subdecls. Then other functions could use this logic and apply interesting refactors, such as my prependDecl and appendDecl. It also seems really useful to just have a function that naively replaces the subdecls of an expression without messing with the decls provided.

Obviously, I'm quite new to GHC's exactprint, so this is only my newbie thoughts. Perhaps it'd be good to hop on a call? I feel like I would gain understanding quicker that way!