From fafb2ca1c0d079a9ee4be15a0282fe089243fb82 Mon Sep 17 00:00:00 2001 From: Mitchell Hanberg Date: Sat, 17 Jun 2023 22:45:08 -0700 Subject: [PATCH] feat(elixir): compiler diagnostics (#8) Introduce the "extension" concept and creates the ElixirExtension, which provides Elixir compiler diagnostics. TODO: Correct the column information on the diagnostics. I think there are some commits on main that fix some of this. But there are Tokenizer diagnostics that need some massaging as well. The interface for extensions also needs to to be finalized. --- lib/next_ls.ex | 82 ++++++++++--- lib/next_ls/diagnostic_cache.ex | 35 ++++++ lib/next_ls/extensions/elixir_extension.ex | 94 ++++++++++++++ lib/next_ls/lsp_supervisor.ex | 10 +- lib/next_ls/runtime.ex | 29 +++-- priv/monkey/_next_ls_private_compiler.ex | 6 +- .../extensions/elixir_extension_test.exs | 105 ++++++++++++++++ test/next_ls/runtime_test.exs | 30 ++++- test/next_ls_test.exs | 116 ++++++++---------- test/support/project/lib/bar.ex | 2 + 10 files changed, 412 insertions(+), 97 deletions(-) create mode 100644 lib/next_ls/diagnostic_cache.ex create mode 100644 lib/next_ls/extensions/elixir_extension.ex create mode 100644 test/next_ls/extensions/elixir_extension_test.exs diff --git a/lib/next_ls.ex b/lib/next_ls.ex index 30ca5413..b93a5c18 100644 --- a/lib/next_ls.ex +++ b/lib/next_ls.ex @@ -29,8 +29,18 @@ defmodule NextLS do TextDocumentSyncOptions } + alias NextLS.Runtime + alias NextLS.DiagnosticCache + def start_link(args) do - {args, opts} = Keyword.split(args, [:task_supervisor, :runtime_supervisor]) + {args, opts} = + Keyword.split(args, [ + :cache, + :task_supervisor, + :dynamic_supervisor, + :extensions, + :extension_registry + ]) GenLSP.start_link(__MODULE__, args, opts) end @@ -38,15 +48,21 @@ defmodule NextLS do @impl true def init(lsp, args) do task_supervisor = Keyword.fetch!(args, :task_supervisor) - runtime_supervisor = Keyword.fetch!(args, :runtime_supervisor) + dynamic_supervisor = Keyword.fetch!(args, :dynamic_supervisor) + extension_registry = Keyword.fetch!(args, :extension_registry) + extensions = Keyword.get(args, :extensions, [NextLS.ElixirExtension]) + cache = Keyword.fetch!(args, :cache) {:ok, assign(lsp, exit_code: 1, documents: %{}, refresh_refs: %{}, + cache: cache, task_supervisor: task_supervisor, - runtime_supervisor: runtime_supervisor, + dynamic_supervisor: dynamic_supervisor, + extension_registry: extension_registry, + extensions: extensions, runtime_task: nil, ready: false )} @@ -90,12 +106,20 @@ defmodule NextLS do working_dir = URI.parse(lsp.assigns.root_uri).path + for extension <- lsp.assigns.extensions do + {:ok, _} = + DynamicSupervisor.start_child( + lsp.assigns.dynamic_supervisor, + {extension, cache: lsp.assigns.cache, registry: lsp.assigns.extension_registry, publisher: self()} + ) + end + GenLSP.log(lsp, "[NextLS] Booting runime...") {:ok, runtime} = DynamicSupervisor.start_child( - lsp.assigns.runtime_supervisor, - {NextLS.Runtime, working_dir: working_dir, parent: self()} + lsp.assigns.dynamic_supervisor, + {NextLS.Runtime, extension_registry: lsp.assigns.extension_registry, working_dir: working_dir, parent: self()} ) Process.monitor(runtime) @@ -117,7 +141,7 @@ defmodule NextLS do :ready end) - {:noreply, assign(lsp, runtime_task: task)} + {:noreply, assign(lsp, refresh_refs: Map.put(lsp.assigns.refresh_refs, task.ref, task.ref), runtime_task: task)} end def handle_notification(%TextDocumentDidSave{}, %{assigns: %{ready: false}} = lsp) do @@ -133,7 +157,15 @@ defmodule NextLS do }, %{assigns: %{ready: true}} = lsp ) do - {:noreply, lsp |> then(&put_in(&1.assigns.documents[uri], String.split(text, "\n")))} + task = + Task.Supervisor.async_nolink(lsp.assigns.task_supervisor, fn -> + Runtime.compile(lsp.assigns.runtime) + end) + + {:noreply, + lsp + |> then(&put_in(&1.assigns.documents[uri], String.split(text, "\n"))) + |> then(&put_in(&1.assigns.refresh_refs[task.ref], task.ref))} end def handle_notification(%TextDocumentDidChange{}, %{assigns: %{ready: false}} = lsp) do @@ -142,7 +174,7 @@ defmodule NextLS do def handle_notification(%TextDocumentDidChange{}, lsp) do for task <- Task.Supervisor.children(lsp.assigns.task_supervisor), - task != lsp.assigns.runtime_task do + task != lsp.assigns.runtime_task.pid do Process.exit(task, :kill) end @@ -170,6 +202,24 @@ defmodule NextLS do {:noreply, lsp} end + def handle_info(:publish, lsp) do + all = + for {_namespace, cache} <- DiagnosticCache.get(lsp.assigns.cache), {file, diagnostics} <- cache, reduce: %{} do + d -> Map.update(d, file, diagnostics, fn value -> value ++ diagnostics end) + end + + for {file, diagnostics} <- all do + GenLSP.notify(lsp, %GenLSP.Notifications.TextDocumentPublishDiagnostics{ + params: %GenLSP.Structures.PublishDiagnosticsParams{ + uri: "file://#{file}", + diagnostics: diagnostics + } + }) + end + + {:noreply, lsp} + end + def handle_info({ref, resp}, %{assigns: %{refresh_refs: refs}} = lsp) when is_map_key(refs, ref) do Process.demonitor(ref, [:flush]) @@ -178,19 +228,21 @@ defmodule NextLS do lsp = case resp do :ready -> - assign(lsp, ready: true) + task = + Task.Supervisor.async_nolink(lsp.assigns.task_supervisor, fn -> + Runtime.compile(lsp.assigns.runtime) + end) + + assign(lsp, ready: true, refresh_refs: Map.put(refs, task.ref, task.ref)) _ -> - lsp + assign(lsp, refresh_refs: refs) end - {:noreply, assign(lsp, refresh_refs: refs)} + {:noreply, lsp} end - def handle_info( - {:DOWN, ref, :process, _pid, _reason}, - %{assigns: %{refresh_refs: refs}} = lsp - ) + def handle_info({:DOWN, ref, :process, _pid, _reason}, %{assigns: %{refresh_refs: refs}} = lsp) when is_map_key(refs, ref) do {_token, refs} = Map.pop(refs, ref) diff --git a/lib/next_ls/diagnostic_cache.ex b/lib/next_ls/diagnostic_cache.ex new file mode 100644 index 00000000..83c0cb30 --- /dev/null +++ b/lib/next_ls/diagnostic_cache.ex @@ -0,0 +1,35 @@ +defmodule NextLS.DiagnosticCache do + # TODO: this should be an ETS table + @moduledoc """ + Cache for diagnostics. + """ + use Agent + + def start_link(opts) do + Agent.start_link(fn -> Map.new() end, Keyword.take(opts, [:name])) + end + + def get(cache) do + Agent.get(cache, & &1) + end + + def put(cache, namespace, filename, diagnostic) do + Agent.update(cache, fn cache -> + Map.update(cache, namespace, %{filename => [diagnostic]}, fn cache -> + Map.update(cache, filename, [diagnostic], fn v -> + [diagnostic | v] + end) + end) + end) + end + + def clear(cache, namespace) do + Agent.update(cache, fn cache -> + Map.update(cache, namespace, %{}, fn cache -> + for {k, _} <- cache, into: Map.new() do + {k, []} + end + end) + end) + end +end diff --git a/lib/next_ls/extensions/elixir_extension.ex b/lib/next_ls/extensions/elixir_extension.ex new file mode 100644 index 00000000..e27b8827 --- /dev/null +++ b/lib/next_ls/extensions/elixir_extension.ex @@ -0,0 +1,94 @@ +defmodule NextLS.ElixirExtension do + use GenServer + + alias NextLS.DiagnosticCache + + def start_link(args) do + GenServer.start_link( + __MODULE__, + Keyword.take(args, [:cache, :registry, :publisher]), + Keyword.take(args, [:name]) + ) + end + + @impl GenServer + def init(args) do + cache = Keyword.fetch!(args, :cache) + registry = Keyword.fetch!(args, :registry) + publisher = Keyword.fetch!(args, :publisher) + + Registry.register(registry, :extension, :elixir) + + {:ok, %{cache: cache, registry: registry, publisher: publisher}} + end + + @impl GenServer + def handle_info({:compiler, diagnostics}, state) do + DiagnosticCache.clear(state.cache, :elixir) + + for d <- diagnostics do + # TODO: some compiler diagnostics only have the line number + # but we want to only highlight the source code, so we + # need to read the text of the file (either from the lsp cache + # if the source code is "open", or read from disk) and calculate the + # column of the first non-whitespace character. + # + # it is not clear to me whether the LSP process or the extension should + # be responsible for this. The open documents live in the LSP process + DiagnosticCache.put(state.cache, :elixir, d.file, %GenLSP.Structures.Diagnostic{ + severity: severity(d.severity), + message: d.message, + source: d.compiler_name, + range: range(d.position) + }) + end + + send(state.publisher, :publish) + + {:noreply, state} + end + + defp severity(:error), do: GenLSP.Enumerations.DiagnosticSeverity.error() + defp severity(:warning), do: GenLSP.Enumerations.DiagnosticSeverity.warning() + defp severity(:info), do: GenLSP.Enumerations.DiagnosticSeverity.information() + defp severity(:hint), do: GenLSP.Enumerations.DiagnosticSeverity.hint() + + defp range({start_line, start_col, end_line, end_col}) do + %GenLSP.Structures.Range{ + start: %GenLSP.Structures.Position{ + line: start_line - 1, + character: start_col + }, + end: %GenLSP.Structures.Position{ + line: end_line - 1, + character: end_col + } + } + end + + defp range({line, col}) do + %GenLSP.Structures.Range{ + start: %GenLSP.Structures.Position{ + line: line - 1, + character: col + }, + end: %GenLSP.Structures.Position{ + line: line - 1, + character: 999 + } + } + end + + defp range(line) do + %GenLSP.Structures.Range{ + start: %GenLSP.Structures.Position{ + line: line - 1, + character: 0 + }, + end: %GenLSP.Structures.Position{ + line: line - 1, + character: 999 + } + } + end +end diff --git a/lib/next_ls/lsp_supervisor.ex b/lib/next_ls/lsp_supervisor.ex index b48d194c..1c9e5fb1 100644 --- a/lib/next_ls/lsp_supervisor.ex +++ b/lib/next_ls/lsp_supervisor.ex @@ -33,10 +33,16 @@ defmodule NextLS.LSPSupervisor do end children = [ - {DynamicSupervisor, name: NextLS.RuntimeSupervisor}, + {DynamicSupervisor, name: NextLS.DynamicSupervisor}, {Task.Supervisor, name: NextLS.TaskSupervisor}, {GenLSP.Buffer, buffer_opts}, - {NextLS, task_supervisor: NextLS.TaskSupervisor, runtime_supervisor: NextLS.RuntimeSupervisor} + {NextLS.DiagnosticCache, [name: :diagnostic_cache]}, + {Registry, name: NextLS.ExtensionRegistry, keys: :duplicate}, + {NextLS, + cache: :diagnostic_cache, + task_supervisor: NextLS.TaskSupervisor, + dynamic_supervisor: NextLS.DynamicSupervisor, + extension_registry: NextLS.ExtensionRegistry} ] Supervisor.init(children, strategy: :one_for_one) diff --git a/lib/next_ls/runtime.ex b/lib/next_ls/runtime.ex index 410a446e..9d7a00ce 100644 --- a/lib/next_ls/runtime.ex +++ b/lib/next_ls/runtime.ex @@ -32,11 +32,16 @@ defmodule NextLS.Runtime do end end + def compile(server) do + GenServer.call(server, :compile) + end + @impl GenServer def init(opts) do sname = "nextls#{System.system_time()}" working_dir = Keyword.fetch!(opts, :working_dir) parent = Keyword.fetch!(opts, :parent) + extension_registry = Keyword.fetch!(opts, :extension_registry) port = Port.open( @@ -80,21 +85,13 @@ defmodule NextLS.Runtime do |> Path.join("monkey/_next_ls_private_compiler.ex") |> then(&:rpc.call(node, Code, :compile_file, [&1])) - :ok = - :rpc.call( - node, - :_next_ls_private_compiler, - :compile, - [] - ) - send(me, {:node, node}) else _ -> send(me, :cancel) end end) - {:ok, %{port: port, parent: parent}} + {:ok, %{port: port, parent: parent, errors: nil, extension_registry: extension_registry}} end @impl GenServer @@ -111,6 +108,20 @@ defmodule NextLS.Runtime do {:reply, reply, state} end + def handle_call(:compile, _, %{node: node} = state) do + {_, errors} = :rpc.call(node, :_next_ls_private_compiler, :compile, []) + + foo = "foo" + + Registry.dispatch(state.extension_registry, :extension, fn entries -> + for {pid, _} <- entries do + send(pid, {:compiler, errors}) + end + end) + + {:reply, errors, %{state | errors: errors}} + end + @impl GenServer def handle_info({:node, node}, state) do Node.monitor(node, true) diff --git a/priv/monkey/_next_ls_private_compiler.ex b/priv/monkey/_next_ls_private_compiler.ex index ef291e83..86d28aa6 100644 --- a/priv/monkey/_next_ls_private_compiler.ex +++ b/priv/monkey/_next_ls_private_compiler.ex @@ -5,6 +5,8 @@ defmodule :_next_ls_private_compiler do # keep stdout on this node Process.group_leader(self(), Process.whereis(:user)) + Mix.Task.clear() + # load the paths for deps and compile them # will noop if they are already compiled # The mix cli basically runs this before any mix task @@ -13,9 +15,7 @@ defmodule :_next_ls_private_compiler do # --no-compile, so nothing was compiled, but the # task was not re-enabled it seems Mix.Task.rerun("deps.loadpaths") - Mix.Task.rerun("compile") - - :ok + Mix.Task.rerun("compile", ["--no-protocol-consolidation", "--return-errors"]) rescue e -> {:error, e} end diff --git a/test/next_ls/extensions/elixir_extension_test.exs b/test/next_ls/extensions/elixir_extension_test.exs new file mode 100644 index 00000000..7b332bec --- /dev/null +++ b/test/next_ls/extensions/elixir_extension_test.exs @@ -0,0 +1,105 @@ +defmodule NextLS.ElixirExtensionTest do + use ExUnit.Case, async: true + + alias NextLS.ElixirExtension + alias NextLS.DiagnosticCache + + setup do + cache = start_supervised!(DiagnosticCache) + start_supervised!({Registry, [keys: :unique, name: Registry.ElixirExtensionTest]}) + + extension = + start_supervised!({ElixirExtension, cache: cache, registry: Registry.ElixirExtensionTest, publisher: self()}) + + Process.link(extension) + + [extension: extension, cache: cache] + end + + test "inserts lsp diagnostics into cache", %{extension: extension, cache: cache} do + only_line = %Mix.Task.Compiler.Diagnostic{ + file: "lib/bar.ex", + severity: :warning, + message: "kind of bad", + position: 2, + compiler_name: "Elixir", + details: nil + } + + line_and_col = %Mix.Task.Compiler.Diagnostic{ + file: "lib/foo.ex", + severity: :error, + message: "nothing works", + position: {4, 7}, + compiler_name: "Elixir", + details: nil + } + + start_and_end = %Mix.Task.Compiler.Diagnostic{ + file: "lib/baz.ex", + severity: :hint, + message: "here's a hint", + position: {4, 7, 8, 3}, + compiler_name: "Elixir", + details: nil + } + + send(extension, {:compiler, [only_line, line_and_col, start_and_end]}) + + assert_receive :publish + + assert %{ + only_line.file => [ + %GenLSP.Structures.Diagnostic{ + severity: 2, + message: "kind of bad", + source: "Elixir", + range: %GenLSP.Structures.Range{ + start: %GenLSP.Structures.Position{ + line: 1, + character: 0 + }, + end: %GenLSP.Structures.Position{ + line: 1, + character: 999 + } + } + } + ], + line_and_col.file => [ + %GenLSP.Structures.Diagnostic{ + severity: 1, + message: "nothing works", + source: "Elixir", + range: %GenLSP.Structures.Range{ + start: %GenLSP.Structures.Position{ + line: 3, + character: 7 + }, + end: %GenLSP.Structures.Position{ + line: 3, + character: 999 + } + } + } + ], + start_and_end.file => [ + %GenLSP.Structures.Diagnostic{ + severity: 4, + message: "here's a hint", + source: "Elixir", + range: %GenLSP.Structures.Range{ + start: %GenLSP.Structures.Position{ + line: 3, + character: 7 + }, + end: %GenLSP.Structures.Position{ + line: 7, + character: 3 + } + } + } + ] + } == DiagnosticCache.get(cache).elixir + end +end diff --git a/test/next_ls/runtime_test.exs b/test/next_ls/runtime_test.exs index 84c19c3b..7dc54d4c 100644 --- a/test/next_ls/runtime_test.exs +++ b/test/next_ls/runtime_test.exs @@ -28,13 +28,39 @@ defmodule NextLs.RuntimeTest do [logger: logger, cwd: Path.absname(tmp_dir)] end - test "can run code on the node", %{logger: logger, cwd: cwd} do + test "compiles the code and returns diagnostics", %{logger: logger, cwd: cwd} do + start_supervised!({Registry, keys: :unique, name: RuntimeTestRegistry}) + capture_log(fn -> - pid = start_supervised!({Runtime, working_dir: cwd, parent: logger}) + pid = start_supervised!({Runtime, working_dir: cwd, parent: logger, extension_registry: RuntimeTestRegistry}) Process.link(pid) assert wait_for_ready(pid) + + file = Path.join(cwd, "lib/bar.ex") + + assert [ + %Mix.Task.Compiler.Diagnostic{ + file: ^file, + severity: :warning, + message: + "variable \"arg1\" is unused (if the variable is not meant to be used, prefix it with an underscore)", + position: 2, + compiler_name: "Elixir", + details: nil + } + ] = Runtime.compile(pid) + + File.write!(file, """ + defmodule Bar do + def foo(arg1) do + arg1 + end + end + """) + + assert [] == Runtime.compile(pid) end) =~ "Connected to node" end diff --git a/test/next_ls_test.exs b/test/next_ls_test.exs index b802766f..76b8cbf6 100644 --- a/test/next_ls_test.exs +++ b/test/next_ls_test.exs @@ -12,11 +12,17 @@ defmodule NextLSTest do tvisor = start_supervised!(Task.Supervisor) rvisor = start_supervised!({DynamicSupervisor, [strategy: :one_for_one]}) + start_supervised!({Registry, [keys: :unique, name: Registry.NextLSTest]}) + extensions = [NextLS.ElixirExtension] + cache = start_supervised!(NextLS.DiagnosticCache) server = server(NextLS, task_supervisor: tvisor, - runtime_supervisor: rvisor + dynamic_supervisor: rvisor, + extension_registry: Registry.NextLSTest, + extensions: extensions, + cache: cache ) Process.link(server.lsp) @@ -46,10 +52,8 @@ defmodule NextLSTest do params: %{} }) - assert_notification( - "window/logMessage", - %{"message" => "[NextLS] Runtime ready..."} - ) + assert_notification "window/logMessage", + %{"message" => "[NextLS] Runtime ready..."} assert :ok == request(client, %{ @@ -121,71 +125,51 @@ defmodule NextLSTest do ) end - @tag :pending - test "publishes diagnostics once the client has initialized", %{ - client: _client, - cwd: _cwd - } do - # assert :ok == - # notify(client, %{ - # method: "initialized", - # jsonrpc: "2.0", - # params: %{} - # }) + test "publishes diagnostics once the client has initialized", %{client: client, cwd: cwd} do + assert :ok == + notify(client, %{ + method: "initialized", + jsonrpc: "2.0", + params: %{} + }) - # assert_notification( - # "window/logMessage", - # %{ - # "message" => "[NextLS] LSP Initialized!", - # "type" => 4 - # } - # ) + assert_notification( + "window/logMessage", + %{ + "message" => "[NextLS] LSP Initialized!", + "type" => 4 + } + ) # assert_notification("$/progress", %{"value" => %{"kind" => "begin"}}) - # for file <- ["foo.ex", "bar.ex"] do - # uri = - # to_string(%URI{ - # host: "", - # scheme: "file", - # path: Path.join([cwd, "lib", file]) - # }) - - # assert_notification( - # "textDocument/publishDiagnostics", - # %{ - # "uri" => ^uri, - # "diagnostics" => [ - # %{ - # "source" => "credo", - # "code" => "NextLS.Check.Readability.ModuleDoc", - # "codeDescription" => %{ - # "href" => "https://hexdocs.pm/credo/NextLS.Check.Readability.ModuleDoc.html" - # }, - # "severity" => 3 - # } - # ] - # } - # ) - # end - - # uri = - # to_string(%URI{ - # host: "", - # scheme: "file", - # path: Path.join([cwd, "lib", "code_action.ex"]) - # }) - - # assert_notification( - # "textDocument/publishDiagnostics", - # %{ - # "uri" => ^uri, - # "diagnostics" => [ - # %{"severity" => 3}, - # %{"severity" => 3} - # ] - # } - # ) + for file <- ["bar.ex"] do + uri = + to_string(%URI{ + host: "", + scheme: "file", + path: Path.join([cwd, "lib", file]) + }) + + assert_notification( + "textDocument/publishDiagnostics", + %{ + "uri" => ^uri, + "diagnostics" => [ + %{ + "source" => "Elixir", + "severity" => 2, + "message" => + "variable \"arg1\" is unused (if the variable is not meant to be used, prefix it with an underscore)", + "range" => %{ + "start" => %{"line" => 1, "character" => 0}, + "end" => %{"line" => 1, "character" => 999} + } + } + ] + } + ) + end # assert_notification( # "$/progress", diff --git a/test/support/project/lib/bar.ex b/test/support/project/lib/bar.ex index cb2fd689..0f6fd837 100644 --- a/test/support/project/lib/bar.ex +++ b/test/support/project/lib/bar.ex @@ -1,2 +1,4 @@ defmodule Bar do + def foo(arg1) do + end end