Merge pull request #1023 from keynslug/fix/scope-include-loop

Prevent infinite recursion when enumerating document POIs

apps/els_lsp/priv/code_navigation/include/transitive.hrl

@@ -1 +1,7 @@
+-ifndef(__TRANSITIVE_H__).
+-define(__TRANSITIVE_H__, included).
+
+-include("transitive.hrl").
 -include("definition.hrl").
+
+-endif.

apps/els_lsp/src/els_diagnostics_utils.erl

@@ -8,7 +8,9 @@
 %%==============================================================================
 -export([ dependencies/1
         , included_uris/1
+        , included_documents/1
         , range/2
+        , traverse_include_graph/3
         ]).
 %%==============================================================================
 %% Includes
@@ -25,6 +27,20 @@ included_uris(Document) ->
   POIs = els_dt_document:pois(Document, [include, include_lib]),
   included_uris([Id || #{id := Id} <- POIs], []).
 
+-spec included_documents(els_dt_document:item()) -> [els_dt_document:item()].
+included_documents(Document) ->
+  lists:filtermap(fun find_included_document/1, included_uris(Document)).
+
+-spec find_included_document(uri()) -> {true, els_dt_document:item()} | false.
+find_included_document(Uri) ->
+  case els_utils:lookup_document(Uri) of
+    {ok, IncludeDocument} ->
+      {true, IncludeDocument};
+    {error, _} ->
+      ?LOG_WARNING("Failed included document lookup [uri=~p]", [Uri]),
+      false
+  end.
+
 -spec range(els_dt_document:item() | undefined,
             erl_anno:anno() | none) -> poi_range().
 range(Document, none) ->
@@ -60,6 +76,17 @@ range(Document, Anno) ->
       end
   end.
 
+-spec traverse_include_graph(AccFun, AccT, From) -> AccT when
+    AccFun :: fun((Included, Includer, AccT) -> AccT),
+    From :: els_dt_document:item(),
+    Included :: els_dt_document:item(),
+    Includer :: els_dt_document:item().
+traverse_include_graph(AccFun, Acc, From) ->
+    Graph = els_fungraph:new(
+      fun els_dt_document:uri/1,
+      fun included_documents/1),
+    els_fungraph:traverse(AccFun, Acc, From, Graph).
+
 %%==============================================================================
 %% Internal Functions
 %%==============================================================================

apps/els_lsp/src/els_dt_document.erl

@@ -25,6 +25,7 @@
         , pois/1
         , pois/2
         , get_element_at_pos/3
+        , uri/1
         ]).
 
 %%==============================================================================
@@ -162,3 +163,8 @@ get_element_at_pos(Item, Line, Column) ->
   POIs = pois(Item),
   MatchedPOIs = els_poi:match_pos(POIs, {Line, Column}),
   els_poi:sort(MatchedPOIs).
+
+%% @doc Returns the URI of the current document
+-spec uri(item()) -> uri().
+uri(#{ uri := Uri }) ->
+  Uri.

apps/els_lsp/src/els_fungraph.erl

@@ -0,0 +1,45 @@
+-module(els_fungraph).
+
+-type id_fun(NodeT) :: fun((NodeT) -> _NodeID).
+-type edges_fun(NodeT) :: fun((NodeT) -> list(NodeT)).
+
+-opaque graph(NodeT) :: {?MODULE, id_fun(NodeT), edges_fun(NodeT)}.
+
+-export_type([graph/1]).
+
+-export([new/2]).
+-export([traverse/4]).
+
+-spec new(id_fun(NodeT), edges_fun(NodeT)) -> graph(NodeT).
+new(IdFun, EdgesFun) ->
+  {?MODULE, IdFun, EdgesFun}.
+
+-type acc_fun(NodeT, AccT) ::
+  fun((_Node :: NodeT, _From :: NodeT, AccT) -> AccT).
+
+-spec traverse(acc_fun(NodeT, AccT), AccT, NodeT, graph(NodeT)) -> AccT.
+traverse(AccFun, Acc, From, G) ->
+  traverse(AccFun, Acc, [From], sets:new(), G).
+
+-spec traverse(acc_fun(NodeT, AccT), AccT, [NodeT], sets:set(), graph(NodeT)) ->
+  AccT.
+traverse(AccFun, Acc, [Node | Rest], Visited, G = {?MODULE, IdFun, EdgesFun}) ->
+  {AdjacentNodes, VisitedNext} = lists:foldr(
+    fun(Adjacent, {NodesAcc, VisitedAcc}) ->
+      ID = IdFun(Adjacent),
+      case sets:is_element(ID, VisitedAcc) of
+        false -> {[Adjacent | NodesAcc], sets:add_element(ID, VisitedAcc)};
+        true -> {NodesAcc, VisitedAcc}
+      end
+    end,
+    {[], Visited},
+    EdgesFun(Node)
+  ),
+  AccNext = lists:foldl(
+    fun(Adjacent, AccIn) -> AccFun(Adjacent, Node, AccIn) end,
+    Acc,
+    AdjacentNodes
+  ),
+  traverse(AccFun, AccNext, AdjacentNodes ++ Rest, VisitedNext, G);
+traverse(_AccFun, Acc, [], _Visited, _G) ->
+  Acc.

apps/els_lsp/src/els_scope.erl

@@ -18,23 +18,14 @@ local_and_included_pois(Document, Kinds) ->
                 ]).
 
 %% @doc Return POIs of the provided `Kinds' in included files from `Document'
--spec included_pois(els_dt_document:item(), [poi_kind()]) -> [[map()]].
+-spec included_pois(els_dt_document:item(), [poi_kind()]) -> [poi()].
 included_pois(Document, Kinds) ->
