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

Add support for textDocument/hover (for modules) #331

Merged
merged 11 commits into from
Sep 5, 2023
Merged

Add support for textDocument/hover (for modules) #331

merged 11 commits into from
Sep 5, 2023

Conversation

zachallaun
Copy link
Collaborator

This first commit is essentially just scaffolding, including the generated types for textDocument/hover and basic request/response handling. It currently replies with static content.

There's some additional WIP code for retrieving the docs for the entity being hovered, but I'm not sure how much of it will actually end up in the final feature.


@scohen, some thoughts/questions on the best way to proceed that I'd love your input on.

  1. The hover response contains two things, an optional range, representing the entity that you retrieved hover content for, and the contents themselves as markdown. For this initial implementation, I'd thinking to handle modules, functions, and types -- see the Project.Docs fixture for what I'm planning to test with to start. Does this seem reasonable?
  2. I'm planning to use Code.fetch_docs/1 to retrieve docs. It retrieves docs for an entire module, all functions/types/etc. defined within. As per (Added thoughts about code intelligence and indexing #223), it'll eventually be worth caching this stuff, but for now I'm thinking to just scan the result each time.
  3. Go-to-definition currently uses ElixirSense to retrieve the definition given a doc/line/character. I'm assuming that I should also use it for now to retrieve the range and module associated with the currently hovered entity?

@zachallaun zachallaun marked this pull request as draft August 14, 2023 18:37
@zachallaun
Copy link
Collaborator Author

  1. This shares a lot of behavior with Definition -- I'm inclined to co-locate them instead of adding a new Docs or something.

@scohen
Copy link
Collaborator

scohen commented Aug 14, 2023

@zachallaun

  1. Yes, this seems reasonable. I'd even say this sounds like a lot, perhaps start with modules and functions?
  2. Go for it, I haven't made any progresson the intelligence and indexing features.
  3. I would like to stay away from elixir_sense, as it's a dependency that is going away, and doesn't have a release cycle or versioning. I do understand that identifying stuff under the cursor is pretty tricky in elixir. I'd like your thoughts on what you feel comfortable doing.
  4. Ok, maybe the name Definition needs work then?

@zachallaun
Copy link
Collaborator Author

zachallaun commented Aug 14, 2023

  1. Let's see how much we get "for free" -- the hard part is figuring out the entity under cursor, after that it's just filtering a list.
  2. 👍
  3. This is somewhat related to co-locating with Definition, which also relies on elixir_sense. So if we co-locate, I'll factor things such that when elixir_sense is removed, there's a minimal interface to replace. I'll keep an eye on opportunities to remove dependence on it, though. Using Code.fetch_docs directly is one such opportunity already, vs. ElixirSense.docs.
  4. Agreed, I was going to bring that up. CodeEntity? Just Entity? Artifact? I'm not really sure what the nomenclature should be -- basically any unit that can be defined somewhere and referred to either by an atom or module-function-arity.

@scohen
Copy link
Collaborator

scohen commented Aug 14, 2023

  1. This is what's dangerous about using ElixirSense; you get a LOT for free, and then it becomes super hard to replace the whole lot. At this point, I'd almost favor going the other way and starting small with, say Modules, and working our way up.
  2. I'd favor SytaxEntity or CodeEntity. Perhaps in a Syntax.Entity or Code.Entity module. Thoughts?

@zachallaun
Copy link
Collaborator Author

  1. Sounds good -- I'll start with modules and see if we can avoid elixir sense altogether. Basically these cases:
Some.Module| # docs for Some.Module

alias Some.Module
Module| # docs for Some.Module

Some.Module|. # compile error, should still be able to identify Some.Module

Some|.Module # docs for Some
  1. I think Code.Entity makes sense. This is pretty foundational, probably the kind of thing that will be in shared or common eventually (plugins will want access to it, methinks). So it'll basically be Lexical.Code.Entity, I think?

@scohen
Copy link
Collaborator

scohen commented Aug 14, 2023

