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

For multimethods, add implementation and dispatch key to document-symbols #1016

Closed
jwhitlark opened this issue Jun 8, 2022 · 4 comments · Fixed by #1130
Closed

For multimethods, add implementation and dispatch key to document-symbols #1016

jwhitlark opened this issue Jun 8, 2022 · 4 comments · Fixed by #1130
Labels
enhancement New feature or request
Projects

Comments

@jwhitlark
Copy link
Sponsor

jwhitlark commented Jun 8, 2022

Is your feature request related to a problem? Please describe.
Multimethod implementations don't show up in Calva outline. Calva uses clojure-lsp to populate the outline view, per @PEZ.

Describe the solution you'd like
I'd like to see both the multifn name and dispatch value show up in the outline. Even just adding the multifn names would be an improvement, if dispatch values required additional or upstream work.

Describe alternatives you've considered
In theory, I could declare a dispatch value, but it would require code changes only for this reason, and is a non-starter on the projects I work on.

Additional context
Discussion of this particular issue started on #662 with @mainej. Note that I am not certain document-symbols is the correct location, but seems highly likely, after conversations.

@jwhitlark jwhitlark added the enhancement New feature or request label Jun 8, 2022
@mainej
Copy link
Collaborator

mainej commented Jun 8, 2022

A few implementation notes:

  1. Currently document-symbols filters the analysis for :var-definitions only. This would mean including :var-usages that have :defmethod true.
  2. The dispatch-val is a little tricky. @borkdude are you interested in including dispatch-val in the analysis of a defmethod? If not, clojure-lsp would need to use the rewrite-clj tree. The most efficient way to search through that tree is with clojure-lsp.parser/to-pos, which we could call using position data from the clj-kondo analysis element. That would still mean multiple calls to parser/to-pos, so we'd have to keep an eye on performance. Alternatively, as @jwhitlark said, we could omit the dispatch-val and only show the multifn name.
  3. Whatever changes we decide on should probably be applied to workspace-symbols too.

@borkdude
Copy link
Contributor

borkdude commented Jun 8, 2022

@mainej I think that's potentially interesting, also for detecting duplicate dispatch values which we could warn about. There's already an issue about that here. The dispatch val is always at the same position right? So then the analysis should not be so hard to implement. Feel free to open an issue and a candidate PR.

@ericdallo ericdallo added this to Low priority in clojure-lsp via automation Jun 8, 2022
@mainej
Copy link
Collaborator

mainej commented Jun 8, 2022

@borkdude I'll open an issue. I can't commit to a PR right now. @ericdallo I'm also just helping spec out this problem, not claiming it. It sounds like a quick fix would be to split this into two parts:

  1. add the multifn name to document-symbols now, without the dispatch-val, and
  2. add the dispatch-val after it's available from clj-kondo

@ericdallo
Copy link
Member

ericdallo commented Jun 8, 2022

Agreed @mainej !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging a pull request may close this issue.

4 participants