Fix prefixing unused variables with underscore

apps/language_server/lib/language_server/providers/code_action.ex

@@ -1,48 +1,61 @@
 defmodule ElixirLS.LanguageServer.Providers.CodeAction do
   use ElixirLS.LanguageServer.Protocol
 
-  def code_actions(uri, diagnostics) do
+  alias ElixirLS.LanguageServer.SourceFile
+
+  @variable_is_unused ~r/variable "(.*)" is unused/
+
+  def code_actions(uri, diagnostics, source_file) do
     actions =
       diagnostics
-      |> Enum.map(fn diagnostic -> actions(uri, diagnostic) end)
+      |> Enum.map(fn diagnostic -> actions(uri, diagnostic, source_file) end)
       |> List.flatten()
 
     {:ok, actions}
   end
 
-  defp actions(uri, %{"message" => message} = diagnostic) do
+  defp actions(uri, %{"message" => message} = diagnostic, source_file) do
     [
-      {~r/variable "(.*)" is unused/, &prefix_with_underscore/2},
-      {~r/variable "(.*)" is unused/, &remove_variable/2}
+      {@variable_is_unused, &prefix_with_underscore/3},
+      {@variable_is_unused, &remove_variable/3}
     ]
     |> Enum.filter(fn {r, _fun} -> String.match?(message, r) end)
-    |> Enum.map(fn {_r, fun} -> fun.(uri, diagnostic) end)
+    |> Enum.map(fn {_r, fun} -> fun.(uri, diagnostic, source_file) end)
   end
 
