Fix crash in references provider (#807)

* fixformatting

* add tests for erlang function and module references

* workaround elixir tracer returning invalid paths

elixir-lang/elixir#12393

Author: lukaszsamson

apps/language_server/lib/language_server/providers/references.ex

@@ -12,6 +12,7 @@ defmodule ElixirLS.LanguageServer.Providers.References do
 
   alias ElixirLS.LanguageServer.{SourceFile, Build}
   import ElixirLS.LanguageServer.Protocol
+  require Logger
 
   def references(text, uri, line, character, _include_declaration) do
     {line, character} = SourceFile.lsp_position_to_elixir(text, {line, character})
@@ -24,24 +25,32 @@ defmodule ElixirLS.LanguageServer.Providers.References do
         elixir_sense_reference
         |> build_reference(uri, text)
       end)
+      |> Enum.filter(&(not is_nil(&1)))
     end)
   end
 
   defp build_reference(ref, current_file_uri, current_file_text) do
-    text = get_text(ref, current_file_text)
+    case get_text(ref, current_file_text) do
+      {:ok, text} ->
+        {start_line, start_column} =
+          SourceFile.elixir_position_to_lsp(text, {ref.range.start.line, ref.range.start.column})
 
-    {start_line, start_column} =
-      SourceFile.elixir_position_to_lsp(text, {ref.range.start.line, ref.range.start.column})
+        {end_line, end_column} =
+          SourceFile.elixir_position_to_lsp(text, {ref.range.end.line, ref.range.end.column})
 
-    {end_line, end_column} =
-      SourceFile.elixir_position_to_lsp(text, {ref.range.end.line, ref.range.end.column})
+        range = range(start_line, start_column, end_line, end_column)
 
-    range = range(start_line, start_column, end_line, end_column)
+        %{
+          "range" => range,
+          "uri" => build_uri(ref, current_file_uri)
+        }
 
-    %{
-      "range" => range,
-      "uri" => build_uri(ref, current_file_uri)
-    }
+      {:error, reason} ->
+        # workaround for elixir tracer returning invalid paths
+        # https://github.com/elixir-lang/elixir/issues/12393
+        Logger.warn("Unable to open reference from #{inspect(ref.uri)}: #{inspect(reason)}")
+        nil
+    end
   end
 
   def build_uri(elixir_sense_ref, current_file_uri) do
@@ -57,8 +66,8 @@ defmodule ElixirLS.LanguageServer.Providers.References do
 
   def get_text(elixir_sense_ref, current_file_text) do
     case elixir_sense_ref.uri do
-      nil -> current_file_text
-      path when is_binary(path) -> File.read!(path)
+      nil -> {:ok, current_file_text}
+      path when is_binary(path) -> File.read(path)
     end
   end
 end

apps/language_server/test/experimental/project_test.exs

@@ -62,7 +62,7 @@ defmodule ElixirLS.Experimental.ProjectTest do
       Project.new(root_uri())
       root_path = SourceFile.Path.absolute_from_uri(root_uri())
 
-      assert_called File.cd(^root_path)
+      assert_called(File.cd(^root_path))
     end
 
     test "shouldn't cd to the root uri if it doesn't exist" do
@@ -86,52 +86,52 @@ defmodule ElixirLS.Experimental.ProjectTest do
       assert {:ok, %Project{} = project} = Project.change_mix_env(ctx.project, "dev")
 
       assert project.mix_env == :dev
-      assert_called Mix.env(:dev)
+      assert_called(Mix.env(:dev))
     end
 
     test "defaults to test", ctx do
       assert {:ok, %Project{} = project} = Project.change_mix_env(ctx.project, "")
 
       assert project.mix_env == :test
-      assert_called Mix.env(:test)
+      assert_called(Mix.env(:test))
     end
 
     test "defaults to test with an empty param", ctx do
       assert {:ok, %Project{} = project} = Project.change_mix_env(ctx.project, nil)
       assert project.mix_env == :test
-      assert_called Mix.env(:test)
+      assert_called(Mix.env(:test))
     end
 
     test "with the same mix env has no effect ", ctx do
       project = %{ctx.project | mix_env: :dev}
 
       assert {:ok, %Project{} = project} = Project.change_mix_env(project, "dev")
       assert project.mix_env == :dev