Aliases can get pretty tricky, but luckily we have an alias tracker in lib/lexical/code_mod/ast/aliases.ex It allows you to give the ast and then do Aliases.at(doc, position) which in your second example, will give you a map of %{:Module => Elixir.Some.Module}

@scohen
Copy link
Collaborator

scohen commented Aug 14, 2023

So it'll basically be Lexical.Code.Entity, I think?

Yep, start off in common and if we need it in plugins, we'll move it to lexical_shared.

@zachallaun
Copy link
Collaborator Author

I'm going to mark this PR as ready for review. Currently it is able to resolve modules under the cursor and display module docs. The code (and commits) need to be cleaned up a bit, however. That said, I think it's in a good-enough spot that it's useful to get feedback and make sure I'm not too far astray.

image

@zachallaun zachallaun marked this pull request as ready for review August 18, 2023 19:43
@zachallaun zachallaun changed the title WIP: Add support for textDocument/hover Add support for textDocument/hover (for modules) Aug 18, 2023
@zachallaun zachallaun requested review from scohen and scottming August 25, 2023 19:22
Copy link
Collaborator

@scohen scohen left a comment

Choose a reason for hiding this comment

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

Wow, what an improvement. I'm really liking the direction here (and I can't wait until we can get Ast.fragment merged so I can use it in my indexer PR.

I would like to call out the tests though, I think (hope) we can improve them so that each test clearly shows both the module being defined (and its docs, or lack thereof) and the module being hovered over.

apps/common/lib/lexical/ast.ex Outdated Show resolved Hide resolved
apps/common/lib/lexical/ast.ex Outdated Show resolved Hide resolved
apps/common/lib/lexical/ast.ex Outdated Show resolved Hide resolved
apps/common/lib/lexical/ast.ex Outdated Show resolved Hide resolved
@scottming
Copy link
Collaborator

I noticed that this PR also supports built-in Elixir modules like Enum, so I think we might need to add support for Hexdocs links.

Like livebook:

CleanShot 2023-08-28 at 21 25 31@2x

Additionally, I found that when the cursor is on a dot, it displays the documentation for the previous module, which is great. However, it doesn't work when there are no functions or submodules following it, like Lexical.|, but Lexical.|RemoteControl will work.

@zachallaun
Copy link
Collaborator Author

I noticed that this PR also supports built-in Elixir modules like Enum, so I think we might need to add support for Hexdocs links.

Those aren't embedded in the module docs, but added after the fact. I do think it is worth adding them (Elixir LS does as well), but I'd rather that happen in another PR.

Additionally, I found that when the cursor is on a dot, it displays the documentation for the previous module, which is great. However, it doesn't work when there are no functions or submodules following it, like Lexical.|, but Lexical.|RemoteControl will work.

Will look into it!

@zachallaun
Copy link
Collaborator Author

@scohen @scottming I believe everything that's been mentioned so far has been addressed. Please let me know what else y'all would like to see prior to merge!

* Add `resolve/2`

This is a general-purpose API for resolving the code entity at a given
position. Currently it only supports resolving modules (and aliased modules).
It will have immediate use for accessing docs on hover.

* Move `definition/3`

Entity resolution and go-to-definition logic are related: the first part of
going to a definition is resolving the entity under the cursor so that you know
what to go to. While go-to-definition still currently relies on `elixir_sense`,
the plan is to eventually remove that dependency, at which point it will share
logic with or use `resolve/2`.
This commit adds initial support for `textDocument/hover` LSP requests, showing
basic moduledocs when a module or alias is hovered.
This commit swaps out the use of `Sourceror.parse_string/1` with a call to
`Sourceror.string_to_quoted/2` that includes the `:static_atoms_encoder`
option. This option is used to retain line/column metadata on certain atoms
(like the individual segments of an alias) that would otherwise be discarded.

An explanatory comment has been added to `Lexical.RemoteControl.CodeMod.Ast/parse/1`
for additional context.
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.

3 participants