Leverage last apply-refact improvements in hlint plugin (include getParsedModuleWithComments in ghcide) #635

jneira · 2020-11-28T22:15:47Z

This pr take advantage of recent improvements in apply-refact, courtesy of @zliu41:

for ghc<8.10 we can pass ghc extensions used by the hls ghc session to apply-refact and make it work. So this would fix "Apply hint" code action for hlint does not work with default-extensions #590
- Issue: Allow apply refactorings to an already parsed module mpickering/apply-refact#91
- pr: Make applyRefactoring take GHC extensions mpickering/apply-refact#98 (and Make applyRefactorings take [String] instead of ([Extension], [Extension]) mpickering/apply-refact#99)
for ghc-8.10 we can pass directly the already parsed module instead use a intermediate file that apply-refact has to parse again
- it will avoid all problems of reparsing the source file, including the previous one
- pr: Add applyRefactorings' mpickering/apply-refact#100
there had been some bug fixes:
- Do not apply a refactoring if it drops comments mpickering/apply-refact#101 -> it will alliviate Applying hlint deindents where clause and remove comments #597
- Set annSortKey properly for eta-reduce with local binds mpickering/apply-refact#96 -> closes Applying hlint deindents where clause and remove comments #597

We should use a release version of apply-refact, if possible

plugins/hls-hlint-plugin/src/Ide/Plugin/Hlint.hs

jneira · 2020-12-02T07:58:03Z

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

jneira · 2020-12-19T11:46:05Z

rebased over the tests recently added in master

jneira · 2020-12-23T06:30:23Z

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

jneira · 2020-12-23T08:00:48Z

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

jneira · 2020-12-28T10:58:52Z

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:

haskell-language-server/plugins/hls-hlint-plugin/src/Ide/Plugin/Hlint.hs

Lines 342 to 356 in 9ac127e

    
           -- set Nothing as "position" for "applyRefactorings" because 
        
           -- applyRefactorings expects the provided position to be _within_ the scope 
        
           -- of each refactoring it will apply. 
        
           -- But "Idea"s returned by HLint point to starting position of the expressions 
        
           -- that contain refactorings, so they are often outside the refactorings' boundaries. 
        
           -- Example: 
        
           -- Given an expression "hlintTest = reid $ (myid ())" 
        
           -- Hlint returns an idea at the position (1,13) 
        
           -- That contains "Redundant brackets" refactoring at position (1,20): 
        
           -- 
        
           -- [("src/App/Test.hs:5:13: Warning: Redundant bracket\nFound:\n  reid $ (myid ())\nWhy not:\n  reid $ myid ()\n",[Replace {rtype = Expr, pos = SrcSpan {startLine = 5, startCol = 20, endLine = 5, endCol = 29}, subts = [("x",SrcSpan {startLine = 5, startCol = 21, endLine = 5, endCol = 28})], orig = "x"}])] 
        
           -- 
        
           -- If we provide "applyRefactorings" with "Just (1,13)" then 
        
           -- the "Redundant bracket" hint will never be executed 
        
           -- because SrcSpan (1,20,??,??) doesn't contain position (1,13).

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

ndmitchell · 2020-12-29T13:34:05Z

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.

@zliu41

* applyRefactoring accepts ghc extensions to parse the file * new applyRefactoring' function accepts the parsed module Thanks @zliu41!

jneira · 2021-01-12T06:39:37Z

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.

jneira · 2021-01-12T09:41:03Z

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

pepeiborra

The ghcide changes lgtm

jneira · 2021-01-14T06:28:18Z

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.

jneira · 2021-01-14T12:19:40Z

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.

jneira · 2021-01-14T13:16:44Z

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') }}:

haskell-language-server/.github/workflows/test.yml

Lines 74 to 86 in a4627e5

    
                 - name: Cache Cabal 
        
                   uses: actions/cache@v2 
        
                   env: 
        
                     cache-name: cache-cabal 
        
                   with: 
        
                     path: | 
        
                       ${{ env.CABAL_PKGS_DIR }} 
        
                       ${{ env.CABAL_STORE_DIR }} 
        
                     key: ${{ runner.os }}-${{ matrix.ghc }}-build-${{ hashFiles('cabal.project') }} 
        
                     restore-keys: | 
        
                       ${{ runner.os }}-${{ matrix.ghc }}-bench-${{ hashFiles('cabal.project') }} 
        
                       ${{ runner.os }}-${{ matrix.ghc }}-build- 
        
                       ${{ runner.os }}-${{ matrix.ghc }}

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.

pepeiborra · 2021-01-14T14:21:20Z

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

jneira · 2021-01-14T15:22:12Z

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

pepeiborra reviewed Nov 28, 2020

View reviewed changes

plugins/hls-hlint-plugin/src/Ide/Plugin/Hlint.hs Outdated Show resolved Hide resolved

pepeiborra reviewed Nov 28, 2020

View reviewed changes

plugins/hls-hlint-plugin/src/Ide/Plugin/Hlint.hs Outdated Show resolved Hide resolved

jneira marked this pull request as draft December 2, 2020 07:58

jneira force-pushed the apply-refact-exts branch from b4c8d6a to 675ec23 Compare December 4, 2020 19:54

jneira mentioned this pull request Dec 15, 2020

Add hlint tests over cpp, extensions and ignore hints #674

Merged

jneira force-pushed the apply-refact-exts branch from 675ec23 to 9e30310 Compare December 19, 2020 11:45

