Guard against invalid positions in diagnostics

Return line with column from message
Fixes #707

Author: lukaszsamson

apps/language_server/lib/language_server/diagnostics.ex

@@ -4,13 +4,13 @@ defmodule ElixirLS.LanguageServer.Diagnostics do
 
   def normalize(diagnostics, root_path) do
     for diagnostic <- diagnostics do
-      {type, file, line, description, stacktrace} =
+      {type, file, position, description, stacktrace} =
         extract_message_info(diagnostic.message, root_path)
 
       diagnostic
       |> update_message(type, description, stacktrace)
       |> maybe_update_file(file)
-      |> maybe_update_position(type, line, stacktrace)
+      |> maybe_update_position(type, position, stacktrace)
     end
   end
 
@@ -32,9 +32,9 @@ defmodule ElixirLS.LanguageServer.Diagnostics do
     stacktrace = reversed_stacktrace |> Enum.map(&String.trim/1) |> Enum.reverse()
 
     {type, message_without_type} = split_type_and_message(message)
-    {file, line, description} = split_file_and_description(message_without_type, root_path)
+    {file, position, description} = split_file_and_description(message_without_type, root_path)
 
-    {type, file, line, description, stacktrace}
+    {type, file, position, description, stacktrace}
   end
 
   defp update_message(diagnostic, type, description, stacktrace) do
@@ -68,31 +68,31 @@ defmodule ElixirLS.LanguageServer.Diagnostics do
     end
   end
 
-  defp maybe_update_position(diagnostic, "TokenMissingError", line, stacktrace) do
+  defp maybe_update_position(diagnostic, "TokenMissingError", position, stacktrace) do
     case extract_line_from_missing_hint(diagnostic.message) do
-      line when is_integer(line) ->
+      line when is_integer(line) and line > 0 ->
         %{diagnostic | position: line}
 
       _ ->
-        do_maybe_update_position(diagnostic, line, stacktrace)
+        do_maybe_update_position(diagnostic, position, stacktrace)
     end
   end
 
-  defp maybe_update_position(diagnostic, _type, line, stacktrace) do
-    do_maybe_update_position(diagnostic, line, stacktrace)
+  defp maybe_update_position(diagnostic, _type, position, stacktrace) do
+    do_maybe_update_position(diagnostic, position, stacktrace)
   end
 
-  defp do_maybe_update_position(diagnostic, line, stacktrace) do
+  defp do_maybe_update_position(diagnostic, position, stacktrace) do
     cond do
-      line ->
-        %{diagnostic | position: line}
+      position != nil ->
+        %{diagnostic | position: position}
 
       diagnostic.position ->
         diagnostic
 
       true ->
         line = extract_line_from_stacktrace(diagnostic.file, stacktrace)
-        %{diagnostic | position: line}
+        %{diagnostic | position: max(line, 0)}
     end
   end
 
@@ -107,9 +107,18 @@ defmodule ElixirLS.LanguageServer.Diagnostics do
   end
 
   defp split_file_and_description(message, root_path) do
-    with {file, line, _column, description} <- get_message_parts(message),
+    with {file, line, column, description} <- get_message_parts(message),
          {:ok, path} <- file_path(file, root_path) do
-      {path, String.to_integer(line), description}
+      line = String.to_integer(line)
+
+      position =
+        cond do
+          line == 0 -> 0
+          column == "" -> line
+          true -> {line, String.to_integer(column)}
+        end
+
+      {path, position, description}
     else
       _ ->
         {nil, nil, message}
@@ -279,7 +288,7 @@ defmodule ElixirLS.LanguageServer.Diagnostics 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
+    # SourceFile.elixir_character_to_lsp assumes char to be 1 based but it's 0 based here
     character = SourceFile.elixir_character_to_lsp(start_line, char_start + 1)
 
     %{
@@ -303,7 +312,7 @@ defmodule ElixirLS.LanguageServer.Diagnostics do
     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
+    # SourceFile.elixir_character_to_lsp assumes char to be 1 based but it's 0 based here
     start_char = SourceFile.elixir_character_to_lsp(start_line, char_start + 1)
     end_char = SourceFile.elixir_character_to_lsp(end_line, char_end + 1)
 
@@ -319,16 +328,10 @@ defmodule ElixirLS.LanguageServer.Diagnostics do
     }
   end
 