-      refute_called Mix.env(_)
+      refute_called(Mix.env(_))
     end
 
     test "overriding with nil has no effect", ctx do
       project = %{ctx.project | mix_env: :dev}
 
       assert {:ok, %Project{} = project} = Project.change_mix_env(project, nil)
       assert project.mix_env == :dev
-      refute_called Mix.env(_)
+      refute_called(Mix.env(_))
     end
 
     test "overriding with an emppty string has no effect", ctx do
       project = %{ctx.project | mix_env: :dev}
 
       assert {:ok, %Project{} = project} = Project.change_mix_env(project, "")
       assert project.mix_env == :dev
-      refute_called Mix.env(_)
+      refute_called(Mix.env(_))
     end
 
     test "to a new env requires a restar", ctx do
       project = %{ctx.project | mix_env: :prod}
 
       assert {:restart, :warning, message} = Project.change_mix_env(project, "dev")
       assert message =~ "Mix env change detected."
-      refute_called Mix.env(_)
+      refute_called(Mix.env(_))
     end
   end
 
@@ -154,7 +154,7 @@ defmodule ElixirLS.Experimental.ProjectTest do
       }
 
       assert project.env_variables == expected_env_vars
-      assert_called System.put_env(^expected_env_vars)
+      assert_called(System.put_env(^expected_env_vars))
     end
 
     test "keeps existing env vars if they're the same as the old ones", ctx do
@@ -168,15 +168,15 @@ defmodule ElixirLS.Experimental.ProjectTest do
 
       assert {:ok, %Project{} = project} = Project.change_environment_variables(project, vars)
       assert project.env_variables == expected_env_vars
-      refute_called System.put_env(_)
+      refute_called(System.put_env(_))
     end
 
     test "rejects env variables that aren't a compatible format", ctx do
       vars = ["a", "b", "c"]
 
       assert {:ok, %Project{} = project} = Project.change_environment_variables(ctx.project, vars)
       assert project.env_variables == nil
-      refute_called System.put_env(_)
+      refute_called(System.put_env(_))
     end
 
     test "requires a restart if the variables have been set and are being overridden", ctx do
@@ -185,7 +185,7 @@ defmodule ElixirLS.Experimental.ProjectTest do
 
       assert {:restart, :warning, message} = Project.change_environment_variables(project, vars)
       assert message =~ "Environment variables have changed"
-      refute_called System.put_env(_)
+      refute_called(System.put_env(_))
     end
   end
 
@@ -200,35 +200,35 @@ defmodule ElixirLS.Experimental.ProjectTest do
     test "allows you to set the mix target if it was unset", ctx do
       assert {:ok, %Project{} = project} = Project.change_mix_target(ctx.project, "local")
       assert project.mix_target == :local
-      assert_called Mix.target(:local)
+      assert_called(Mix.target(:local))
     end
 
     test "rejects nil for the new target", ctx do
       assert {:ok, %Project{} = project} = Project.change_mix_target(ctx.project, nil)
       assert project.mix_target == nil
-      refute_called Mix.target(:local)
+      refute_called(Mix.target(:local))
     end
 
     test "rejects empty string for the new target", ctx do
       assert {:ok, %Project{} = project} = Project.change_mix_target(ctx.project, "")
       assert project.mix_target == nil
-      refute_called Mix.target(:local)
+      refute_called(Mix.target(:local))
     end
 
     test "does nothing if the mix target is the same as the old target", ctx do
       project = %Project{ctx.project | mix_target: :local}
 
       assert {:ok, %Project{} = project} = Project.change_mix_target(project, "local")
       assert project.mix_target == :local
-      refute_called Mix.target(:local)
+      refute_called(Mix.target(:local))
     end
 
     test "requires a restart if it was changed after being previously set", ctx do
       project = %Project{ctx.project | mix_target: :local}
 
       assert {:restart, :warning, message} = Project.change_mix_target(project, "docs")
       assert message =~ "Mix target change detected."
-      refute_called Mix.target(_)
+      refute_called(Mix.target(_))
     end
   end
 

apps/language_server/test/experimental/server/configuration_test.exs

@@ -67,7 +67,7 @@ defmodule ElixirLS.Experimental.Server.ConfigurationTest do
       Configuration.new(root_uri(), fixture())
       root_path = SourceFile.Path.absolute_from_uri(root_uri())
 
