Build cleanup (#698)

* check if loading apps is still needed

* Revert "check if loading apps is still needed"

This reverts commit d03cfe5.

* rename function to match what it actually do

* extract diagnostic related code

* extract common mix.exs code

* fix tests

Author: lukaszsamson

apps/elixir_ls_debugger/lib/debugger/server.ex

@@ -25,6 +25,7 @@ defmodule ElixirLS.Debugger.Server do
   }
 
   alias ElixirLS.Debugger.Stacktrace.Frame
+  alias ElixirLS.Utils.MixfileHelpers
   use GenServer
   use Protocol
 
@@ -955,11 +956,11 @@ defmodule ElixirLS.Debugger.Server do
     File.cd!(project_dir)
 
     # Mixfile may already be loaded depending on cwd when launching debugger task
-    mixfile = Path.absname(System.get_env("MIX_EXS") || "mix.exs")
+    mixfile = Path.absname(MixfileHelpers.mix_exs())
 
     # FIXME: Private API
     unless match?(%{file: ^mixfile}, Mix.ProjectStack.peek()) do
-      Code.compile_file(System.get_env("MIX_EXS") || "mix.exs")
+      Code.compile_file(MixfileHelpers.mix_exs())
     end
 
     task = task || Mix.Project.config()[:default_task]

apps/elixir_ls_utils/lib/mixfile_helpers.ex

@@ -0,0 +1,5 @@
+defmodule ElixirLS.Utils.MixfileHelpers do
+  def mix_exs do
+    System.get_env("MIX_EXS") || "mix.exs"
+  end
+end

apps/elixir_ls_utils/test/support/mix_test.case.ex

@@ -1,6 +1,7 @@
 defmodule ElixirLS.Utils.MixTest.Case do
   # This module is based heavily on MixTest.Case in Elixir's tests
   use ExUnit.CaseTemplate
+  alias ElixirLS.Utils.MixfileHelpers
 
   using do
     quote do
@@ -103,7 +104,7 @@ defmodule ElixirLS.Utils.MixTest.Case do
     clear_mix_cache()
 
     # Attempt to purge mixfiles for dependencies to avoid module redefinition warnings
-    mix_exs = System.get_env("MIX_EXS") || "mix.exs"
+    mix_exs = MixfileHelpers.mix_exs()
 
     for {mod, :in_memory} <- :code.all_loaded(),
         source = mod.module_info[:compile][:source],

apps/language_server/lib/language_server/build.ex

@@ -1,5 +1,6 @@
 defmodule ElixirLS.LanguageServer.Build do
-  alias ElixirLS.LanguageServer.{Server, JsonRpc, SourceFile, Diagnostics}
+  alias ElixirLS.LanguageServer.{Server, JsonRpc, Diagnostics}
+  alias ElixirLS.Utils.MixfileHelpers
 
   def build(parent, root_path, opts) when is_binary(root_path) do
     if Path.absname(File.cwd!()) != Path.absname(root_path) do
@@ -28,8 +29,8 @@ defmodule ElixirLS.LanguageServer.Build do
                   purge_consolidated_protocols()
                   {status, diagnostics} = compile()
 
-                  if status in [:ok, :noop] and Keyword.get(opts, :load_all_modules?) do
-                    load_all_modules()
+                  if status in [:ok, :noop] and Keyword.get(opts, :load_all_mix_applications?) do
+                    load_all_mix_applications()
                   end
 
                   diagnostics = Diagnostics.normalize(diagnostics, root_path)
@@ -49,79 +50,12 @@ defmodule ElixirLS.LanguageServer.Build do
     end
   end
 
