fix: handle missing metadata in indexer extractors (#390)

Fixes #379 

The indexer was crashing because it encountered nodes with missing
location metadata. I'm not sure what code exactly triggered that, I
suspect it might be broken code so I added tests for that.

I also looked at other places in the indexer where we would crash on
missing metadata and added error handling there too.

---------

Co-authored-by: Mitchell Hanberg <mitch@mitchellhanberg.com>

Author: doorgan

apps/engine/lib/engine/search/indexer/extractors/ex_unit.ex

@@ -9,8 +9,6 @@ defmodule Engine.Search.Indexer.Extractors.ExUnit do
   alias Forge.Formats
   alias Forge.Search.Indexer.Entry
 
-  require Logger
-
   # setup block i.e. setup do... or setup arg do...
   def extract({setup_fn, _, args} = setup, %Reducer{} = reducer)
       when setup_fn in [:setup, :setup_all] and length(args) > 0 do
@@ -19,16 +17,10 @@ defmodule Engine.Search.Indexer.Extractors.ExUnit do
     subject = Formats.mfa(module, setup_fn, arity)
     setup_type = :"ex_unit_#{setup_fn}"
 
-    entry =
-      case Metadata.location(setup) do
-        {:block, _, _, _} ->
-          block_entry(reducer, setup, setup_type, subject)
-
-        {:expression, _} ->
-          expression_entry(reducer, setup, setup_type, subject)
-      end
-
-    {:ok, entry}
+    case Metadata.location(setup) do
+      {:block, _, _, _} -> block_entry(reducer, setup, setup_type, subject)
+      {:expression, _} -> expression_entry(reducer, setup, setup_type, subject)
+    end
   end
 
   # Test block test "test name" do ... or test "test name", arg do
@@ -39,18 +31,10 @@ defmodule Engine.Search.Indexer.Extractors.ExUnit do
     module_name = Formats.module(module)
     subject = "#{module_name}.[\"#{test_name}\"]/#{arity}"
 
-    entry =
-      case Metadata.location(test) do
-        {:block, _, _, _} ->
-          # a test with a body
-          block_entry(reducer, test, :ex_unit_test, subject)
-
-        {:expression, _} ->
-          # a pending test
-          expression_entry(reducer, test, :ex_unit_test, subject)
-      end
-
-    {:ok, entry}
+    case Metadata.location(test) do
+      {:block, _, _, _} -> block_entry(reducer, test, :ex_unit_test, subject)
+      {:expression, _} -> expression_entry(reducer, test, :ex_unit_test, subject)
+    end
   end
 
   # describe blocks
@@ -60,9 +44,7 @@ defmodule Engine.Search.Indexer.Extractors.ExUnit do
     module_name = Formats.module(module)
     subject = "#{module_name}[\"#{describe_name}\"]/#{arity}"
 
-    entry = block_entry(reducer, test, :ex_unit_describe, subject)
-
-    {:ok, entry}
+    block_entry(reducer, test, :ex_unit_describe, subject)
   end
 
   def extract(_ign, _) do
@@ -75,9 +57,11 @@ defmodule Engine.Search.Indexer.Extractors.ExUnit do
 
     {:ok, module} = Analyzer.current_module(reducer.analysis, Reducer.position(reducer))
     app = Application.get_application(module)
-    detail_range = detail_range(reducer.analysis, ast)
 
-    Entry.definition(path, block, subject, type, detail_range, app)
+    case detail_range(reducer.analysis, ast) do
+      nil -> :ignored
+      range -> {:ok, Entry.definition(path, block, subject, type, range, app)}
+    end
   end
 
   defp block_entry(%Reducer{} = reducer, ast, type, subject) do
@@ -86,9 +70,23 @@ defmodule Engine.Search.Indexer.Extractors.ExUnit do
 
     {:ok, module} = Analyzer.current_module(reducer.analysis, Reducer.position(reducer))
     app = Application.get_application(module)
-    detail_range = detail_range(reducer.analysis, ast)
-    block_range = block_range(reducer.analysis, ast)
-    Entry.block_definition(path, block, subject, type, block_range, detail_range, app)
+
+    case detail_range(reducer.analysis, ast) do
+      nil ->
+        :ignored
+
+      detail_range ->
+        {:ok,
+         Entry.block_definition(
+           path,
+           block,
+           subject,
+           type,
+           block_range(reducer.analysis, ast),
+           detail_range,
+           app
+         )}
+    end
   end
 
   defp block_range(%Analysis{} = analysis, ast) do
@@ -113,6 +111,9 @@ defmodule Engine.Search.Indexer.Extractors.ExUnit do
           Position.new(analysis.document, start_line, start_column),
           Position.new(analysis.document, end_line, end_column)
         )
