Completion: don't insert extra impl attribute if it's already present (…

…#801)

Behaviour callback completions implement `@impl true` above the definition, which is usually helpful, except for the case where you've already written it. This changes prevents a second one from being added.

apps/server/lib/lexical/server/code_intelligence/completion/translations/callback.ex

@@ -1,5 +1,6 @@
 defmodule Lexical.Server.CodeIntelligence.Completion.Translations.Callback do
   alias Lexical.Ast.Env
+  alias Lexical.Document
   alias Lexical.RemoteControl.Completion.Candidate.Callback
   alias Lexical.Server.CodeIntelligence.Completion.Builder
   alias Lexical.Server.CodeIntelligence.Completion.SortScope
@@ -17,7 +18,7 @@ defmodule Lexical.Server.CodeIntelligence.Completion.Translations.Callback do
 
       env
       |> Builder.text_edit_snippet(
-        insert_text(name, arg_names),
+        insert_text(name, arg_names, env),
         line_range(line),
         label: label(name, arg_names),
         kind: :interface,
@@ -29,11 +30,11 @@ defmodule Lexical.Server.CodeIntelligence.Completion.Translations.Callback do
       |> Builder.set_sort_scope(SortScope.local())
     end
 
-    defp insert_text(name, arg_names)
+    defp insert_text(name, arg_names, env)
          when is_binary(name) and is_list(arg_names) do
-      impl_line(name) <>
+      impl_line(name, env) <>
         "def #{name}(#{arg_text(arg_names)}) do" <>
-        "\n\t$0\nend"
+        "\n  $0\nend"
     end
 
     # add tab stops and join with ", "
@@ -47,11 +48,18 @@ defmodule Lexical.Server.CodeIntelligence.Completion.Translations.Callback do
 
     # elixir_sense suggests child_spec/1 as a callback as it's a common idiom,
     # but not an actual callback of behaviours like GenServer.
-    defp impl_line("child_spec"), do: ""
+    defp impl_line("child_spec", _env), do: ""
 
     # It's generally safe adding `@impl true` to callbacks as Elixir warns
     # of conflicting behaviours, and they're virtually non-existent anyway.
-    defp impl_line(_), do: "@impl true\n"
+    defp impl_line(_, %Env{} = env) do
+      with {:ok, line_before} <- Document.fetch_text_at(env.document, env.position.line - 1),
+           true <- line_before =~ "@impl" do
+        ""
+      else
+        _ -> "@impl true\n"
+      end
+    end
 
     defp line_range(line) when is_binary(line) do
       start_char =

apps/server/test/lexical/server/code_intelligence/completion/translations/callback_test.exs

@@ -35,5 +35,30 @@ defmodule Lexical.Server.CodeIntelligence.Completion.Translations.CallbackTest d
       assert apply_completion(completion) =~
                "@impl true\ndef handle_info(${1:msg}, ${2:state}) do"
     end
+
+    test "does not add second @impl if one is already present", %{project: project} do
+      source = ~q[
+        defmodule MyServer do
+          use GenServer
+          @impl true
+          def handle_inf|
+        end
+      ]
+
+      {:ok, completion} =
+        project
+        |> complete(source)
+        |> fetch_completion(kind: :interface)
+
+      assert apply_completion(completion) == """
+             defmodule MyServer do
+               use GenServer
+               @impl true
+               def handle_info(${1:msg}, ${2:state}) do
+               $0
+             end
+             end
+             """
+    end
   end
 end