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

feat: add hover language feature for modules and module function calls #104

Closed
wants to merge 3 commits into from

Conversation

sineed
Copy link
Contributor

@sineed sineed commented Jul 10, 2023

closes #48

762e0597-d753-4581-9aa5-33599ee88c91.mp4

It figures out if module or function call is hovered using Code.Fragment.surround_context/3 and gets docs via Code.fetch_docs/1 functions. It also supports module aliases in three forms:

What is supported:

  • the module from this file
  • aliased modules and their functions:
    • Basic aliasing
    • Aliasing with :as option
    • Multi alias
  • Kernel and Kernel.SpecialForms functions
  • Erlang modules and functions
  • Structs
  • Imported module functions

@sineed sineed changed the title feat: add hover language feature for modules and module function calls wip: add hover language feature for modules and module function calls Jul 10, 2023
@sineed sineed marked this pull request as draft July 10, 2023 17:32
@sineed sineed marked this pull request as ready for review July 11, 2023 12:13
@sineed sineed changed the title wip: add hover language feature for modules and module function calls feat: add hover language feature for modules and module function calls Jul 11, 2023
@sineed
Copy link
Contributor Author

sineed commented Jul 11, 2023

The missing piece is imported functions. I didn't cover it as it is connected to compile tracer and I know that the SymbolTable is a subject of change. I think better to have a follow-up PR for imports.

Runtime.call(lsp.assigns.runtime, {Code, :fetch_docs, [module]})
end

defp find_aliased_module(document, module) do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is potentially better to extend tracer and collect all aliases instead of analyzing it when hovering. Let me know if this is a blocker and needs to be rearranged

Copy link
Collaborator

Choose a reason for hiding this comment

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

My intuition for how to implement this feature was going to be using the reference table to find the right thing and then look up the docs (or, just stick the docs in the symbol table), but i did have concerns about being able to lookup docs for code you've written but hasn't compiled yet, which using your implementation would solve for.

I'll probably get time tonight to review this, but even if we want to use the ref and symbol tables, we can probably merge this and change it later.

Copy link
Contributor Author

@sineed sineed Jul 12, 2023

Choose a reason for hiding this comment

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

Sounds a better idea. My approach doesn't support imported functions which you can find quite often in files using Phoenix.LiveView and Phoenix.Controller. But the reference table has this info OOTB.
As for the code not yet compiled I don't see it a big problem as long as user needs to just save the file. If you don't agree there may be two steps:

  1. It checks references table
  2. If nothing found it uses Code.Fragment.surround_context/2

I'm moving it back to draft but waiting for your feedback

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think moving to the reference table is a good idea, so you can move forward with that if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not quite satisfied with results: only 6 out of 15 assertions that I added for the hovering test were passed. I may need to extend code in Tracer a bit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could make it feel at least "ok" using two-step approach. Data from reference may not produce good results. For example, I was lucky using Atom.to_string/1 in my example code as there is no reference for such function (there is a reference but for the :erlang.atom_to_binary function).

I also found that there is no event when Erlang module is being referenced. There may be a workaround for it when Erlang function is found though.

I also tried to register references for aliases but it doesn't work well for multi aliases. For example this

  alias Bar.{
    Fiz,
    Baz
  }

Will produce two events with the same line and column information (both start and end). In such case it is problematic to decide what reference to use without additional code around it.

I have only one good news that now imported functions are supported when hovering.

One more thing to note: Erlang docs are not in the build unless the KERL_BUILD_DOCS=yes is set before compiling the source.

@sineed sineed marked this pull request as draft July 12, 2023 15:21
@sineed sineed marked this pull request as ready for review July 21, 2023 10:39
@mhanberg
Copy link
Collaborator

mhanberg commented Aug 1, 2023

@sineed I apologize for probably creating a horrid rebase situation, but I switched Next LS to use sqlite3 instead of dets.

I am still interested in merging this work, please let me know if you'll get the time to rebase this on main.

Thanks again!

@leonqadirie
Copy link

Apologies for hijacking; would we be open to use explicit language identifiers in the generated markdown code-fences, so editors like helix could highlight the hover syntax?

Analogous to this issue opened in ElixirLS.

@mhanberg
Copy link
Collaborator

Apologies for hijacking; would we be open to use explicit language identifiers in the generated markdown code-fences, so editors like helix could highlight the hover syntax?

Analogous to this issue opened in ElixirLS.

Potentially, the problem would be potentially mis highlighting code snippets that are of js or html/heex but are lacking the name in the fence

but, i think exdoc assumes its elixir if you don't specify, so there is already a precedence of that.

This would be a nice enhancement once this lands, as well as translating the erlang documentation into markdown as well (it's stored in a pseduo html syntax tree)

This was referenced Sep 15, 2023
@mhanberg
Copy link
Collaborator

Superceded by #220

@mhanberg mhanberg closed this Sep 17, 2023
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.

hover docs
3 participants