+
+      _ ->
+        nil
     end
   end
 

apps/engine/lib/engine/search/indexer/extractors/function_reference.ex

@@ -25,8 +25,9 @@ defmodule Engine.Search.Indexer.Extractors.FunctionReference do
         %Reducer{} = reducer
       )
       when is_list(arg_list) and is_atom(function_name) do
-    entry = entry(reducer, apply_meta, apply_meta, module, function_name, arg_list)
-    {:ok, entry, nil}
+    reducer
+    |> entry(apply_meta, apply_meta, module, function_name, arg_list)
+    |> without_further_analysis()
   end
 
   # Dynamic call via Kernel.apply Kernel.apply(Module, :function, [1, 2])
@@ -40,8 +41,9 @@ defmodule Engine.Search.Indexer.Extractors.FunctionReference do
         %Reducer{} = reducer
       )
       when is_list(arg_list) and is_atom(function_name) do
-    entry = entry(reducer, start_metadata, apply_meta, module, function_name, arg_list)
-    {:ok, entry, nil}
+    reducer
+    |> entry(start_metadata, apply_meta, module, function_name, arg_list)
+    |> without_further_analysis()
   end
 
   # remote function OtherModule.foo(:arg), OtherModule.foo() or OtherModule.foo
@@ -50,9 +52,7 @@ defmodule Engine.Search.Indexer.Extractors.FunctionReference do
         %Reducer{} = reducer
       )
       when is_atom(fn_name) do
-    entry = entry(reducer, start_metadata, end_metadata, module, fn_name, args)
-
-    {:ok, entry}
+    entry(reducer, start_metadata, end_metadata, module, fn_name, args)
   end
 
   # local function capture &downcase/1
@@ -65,8 +65,9 @@ defmodule Engine.Search.Indexer.Extractors.FunctionReference do
     {module, _, _} =
       Engine.Analyzer.resolve_local_call(reducer.analysis, position, fn_name, arity)
 
-    entry = entry(reducer, end_metadata, arity_meta, module, fn_name, arity)
-    {:ok, entry, nil}
+    reducer
+    |> entry(end_metadata, arity_meta, module, fn_name, arity)
+    |> without_further_analysis()
   end
 
   # Function capture with arity: &OtherModule.foo/3
@@ -81,13 +82,13 @@ defmodule Engine.Search.Indexer.Extractors.FunctionReference do
          ]},
         %Reducer{} = reducer
       ) do
-    entry = entry(reducer, start_metadata, end_metadata, module, function_name, arity)
-
+    reducer
+    |> entry(start_metadata, end_metadata, module, function_name, arity)
     # we return nil here to stop analysis from progressing down the syntax tree,
     # because if it did, the function head that deals with normal calls will pick
     # up the rest of the call and return a reference to MyModule.function/0, which
     # is incorrect
-    {:ok, entry, nil}
+    |> without_further_analysis()
   end
 
   def extract({:|>, pipe_meta, [pipe_start, {fn_name, meta, args}]}, %Reducer{}) do
@@ -134,16 +135,17 @@ defmodule Engine.Search.Indexer.Extractors.FunctionReference do
       {module, _, _} =
         Engine.Analyzer.resolve_local_call(reducer.analysis, position, fn_name, arity)
 
-      entry = entry(reducer, meta, meta, [module], fn_name, args)
-
-      {:ok, entry}
+      entry(reducer, meta, meta, [module], fn_name, args)
     end
   end
 
   def extract(_ast, _reducer) do
     :ignored
   end
 
