Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[#1337] Make goto variable definition respect list comprehension scopes #1403

Merged
merged 1 commit into from
Dec 21, 2023
Merged

[#1337] Make goto variable definition respect list comprehension scopes #1403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions apps/els_lsp/priv/code_navigation/src/variable_list_comp.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
-module(variable_list_comp).

one() ->
Var = 1,
[ Var || Var <- [1, 2, 3] ],
Var.

two() ->
[ Var || Var <- [1, 2, 3] ],
[ Var || Var <- [4, 5, 6] ].

three() ->
Var = 1,
[ Var || _ <- [1, 2, 3] ],
Var.

four() ->
[ {Var, Var2} || Var <- [4, 5, 6],
Var2 <- [ Var || Var <- [1, 2, 3] ]
].
60 changes: 55 additions & 5 deletions apps/els_lsp/src/els_code_navigation.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,13 +271,63 @@ maybe_imported(_Document, _Kind, _Data) ->
[].

-spec find_in_scope(uri(), els_poi:poi()) -> [els_poi:poi()].
find_in_scope(Uri, #{kind := variable, id := VarId, range := VarRange}) ->
find_in_scope(
Uri,
#{kind := variable, id := VarId, range := VarRange}
) ->
{ok, Document} = els_utils:lookup_document(Uri),
LcPOIs = els_poi:sort(els_dt_document:pois(Document, [list_comp])),
VarPOIs = els_poi:sort(els_dt_document:pois(Document, [variable])),
ScopeRange = els_scope:variable_scope_range(VarRange, Document),
[
MatchInScope = [
POI
|| #{range := Range, id := Id} = POI <- VarPOIs,
els_range:in(Range, ScopeRange),
|| #{id := Id} = POI <- pois_in(VarPOIs, ScopeRange),
Id =:= VarId
].
],
%% Handle special case if variable POI is inside list comprehension (LC)
MatchingLcPOIs = pois_contains(LcPOIs, VarRange),
case find_in_scope_list_comp(MatchingLcPOIs, MatchInScope) of
[] ->
MatchInScope -- find_in_scope_list_comp(LcPOIs, MatchInScope);
MatchInLc ->
MatchInLc
end.

-spec find_in_scope_list_comp([els_poi:poi()], [els_poi:poi()]) ->
[els_poi:poi()].
find_in_scope_list_comp([], _VarPOIs) ->
%% No match in LC, use regular scope
[];
find_in_scope_list_comp([LcPOI | LcPOIs], VarPOIs) ->
#{data := #{pattern_ranges := PatRanges}, range := LcRange} = LcPOI,
VarsInLc = pois_in(VarPOIs, LcRange),
{PatVars, OtherVars} =
lists:partition(
fun(#{range := Range}) ->
lists:any(
fun(PatRange) ->
els_range:in(Range, PatRange)
end,
PatRanges
)
end,
VarsInLc
),
case PatVars of
[] ->
%% Didn't find any patterned vars in this LC, try the next one
find_in_scope_list_comp(LcPOIs, VarPOIs);
_ ->
%% Put pattern vars first to make goto definition work
PatVars ++ OtherVars
end.

-spec pois_in([els_poi:poi()], els_poi:poi_range()) ->
[els_poi:poi()].
pois_in(POIs, Range) ->
[POI || #{range := R} = POI <- POIs, els_range:in(R, Range)].

-spec pois_contains([els_poi:poi()], els_poi:poi_range()) ->
[els_poi:poi()].
pois_contains(POIs, Range) ->
[POI || #{range := R} = POI <- POIs, els_range:in(Range, R)].
17 changes: 17 additions & 0 deletions apps/els_lsp/src/els_parser.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,8 @@ do_points_of_interest(Tree) ->
record_index_expr(Tree);
record_expr ->
record_expr(Tree);
list_comp ->
list_comp(Tree);
variable ->
variable(Tree);
atom ->
Expand Down Expand Up @@ -721,6 +723,21 @@ macro(Tree) ->
Anno = macro_location(Tree),
[poi(Anno, macro, macro_name(Tree))].

-spec list_comp(tree()) -> [els_poi:poi()].
list_comp(Tree) ->
Pos = erl_syntax:get_pos(Tree),
Body = erl_syntax:list_comp_body(Tree),
PatRanges = [
els_range:range(
erl_syntax:get_pos(
erl_syntax:generator_pattern(Gen)
)
)
|| Gen <- Body,
erl_syntax:type(Gen) =:= generator
],
[poi(Pos, list_comp, undefined, #{pattern_ranges => PatRanges})].

-spec map_record_def_fields(Fun, tree(), atom()) -> [Result] when
Fun :: fun((tree(), atom()) -> Result).
map_record_def_fields(Fun, Fields, RecordName) ->
Expand Down
92 changes: 70 additions & 22 deletions apps/els_lsp/test/els_definition_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
type_application_user/1,
type_export_entry/1,
variable/1,
variable_list_comp/1,
opaque_application_remote/1,
opaque_application_user/1,
parse_incomplete/1
Expand Down Expand Up @@ -623,38 +624,77 @@ type_export_entry(Config) ->
-spec variable(config()) -> ok.
variable(Config) ->
Uri = ?config(code_navigation_uri, Config),
Def0 = els_client:definition(Uri, 104, 9),
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}}),
Range0
{#{from => {103, 12}, to => {103, 15}}, Uri},
definition(Uri, 104, 9)
),
?assertEqual(
els_protocol:range(#{from => {104, 3}, to => {104, 6}}),
Range1
{#{from => {104, 3}, to => {104, 6}}, Uri},
definition(Uri, 105, 10)
),
?assertEqual(
els_protocol:range(#{from => {106, 12}, to => {106, 15}}),
Range2
{#{from => {106, 12}, to => {106, 15}}, Uri},
definition(Uri, 107, 10)
),
?assertEqual(
els_protocol:range(#{from => {106, 12}, to => {106, 15}}),
Range3
{#{from => {106, 12}, to => {106, 15}}, Uri},
definition(Uri, 108, 10)
),
%% Inside macro
?assertEqual(
els_protocol:range(#{from => {19, 17}, to => {19, 18}}),
Range4
{#{from => {19, 17}, to => {19, 18}}, Uri},
definition(Uri, 19, 36)
),
ok.

-spec variable_list_comp(config()) -> ok.
variable_list_comp(Config) ->
Uri = ?config(variable_list_comp_uri, Config),

%% one: Should skip LC
?assertEqual(
{#{from => {4, 5}, to => {4, 8}}, Uri},
definition(Uri, 6, 5)
),
%% one: Should go to LC generator pattern
?assertEqual(
{#{from => {5, 14}, to => {5, 17}}, Uri},
definition(Uri, 5, 7)
),
%% two: Should go to first LC generator pattern
?assertEqual(
{#{from => {9, 14}, to => {9, 17}}, Uri},
definition(Uri, 9, 7)
),
%% two: Should go to second LC generator pattern
?assertEqual(
{#{from => {10, 14}, to => {10, 17}}, Uri},
definition(Uri, 10, 7)
),
%% three: Should go to variable definition
?assertEqual(
{#{from => {13, 5}, to => {13, 8}}, Uri},
definition(Uri, 14, 7)
),
%% three: Should go to variable definition (same)
?assertEqual(
{#{from => {13, 5}, to => {13, 8}}, Uri},
definition(Uri, 15, 5)
),
%% four: Should go to first LC generator pattern
?assertEqual(
{#{from => {18, 22}, to => {18, 25}}, Uri},
definition(Uri, 18, 8)
),
%% four: Should go to second LC generator pattern
?assertEqual(
{#{from => {19, 22}, to => {19, 26}}, Uri},
definition(Uri, 18, 13)
),
%% four: Should go to third LC generator pattern
?assertEqual(
{#{from => {19, 39}, to => {19, 42}}, Uri},
definition(Uri, 19, 32)
),
ok.

Expand Down Expand Up @@ -716,3 +756,11 @@ parse_incomplete(Config) ->
els_client:definition(Uri, 19, 3)
),
ok.

definition(Uri, Line, Char) ->
case els_client:definition(Uri, Line, Char) of
#{result := [#{range := Range, uri := DefUri}]} ->
{els_range:to_poi_range(Range), DefUri};
Res ->
{error, Res}
end.
94 changes: 94 additions & 0 deletions apps/els_lsp/test/els_rename_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
rename_macro/1,
rename_module/1,
rename_variable/1,
rename_variable_list_comp/1,
rename_function/1,
rename_function_quoted_atom/1,
rename_type/1,
Expand Down Expand Up @@ -277,6 +278,99 @@ rename_variable(Config) ->
assert_changes(Expected10, Result10),
assert_changes(Expected11, Result11).

-spec rename_variable_list_comp(config()) -> ok.
rename_variable_list_comp(Config) ->
Uri = ?config(variable_list_comp_uri, Config),
UriAtom = binary_to_atom(Uri, utf8),
NewName = <<"NewAwesomeName">>,
%% one: Skip LC
Result1 = rename(Uri, 3, 4, NewName),
Result1 = rename(Uri, 5, 4, NewName),
Expected1 = #{
changes => #{
UriAtom => [
change(NewName, {3, 4}, {3, 7}),
change(NewName, {5, 4}, {5, 7})
]
}
},
%% one: Rename in LC only
Result2 = rename(Uri, 4, 6, NewName),
Result2 = rename(Uri, 4, 13, NewName),
Expected2 = #{
changes => #{
UriAtom => [
change(NewName, {4, 6}, {4, 9}),
change(NewName, {4, 13}, {4, 16})
]
}
},
%% two: Rename in first LC only
Result3 = rename(Uri, 8, 6, NewName),
Result3 = rename(Uri, 8, 13, NewName),
Expected3 = #{
changes => #{
UriAtom => [
change(NewName, {8, 6}, {8, 9}),
change(NewName, {8, 13}, {8, 16})
]
}
},
%% two: Rename in second LC only
Result4 = rename(Uri, 9, 6, NewName),
Result4 = rename(Uri, 9, 13, NewName),
Expected4 = #{
changes => #{
UriAtom => [
change(NewName, {9, 6}, {9, 9}),
change(NewName, {9, 13}, {9, 16})
]
}
},
%% three: Rename all Var (no Var in LC pattern)
Result5 = rename(Uri, 12, 4, NewName),
Result5 = rename(Uri, 13, 6, NewName),
Result5 = rename(Uri, 14, 4, NewName),
Expected5 = #{
changes => #{
UriAtom => [
change(NewName, {12, 4}, {12, 7}),
change(NewName, {13, 6}, {13, 9}),
change(NewName, {14, 4}, {14, 7})
]
}
},
%% four: Rename Var2, second LC generator pattern
Result6 = rename(Uri, 17, 12, NewName),
Result6 = rename(Uri, 18, 21, NewName),
Expected6 = #{
changes => #{
UriAtom => [
change(NewName, {17, 12}, {17, 16}),
change(NewName, {18, 21}, {18, 25})
]
}
},
%% four: FIXME: Bug, shouldn't rename Var inside second LC
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still at least one corner case to fix, leaving that for the future.

%% Result7 = rename(Uri, 17, 7, NewName),
%% Result7 = rename(Uri, 17, 21, NewName),
%% Expected7 = #{
%% changes => #{
%% UriAtom => [
%% change(NewName, {17, 7}, {17, 10}),
%% change(NewName, {17, 21}, {17, 24})
%% ]
%% }
%% },
assert_changes(Expected1, Result1),
assert_changes(Expected2, Result2),
assert_changes(Expected3, Result3),
assert_changes(Expected4, Result4),
assert_changes(Expected5, Result5),
assert_changes(Expected6, Result6),
%% assert_changes(Expected7, Result7),
ok.

-spec rename_macro(config()) -> ok.
rename_macro(Config) ->
Uri = ?config(rename_h_uri, Config),
Expand Down
Loading