jneira force-pushed the apply-refact-exts branch 4 times, most recently from a671a0e to 18d20e0 Compare December 22, 2020 15:20

jneira force-pushed the apply-refact-exts branch from b616aa5 to 1c56e0e Compare December 23, 2020 07:33

pepeiborra force-pushed the master branch from f10a411 to 66ada8c Compare December 29, 2020 10:39

pepeiborra force-pushed the master branch from 66ada8c to d4f5d43 Compare December 29, 2020 13:34

jneira force-pushed the apply-refact-exts branch from 1c56e0e to 1cb06ed Compare January 8, 2021 23:18

jneira added 10 commits January 11, 2021 14:24

Use last apply-refact for cabal

5be067e

Add ghc-exactprint as dependency

cf508ba

Leverage apply-refact improvements

9b6395f

* applyRefactoring accepts ghc extensions to parse the file * new applyRefactoring' function accepts the parsed module Thanks @zliu41!

Inline utility function

1416d62

Fix build for ghc < 8.10

aa2dd1a

Fix ghc-8.8 build

7328e3b

Reparse extensions to remove invalid ones

cffaa4f

Restore hlint test changing doc content

899230d

Remove knownBroken for ghc < 8.10

6db91f3

Rename GHC_LIB cpp option

d02380d

jneira added 2 commits January 11, 2021 14:24

Use ghcide with getParsedModuleWithComments in hlint

ceeeabc

Restore utility function

8a7f68c

jneira force-pushed the apply-refact-exts branch from 1cb06ed to 8a7f68c Compare January 11, 2021 13:24

Fix getParsedModuleWithComments and add comments

f17448e

jneira marked this pull request as ready for review January 12, 2021 09:41

jneira requested review from pepeiborra and ndmitchell January 12, 2021 09:41

jneira changed the title ~~Leverage last apply-refact improvements~~ Leverage last apply-refact improvements in hlint plugin (include getParsedModuleWithComments in ghcide) Jan 12, 2021

pepeiborra approved these changes Jan 12, 2021

View reviewed changes

jneira added 3 commits January 13, 2021 07:50

Used apply-refact HEAD

df20702

Use again apply-refact 4fbd3a

7144430

Use ghc-exactprint-0.6.3.3

88852f0

jneira force-pushed the apply-refact-exts branch from a10f03e to bb41ae6 Compare January 13, 2021 13:40

Test comment inside refactoring

decf082

jneira force-pushed the apply-refact-exts branch from bb41ae6 to decf082 Compare January 13, 2021 19:55

jneira added the merge me Label to trigger pull request merge label Jan 14, 2021

Merge branch 'master' into apply-refact-exts

8434386

Update hackage index to invalidate gha cache

c16f516

Invalidate cache using versioning

a0b3e4d

jneira merged commit 416ca36 into haskell:master Jan 14, 2021

expipiplus1 mentioned this pull request Jan 15, 2021

"Apply hint" code action for hlint does not work with default-extensions #590

Closed

jneira deleted the apply-refact-exts branch January 15, 2021 06:52

jneira mentioned this pull request Oct 10, 2021

Can't find project modules in test/Spec.hs until I restart VSCode #1909

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leverage last apply-refact improvements in hlint plugin (include getParsedModuleWithComments in ghcide) #635

Leverage last apply-refact improvements in hlint plugin (include getParsedModuleWithComments in ghcide) #635

jneira commented Nov 28, 2020

jneira commented Dec 2, 2020

jneira commented Dec 19, 2020

jneira commented Dec 23, 2020

jneira commented Dec 23, 2020 •

edited

Loading

jneira commented Dec 28, 2020 •

edited

Loading

ndmitchell commented Dec 29, 2020

jneira commented Jan 12, 2021 •

edited

Loading

jneira commented Jan 12, 2021

pepeiborra left a comment

jneira commented Jan 14, 2021 •

edited

Loading

jneira commented Jan 14, 2021 •

edited

Loading

jneira commented Jan 14, 2021 •

edited

Loading

pepeiborra commented Jan 14, 2021

jneira commented Jan 14, 2021 •

edited

Loading

Leverage last apply-refact improvements in hlint plugin (include getParsedModuleWithComments in ghcide) #635

Leverage last apply-refact improvements in hlint plugin (include getParsedModuleWithComments in ghcide) #635

Conversation

jneira commented Nov 28, 2020

jneira commented Dec 2, 2020

jneira commented Dec 19, 2020

jneira commented Dec 23, 2020

jneira commented Dec 23, 2020 • edited Loading

jneira commented Dec 28, 2020 • edited Loading

ndmitchell commented Dec 29, 2020

jneira commented Jan 12, 2021 • edited Loading

jneira commented Jan 12, 2021

pepeiborra left a comment

Choose a reason for hiding this comment

jneira commented Jan 14, 2021 • edited Loading

jneira commented Jan 14, 2021 • edited Loading

jneira commented Jan 14, 2021 • edited Loading

pepeiborra commented Jan 14, 2021

jneira commented Jan 14, 2021 • edited Loading

jneira commented Dec 23, 2020 •

edited

Loading

jneira commented Dec 28, 2020 •

edited

Loading

jneira commented Jan 12, 2021 •

edited

Loading

jneira commented Jan 14, 2021 •

edited

Loading

jneira commented Jan 14, 2021 •

edited

Loading

jneira commented Jan 14, 2021 •

edited

Loading

jneira commented Jan 14, 2021 •

edited

Loading