Fix many cases of invalid Mix.Compiler.Diagnostic position handling

do not return negative numbers as those are forbidden by the LSP properly treat 0 as unknown position add missing handling for positions with column add missing handling for positions as ranges if file the diagnostics refer to is not open fall back to loading it from the file system do not return lines and columns if we cannot load the file Fixes elixir-lsp#695
timgent · May 25, 2022 · ff130f3 · ff130f3
1 parent ae40661
commit ff130f3
Show file tree

Hide file tree

Showing 3 changed files with 103 additions and 36 deletions.
diff --git a/apps/language_server/lib/language_server/build.ex b/apps/language_server/lib/language_server/build.ex
@@ -108,7 +108,8 @@ defmodule ElixirLS.LanguageServer.Build do
     %Mix.Task.Compiler.Diagnostic{
       compiler_name: "ElixirLS",
       file: Path.absname(System.get_env("MIX_EXS") || "mix.exs"),
-      position: nil,
+      # 0 means unknown
+      position: 0,
       message: msg,
       severity: :error,
       details: error
@@ -278,17 +279,15 @@ defmodule ElixirLS.LanguageServer.Build do
     :ok
   end
 
-  defp range(position, nil) when is_integer(position) do
-    line = position - 1
-
-    # we don't care about utf16 positions here as we send 0
-    %{
-      "start" => %{"line" => line, "character" => 0},
-      "end" => %{"line" => line, "character" => 0}
-    }
-  end
+  # for details see
+  # https://hexdocs.pm/mix/1.13.4/Mix.Task.Compiler.Diagnostic.html#t:position/0
+  # https://microsoft.github.io/language-server-protocol/specifications/specification-3-16/#diagnostic
 
-  defp range(position, source_file) when is_integer(position) do
+  # position is a 1 based line number
+  # we return a range of trimmed text in that line
+  defp range(position, source_file)
+       when is_integer(position) and position >= 1 and is_binary(source_file) do
+    # line is 1 based
     line = position - 1
     text = Enum.at(SourceFile.lines(source_file), line) || ""
 
@@ -307,12 +306,64 @@ defmodule ElixirLS.LanguageServer.Build do
     }
   end
 
-  defp range(_, nil) do
-    # we don't care about utf16 positions here as we send 0
-    %{"start" => %{"line" => 0, "character" => 0}, "end" => %{"line" => 0, "character" => 0}}
+  # position is a 1 based line number and 0 based character cursor (UTF8)
+  # we return a 0 length range exactly at that location
+  defp range({line_start, char_start}, source_file)
+       when line_start >= 1 and is_binary(source_file) do
+    lines = SourceFile.lines(source_file)
+    # line is 1 based
+    start_line = Enum.at(lines, line_start - 1)
+    # SourceFile.elixir_character_to_lsp assumes char to be 1 based but it's 0 based bere
+    character = SourceFile.elixir_character_to_lsp(start_line, char_start + 1)
+
+    %{
+      "start" => %{
+        "line" => line_start - 1,
+        "character" => character
+      },
+      "end" => %{
+        "line" => line_start - 1,
+        "character" => character
+      }
+    }
   end
 
-  defp range(_, source_file) do
+  # position is a range defined by 1 based line numbers and 0 based character cursors (UTF8)
+  # we return exactly that range
+  defp range({line_start, char_start, line_end, char_end}, source_file)
+       when line_start >= 1 and line_end >= 1 and is_binary(source_file) do
+    lines = SourceFile.lines(source_file)
+    # line is 1 based
+    start_line = Enum.at(lines, line_start - 1)
+    end_line = Enum.at(lines, line_end - 1)
+
+    # SourceFile.elixir_character_to_lsp assumes char to be 1 based but it's 0 based bere
+    start_char = SourceFile.elixir_character_to_lsp(start_line, char_start + 1)
+    end_char = SourceFile.elixir_character_to_lsp(end_line, char_end + 1)
+
+    %{
+      "start" => %{
+        "line" => line_start - 1,
+        "character" => start_char
+      },
+      "end" => %{
+        "line" => line_end - 1,
+        "character" => end_char
+      }
+    }
+  end
+
+  # position is 0 which means unknown
+  # we return the full file range
+  defp range(0, source_file) when is_binary(source_file) do
     SourceFile.full_range(source_file)
   end
+
+  # source file is unknown
+  # we discard any position information as it is meaningless
+  # unfortunately LSP does not allow `null` range so we need to return something
+  defp range(_, nil) do
+    # we don't care about utf16 positions here as we send 0
+    %{"start" => %{"line" => 0, "character" => 0}, "end" => %{"line" => 0, "character" => 0}}
+  end
 end
diff --git a/apps/language_server/lib/language_server/server.ex b/apps/language_server/lib/language_server/server.ex
@@ -1043,13 +1043,29 @@ defmodule ElixirLS.LanguageServer.Server do
     Dialyzer.check_support() == :ok and build_enabled?(state) and state.dialyzer_sup != nil
   end
 
