Skip to content
This repository has been archived by the owner on Jan 2, 2021. It is now read-only.

Add links to haddock and hscolour pages in documentation #699

Merged
merged 1 commit into from
Jul 27, 2020

Conversation

lukel97
Copy link
Collaborator

@lukel97 lukel97 commented Jul 15, 2020

Currently this only searches local documentation (generated with
cabal haddock --haddock-hyperlink-source or equivalent) but could be
extended to support searching via Hoogle in the future. And it works for
any of the core libraries since they come installed with documentation.
Will show up in hover and completions.

Also fixes extra markdown horizontal rules being inserted with no
content in between them.

https://streamable.com/59l6zo

The above example needs a patched version of the vscode plugin to open the url inside vscode, but hopefully other clients will be able to handle the link fine

df <- getSessionDynFlags
(docFp, srcFp) <-
case nameModule_maybe name of
Just mod -> liftIO $ do
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires calling into IO, which means now that after lukel97@7dc6e26 I'm not sure how to integrate this with doc local completions, which seems to be a function purely based off of the ParsedModule cc @pepeiborra

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Local completions can use IO so I presume that's not what is blocking you.

Local completions do not use the ghc api to extract documentation, i.e. they do not call getDocumentationTryGhc, since that requires a ModIface which is only available after type checking. Instead, they get the documentation from the parse tree by calling D.IDE.Spans.Documentation.getDocumentation.

So I think the problem is that there are two code paths to get documentation and this PR needs to be extended to cover both.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also needs to get access to the DynFlags which I'm not sure how to get outside of the GHC session, i.e. in getDocumentation. In localCompletionsForParsedModule

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to run this code for local completions anyway? Locally defined identifiers will not have any documentation links

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was thinking that too. The PR currently just ignores it in Logic.hs:

    mkComp n ctyp ty =
         CI ctyp pn thisModName ty pn Nothing doc (ctyp `elem` [CiStruct, CiClass])
       where
         pn = ppr n
         doc = SpanDocText $ getDocumentation [pm] n
         doc = SpanDocText (getDocumentation [pm] n) (SpanDocUris Nothing Nothing)

@lukel97 lukel97 force-pushed the haddock-links branch 2 times, most recently from e70eaf2 to dde671a Compare July 15, 2020 11:41
@pepeiborra
Copy link
Collaborator

