From 069cf41f89f957fc2b56772cd3323fea6f19a1a2 Mon Sep 17 00:00:00 2001 From: Samuel Heldak Date: Tue, 12 Mar 2024 16:19:29 +0100 Subject: [PATCH 1/2] Use built-in function to convert source code to ast --- .../providers/code_action/helpers.ex | 77 ++--- .../code_action/replace_remote_function.ex | 129 ++++---- .../code_action/replace_with_underscore.ex | 96 +++--- .../language_server/providers/code_mod/ast.ex | 35 ++- .../providers/code_mod/text.ex | 33 -- .../replace_remote_function_test.exs | 282 +++++++++++------- .../replace_with_underscore_test.exs | 178 ++++++----- .../test/support/code_mode_case.ex | 10 +- 8 files changed, 446 insertions(+), 394 deletions(-) delete mode 100644 apps/language_server/lib/language_server/providers/code_mod/text.ex diff --git a/apps/language_server/lib/language_server/providers/code_action/helpers.ex b/apps/language_server/lib/language_server/providers/code_action/helpers.ex index cc4a50358..3dab92000 100644 --- a/apps/language_server/lib/language_server/providers/code_action/helpers.ex +++ b/apps/language_server/lib/language_server/providers/code_action/helpers.ex @@ -1,7 +1,46 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.Helpers do alias ElixirLS.LanguageServer.Protocol.TextEdit - alias ElixirLS.LanguageServer.Providers.CodeMod.Ast - alias ElixirLS.LanguageServer.Providers.CodeMod.Text + alias ElixirLS.LanguageServer.Providers.CodeMod.Diff + + @spec to_text_edits(String.t(), String.t()) :: {:ok, [TextEdit.t()]} | :error + def to_text_edits(unformatted_text, updated_text) do + formatted_text = + unformatted_text + |> Code.format_string!(line_length: :infinity) + |> IO.iodata_to_binary() + + change_text_edits = Diff.diff(formatted_text, updated_text) + + with {:ok, changed_line} <- changed_line(change_text_edits) do + is_line_formatted = + unformatted_text + |> Diff.diff(formatted_text) + |> Enum.filter(fn %TextEdit{range: range} -> + range["start"]["line"] == changed_line or range["end"]["line"] == changed_line + end) + |> Enum.empty?() + + if is_line_formatted do + {:ok, change_text_edits} + else + :error + end + end + end + + defp changed_line(text_edits) do + lines = + text_edits + |> Enum.flat_map(fn %TextEdit{range: range} -> + [range["start"]["line"], range["end"]["line"]] + end) + |> Enum.uniq() + + case lines do + [line] -> {:ok, line} + _ -> :error + end + end @spec update_line(TextEdit.t(), non_neg_integer()) :: TextEdit.t() def update_line( @@ -16,38 +55,4 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.Helpers do } } end - - @spec to_one_line_string(Ast.t()) :: {:ok, String.t()} | :error - def to_one_line_string(updated_ast) do - updated_ast - |> Ast.to_string() - # We're dealing with a single error on a single line. - # If the line doesn't compile (like it has a do with no end), ElixirSense - # adds additional lines to documents with errors. Also, in case of a one-line do, - # ElixirSense creates do with end from the AST. - |> maybe_recover_one_line_do(updated_ast) - |> Text.fetch_line(0) - end - - @do_regex ~r/\s*do\s*/ - defp maybe_recover_one_line_do(updated_text, {_name, context, _children} = _updated_ast) do - wrong_do_end_conditions = [ - not Keyword.has_key?(context, :do), - not Keyword.has_key?(context, :end), - Regex.match?(@do_regex, updated_text), - String.ends_with?(updated_text, "\nend") - ] - - if Enum.all?(wrong_do_end_conditions) do - updated_text - |> String.replace(@do_regex, ", do: ") - |> String.trim_trailing("\nend") - else - updated_text - end - end - - defp maybe_recover_one_line_do(updated_text, _updated_ast) do - updated_text - end end diff --git a/apps/language_server/lib/language_server/providers/code_action/replace_remote_function.ex b/apps/language_server/lib/language_server/providers/code_action/replace_remote_function.ex index 52031c4b2..56fc91c8d 100644 --- a/apps/language_server/lib/language_server/providers/code_action/replace_remote_function.ex +++ b/apps/language_server/lib/language_server/providers/code_action/replace_remote_function.ex @@ -6,10 +6,9 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunction do use ElixirLS.LanguageServer.Protocol + alias ElixirLS.LanguageServer.Protocol.TextEdit alias ElixirLS.LanguageServer.Providers.CodeAction.CodeActionResult alias ElixirLS.LanguageServer.Providers.CodeMod.Ast - alias ElixirLS.LanguageServer.Providers.CodeMod.Diff - alias ElixirLS.LanguageServer.Providers.CodeMod.Text alias ElixirLS.LanguageServer.SourceFile alias ElixirSense.Core.Parser @@ -18,11 +17,14 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunction do @spec apply(SourceFile.t(), String.t(), [map()]) :: [CodeActionResult.t()] def apply(%SourceFile{} = source_file, uri, diagnostics) do Enum.flat_map(diagnostics, fn diagnostic -> - with {:ok, module, function, arity, line_number} <- extract_function_and_line(diagnostic), - {:ok, suggestions} <- prepare_suggestions(module, function, arity) do - to_code_actions(source_file, line_number, module, function, suggestions, uri) - else - _ -> [] + case extract_function_and_line(diagnostic) do + {:ok, module, function, arity, line} -> + suggestions = prepare_suggestions(module, function, arity) + + build_code_actions(source_file, line, module, function, suggestions, uri) + + :error -> + [] end end) end @@ -38,6 +40,9 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunction do with [[_, module_and_function, arity]] <- Regex.scan(@function_re, message), {:ok, module, function_name} <- separate_module_from_function(module_and_function) do {:ok, module, function_name, String.to_integer(arity)} + else + _ -> + :error end end @@ -65,17 +70,14 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunction do @function_threshold 0.77 @max_suggestions 5 defp prepare_suggestions(module, function, arity) do - suggestions = - for {module_function, ^arity} <- module_functions(module), - distance = module_function |> Atom.to_string() |> String.jaro_distance(function), - distance >= @function_threshold do - {distance, module_function} - end - |> Enum.sort(:desc) - |> Enum.take(@max_suggestions) - |> Enum.map(fn {_distance, module_function} -> module_function end) - - {:ok, suggestions} + for {module_function, ^arity} <- module_functions(module), + distance = module_function |> Atom.to_string() |> String.jaro_distance(function), + distance >= @function_threshold do + {distance, module_function} + end + |> Enum.sort(:desc) + |> Enum.take(@max_suggestions) + |> Enum.map(fn {_distance, module_function} -> module_function end) end defp module_functions(module) do @@ -86,12 +88,12 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunction do end end - defp to_code_actions(%SourceFile{} = source_file, line_number, module, name, suggestions, uri) do + defp build_code_actions(%SourceFile{} = source_file, line, module, name, suggestions, uri) do suggestions |> Enum.reduce([], fn suggestion, acc -> - case apply_transform(source_file, line_number, module, name, suggestion) do + case text_edits(source_file, line, module, name, suggestion) do {:ok, [_ | _] = text_edits} -> - text_edits = Enum.map(text_edits, &update_line(&1, line_number)) + text_edits = Enum.map(text_edits, &update_line(&1, line)) code_action = CodeActionResult.new("Rename to #{suggestion}", "quickfix", text_edits, uri) @@ -105,58 +107,51 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunction do |> Enum.reverse() end - defp apply_transform(source_file, line_number, module, name, suggestion) do - with {:ok, text} <- fetch_line(source_file, line_number), - {:ok, ast} <- Ast.from(text) do - function_atom = String.to_atom(name) - - leading_indent = Text.leading_indent(text) - trailing_comment = Text.trailing_comment(text) - - ast - |> Macro.postwalk(fn - {:., function_meta, [{:__aliases__, module_meta, module_alias}, ^function_atom]} -> - case expand_alias(source_file, module_alias, line_number) do - {:ok, ^module} -> - {:., function_meta, [{:__aliases__, module_meta, module_alias}, suggestion]} - - _ -> - {:., function_meta, [{:__aliases__, module_meta, module_alias}, function_atom]} - end - - # erlang call - {:., function_meta, [^module, ^function_atom]} -> - {:., function_meta, [module, suggestion]} - - other -> - other - end) - |> to_one_line_string() - |> case do - {:ok, updated_text} -> - text_edits = Diff.diff(text, "#{leading_indent}#{updated_text}#{trailing_comment}") - - {:ok, text_edits} - - :error -> - :error - end + @spec text_edits(SourceFile.t(), non_neg_integer(), atom(), String.t(), atom()) :: + {:ok, [TextEdit.t()]} | :error + defp text_edits(%SourceFile{} = source_file, line, module, name, suggestion) do + with {:ok, updated_text} <- apply_transform(source_file, line, module, name, suggestion) do + to_text_edits(source_file.text, updated_text) end end - defp fetch_line(%SourceFile{} = source_file, line_number) do - lines = SourceFile.lines(source_file) + defp apply_transform(source_file, line, module, name, suggestion) do + with {:ok, ast, comments} <- Ast.from(source_file) do + function_atom = String.to_atom(name) - if length(lines) > line_number do - {:ok, Enum.at(lines, line_number)} - else - :error + one_based_line = line + 1 + + updated_text = + ast + |> Macro.postwalk(fn + {:., [line: ^one_based_line], + [{:__aliases__, module_meta, module_alias}, ^function_atom]} -> + case expand_alias(source_file, module_alias, line) do + {:ok, ^module} -> + {:., [line: one_based_line], + [{:__aliases__, module_meta, module_alias}, suggestion]} + + _ -> + {:., [line: one_based_line], + [{:__aliases__, module_meta, module_alias}, function_atom]} + end + + # erlang call + {:., [line: ^one_based_line], [{:__block__, module_meta, [^module]}, ^function_atom]} -> + {:., [line: one_based_line], [{:__block__, module_meta, [module]}, suggestion]} + + other -> + other + end) + |> Ast.to_string(comments) + + {:ok, updated_text} end end @spec expand_alias(SourceFile.t(), [atom()], non_neg_integer()) :: {:ok, atom()} | :error - defp expand_alias(source_file, module_alias, line_number) do - with {:ok, aliases} <- aliases_at(source_file, line_number) do + defp expand_alias(source_file, module_alias, line) do + with {:ok, aliases} <- aliases_at(source_file, line) do aliases |> Enum.map(fn {module, aliased} -> module = module |> module_to_alias() |> List.first() @@ -177,8 +172,8 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunction do end end - defp aliases_at(source_file, line_number) do - one_based_line = line_number + 1 + defp aliases_at(source_file, line) do + one_based_line = line + 1 metadata = Parser.parse_string(source_file.text, true, true, {one_based_line, 1}) diff --git a/apps/language_server/lib/language_server/providers/code_action/replace_with_underscore.ex b/apps/language_server/lib/language_server/providers/code_action/replace_with_underscore.ex index 311b92239..4318a4b72 100644 --- a/apps/language_server/lib/language_server/providers/code_action/replace_with_underscore.ex +++ b/apps/language_server/lib/language_server/providers/code_action/replace_with_underscore.ex @@ -8,8 +8,6 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceWithUnderscore do alias ElixirLS.LanguageServer.Protocol.TextEdit alias ElixirLS.LanguageServer.Providers.CodeAction.CodeActionResult alias ElixirLS.LanguageServer.Providers.CodeMod.Ast - alias ElixirLS.LanguageServer.Providers.CodeMod.Diff - alias ElixirLS.LanguageServer.Providers.CodeMod.Text alias ElixirLS.LanguageServer.SourceFile import ElixirLS.LanguageServer.Providers.CodeAction.Helpers @@ -17,11 +15,11 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceWithUnderscore do @spec apply(SourceFile.t(), String.t(), [map()]) :: [CodeActionResult.t()] def apply(%SourceFile{} = source_file, uri, diagnostics) do Enum.flat_map(diagnostics, fn diagnostic -> - with {:ok, variable_name, line_number} <- extract_variable_and_line(diagnostic), - {:ok, reply} <- build_code_action(source_file, uri, line_number, variable_name) do + with {:ok, variable_name, line} <- extract_variable_and_line(diagnostic), + {:ok, reply} <- build_code_action(source_file, uri, line, variable_name) do [reply] else - _ -> + :error -> [] end end) @@ -44,71 +42,51 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceWithUnderscore do end end - defp build_code_action(%SourceFile{} = source_file, uri, line_number, variable_name) do - with {:ok, line_text} <- fetch_line(source_file, line_number), - {:ok, line_ast} <- Ast.from(line_text), - {:ok, text_edits} <- text_edits(line_text, line_ast, variable_name) do - case text_edits do - [] -> - :error - - [_ | _] -> - text_edits = Enum.map(text_edits, &update_line(&1, line_number)) - - reply = - CodeActionResult.new( - "Rename to _#{variable_name}", - "quickfix", - text_edits, - uri - ) - - {:ok, reply} - end - end - end + defp build_code_action(%SourceFile{} = source_file, uri, line, variable_name) do + case text_edits(source_file, line, variable_name) do + {:ok, [_ | _] = text_edits} -> + text_edits = Enum.map(text_edits, &update_line(&1, line)) - defp fetch_line(%SourceFile{} = source_file, line_number) do - lines = SourceFile.lines(source_file) + reply = + CodeActionResult.new( + "Rename to _#{variable_name}", + "quickfix", + text_edits, + uri + ) - if length(lines) > line_number do - {:ok, Enum.at(lines, line_number)} - else - :error + {:ok, reply} + + :error -> + :error end end - @spec text_edits(String.t(), Ast.t(), atom()) :: {:ok, [TextEdit.t()]} | :error - defp text_edits(original_text, ast, variable_name) do - with {:ok, transformed} <- apply_transform(original_text, ast, variable_name) do - {:ok, to_text_edits(original_text, transformed)} + @spec text_edits(SourceFile.t(), non_neg_integer(), atom()) :: {:ok, [TextEdit.t()]} | :error + defp text_edits(%SourceFile{text: unformatted_text} = source_file, line, variable_name) do + with {:ok, updated_text} <- apply_transform(source_file, line, variable_name) do + to_text_edits(unformatted_text, updated_text) end end - defp apply_transform(line_text, quoted_ast, unused_variable_name) do - underscored_variable_name = :"_#{unused_variable_name}" - leading_indent = Text.leading_indent(line_text) + defp apply_transform(source_file, line, unused_variable_name) do + with {:ok, ast, comments} <- Ast.from(source_file) do + underscored_variable_name = :"_#{unused_variable_name}" - Macro.postwalk(quoted_ast, fn - {^unused_variable_name, meta, nil} -> - {underscored_variable_name, meta, nil} + one_based_line = line + 1 - other -> - other - end) - |> to_one_line_string() - |> case do - {:ok, text} -> - {:ok, "#{leading_indent}#{text}"} + updated_text = + ast + |> Macro.postwalk(fn + {^unused_variable_name, [line: ^one_based_line], nil} -> + {underscored_variable_name, [line: one_based_line], nil} - :error -> - :error - end - end + other -> + other + end) + |> Ast.to_string(comments) - defp to_text_edits(original_text, fixed_text) do - original_text - |> Diff.diff(fixed_text) - |> Enum.filter(&(&1.newText == "_")) + {:ok, updated_text} + end end end diff --git a/apps/language_server/lib/language_server/providers/code_mod/ast.ex b/apps/language_server/lib/language_server/providers/code_mod/ast.ex index 7749330a4..c4b9c4c7b 100644 --- a/apps/language_server/lib/language_server/providers/code_mod/ast.ex +++ b/apps/language_server/lib/language_server/providers/code_mod/ast.ex @@ -1,29 +1,34 @@ defmodule ElixirLS.LanguageServer.Providers.CodeMod.Ast do alias ElixirLS.LanguageServer.SourceFile - @type source :: SourceFile.t() | String.t() - @type t :: - atom() - | binary() - | [any()] - | number() - | {any(), any()} - | {atom() | {any(), [any()], atom() | [any()]}, Keyword.t(), atom() | [any()]} - - @spec from(source() | String.t()) :: {:ok, t()} | :error + @spec from(SourceFile.t() | String.t()) :: {:ok, Macro.t(), [map()]} | :error def from(%SourceFile{text: text}) do from(text) end def from(text) when is_binary(text) do - case ElixirSense.string_to_quoted(text, {1, 1}) do - {:ok, ast} -> {:ok, ast} + text = String.trim(text) + + to_quoted_opts = + [ + literal_encoder: &{:ok, {:__block__, &2, [&1]}}, + token_metadata: true, + unescape: false + ] + + case Code.string_to_quoted_with_comments(text, to_quoted_opts) do + {:ok, ast, comments} -> {:ok, ast, comments} _ -> :error end end - @spec to_string(t()) :: String.t() - def to_string(ast) do - Macro.to_string(ast) + @spec to_string(Macro.t(), [map()]) :: String.t() + def to_string(ast, comments) do + to_algebra_opts = [comments: comments, escape: false] + + ast + |> Code.quoted_to_algebra(to_algebra_opts) + |> Inspect.Algebra.format(:infinity) + |> IO.iodata_to_binary() end end diff --git a/apps/language_server/lib/language_server/providers/code_mod/text.ex b/apps/language_server/lib/language_server/providers/code_mod/text.ex deleted file mode 100644 index ab3fed99b..000000000 --- a/apps/language_server/lib/language_server/providers/code_mod/text.ex +++ /dev/null @@ -1,33 +0,0 @@ -defmodule ElixirLS.LanguageServer.Providers.CodeMod.Text do - @indent_regex ~r/^\s+/ - @comment_regex ~r/\s*#.*/ - - @spec leading_indent(String.t()) :: String.t() - def leading_indent(line_text) do - case Regex.scan(@indent_regex, line_text) do - [indent] -> indent - _ -> "" - end - end - - @spec trailing_comment(String.t()) :: String.t() - def trailing_comment(line_text) do - case Regex.scan(@comment_regex, line_text) do - [comment] -> comment - _ -> "" - end - end - - @spec fetch_line(String.t(), non_neg_integer()) :: {:ok, String.t()} | :error - def fetch_line(message, line_number) do - line = - message - |> String.split(["\r\n", "\r", "\n"]) - |> Enum.at(line_number) - - case line do - nil -> :error - other -> {:ok, other} - end - end -end diff --git a/apps/language_server/test/providers/code_action/replace_remote_function_test.exs b/apps/language_server/test/providers/code_action/replace_remote_function_test.exs index 5a38ce9ad..bc60afc56 100644 --- a/apps/language_server/test/providers/code_action/replace_remote_function_test.exs +++ b/apps/language_server/test/providers/code_action/replace_remote_function_test.exs @@ -15,6 +15,12 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunctionTest * count/2 """ + @foo_message """ + ElixirLS.Test.RemoteFunction.fou/1 is undefined or private. Did you mean: + + * foo/1 + """ + def apply_code_mod(original_text, options) do line_number = Keyword.get(options, :line, 0) @@ -51,8 +57,8 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunctionTest test "applied to a standalone call" do {:ok, [result]} = ~q{ - Enum.counts([1, 2, 3]) - } + Enum.counts([1, 2, 3]) + }t |> modify() assert result == "Enum.count([1, 2, 3])" @@ -61,28 +67,18 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunctionTest test "applied to a variable match" do {:ok, [result]} = ~q{ - x = Enum.counts([1, 2, 3]) - } + x = Enum.counts([1, 2, 3]) + }t |> modify() assert result == "x = Enum.count([1, 2, 3])" end - test "applied to a variable match, preserves comments" do - {:ok, [result]} = - ~q{ - x = Enum.counts([1, 2, 3]) # TODO: Fix this - } - |> modify() - - assert result == "x = Enum.count([1, 2, 3]) # TODO: Fix this" - end - test "not changing variable name" do {:ok, [result]} = ~q{ - counts = Enum.counts([1, 2, 3]) - } + counts = Enum.counts([1, 2, 3]) + }t |> modify() assert result == "counts = Enum.count([1, 2, 3])" @@ -91,8 +87,8 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunctionTest test "applied to a call after a pipe" do {:ok, [result]} = ~q{ - [1, 2, 3] |> Enum.counts() - } + [1, 2, 3] |> Enum.counts() + }t |> modify() assert result == "[1, 2, 3] |> Enum.count()" @@ -101,8 +97,8 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunctionTest test "changing only a function from provided possible modules" do {:ok, [result]} = ~q{ - Enumerable.counts([1, 2, 3]) + Enum.counts([3, 2, 1]) - } + Enumerable.counts([1, 2, 3]) + Enum.counts([3, 2, 1]) + }t |> modify() assert result == "Enumerable.counts([1, 2, 3]) + Enum.count([3, 2, 1])" @@ -111,8 +107,8 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunctionTest test "changing all occurrences of the function in the line" do {:ok, [result]} = ~q{ - Enum.counts([1, 2, 3]) + Enum.counts([3, 2, 1]) - } + Enum.counts([1, 2, 3]) + Enum.counts([3, 2, 1]) + }t |> modify() assert result == "Enum.count([1, 2, 3]) + Enum.count([3, 2, 1])" @@ -121,8 +117,8 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunctionTest test "applied in a comprehension" do {:ok, [result]} = ~q{ - for x <- Enum.counts([[1], [2], [3]]), do: Enum.counts([[1], [2], [3], [x]]) - } + for x <- Enum.counts([[1], [2], [3]]), do: Enum.counts([[1], [2], [3], [x]]) + }t |> modify(suggestion: "Enum.concat") assert result == @@ -132,40 +128,60 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunctionTest test "applied in a with block" do {:ok, [result]} = ~q{ - with x <- Enum.counts([1, 2, 3]), do: x - } + with x <- Enum.counts([1, 2, 3]), do: x + }t |> modify() assert result == "with x <- Enum.count([1, 2, 3]), do: x" end - test "applied in a with block, preserves comment" do + test "applied in a do-end with block preserves indent" do {:ok, [result]} = ~q{ - with x <- Enum.counts([1, 2, 3]), do: x # TODO: Fix this - } + with x <- Enum.counts([1, 2, 3]) do + nil + end + }t |> modify() - assert result == "with x <- Enum.count([1, 2, 3]), do: x # TODO: Fix this" + assert result == "with x <- Enum.count([1, 2, 3]) do\n nil\nend" end - test "applied in a with block with started do end" do + test "applied to a branch in a case" do {:ok, [result]} = ~q{ - with x <- Enum.counts([1, 2, 3]) do - } - |> modify() + case my_thing do + :ok -> Enum.counts([1, 2, 3]) + _ -> :error + end + }t + |> modify(line: 1) + + expected = + ~q{ + case my_thing do + :ok -> Enum.count([1, 2, 3]) + _ -> :error + end + }t - assert result == "with x <- Enum.count([1, 2, 3]) do" + assert result == expected end - test "preserving the leading indent" do - {:ok, [result]} = modify(" Enum.counts([1, 2, 3])", trim: false) - - assert result == " Enum.count([1, 2, 3])" + test "no change when unformatted line" do + {:ok, result} = + ~q{ + case my_thing do + :ok -> Enum.counts([1, 2, 3]) + _ -> :error + end + }t + |> modify(line: 1) + + assert result == [] end - test "handles erlang functions" do + test "applied to an erlang function" do message = """ :ets.inserd/2 is undefined or private. Did you mean: * insert/2 @@ -174,45 +190,60 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunctionTest {:ok, [result]} = ~q{ - :ets.inserd(a, b) - } + :ets.inserd(a, b) + }t |> modify(message: message, suggestion: ":ets.insert(a, b)") assert result == ":ets.insert(a, b)" end test "when aliased" do - message = """ - ElixirLS.Test.RemoteFunction.fou/1 is undefined or private. Did you mean: - - * foo/1 - """ - {:ok, [result]} = ~q{ - alias ElixirLS.Test.RemoteFunction - RemoteFunction.fou(42) - } - |> modify(message: message, suggestion: "RemoteFunction.foo", line: 1) + alias ElixirLS.Test.RemoteFunction + RemoteFunction.fou(42) + }t + |> modify(message: @foo_message, suggestion: "RemoteFunction.foo", line: 1) assert result == "alias ElixirLS.Test.RemoteFunction\nRemoteFunction.foo(42)" end test "when aliased with a custom name" do - message = """ - ElixirLS.Test.RemoteFunction.fou/1 is undefined or private. Did you mean: + {:ok, [result]} = + ~q{ + alias ElixirLS.Test.RemoteFunction, as: Remote + Remote.fou(42) + }t + |> modify(message: @foo_message, suggestion: "Remote.foo", line: 1) - * foo/1 - """ + assert result == "alias ElixirLS.Test.RemoteFunction, as: Remote\nRemote.foo(42)" + end + test "preserves other lines" do {:ok, [result]} = ~q{ - alias ElixirLS.Test.RemoteFunction, as: Remote - Remote.fou(42) - } - |> modify(message: message, suggestion: "Remote.foo", line: 1) + # 1st comment - assert result == "alias ElixirLS.Test.RemoteFunction, as: Remote\nRemote.foo(42)" + + Enum.counts([1, 2, 3]) # 2nd comment + # 3rd comment + + Enum.counts([1, 2, 3]) + }t + |> modify(line: 6, suggestion: "Enum.concat") + + expected = + ~q{ + # 1st comment + + + Enum.counts([1, 2, 3]) # 2nd comment + # 3rd comment + + Enum.concat([1, 2, 3]) + }t + + assert result == expected end end @@ -220,8 +251,8 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunctionTest test "applied to a standalone function" do {:ok, [result]} = ~q[ - &Enum.counts/1 - ] + &Enum.counts/1 + ]t |> modify() assert result == "&Enum.count/1" @@ -230,28 +261,18 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunctionTest test "applied to a variable match" do {:ok, [result]} = ~q[ - x = &Enum.counts/1 - ] + x = &Enum.counts/1 + ]t |> modify() assert result == "x = &Enum.count/1" end - test "applied to a variable match, preserves comments" do - {:ok, [result]} = - ~q[ - x = &Enum.counts/1 # TODO: Fix this - ] - |> modify() - - assert result == "x = &Enum.count/1 # TODO: Fix this" - end - test "not changing variable name" do {:ok, [result]} = - ~q{ - counts = &Enum.counts/1 - } + ~q[ + counts = &Enum.counts/1 + ]t |> modify() assert result == "counts = &Enum.count/1" @@ -260,8 +281,8 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunctionTest test "applied to an argument" do {:ok, [result]} = ~q{ - [[1, 2], [3, 4]] |> Enum.map(&Enum.counts/1) - } + [[1, 2], [3, 4]] |> Enum.map(&Enum.counts/1) + }t |> modify() assert result == "[[1, 2], [3, 4]] |> Enum.map(&Enum.count/1)" @@ -270,8 +291,8 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunctionTest test "changing only a function from provided possible modules" do {:ok, [result]} = ~q{ - [&Enumerable.counts/1, &Enum.counts/1] - } + [&Enumerable.counts/1, &Enum.counts/1] + }t |> modify() assert result == "[&Enumerable.counts/1, &Enum.count/1]" @@ -280,20 +301,48 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunctionTest test "changing all occurrences of the function in the line" do {:ok, [result]} = ~q{ - [&Enum.counts/1, &Enum.counts/1] - } + [&Enum.counts/1, &Enum.counts/1] + }t |> modify() assert result == "[&Enum.count/1, &Enum.count/1]" end - test "preserving the leading indent" do - {:ok, [result]} = modify(" &Enum.counts/1", trim: false) + test "applied to a branch in a case" do + {:ok, [result]} = + ~q[ + case my_thing do + :ok -> &Enum.counts/1 + _ -> :error + end + ]t + |> modify(line: 1) + + expected = + ~q[ + case my_thing do + :ok -> &Enum.count/1 + _ -> :error + end + ]t - assert result == " &Enum.count/1" + assert result == expected end - test "handles erlang functions" do + test "no change when unformatted line" do + {:ok, result} = + ~q{ + case my_thing do + :ok -> & Enum.counts/1 + _ -> :error + end + }t + |> modify(line: 1) + + assert result == [] + end + + test "applied to an erlang function" do message = """ :ets.inserd/2 is undefined or private. Did you mean: * insert/2 @@ -302,45 +351,60 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceRemoteFunctionTest {:ok, [result]} = ~q{ - &:ets.inserd/2 - } + &:ets.inserd/2 + }t |> modify(message: message, suggestion: ":ets.insert/2") assert result == "&:ets.insert/2" end test "when aliased" do - message = """ - ElixirLS.Test.RemoteFunction.fou/1 is undefined or private. Did you mean: - - * foo/1 - """ - {:ok, [result]} = ~q{ - alias ElixirLS.Test.RemoteFunction - &RemoteFunction.fou/1 - } - |> modify(message: message, suggestion: "RemoteFunction.foo", line: 1) + alias ElixirLS.Test.RemoteFunction + &RemoteFunction.fou/1 + }t + |> modify(message: @foo_message, suggestion: "RemoteFunction.foo", line: 1) assert result == "alias ElixirLS.Test.RemoteFunction\n&RemoteFunction.foo/1" end test "when aliased with a custom name" do - message = """ - ElixirLS.Test.RemoteFunction.fou/1 is undefined or private. Did you mean: + {:ok, [result]} = + ~q{ + alias ElixirLS.Test.RemoteFunction, as: Remote + &Remote.fou/1 + }t + |> modify(message: @foo_message, suggestion: "Remote.foo", line: 1) - * foo/1 - """ + assert result == "alias ElixirLS.Test.RemoteFunction, as: Remote\n&Remote.foo/1" + end + test "preserves other lines" do {:ok, [result]} = ~q{ - alias ElixirLS.Test.RemoteFunction, as: Remote - &Remote.fou/1 - } - |> modify(message: message, suggestion: "Remote.foo", line: 1) + # 1st comment - assert result == "alias ElixirLS.Test.RemoteFunction, as: Remote\n&Remote.foo/1" + + x = &Enum.counts/1 # 2nd comment + # 3rd comment + + &Enum.counts/1 + }t + |> modify(line: 6, suggestion: "Enum.concat") + + expected = + ~q{ + # 1st comment + + + x = &Enum.counts/1 # 2nd comment + # 3rd comment + + &Enum.concat/1 + }t + + assert result == expected end end end diff --git a/apps/language_server/test/providers/code_action/replace_with_underscore_test.exs b/apps/language_server/test/providers/code_action/replace_with_underscore_test.exs index 13dcb4b43..8323d330d 100644 --- a/apps/language_server/test/providers/code_action/replace_with_underscore_test.exs +++ b/apps/language_server/test/providers/code_action/replace_with_underscore_test.exs @@ -40,81 +40,81 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceWithUnderscoreTest test "applied to an unadorned param" do {:ok, [result]} = ~q[ - def my_func(unused) do - ] + def my_func(unused), do: nil + ]t |> modify() - assert result == "def my_func(_unused) do" + assert result == "def my_func(_unused), do: nil" end test "applied to a pattern match in params" do {:ok, [result]} = ~q[ - def my_func(%SourceFile{} = unused) do - ] + def my_func(%SourceFile{} = unused), do: nil + ]t |> modify() - assert result == "def my_func(%SourceFile{} = _unused) do" + assert result == "def my_func(%SourceFile{} = _unused), do: nil" end test "applied to a pattern match preceding a struct in params" do {:ok, [result]} = ~q[ - def my_func(unused = %SourceFile{}) do - ] + def my_func(unused = %SourceFile{}), do: nil + ]t |> modify() - assert result == "def my_func(_unused = %SourceFile{}) do" + assert result == "def my_func(_unused = %SourceFile{}), do: nil" end test "applied prior to a map" do {:ok, [result]} = ~q[ - def my_func(unused = %{}) do - ] + def my_func(unused = %{}), do: nil + ]t |> modify() - assert result == "def my_func(_unused = %{}) do" + assert result == "def my_func(_unused = %{}), do: nil" end test "applied after a map %{} = unused" do {:ok, [result]} = ~q[ - def my_func(%{} = unused) do - ] + def my_func(%{} = unused), do: nil + ]t |> modify() - assert result == "def my_func(%{} = _unused) do" + assert result == "def my_func(%{} = _unused), do: nil" end test "applied to a map key %{foo: unused}" do {:ok, [result]} = ~q[ - def my_func(%{foo: unused}) do - ] + def my_func(%{foo: unused}), do: nil + ]t |> modify() - assert result == "def my_func(%{foo: _unused}) do" + assert result == "def my_func(%{foo: _unused}), do: nil" end test "applied to a list element params = [unused, a, b | rest]" do {:ok, [result]} = ~q{ - def my_func([unused, a, b | rest]) do - } + def my_func([unused, a, b | rest]), do: nil + }t |> modify() - assert result == "def my_func([_unused, a, b | rest]) do" + assert result == "def my_func([_unused, a, b | rest]), do: nil" end test "applied to the tail of a list params = [a, b, | unused]" do {:ok, [result]} = ~q{ - def my_func([a, b | unused]) do - } + def my_func([a, b | unused]), do: nil + }t |> modify() - assert result == "def my_func([a, b | _unused]) do" + assert result == "def my_func([a, b | _unused]), do: nil" end test "does not change the name of a function if it is the same as a parameter" do @@ -122,11 +122,21 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceWithUnderscoreTest ~q{ def unused(unused) do end - } + }t |> modify() assert result == "def unused(_unused) do\nend" end + + test "no change when unformatted line" do + {:ok, result} = + ~q[ + def my_func(:a, unused), do: :ok + ]t + |> modify() + + assert result == [] + end end describe "fixes in variables" do @@ -134,55 +144,39 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceWithUnderscoreTest {:ok, [result]} = ~q[ x = 3 - ] + ]t |> modify(variable: "x") assert result == "_x = 3" end - test "applied to a variable match, preserves comments" do + test "preserves indentation" do {:ok, [result]} = ~q[ - unused = bar # TODO: Fix this - ] - |> modify() - - assert result == "_unused = bar # TODO: Fix this" - end - - test "preserves spacing" do - {:ok, [result]} = - " x = 3" - |> modify(variable: "x", trim: false) + if true do + x = 3 + end + ]t + |> modify(variable: "x", line: 1) - assert result == " _x = 3" + assert result == "if true do\n _x = 3\nend" end test "applied to a variable with a pattern matched struct" do {:ok, [result]} = ~q[ unused = %Struct{} - ] + ]t |> modify() assert result == "_unused = %Struct{}" end - test "applied to a variable with a pattern matched struct preserves trailing comments" do - {:ok, [result]} = - ~q[ - unused = %Struct{} # TODO: fix - ] - |> modify() - - assert result == "_unused = %Struct{} # TODO: fix" - end - test "applied to struct param matches" do {:ok, [result]} = ~q[ %Struct{field: unused, other_field: used} - ] + ]t |> modify() assert result == "%Struct{field: _unused, other_field: used}" @@ -192,7 +186,7 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceWithUnderscoreTest {:ok, [result]} = ~q[ %unused{field: first, other_field: used} - ] + ]t |> modify() assert result == "%_unused{field: first, other_field: used}" @@ -202,7 +196,7 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceWithUnderscoreTest {:ok, [result]} = ~q[ {a, b, unused, c} = whatever - ] + ]t |> modify() assert result == "{a, b, _unused, c} = whatever" @@ -212,7 +206,7 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceWithUnderscoreTest {:ok, [result]} = ~q{ [a, b, unused, c] = whatever - } + }t |> modify() assert result == "[a, b, _unused, c] = whatever" @@ -222,19 +216,58 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceWithUnderscoreTest {:ok, [result]} = ~q[ %{foo: a, bar: unused} = whatever - ] + ]t |> modify() assert result == "%{foo: a, bar: _unused} = whatever" end + + test "preserves other lines" do + {:ok, [result]} = + ~q[ + # 1st comment + + + x = 3 # 2nd comment + # 3rd comment + + {foo, unused, bar} + ]t + |> modify(line: 6) + + expected = + ~q[ + # 1st comment + + + x = 3 # 2nd comment + # 3rd comment + + {foo, _unused, bar} + ]t + + assert result == expected + end + + test "no change when unformatted multiline map" do + {:ok, result} = + ~q[ + %{ + foo: a, + bar: unused} = whatever + ]t + |> modify(line: 2) + + assert result == [] + end end describe "fixes in structures" do test "applied to a match of a comprehension" do {:ok, [result]} = ~q[ - for {unused, something_else} <- my_enum, do: something_else - ] + for {unused, something_else} <- my_enum, do: something_else + ]t |> modify() assert result == "for {_unused, something_else} <- my_enum, do: something_else" @@ -244,22 +277,31 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.ReplaceWithUnderscoreTest {:ok, [result]} = ~q[ with {unused, something_else} <- my_enum, do: something_else - ] + ]t |> modify() assert result == "with {_unused, something_else} <- my_enum, do: something_else" end - end - - test "it preserves the leading indent" do - {:ok, [result]} = modify(" {foo, unused, bar}", trim: false) - - assert result == " {foo, _unused, bar}" - end - test "it preserves a comment" do - {:ok, [result]} = modify("{foo, unused, bar} # TODO Fix this") + test "applied to a branch in a case" do + {:ok, [result]} = + ~q[ + case my_thing do + {:ok, unused} -> :ok + _ -> :error + end + ]t + |> modify(line: 1) + + expected = + ~q[ + case my_thing do + {:ok, _unused} -> :ok + _ -> :error + end + ]t - assert result == "{foo, _unused, bar} # TODO Fix this" + assert result == expected + end end end diff --git a/apps/language_server/test/support/code_mode_case.ex b/apps/language_server/test/support/code_mode_case.ex index d81fce8a0..2e8d76a63 100644 --- a/apps/language_server/test/support/code_mode_case.ex +++ b/apps/language_server/test/support/code_mode_case.ex @@ -20,24 +20,20 @@ defmodule ElixirLS.LanguageServer.Test.CodeMod.Case do def modify(original, options \\ []) do with {:ok, changes} <- apply_code_mod(original, options) do original - |> unquote(__MODULE__).apply_changes(changes, options) + |> unquote(__MODULE__).apply_changes(changes) |> filter_edited_texts(options) end end end end - def apply_changes(original_text, changes, opts) do + def apply_changes(original_text, changes) do Enum.map(changes, fn text_edits -> %SourceFile{text: edited_text} = %SourceFile{text: original_text, version: 0} |> SourceFile.apply_content_changes(text_edits) - if Keyword.get(opts, :trim, true) do - String.trim(edited_text) - else - edited_text - end + String.trim(edited_text) end) end From e6c00891d6a474b8edad41c6b14a9e508e71ca65 Mon Sep 17 00:00:00 2001 From: Samuel Heldak Date: Thu, 14 Mar 2024 10:02:04 +0100 Subject: [PATCH 2/2] no code action for potentially unformatted line --- .../language_server/providers/code_action/helpers.ex | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/apps/language_server/lib/language_server/providers/code_action/helpers.ex b/apps/language_server/lib/language_server/providers/code_action/helpers.ex index 3dab92000..59bb1bc1b 100644 --- a/apps/language_server/lib/language_server/providers/code_action/helpers.ex +++ b/apps/language_server/lib/language_server/providers/code_action/helpers.ex @@ -15,9 +15,7 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.Helpers do is_line_formatted = unformatted_text |> Diff.diff(formatted_text) - |> Enum.filter(fn %TextEdit{range: range} -> - range["start"]["line"] == changed_line or range["end"]["line"] == changed_line - end) + |> Enum.filter(&near_changed_line(&1, changed_line)) |> Enum.empty?() if is_line_formatted do @@ -42,6 +40,13 @@ defmodule ElixirLS.LanguageServer.Providers.CodeAction.Helpers do end end + defp near_changed_line(%TextEdit{range: range}, changed_line) do + changed_line_neighborhood = [changed_line - 1, changed_line, changed_line + 1] + + range["start"]["line"] in changed_line_neighborhood or + range["end"]["line"] in changed_line_neighborhood + end + @spec update_line(TextEdit.t(), non_neg_integer()) :: TextEdit.t() def update_line( %TextEdit{range: %{"start" => start_line, "end" => end_line}} = text_edit,