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

Call hierarchy support #1955

Merged
merged 33 commits into from Jul 27, 2021
Merged

Call hierarchy support #1955

merged 33 commits into from Jul 27, 2021

Conversation

July541
Copy link
Collaborator

@July541 July541 commented Jun 21, 2021

Hello all,

This pr is about my GSoC 2021 project, call hierarchy support. I create a draft to show my progress to those who may be interested.

Progress

  • Prepare call hierarchy (working now)
  • Incoming calls
  • Outgoing calls

Known issues

  • Data family range error (see commented test)
  • Incoming/Outgoing test missing
  • Name display not optimized for DuplicateRecordFields

@July541 July541 marked this pull request as draft June 21, 2021 13:01
@wz1000
Copy link
Collaborator

wz1000 commented Jun 21, 2021

If I understand correctly, call hierarchy is meant to be global and project wide. However, you seem to be generating the tree using a single HieAst.

I think it would be a good idea to make use of hiedb for this rather than directly using .hie files/HieAst. Take a look at the getGraph, getReachable and getVertices functions in hiedb: https://github.com/wz1000/HieDb/blob/master/src/HieDb/Query.hs#L175

The code you have for traversing the HieAst could still be very useful - we only keep up to date version of files the user isn't currently editing in the hiedb. For the files the user has open in their editor (IsFileOfInterest => True), to get an accurate callgraph the information from the HieAst traversal would need to be combined with the information from hiedb.

@July541
Copy link
Collaborator Author

July541 commented Jun 21, 2021

If I understand correctly, call hierarchy is meant to be global and project wide. However, you seem to be generating the tree using a single HieAst.

You're right, currently, I just finished the part of prepare call hierarchy. This part only constructs the CallHierarchyItem without any relation to other statements. I think HieAst is enough now.

The hierarchy is queried at the incoming/outgoing steps, which I have not started, your suggestion is very feasible, I'll follow it in the next part.

@pepeiborra
Copy link
Collaborator

@July541 I can see that you uploaded some commits yesterday. Care to share an update on what the recent changes are?

@July541
Copy link
Collaborator Author

July541 commented Jul 8, 2021

@pepeiborra It's about a prototype of the implementation of outgoing calls which has functionality but a bad code style. I'll show a complete call hierarchy after incoming calls are finished, it'll come soon.

@July541
Copy link
Collaborator Author

July541 commented Jul 11, 2021

2021-07-11.21-45-42.mp4

Recent progress, incoming calls and outgoing calls have basic functions, see demo above for details.

Current problems:

  • Duplication items(multi range merge)
  • Range of incoming/outgoing calls still exist problems
  • Test missing
  • Query will be broken if the target is a parameter

@pepeiborra
Copy link
Collaborator

This looks quite reasonable to me, nice job @July541 !
I would argue for landing it and then opening new tickets to fix the outstanding problems.
What do you think ?

import Name

incomingCalls :: HieDb -> Symbol -> IO [Vertex]
incomingCalls (getConn -> conn) symbol = do
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be best to move these functions into hiedb.

@July541
Copy link
Collaborator Author

July541 commented Jul 12, 2021

@pepeiborra I agree with you. I think it can be reviewed after the break issue is fixed, not take long.

@wz1000 It turns out I wrote it in HieDb from the beginning. But the following questions bothered me and made me turn to the internal implementation:

  1. The definition of Vertex is vague, we may need to rewrite the relevant parts, in which the compatibility comes at a high price to keep code elegant(for HieDb developer, not user).

  2. The two calls(incoming/outgoing) seem to be domain-specific, and I'm not sure they will have other usages, so I wrote them inside the call hierarchy.

Honestly, I'm not sure it's a reasonable solution. if you think that move to HieDb is better, I'll transfer it.

@pepeiborra
Copy link
Collaborator

@pepeiborra I agree with you. I think it can be reviewed after the break issue is fixed, not take long.

Awesome, please shout if you need any help

@isovector
Copy link
Collaborator

Your CI appears to be blocked by #2016