+  defp without_further_analysis(:ignored), do: :ignored
+  defp without_further_analysis({:ok, entry}), do: {:ok, entry, nil}
+
   defp entry(
          %Reducer{} = reducer,
          start_metadata,
@@ -156,64 +158,81 @@ defmodule Engine.Search.Indexer.Extractors.FunctionReference do
     block = Reducer.current_block(reducer)
 
     range =
-      get_reference_range(reducer.analysis.document, start_metadata, end_metadata, function_name)
-
-    case Engine.Analyzer.expand_alias(module, reducer.analysis, range.start) do
-      {:ok, module} ->
-        mfa = Subject.mfa(module, function_name, arity)
+      get_reference_range(
+        reducer.analysis.document,
+        start_metadata,
+        end_metadata,
+        function_name
+      )
 
-        Entry.reference(
-          reducer.analysis.document.path,
-          block,
-          mfa,
-          {:function, :usage},
-          range,
-          Application.get_application(module)
-        )
+    case range do
+      nil ->
+        :ignored
 
       _ ->
-        human_location = Reducer.human_location(reducer)
-
-        Logger.warning(
-          "Could not expand #{inspect(module)} into an alias. Please report this. (at #{human_location})"
-        )
-
-        nil
+        case Engine.Analyzer.expand_alias(module, reducer.analysis, range.start) do
+          {:ok, module} ->
+            mfa = Subject.mfa(module, function_name, arity)
+
+            {:ok,
+             Entry.reference(
+               reducer.analysis.document.path,
+               block,
+               mfa,
+               {:function, :usage},
+               range,
+               Application.get_application(module)
+             )}
+
+          _ ->
+            human_location = Reducer.human_location(reducer)
+
+            Logger.warning(
+              "Could not expand #{inspect(module)} into an alias (at #{human_location}). Please open an issue!"
+            )
+
+            :ignored
+        end
     end
   end
 
   defp get_reference_range(document, start_metadata, end_metadata, function_name) do
-    {start_line, start_column} = start_position(start_metadata)
-    start_position = Position.new(document, start_line, start_column)
-    has_parens? = not Keyword.get(end_metadata, :no_parens, false)
-
-    {end_line, end_column} =
-      case Metadata.position(end_metadata, :closing) do
-        {line, column} ->
-          if has_parens? do
-            {line, column + 1}
-          else
-            {line, column}
-          end
-
-        nil ->
-          {line, column} = Metadata.position(end_metadata)
-
-          if has_parens? do
-            {line, column + 1}
-          else
-            name_length = function_name |> Atom.to_string() |> String.length()
-            # without parens, the metadata points to the beginning of the call, so
-            # we need to add the length of the function name to be sure we have it
-            # all
-            {line, column + name_length}
-          end
-      end
+    if valid_position_metadata?(start_metadata) and valid_position_metadata?(end_metadata) do
+      {start_line, start_column} = start_position(start_metadata)
+      start_pos = Position.new(document, start_line, start_column)
+      has_parens? = not Keyword.get(end_metadata, :no_parens, false)
+
+      {end_line, end_column} =
+        case Metadata.position(end_metadata, :closing) do
+          {line, column} when has_parens? -> {line, column + 1}
+          {line, column} -> {line, column}
+          nil -> adjust_position_for_name(end_metadata, function_name, has_parens?)
+        end
+
+      end_pos = Position.new(document, end_line, end_column)
+      Range.new(start_pos, end_pos)
+    else
+      nil
+    end
+  end
 
-    end_position = Position.new(document, end_line, end_column)
-    Range.new(start_position, end_position)
+  defp adjust_position_for_name(metadata, function_name, has_parens?) do
+    {line, column} = Metadata.position(metadata)
+
+    if has_parens? do
+      {line, column + 1}
+    else
+      name_length = function_name |> Atom.to_string() |> String.length()
+      {line, column + name_length}
+    end
+  end
+
+  defp valid_position_metadata?(metadata) when is_list(metadata) do
+    Keyword.has_key?(metadata, :line) and Keyword.has_key?(metadata, :column)
   end
 
+  defp valid_position_metadata?(_), do: false
+
   defp start_position(metadata) do
     Metadata.position(metadata)
   end