haskell-language-server
haskell-language-server copied to clipboard
haskell
Goto dependency definition
For users using cabal HEAD, enables indexing of dependency HIE files so that the source can be written out on calls to gotoDefinition.
Opening this instead of #3704 because rebasing over the ghcide test refactor ended up being more trouble than it was worth.
There are a few things still missing, but I think this is ready for more eyes.
What I know is still missing - feedback on all of these items is welcome:
- What should we do if gotoDefinition is called while the module with the definition has not been indexed yet, because it is still in the indexQueue? @wz1000 has suggested that I look into what other language servers (rust, java, etc) do for this, so I plan to look into that over the next few days.
- Say that for some reason a package's HIE files get indexed and then for some reason the cabal store gets corrupted or deleted. In this case,
loadHieFileinlookupModwould fail. What should we do in this case? Should we send a message to the client? We should probably at least unindex the package. - There is one test of gotoDefinition for a dependency. I could add another that goes to a dependency definition and then from there goes to a transitive dependency definition. Do we think this would be a good test to add?
- Documentation: I think instructions on how to enable -fwrite-ide-info would be helpful, along with some troubleshooting tips. For example, it is possible to have the same hash in the cabal store as what we would have with the
extra-compilation-artifactsdirectory (where the HIE files should be found), but missingextra-compilation-artifacts. This can happen if the project is build with the -fwrite-ide-info flag, but with an older version of cabal that does not produce the HIE files. - I need to update CI to install cabal HEAD, or my dependency gotoDefinition test will fail.
Thanks for your work!
I'm glad to try this, If I compile cabal from master and compile HLS with this thread, everything should work then without any extra configruation?
@July541 You're welcome! You do need to add ghc-options: -fwrite-ide-info to some cabal config. It can be your global cabal config, the cabal.project for your project, or a .cabal file.
What should we do if gotoDefinition is called while the module with the definition has not been indexed yet, because it is still in the indexQueue?
I would say that the answer should be the same as "what do we do if gotoDefinition is called while a module has not yet been typechecked and it is in the shake build queue (or we're building dependencies, or whatever)?".
In this case, loadHieFile in lookupMod would fail. What should we do in this case?
What happens if the cabal store gets corrupted and the interface files for a dependency get deleted? Presumably we will die horribly in some way? If so, I think it's fairly reasonable to die horribly here also.
Do we think this would be a good test to add?
I definitely think we should have some more tests. The one you described sounds good. It would be good to think of some weird situations and try to have tests for them. Quick brainstorming:
- In a direct dependency
- Definition of a term
- Definition of a type
- Are there any entities that we know are not currently recorded in HIE files but are available for locally compiled modules? Might be worth adding some failing tests for those in order to show we still have stuff to fix
- In a transitive dependency
- In a boot library dependency (presumably will be broken until we get GHC shipped with HIE files, but worth recording)
- Open two files in different components that both depend on X, but different versions of X. Try a goto definition request in both, check that each goes to the right version of X
- In a direct dependency, but defined in a generated file like the
Paths_module
Any other weird cases people can think of?
I think instructions on how to enable -fwrite-ide-info would be helpful, along with some troubleshooting tips
So, if someone asks to go to a definition in a dependency and we don't have the IDE files, then we should definitely log something (or maybe even show a message?), but whatever we log and show could say that we're missing the IDE files and you can get them by doing X. Maybe that is a good enough UX?
@michaelpj I am taking a stab at this test idea:
Open two files in different components that both depend on X, but different versions of X. Try a goto definition request in both, check that each goes to the right version of X
and as far as I can tell, different components within the same project are not allowed to depend on different versions of the same package. Am I missing something, or misunderstanding what you meant? You can see my attempt at the test here: https://github.com/nlander/haskell-language-server/commit/da0ac643713656c04d72a7324a20ca2c9793c050
and as far as I can tell, different components within the same project are not allowed to depend on different versions of the same package. Am I missing something, or misunderstanding what you meant?
They have to be independent, e.g. an executable component and a library component, something like:
library
...
build-depends: foo == 1.0
executable
build-depdnds: foo == 2.0 -- note, no dependency on the main library!
(executables get their own build plan that doesn't necessarily have to be the same as the main build plan for the libraries, hence how this can happen!)
@michaelpj I have tried making one component an executable and the other a library and cabal still seems to think that they depend on each other :( even when I don't specify either one as a dependency of the other. I have tried this for one cabal file and two cabal files. Both attempts can be found here: https://github.com/nlander/haskell-language-server/commit/3c458b2049e115ccbc53c3169994d6742ffd2540. Is there something else that I need to add to allow the executable to have its own build plan?
Okay, turns out I'm just wrong. I asked and the only things that get independent build plans are custom setups and build tools. And in practice it sounds like the only place this could happen is Setup.hs, and we don't support that anyway and it's not that important.
User feedback: I've using this for about three days, going to third-party package is really really fantastic!
I still found some packages like base and time(for my personal experience) don't work for the goto definition, I dont' know why and the log dont' gives a reason. And hls seems trying to work on .hls folder, so there are some diagnostic errors.
My big comments:
- It would be great to have a note explaining how it all goes together. The flow of logic is pretty unclear to me, I have no idea what order things happen in or why.
- It seems like there's a bunch of work to try and avoid running certain rules and plugins on dependency files. I'm not clear why we can't just let them fail?
It seems like there's a bunch of work to try and avoid running certain rules and plugins on dependency files. I'm not clear why we can't just let them fail?
I took this design choice at the request of @wz1000. It was his idea to create a whitelist of Rules that we think are appropriate for dependency files. It was also his idea to create a notion of which file types plugins are defined for, defaulting to not supporting dependency files.
@michaelpj I wonder what justification you would give for running code that we know will fail? I was under the impression that plugin failures were not something that were handled gracefully, but if they are I suppose it doesn't matter which way we go on this. It just seems wasteful to me and likely not very performant to run extra code.
It would be great to have a note explaining how it all goes together. The flow of logic is pretty unclear to me, I have no idea what order things happen in or why.
There are two main components of the functionality:
- the changes to the
lookupModfunction inghcide/src/Development/IDE/Core/Actions.hs, which are triggered on calls togotoDefinition. - the code that indexes dependencies in the hiedb, which can be found in the new file
ghcide/src/Development/IDE/Core/Dependencies.hs. This gets run asynchronously, triggering every timenewHscEnvEqWithImportPathsgets called.
The gotoDefinition code was written in such a way that it was expecting that we would eventually be able to go to dependency definitions. Before my changes, lookupMod was a no-op stub intended to be where functionality would eventually go for dependencies. You can see the code that eventually ends up calling lookupMod in the function nameToLocation in ghcide/src/Development/IDE/Spans/AtPoint.hs. To summarize, gotoDefinition will look for a file in the project, and look in the hiedb if it can't find it. In this sense, the name lookupMod might be a little misleading, because by the time it gets called, the HIE file has already been looked up in the database and we have the FilePath of its location. A more appropriate name might be something like loadModule, since what it does is load the module source code from an HIE file and write it out to .hls/dependencies. The way nameToLocation works (which my code hasn't changed), if we have already opened a dependency file once, lookupMod won't get called. In addition to loading the dependency source and writing it out, lookupMod handles indexing the source file that we wrote out, which can't happen in the initial indexing since the source file doesn't exist at that point. To summarize, for gotoDefinition to work for a dependency we need to have already indexed the HIE file for that dependency module.
The indexing process gets the packages and modules for dependencies from the HscEnv. It filters them for packages we know are direct or transitive dependencies, using the function calculateTransitiveDependencies. indexDependencyHieFiles attempts to load an HIE file for each module, checking for it in the extra-compilation-artifacts directory, found in the package lib directory. This fails for the packages that ship with GHC, because it doesn't yet generate HIE files. @July541 that is why it fails for base and time. If it is able to load the HIE file, it indexes it in hiedb using indexHieFile, which is the same function used to index project HIE files.
Everything else is plumbing to make the two above functionalities work, code that disables functionality that won't work on dependency files (which may or may not be necessary as discussed above), and tests.
@michaelpj does this answer your questions about logic flow? Do you think this should be documented somewhere other than this comment, and if so where?
And hls seems trying to work on .hls folder, so there are some diagnostic errors.
@July541 I think this is a consequence of how hls handles a plugin being enabled or disabled. This may be the first instance of many files in a project having multiple plugins disabled (which may not be the right approach anyway as @michaelpj pointed out above). I think maybe there are some changes that should be made that aren't directly related to this functionality.
Do you think this should be documented somewhere other than this comment, and if so where?
Great comment! Yes, please do write a Note! I would probably put it close to where the indexing is happening. Something like Note [Going to definitions in dependencies].
A more appropriate name might be something like loadModule
:+1:
In addition to loading the dependency source and writing it out, lookupMod handles indexing the source file that we wrote out, which can't happen in the initial indexing since the source file doesn't exist at that point
I think I asked this already, but how bad would it be to eagerly write out the source files and index them during the initial process? It would make startup slower, since we'd be doing work for files that we might not need, but it seems like it would simplify the logic quite a bit.
Also, this process has the flavour of "do X, but only when it's requested". Would it make sense to handle this with a Rule? So we could have a Rule for GetHieSourceFile, which would either return the created source file, or create and index it.
I wonder what justification you would give for running code that we know will fail? I was under the impression that plugin failures were not something that were handled gracefully, but if they are I suppose it doesn't matter which way we go on this. It just seems wasteful to me and likely not very performant to run extra code.
Plugin failures and rule failures have to be handled somewhat gracefully, since failure is normal state. If the user has a Haskell file that has a parse error, then many things will fail! Rules that depend on parsing will fail, plugin handlers that depend on those rules will fail (some will work because they depend on stale information, but still). (I'm not too worried about the cost - the rule failures should cascade pretty quickly, I'd think)
Maybe this will all be too much if we just can't process the file at all, but in some sense it seems like it should be fine if, say, the Typecheck rule just always says "nope" when you run it on a dependency file. I could imagine that we might want to tweak it to make sure it doesn't emit diagnostics if it fails in such a way that we shouldn't expect it to succeed, but it would be nice if the cascading failures were handled gracefully.
Perhaps this is harder than I think - I know we had similar issues with the cabal plugin, which is what led to us having the filetype associations for plugins. So I guess it's not a hard and fast rule. But the changes here seem more invasive, and I wonder if we could just go for the "let it fail" approach.
In a direct dependency Definition of a term Definition of a type Are there any entities that we know are not currently recorded in HIE files but are available for locally compiled modules? Might be worth adding some failing tests for those in order to show we still have stuff to fix In a transitive dependency In a boot library dependency (presumably will be broken until we get GHC shipped with HIE files, but worth recording) In a direct dependency, but defined in a generated file like the Paths_ module
@michaelpj I have added tests for all of these. For weird cases, I found that gotoDefinition doesn't seem to work for definitions in where clauses in dependencies. The tests are not particularly DRY. Do you think that is something I should clean up, or are these tests OK as they are?
Does anyone else have any ideas about any other tests that should be added?
For weird cases, I found that gotoDefinition doesn't seem to work for definitions in where clauses in dependencies.
Great find! Do you have any idea why? I guess that information isn't in HIE files? Unclear if we expect that to work, but worth opening a ticket here or in GHC if we think it's a bug there.
The tests are not particularly DRY. Do you think that is something I should clean up, or are these tests OK as they are?
Let me flip it around: how would you feel about opening that test file in six months to do some maintenance on it? If you think "fine, whatever" then it's probably okay, if you think "ugh that would suck", maybe time to do a bit of refactoring.
Let me flip it around: how would you feel about opening that test file in six months to do some maintenance on it? If you think "fine, whatever" then it's probably okay, if you think "ugh that would suck", maybe time to do a bit of refactoring.
In this case I think "fine, whatever", because even though these test have a lot of repetition, I think it's unlikely that I would need to change all or most of the tests in the same way. It seems more likely that I would be changing small things that are specific to one or two tests. If I'm wrong and I find a structural issue with multiple tests, I think I wouldn't mind making the refactor then.
Plugin failures and rule failures have to be handled somewhat gracefully, since failure is normal state. If the user has a Haskell file that has a parse error, then many things will fail! Rules that depend on parsing will fail, plugin handlers that depend on those rules will fail (some will work because they depend on stale information, but still). (I'm not too worried about the cost - the rule failures should cascade pretty quickly, I'd think)
Maybe this will all be too much if we just can't process the file at all, but in some sense it seems like it should be fine if, say, the Typecheck rule just always says "nope" when you run it on a dependency file. I could imagine that we might want to tweak it to make sure it doesn't emit diagnostics if it fails in such a way that we shouldn't expect it to succeed, but it would be nice if the cascading failures were handled gracefully.
Perhaps this is harder than I think - I know we had similar issues with the cabal plugin, which is what led to us having the filetype associations for plugins. So I guess it's not a hard and fast rule. But the changes here seem more invasive, and I wonder if we could just go for the "let it fail" approach.
@michaelpj I would like to hear what @wz1000 has to say about this, but in the meantime I can at least check whether or not the tests still pass if I revert this behavior. I will let you know what I find out!
Great find! Do you have any idea why? I guess that information isn't in HIE files? Unclear if we expect that to work, but worth opening a ticket here or in GHC if we think it's a bug there.
I have no idea but the information being missing from HIE files is my best guess too. @wz1000 do you have any ideas about what could be causing this?
I think I asked this already, but how bad would it be to eagerly write out the source files and index them during the initial process? It would make startup slower, since we'd be doing work for files that we might not need, but it seems like it would simplify the logic quite a bit.
It's worth noting that if we eagerly write out all dependency source files we will likely be writing out thousands of dependency modules, even though the user might only end up opening a few of them. I don't have a super strong opinion about this, but I do think it would be a pretty major refactor of what we have now. In addition to moving all of the logic from lookupMod into indexDependencyHieFiles, I think we would need to remove some of the logic from nameToLocation in Developent.IDE.Spans.AtPoint, as it would be redundant. I wonder if it would be better to put a refactor like this into another PR after this one has been merged. After all, the existence of lookupMod predates my code, and I just filled in the stub that was there.
Also, this process has the flavour of "do X, but only when it's requested". Would it make sense to handle this with a Rule? So we could have a Rule for GetHieSourceFile, which would either return the created source file, or create and index it.
This would be a pretty simple change to make I think. I'm pretty sure the lookupMod function could be entirely replaced with a Rule with just a few minor changes. I think it should be either this or the refactor above, though, as I think the two approaches are incompatible.
@wz1000 I would love to hear your thoughts about these ideas.
I think it could be reasonable to let rules fail when applied to dependency files - but we must ensure that any diagnostics from such failures don't get shown to the user. At most we should log the failure, but the user should not get any diagnostics when rules which are not meant to be run on dependency files fail. In this sense, the whitelist is still useful.
We really don't want to write out dependency files eagerly because there could be many thousands of these and it will be a significant cost to startup performance and disk usage to do this for every project. This work will have to be duplicated for each project the user opens.
If we really want to write them out eagerly, I would suggest writing them out in a global location (like ~/.cache/ghcide) and then lazily creating hardlinks to the files we need from $PROJECTROOT/.hls.
I also don't see the benefit of writing them out with a rule, given that the way "go to definition" is set up makes it particularly hard to call rules for performance reasons, and they have no meaningful dependencies that we need to keep track of.
And hls seems trying to work on .hls folder, so there are some diagnostic errors.
@July541 Are you still getting diagnostics errors in dependency files with the most recent commits I have pushed to this branch?
I think this is good to go from my side apart from I would still like a big Note explaining how it all fits together.
Maybe we could open some issues for the follow up things.
I also don't see the benefit of writing them out with a rule, given that the way "go to definition" is set up makes it particularly hard to call rules for performance reasons, and they have no meaningful dependencies that we need to keep track of.
I agree it's not that much, but we do have a bit of state: "has the source file for this HIE file been written out and indexed already". At the moment I think we use the presence of the source file on disk to track this state, which I guess is fine, but it does mean that we have to do filesystem IO to check that things are up-to-date. I guess in principle this could matter, depending how slow the filesystem IO is and how many source files we're checking.
Which makes me think: could we add a shake benchmark for go-to-definition with a dependency definition? It would be interesting to see if it's noticeably slower than the local one!
Hmmm, with the current setup, will the tests produce very many .hls directories in each of the test directories that we use as project directories? I do still have a feeling that the .hls directory should be configurable, and this seems like one reason to do it: if you don't need HLS to work in dependency files, then you can put it wherever you want, including in a tmpdir or something, which would be handy during testing.
I'm afraid I've created a bunch of merge conflicts for you again, however I'll try to get you a patch for your branch to resolve those conflicts in a day or two
@nlander It looks like FOI's are not typechecked on changes while you have a third-party dep source open in your editor.
It looks like FOI's are not typechecked on changes while you have a third-party dep source open in your editor.
@fendor Thank you for catching this. I have found the source of this bug, but I'm not sure what the best approach is to fix it yet. What's happening is that when we edit a project file, for some reason that triggers the GetHlintDiagnostics rule for the dependency file. This rule is not on the whitelist for dependency files, so the error in defineEarlyCutoff' gets triggered. I could fix this by going directly into the rule for GetHlintDiagnostics, but I'm not sure that's the correct approach. Normally, all plugins with the defaultPluginDescriptor should be disabled for dependency files by the pluginResponsible function in hls-plugin-api/src/Ide/Types.hs. So it seems that somehow pluginResponsible is being bypassed for GetHlintDiagnostics. Any idea how a plugin could be triggered despite pluginResponsible returning false? @michaelpj @wz1000 If you have any ideas what could be causing this your input would be welcome and appreciated. If no one knows what could cause a plugin to get around the pluginResponsible mechanism, I will continue investigating myself. Thanks!