@July541 July541 marked this pull request as ready for review July 15, 2021 10:49
.github/workflows/test.yml Outdated Show resolved Hide resolved
@July541
Copy link
Collaborator Author

July541 commented Jul 27, 2021

I think the current fail can be fixed after the new lsp is released.

@pepeiborra
Copy link
Collaborator

This looks ready to merge, great job @July541 !!!
Do you want to do the honours @jneira ?

@jneira
Copy link
Member

jneira commented Jul 27, 2021

@July541 amazing work, hls users will be amazed, congrats
And thank you @pepeiborra for taking care, I had not all time I wish

@pepeiborra pepeiborra merged commit d815d04 into haskell:master Jul 27, 2021
@July541
Copy link
Collaborator Author

July541 commented Jul 28, 2021

@pepeiborra I noticed an unexpected ghcide fail on 8.6.5, not sure if it is caused by me.

@jhrcek
Copy link
Collaborator

jhrcek commented Jul 28, 2021

I pulled master this morning and I'm also not able to do stack ./install.hs hls-8.8.4 locally.
It seems it's related to this PR.
I'm getting build failures similar to what I see in recent CI runs:
e.g. this

build failures in CI
Linking .stack-work/dist/x86_64-linux/Cabal-3.0.1.0/build/ghcide/ghcide ...
ghcide                           > /root/.stack/snapshots/x86_64-linux/26f5581bf7f9f176d3f7ba334912f1d717717a1ecee407868101798f77c5c139/8.8.3/lib/x86_64-linux-ghc-8.8.3/libHSlsp-1.2.0.0-LiIFgQ1fCTv73zSCj7uRmO-ghc8.8.3.so: error: undefined reference to 'lspzmtypeszm1zi2zi0zi0zmDHDZZnuOdhmBHZZjePkaCNbQ_LanguageziLSPziTypesziCallHierarchy_zdfFromJSONCallHierarchyOutgoingCallsParams_closure'
ghcide                           > /root/.stack/snapshots/x86_64-linux/26f5581bf7f9f176d3f7ba334912f1d717717a1ecee407868101798f77c5c139/8.8.3/lib/x86_64-linux-ghc-8.8.3/libHSlsp-1.2.0.0-LiIFgQ1fCTv73zSCj7uRmO-ghc8.8.3.so: error: undefined reference to 'lspzmtypeszm1zi2zi0zi0zmDHDZZnuOdhmBHZZjePkaCNbQ_LanguageziLSPziTypesziCallHierarchy_zdfFromJSONCallHierarchyIncomingCallsParams_closure'
ghcide                           > /root/.stack/snapshots/x86_64-linux/26f5581bf7f9f176d3f7ba334912f1d717717a1ecee407868101798f77c5c139/8.8.3/lib/x86_64-linux-ghc-8.8.3/libHSlsp-1.2.0.0-LiIFgQ1fCTv73zSCj7uRmO-ghc8.8.3.so: error: undefined reference to 'lspzmtypeszm1zi2zi0zi0zmDHDZZnuOdhmBHZZjePkaCNbQ_LanguageziLSPziTypesziCallHierarchy_zdfFromJSONCallHierarchyPrepareParams_closure'
ghcide                           > /root/.stack/snapshots/x86_64-linux/26f5581bf7f9f176d3f7ba334912f1d717717a1ecee407868101798f77c5c139/8.8.3/lib/x86_64-linux-ghc-8.8.3/libHSlsp-1.2.0.0-LiIFgQ1fCTv73zSCj7uRmO-ghc8.8.3.so: error: undefined reference to 'lspzmtypeszm1zi2zi0zi0zmDHDZZnuOdhmBHZZjePkaCNbQ_LanguageziLSPziTypesziMethod_zdfFromJSONSMethod54_closure'
ghcide                           > /root/.stack/snapshots/x86_64-linux/26f5581bf7f9f176d3f7ba334912f1d717717a1ecee407868101798f77c5c139/8.8.3/lib/x86_64-linux-ghc-8.8.3/libHSlsp-1.2.0.0-LiIFgQ1fCTv73zSCj7uRmO-ghc8.8.3.so: error: undefined reference to 'lspzmtypeszm1zi2zi0zi0zmDHDZZnuOdhmBHZZjePkaCNbQ_LanguageziLSPziTypesziMethod_zdfFromJSONSMethod55_closure'
ghcide                           > /root/.stack/snapshots/x86_64-linux/26f5581bf7f9f176d3f7ba334912f1d717717a1ecee407868101798f77c5c139/8.8.3/lib/x86_64-linux-ghc-8.8.3/libHSlsp-1.2.0.0-LiIFgQ1fCTv73zSCj7uRmO-ghc8.8.3.so: error: undefined reference to 'lspzmtypeszm1zi2zi0zi0zmDHDZZnuOdhmBHZZjePkaCNbQ_LanguageziLSPziTypesziMethod_zdWSTextDocumentPrepareCallHierarchy_closure'
ghcide                           > /root/build/ghcide/.stack-work/dist/x86_64-linux/Cabal-3.0.1.0/build/libHSghcide-1.4.0.3-GzSd9eXvctl5jyqcFJIb49-ghc8.8.3.so: error: undefined reference to 'lspzmtypeszm1zi2zi0zi0zmDHDZZnuOdhmBHZZjePkaCNbQ_LanguageziLSPziTypesziMethod_zdfShowSMethod114_closure'
ghcide                           > /root/build/ghcide/.stack-work/dist/x86_64-linux/Cabal-3.0.1.0/build/libHSghcide-1.4.0.3-GzSd9eXvctl5jyqcFJIb49-ghc8.8.3.so: error: undefined reference to 'lspzmtypeszm1zi2zi0zi0zmDHDZZnuOdhmBHZZjePkaCNbQ_LanguageziLSPziTypesziMethod_zdfShowSMethod112_closure'
ghcide                           > /root/build/ghcide/.stack-work/dist/x86_64-linux/Cabal-3.0.1.0/build/libHSghcide-1.4.0.3-GzSd9eXvctl5jyqcFJIb49-ghc8.8.3.so: error: undefined reference to 'lspzmtypeszm1zi2zi0zi0zmDHDZZnuOdhmBHZZjePkaCNbQ_LanguageziLSPziTypesziMethod_zdfShowSMethod116_closure'
ghcide                           > collect2: error: ld returned 1 exit status
ghcide                           > `gcc' failed in phase `Linker'. (Exit code: 1)
  

