Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Streamline code for getDocumentationTryGhc #2349

Closed
wants to merge 37 commits into from

Conversation

Anton-Latukha
Copy link
Collaborator

@Anton-Latukha Anton-Latukha commented Nov 13, 2021

at least trying to

Work towards #2348 & cleaning-up worked-with code on the go.


PR solves:

getDocumentationTryGhc :: HscEnv -> Module -> Name -> IO SpanDoc
getDocumentationTryGhc env mod n = head <$> getDocumentationsTryGhc env mod [n]

As getDocumentationTryGhc is the main code that retrieves the docs from GHC - the PR provides the element retrieval code.


This PR is a stage toward provisioning the argument docs into the LSP interface. PR addressed some underlying code.


Initial master code conditions:

  • getDocsBatch had a doc:

    -- | Non-interactive, batch version of 'InteractiveEval.getDocs'.
    -- The interactive paths create problems in ghc-lib builds
    --- and leads to fun errors like "Cannot continue after interface file error".

  • getDocsBatch not optimized for batch retrieval, it does 1 element retrieval for every element. (Locates & loads a module interface, retrieves 1 element, locates & loads another module interface, retrieves 1 element ...).

  • getDocumentationsTryGhc (plural *s*) - in the project had only one use - in getDocumentationTryGhc (single) & so was its implementation.

  • So, over the project only (single) (meaning getDocumentationTryGhc) was used - & all doc requests to GHC - were 1 element requests, which got wrapped into singleton & processed as a list & by list function down the codepath.

  • So getDocsBatch both was externally invoked for 1 elem & internally was processing lists per 1 elem. Batch processing optimization is addressed in getDocsBatch for bulk processing #2371.


The results of the PR:

  1. The non-interactive analog of GHCs getDocs - getDocsNonInteractive is created (for purposes mentioned in the initial doc above) it is designed for (all current requests) case of looking-up an entity.
  2. Creation of getDocsNonInteractive allows to stop using getDocumentationsTryGhcs (plural) for getDocumentationTryGhc (single), which makes possible to remove unsafe head from the main code path.
  3. Optimize the use case currently used everywhere.
  4. Code shared between getDocsNonInteractive & getDocsBatch formed getDocsNonInteractive' & initTypecheckEnv which self-explained the code.

@Anton-Latukha Anton-Latukha force-pushed the 2021-11-11-upds branch 2 times, most recently from 32f9de1 to 19f65cb Compare November 17, 2021 14:31
@Anton-Latukha
Copy link
Collaborator Author

Anton-Latukha commented Nov 17, 2021

https://github.com/Anton-Latukha/haskell-language-server/blob/19f65cb391e781ffba3dbecaedead01c3e967317/ghcide/src/Development/IDE/Spans/Documentation.hs#L71-L79

That IO gathering/handling seems strange to me, it catches IO exceptions from getDocsBatch.

While:
https://github.com/Anton-Latukha/haskell-language-server/blob/19f65cb391e781ffba3dbecaedead01c3e967317/ghcide/src/Development/IDE/Core/Compile.hs#L964-L982

There is a lot of Maybe & Either already around it & exceptions handling through IO on top of it. Code throws exception into IO just to catch it immediately. That means is somebody else uses getDocsBatch - afaiu would catch an exception.

@Anton-Latukha Anton-Latukha force-pushed the 2021-11-11-upds branch 2 times, most recently from 4402d47 to df95e6b Compare November 17, 2021 15:58
@Anton-Latukha

This comment has been minimized.

@Anton-Latukha

This comment has been minimized.

@Anton-Latukha Anton-Latukha force-pushed the 2021-11-11-upds branch 2 times, most recently from a94a2f3 to cc4d333 Compare November 19, 2021 14:36
@Anton-Latukha
Copy link
Collaborator Author

Anton-Latukha commented Nov 19, 2021

Separately have proper bulk-optimized retrieval: getDocumentationsTryGhc & its getDocsBatch, which goes into opened #2371 - with a bulking code.

I currently think singleton process should be kept, but element retrieval should be written as it is used - not for a list but for one element: getDocumentationTryGhc & its getDocsNonInteractive. - that is probably nice for hover-like docs retrieval (main agenda). From history of getDocsBatch & usage of it - getDocsNonInteractive should become a proper module-scoped function.

@Anton-Latukha Anton-Latukha force-pushed the 2021-11-11-upds branch 7 times, most recently from 5801b68 to 5be04f7 Compare November 26, 2021 20:17
@Anton-Latukha
Copy link
Collaborator Author

Anton-Latukha commented Nov 26, 2021

Since HLS uses per-1-element retrieval from GHC:

The implementations for according mode are now provided.

And switched the code into it.

HLS now loads docs from module interfaces though proper mode implementation.

@Anton-Latukha Anton-Latukha force-pushed the 2021-11-11-upds branch 4 times, most recently from da06f06 to 2f6553e Compare November 27, 2021 13:53
@Anton-Latukha Anton-Latukha changed the title Implementing showing of function argument documentation Streamline GHC docs retrieval codepath getDoc{umentationTryGhc,sNonInteractive} Nov 27, 2021
@Anton-Latukha Anton-Latukha marked this pull request as ready for review November 27, 2021 14:00
@Anton-Latukha Anton-Latukha changed the title Streamline GHC docs retrieval codepath getDoc{umentationTryGhc,sNonInteractive} Streamline code for getDocumentationTryGhc Nov 27, 2021
@Anton-Latukha Anton-Latukha marked this pull request as draft November 27, 2021 20:02
Make code easier to reason about & functionally enhancable.
Turn `[]` into idiomatic Map.
Throw was vacuous - it was thrown & catched & ignored.

At least it shows explicit type to process further.
We can not process/reason on the ArgDoc logic - if we pretend no docs are docs.

It is also aligns with main doc blocks processing.
Showing error should be explicit, & conversion of error type should be a
separate handling.

This would also allow to establish proper processing for all these exception
types.
This function was "inspired" from GHC code of `getDocs`.

Since `getDocsBatch` is not really used for batch - only for singleton elements,
lets make 1 element processing function & use it.
`getDocsBatch` cuurently (& before) used only for single name retrieval
function. Use of it is in `Documentation` module `getDocumentationTryGhc` where
it wraps arg into singleton & gives to `getDocsBatch` & then recieves a Map with
1 entry & unsafely "lookups" doc in it.

This work is to supply the proper single name retrieval-optimized version to
stop that `getDocsBatch` there.

& further ideally `getDocumentationTryGhc` uses single-retrieval &
`getDocumentationsTryGhc` uses a batch mode & batch mode gets optimized along
the lines of: haskell#2371
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants