Skip parsing without haddock for above GHC9.0 #2338

yoshitsugu · 2021-11-10T08:45:19Z

For #1892
Just skip by #if, is it enough for this issue? 🤔

jneira · 2021-11-10T08:51:13Z

For #1892 Just skip by #if, is it enough for this issue? thinking

Thanks for the pr, i think we could use runtime checks instead cpp as both branches compile with all ghcs

Runtime checks are here:

haskell-language-server/ghcide/src/Development/IDE/GHC/Compat.hs

Lines 277 to 299 in 7011d5e

    
           data GhcVersion 
        
             = GHC86 
        
             | GHC88 
        
             | GHC810 
        
             | GHC90 
        
             | GHC92 
        
             deriving (Eq, Ord, Show) 
        
           ghcVersionStr :: String 
        
           ghcVersionStr = VERSION_ghc 
        
           ghcVersion :: GhcVersion 
        
           #if MIN_VERSION_GLASGOW_HASKELL(9,2,0,0) 
        
           ghcVersion = GHC92 
        
           #elif MIN_VERSION_GLASGOW_HASKELL(9,0,0,0) 
        
           ghcVersion = GHC90 
        
           #elif MIN_VERSION_GLASGOW_HASKELL(8,10,0,0) 
        
           ghcVersion = GHC810 
        
           #elif MIN_VERSION_GLASGOW_HASKELL(8,8,0,0) 
        
           ghcVersion = GHC88 
        
           #elif MIN_VERSION_GLASGOW_HASKELL(8,6,0,0) 
        
           ghcVersion = GHC86 
        
           #endif

Example here:

haskell-language-server/ghcide/session-loader/Development/IDE/Session.hs

Line 529 in 613ec40

when (Compat.ghcVersion < Compat.GHC90) $

yoshitsugu · 2021-11-10T08:52:36Z

@jneira oh i see. i will fix to use runtime check

jneira · 2021-11-10T13:29:01Z

Looks good to me but i would like to know @pepeiborra thoughts

pepeiborra · 2021-11-10T14:25:54Z

This looks reasonable to me, but does it actually work?

Do we still collect haddocks in GHC 9? There seems to be some problem here, according to the testsuite.
Do we no longer parse twice in GHC 9 when there is a Haddock error? The testsuite cannot verify this, but maybe you can use a trace to verify empirically.

yoshitsugu · 2021-11-12T04:17:55Z

Sorry but I don't have enough knowledge to answer @pepeiborra' s questions. Could anyone give me any advice where to start to investigate about these questions? ( @jneira )

* sort completions * add an example * Include fuzzy scores in completions sort text * hlints * Extend completion documentation to inform whether an identifier is alreaady imported * Ditch alphabetical ordering - it's incompatible with qualified completions * Fix bugs in completion help text This fixes the ugly "Imported from 'Just B'" and other inconsistencies * added tests for qualified completions * Fix redundant import * Inline Fuzzy.match to apply [1] and to be case-sensitive on first match [1] - joom/fuzzy#4 * fixup! Fix bugs in completion help text * Sort qualified completions first * Filter out global suggestions that overlap with local For example, don't suggest GHC.Exts.fromList when Data.Map.fromList is in scope alraedy * Sort completions alphabetically * Show provenance in detail text * Sort local/in-scope completions first * Fix build with GHC 9 * Ignore func symbol tests Co-authored-by: Alex Naspo <[email protected]> Co-authored-by: Javier Neira <[email protected]>

jneira · 2021-11-12T06:24:26Z

Sorry but I don't have enough knowledge to answer @pepeiborra' s questions. Could anyone give me any advice where to start to investigate about these questions? ( @jneira )

Hi, well there is a failing test which seems to be related with the pr:

https://github.com/haskell/haskell-language-server/runs/4165727646?check_suite_focus=true

ghcide
  get
    hover
      documentation                   #1129: FAIL (1.19s)
        test/exe/Main.hs:3705:
        failed to find: `Recognizable docs: kpqz` in hover message:
        
        '''haskell
        documented :: forall (m :: * -> *) a. Monad m => Either Int (m a)
        '''
        
        *Defined at /tmp/extra-dir-14222270143316/GotoHover.hs:46:1*
        
        Use -p '/documentation                   #1129/' to rerun this test only.

As mentioned by pepe maybe we were relying in the double parsing to effectively get docs so removing it seems to be not enough, i am afraid. The test fail has to be investigated and fixed.

Related with that is the second question. Add temporary some traces (using Debug.Trace for example) could help to:

know why the test is failing (so to observe the behaviour when there are docs without errors, which should be collected and send in the hover information to the client)
confirm we are not double parsing in front of docs with errors so we should get the test case with haddock errors and check the traces added inside the else branch are not printed.

yoshitsugu · 2021-11-12T08:06:35Z

@jneira i see. thank you for your kind comment. i'll try.

yoshitsugu

@pepeiborra @jneira
I've fixed code and confirmed ghcide test code passed in my local machine. (It seems to fail testing in Windows environment on GitHub Actions, but I don't know whether it's required to pass. Please notify me if I need to confirm the Windows environemnt.)

For question 2, I confirmed not to parse twice in GHC 9.0.1 with Debug.Trace by inserting trace to getParsedModuleDefinition function.

Please point out if I misunderstood something or need to do something else.

yoshitsugu · 2021-11-13T10:38:05Z

ghcide/src/Development/IDE/Core/Rules.hs

+    res@(_,pmod) <- if Compat.ghcVersion >= Compat.GHC90 then
+      liftIO $ (fmap.fmap.fmap) reset_ms $ getParsedModuleDefinition hsc opt file (withOptHaddock ms)


I fixed my code to collect haddock information even if Opt_Haddock returns False.
It seems to do so for display haddock information in the hover as the failed test suites indicated.
(for question 1)

That's right, with GHC >9.0 we want to parse with Opt_Haddock on always

pepeiborra · 2021-11-13T12:40:15Z

ghcide/src/Development/IDE/Core/Rules.hs

+                -- non-interest are always parsed with Haddocks
+                -- If we can parse Haddocks, might as well use them
+                --
+                -- HLINT INTEGRATION: might need to save the other parsed module too


this comment is confusing , I think it can be deleted since GetParsedModuleWithComments was introduced. What do you think @jneira ?

yeah, it was even not hlint specific and can be removed

I've removed this comment 👍

pepeiborra

The test failure looks unrelated, testsuite is flaky in Windows. I don't see any problems with the change, looks good. Thanks for testing @yoshitsugu !

…ub.com/yoshitsugu/haskell-language-server into feature/skip_parse_without_haddock_ghc90

jneira · 2021-11-15T05:51:23Z

circleci build is stuck, it says there is no circleci config, which is not true, let's see what is doing in the next branch update

jneira · 2021-11-15T20:33:52Z

I've opened a ticket with circleci support but the issue is not resolved so i am gonna merging it manually, as gh workflow is green

Ailrun · 2021-11-15T20:36:18Z

@jneira Sometimes it does, and you can reactivate it by close and reopen PR.

yoshitsugu · 2021-11-15T22:01:21Z

@pepeiborra @jneira thanks for advising me!

jneira · 2021-11-15T22:31:44Z

@jneira Sometimes it does, and you can reactivate it by close and reopen PR.

yeah, but this time create a manual workflow run in all pr's fails due to a false positive:

We have detected an anomaly that violates the CircleCI Terms of Service.
We did not test this push because the project has been suspended. Please contact us if you think this is a mistake.

https://app.circleci.com/pipelines/github/haskell/haskell-language-server/4422/workflows/5aa08e6e-359e-4da8-8b8a-ed7a810935fe/jobs/38455