-  # position is 0 which means unknown
-  # we return the full file range
-  defp range(0, source_file) when not is_nil(source_file) do
-    SourceFile.full_range(source_file)
-  end
-
-  # source file is unknown
+  # source file is unknown, position is 0 or invalid
   # 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
+  defp range(_, _) do
     # we don't care about utf16 positions here as we send 0
     %{"start" => %{"line" => 0, "character" => 0}, "end" => %{"line" => 0, "character" => 0}}
   end

apps/language_server/lib/language_server/dialyzer.ex

@@ -475,25 +475,42 @@ defmodule ElixirLS.LanguageServer.Dialyzer do
 
   defp to_diagnostics(warnings_map, warn_opts, warning_format) do
     tags_enabled = Analyzer.matching_tags(warn_opts)
+    deps_path = Mix.Project.deps_path()
 
     for {_beam_file, warnings} <- warnings_map,
-        {source_file, line, data} <- warnings,
+        {source_file, position, data} <- warnings,
         {tag, _, _} = data,
         tag in tags_enabled,
         source_file = Path.absname(to_string(source_file)),
         in_project?(source_file),
-        not String.starts_with?(source_file, Mix.Project.deps_path()) do
+        not String.starts_with?(source_file, deps_path) do
       %Mix.Task.Compiler.Diagnostic{
         compiler_name: "ElixirLS Dialyzer",
         file: source_file,
-        position: line,
+        position: normalize_postion(position),
         message: warning_message(data, warning_format),
         severity: :warning,
         details: data
       }
     end
   end
 
+  # up until OTP 23 position was line :: non_negative_integer
+  # starting from OTP 24 it is erl_anno:location() :: line | {line, column}
+  defp normalize_postion({line, column}) when line > 0 do
+    {line, column}
+  end
+
+  # 0 means unknown line
+  defp normalize_postion(line) when line >= 0 do
+    line
+  end
+
+  defp normalize_postion(position) do
+    IO.warn("dialyzer returned warning with invalid position #{inspect(position)}")
+    0
+  end
+
   defp warning_message({_, _, {warning_name, args}} = raw_warning, warning_format)
        when warning_format in ["dialyxir_long", "dialyxir_short"] do
     format_function =

apps/language_server/lib/language_server/providers/execute_command/apply_spec.ex

@@ -94,4 +94,3 @@ defmodule ElixirLS.LanguageServer.Providers.ExecuteCommand.ApplySpec do
     end
   end
 end
-

apps/language_server/lib/language_server/providers/formatting.ex

@@ -36,7 +36,6 @@ defmodule ElixirLS.LanguageServer.Providers.Formatting do
     do_format(source_file)
   end
 
-
   defp do_format(%SourceFile{} = source_file, opts \\ []), do: do_format(source_file, nil, opts)
 
   defp do_format(%SourceFile{text: text}, formatter, opts) do

apps/language_server/lib/language_server/server.ex

@@ -1278,4 +1278,3 @@ defmodule ElixirLS.LanguageServer.Server do
     end)
   end
 end
-

apps/language_server/lib/language_server/source_file.ex

@@ -431,4 +431,3 @@ defmodule ElixirLS.LanguageServer.SourceFile do
     {elixir_line - 1, utf16_character}
   end
 end
-

apps/language_server/test/diagnostics_test.exs

@@ -61,6 +61,30 @@ defmodule ElixirLS.LanguageServer.DiagnosticsTest do
       assert diagnostic.position == 3
     end
 
+    test "update file and position with column if file is present in the message" do
+      root_path = Path.join(__DIR__, "fixtures/build_errors")
+      file = Path.join(root_path, "lib/has_error.ex")
+      position = 2
+
+      message = """
+      ** (CompileError) lib/has_error.ex:3:5: some message
+          lib/my_app/my_module.ex:10: MyApp.MyModule.render/1
+      """
+
+      [diagnostic | _] =
+        [build_diagnostic(message, file, position)]
+        |> Diagnostics.normalize(root_path)
+
+      assert diagnostic.message == """
+             (CompileError) some message
+
+             Stacktrace:
+               │ lib/my_app/my_module.ex:10: MyApp.MyModule.render/1\
+             """
+
+      assert diagnostic.position == {3, 5}
+    end
+
     test "update file and position if file is present in the message (umbrella)" do
       root_path = Path.join(__DIR__, "fixtures/umbrella")
       file = Path.join(root_path, "lib/file_to_be_replaced.ex")