Skip to content

Commit ff130f3

Browse files
lukaszsamsonlukaszsamson
committed
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 #695
1 parent ae40661 commit ff130f3

File tree

3 files changed

+103
-36
lines changed

3 files changed

+103
-36
lines changed

‎apps/language_server/lib/language_server/build.ex

Lines changed: 66 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,8 @@ defmodule ElixirLS.LanguageServer.Build do
108108
%Mix.Task.Compiler.Diagnostic{
109109
compiler_name: "ElixirLS",
110110
file: Path.absname(System.get_env("MIX_EXS") || "mix.exs"),
111-
position: nil,
111+
# 0 means unknown
112+
position: 0,
112113
message: msg,
113114
severity: :error,
114115
details: error
@@ -278,17 +279,15 @@ defmodule ElixirLS.LanguageServer.Build do
278279
:ok
279280
end
280281

281-
defp range(position, nil) when is_integer(position) do
282-
line = position - 1
283-
284-
# we don't care about utf16 positions here as we send 0
285-
%{
286-
"start" => %{"line" => line, "character" => 0},
287-
"end" => %{"line" => line, "character" => 0}
288-
}
289-
end
282+
# for details see
283+
# https://hexdocs.pm/mix/1.13.4/Mix.Task.Compiler.Diagnostic.html#t:position/0
284+
# https://microsoft.github.io/language-server-protocol/specifications/specification-3-16/#diagnostic
290285

291-
defp range(position, source_file) when is_integer(position) do
286+
# position is a 1 based line number
287+
# we return a range of trimmed text in that line
288+
defp range(position, source_file)
289+
when is_integer(position) and position >= 1 and is_binary(source_file) do
290+
# line is 1 based
292291
line = position - 1
293292
text = Enum.at(SourceFile.lines(source_file), line) || ""
294293

@@ -307,12 +306,64 @@ defmodule ElixirLS.LanguageServer.Build do
307306
}
308307
end
309308

310-
defp range(_, nil) do
311-
# we don't care about utf16 positions here as we send 0
312-
%{"start" => %{"line" => 0, "character" => 0}, "end" => %{"line" => 0, "character" => 0}}
309+
# position is a 1 based line number and 0 based character cursor (UTF8)
310+
# we return a 0 length range exactly at that location
311+
defp range({line_start, char_start}, source_file)
312+
when line_start >= 1 and is_binary(source_file) do
313+
lines = SourceFile.lines(source_file)
314+
# line is 1 based
315+
start_line = Enum.at(lines, line_start - 1)
316+
# SourceFile.elixir_character_to_lsp assumes char to be 1 based but it's 0 based bere
317+
character = SourceFile.elixir_character_to_lsp(start_line, char_start + 1)
318+
319+
%{
320+
"start" => %{
321+
"line" => line_start - 1,
322+
"character" => character
323+
},
324+
"end" => %{
325+
"line" => line_start - 1,
326+
"character" => character
327+
}
328+
}
313329
end
314330

315-
defp range(_, source_file) do
331+
# position is a range defined by 1 based line numbers and 0 based character cursors (UTF8)
332+
# we return exactly that range
333+
defp range({line_start, char_start, line_end, char_end}, source_file)
334+
when line_start >= 1 and line_end >= 1 and is_binary(source_file) do
335+
lines = SourceFile.lines(source_file)
336+
# line is 1 based
337+
start_line = Enum.at(lines, line_start - 1)
338+
end_line = Enum.at(lines, line_end - 1)
339+
340+
# SourceFile.elixir_character_to_lsp assumes char to be 1 based but it's 0 based bere
341+
start_char = SourceFile.elixir_character_to_lsp(start_line, char_start + 1)
342+
end_char = SourceFile.elixir_character_to_lsp(end_line, char_end + 1)
343+
344+
%{
345+
"start" => %{
346+
"line" => line_start - 1,
347+
"character" => start_char
348+
},
349+
"end" => %{
350+
"line" => line_end - 1,
351+
"character" => end_char
352+
}
353+
}
354+
end
355+
356+
# position is 0 which means unknown
357+
# we return the full file range
358+
defp range(0, source_file) when is_binary(source_file) do
316359
SourceFile.full_range(source_file)
317360
end
361+
362+
# source file is unknown
363+
# we discard any position information as it is meaningless
364+
# unfortunately LSP does not allow `null` range so we need to return something
365+
defp range(_, nil) do
366+
# we don't care about utf16 positions here as we send 0
367+
%{"start" => %{"line" => 0, "character" => 0}, "end" => %{"line" => 0, "character" => 0}}
368+
end
318369
end

‎apps/language_server/lib/language_server/server.ex

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1043,13 +1043,29 @@ defmodule ElixirLS.LanguageServer.Server do
10431043
Dialyzer.check_support() == :ok and build_enabled?(state) and state.dialyzer_sup != nil
10441044
end
10451045

1046+
defp safely_read_file(file) do
1047+
case File.read(file) do
1048+
{:ok, text} ->
1049+
text
1050+
1051+
{:error, reason} ->
1052+
IO.warn("Couldn't read file #{file}: #{inspect(reason)}")
1053+
nil
1054+
end
1055+
end
1056+
10461057
defp publish_diagnostics(new_diagnostics, old_diagnostics, source_files) do
10471058
files =
10481059
Enum.uniq(Enum.map(new_diagnostics, & &1.file) ++ Enum.map(old_diagnostics, & &1.file))
10491060

