I've detected that the ghc-8.10 path that uses the new applyRefactoring' is dropping language pragmas from the module header. 😟
Calling hlint Module.hs --refactor works so i guess something is missing from ghc-exactprint annotations

rebased over the tests recently added in master

Ok, tests are passing for ghc <= 8.8
About the lose of language pragmas, i guess the parsed module we are getting from ghcide does not have them (or doesn't in a way apply-refact is able to include in the output).
apply-refact is using ghc-exactprint to parse the module source file: https://github.com/alanz/ghc-exactprint/blob/81c2ef9cdcf284724df0d66ef1fe5616fdc8ae6c/src/Language/Haskell/GHC/ExactPrint/Parsers.hs#L293 and it is preserving language pragmas with this code path

We have two options:

• to have in the shake graph a version of the parsed module extracted from ghc-exactprint while it is not integrated in ghc itself (and keep it for previous versions)
  • or having a version of the parsed module preserving them (if it is not doing it already)
• try to recover language pragmas in the plugin with existing info

As expected, the ghc-8.10 code path does not preserve normal comments 😟
The behaviour is weird indeed:

• It removes inline comments ({- -}) entirely (so like pragmas)
• It removes regular comments starting with -- entirely
• It removes haddock comment tokens -- | but keeps the comment itself, duplicating it 😕

 FAIL (1.77s)
        test/functional/FunctionalCodeAction.hs:186:
        expected: "-- comment before header\nmodule ApplyRefact6 where\n\n{-# standalone annotation #-}\n\n-- standalone comment\n\n-- | haddock comment\nf = {- inline comment -} 1 -- ending comment\n\n-- final comment\n"
         but got: "\nmodule ApplyRefact6 where\n\n\n\n\n\n haddock comment haddock comment\nf =                      1\n\n\n"

maybe any ghc-exactprint expert could help me to find the way to preserve them? 🙏
//cc @alanz @zliu41

ok, i think i know the cause of mising comments:

• getParsedModule parses the source file with Opt_Haddock to get its nice comments over definitions
  • but that option is not compatible with Opt_KeepRawTokenStream, as warned in ghcide getParsedModuleRule:

-- | IMPORTANT FOR HLINT INTEGRATION:
-- We currently parse the module both with and without Opt_Haddock, and
-- return the one with Haddocks if it -- succeeds. However, this may not work
-- for hlint, and we might need to save the one without haddocks too.
-- See https://github.com/haskell/ghcide/pull/350#discussion_r370878197
-- and https://github.com/mpickering/ghcide/pull/22#issuecomment-625070490

links: haskell/ghcide#350 (comment), and mpickering/ghcide#22 (comment), ghc wiki about https://gitlab.haskell.org/ghc/ghc/-/wikis/api-annotations

• otoh apply refactorings has no position informed, to avoid mismatches between the hint and the refactoring, so the entire file is being replaced by the refactor, so the edit must be perfect:

So the plan would be

• Create a rule to get the ParsedModule with all comments using Opt_KeepRawTokenStream (the default i guess), i would name it GetParsedModuleWithComments or GetParsedModuleKeepRawTokenStream
• Try to be more precise with the scope of the refactoring, maybe taking all the line or the line from the hint position instead the entire file

Create a rule to get the ParsedModule with all comments using Opt_KeepRawTokenStream (the defaul i guess), i would name it GetParsedModuleWithComments or GetParsedModuleKeepRawTokenStream

That seems very sensible. Have two graph nodes which are the parsed module with Opt_KeepRawTokenStream and with Opt_Haddock as separate entities with distinct names. Then you have a single "I don't care" node which is haddock if you have it, otherwise not.

Try to be more precise with the sope of the refactoring, maybe taking all the line or the line from the hint position instead the entire file

I think this is a matter of heuristics. Is the case you are worried about having CPP? I would play around and see what works best, but I think its inevitable that you aren't going to be able to do all cases every time.

I think this is a matter of heuristics. Is the case you are worried about having CPP? I would play around and see what works best, but I think its inevitable that you aren't going to be able to do all cases every time.

A possible heuristic could be take the smallest source span between:

• the closest cpp #if ... #endif block
• the closest top level definition

Unless a refactoring could cross those boundaries...
Not sure how to query the actual info to get those source spans 🤔

In any case i would try to do it in a new pr.

Ok, tests now are green so the pr could be reviewed

I am gonna merge this as is, if nobody disagree.
There is an unexpected behavior with comments in apply refact but hopefully it will be resolved before the next release.

The build is failing with

 session-loader/Development/IDE/Session/VersionCheck.hs:17:24: error:
    • Exception when trying to run compile-time code:
        I could not find a GHC installation at /home/runner/.ghcup/ghc/8.10.3/lib/ghc-8.10.3. Please do a clean rebuild and/or reinstall GHC.
CallStack (from HasCallStack):
  error, called at src/GHC/Check.hs:166:7 in ghc-check-0.5.0.3-a5c4fe466b9db954d5acec57d6f2911207e4ee3d449b9c03333ebfde2286e811:GHC.Check
      Code: makeGhcVersionChecker
              (fromMaybe GHC.Paths.libdir <$> lookupEnv "NIX_GHC_LIBDIR")
    • In the Template Haskell splice
        $$(makeGhcVersionChecker
             (fromMaybe GHC.Paths.libdir <$> lookupEnv "NIX_GHC_LIBDIR"))
      In the expression:
        $$(makeGhcVersionChecker
             (fromMaybe GHC.Paths.libdir <$> lookupEnv "NIX_GHC_LIBDIR"))
      In an equation for ‘ghcVersionChecker’:
          ghcVersionChecker
            = $$(makeGhcVersionChecker
                   (fromMaybe GHC.Paths.libdir <$> lookupEnv "NIX_GHC_LIBDIR"))
   |
17 | ghcVersionChecker = $$(makeGhcVersionChecker (fromMaybe GHC.Paths.libdir <$> lookupEnv "NIX_GHC_LIBDIR"))
   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

ghc was already installed in other location:

 Preparing to setup a Haskell environment
Resolved 3.2 to 3.2.0.0
Installing ghc version 8.10.3
  Found ghc 8.10.3 in cache at path /opt/ghc/8.10.3/bin. Setup successful.

So i think ghc was installed using ppa:hvr and the haskell-setup is not longer using the runtime installation using ghcup.
But the previous libdir has been cached in the ghc-paths installation in the cabal store.

Weird thing, i wonder if the session checker should be able to use the libdir returned by the cradle to avoid this kind of errors
// cc @pepeiborra

I am gonna update the hackage index in cabal project, to invalidate the cache.

The cache mechanism has ignored the change in the cabal.project, it is using the same cache key:

Cache restored from key: Linux-8.10.3-build-983ae526727aa0f660de5830ca5d98226b86c55f1bba83af04420296d5f3b317

But afaiu the cache had to be invalidated due to ${{ hashFiles('cabal.project') }}:

I think the restore key ${{ runner.os }}-${{ matrix.ghc }}-build is hitting just the cache i want to invalidate 🤦

Add the ghc libdir in the cache could be a way to fix it but i am not sure if it is a good idea.

Adding the ghc lib dir to the cache key does sound like a good idea

well it only will be an issue when using a new ghc version and I think the right solution could be

i wonder if the ghc session checker should be able to use the libdir returned by the cradle to avoid this kind of errors

I want to remove the hardcoded libdir from ghc-exactprint.

will do in a new pr if needed