-      assert_called File.cd(^root_path)
+      assert_called(File.cd(^root_path))
     end
 
     test "shouldn't cd to the root uri if it doesn't exist" do
@@ -135,15 +135,15 @@ defmodule ElixirLS.Experimental.Server.ConfigurationTest do
       assert {:ok, %Configuration{} = config} = Configuration.on_change(ctx.config, change)
 
       assert config.project.mix_env == :dev
-      assert_called Mix.env(:dev)
+      assert_called(Mix.env(:dev))
     end
 
     test "defaults to test", ctx do
       change = DidChangeConfiguration.new(settings: %{})
       assert {:ok, %Configuration{} = config} = Configuration.on_change(ctx.config, change)
 
       assert config.project.mix_env == :test
-      assert_called Mix.env(:test)
+      assert_called(Mix.env(:test))
     end
   end
 
@@ -172,7 +172,7 @@ defmodule ElixirLS.Experimental.Server.ConfigurationTest do
       }
 
       assert config.project.env_variables == expected_env_vars
-      assert_called System.put_env(^expected_env_vars)
+      assert_called(System.put_env(^expected_env_vars))
     end
   end
 
@@ -189,7 +189,7 @@ defmodule ElixirLS.Experimental.Server.ConfigurationTest do
 
       assert {:ok, %Configuration{} = config} = Configuration.on_change(ctx.config, change)
       assert config.project.mix_target == :local
-      assert_called Mix.target(:local)
+      assert_called(Mix.target(:local))
     end
   end
 

apps/language_server/test/providers/references_test.exs

@@ -32,6 +32,7 @@ defmodule ElixirLS.LanguageServer.Providers.ReferencesTest do
     Code.compile_file(FixtureHelpers.get_path("references_remote.ex"))
     Code.compile_file(FixtureHelpers.get_path("uses_macro_a.ex"))
     Code.compile_file(FixtureHelpers.get_path("macro_a.ex"))
+    Code.compile_file(FixtureHelpers.get_path("references_erlang.ex"))
     {:ok, context}
   end
 
@@ -176,4 +177,42 @@ defmodule ElixirLS.LanguageServer.Providers.ReferencesTest do
              }
            ]
   end
+
+  test "finds remote references to erlang function" do
+    file_path = FixtureHelpers.get_path("references_referenced.ex")
+    text = File.read!(file_path)
+    uri = SourceFile.Path.to_uri(file_path)
+
+    {line, char} = {31, 10}
+
+    ElixirLS.Test.TextLoc.annotate_assert(file_path, line, char, """
+        :ets.new(:abc, [])
+              ^
+    """)
+
+    list = References.references(text, uri, line, char, true)
+
+    assert length(list) == 2
+    assert Enum.any?(list, &(&1["uri"] |> String.ends_with?("references_erlang.ex")))
+    assert Enum.any?(list, &(&1["uri"] |> String.ends_with?("references_referenced.ex")))
+  end
+
+  test "finds remote references to erlang module" do
+    file_path = FixtureHelpers.get_path("references_referenced.ex")
+    text = File.read!(file_path)
+    uri = SourceFile.Path.to_uri(file_path)
+
+    {line, char} = {31, 6}
+
+    ElixirLS.Test.TextLoc.annotate_assert(file_path, line, char, """
+        :ets.new(:abc, [])
+          ^
+    """)
+
+    list = References.references(text, uri, line, char, true)
+
+    assert length(list) == 2
+    assert Enum.any?(list, &(&1["uri"] |> String.ends_with?("references_erlang.ex")))
+    assert Enum.any?(list, &(&1["uri"] |> String.ends_with?("references_referenced.ex")))
+  end
 end

apps/language_server/test/support/fixtures/references_erlang.ex

@@ -0,0 +1,5 @@
+defmodule ElixirLS.Test.ReferencesErlang do
+  def uses_fun do
+    :ets.new(:my_table, [])
+  end
+end

apps/language_server/test/support/fixtures/references_referenced.ex

@@ -27,4 +27,8 @@ defmodule ElixirLS.Test.ReferencesReferenced do
   def uses_attribute do
     @referenced_attribute
   end
+
+  def uses_erlang_fun() do
+    :ets.new(:abc, [])
+  end
 end