Fix: Function definition extractor chokes on macro functions (#682)

* Fix: Function definition extractor chokes on macro functions

There's no location information on macro functions, and we needed it
for our detail block. This would cause crashes during indexing.

Fixes #680

apps/remote_control/lib/lexical/remote_control/search/indexer/extractors/function_definition.ex

@@ -1,85 +1,57 @@
 defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinition do
+  alias Lexical.Ast
   alias Lexical.Ast.Analysis
-  alias Lexical.Ast.Range
-  alias Lexical.Document.Position
-  alias Lexical.Document.Range
-  alias Lexical.RemoteControl
+  alias Lexical.RemoteControl.Analyzer
   alias Lexical.RemoteControl.Search.Indexer.Entry
-  alias Lexical.RemoteControl.Search.Indexer.Metadata
-  alias Lexical.RemoteControl.Search.Indexer.Source.Block
   alias Lexical.RemoteControl.Search.Indexer.Source.Reducer
   alias Lexical.RemoteControl.Search.Subject
 
   @function_definitions [:def, :defp]
 
-  def extract({definition, metadata, [{fn_name, _, args}, body]} = ast, %Reducer{} = reducer)
+  def extract({definition, _, [{fn_name, _, args} = def_ast, body]} = ast, %Reducer{} = reducer)
       when is_atom(fn_name) and definition in @function_definitions do
-    detail_range = detail_range(reducer.analysis, metadata, body)
-
-    {:ok, module} = RemoteControl.Analyzer.current_module(reducer.analysis, detail_range.start)
-
-    arity =
-      case args do
-        list when is_list(list) ->
-          length(list)
-
-        nil ->
-          0
-      end
-
-    type =
-      case definition do
-        :def -> :public_function
-        :defp -> :private_function
-      end
-
-    mfa = Subject.mfa(module, fn_name, arity)
-    %Block{} = block = Reducer.current_block(reducer)
-    path = reducer.analysis.document.path
-
-    block_range = block_range(reducer.analysis, ast)
-
-    entry =
-      Entry.block_definition(
-        path,
-        block,
-        mfa,
-        type,
-        block_range,
-        detail_range,
-        Application.get_application(module)
-      )
-
-    {:ok, entry, [args, body]}
+    with {:ok, detail_range} <- Ast.Range.fetch(def_ast, reducer.analysis.document),
+         {:ok, module} <- Analyzer.current_module(reducer.analysis, detail_range.start),
+         {fun_name, arity} when is_atom(fun_name) <- fun_name_and_arity(def_ast) do
+      entry =
+        Entry.block_definition(
+          reducer.analysis.document.path,
+          Reducer.current_block(reducer),
+          Subject.mfa(module, fun_name, arity),
+          type(definition),
+          block_range(reducer.analysis, ast),
+          detail_range,
+          Application.get_application(module)
+        )
+
+      {:ok, entry, [args, body]}
+    else
+      _ ->
+        :ignored
+    end
   end
 
   def extract(_ast, _reducer) do
     :ignored
   end
 
-  defp detail_range(%Analysis{} = analysis, def_metadata, block) do
-    {line, column} = Metadata.position(def_metadata)
+  defp type(:def), do: :public_function
+  defp type(:defp), do: :private_function
 
-    {do_line, do_column} =
-      case Sourceror.get_range(block) do
-        %{start: do_meta} ->
-          do_line = do_meta[:line]
-          do_column = do_meta[:column]
-          {do_line, do_column + 2}
-
-        nil ->
-          {line, column} = Metadata.position(def_metadata, :do)
-          # add two for the do
-          {line, column + 2}
-      end
+  defp fun_name_and_arity({:when, _, [{fun_name, _, fun_args} | _]}) do
+    # a function with guards
+    {fun_name, arity(fun_args)}
+  end
 
-    start_pos = Position.new(analysis.document, line, column)
-    do_pos = Position.new(analysis.document, do_line, do_column)
-    Range.new(start_pos, do_pos)
+  defp fun_name_and_arity({fun_name, _, fun_args}) do
+    {fun_name, arity(fun_args)}
   end
 
+  defp arity(nil), do: 0
+  defp arity(args) when is_list(args), do: length(args)
+
   defp block_range(%Analysis{} = analysis, def_ast) do
-    case Lexical.Ast.Range.fetch(def_ast, analysis.document) do
+    case Ast.Range.fetch(def_ast, analysis.document) do
       {:ok, range} -> range
       _ -> nil
     end

apps/remote_control/test/lexical/remote_control/code_intelligence/symbols_test.exs

@@ -118,7 +118,7 @@ defmodule Lexical.RemoteControl.CodeIntelligence.SymbolsTest do
       |> document_symbols()
 
     assert [function] = module.children
-    assert decorate(doc, function.detail_range) =~ " «def my_fn do»"
+    assert decorate(doc, function.detail_range) =~ " def «my_fn» do"
   end
 
   test "private function definitions are found" do
@@ -132,7 +132,7 @@ defmodule Lexical.RemoteControl.CodeIntelligence.SymbolsTest do
       |> document_symbols()
 
     assert [function] = module.children
-    assert decorate(doc, function.detail_range) =~ " «defp my_fn do»"
+    assert decorate(doc, function.detail_range) =~ " defp «my_fn» do"
     assert function.name == "my_fn"
   end
 
@@ -199,7 +199,7 @@ defmodule Lexical.RemoteControl.CodeIntelligence.SymbolsTest do
       |> document_symbols()
 
     [fun] = module.children
-    assert decorate(doc, fun.detail_range) =~ "  «def my_fun(x) when x > 0 do»"
+    assert decorate(doc, fun.detail_range) =~ "  def «my_fun(x) when x > 0» do"
     assert fun.type == :public_function
     assert fun.name == "my_fun(x) when x > 0"
     assert [] == fun.children

apps/remote_control/test/lexical/remote_control/search/indexer/extractors/function_definition_test.exs

@@ -27,7 +27,8 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
       assert zero_arity.type == :public_function
       assert zero_arity.subtype == :definition
       assert zero_arity.subject == "Parent.zero_arity/0"
-      assert "def zero_arity do" == extract(code, zero_arity.range)
+      assert "zero_arity" == extract(code, zero_arity.range)
+      assert "def zero_arity do\nend" == extract(code, zero_arity.block_range)
     end
 
     test "finds zero arity one line public functions (no parens)" do
@@ -42,7 +43,7 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
       assert zero_arity.type == :public_function
       assert zero_arity.subtype == :definition
       assert zero_arity.subject == "Parent.zero_arity/0"
-      assert "def zero_arity, do" == extract(code, zero_arity.range)
+      assert "zero_arity" == extract(code, zero_arity.range)
       assert "def zero_arity, do: true" == extract(code, zero_arity.block_range)
     end
 
@@ -59,7 +60,7 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
       assert zero_arity.type == :public_function
       assert zero_arity.subtype == :definition
       assert zero_arity.subject == "Parent.zero_arity/0"
-      assert "def zero_arity() do" == extract(code, zero_arity.range)
+      assert "zero_arity()" == extract(code, zero_arity.range)
       assert "def zero_arity() do\nend" == extract(code, zero_arity.block_range)
     end
 
@@ -77,10 +78,30 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
       assert one_arity.type == :public_function
       assert one_arity.subtype == :definition
       assert one_arity.subject == "Parent.one_arity/1"
-      assert "def one_arity(a) do" == extract(code, one_arity.range)
+      assert "one_arity(a)" == extract(code, one_arity.range)
       assert "def one_arity(a) do\na + 1\nend" == extract(code, one_arity.block_range)
     end
 
+    test "finds one arity public function with a guard" do
+      code =
+        ~q[
+          def one_arity(a) when is_integer(a) do
+            a + 1
+          end
+        ]
+        |> in_a_module()
+
+      {:ok, [one_arity], _} = index(code)
+
+      assert one_arity.type == :public_function
+      assert one_arity.subtype == :definition
+      assert one_arity.subject == "Parent.one_arity/1"
+      assert "one_arity(a) when is_integer(a)" == extract(code, one_arity.range)
+
+      assert "def one_arity(a) when is_integer(a) do\na + 1\nend" ==
+               extract(code, one_arity.block_range)
+    end
+
     test "finds multi arity public function" do
       code =
         ~q[
@@ -95,7 +116,7 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
       assert multi_arity.type == :public_function
       assert multi_arity.subtype == :definition
       assert multi_arity.subject == "Parent.multi_arity/4"
-      assert "def multi_arity(a, b, c, d) do" == extract(code, multi_arity.range)
+      assert "multi_arity(a, b, c, d)" == extract(code, multi_arity.range)
 
       assert "def multi_arity(a, b, c, d) do\n{a, b, c, d}\nend" ==
                extract(code, multi_arity.block_range)
@@ -116,10 +137,10 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
 
       expected =
         """
-        def multi_line(a,
+        multi_line(a,
         b,
         c,
-        d) do
+        d)
         """
         |> String.trim()
 
@@ -139,7 +160,7 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
         |> in_a_module()
 
       {:ok, [something], _} = index(code)
-      assert "def something(name) do" = extract(code, something.range)
+      assert "something(name)" = extract(code, something.range)
     end
 
     test "returns no references" do
@@ -153,7 +174,7 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
 
       assert function_definition.type == :public_function
       assert function_definition.subtype == :definition
-      assert "def my_fn(a, b) do" = extract(doc, function_definition.range)
+      assert "my_fn(a, b)" = extract(doc, function_definition.range)
     end
   end
 
@@ -170,7 +191,7 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
       assert zero_arity.type == :private_function
       assert zero_arity.subtype == :definition
       assert zero_arity.subject == "Parent.zero_arity/0"
-      assert "defp zero_arity, do" == extract(code, zero_arity.range)
+      assert "zero_arity" == extract(code, zero_arity.range)
       assert "defp zero_arity, do: true" == extract(code, zero_arity.block_range)
     end
 
@@ -186,7 +207,7 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
       assert zero_arity.type == :private_function
       assert zero_arity.subtype == :definition
       assert zero_arity.subject == "Parent.zero_arity/0"
-      assert "defp zero_arity(), do" == extract(code, zero_arity.range)
+      assert "zero_arity()" == extract(code, zero_arity.range)
       assert "defp zero_arity(), do: true" == extract(code, zero_arity.block_range)
     end
 
@@ -203,7 +224,7 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
       assert zero_arity.type == :private_function
       assert zero_arity.subtype == :definition
       assert zero_arity.subject == "Parent.zero_arity/0"
-      assert "defp zero_arity do" == extract(code, zero_arity.range)
+      assert "zero_arity" == extract(code, zero_arity.range)
       assert "defp zero_arity do\nend" == extract(code, zero_arity.block_range)
     end
 
@@ -220,7 +241,7 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
       assert zero_arity.type == :private_function
       assert zero_arity.subtype == :definition
       assert zero_arity.subject == "Parent.zero_arity/0"
-      assert "defp zero_arity() do" == extract(code, zero_arity.range)
+      assert "zero_arity()" == extract(code, zero_arity.range)
       assert "defp zero_arity() do\nend" == extract(code, zero_arity.block_range)
     end
 
@@ -236,7 +257,7 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
       assert one_arity.type == :private_function
       assert one_arity.subtype == :definition
       assert one_arity.subject == "Parent.one_arity/1"
-      assert "defp one_arity(a), do" == extract(code, one_arity.range)
+      assert "one_arity(a)" == extract(code, one_arity.range)
       assert "defp one_arity(a), do: a + 1" == extract(code, one_arity.block_range)
     end
 
@@ -254,7 +275,8 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
       assert one_arity.type == :private_function
       assert one_arity.subtype == :definition
       assert one_arity.subject == "Parent.one_arity/1"
-      assert "defp one_arity(a) do" == extract(code, one_arity.range)
+      assert "one_arity(a)" == extract(code, one_arity.range)
+      assert "defp one_arity(a) do\na + 1\nend" == extract(code, one_arity.block_range)
     end
 
     test "finds multi-arity one-line private functions" do
@@ -269,7 +291,7 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
       assert multi_arity.type == :private_function
       assert multi_arity.subtype == :definition
       assert multi_arity.subject == "Parent.multi_arity/3"
-      assert "defp multi_arity(a, b, c), do" == extract(code, multi_arity.range)
+      assert "multi_arity(a, b, c)" == extract(code, multi_arity.range)
       assert "defp multi_arity(a, b, c), do: {a, b, c}" = extract(code, multi_arity.block_range)
     end
 
@@ -287,7 +309,10 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
       assert multi_arity.type == :private_function
       assert multi_arity.subtype == :definition
       assert multi_arity.subject == "Parent.multi_arity/4"
-      assert "defp multi_arity(a, b, c, d) do" == extract(code, multi_arity.range)
+      assert "multi_arity(a, b, c, d)" == extract(code, multi_arity.range)
+
+      assert "defp multi_arity(a, b, c, d) do\n{a, b, c, d}\nend" ==
+               extract(code, multi_arity.block_range)
     end
 
     test "skips private functions defined in quote blocks" do
@@ -304,7 +329,25 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
         |> in_a_module()
 
       {:ok, [something], _} = index(code)
-      assert "defp something(name) do" = extract(code, something.range)
+      assert something.type == :private_function
+      assert "something(name)" = extract(code, something.range)
+    end
+
+    test "handles macro calls that define functions" do
+      {:ok, [definiton], doc} =
+        ~q[
+          quote do
+            def rpc_call(pid, call = %Call{method: unquote(method_name)}),
+                do: GenServer.unquote(genserver_method)(pid, call)
+          end
+        ]x
+        |> in_a_module()
+        |> index()
+
+      assert definiton.type == :public_function
+
+      assert decorate(doc, definiton.range) =~
+               "def «rpc_call(pid, call = %Call{method: unquote(method_name)})»"
     end
 
     test "returns no references" do
@@ -318,7 +361,7 @@ defmodule Lexical.RemoteControl.Search.Indexer.Extractors.FunctionDefinitionTest
 
       assert function_definition.type == :private_function
       assert function_definition.subtype == :definition
-      assert "defp my_fn(a, b) do" = extract(doc, function_definition.range)
+      assert "my_fn(a, b)" = extract(doc, function_definition.range)
     end
   end
 end