10501061
for file <- files,
10511062
uri = SourceFile.path_to_uri(file),
1052-
do: Build.publish_file_diagnostics(uri, new_diagnostics, Map.get(source_files, uri))
1063+
do:
1064+
Build.publish_file_diagnostics(
1065+
uri,
1066+
new_diagnostics,
1067+
Map.get_lazy(source_files, uri, fn -> safely_read_file(file) end)
1068+
)
10531069
end
10541070

10551071
defp show_version_warnings do

‎apps/language_server/test/dialyzer_test.exs

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -41,17 +41,17 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
4141
%{
4242
"message" => error_message1,
4343
"range" => %{
44-
"end" => %{"character" => 0, "line" => _},
45-
"start" => %{"character" => 0, "line" => _}
44+
"end" => %{"character" => 12, "line" => 1},
45+
"start" => %{"character" => 2, "line" => 1}
4646
},
4747
"severity" => 2,
4848
"source" => "ElixirLS Dialyzer"
4949
},
5050
%{
5151
"message" => error_message2,
5252
"range" => %{
53-
"end" => %{"character" => 0, "line" => _},
54-
"start" => %{"character" => 0, "line" => _}
53+
"end" => %{"character" => 17, "line" => 2},
54+
"start" => %{"character" => 4, "line" => 2}
5555
},
5656
"severity" => 2,
5757
"source" => "ElixirLS Dialyzer"
@@ -175,17 +175,17 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
175175
%{
176176
"message" => error_message1,
177177
"range" => %{
178-
"end" => %{"character" => 0, "line" => _},
179-
"start" => %{"character" => 0, "line" => _}
178+
"end" => %{"character" => 12, "line" => 1},
179+
"start" => %{"character" => 2, "line" => 1}
180180
},
181181
"severity" => 2,
182182
"source" => "ElixirLS Dialyzer"
183183
},
184184
%{
185185
"message" => error_message2,
186186
"range" => %{
187-
"end" => %{"character" => 0, "line" => _},
188-
"start" => %{"character" => 0, "line" => _}
187+
"end" => %{"character" => 17, "line" => 2},
188+
"start" => %{"character" => 4, "line" => 2}
189189
},
190190
"severity" => 2,
191191
"source" => "ElixirLS Dialyzer"
@@ -229,17 +229,17 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
229229
%{
230230
"message" => error_message1,
231231
"range" => %{
232-
"end" => %{"character" => 0, "line" => _},
233-
"start" => %{"character" => 0, "line" => _}
232+
"end" => %{"character" => 12, "line" => 1},
233+
"start" => %{"character" => 2, "line" => 1}
234234
},
235235
"severity" => 2,
236236
"source" => "ElixirLS Dialyzer"
237237
},
238238
%{
239239
"message" => error_message2,
240240
"range" => %{
241-
"end" => %{"character" => 0, "line" => _},
242-
"start" => %{"character" => 0, "line" => _}
241+
"end" => %{"character" => 17, "line" => 2},
242+
"start" => %{"character" => 4, "line" => 2}
243243
},
244244
"severity" => 2,
245245
"source" => "ElixirLS Dialyzer"
@@ -274,17 +274,17 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
274274
%{
275275
"message" => error_message1,
276276
"range" => %{
277-
"end" => %{"character" => 0, "line" => _},
278-
"start" => %{"character" => 0, "line" => _}
277+
"end" => %{"character" => 12, "line" => 1},
278+
"start" => %{"character" => 2, "line" => 1}
279279
},
280280
"severity" => 2,
281281
"source" => "ElixirLS Dialyzer"
282282
},
283283
%{
284284
"message" => _error_message2,
285285
"range" => %{
286-
"end" => %{"character" => 0, "line" => _},
287-
"start" => %{"character" => 0, "line" => _}
286+
"end" => %{"character" => 17, "line" => 2},
287+
"start" => %{"character" => 4, "line" => 2}
288288
},
289289
"severity" => 2,
290290
"source" => "ElixirLS Dialyzer"
@@ -320,17 +320,17 @@ defmodule ElixirLS.LanguageServer.DialyzerTest do
320320
%{
321321
"message" => error_message1,
322322
"range" => %{
323-
"end" => %{"character" => 0, "line" => _},
324-
"start" => %{"character" => 0, "line" => _}
323+
"end" => %{"character" => 22, "line" => 1},
324+
"start" => %{"character" => 2, "line" => 1}
325325
},
326326
"severity" => 2,
327327
"source" => "ElixirLS Dialyzer"
328328
},
329329
%{
330330
"message" => error_message2,
331331
"range" => %{
332-
"end" => %{"character" => 0, "line" => _},
333-
"start" => %{"character" => 0, "line" => _}
332+
"end" => %{"character" => 22, "line" => 2},
333+
"start" => %{"character" => 4, "line" => 2}
334334
},
335335
"severity" => 2,
336336
"source" => "ElixirLS Dialyzer"

0 commit comments

Comments
 (0)