From 9ee4f0852459d612b3fb490a2f7a616f4f593648 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Fri, 16 Aug 2024 13:31:05 +0200 Subject: [PATCH] Make the go-to-definition link a client concern (#2746) --- assets/js/hooks/cell_editor/live_editor.js | 37 +++- .../live_editor/codemirror/theme.js | 22 ++- assets/js/hooks/session.js | 14 -- lib/livebook/intellisense.ex | 164 +++++++----------- lib/livebook/runtime.ex | 20 +-- lib/livebook_web/live/session_live.ex | 4 - test/livebook/intellisense_test.exs | 107 +++++------- 7 files changed, 156 insertions(+), 212 deletions(-) diff --git a/assets/js/hooks/cell_editor/live_editor.js b/assets/js/hooks/cell_editor/live_editor.js index c156a86a2..052fc62b9 100644 --- a/assets/js/hooks/cell_editor/live_editor.js +++ b/assets/js/hooks/cell_editor/live_editor.js @@ -535,11 +535,30 @@ export default class LiveEditor { const dom = document.createElement("div"); dom.classList.add("cm-hoverDocs"); + if (response.definition) { + const link = document.createElement("a"); + link.classList.add("cm-hoverDocsDefinitionLink"); + link.innerHTML = ` Go to definition`; + dom.appendChild(link); + + link.addEventListener("click", (event) => { + globalPubsub.broadcast("jump_to_editor", { + line: response.definition.line, + file: response.definition.file, + }); + event.preventDefault(); + }); + } + + const contents = document.createElement("div"); + contents.classList.add("cm-hoverDocsContents"); + dom.appendChild(contents); + for (const content of response.contents) { const item = document.createElement("div"); item.classList.add("cm-hoverDocsContent"); item.classList.add("cm-markdown"); - dom.appendChild(item); + contents.appendChild(item); new Markdown(item, content, { defaultCodeLanguage: this.language, @@ -565,16 +584,16 @@ export default class LiveEditor { if (column < 1 || column > lineLength) return null; return this.connection - .intellisenseRequest("definition", { line: text, column }) + .intellisenseRequest("details", { line: text, column }) .then((response) => { - globalPubsub.broadcast("jump_to_editor", { - line: response.line, - file: response.file, - }); - - return true; + if (response.definition) { + globalPubsub.broadcast("jump_to_editor", { + line: response.definition.line, + file: response.definition.file, + }); + } }) - .catch(() => false); + .catch(() => null); } /** @private */ diff --git a/assets/js/hooks/cell_editor/live_editor/codemirror/theme.js b/assets/js/hooks/cell_editor/live_editor/codemirror/theme.js index cdbec526a..6dd692279 100644 --- a/assets/js/hooks/cell_editor/live_editor/codemirror/theme.js +++ b/assets/js/hooks/cell_editor/live_editor/codemirror/theme.js @@ -265,10 +265,28 @@ function buildEditorTheme(colors, { dark }) { maxWidth: "800px", maxHeight: "300px", overflowY: "auto", - padding: "8px", display: "flex", flexDirection: "column", - gap: "64px", + + "& .cm-hoverDocsDefinitionLink": { + padding: "4px 8px", + cursor: "pointer", + fontSize: "0.875em", + fontFamily: fonts.sans, + opacity: 0.8, + borderBottom: `1px solid ${colors.separator}`, + + "& i": { + marginRight: "2px", + }, + }, + + "& .cm-hoverDocsContents": { + padding: "8px", + display: "flex", + flexDirection: "column", + gap: "64px", + }, }, // Signature diff --git a/assets/js/hooks/session.js b/assets/js/hooks/session.js index 8ee2e3861..d2f3529fb 100644 --- a/assets/js/hooks/session.js +++ b/assets/js/hooks/session.js @@ -546,20 +546,6 @@ const Session = { this.setInsertMode(false); } - if ( - event.target.matches("a") && - event.target.hash.startsWith("#go-to-definition") - ) { - const search = event.target.hash.replace("#go-to-definition", ""); - const params = new URLSearchParams(search); - const line = parseInt(params.get("line"), 10); - const file = params.get("file"); - - this.jumpToLine(file, line); - - event.preventDefault(); - } - const evalButton = event.target.closest( `[data-el-queue-cell-evaluation-button]`, ); diff --git a/lib/livebook/intellisense.ex b/lib/livebook/intellisense.ex index 6768fbfd3..e566e82e3 100644 --- a/lib/livebook/intellisense.ex +++ b/lib/livebook/intellisense.ex @@ -82,10 +82,6 @@ defmodule Livebook.Intellisense do get_details(line, column, context, node) end - def handle_request({:definition, line, column}, context, node) do - get_definitions(line, column, context, node) - end - def handle_request({:signature, hint}, context, node) do get_signature_items(hint, context, node) end @@ -447,12 +443,12 @@ defmodule Livebook.Intellisense do nil matches -> - contents = - matches - |> Enum.sort_by(& &1[:arity], :asc) - |> Enum.map(&format_details_item(&1, context)) + matches = Enum.sort_by(matches, & &1[:arity], :asc) + contents = Enum.map(matches, &format_details_item/1) - %{range: range, contents: contents} + definition = get_definition_location(hd(matches), context) + + %{range: range, contents: contents, definition: definition} end end @@ -460,13 +456,13 @@ defmodule Livebook.Intellisense do defp include_in_details?(%{kind: :bitstring_modifier}), do: false defp include_in_details?(_), do: true - defp format_details_item(%{kind: :variable, name: name}, _context), do: code(name) + defp format_details_item(%{kind: :variable, name: name}), do: code(name) - defp format_details_item(%{kind: :map_field, name: name}, _context), do: code(name) + defp format_details_item(%{kind: :map_field, name: name}), do: code(name) - defp format_details_item(%{kind: :in_map_field, name: name}, _context), do: code(name) + defp format_details_item(%{kind: :in_map_field, name: name}), do: code(name) - defp format_details_item(%{kind: :in_struct_field, name: name, default: default}, _context) do + defp format_details_item(%{kind: :in_struct_field, name: name, default: default}) do join_with_divider([ code(name), """ @@ -479,35 +475,27 @@ defmodule Livebook.Intellisense do ]) end - defp format_details_item( - %{kind: :module, module: module, documentation: documentation}, - context - ) do + defp format_details_item(%{kind: :module, module: module, documentation: documentation}) do join_with_divider([ code(inspect(module)), - format_definition_link(module, context, {:module, module}), format_docs_link(module), format_documentation(documentation, :all) ]) end - defp format_details_item( - %{ - kind: :function, - module: module, - name: name, - arity: arity, - documentation: documentation, - signatures: signatures, - specs: specs, - meta: meta - }, - context - ) do + defp format_details_item(%{ + kind: :function, + module: module, + name: name, + arity: arity, + documentation: documentation, + signatures: signatures, + specs: specs, + meta: meta + }) do join_with_divider([ format_signatures(signatures, module) |> code(), join_with_middle_dot([ - format_definition_link(module, context, {:function, name, arity}), format_docs_link(module, {:function, name, arity}), format_meta(:since, meta) ]), @@ -517,36 +505,60 @@ defmodule Livebook.Intellisense do ]) end - defp format_details_item( - %{ - kind: :type, - module: module, - name: name, - arity: arity, - documentation: documentation, - type_spec: type_spec - }, - context - ) do + defp format_details_item(%{ + kind: :type, + module: module, + name: name, + arity: arity, + documentation: documentation, + type_spec: type_spec + }) do join_with_divider([ format_type_signature(type_spec, module) |> code(), - format_definition_link(module, context, {:type, name, arity}), format_docs_link(module, {:type, name, arity}), format_type_spec(type_spec, @extended_line_length) |> code(), format_documentation(documentation, :all) ]) end - defp format_details_item( - %{kind: :module_attribute, name: name, documentation: documentation}, - _context - ) do + defp format_details_item(%{kind: :module_attribute, name: name, documentation: documentation}) do join_with_divider([ code("@#{name}"), format_documentation(documentation, :all) ]) end + defp get_definition_location(%{kind: :module, module: module}, context) do + get_definition_location(module, context, {:module, module}) + end + + defp get_definition_location( + %{kind: :function, module: module, name: name, arity: arity}, + context + ) do + get_definition_location(module, context, {:function, name, arity}) + end + + defp get_definition_location(%{kind: :type, module: module, name: name, arity: arity}, context) do + get_definition_location(module, context, {:type, name, arity}) + end + + defp get_definition_location(_idenfitier, _context), do: nil + + defp get_definition_location(module, context, identifier) do + if context.ebin_path do + path = Path.join(context.ebin_path, "#{module}.beam") + + with true <- File.exists?(path), + {:ok, line} <- Docs.locate_definition(path, identifier) do + file = module.module_info(:compile)[:source] + %{file: to_string(file), line: line} + else + _otherwise -> nil + end + end + end + # Formatting helpers defp join_with_divider(strings), do: join_with(strings, "\n\n---\n\n") @@ -572,12 +584,6 @@ defmodule Livebook.Intellisense do """ end - defp format_definition_link(module, context, identifier) do - if query = get_definition_location(module, context, identifier) do - "[Go to definition](#go-to-definition?#{URI.encode_query(query)})" - end - end - defp format_docs_link(module, function_or_type \\ nil) do app = Application.get_application(module) module_name = module_name(module) @@ -729,56 +735,6 @@ defmodule Livebook.Intellisense do raise "unknown documentation format #{inspect(format)}" end - @doc """ - Returns the identifier definition located in `column` in `line`. - """ - @spec get_definitions(String.t(), pos_integer(), context(), node()) :: - Runtime.definition_response() | nil - def get_definitions(line, column, context, node) do - case IdentifierMatcher.locate_identifier(line, column, context, node) do - %{matches: []} -> - nil - - %{matches: matches, range: range} -> - matches - |> Enum.sort_by(& &1[:arity], :asc) - |> Enum.flat_map(&List.wrap(get_definition_location(&1, context))) - |> case do - [%{file: file, line: line} | _] -> %{range: range, file: file, line: line} - _ -> nil - end - end - end - - defp get_definition_location(%{kind: :module, module: module}, context) do - get_definition_location(module, context, {:module, module}) - end - - defp get_definition_location( - %{kind: :function, module: module, name: name, arity: arity}, - context - ) do - get_definition_location(module, context, {:function, name, arity}) - end - - defp get_definition_location(%{kind: :type, module: module, name: name, arity: arity}, context) do - get_definition_location(module, context, {:type, name, arity}) - end - - defp get_definition_location(module, context, identifier) do - if context.ebin_path do - path = Path.join(context.ebin_path, "#{module}.beam") - - with true <- File.exists?(path), - {:ok, line} <- Docs.locate_definition(path, identifier) do - file = module.module_info(:compile)[:source] - %{file: to_string(file), line: line} - else - _otherwise -> nil - end - end - end - # Erlang HTML AST # See https://erlang.org/doc/apps/erl_docgen/doc_storage.html#erlang-documentation-format diff --git a/lib/livebook/runtime.ex b/lib/livebook/runtime.ex index a280d658c..83ddf0f2b 100644 --- a/lib/livebook/runtime.ex +++ b/lib/livebook/runtime.ex @@ -503,7 +503,6 @@ defprotocol Livebook.Runtime do @type intellisense_request :: completion_request() | details_request() - | definition_request() | signature_request() | format_request() @@ -517,7 +516,6 @@ defprotocol Livebook.Runtime do nil | completion_response() | details_response() - | definition_response() | signature_response() | format_response() @@ -552,22 +550,8 @@ defprotocol Livebook.Runtime do from: non_neg_integer(), to: non_neg_integer() }, - contents: list(String.t()) - } - - @typedoc """ - Looks up more the definition about an identifier found in `column` in - `line`. - """ - @type definition_request :: {:definition, line :: String.t(), column :: pos_integer()} - - @type definition_response :: %{ - range: %{ - from: non_neg_integer(), - to: non_neg_integer() - }, - line: pos_integer(), - file: String.t() + contents: list(String.t()), + definition: %{file: String.t(), line: pos_integer()} | nil } @typedoc """ diff --git a/lib/livebook_web/live/session_live.ex b/lib/livebook_web/live/session_live.ex index 2e30bc11f..538a05c0c 100644 --- a/lib/livebook_web/live/session_live.ex +++ b/lib/livebook_web/live/session_live.ex @@ -616,10 +616,6 @@ defmodule LivebookWeb.SessionLive do column = Text.JS.js_column_to_elixir(column, line) {:details, line, column} - %{"type" => "definition", "line" => line, "column" => column} -> - column = Text.JS.js_column_to_elixir(column, line) - {:definition, line, column} - %{"type" => "signature", "hint" => hint} -> {:signature, hint} diff --git a/test/livebook/intellisense_test.exs b/test/livebook/intellisense_test.exs index d560e0c21..a4ded3f07 100644 --- a/test/livebook/intellisense_test.exs +++ b/test/livebook/intellisense_test.exs @@ -1616,75 +1616,60 @@ defmodule Livebook.IntellisenseTest do assert content =~ ~r"https://www.erlang.org/doc/man/string.html#uppercase-1" end - end - @tag :tmp_dir - test "get_definitions/4 returns the go to definition query string", %{tmp_dir: tmp_dir} do - Code.put_compiler_option(:debug_info, true) + @tag :tmp_dir + test "includes definition location for runtime modules", %{tmp_dir: tmp_dir} do + Code.put_compiler_option(:debug_info, true) - context = - eval tmp_dir do - alias Livebook.IntellisenseTest.GoToDefinition - end + context = + eval tmp_dir do + alias Livebook.IntellisenseTest.GoToDefinition + end - code = ~S''' - defmodule Livebook.IntellisenseTest.GoToDefinition do - @type t :: term() - @type foo :: foo(:bar) - @type foo(var) :: {var, t()} + code = ~S''' + defmodule Livebook.IntellisenseTest.GoToDefinition do + @type t :: term() + @type foo :: foo(:bar) + @type foo(var) :: {var, t()} - defmacro with_logging(do: block) do - quote do - require Logger - Logger.debug("Running code") - result = unquote(block) - Logger.debug("Result: #{inspect(result)}") - result + defmacro with_logging(do: block) do + quote do + require Logger + Logger.debug("Running code") + result = unquote(block) + Logger.debug("Result: #{inspect(result)}") + result + end + end + + @spec hello(var :: term()) :: foo(term()) + def hello(message) do + {:bar, message} end end + ''' - @spec hello(var :: term()) :: foo(term()) - def hello(message) do - {:bar, message} - end + file = "#{__ENV__.file}#cell:#{Livebook.Utils.random_short_id()}" + compile_and_save_bytecode(tmp_dir, code, file) + + assert %{definition: %{line: 1, file: ^file}} = + Intellisense.get_details("GoToDefinition", 14, context, node()) + + assert %{definition: %{line: 2, file: ^file}} = + Intellisense.get_details("GoToDefinition.t", 16, context, node()) + + # Currently we are fetching location of the lowest arity + assert %{definition: %{line: 3, file: ^file}} = + Intellisense.get_details("GoToDefinition.foo", 18, context, node()) + + assert %{definition: %{line: 6, file: ^file}} = + Intellisense.get_details("GoToDefinition.with_logging", 20, context, node()) + + assert %{definition: %{line: 17, file: ^file}} = + Intellisense.get_details("GoToDefinition.hello", 18, context, node()) + after + Code.put_compiler_option(:debug_info, false) end - ''' - - file = "#{__ENV__.file}#cell:#{Livebook.Utils.random_short_id()}" - compile_and_save_bytecode(tmp_dir, code, file) - - assert Intellisense.get_definitions("GoToDefinition", 14, context, node()) == %{ - line: 1, - file: file, - range: %{to: 15, from: 1} - } - - assert Intellisense.get_definitions("GoToDefinition.t", 16, context, node()) == %{ - line: 2, - file: file, - range: %{to: 17, from: 1} - } - - # For now, we aren't fetching the expected arity but we will address it later. - assert Intellisense.get_definitions("GoToDefinition.foo", 18, context, node()) == %{ - line: 3, - file: file, - range: %{to: 19, from: 1} - } - - assert Intellisense.get_definitions("GoToDefinition.with_logging", 20, context, node()) == %{ - line: 6, - file: file, - range: %{to: 28, from: 1} - } - - assert Intellisense.get_definitions("GoToDefinition.hello", 18, context, node()) == %{ - line: 17, - file: file, - range: %{to: 21, from: 1} - } - after - Code.put_compiler_option(:debug_info, false) end describe "get_signature_items/3" do