Improve goto definition for variables (erlang-ls#1187)

* Use same scoping rules as variable renaming
* No longer limited to only variables inside functions
* Add support for finding variable references

apps/els_lsp/src/els_code_navigation.erl

@@ -8,7 +8,9 @@
 %%==============================================================================
 
 %% API
--export([ goto_definition/2 ]).
+-export([ goto_definition/2
+ , find_in_scope/2
+ ]).
 
 %%==============================================================================
 %% Includes
@@ -23,28 +25,13 @@
 -spec goto_definition(uri(), poi()) ->
  {ok, uri(), poi()} | {error, any()}.
 goto_definition( Uri
- , Var = #{kind := variable, id := VarId, range := VarRange}
+ , Var = #{kind := variable}
  ) ->
  %% This will naively try to find the definition of a variable by finding the
- %% first occurrence of the variable in the function clause.
- {ok, Document} = els_utils:lookup_document(Uri),
- FunPOIs = els_poi:sort(els_dt_document:pois(Document, [function_clause])),
- VarPOIs = els_poi:sort(els_dt_document:pois(Document, [variable])),
- %% Find the function clause we are in
- case [Range || #{range := Range} <- FunPOIs,
- els_range:compare(Range, VarRange)] of
- [] -> {error, not_in_function_clause};
- FunRanges ->
- FunRange = lists:last(FunRanges),
- %% Find the first occurrence of the variable in the function clause
- [POI|_] = [P || P = #{range := Range, id := Id} <- VarPOIs,
- els_range:compare(Range, VarRange),
- els_range:compare(FunRange, Range),
- Id =:= VarId],
- case POI of
- Var -> {error, already_at_definition};
- POI -> {ok, Uri, POI}
- end
+ %% first occurrence of the variable in variable scope.
+ case find_in_scope(Uri, Var) of
+ [Var|_] -> {error, already_at_definition};
+ [POI|_] -> {ok, Uri, POI}
  end;
 goto_definition( _Uri
  , #{ kind := Kind, id := {M, F, A} }
@@ -213,3 +200,12 @@ maybe_imported(Document, function, {F, A}) ->
  end;
 maybe_imported(_Document, _Kind, _Data) ->
  {error, not_found}.
+
+-spec find_in_scope(uri(), poi()) -> [poi()].
+find_in_scope(Uri, #{kind := variable, id := VarId, range := VarRange}) ->
+ {ok, Document} = els_utils:lookup_document(Uri),
+ VarPOIs = els_poi:sort(els_dt_document:pois(Document, [variable])),
+ ScopeRange = els_scope:variable_scope_range(VarRange, Document),
+ [POI || #{range := Range, id := Id} = POI <- VarPOIs,
+ els_range:in(Range, ScopeRange),
+ Id =:= VarId].

apps/els_lsp/src/els_references_provider.erl

@@ -67,6 +67,9 @@ find_references(Uri, #{ kind := Kind
  {M, F, A} -> {M, F, A}
  end,
  find_references_for_id(Kind, Key);
+find_references(Uri, #{kind := variable} = Var) ->
+ POIs = els_code_navigation:find_in_scope(Uri, Var),
+ [location(Uri, Range) || #{range := Range} = POI <- POIs, POI =/= Var];
 find_references(Uri, #{ kind := Kind
  , id := Id
  }) when Kind =:= function_clause ->

apps/els_lsp/src/els_rename_provider.erl

@@ -130,20 +130,9 @@ editable_range(#{kind := _Kind, range := Range}) ->
  els_protocol:range(Range).
 
 -spec changes(uri(), poi(), binary()) -> #{uri() => [text_edit()]} | null.
-changes(Uri, #{kind := variable, id := VarId, range := VarRange}, NewName) ->
- %% Rename variable in function clause scope
- case els_utils:lookup_document(Uri) of
- {ok, Document} ->
- FunRange = variable_scope_range(VarRange, Document),
- Changes = [#{range => editable_range(POI), newText => NewName} ||
- POI <- els_dt_document:pois(Document, [variable]),
- maps:get(id, POI) =:= VarId,
- els_range:in(maps:get(range, POI), FunRange)
- ],
- #{Uri => Changes};
- {error, _} ->
- null
- end;
+changes(Uri, #{kind := variable} = Var, NewName) ->
+ POIs = els_code_navigation:find_in_scope(Uri, Var),
+ #{Uri => [#{range => editable_range(P), newText => NewName} || P <- POIs]};
 changes(Uri, #{kind := type_definition, id := {Name, A}}, NewName) ->
  ?LOG_INFO("Renaming type ~p/~p to ~s", [Name, A, NewName]),
  {ok, Doc} = els_utils:lookup_document(Uri),
@@ -222,87 +211,6 @@ new_name(#{kind := record_expr}, NewName) ->
 new_name(_, NewName) ->
  NewName.
 
--spec variable_scope_range(poi_range(), els_dt_document:item()) -> poi_range().
-variable_scope_range(VarRange, Document) ->
- Attributes = [spec, callback, define, record, type_definition],
- AttrPOIs = els_dt_document:pois(Document, Attributes),
- case pois_match(AttrPOIs, VarRange) of
- [#{range := Range}] ->
- %% Inside attribute, simple.
- Range;
- [] ->
- %% If variable is not inside an attribute we need to figure out where the
- %% scope of the variable begins and ends.
- %% The scope of variables inside functions are limited by function clauses
- %% The scope of variables outside of function are limited by top-level
- %% POIs (attributes and functions) before and after.
- FunPOIs = els_poi:sort(els_dt_document:pois(Document, [function])),
- POIs = els_poi:sort(els_dt_document:pois(Document, [ function_clause
- | Attributes
- ])),
- CurrentFunRange = case pois_match(FunPOIs, VarRange) of
- [] -> undefined;
- [POI] -> range(POI)
- end,
- IsInsideFunction = CurrentFunRange /= undefined,
- BeforeFunRanges = [range(POI) || POI <- pois_before(FunPOIs, VarRange)],
- %% Find where scope should begin
- From =
- case [R || #{range := R} <- pois_before(POIs, VarRange)] of
- [] ->
- %% No POIs before
- {0, 0};
- [BeforeRange|_] when IsInsideFunction ->
- %% Inside function, use beginning of closest function clause
- maps:get(from, BeforeRange);
- [BeforeRange|_] when BeforeFunRanges == [] ->
- %% No function before, use end of closest POI
- maps:get(to, BeforeRange);
- [BeforeRange|_] ->
- %% Use end of closest POI, including functions.
- max(maps:get(to, hd(BeforeFunRanges)),
- maps:get(to, BeforeRange))
- end,
- %% Find when scope should end
- To =
- case [R || #{range := R} <- pois_after(POIs, VarRange)] of
- [] when IsInsideFunction ->
- %% No POIs after, use end of function
- maps:get(to, CurrentFunRange);
- [] ->
- %% No POIs after, use end of document
- {999999999, 999999999};
- [AfterRange|_] when IsInsideFunction ->
- %% Inside function, use closest of end of function *OR*
- %% beginning of the next function clause
- min(maps:get(to, CurrentFunRange), maps:get(from, AfterRange));
- [AfterRange|_] ->
- %% Use beginning of next POI
- maps:get(from, AfterRange)
- end,
- #{from => From, to => To}
- end.
-
--spec pois_before([poi()], poi_range()) -> [poi()].
-pois_before(POIs, VarRange) ->
- %% Reverse since we are typically interested in the last POI
- lists:reverse([POI || POI <- POIs, els_range:compare(range(POI), VarRange)]).
-
--spec pois_after([poi()], poi_range()) -> [poi()].
-pois_after(POIs, VarRange) ->
- [POI || POI <- POIs, els_range:compare(VarRange, range(POI))].
-
--spec pois_match([poi()], poi_range()) -> [poi()].
-pois_match(POIs, Range) ->
- [POI || POI <- POIs, els_range:in(Range, range(POI))].
-
--spec range(poi()) -> poi_range().
-range(#{kind := function, data := #{wrapping_range := Range}}) ->
- Range;
-range(#{range := Range}) ->
- Range.
-
-
 -spec convert_references_to_pois([els_dt_references:item()], [poi_kind()]) ->
  [{uri(), poi()}].
 convert_references_to_pois(Refs, Kinds) ->

apps/els_lsp/src/els_scope.erl

@@ -3,6 +3,7 @@
 
 -export([ local_and_included_pois/2
  , local_and_includer_pois/2
+ , variable_scope_range/2
  ]).
 
 -include("els_lsp.hrl").
@@ -68,3 +69,86 @@ find_includers(Uri) ->
 find_includers(Kind, Id) ->
  {ok, Items} = els_dt_references:find_by_id(Kind, Id),
  [Uri || #{uri := Uri} <- Items].
+
+%% @doc Find the rough scope of a variable, this is based on heuristics and
+%% won't always be correct.
+%% `VarRange' is expected to be the range of the variable.
+-spec variable_scope_range(poi_range(), els_dt_document:item()) -> poi_range().
+variable_scope_range(VarRange, Document) ->
+ Attributes = [spec, callback, define, record, type_definition],
+ AttrPOIs = els_dt_document:pois(Document, Attributes),
+ case pois_match(AttrPOIs, VarRange) of
+ [#{range := Range}] ->
+ %% Inside attribute, simple.
+ Range;
+ [] ->
+ %% If variable is not inside an attribute we need to figure out where the
+ %% scope of the variable begins and ends.
+ %% The scope of variables inside functions are limited by function clauses
+ %% The scope of variables outside of function are limited by top-level
+ %% POIs (attributes and functions) before and after.
+ FunPOIs = els_poi:sort(els_dt_document:pois(Document, [function])),
+ POIs = els_poi:sort(els_dt_document:pois(Document, [ function_clause
+ | Attributes
+ ])),
+ CurrentFunRange = case pois_match(FunPOIs, VarRange) of
+ [] -> undefined;
+ [POI] -> range(POI)
+ end,
+ IsInsideFunction = CurrentFunRange /= undefined,
+ BeforeFunRanges = [range(POI) || POI <- pois_before(FunPOIs, VarRange)],
+ %% Find where scope should begin
+ From =
+ case [R || #{range := R} <- pois_before(POIs, VarRange)] of
+ [] ->
+ %% No POIs before
+ {0, 0};
+ [BeforeRange|_] when IsInsideFunction ->
+ %% Inside function, use beginning of closest function clause
+ maps:get(from, BeforeRange);
+ [BeforeRange|_] when BeforeFunRanges == [] ->
+ %% No function before, use end of closest POI
+ maps:get(to, BeforeRange);
+ [BeforeRange|_] ->
+ %% Use end of closest POI, including functions.
+ max(maps:get(to, hd(BeforeFunRanges)),
+ maps:get(to, BeforeRange))
+ end,
+ %% Find when scope should end
+ To =
+ case [R || #{range := R} <- pois_after(POIs, VarRange)] of
+ [] when IsInsideFunction ->
+ %% No POIs after, use end of function
+ maps:get(to, CurrentFunRange);
+ [] ->
+ %% No POIs after, use end of document
+ {999999999, 999999999};
+ [AfterRange|_] when IsInsideFunction ->
+ %% Inside function, use closest of end of function *OR*
+ %% beginning of the next function clause
+ min(maps:get(to, CurrentFunRange), maps:get(from, AfterRange));
+ [AfterRange|_] ->
+ %% Use beginning of next POI
+ maps:get(from, AfterRange)
+ end,
+ #{from => From, to => To}
+ end.
+
+-spec pois_before([poi()], poi_range()) -> [poi()].
+pois_before(POIs, VarRange) ->
+ %% Reverse since we are typically interested in the last POI
+ lists:reverse([POI || POI <- POIs, els_range:compare(range(POI), VarRange)]).
+
+-spec pois_after([poi()], poi_range()) -> [poi()].
+pois_after(POIs, VarRange) ->
+ [POI || POI <- POIs, els_range:compare(VarRange, range(POI))].
+
+-spec pois_match([poi()], poi_range()) -> [poi()].
+pois_match(POIs, Range) ->
+ [POI || POI <- POIs, els_range:in(Range, range(POI))].
+
+-spec range(poi()) -> poi_range().
+range(#{kind := function, data := #{wrapping_range := Range}}) ->
+ Range;
+range(#{range := Range}) ->
+ Range.

apps/els_lsp/test/els_definition_SUITE.erl

@@ -463,10 +463,12 @@ variable(Config) ->
  Def1 = els_client:definition(Uri, 105, 10),
  Def2 = els_client:definition(Uri, 107, 10),
  Def3 = els_client:definition(Uri, 108, 10),
+ Def4 = els_client:definition(Uri, 19, 36),
  #{result := #{range := Range0, uri := DefUri0}} = Def0,
  #{result := #{range := Range1, uri := DefUri0}} = Def1,
  #{result := #{range := Range2, uri := DefUri0}} = Def2,
  #{result := #{range := Range3, uri := DefUri0}} = Def3,
+ #{result := #{range := Range4, uri := DefUri0}} = Def4,
 
  ?assertEqual(?config(code_navigation_uri, Config), DefUri0),
  ?assertEqual( els_protocol:range(#{from => {103, 12}, to => {103, 15}})
@@ -477,6 +479,9 @@ variable(Config) ->
  , Range2),
  ?assertEqual( els_protocol:range(#{from => {106, 12}, to => {106, 15}})
  , Range3),
+ %% Inside macro
+ ?assertEqual( els_protocol:range(#{from => {19, 17}, to => {19, 18}})
+ , Range4),
  ok.
 
 

apps/els_lsp/test/els_rename_SUITE.erl

@@ -479,7 +479,7 @@ assert_changes(#{ changes := ExpectedChanges }, #{ changes := Changes }) ->
  lists:sort(maps:to_list(ExpectedChanges))),
  [ begin
  ?assertEqual(ExpectedKey, Key),
- ?assertEqual(Expected, Change)
+ ?assertEqual(lists:sort(Expected), lists:sort(Change))
  end
  || {{Key, Change}, {ExpectedKey, Expected}} <- Pairs
  ],