lexical-lsp / lexical

Lexical is a next-generation elixir language server
877 stars 81 forks source link

plugin empty diagnostics will clear mix diagnostics #207

Closed scottming closed 1 year ago

scottming commented 1 year ago

I noticed that there is a delay in Diagnostics when editing now. It only appears when I stop editing, but not while actively making changes. This creates a significant difference in user experience compared to before and doesn't feel as real-time.

I think this will cause a lot of disturbance to users, because the Diagnostic will keep flashing during the editing process and it is not accurate. You can see the difference by watching these two 10-second videos below.

now

https://github.com/lexical-lsp/lexical/assets/12830256/f10705ef-1c00-4bb1-94ea-483794495770

before

https://github.com/lexical-lsp/lexical/assets/12830256/8d49070b-4a53-4915-99b3-225750548f13

This is because we pushed empty Diagnostics during the editing process. When the plugin diagnostics is empty, we should not push Diagnostics because this will overwrite our original Diagnostics and cause flickering.

you can comment on this line: https://github.com/lexical-lsp/lexical/blob/30b96d9/apps/server/lib/lexical/server/state.ex#L104 to let this problem go, but we need to fix this.

scottming commented 1 year ago
[DEBUG][2023-06-05 21:08:08] .../vim/lsp/rpc.lua:387    "rpc.receive"   {
  jsonrpc = "2.0",
  method = "textDocument/publishDiagnostics",
  params = {
    diagnostics = {},
    uri = "file:///Users/scottming/Code/dummy/lib/dummy.ex"
  }
}
[DEBUG][2023-06-05 21:08:08] .../vim/lsp/rpc.lua:387    "rpc.receive"   {
  jsonrpc = "2.0",
  method = "textDocument/publishDiagnostics",
  params = {
    diagnostics = { {
        message = "warning: Enum.map/1 is undefined or private. Did you mean:\n\n      * map/2\n\n  lib/dummy.ex:6: Dummy.foo/0",
        range = {
          ["end"] = {
            character = 0,
            line = 6
          },
          start = {
            character = 4,
            line = 5
          }
        },
        severity = 2,
        source = "Elixir"
      } },
    uri = "file:///Users/scottming/Code/dummy/lib/dummy.ex"
  }
}
scohen commented 1 year ago

I don't understand what you're getting at. If anything, I prefer the "after", since there are no diagnostics on the line that are worth anything anyways. I've been pretty happy with the recent diagnostics changes, they feel much less "bouncy" and interruptive than before.

scohen commented 1 year ago

Also, prior, some diagnostics would get "stuck" and never disappear. This seems to have fixed the problem.

I need you to clarify the following:

This is because we pushed empty Diagnostics during the editing process.

What is "the editing process"? When does it start and when does it end? How do we figure this out?

When the plugin is empty,

What does it mean for a plugin to be "empty" ?

Are you asking for some kind of throttling?

scottming commented 1 year ago

If anything, I prefer the "after", since there are no diagnostics on the line that are worth anything anyways. I've been pretty happy with the recent diagnostics changes, they feel much less "bouncy" and interruptive than before.

Do you prefer the effect of this video? right?

image

I mean the suffix is 20.57.15-converted.mp4

scottming commented 1 year ago

What is "the editing process"? When does it start and when does it end? How do we figure this out?

Forgive my poor English, I am not referring to the process of Erlang, but rather the editing progress. I mean that it should have displayed "Diagnostics", but instead there was a empty Diagnostics causing flickering.

Did you notice that in this video: CleanShot.2023-06-05.at.21.00.12-converted.mp4, from 4 to 6 seconds, I was editing [] |> Enum -> [] |> Enum.map, and during this time my code was not correct at all. Therefore, the Diagnostics should not disappear but rather be updated.

scottming commented 1 year ago

Are you asking for some kind of throttling?

nope

What does it mean for a plugin to be "empty" ?

let use this example again:

edit this code from [] |> Enum to [] |> Enum.map

I use this diff to log things:

diff --git a/apps/remote_control/lib/lexical/remote_control/plugin.ex b/apps/remote_control/lib/lexical/remote_control/plugin.ex
index 0ea2abe..b040743 100644
--- a/apps/remote_control/lib/lexical/remote_control/plugin.ex
+++ b/apps/remote_control/lib/lexical/remote_control/plugin.ex
@@ -34,6 +34,8 @@ defmodule Lexical.RemoteControl.Plugin do
         diagnostics: diagnostics
       )

+    Logger.info("plugin diagnostic_message: #{inspect(diagnostic_message)}")
+
     RemoteControl.notify_listener(diagnostic_message)
   end

diff --git a/apps/server/lib/lexical/server/project/diagnostics.ex b/apps/server/lib/lexical/server/project/diagnostics.ex
index 455919f..cb37ddf 100644
--- a/apps/server/lib/lexical/server/project/diagnostics.ex
+++ b/apps/server/lib/lexical/server/project/diagnostics.ex
@@ -114,8 +114,15 @@ defmodule Lexical.Server.Project.Diagnostics do
   end

   @impl GenServer
-  def handle_info(file_diagnostics(uri: uri, diagnostics: diagnostics), %State{} = state) do
+  def handle_info(
+        file_diagnostics(uri: uri, diagnostics: diagnostics),
+        %State{} = state
+      ) do
+    Logger.info("file diagnostics: #{inspect diagnostics}")
+
+    Logger.info("state diagnostics by uri before: #{inspect(state.diagnostics_by_uri)}")
     state = State.clear(state, uri)
+    Logger.info("state diagnostics by uri after: #{inspect(state.diagnostics_by_uri)}")

     state =
       Enum.reduce(diagnostics, state, fn diagnostic, state ->

you'll see

09:25:11.210 [info] plugin diagnostic_message: {:file_diagnostics, %Lexical.Project{root_uri: "file:///Users/scottming/Code/dummy", mix_exs_uri: "file:///Users/scottming/Code/dummy/mix.exs", mix_project?: true, mix_env: nil, mix_target: nil, env_variables: %{}}, "file:///Users/scottming/Code/dummy/lib/dummy.ex", []}
09:25:11.210 [info] file diagnostics: []
09:25:11.210 [info] state diagnostics by uri before: %{"file:///Users/scottming/Code/dummy/lib/dummy.ex" => []}
09:25:11.210 [info] state diagnostics by uri after: %{"file:///Users/scottming/Code/dummy/lib/dummy.ex" => []}
09:25:11.340 [info] plugin diagnostic_message: {:file_diagnostics, %Lexical.Project{root_uri: "file:///Users/scottming/Code/dummy", mix_exs_uri: "file:///Users/scottming/Code/dummy/mix.exs", mix_project?: true, mix_env: nil, mix_target: nil, env_variables: %{}}, "file:///Users/scottming/Code/dummy/lib/dummy.ex", []}
09:25:11.340 [info] file diagnostics: []
09:25:11.340 [info] state diagnostics by uri before: %{"file:///Users/scottming/Code/dummy/lib/dummy.ex" => []}
09:25:11.340 [info] state diagnostics by uri after: %{"file:///Users/scottming/Code/dummy/lib/dummy.ex" => []}
09:25:11.639 [info] file diagnostics: [%Lexical.Plugin.Diagnostic.Result{details: {Dummy, :foo, 0}, message: "warning: Enum.map/1 is undefined or private. Did you mean:\n\n      * map/2\n\n  lib/dummy.ex:5: Dummy.foo/0", position: 5, severity: :warning, source: "Elixir", uri: "file:///Users/scottming/Code/dummy/lib/dummy.ex"}]
09:25:11.639 [info] state diagnostics by uri before: %{"file:///Users/scottming/Code/dummy/lib/dummy.ex" => []}
09:25:11.639 [info] state diagnostics by uri after: %{"file:///Users/scottming/Code/dummy/lib/dummy.ex" => []}

Did you notice the overwriting?