-  defp prefix_with_underscore(uri, %{"range" => range}) do
-    %{
-      "title" => "Add '_' to unused variable",
-      "kind" => "quickfix",
-      "edit" => %{
-        "changes" => %{
-          uri => [
-            %{
-              "newText" => "_",
-              "range" =>
-                range(
-                  range["start"]["line"],
-                  range["start"]["character"],
-                  range["start"]["line"],
-                  range["start"]["character"]
-                )
-            }
-          ]
+  defp prefix_with_underscore(uri, %{"message" => message, "range" => range}, source_file) do
+    [_, variable] = Regex.run(@variable_is_unused, message)
+
+    start_line = start_line_from_range(range)
+
+    source_line =
+      source_file
+      |> SourceFile.lines()
+      |> Enum.at(start_line)
+
+    pattern = Regex.compile!("(?<![[:alnum:]._])#{Regex.escape(variable)}(?![[:alnum:]._])")
+
+    if pattern |> Regex.scan(source_line) |> length() == 1 do
+      %{
+        "title" => "Add '_' to unused variable",
+        "kind" => "quickfix",
+        "edit" => %{
+          "changes" => %{
+            uri => [
+              %{
+                "newText" => String.replace(source_line, pattern, "_" <> variable),
+                "range" => range(start_line, 0, start_line, String.length(source_line))
+              }
+            ]
+          }
         }
       }
-    }
+    else
+      []
+    end
   end
 
-  defp remove_variable(uri, %{"range" => range}) do
+  defp remove_variable(uri, %{"range" => range}, _source_file) do
     %{
       "title" => "Remove unused variable",
       "kind" => "quickfix",
@@ -58,4 +71,6 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction do
       }
     }
   end
+
+  defp start_line_from_range(%{"start" => %{"line" => start_line}}), do: start_line
 end

apps/language_server/lib/language_server/server.ex

@@ -781,7 +781,9 @@ defmodule ElixirLS.LanguageServer.Server do
   end
 
   defp handle_request(code_action_req(_id, uri, diagnostics), state = %__MODULE__{}) do
-    {:async, fn -> CodeAction.code_actions(uri, diagnostics) end, state}
+    source_file = get_source_file(state, uri)
+
+    {:async, fn -> CodeAction.code_actions(uri, diagnostics, source_file) end, state}
   end
 
   defp handle_request(%{"method" => "$/" <> _}, state = %__MODULE__{}) do

apps/language_server/test/providers/code_action_test.exs

@@ -0,0 +1,230 @@
+defmodule ElixirLS.LanguageServer.Providers.CodeActionTest do
+  use ExUnit.Case
+
+  alias ElixirLS.LanguageServer.Providers.CodeAction
+  alias ElixirLS.LanguageServer.SourceFile
+
+  @uri "file:///some_file.ex"
+
+  describe "prefix with underscore" do
+    test "variable in the function header" do
+      text = """
+      defmodule Example do
+        def foo(var) do
+          42
+        end
+      end
+      """
+
+      source_file = %SourceFile{text: text}
+
+      diagnostic_range = create_range(1, 2, 1, 17)
+
+      diagnostic =
+        "var"
+        |> create_message()
+        |> create_diagnostic(diagnostic_range)
+
+      assert {:ok, [underscore_action, _delete_action]} =
+               CodeAction.code_actions(@uri, diagnostic, source_file)
+
+      new_text = "  def foo(_var) do"
+      quickfix_range = create_range(1, 0, 1, 17)
+
+      assert_quickfix(new_text, quickfix_range, underscore_action)
+    end
+
+    test "assignment" do
+      text = """
+      defmodule Example do
+        def foo do
+          var = 42
+        end
+      end
+      """
+
+      source_file = %SourceFile{text: text}
+
+      diagnostic_range = create_range(2, 4, 2, 12)
+
+      diagnostic =
+        "var"
+        |> create_message()
+        |> create_diagnostic(diagnostic_range)
+
+      assert {:ok, [underscore_action, _delete_action]} =
+               CodeAction.code_actions(@uri, diagnostic, source_file)
+
+      new_text = "    _var = 42"
+      quickfix_range = create_range(2, 0, 2, 12)
+
+      assert_quickfix(new_text, quickfix_range, underscore_action)
+    end
+
+    test "pattern matching on map" do
+      text = """
+      defmodule Example do
+        def foo do
+          %{a: var} = %{a: 42}
+        end
+      end
+      """
+
+      source_file = %SourceFile{text: text}
+
+      diagnostic_range = create_range(2, 4, 2, 24)
+
+      diagnostic =
+        "var"
+        |> create_message()
+        |> create_diagnostic(diagnostic_range)
+
+      assert {:ok, [underscore_action, _delete_action]} =
+               CodeAction.code_actions(@uri, diagnostic, source_file)
+
+      new_text = "    %{a: _var} = %{a: 42}"
+      quickfix_range = create_range(2, 0, 2, 24)
+
+      assert_quickfix(new_text, quickfix_range, underscore_action)
+    end
+
+    test "pattern matching on list" do
+      text = """
+      defmodule Example do
+        def foo do
+          [var] = [42]
+        end
+      end
+      """
+
+      source_file = %SourceFile{text: text}
+
+      diagnostic_range = create_range(2, 4, 2, 16)
+
+      diagnostic =
+        "var"
+        |> create_message()
+        |> create_diagnostic(diagnostic_range)
+
+      assert {:ok, [underscore_action, _delete_action]} =
+               CodeAction.code_actions(@uri, diagnostic, source_file)
+
+      new_text = "    [_var] = [42]"
+      quickfix_range = create_range(2, 0, 2, 16)
+
+      assert_quickfix(new_text, quickfix_range, underscore_action)
+    end
+
+    test "argument of an anonymous function" do
+      text = """
+      defmodule Example do
+        def foo do
+          Enum.map([42], fn var -> 42 end)
+        end
+      end
+      """
+
+      source_file = %SourceFile{text: text}
+
+      diagnostic_range = create_range(2, 4, 2, 36)
+
+      diagnostic =
+        "var"
+        |> create_message()
+        |> create_diagnostic(diagnostic_range)
+
+      assert {:ok, [underscore_action, _delete_action]} =
+               CodeAction.code_actions(@uri, diagnostic, source_file)
+
+      new_text = "    Enum.map([42], fn _var -> 42 end)"
+      quickfix_range = create_range(2, 0, 2, 36)
+
+      assert_quickfix(new_text, quickfix_range, underscore_action)
+    end
+
+    test "no quickfix if there are two variables of the same name" do
+      text = """
+      defmodule Example do
+        def foo do
+          var = Enum.map([42], fn var -> 42 end)
+        end
+      end
+      """
+
+      source_file = %SourceFile{text: text}
+
+      diagnostic_range = create_range(2, 4, 2, 42)
+
+      diagnostic =
+        "var"
+        |> create_message()
+        |> create_diagnostic(diagnostic_range)
+
+      assert {:ok, [_delete_action]} = CodeAction.code_actions(@uri, diagnostic, source_file)
+    end
+
+    test "functions are not matched for quickfix" do
+      text = """
+      defmodule Example do
+        def foo do
+          count = Enum.count([40, 2])
+        end
+      end
+      """
+
+      source_file = %SourceFile{text: text}
+
+      diagnostic_range = create_range(2, 4, 2, 31)
+
+      diagnostic =
+        "count"
+        |> create_message()
+        |> create_diagnostic(diagnostic_range)
+
+      assert {:ok, [underscore_action, _delete_action]} =
+               CodeAction.code_actions(@uri, diagnostic, source_file)
+
+      new_text = "    _count = Enum.count([40, 2])"
+      quickfix_range = create_range(2, 0, 2, 31)
+
+      assert_quickfix(new_text, quickfix_range, underscore_action)
+    end
+
+    defp create_message(variable) do
+      "variable \"#{variable}\" is unused (if the variable is not meant to be used, prefix it with an underscore)"
+    end
+
+    defp create_range(start_line, start_character, end_line, end_character) do
+      %{
+        "end" => %{"character" => end_character, "line" => end_line},
+        "start" => %{"character" => start_character, "line" => start_line}
+      }
+    end
+
+    defp create_diagnostic(message, range) do
+      [
+        %{
+          "message" => message,
+          "range" => range,
+          "severity" => 2,
+          "source" => "Elixir"
+        }
+      ]
+    end
+
+    defp assert_quickfix(new_text, range, action) do
+      assert %{
+               "edit" => %{
+                 "changes" => %{
+                   @uri => [
+                     %{
+                       "newText" => ^new_text,
+                       "range" => ^range
+                     }
+                   ]
+                 }
+               }
+             } = action
+    end
+  end
+end