Add Fix for #1202 remove unused include #1227
Conversation
Add Fix for erlang-ls#1202 remove unused include
There was a problem hiding this comment.
Hi @rahulraina7! This looks great. I believe the only thing missing is to go through the ensure_range function like in the other code actions. Other than that, LGTM!
| els_compiler_diagnostics:inclusion_range(Path, Document, include_lib) of | ||
| [Range|_] -> | ||
| case els_range:inclusion_range(Uri, Document) of | ||
| {ok, Range} -> | ||
| Range; | ||
| _ -> |
There was a problem hiding this comment.
Nit: Since this has been refactored (nice!), this pattern can be restricted to error.
| [ {"function (.*) is unused", fun action_export_function/3} | ||
| , {"variable '(.*)' is unused", fun action_ignore_variable/3} | ||
| , {"variable '(.*)' is unbound", fun action_suggest_variable/3} | ||
| [ {"function (.*) is unused", fun action_export_function/4} |
There was a problem hiding this comment.
Good that we are now passing the optional data field to the specific functions. It could be a nice way to exchange information. Later on (no need to do it as part of this PR), we could always ensure there's data in a diagnostic when constructing one. We may also want to use a map instead of a binary and base64 encode it to ensure it can be passed around between client and server. See the els_utils:base64_encode_term/1 function and its usages for Call Hierarchy. Again, this is more of a mental note and can be done as a separate task.
| -spec action_remove_unused(uri(), range(), binary(), [binary()]) -> [map()]. | ||
| action_remove_unused(Uri, _Range0, Data, [Var]) -> | ||
| {ok, Document} = els_utils:lookup_document(Uri), | ||
| case els_range:inclusion_range(Data, Document) of |
There was a problem hiding this comment.
I suspect we still need to go through the ensure_range function like in the other cases, to avoid situations where the file changed in the meantime, causing text edits not to be applicable any longer. @plux we probably need to figure out a better way to handle this, since this step can easily be missed. Maybe have a synchronization point before applying edits?
There was a problem hiding this comment.
Instead of using POIs we could store the text that the diagnostic refers to and simply check that the document text in the range that the diagnostic points to is the same. Does that make sense?
There was a problem hiding this comment.
@robertoaloi Isn't inclusion_range essentially doing the same thing instead of using POI's its using text from Diagnostics to find a range. I can't directly reuse ensure_range since it wont work for this use-case specifically this line Id =:= SubjectAtom , I can either make another similar function ensure_range_imports which will have the same idea as this one, if it makes sense ?
There was a problem hiding this comment.
To me we have two options:
- Add a clause to the
ensure_rangewhich doesn't perform the conversion to atoms - Probably cleaner, what @plux suggested: store the text that the diagnostic refers to on construction and then check here that this is still a valid "selection".
There was a problem hiding this comment.
Sorry I may be missing something.
els_range:inclusion_range looks like this.
inclusion_range(Uri, Document) ->
Path = binary_to_list(els_uri:path(Uri)),
case
els_compiler_diagnostics:inclusion_range(Path, Document, include) ++
els_compiler_diagnostics:inclusion_range(Path, Document, include_lib) of
[Range|_] ->
{ok, Range};
[] ->
error
end.
els_compiler_diagnostics:inclusion_range Looks like this
inclusion_range(IncludePath, Document, include) ->
POIs = els_dt_document:pois(Document, [include]),
IncludeId = els_utils:include_id(IncludePath),
[Range || #{id := Id, range := Range} <- POIs, Id =:= IncludeId];
This is what we want right ?
There was a problem hiding this comment.
I think you have a point. The major difference (compared to the other code actions) is that we ignore the range returned by the diagnostic and we just fetch it directly from the updated list of POIs. In the other cases we would check whether the range from the updated list of POIs would fit inside the same line of the one reported by the diagnostic (prior versions of Erlang didn't have column numbers in diagnostics). In practice I'm not sure this matters much for unused includes. I am ok to merge this one as-is and to eventually rethink this "checking" strategy, either by taking @plux's approach or by versioning the version of the text.
|
Thanks! |
Adds code action to remove unused fixes / include
Fixes #1202 .