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

Fix issues with rename variable involving specs #1183

Merged
merged 6 commits into from
Jan 30, 2022
Merged

Fix issues with rename variable involving specs #1183

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2ba13ae
Fix issues with rename variable involving specs
plux Jan 26, 2022
61c8d77
Support more attributes
plux Jan 27, 2022
d25371f
Support more attributes
plux Jan 27, 2022
7f0a45e
WIP
plux Jan 27, 2022
a4fbb9e
Clean up
plux Jan 28, 2022
dbb52a6
Add specs
plux Jan 28, 2022
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
5 changes: 5 additions & 0 deletions apps/els_lsp/priv/code_navigation/src/rename_variable.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,8 @@ foo(_Var) ->

bar(Var) ->
Var.

-spec baz(Var) -> Var
when Var :: atom().
baz(Var) ->
Var.
35 changes: 23 additions & 12 deletions apps/els_lsp/src/els_rename_provider.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,18 +222,29 @@ new_name(_, NewName) ->

-spec function_clause_range(poi_range(), els_dt_document:item()) -> poi_range().
function_clause_range(VarRange, Document) ->
FunPOIs = els_poi:sort(els_dt_document:pois(Document, [function_clause])),
%% Find beginning of first function clause before VarRange
From = case [R || #{range := R} <- FunPOIs, els_range:compare(R, VarRange)] of
[] -> {0, 0}; % Beginning of document
FunRanges -> maps:get(from, lists:last(FunRanges))
end,
%% Find beginning of first function clause after VarRange
To = case [R || #{range := R} <- FunPOIs, els_range:compare(VarRange, R)] of
[] -> {999999999, 999999999}; % End of document
[#{from := End}|_] -> End
end,
#{from => From, to => To}.
POIs = els_poi:sort(els_dt_document:pois(Document, [ function_clause
, spec
])),
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably consider all possible top-level forms. I haven't checked, but I'd imagine similar issue will happen with things like:

foo(A) -> A.
-define(?MACRO(A), A).

and other top-level items (like -callback and -type).

TBH, I'm not sure why the logic tries to look for the beginning point of the next form. Wouldn't it be enough to look for the end point of the current clause?

Copy link
Member

@robertoaloi robertoaloi Jan 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If I remember correctly, it's to ensure that:

  • renaming is applied to all clauses of the functions (one may argue this is not always desirable)
  • it works even when the current function is not fully parsable (still being written).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points, I will add support for other top-level forms as well.
The reason for the currently implemented logic is that function_clause ranges don't contain the beginning and the end of the body of the clause, but rather just the function clause head.

SpecPOIs = els_poi:sort(els_dt_document:pois(Document, [spec])),
case [R || #{range := R} <- SpecPOIs, els_range:in(VarRange, R)] of
[SpecRange] ->
%% Renaming variable in spec
SpecRange;
[] ->
%% Find beginning of first function clause before VarRange
From =
case [R || #{range := R} <- POIs, els_range:compare(R, VarRange)] of
[] -> {0, 0}; % Beginning of document
FunRanges -> maps:get(from, lists:last(FunRanges))
end,
%% Find beginning of first function clause after VarRange
To =
Copy link
Contributor

Choose a reason for hiding this comment

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

just an idea: the function POI has a wrapping_range data field (for folding ranges). The end of the wrapping range could be used as an upper bound if the next function clause is outside of wrapping range. (There could be variables in other forms than spec, like type or macro definitions)

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a better idea is for els_parser to store the full range of a function clause in the function_clause POI data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I will see if I can implement an upper bound using the function wrapping_range.
If els_parser could return the full range of the function_clause that would make the implementation of this functionality cleaner, not sure how tricky it is to implement in els_parser..

case [R || #{range := R} <- POIs, els_range:compare(VarRange, R)] of
[] -> {999999999, 999999999}; % End of document
[#{from := End}|_] -> End
end,
#{from => From, to => To}
end.

-spec convert_references_to_pois([els_dt_references:item()], [poi_kind()]) ->
[{uri(), poi()}].
Expand Down
8 changes: 7 additions & 1 deletion 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 @@ -135,10 +135,16 @@ rename_variable(Config) ->
Expected4 = #{changes => #{UriAtom => [ change(NewName, {11, 2}, {11, 5})
, change(NewName, {10, 4}, {10, 7})
]}},
#{result := Result5} = els_client:document_rename(Uri, 13, 10, NewName),
Expected5 = #{changes => #{UriAtom => [ change(NewName, {14, 15}, {14, 18})
, change(NewName, {13, 18}, {13, 21})
, change(NewName, {13, 10}, {13, 13})
]}},
assert_changes(Expected1, Result1),
assert_changes(Expected2, Result2),
assert_changes(Expected3, Result3),
assert_changes(Expected4, Result4).
assert_changes(Expected4, Result4),
assert_changes(Expected5, Result5).

-spec rename_macro(config()) -> ok.
rename_macro(Config) ->
Expand Down