From 9c7b11ed54f58c8406d2a1e14a522fddd77cd6ae Mon Sep 17 00:00:00 2001 From: Steve Cohen Date: Fri, 31 May 2024 16:39:37 -0700 Subject: [PATCH] Code Action: Remove unused aliases (#748) * Code Action: Remove unused aliases This code action removes any unused aliases. It handles both single aliases where it deletes the entire line up to the next line, and multiple aliases, where it deletes the single alias in the list, handling edge cases when it deletes both the last entry in the list, deleting the previous line's comma and when there's only one entry in the list when it deletes the whole alias. The approach here is much more complicated than I wanted, I described the reasons why it's that way in the module's documentation, hopefully, we can get a fix from sourceror soon and improve this. * made Ast.contains_position? inclusive, like range, and switched to it --- apps/common/lib/lexical/ast.ex | 4 +- apps/common/test/lexical/ast_test.exs | 4 +- .../lib/lexical/remote_control/code_action.ex | 3 +- .../handlers/remove_unused_alias.ex | 245 ++++++++++++ .../handlers/remove_unused_alias_test.exs | 360 ++++++++++++++++++ 5 files changed, 611 insertions(+), 5 deletions(-) create mode 100644 apps/remote_control/lib/lexical/remote_control/code_action/handlers/remove_unused_alias.ex create mode 100644 apps/remote_control/test/lexical/remote_control/code_action/handlers/remove_unused_alias_test.exs diff --git a/apps/common/lib/lexical/ast.ex b/apps/common/lib/lexical/ast.ex index 008d45930..a7d506dab 100644 --- a/apps/common/lib/lexical/ast.ex +++ b/apps/common/lib/lexical/ast.ex @@ -321,13 +321,13 @@ defmodule Lexical.Ast do cond do on_same_line? -> - position.character >= start_pos[:column] and position.character < end_pos[:column] + position.character >= start_pos[:column] and position.character <= end_pos[:column] position.line == start_pos[:line] -> position.character >= start_pos[:column] position.line == end_pos[:line] -> - position.character < end_pos[:column] + position.character <= end_pos[:column] true -> position.line > start_pos[:line] and position.line < end_pos[:line] diff --git a/apps/common/test/lexical/ast_test.exs b/apps/common/test/lexical/ast_test.exs index 68999335f..c91e3dee7 100644 --- a/apps/common/test/lexical/ast_test.exs +++ b/apps/common/test/lexical/ast_test.exs @@ -211,7 +211,7 @@ defmodule Lexical.AstTest do setup do {range, code} = pop_range(~q| [ - «single_line_call(1, 2, 3») + «single_line_call(1, 2, 3)» ] |) @@ -247,7 +247,7 @@ defmodule Lexical.AstTest do [ «multi_line_call( 1, 2, 3 - ») + )» ] |) diff --git a/apps/remote_control/lib/lexical/remote_control/code_action.ex b/apps/remote_control/lib/lexical/remote_control/code_action.ex index b92043616..4bbef10de 100644 --- a/apps/remote_control/lib/lexical/remote_control/code_action.ex +++ b/apps/remote_control/lib/lexical/remote_control/code_action.ex @@ -29,7 +29,8 @@ defmodule Lexical.RemoteControl.CodeAction do Handlers.ReplaceRemoteFunction, Handlers.ReplaceWithUnderscore, Handlers.OrganizeAliases, - Handlers.AddAlias + Handlers.AddAlias, + Handlers.RemoveUnusedAlias ] @spec new(Lexical.uri(), String.t(), code_action_kind(), Changes.t()) :: t() diff --git a/apps/remote_control/lib/lexical/remote_control/code_action/handlers/remove_unused_alias.ex b/apps/remote_control/lib/lexical/remote_control/code_action/handlers/remove_unused_alias.ex new file mode 100644 index 000000000..2dfa432fd --- /dev/null +++ b/apps/remote_control/lib/lexical/remote_control/code_action/handlers/remove_unused_alias.ex @@ -0,0 +1,245 @@ +defmodule Lexical.RemoteControl.CodeAction.Handlers.RemoveUnusedAlias do + @moduledoc """ + A code action that removes an unused alias + + Most of the code actions are fairly straightforward, but I think this one deserves a couple of comments on + the approach. I initially tried the following: + + * Finding the alias via Sourceror's Zipper.find + * Rewriting the ast via Macro.prewalk / postwalk + * Using Macro.travesrse where I'd mark the metadata as being deleted in the prewalker, and + delete it in the postwalker. + + They had the following problems. + Sourceror would consistently produce ast that was not recognized by elixir 1.14's code normalizer, causing a crash. + Using AST rewriting was susceptible to infinite recursion, and it was extremely difficult to delete blocks reliably. + Blocks in one context would be deleted, but with a different formulation, nils would appear in the output code. + It was also very difficult to pop up the stack and delete an entire multiple alias without zippers. + + So the approach we have here utilizes a hybrid of AST walking / text replacement. It works for all the examples + I could come up with, but it's a bit longer than I desired. Dorgan said he'd take a look at the errors in the + normalizer and possibly fix sourceror, so until then, this is what we have. + """ + + alias Lexical.Ast + alias Lexical.Ast.Analysis + alias Lexical.Document + alias Lexical.Document.Changes + alias Lexical.Document.Edit + alias Lexical.Document.Position + alias Lexical.Document.Range + alias Lexical.RemoteControl.Analyzer + alias Lexical.RemoteControl.CodeAction + alias Lexical.RemoteControl.CodeAction.Diagnostic + alias Sourceror.Zipper + + import Record + + defrecordp :multi_alias_metadata, [ + :document, + :multi_alias_range, + :removed_alias_range, + :alias_count + ] + + defrecordp :single_alias_metadata, [:document, :range] + @behaviour CodeAction.Handler + + @impl CodeAction.Handler + def actions(%Document{} = document, %Range{} = range, diagnostics) do + Enum.reduce(diagnostics, [], fn %Diagnostic{} = diagnostic, acc -> + case to_edit(document, range.start, diagnostic) do + {:ok, module_name, edit} -> + changes = Changes.new(document, [edit]) + action = CodeAction.new(document.uri, "Remove alias #{module_name}", :source, changes) + + [action | acc] + + _ -> + acc + end + end) + end + + @impl CodeAction.Handler + def kinds do + [:source] + end + + defp to_edit(%Document{} = document, %Position{} = position, %Diagnostic{} = diagnostic) do + with {:ok, module_string} <- fetch_unused_alias_module_string(diagnostic), + {:ok, _doc, %Analysis{} = analysis} <- Document.Store.fetch(document.uri, :analysis), + last_segment = String.to_atom(module_string), + {:ok, full_alias} <- fetch_full_alias(analysis, position, last_segment), + {:ok, alias_meta} <- fetch_alias_metadata(position, analysis, full_alias, last_segment), + {:ok, edit} <- fetch_edit(alias_meta) do + {:ok, module_string, edit} + else + _ -> + :error + end + end + + @alias_regex ~r/unused alias (\w+)/ + defp fetch_unused_alias_module_string(%Diagnostic{} = diagnostic) do + case Regex.scan(@alias_regex, diagnostic.message) do + [[_, module_string]] -> {:ok, module_string} + _ -> :error + end + end + + defp fetch_alias_metadata( + %Position{} = cursor, + %Analysis{} = analysis, + full_alias, + last_segment + ) do + zipper = Zipper.zip(analysis.ast) + document = analysis.document + + with :error <- find_single_alias(document, cursor, zipper, full_alias, last_segment) do + find_multi_alias(document, cursor, zipper, last_segment) + end + end + + defp find_single_alias( + %Document{} = document, + %Position{} = cursor, + %Zipper{} = zipper, + full_alias, + last_segment + ) do + finder = fn + {:alias, _, [{:__aliases__, _, ^full_alias}]} = node -> + Ast.contains_position?(node, cursor) + + {:alias, _, + [ + {:__aliases__, _, ^full_alias}, + [{{:__block__, _, [:as]}, {:__aliases__, _, [^last_segment]}}] + ]} = node -> + Ast.contains_position?(node, cursor) + + _ -> + false + end + + case Zipper.find(zipper, finder) do + nil -> + :error + + %Zipper{node: node} -> + metadata = + single_alias_metadata(document: document, range: Ast.Range.fetch!(node, document)) + + {:ok, metadata} + end + end + + defp find_multi_alias( + %Document{} = document, + %Position{} = cursor, + %Zipper{} = zipper, + last_segment + ) do + finder = fn + {:alias, _, [{{:., _, _}, _, multi_alias_list}]} = node -> + Enum.find_value(multi_alias_list, &segment_matches?(&1, last_segment)) and + Ast.contains_position?(node, cursor) + + _ -> + false + end + + case Zipper.find(zipper, finder) do + nil -> + :error + + %Zipper{node: {:alias, _, [{{:., _, _}, _, multi_alias_list}]}} = zipper -> + alias_node = Enum.find(multi_alias_list, &segment_matches?(&1, last_segment)) + + multi_alias = + multi_alias_metadata( + document: document, + multi_alias_range: Ast.Range.fetch!(zipper.node, document), + removed_alias_range: Ast.Range.fetch!(alias_node, document), + alias_count: length(multi_alias_list) + ) + + {:ok, multi_alias} + end + end + + defp fetch_full_alias(%Analysis{} = analysis, %Position{} = position, last_segment) do + aliases = Analyzer.aliases_at(analysis, position) + + with {:ok, aliased_module} <- Map.fetch(aliases, last_segment), + {:elixir, full_alias} <- Ast.Module.safe_split(aliased_module, as: :atoms) do + {:ok, full_alias} + end + end + + defp segment_matches?({:__aliases__, _, segments}, last_segment) do + List.last(segments) == last_segment + end + + defp segment_matches?(_, _), do: false + + defp fetch_edit(single_alias_metadata(range: %Range{} = range)) do + updated_range = + range + |> put_in([:start, :character], 1) + |> include_next_line() + + {:ok, Edit.new("", updated_range)} + end + + defp fetch_edit(multi_alias_metadata(alias_count: 1, multi_alias_range: range)) do + # we're removing the last alias, so we can remove the entire thing. + {:ok, Edit.new("", range)} + end + + defp fetch_edit( + multi_alias_metadata( + document: %Document{} = document, + removed_alias_range: %Range{} = range + ) + ) do + current_line = line_text(document, range.start.line) + previous_line = line_text(document, range.start.line - 1) + + {range, edit_text} = + if not String.ends_with?(current_line, ",") and String.ends_with?(previous_line, ",") do + # delete the previous line's comma + range = %Range{ + range + | start: Position.new(document, range.start.line - 1, String.length(previous_line)) + } + + {range, "\n"} + else + {put_in(range.start.character, 1), ""} + end + + {:ok, Edit.new(edit_text, include_next_line(range))} + end + + defp fetch_edit(_), do: :error + + defp line_text(%Document{} = document, line_number) do + case Document.fetch_text_at(document, line_number) do + {:ok, line_text} -> line_text + _ -> "" + end + end + + defp include_next_line(%Range{} = range) do + update_in(range.end, fn old_position -> + %Position{ + old_position + | line: old_position.line + 1, + character: 1 + } + end) + end +end diff --git a/apps/remote_control/test/lexical/remote_control/code_action/handlers/remove_unused_alias_test.exs b/apps/remote_control/test/lexical/remote_control/code_action/handlers/remove_unused_alias_test.exs new file mode 100644 index 000000000..7c1f619c0 --- /dev/null +++ b/apps/remote_control/test/lexical/remote_control/code_action/handlers/remove_unused_alias_test.exs @@ -0,0 +1,360 @@ +defmodule Lexical.RemoteControl.CodeAction.Handlers.RemoveUnusedAliasTest do + alias Lexical.Ast + alias Lexical.Document + alias Lexical.Document.Range + alias Lexical.RemoteControl.CodeAction.Diagnostic + alias Lexical.RemoteControl.CodeAction.Handlers.RemoveUnusedAlias + + import Lexical.Test.CursorSupport + import Lexical.Test.CodeSigil + + use Lexical.Test.CodeMod.Case, enable_ast_conversion: false + + def apply_code_mod(original_text, _ast, options) do + Document.Store.open("file:///file.ex", original_text, 1) + {:ok, document} = Document.Store.fetch("file:///file.ex") + + cursor = Keyword.get(options, :cursor) + + start_pos = update_in(cursor.character, fn _ -> 1 end) + end_pos = update_in(start_pos.line, &(&1 + 1)) + + message = + case Keyword.get(options, :alias) do + nil -> + Keyword.get(options, :message, "warning: unused alias Foo") + + module -> + "warning: unused alias #{module}" + end + + range = Document.Range.new(cursor, cursor) + line_range = Range.new(start_pos, end_pos) + + diagnostic = Diagnostic.new(line_range, message, :elixir) + + edits = + document + |> RemoveUnusedAlias.actions(range, [diagnostic]) + |> Enum.flat_map(& &1.changes.edits) + + {:ok, edits} + end + + def remove_alias(orig_text, opts \\ []) do + {position, stripped} = pop_cursor(orig_text) + + opts = Keyword.merge(opts, cursor: position) + modify(stripped, opts) + end + + setup do + start_supervised!({Document.Store, derive: [analysis: &Lexical.Ast.analyze/1]}) + :ok + end + + describe "at the top level" do + test "removes an alias" do + assert {:ok, ""} = remove_alias("alias Foo.Bar.Baz|", alias: "Baz") + end + + test "deletes the line completely" do + {:ok, doc} = + ~q[ + alias Foo.Bar.Baz| + Remote.function_call() + ] + |> remove_alias(alias: "Baz") + + assert "Remote.function_call()" == doc + end + + test "removes an alias in the middle of an alias block" do + {:ok, removed} = + ~q[ + alias Foo.Bar.Baz + alias Quux.Stuff| + alias Yet.More.Things + ] + |> remove_alias(alias: "Stuff") + + assert ~q[ + alias Foo.Bar.Baz + alias Yet.More.Things + ] =~ removed + end + + test "removes an alias at the end of an alias block" do + {:ok, removed} = + ~q[ + alias Foo.Bar.Baz + alias Quux.Stuff + alias Yet.More.Things| + ] + |> remove_alias(alias: "Things") + + assert ~q[ + alias Foo.Bar.Baz + alias Quux.Stuff + ] =~ removed + end + + test "works using as" do + {:ok, removed} = + ~q[ + alias Foo.Bar.Baz, as: Quux| + ] + |> remove_alias(alias: "Quux") + + assert "" == removed + end + + test "only deletes the alias on the cursor's line" do + {:ok, removed} = + ~q[ + alias Foo.Bar + alias Something.Else + alias Foo.Bar| + ] + |> remove_alias(alias: "Bar") + + assert ~q[ + alias Foo.Bar + alias Something.Else + ] =~ removed + end + + test "leaves things alone if the message is different" do + assert {:ok, "alias This.Is.Correct"} == + remove_alias("alias This.Is.Correct|", message: "ugly code") + end + end + + describe "in a module" do + test "removes an alias" do + {:ok, removed} = + ~q[ + defmodule MyModule do + alias Foo.Bar.Baz| + end + ] + |> remove_alias(alias: "Baz") + + assert "defmodule MyModule do\nend" =~ removed + end + + test "removes an alias in the middle of an alias block" do + {:ok, removed} = + ~q[ + defmodule MyModule do + alias Foo.Bar.Baz + alias Quux.Stuff| + alias Yet.More.Things + end + ] + |> remove_alias(alias: "Stuff") + + assert ~q[ + defmodule MyModule do + alias Foo.Bar.Baz + alias Yet.More.Things + end + ] =~ removed + end + + test "removes an alias at the end of an alias block" do + {:ok, removed} = + ~q[ + defmodule MyModule do + alias Foo.Bar.Baz + alias Quux.Stuff + alias Yet.More.Things| + end + ] + |> remove_alias(alias: "Things") + + assert ~q[ + defmodule MyModule do + alias Foo.Bar.Baz + alias Quux.Stuff + end + ] =~ removed + end + end + + describe "in deeply nested modules" do + test "aliases are removed completely" do + {:ok, removed} = + ~q[ + defmodule Grandparent do + defmodule Parent do + defmodule Child do + alias This.Goes.Bye| + end + end + end + ] + |> remove_alias(alias: "Bye") + + expected = ~q[ + defmodule Grandparent do + defmodule Parent do + defmodule Child do + end + end + end + ] + + assert expected =~ removed + end + end + + describe "multi-line alias block" do + test "the first alias can be removed" do + {:ok, removed} = + ~q[ + alias Grandparent.Parent.|{ + Child1, + Child2, + Child3 + } + ] + |> remove_alias(alias: "Child1") + + expected = ~q[ + alias Grandparent.Parent.{ + Child2, + Child3 + } + ] + + assert expected =~ removed + end + + test "the second alias can be removed" do + {:ok, removed} = + ~q[ + alias Grandparent.Parent.|{ + Child1, + Child2, + Child3 + } + ] + |> remove_alias(alias: "Child2") + + expected = ~q[ + alias Grandparent.Parent.{ + Child1, + Child3 + } + ] + + assert expected =~ removed + end + + test "the last alias can be removed" do + {:ok, removed} = + ~q[ + alias Grandparent.Parent.|{ + Child1, + Child2, + Child3 + } + ] + |> remove_alias(alias: "Child3") + + expected = ~q[ + alias Grandparent.Parent.{ + Child1, + Child2 + } + ] + + assert expected =~ removed + end + + test "the only alias can be removed" do + {:ok, removed} = + ~q[ + alias Grandparent.Parent.|{ + Child1 + } + ] + |> remove_alias(alias: "Child1") + + assert "" =~ removed + end + + test "when there are dotted aliases in the list" do + {:ok, removed} = + ~q[ + alias Grandparent.Parent.{ + Child.Stinky, + Child.Smelly|, + Other.Reeky + } + ] + |> remove_alias(alias: "Smelly") + + expected = ~q[ + alias Grandparent.Parent.{ + Child.Stinky, + Other.Reeky + } + ] + + assert expected =~ removed + end + end + + describe "single-line alias block" do + test "the first alias can be removed" do + {:ok, removed} = + ~q[alias Grandparent.Parent.|{Child1, Child2, Child3}] + |> remove_alias(alias: "Child1") + + expected = ~q[alias Grandparent.Parent.{Child2, Child3}] + + assert expected =~ removed + end + + test "the second alias can be removed" do + {:ok, removed} = + ~q[alias Grandparent.Parent.|{Child1, Child2, Child3}] + |> remove_alias(alias: "Child2") + + expected = ~q[ + alias Grandparent.Parent.{Child1, Child3} + ] + + assert expected =~ removed + end + + test "the last alias can be removed" do + {:ok, removed} = + ~q[ + alias Grandparent.Parent.|{Child1, Child2, Child3}] + |> remove_alias(alias: "Child3") + + expected = ~q[alias Grandparent.Parent.{Child1, Child2] + + assert expected =~ removed + end + + test "the only alias can be removed" do + {:ok, removed} = + ~q[alias Grandparent.Parent.|{Child1}] + |> remove_alias(alias: "Child1") + + assert "" =~ removed + end + + test "when there are dotted aliases in the list" do + {:ok, removed} = + "alias Grandparent.Parent.{Child.Stinky, Child.Smelly, Other.Reeky}" + |> remove_alias(alias: "Smelly") + + assert "alias Grandparent.Parent.{Child.Stinky, Other.Reeky}" =~ removed + end + end +end