Fix issues with rename variable involving specs #1183
Conversation
plux
force-pushed
the
fix-rename-variable-spec
branch
from
c61110d
to
2ba13ae
Compare
| POIs = els_poi:sort(els_dt_document:pois(Document, [ function_clause | ||
| , spec | ||
| ])), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| FunRanges -> maps:get(from, lists:last(FunRanges)) | ||
| end, | ||
| %% Find beginning of first function clause after VarRange | ||
| To = |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
maybe a better idea is for els_parser to store the full range of a function clause in the function_clause POI data
There was a problem hiding this comment.
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..
|
|
||
| -record(foo, {a :: Var, | ||
| b :: [Var]}). | ||
| %% BUG: `Var' in MACRO(`Var') is not considered a variable POI |
There was a problem hiding this comment.
thanks for finding this bug, indeed els_parser currently does not expect sub-POIs in the define name, so it does not recurse into it (els_parser:attribute_subtrees)
There was a problem hiding this comment.
Should we file a ticket about this?
There was a problem hiding this comment.
To me this looks like a very good step forward. I think we can merge this in and eventually improve it later. Thanks @plux !
|
|
||
| -record(foo, {a :: Var, | ||
| b :: [Var]}). | ||
| %% BUG: `Var' in MACRO(`Var') is not considered a variable POI |
There was a problem hiding this comment.
Should we file a ticket about this?
Description
Tweak the heuristic for variable renaming inside functions so it doesn't bleed over to following -spec.
Add possibility to rename variables in specs.
Fixes #1180.