-  POIs  = els_dt_document:pois(Document, [include, include_lib]),
-  [include_file_pois(Name, Kinds) || #{id := Name} <- POIs].
-
-%% @doc Return POIs of the provided `Kinds' in the included file
--spec include_file_pois(string(), [poi_kind()]) -> [map()].
-include_file_pois(Name, Kinds) ->
-  case els_utils:find_header(els_utils:filename_to_atom(Name)) of
-    {ok, Uri} ->
-      {ok, IncludeDocument} = els_utils:lookup_document(Uri),
-      %% NB: Recursive call to support includes in the include file
-      IncludedInHeader = lists:flatten(included_pois(IncludeDocument, Kinds)),
-      els_dt_document:pois(IncludeDocument, Kinds) ++ IncludedInHeader;
-    {error, _} ->
-      []
-  end.
+  els_diagnostics_utils:traverse_include_graph(
+    fun(IncludedDocument, _Includer, Acc) ->
+      els_dt_document:pois(IncludedDocument, Kinds) ++ Acc
+    end,
+    [],
+    Document).
 
 %% @doc Return POIs of the provided `Kinds' in the local document and files that
 %% (maybe recursively) include it

apps/els_lsp/src/els_unused_includes_diagnostics.erl

@@ -20,7 +20,6 @@
 %% Includes
 %%==============================================================================
 -include("els_lsp.hrl").
--include_lib("kernel/include/logger.hrl").
 
 %%==============================================================================
 %% Callback Functions
@@ -55,7 +54,7 @@ source() ->
 %%==============================================================================
 -spec find_unused_includes(els_dt_document:item(), [poi()]) -> [uri()].
 find_unused_includes(#{uri := Uri} = Document, Includes) ->
-  Graph = expand_includes(Uri),
+  Graph = expand_includes(Document),
   POIs = els_dt_document:pois(Document),
   IncludedUris = els_diagnostics_utils:included_uris(Document),
   Fun = fun(POI, Acc) ->
@@ -68,6 +67,8 @@ find_unused_includes(#{uri := Uri} = Document, Includes) ->
 -spec update_unused(digraph:graph(), uri(), poi(), [uri()]) -> [uri()].
 update_unused(Graph, Uri, POI, Acc) ->
   case els_code_navigation:goto_definition(Uri, POI) of
+    {ok, Uri, _DefinitionPOI} ->
+      Acc;
     {ok, DefinitionUri, _DefinitionPOI} ->
       case digraph:get_path(Graph, DefinitionUri, Uri) of
         false ->
@@ -79,34 +80,16 @@ update_unused(Graph, Uri, POI, Acc) ->
       Acc
   end.
 
--spec expand_includes(uri()) -> digraph:graph().
-expand_includes(Uri) ->
-  expand_includes([Uri], digraph:new(), sets:new()).
-
--spec expand_includes([uri()], digraph:graph(), sets:set()) -> digraph:graph().
-expand_includes([], Graph, _Visited) ->
-  Graph;
-expand_includes([Uri|Uris], Graph, Visited) ->
-  case els_utils:lookup_document(Uri) of
-    {ok, Document} ->
-      IncludedUris = els_diagnostics_utils:included_uris(Document),
-      NonVisitedIncludedUris = [U || U <- IncludedUris
-                                       , not sets:is_element(U, Visited)],
-      Dest = digraph:add_vertex(Graph, Uri),
-      NewGraph = lists:foldl(fun(U, G) ->
-                                 Src = digraph:add_vertex(G, U),
-                                 digraph:add_edge(G, Src, Dest),
-                                 G
-                             end
-                            , Graph
-                            , IncludedUris),
-      expand_includes( Uris ++ NonVisitedIncludedUris
-                     , NewGraph
-                     , sets:add_element(Uri, Visited));
-    {error, _Error} ->
-      ?LOG_WARNING("Failed lookup while expanding includes [uri=~p]", [Uri]),
-      []
-  end.
+-spec expand_includes(els_dt_document:item()) -> digraph:graph().
+expand_includes(Document) ->
+  DG = digraph:new(),
+  AccFun = fun(#{uri := IncludedUri}, #{uri := IncluderUri}, _) ->
+    Dest = digraph:add_vertex(DG, IncluderUri),
+    Src = digraph:add_vertex(DG, IncludedUri),
+    _IncludedBy = digraph:add_edge(DG, Src, Dest),
+    DG
+  end,
+  els_diagnostics_utils:traverse_include_graph(AccFun, DG, Document).
 
 -spec inclusion_range(uri(), els_dt_document:item()) -> poi_range().
 inclusion_range(Uri, Document) ->

apps/els_lsp/test/els_fungraph_SUITE.erl

@@ -0,0 +1,31 @@
+-module(els_fungraph_SUITE).
+
+-include_lib("eunit/include/eunit.hrl").
+
+-export([all/0]).
+
+-export([dense_graph_traversal/1]).
+
+-spec all() -> [atom()].
+all() ->
+  [dense_graph_traversal].
+
+-spec dense_graph_traversal(_Config) -> ok.
+dense_graph_traversal(_) ->
+  MaxID = 40,
+  % Visited nodes must be unique
+  ?assertEqual(
+    lists:reverse(lists:seq(1, MaxID)),
+    els_fungraph:traverse(
+      fun(ID, _From, Acc) -> [ID | Acc] end,
+      [],
+      1,
+      els_fungraph:new(
+        fun(ID) -> ID end,
+        fun(ID) ->
+          % Each node has edges to nodes in range [ID; ID * 2]
+          [NextID || NextID <- lists:seq(ID, ID * 2), NextID =< MaxID]
+        end
+      )
+    )
+  ).