-  def publish_file_diagnostics(uri, all_diagnostics, source_file) do
-    diagnostics =
-      all_diagnostics
-      |> Enum.filter(&(SourceFile.path_to_uri(&1.file) == uri))
-      |> Enum.sort_by(fn %{position: position} -> position end)
-
-    diagnostics_json =
-      for diagnostic <- diagnostics do
-        severity =
-          case diagnostic.severity do
-            :error -> 1
-            :warning -> 2
-            :information -> 3
-            :hint -> 4
-          end
-
-        message =
-          case diagnostic.message do
-            m when is_binary(m) -> m
-            m when is_list(m) -> m |> Enum.join("\n")
-          end
-
-        %{
-          "message" => message,
-          "severity" => severity,
-          "range" => range(diagnostic.position, source_file),
-          "source" => diagnostic.compiler_name
-        }
-      end
-
-    JsonRpc.notify("textDocument/publishDiagnostics", %{
-      "uri" => uri,
-      "diagnostics" => diagnostics_json
-    })
-  end
-
-  def mixfile_diagnostic({file, line, message}, severity) do
-    %Mix.Task.Compiler.Diagnostic{
-      compiler_name: "ElixirLS",
-      file: file,
-      position: line,
-      message: message,
-      severity: severity
-    }
-  end
-
-  def exception_to_diagnostic(error) do
-    msg =
-      case error do
-        {:shutdown, 1} ->
-          "Build failed for unknown reason. See output log."
-
-        _ ->
-          Exception.format_exit(error)
-      end
-
-    %Mix.Task.Compiler.Diagnostic{
-      compiler_name: "ElixirLS",
-      file: Path.absname(System.get_env("MIX_EXS") || "mix.exs"),
-      # 0 means unknown
-      position: 0,
-      message: msg,
-      severity: :error,
-      details: error
-    }
-  end
-
   def with_build_lock(func) do
     :global.trans({__MODULE__, self()}, func)
   end
 
   defp reload_project do
-    mixfile = Path.absname(System.get_env("MIX_EXS") || "mix.exs")
+    mixfile = Path.absname(MixfileHelpers.mix_exs)
 
     if File.exists?(mixfile) do
       # FIXME: Private API
@@ -145,13 +79,13 @@ defmodule ElixirLS.LanguageServer.Build do
       {status, diagnostics} =
         case Kernel.ParallelCompiler.compile([mixfile]) do
           {:ok, _, warnings} ->
-            {:ok, Enum.map(warnings, &mixfile_diagnostic(&1, :warning))}
+            {:ok, Enum.map(warnings, &Diagnostics.mixfile_diagnostic(&1, :warning))}
 
           {:error, errors, warnings} ->
             {
               :error,
-              Enum.map(warnings, &mixfile_diagnostic(&1, :warning)) ++
-                Enum.map(errors, &mixfile_diagnostic(&1, :error))
+              Enum.map(warnings, &Diagnostics.mixfile_diagnostic(&1, :warning)) ++
+                Enum.map(errors, &Diagnostics.mixfile_diagnostic(&1, :error))
             }
         end
 
@@ -174,7 +108,11 @@ defmodule ElixirLS.LanguageServer.Build do
     end
   end
 
-  def load_all_modules do
+  # TODO It looks like that function is no longer needed on elixir >= 1.11
+  # it was added in https://github.com/elixir-lsp/elixir-ls/pull/227
+  # removing it doesn't break tests and I'm not able to reproduce
+  # https://github.com/elixir-lsp/elixir-ls/issues/209 on recent elixir (1.13)
+  def load_all_mix_applications do
     apps =
       cond do
         Mix.Project.umbrella?() ->
@@ -278,92 +216,4 @@ defmodule ElixirLS.LanguageServer.Build do
 
     :ok
   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
-
-  # 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 not is_nil(source_file) do
-    # line is 1 based
-    line = position - 1
-    text = Enum.at(SourceFile.lines(source_file), line) || ""
-
-    start_idx = String.length(text) - String.length(String.trim_leading(text)) + 1
-    length = max(String.length(String.trim(text)), 1)
-
-    %{
-      "start" => %{
-        "line" => line,
-        "character" => SourceFile.elixir_character_to_lsp(text, start_idx)
-      },
-      "end" => %{
-        "line" => line,
-        "character" => SourceFile.elixir_character_to_lsp(text, start_idx + length)
-      }
-    }
-  end
-
-  # 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 not is_nil(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
-
-  # 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 not is_nil(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 not is_nil(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