Since this could have performance implications, could you run the benchmark suite (see https://github.com/digital-asset/ghcide/blob/master/README.md#hacking-on-ghcide) and share the differential results?

Currently this only searches local documentation (generated with
`cabal haddock --haddock-hyperlink-source` or equivalent) but could be
extended to support searching via Hoogle in the future. And it works for
any of the core libraries since they come installed with documentation.
Will show up in hover and (non-local) completions.

Also fixes extra markdown horizontal rules being inserted with no
content in between them.
@lukel97
Copy link
Collaborator Author

lukel97 commented Jul 21, 2020

@pepeiborra sorry for the delay, here are the results

version name success samples startup setup experiment maxResidency
upstream hover True 100 10.179730363000001 0.0 0.44325323600000005 176MB
upstream edit True 100 11.011219252 0.0 45.184126208 186MB
upstream getDefinition True 100 11.006734432 0.0 0.715105045 176MB
upstream hover after edit True 100 10.931513793 0.0 1.322317525 182MB
upstream completions after edit True 100 10.604500145000001 0.0 6.240387429 187MB
upstream code actions True 100 9.404200037 0.832277805 0.734758828 213MB
upstream code actions after edit True 100 10.008702262 0.0 20.878563351 215MB
upstream documentSymbols after edit True 100 16.197478229 0.0 1.7972922420000002 255MB
HEAD hover True 100 7.593396530000001 0.0 0.452229941 177MB
HEAD edit True 100 7.665461937000001 0.0 32.973573774 187MB
HEAD getDefinition True 100 7.639179550000001 0.0 0.635095061 178MB
HEAD hover after edit True 100 7.5312538170000005 0.0 1.199751296 182MB
HEAD completions after edit True 100 7.403171093 0.0 5.633501327 192MB
HEAD code actions True 100 7.664663729000001 0.7644245700000001 0.740733792 214MB
HEAD code actions after edit True 100 8.044525588 0.0 20.646143884 214MB
HEAD documentSymbols after edit True 100 7.952895117000001 0.0 2.5444606960000002 181MB

@lukel97
Copy link
Collaborator Author

lukel97 commented Jul 21, 2020

Not sure why the startup time is lower, if I'm reading this correctly

@pepeiborra
Copy link
Collaborator

It looks like you are comparing two wildly different branches, there are lots of performance differences that are not explained by the changes in this PR.
Check the bench/hist.yaml to see what "upstream" means, by default it points to upstream/master. What does that point to in your repo?

@lukel97
Copy link
Collaborator Author

lukel97 commented Jul 21, 2020

@pepeiborra at the bottom of bench/hist.yaml,

- upstream: upstream/master
- HEAD

@lukel97
Copy link
Collaborator Author

lukel97 commented Jul 21, 2020

After rebasing things look more sensible

version name success samples startup setup experiment maxResidency
upstream hover True 100 9.972561843000001 0.0 0.42469480000000004 176MB
upstream edit True 100 9.620346303 0.0 33.033962742 186MB
upstream getDefinition True 100 9.673048695 0.0 0.6386101860000001 177MB
upstream hover after edit True 100 9.887454645 0.0 1.201044994 182MB
upstream completions after edit True 100 10.104105837 0.0 5.317705293 187MB
upstream code actions True 100 10.231499343000001 0.7385650450000001 0.759703196 213MB
upstream code actions after edit True 100 10.438941477 0.0 20.587980832 215MB
upstream documentSymbols after edit True 100 10.566125472000001 0.0 2.3593257920000004 179MB
HEAD hover True 100 10.077416170000001 0.0 0.432401884 177MB
HEAD edit True 100 9.667212247 0.0 31.554076919000003 188MB
HEAD getDefinition True 100 9.760036125000001 0.0 0.633670982 177MB
HEAD hover after edit True 100 9.760202366000001 0.0 1.23968618 183MB
HEAD completions after edit True 100 10.323710017 0.0 5.270130078 192MB
HEAD code actions True 100 10.183525091 0.8032742140000001 0.867096532 214MB
HEAD code actions after edit True 100 10.728177887000001 0.0 21.225001651000003 221MB
HEAD documentSymbols after edit True 100 9.795105816000001 0.0 2.237876946 179MB

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Collaborator

@cocreature cocreature left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you!

@cocreature cocreature merged commit 3eecfd0 into haskell:master Jul 27, 2020
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
…ide#699)

Currently this only searches local documentation (generated with
`cabal haddock --haddock-hyperlink-source` or equivalent) but could be
extended to support searching via Hoogle in the future. And it works for
any of the core libraries since they come installed with documentation.
Will show up in hover and (non-local) completions.

Also fixes extra markdown horizontal rules being inserted with no
content in between them.
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
…ide#699)

Currently this only searches local documentation (generated with
`cabal haddock --haddock-hyperlink-source` or equivalent) but could be
extended to support searching via Hoogle in the future. And it works for
any of the core libraries since they come installed with documentation.
Will show up in hover and (non-local) completions.

Also fixes extra markdown horizontal rules being inserted with no
content in between them.
pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
…ide#699)

Currently this only searches local documentation (generated with
`cabal haddock --haddock-hyperlink-source` or equivalent) but could be
extended to support searching via Hoogle in the future. And it works for
any of the core libraries since they come installed with documentation.
Will show up in hover and (non-local) completions.

Also fixes extra markdown horizontal rules being inserted with no
content in between them.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants