pull configuration on workspace/didChangeConfiguration

add tests
elixir-lsp · Aug 17, 2023 · 2ad5313 · 2ad5313
1 parent 70cf84e
commit 2ad5313
Show file tree

Hide file tree

Showing 7 changed files with 257 additions and 203 deletions.
diff --git a/apps/language_server/lib/language_server/server.ex b/apps/language_server/lib/language_server/server.ex
@@ -294,32 +294,7 @@ defmodule ElixirLS.LanguageServer.Server do
     # according to https://github.com/microsoft/language-server-protocol/issues/567#issuecomment-1060605611
     # this is the best point to pull configuration
 
-    supports_get_configuration =
-      get_in(state.client_capabilities, [
-        "workspace",
-        "configuration"
-      ])
-
-    state =
-      if supports_get_configuration do
-        response = JsonRpc.get_configuration_request(state.root_uri, "elixirLS")
-
-        case response do
-          {:ok, [result]} when is_map(result) ->
-            Logger.info(
-              "Received client configuration via workspace/configuration\n#{inspect(result)}"
-            )
-
-            set_settings(state, result)
-
-          other ->
-            Logger.error("Cannot get client configuration: #{inspect(other)}")
-            state
-        end
-      else
-        Logger.info("Client does not support workspace/configuration request")
-        state
-      end
+    state = pull_configuration(state)
 
     unless state.settings do
       # We still don't have the configuration. Let's wait for workspace/didChangeConfiguration
@@ -372,25 +347,32 @@ defmodule ElixirLS.LanguageServer.Server do
   end
 
   # We don't start performing builds until we receive settings from the client in case they've set
-  # the `projectDir` or `mixEnv` settings. If the settings don't match the format expected, leave
-  # settings unchanged or set default settings if this is the first request.
+  # the `projectDir` or `mixEnv` settings.
+  # note that clients supporting `workspace/configuration` will send nil and we need to pull
   defp handle_notification(did_change_configuration(changed_settings), state = %__MODULE__{}) do
-    Logger.info(
-      "Received client configuration via workspace/didChangeConfiguration:\n#{inspect(changed_settings)}"
-    )
-
+    Logger.info("Received workspace/didChangeConfiguration")
     prev_settings = state.settings || %{}
 
-    new_settings =
-      case changed_settings do
-        %{"elixirLS" => settings} when is_map(settings) ->
-          Map.merge(prev_settings, settings)
+    case changed_settings do
+      %{"elixirLS" => settings} when is_map(settings) ->
+        Logger.info(
+          "Received client configuration via workspace/didChangeConfiguration\n#{inspect(settings)}"
+        )
 
-        _ ->
-          prev_settings
-      end
+        # deprecated push based configuration - interesting config updated
+        new_settings = Map.merge(prev_settings, settings)
+        set_settings(state, new_settings)
+
+      nil ->
+        # pull based configuration
+        # on notification the server should pull current configuration
+        # see https://github.com/microsoft/language-server-protocol/issues/1792
+        pull_configuration(state)
 
-    set_settings(state, new_settings)
+      _ ->
+        # deprecated push based configuration - some other config updated
+        set_settings(state, prev_settings)
+    end
   end
 
   defp handle_notification(notification("exit"), state = %__MODULE__{}) do
@@ -1244,14 +1226,14 @@ defmodule ElixirLS.LanguageServer.Server do
   end
 
   defp listen_for_configuration_changes(server_instance_id) do
-    # the schema is not documented in official LSP docs
-    # using https://github.com/microsoft/vscode-languageserver-node/blob/7792b0b21c994cc9bebc3117eeb652a22e2d9e1f/protocol/src/common/protocol.ts#L1504C18-L1504C59
+    # the schema is not documented but uses https://github.com/microsoft/vscode-languageserver-node/blob/7792b0b21c994cc9bebc3117eeb652a22e2d9e1f/protocol/src/common/protocol.ts#L1504C18-L1504C59
+    # passing empty map without `section` results in notification with `{"settings": null}` config but the server should
+    # simply pull for configuration instead of depending on data sent in notification
+    # see https://github.com/microsoft/language-server-protocol/issues/1792
     case JsonRpc.register_capability_request(
            server_instance_id,
            "workspace/didChangeConfiguration",
-           %{
-             "section" => "elixirLS"
-           }
+           %{}
          ) do
       {:ok, nil} ->
         Logger.info("client/registerCapability succeeded")
@@ -1440,4 +1422,32 @@ defmodule ElixirLS.LanguageServer.Server do
       end
     end
   end
+
+  defp pull_configuration(state) do
+    supports_get_configuration =
+      get_in(state.client_capabilities, [
+        "workspace",
+        "configuration"
+      ])
+
+    if supports_get_configuration do
+      response = JsonRpc.get_configuration_request(state.root_uri, "elixirLS")
+
+      case response do
+        {:ok, [result]} when is_map(result) ->
+          Logger.info(
+            "Received client configuration via workspace/configuration\n#{inspect(result)}"
+          )
+
+          set_settings(state, result)
+
+        other ->
+          Logger.error("Cannot get client configuration: #{inspect(other)}")
+          state
+      end
+    else
+      Logger.info("Client does not support workspace/configuration request")
+      state
+    end
+  end
 end
diff --git a/apps/language_server/test/dialyzer_test.exs b/apps/language_server/test/dialyzer_test.exs
@@ -3,7 +3,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
 
   alias ElixirLS.LanguageServer.{Dialyzer, Server, Protocol, SourceFile, JsonRpc, Tracer, Build}
   import ExUnit.CaptureLog
-  alias ElixirLS.LanguageServer.Test.ServerTestHelpers
+  import ElixirLS.LanguageServer.Test.ServerTestHelpers
   use ElixirLS.Utils.MixTest.Case, async: false
   use Protocol
 
@@ -22,7 +22,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
   end
 
   setup do
-    server = ServerTestHelpers.start_server()
+    server = start_server()
     {:ok, _tracer} = start_supervised(Tracer)
 
     {:ok, %{server: server}}
@@ -34,15 +34,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
       file_a = SourceFile.Path.to_uri(Path.absname("lib/a.ex"))
 
       capture_log(fn ->
-        root_uri = SourceFile.Path.to_uri(File.cwd!())
-        Server.receive_packet(server, initialize_req(1, root_uri, %{}))
-
-        Server.receive_packet(
-          server,
-          did_change_configuration(%{
-            "elixirLS" => %{"dialyzerEnabled" => true, "dialyzerFormat" => "dialyxir_long"}
-          })
-        )
+        initialize(server, %{"dialyzerEnabled" => true, "dialyzerFormat" => "dialyxir_long"})
 
         message = assert_receive %{"method" => "textDocument/publishDiagnostics"}, 20000
 
@@ -107,15 +99,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
       file_c = SourceFile.Path.to_uri(Path.absname("lib/c.ex"))
 
       capture_log(fn ->
-        root_uri = SourceFile.Path.to_uri(File.cwd!())
-        Server.receive_packet(server, initialize_req(1, root_uri, %{}))
-
-        Server.receive_packet(
-          server,
-          did_change_configuration(%{
-            "elixirLS" => %{"dialyzerEnabled" => true, "dialyzerFormat" => "dialyxir_long"}
-          })
-        )
+        initialize(server, %{"dialyzerEnabled" => true, "dialyzerFormat" => "dialyxir_long"})
 
         assert_receive %{"method" => "textDocument/publishDiagnostics"}, 20_000
 
@@ -168,15 +152,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
       file_a = SourceFile.Path.to_uri(Path.absname("lib/a.ex"))
 
       capture_log(fn ->
-        root_uri = SourceFile.Path.to_uri(File.cwd!())
-        Server.receive_packet(server, initialize_req(1, root_uri, %{}))
-
-        Server.receive_packet(
-          server,
-          did_change_configuration(%{
-            "elixirLS" => %{"dialyzerEnabled" => true, "dialyzerFormat" => "dialyxir_long"}
-          })
-        )
+        initialize(server, %{"dialyzerEnabled" => true, "dialyzerFormat" => "dialyxir_long"})
 
         message = assert_receive %{"method" => "textDocument/publishDiagnostics"}, 20000
 
@@ -222,15 +198,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
       file_a = SourceFile.Path.to_uri(Path.absname("lib/a.ex"))
 
       capture_log(fn ->
-        root_uri = SourceFile.Path.to_uri(File.cwd!())
-        Server.receive_packet(server, initialize_req(1, root_uri, %{}))
-
-        Server.receive_packet(
-          server,
-          did_change_configuration(%{
-            "elixirLS" => %{"dialyzerEnabled" => true, "dialyzerFormat" => "dialyxir_short"}
-          })
-        )
+        initialize(server, %{"dialyzerEnabled" => true, "dialyzerFormat" => "dialyxir_short"})
 
         message = assert_receive %{"method" => "textDocument/publishDiagnostics"}, 20000
 
@@ -262,20 +230,12 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
   end
 
   @tag slow: true, fixture: true
-  test "reports dialyzer_formatted error", %{server: server} do
+  test "reports dialyzer formatted error", %{server: server} do
     in_fixture(__DIR__, "dialyzer", fn ->
       file_a = SourceFile.Path.to_uri(Path.absname("lib/a.ex"))
 
       capture_log(fn ->
-        root_uri = SourceFile.Path.to_uri(File.cwd!())
-        Server.receive_packet(server, initialize_req(1, root_uri, %{}))
-
-        Server.receive_packet(
-          server,
-          did_change_configuration(%{
-            "elixirLS" => %{"dialyzerEnabled" => true, "dialyzerFormat" => "dialyzer"}
-          })
-        )
+        initialize(server, %{"dialyzerEnabled" => true, "dialyzerFormat" => "dialyzer"})
 
         message = assert_receive %{"method" => "textDocument/publishDiagnostics"}, 20000
 
@@ -313,15 +273,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
       file_a = SourceFile.Path.to_uri(Path.absname("apps/app1/lib/app1.ex"))
 
       capture_log(fn ->
-        root_uri = SourceFile.Path.to_uri(File.cwd!())
-        Server.receive_packet(server, initialize_req(1, root_uri, %{}))
-
-        Server.receive_packet(
-          server,
-          did_change_configuration(%{
-            "elixirLS" => %{"dialyzerEnabled" => true, "dialyzerFormat" => "dialyxir_short"}
-          })
-        )
+        initialize(server, %{"dialyzerEnabled" => true, "dialyzerFormat" => "dialyxir_short"})
 
         message = assert_receive %{"method" => "textDocument/publishDiagnostics"}, 20000
 
@@ -357,13 +309,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
       file_a = SourceFile.Path.to_uri(Path.absname("lib/a.ex"))
 
       capture_log(fn ->
-        root_uri = SourceFile.Path.to_uri(File.cwd!())
-        Server.receive_packet(server, initialize_req(1, root_uri, %{}))
-
-        Server.receive_packet(
-          server,
-          did_change_configuration(%{"elixirLS" => %{"dialyzerEnabled" => true}})
-        )
+        initialize(server, %{"dialyzerEnabled" => true})
 
         assert_receive publish_diagnostics_notif(^file_a, [_, _]), 20000
 
@@ -381,16 +327,9 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
   @tag slow: true, fixture: true
   test "protocol rebuild does not trigger consolidation warnings", %{server: server} do
     in_fixture(__DIR__, "protocols", fn ->
-      root_uri = SourceFile.Path.to_uri(File.cwd!())
       uri = SourceFile.Path.to_uri(Path.absname("lib/implementations.ex"))
 
-      Server.receive_packet(server, initialize_req(1, root_uri, %{}))
-      Server.receive_packet(server, notification("initialized"))
-
-      Server.receive_packet(
-        server,
-        did_change_configuration(%{"elixirLS" => %{"dialyzerEnabled" => true}})
-      )
+      initialize(server, %{"dialyzerEnabled" => true})
 
       assert_receive notification("window/logMessage", %{"message" => "Compile took" <> _}), 5000
 
@@ -458,15 +397,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
       file_c = SourceFile.Path.to_uri(Path.absname("lib/c.ex"))
 
       capture_log(fn ->
-        root_uri = SourceFile.Path.to_uri(File.cwd!())
-        Server.receive_packet(server, initialize_req(1, root_uri, %{}))
-
-        Server.receive_packet(
-          server,
-          did_change_configuration(%{
-            "elixirLS" => %{"dialyzerEnabled" => true, "dialyzerFormat" => "dialyxir_long"}
-          })
-        )
+        initialize(server, %{"dialyzerEnabled" => true, "dialyzerFormat" => "dialyxir_long"})
 
         message = assert_receive %{"method" => "textDocument/publishDiagnostics"}, 20000
 
@@ -495,19 +426,11 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
       file_c = SourceFile.Path.to_uri(Path.absname("lib/c.ex"))
 
       capture_log(fn ->
-        root_uri = SourceFile.Path.to_uri(File.cwd!())
-        Server.receive_packet(server, initialize_req(1, root_uri, %{}))
-
-        Server.receive_packet(
-          server,
-          did_change_configuration(%{
-            "elixirLS" => %{
-              "dialyzerEnabled" => true,
-              "dialyzerFormat" => "dialyxir_long",
-              "suggestSpecs" => true
-            }
-          })
-        )
+        initialize(server, %{
+          "dialyzerEnabled" => true,
+          "dialyzerFormat" => "dialyxir_long",
+          "suggestSpecs" => true
+        })
 
         message = assert_receive %{"method" => "textDocument/publishDiagnostics"}, 20000
 
@@ -555,7 +478,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
         )
 
         assert_receive(%{
-          "id" => 1,
+          "id" => id,
           "method" => "workspace/applyEdit",
           "params" => %{
             "edit" => %{
@@ -575,19 +498,7 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
           }
         })
 
-        # TODO something is broken in packet capture
-        # using JsonRpc.receive_packet causes the packet to be delivered to LanguageServer
-        # which crashes with no match error
-        # JsonRpc.receive_packet(
-        #   server,
-        #   response(1, %{"applied" => true})
-        # )
-        # instead we fake a callback in JsonRpc server that forwards the response as needed
-        JsonRpc.handle_call(
-          {:packet, response(1, %{"applied" => true})},
-          nil,
-          :sys.get_state(JsonRpc)
-        )
+        JsonRpc.receive_packet(response(id, %{"applied" => true}))
 
         assert_receive(%{"id" => 4, "result" => nil}, 5000)
       end)

diff --git a/apps/language_server/test/providers/execute_command/mix_clean_test.exs b/apps/language_server/test/providers/execute_command/mix_clean_test.exs
@@ -1,27 +1,20 @@
 defmodule ElixirLS.LanguageServer.Providers.ExecuteCommand.MixCleanTest do
   alias ElixirLS.LanguageServer.{Server, Protocol, SourceFile, Tracer}
   use ElixirLS.Utils.MixTest.Case, async: false
+  import ElixirLS.LanguageServer.Test.ServerTestHelpers
   use Protocol
 
   setup do
     {:ok, _} = start_supervised(Tracer)
-    server = ElixirLS.LanguageServer.Test.ServerTestHelpers.start_server()
+    server = start_server()
 
     {:ok, %{server: server}}
   end
 
   @tag fixture: true
   test "mix clean", %{server: server} do
     in_fixture(Path.join(__DIR__, "../.."), "clean", fn ->
-      root_uri = SourceFile.Path.to_uri(File.cwd!())
-      Server.receive_packet(server, initialize_req(1, root_uri, %{}))
-
-      Server.receive_packet(
-        server,
-        did_change_configuration(%{
-          "elixirLS" => %{"dialyzerEnabled" => false}
-        })
-      )
+      initialize(server)
 
       assert_receive %{
                        "method" => "window/logMessage",