see failure in CI

@pepeiborra
Copy link
Collaborator

pepeiborra commented Jul 28, 2021

Have you tried stack clean ?

@pepeiborra
Copy link
Collaborator

@pepeiborra I noticed an unexpected ghcide fail on 8.6.5, not sure if it is caused by me.

Some tests are flaky in Windows, that's not caused by this PR

@July541
Copy link
Collaborator Author

July541 commented Jul 28, 2021

@jhrcek Ideally the updated dependencies are OK for the build.

- github: haskell/lsp
commit: ef59c28b41ed4c5775f0ab0c1e985839359cec96
subdirs:
- lsp-types
- lsp
- lsp-test
# https://github.com/haskell/lsp/pull/332

@jneira
Copy link
Member

jneira commented Jul 28, 2021

@jhrcek i hope #2044 (which remove .stack-work from cache, so equivalent to do a stack clean --full) will take ride of those circleci linker errors

@pepeiborra
Copy link
Collaborator

@jhrcek i hope #2044 (which remove .stack-work from cache, so equivalent to do a stack clean --full) will take ride of those circleci linker errors

Users will still be affected by the stack build problem though.

@jhrcek
Copy link
Collaborator

jhrcek commented Jul 28, 2021

I tried stack clean --full in haskell-language-server repo, but still got build failure during linking.
In the end I removed all build snapshots from ~/.stack/snapshots/x86_64-linux-tinfo6/ and then it built correctly.
The call hierarchy seems to be working great. Thank you! ❤️

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

6 participants