Fix documentation on hover and don't report invalid locations #22
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In this commit we add support for loading multiple components into one ghcide session. The current behaviour is that each component is loaded lazily into the session. When a file from an unrecognised component is loaded, the cradle is consulted again to get a new set of options for the new component. This will cause all the currently loaded files to be reloaded into a new HscEnv which is shared by all the currently known components. The result of this is that functions such as go-to definition work between components if they have been loaded into the same session but you have to open at least one file from each component before it will work. Only minimal changes are needed to the internals to ghcide to make the file searching logic look in include directories for all currently loaded components. The main changes are in exe/Main.hs which has been heavily rewritten to avoid shake indirections. A global map is created which maps a filepath to the HscEnv which should be used to compile it. When a new component is created this map is completely refreshed so each path maps to a new Which paths belong to a componenent is determined by the targets listed by the cradle. Therefore it is important that each cradle also lists all the targets for the cradle. There are some other choices here as well which are less accurate such as mapping via include directories which is the aproach that I implemented in haskell-ide-engine. The commit has been tested so far with cabal and hadrian. Also deleted the .ghci file which was causing errors during testing and seemed broken anyway.
The fix is quite hacky. After parsing we post-process the module to remove any package imports for packages which are in our component. This is ok, because we already decided ourselves which files these imports refer to whilst taking into account package imports (see `locateModule`). If you don't do this processing then GHC will happily decide for itself again that you really meant to use the version of the package from the package database rather than the file in the home module. Hopefully fendor will fix this in GHC this summer.
This introduces a new function `useWithStaleFast` which returns with stale information WITHOUT checking freshness like `use` and `useWithStale`. Greatly improve debug logging All actions triggered by shakeRun now also pass an identifier which means that the debug logging shows which actions are starting/finishing We also distinguish between internal and external events. By default external events are ones triggered by runAction and the debug output is displayed to the user in command line and --lsp mode. In order to see internal logging statements, there is a new flag called --verbose which also prints out internal events such as file modification flushes. Cleaner variant using runAfter Step 1: Do not run actions with shakeRun Queue implementation, living, breathing Use a priority queue to schedule shake actions. Most user actions are answered immediately with a cache but also spawn a shake action to check the cached value we consulted was up to date.
Do NOT kick Kicking was an action triggered on every database run which added approx 0.2s to the time taken to hover for no benefit. Now kicking is controlled in a more fine-grained way directly by triggering a typecheck when a file is modified.
There were a number of issues here
1. The time was just wrong if called via runAction as it timed how long
the original action took rather than how fast runAction returned which
could be quite a bit before in the days of `kick`. Now `kick` is
removed, `runAction` is unecessary so this also removes the complication
to do with the barrier.
2. Time taken to write the profile was included in the time. (This is
significant in my tests, 0.2s approx on GHC)
Now functions like hover will work immediately rather than the information only being computed on the first hover.
Eta-expanding the function means GHC no longer allocates a function closure every time `withProgress` is called (which is a lot).
Ensure that PositionMappings are shared between versions There was a quadratic space leak as the tails of the position maps were not shared with each other. Now the space usage is linear which is produces more acceptable levels of residency after 3000 modifications.
On small projects this wasn't an issue but for a project like GHC this was causing some latency issues.
The scheduler branch is still important with the new architecture, it is just less noticeably than before. It can still cut the executation time for computing GetHieFile in half if nothing needs to be recomputed.
Co-authored-by: Matthew Pickering <matthewtpickering@gmail.com>
The FastAction contains a barrier which can be used to wait for the action to complete if necessary. This should be rarely used but we are experimenting for now. Co-authored-by: Matthew Pickering <matthewtpickering@gmail.com>
Co-authored-by: Matthew Pickering <matthewtpickering@gmail.com>
Co-authored-by: Matthew Pickering <matthewtpickering@gmail.com>
Co-authored-by: Matthew Pickering <matthewtpickering@gmail.com>
Co-authored-by: Matthew Pickering <matthewtpickering@gmail.com>
Co-authored-by: Matthew Pickering <matthewtpickering@gmail.com>
Co-authored-by: Matthew Pickering <matthewtpickering@gmail.com>
A Delta is a change between two versions A Mapping is a change from the current version to a specific older version.
…ange and srcSpanToLocation
This required two separate fixes: 1. When parsing a module, if parsing haddocks succeeds, then use them Previously, even though we were parsing modules twice, with and without haddocks, we were just returning the result of parsing without haddocks 2. When loading the iface before generating the DocMap, use the ModIface in the HomeModInfo, and not the one in the ModInfo field of the TypecheckedModule, since that one does not exist.
|
First commit looks good but I am a bit wary of the second one because of haskell#350 (comment) |
|
Is there a GHC ticket about |
|
We do still parse it both ways, so hlint can use the other one if and when it is integrated. Until then I don't think it makes sense sacrifice usability now for hypothetical future features that won't really be blocked by this any way. |
|
I can add an all caps comment about this in |
|
Yes it's important to document what is happening here as it hasn't yet been done so and you are the third person to attempt to change it. |
|
I commented on the haddock parsing logic which I think needs to be changed. |
mpickering
reviewed
May 8, 2020
| @@ -51,6 +51,9 @@ type instance RuleResult GetModuleGraph = DependencyInformation | |||
| -- that module. | |||
| data TcModuleResult = TcModuleResult | |||
| { tmrModule :: TypecheckedModule | |||
| -- ^ warning, the ModIface in the tm_checked_module_info of the | |||
| -- TypecheckedModule will always be Nothing, use the ModIface in the | |||
| -- HomeModInfo instead | |||
There was a problem hiding this comment.
Could we not set the ModIface in the tm_checked_module_info to make this less surprising?
| -- non-interest are always parsed with Haddocks | ||
| -- If we can parse Haddocks, might as well use them | ||
| ((fp,(diags,res)),(fph,(diagsh,resh))) <- liftIO $ concurrently mainParse haddockParse | ||
| return (fph<|>fp,(mergeDiagnostics diags diagsh, resh<|>res)) |
There was a problem hiding this comment.
I think there are several potential issues here that need comments explaining why it is implemented like this.
- Why not just parse with haddocks on always? What is the point of the mainParse now? The reason it was implemented like this before was that you would get an error in a dependency because of the haddocks and then you would open the file and the error would disappear. So we could always parse with haddocks on and make people fix invalid haddocks by just failing to parse the module.
The argument against this is that only parsing with haddocks on will break down stream tools. Solution to this, add another rule GetParsedModuleTokenStream which can be called to get the parsed module with the token stream? It would only be called for modules of interest so the memory overhead is not too severe.
- There is no particular reason why
fphisJustiffreshisJustso you could end p using the hash from one parse but the result of the other, which is obviously very bad and would be a nightmare to find. Therefore if we are to implement it like this you could case uponreshand then returnfphrather than the current implementation.
Also, please add some spaces or I'll make you write a blog post.
There was a problem hiding this comment.
We discussed this on IRC and we agree that I am correct about point 2. On point 1, Zubin argues that because the parse error is caused by us turning on an extension we should still parse both and fall back to the non-haddock version if there is an error parsing haddocks.
|
I merged this now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.