+  defp safely_read_file(file) do
+    case File.read(file) do
+      {:ok, text} ->
+        text
+
+      {:error, reason} ->
+        IO.warn("Couldn't read file #{file}: #{inspect(reason)}")
+        nil
+    end
+  end
+
   defp publish_diagnostics(new_diagnostics, old_diagnostics, source_files) do
     files =
       Enum.uniq(Enum.map(new_diagnostics, & &1.file) ++ Enum.map(old_diagnostics, & &1.file))
 
     for file <- files,
         uri = SourceFile.path_to_uri(file),
-        do: Build.publish_file_diagnostics(uri, new_diagnostics, Map.get(source_files, uri))
+        do:
+          Build.publish_file_diagnostics(
+            uri,
+            new_diagnostics,
+            Map.get_lazy(source_files, uri, fn -> safely_read_file(file) end)
+          )
   end
 
   defp show_version_warnings do

diff --git a/apps/language_server/test/dialyzer_test.exs b/apps/language_server/test/dialyzer_test.exs
@@ -41,17 +41,17 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
                  %{
                    "message" => error_message1,
                    "range" => %{
-                     "end" => %{"character" => 0, "line" => _},
-                     "start" => %{"character" => 0, "line" => _}
+                     "end" => %{"character" => 12, "line" => 1},
+                     "start" => %{"character" => 2, "line" => 1}
                    },
                    "severity" => 2,
                    "source" => "ElixirLS Dialyzer"
                  },
                  %{
                    "message" => error_message2,
                    "range" => %{
-                     "end" => %{"character" => 0, "line" => _},
-                     "start" => %{"character" => 0, "line" => _}
+                     "end" => %{"character" => 17, "line" => 2},
+                     "start" => %{"character" => 4, "line" => 2}
                    },
                    "severity" => 2,
                    "source" => "ElixirLS Dialyzer"
@@ -175,17 +175,17 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
                  %{
                    "message" => error_message1,
                    "range" => %{
-                     "end" => %{"character" => 0, "line" => _},
-                     "start" => %{"character" => 0, "line" => _}
+                     "end" => %{"character" => 12, "line" => 1},
+                     "start" => %{"character" => 2, "line" => 1}
                    },
                    "severity" => 2,
                    "source" => "ElixirLS Dialyzer"
                  },
                  %{
                    "message" => error_message2,
                    "range" => %{
-                     "end" => %{"character" => 0, "line" => _},
-                     "start" => %{"character" => 0, "line" => _}
+                     "end" => %{"character" => 17, "line" => 2},
+                     "start" => %{"character" => 4, "line" => 2}
                    },
                    "severity" => 2,
                    "source" => "ElixirLS Dialyzer"
@@ -229,17 +229,17 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
                  %{
                    "message" => error_message1,
                    "range" => %{
-                     "end" => %{"character" => 0, "line" => _},
-                     "start" => %{"character" => 0, "line" => _}
+                     "end" => %{"character" => 12, "line" => 1},
+                     "start" => %{"character" => 2, "line" => 1}
                    },
                    "severity" => 2,
                    "source" => "ElixirLS Dialyzer"
                  },
                  %{
                    "message" => error_message2,
                    "range" => %{
-                     "end" => %{"character" => 0, "line" => _},
-                     "start" => %{"character" => 0, "line" => _}
+                     "end" => %{"character" => 17, "line" => 2},
+                     "start" => %{"character" => 4, "line" => 2}
                    },
                    "severity" => 2,
                    "source" => "ElixirLS Dialyzer"
@@ -274,17 +274,17 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
                  %{
                    "message" => error_message1,
                    "range" => %{
-                     "end" => %{"character" => 0, "line" => _},
-                     "start" => %{"character" => 0, "line" => _}
+                     "end" => %{"character" => 12, "line" => 1},
+                     "start" => %{"character" => 2, "line" => 1}
                    },
                    "severity" => 2,
                    "source" => "ElixirLS Dialyzer"
                  },
                  %{
                    "message" => _error_message2,
                    "range" => %{
-                     "end" => %{"character" => 0, "line" => _},
-                     "start" => %{"character" => 0, "line" => _}
+                     "end" => %{"character" => 17, "line" => 2},
+                     "start" => %{"character" => 4, "line" => 2}
                    },
                    "severity" => 2,
                    "source" => "ElixirLS Dialyzer"
@@ -320,17 +320,17 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
                  %{
                    "message" => error_message1,
                    "range" => %{
-                     "end" => %{"character" => 0, "line" => _},
-                     "start" => %{"character" => 0, "line" => _}
+                     "end" => %{"character" => 22, "line" => 1},
+                     "start" => %{"character" => 2, "line" => 1}
                    },
                    "severity" => 2,
                    "source" => "ElixirLS Dialyzer"
                  },
                  %{
                    "message" => error_message2,
                    "range" => %{
-                     "end" => %{"character" => 0, "line" => _},
-                     "start" => %{"character" => 0, "line" => _}
+                     "end" => %{"character" => 22, "line" => 2},
+                     "start" => %{"character" => 4, "line" => 2}
                    },
                    "severity" => 2,
                    "source" => "ElixirLS Dialyzer"