* Skip parsing without haddock for above GHC9.0 * Use runtime ghc version check * Need parse twice in getParsedModuleRule * Include sortText in completions and improve suggestions (#2332) * sort completions * add an example * Include fuzzy scores in completions sort text * hlints * Extend completion documentation to inform whether an identifier is alreaady imported * Ditch alphabetical ordering - it's incompatible with qualified completions * Fix bugs in completion help text This fixes the ugly "Imported from 'Just B'" and other inconsistencies * added tests for qualified completions * Fix redundant import * Inline Fuzzy.match to apply [1] and to be case-sensitive on first match [1] - joom/fuzzy#4 * fixup! Fix bugs in completion help text * Sort qualified completions first * Filter out global suggestions that overlap with local For example, don't suggest GHC.Exts.fromList when Data.Map.fromList is in scope alraedy * Sort completions alphabetically * Show provenance in detail text * Sort local/in-scope completions first * Fix build with GHC 9 * Ignore func symbol tests Co-authored-by: Alex Naspo <[email protected]> Co-authored-by: Javier Neira <[email protected]> * Give unique names to post-jobs (#2337) * Restore comment * Parse only with Haddock above GHC90 * Remove obsolete comment Co-authored-by: Pepe Iborra <[email protected]> Co-authored-by: Alex Naspo <[email protected]> Co-authored-by: Javier Neira <[email protected]>

Skip parsing without haddock for above GHC9.0

981635d

yoshitsugu force-pushed the feature/skip_parse_without_haddock_ghc90 branch from 2e12e20 to 981635d Compare November 10, 2021 08:48

Use runtime ghc version check

c225c64

jneira requested a review from pepeiborra November 10, 2021 11:55

yoshitsugu and others added 3 commits November 12, 2021 15:05

Need parse twice in getParsedModuleRule

50a50b8

Give unique names to post-jobs (haskell#2337)

7ce5681

yoshitsugu force-pushed the feature/skip_parse_without_haddock_ghc90 branch from 2f74519 to 7ce5681 Compare November 12, 2021 06:11

yoshitsugu added 2 commits November 12, 2021 15:12

Merge branch 'master' into feature/skip_parse_without_haddock_ghc90

946812b

Restore comment

5406cf9

yoshitsugu added 2 commits November 13, 2021 05:27

Parse only with Haddock above GHC90

d7b8e18

Merge branch 'master' into feature/skip_parse_without_haddock_ghc90

760a730

yoshitsugu commented Nov 13, 2021

View reviewed changes

pepeiborra reviewed Nov 13, 2021

View reviewed changes

pepeiborra approved these changes Nov 13, 2021

View reviewed changes

yoshitsugu added 3 commits November 15, 2021 09:28

Remove obsolete comment

44bdb81

Merge branch 'feature/skip_parse_without_haddock_ghc90' of ssh://gith…

2c7bc24

…ub.com/yoshitsugu/haskell-language-server into feature/skip_parse_without_haddock_ghc90

Merge branch 'master' into feature/skip_parse_without_haddock_ghc90

ac06a10

jneira merged commit 4e2a99e into haskell:master Nov 15, 2021

jneira linked an issue Nov 18, 2021 that may be closed by this pull request

[GHC 9.0] No need to parse without Haddocks #1892

Closed

jneira mentioned this pull request Nov 18, 2021

[GHC 9.0] No need to parse specially for hlint? #1891

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip parsing without haddock for above GHC9.0 #2338

Skip parsing without haddock for above GHC9.0 #2338

yoshitsugu commented Nov 10, 2021 •

edited

Loading

jneira commented Nov 10, 2021

yoshitsugu commented Nov 10, 2021

jneira commented Nov 10, 2021

pepeiborra commented Nov 10, 2021

yoshitsugu commented Nov 12, 2021

jneira commented Nov 12, 2021

yoshitsugu commented Nov 12, 2021

yoshitsugu left a comment

yoshitsugu Nov 13, 2021

pepeiborra Nov 13, 2021

pepeiborra Nov 13, 2021

jneira Nov 13, 2021

yoshitsugu Nov 15, 2021

pepeiborra left a comment

jneira commented Nov 15, 2021

jneira commented Nov 15, 2021

Ailrun commented Nov 15, 2021

yoshitsugu commented Nov 15, 2021

jneira commented Nov 15, 2021

		res@(_,pmod) <- if Compat.ghcVersion >= Compat.GHC90 then
		liftIO $ (fmap.fmap.fmap) reset_ms $ getParsedModuleDefinition hsc opt file (withOptHaddock ms)

Skip parsing without haddock for above GHC9.0 #2338

Skip parsing without haddock for above GHC9.0 #2338

Conversation

yoshitsugu commented Nov 10, 2021 • edited Loading

jneira commented Nov 10, 2021

yoshitsugu commented Nov 10, 2021

jneira commented Nov 10, 2021

pepeiborra commented Nov 10, 2021

yoshitsugu commented Nov 12, 2021

jneira commented Nov 12, 2021

yoshitsugu commented Nov 12, 2021

yoshitsugu left a comment

Choose a reason for hiding this comment

yoshitsugu Nov 13, 2021

Choose a reason for hiding this comment

pepeiborra Nov 13, 2021

Choose a reason for hiding this comment

pepeiborra Nov 13, 2021

Choose a reason for hiding this comment

jneira Nov 13, 2021

Choose a reason for hiding this comment

yoshitsugu Nov 15, 2021

Choose a reason for hiding this comment

pepeiborra left a comment

Choose a reason for hiding this comment

jneira commented Nov 15, 2021

jneira commented Nov 15, 2021

Ailrun commented Nov 15, 2021

yoshitsugu commented Nov 15, 2021

jneira commented Nov 15, 2021

yoshitsugu commented Nov 10, 2021 •

edited

Loading