Improvements to extract function

* Ignore variables inside funs() and list comprehensions
* Don't suggest to extract function unless it contains more than one poi

Author: plux

apps/els_core/src/els_poi.erl

@@ -44,6 +44,7 @@
     | include
     | include_lib
     | keyword_expr
+    | list_comp
     | macro
     | module
     | nifs

apps/els_core/src/els_text.erl

@@ -12,7 +12,8 @@
     split_at_line/2,
     tokens/1,
     tokens/2,
-    apply_edits/2
+    apply_edits/2,
+    is_keyword_expr/1
 ]).
 -export([strip_comments/1]).
 
@@ -176,6 +177,18 @@ strip_comments(Text) ->
         )
     ).
 
+-spec is_keyword_expr(binary()) -> boolean().
+is_keyword_expr(Text) ->
+    lists:member(Text, [
+        <<"begin">>,
+        <<"case">>,
+        <<"fun">>,
+        <<"if">>,
+        <<"maybe">>,
+        <<"receive">>,
+        <<"try">>
+    ]).
+
 %%==============================================================================
 %% Internal functions
 %%==============================================================================

apps/els_lsp/src/els_code_actions.erl

@@ -421,11 +421,17 @@ fix_atom_typo(Uri, Range, _Data, [Atom]) ->
 -spec extract_function(uri(), range()) -> [map()].
 extract_function(Uri, Range) ->
     {ok, [Document]} = els_dt_document:lookup(Uri),
-    #{from := From = {Line, Column}, to := To} = els_range:to_poi_range(Range),
+    PoiRange = els_range:to_poi_range(Range),
+    #{from := From = {Line, Column}, to := To} = PoiRange,
     %% We only want to extract if selection is large enough
     %% and cursor is inside a function
+    POIsInRange = els_dt_document:pois_in_range(Document, PoiRange),
+    #{text := Text} = Document,
+    MarkedText = els_text:range(Text, From, To),
     case
-        large_enough_range(From, To) andalso
+        (length(POIsInRange) > 1 orelse
+            els_text:is_keyword_expr(MarkedText)) andalso
+            large_enough_range(From, To) andalso
             not contains_function_clause(Document, Line) andalso
             els_dt_document:wrapping_functions(Document, Line, Column) /= []
     of

apps/els_lsp/src/els_dt_document.erl

@@ -30,6 +30,7 @@
     new/4,
     pois/1,
     pois/2,
+    pois_in_range/2,
     pois_in_range/3,
     get_element_at_pos/3,
     uri/1,
@@ -213,6 +214,16 @@ pois(#{pois := POIs}) ->
 pois(Item, Kinds) ->
     [POI || #{kind := K} = POI <- pois(Item), lists:member(K, Kinds)].
 
+%% @doc Returns the list of POIs of the given types in the given range
+%%      for the current document
+-spec pois_in_range(item(), els_poi:poi_range()) -> [els_poi:poi()].
+pois_in_range(Item, Range) ->
+    [
+        POI
+     || #{range := R} = POI <- pois(Item),
+        els_range:in(R, Range)
+    ].
+
 %% @doc Returns the list of POIs of the given types in the given range
 %%      for the current document
 -spec pois_in_range(

apps/els_lsp/src/els_execute_command_provider.erl

@@ -346,33 +346,49 @@ end_symbol(ExtractString) ->
     non_neg_integer()
 ) -> [string()].
 get_args(PoiRange, Document, FromL, FunBeginLine) ->
-    %% TODO: Possible improvement. To make this bullet proof we should
-    %% ignore vars defined inside LCs and funs()
-    VarPOIs = els_poi:sort(els_dt_document:pois(Document, [variable])),
     BeforeRange = #{from => {FunBeginLine, 1}, to => {FromL, 1}},
-    VarsBefore = ids_in_range(BeforeRange, VarPOIs),
-    VarsInside = ids_in_range(PoiRange, VarPOIs),
+    VarPOIsBefore = els_dt_document:pois_in_range(Document, [variable], BeforeRange),
+    %% Remove all variables inside LCs or keyword expressions
+    LCPOIs = els_dt_document:pois(Document, [list_comp]),
+    FunExprPOIs = [
+        POI
+     || #{id := fun_expr} = POI <- els_dt_document:pois(Document, [keyword_expr])
+    ],
+    %% Only consider fun exprs that doesn't contain the selected range
+    ExcludePOIs = [
+        POI
+     || #{range := R} = POI <- FunExprPOIs ++ LCPOIs, not els_range:in(PoiRange, R)
+    ],
+    VarsBefore = [
+        Id
+     || #{range := VarRange, id := Id} <- els_poi:sort(VarPOIsBefore),
+        not_in_any_range(VarRange, ExcludePOIs)
+    ],
+    %% Find all variables defined before the current function that are used
+    %% inside the selected range.
+    VarPOIsInside = els_dt_document:pois_in_range(Document, [variable], PoiRange),
     els_utils:uniq([
         atom_to_list(Id)
-     || Id <- VarsInside,
+     || #{id := Id} <- VarPOIsInside,
         lists:member(Id, VarsBefore)
     ]).
 
--spec ids_in_range(els_poi:poi_range(), [els_poi:poi()]) -> [atom()].
-ids_in_range(PoiRange, VarPOIs) ->
-    [
-        Id
-     || #{range := R, id := Id} <- VarPOIs,
-        els_range:in(R, PoiRange)
-    ].
+-spec not_in_any_range(els_poi:poi_range(), [els_poi:poi()]) -> boolean().
+not_in_any_range(VarRange, POIs) ->
+    not lists:any(
+        fun(#{range := Range}) ->
+            els_range:in(VarRange, Range)
+        end,
+        POIs
+    ).
 
 -spec extract_range(els_dt_document:item(), range()) -> els_poi:poi_range().
 extract_range(#{text := Text} = Document, Range) ->
     PoiRange = els_range:to_poi_range(Range),
     #{from := {CurrL, CurrC} = From, to := To} = PoiRange,
     POIs = els_dt_document:get_element_at_pos(Document, CurrL, CurrC),
     MarkedText = els_text:range(Text, From, To),
-    case is_keyword_expr(MarkedText) of
+    case els_text:is_keyword_expr(MarkedText) of
         true ->
             case sort_by_range_size([P || #{kind := keyword_expr} = P <- POIs]) of
                 [] ->
@@ -384,18 +400,6 @@ extract_range(#{text := Text} = Document, Range) ->
             PoiRange
     end.
 
--spec is_keyword_expr(binary()) -> boolean().
-is_keyword_expr(Text) ->
-    lists:member(Text, [
-        <<"begin">>,
-        <<"case">>,
-        <<"fun">>,
-        <<"if">>,
-        <<"maybe">>,
-        <<"receive">>,
-        <<"try">>
-    ]).
-
 -spec sort_by_range_size(_) -> _.
 sort_by_range_size(POIs) ->
     lists:sort([{range_size(P), P} || P <- POIs]).

apps/els_lsp/src/els_parser.erl

@@ -276,7 +276,8 @@ do_points_of_interest(Tree) ->
                 Type == implicit_fun;
                 Type == maybe_expr;
                 Type == receive_expr;
-                Type == try_expr
+                Type == try_expr;
+                Type == fun_expr
             ->
                 keyword_expr(Type, Tree);
             _Other ->