Improvements to extract function (#1563)

* 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/priv/code_navigation/src/extract_function.erl

@@ -10,7 +10,8 @@ f(A, B) ->
         end,
     H = [X || X <- [A, B, C], X > 1],
     I = {A, B, A},
-    ok.
+    other_function(),
+    [X || X <- [A, B, C], X > 1].
 
 other_function() ->
     hello.

apps/els_lsp/src/els_code_actions.erl

@@ -478,11 +478,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,
@@ -214,6 +215,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} <- 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} <- els_poi:sort(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 ->

apps/els_lsp/test/els_code_action_SUITE.erl

@@ -472,27 +472,100 @@ fix_callbacks(Config) ->
 extract_function(Config) ->
     Uri = ?config(extract_function_uri, Config),
     %% These shouldn't return any code actions
+    %% -export([f/2]).
+    %% ^^^^^
     #{result := []} = els_client:document_codeaction(
         Uri,
         els_protocol:range(#{from => {2, 1}, to => {2, 5}}),
         []
     ),
+    %% <empty line>
     #{result := []} = els_client:document_codeaction(
         Uri,
         els_protocol:range(#{from => {3, 1}, to => {3, 5}}),
         []
     ),
+    %% f(A, B) ->
+    %% ^^^^^
     #{result := []} = els_client:document_codeaction(
         Uri,
         els_protocol:range(#{from => {4, 1}, to => {4, 5}}),
         []
     ),
+    %% C = 1,
+    %%     ^
     #{result := []} = els_client:document_codeaction(
         Uri,
         els_protocol:range(#{from => {5, 8}, to => {5, 9}}),
         []
     ),
+    %% other_function()
+    %% ^^^^^^^^^^^^^^^^
+    #{result := []} = els_client:document_codeaction(
+        Uri,
+        els_protocol:range(#{from => {13, 4}, to => {13, 20}}),
+        []
+    ),
+    %% This should return a code action
+    %% F = A + B + C,
+    %%     ^^^^^^^^^
+    #{
+        result := [
+            #{
+                command := #{
+                    title := <<"Extract function">>,
+                    arguments := [#{uri := Uri}]
+                },
+                kind := <<"refactor.extract">>,
+                title := <<"Extract function">>
+            }
+        ]
+    } = els_client:document_codeaction(
+        Uri,
+        els_protocol:range(#{from => {6, 8}, to => {6, 17}}),
+        []
+    ),
+    %% This should return a code action
+    %% G = case A of
+    %%     ^^^^
+    #{
+        result := [
+            #{
+                command := #{
+                    title := <<"Extract function">>,
+                    arguments := [#{uri := Uri}]
+                },
+                kind := <<"refactor.extract">>,
+                title := <<"Extract function">>
+            }
+        ]
+    } = els_client:document_codeaction(
+        Uri,
+        els_protocol:range(#{from => {7, 9}, to => {7, 13}}),
+        []
+    ),
+    %% This should return a code action
+    %% H = [X || X <- [A, B, C], X > 1],
+    %%     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+    #{
+        result := [
+            #{
+                command := #{
+                    title := <<"Extract function">>,
+                    arguments := [#{uri := Uri}]
+                },
+                kind := <<"refactor.extract">>,
+                title := <<"Extract function">>
+            }
+        ]
+    } = els_client:document_codeaction(
+        Uri,
+        els_protocol:range(#{from => {11, 8}, to => {11, 36}}),
+        []
+    ),
     %% This should return a code action
+    %% I = {A, B, A},
+    %%     ^^^^^^^^^
     #{
         result := [
             #{
@@ -506,7 +579,7 @@ extract_function(Config) ->
         ]
     } = els_client:document_codeaction(
         Uri,
-        els_protocol:range(#{from => {5, 8}, to => {5, 17}}),
+        els_protocol:range(#{from => {12, 8}, to => {12, 17}}),
         []
     ),
     ok.

apps/els_lsp/test/els_execute_command_SUITE.erl

@@ -18,7 +18,8 @@
     suggest_spec/1,
     extract_function/1,
     extract_function_case/1,
-    extract_function_tuple/1
+    extract_function_tuple/1,
+    extract_function_list_comp/1
 ]).
 
 %%==============================================================================
@@ -57,7 +58,8 @@ init_per_testcase(TestCase, Config0) when
     TestCase =:= ct_run_test;
     TestCase =:= extract_function;
     TestCase =:= extract_function_case;
-    TestCase =:= extract_function_tuple
+    TestCase =:= extract_function_tuple;
+    TestCase =:= extract_function_list_comp
 ->
     Config = els_test_utils:init_per_testcase(TestCase, Config0),
     setup_mocks(),
@@ -82,7 +84,8 @@ end_per_testcase(TestCase, Config) when
     TestCase =:= ct_run_test;
     TestCase =:= extract_function;
     TestCase =:= extract_function_case;
-    TestCase =:= extract_function_tuple
+    TestCase =:= extract_function_tuple;
+    TestCase =:= extract_function_list_comp
 ->
     teardown_mocks(),
     els_test_utils:end_per_testcase(TestCase, Config);
@@ -285,8 +288,8 @@ extract_function(Config) ->
                 "    A + B + C.\n\n"
             >>,
             range := #{
-                'end' := #{character := 0, line := 14},
-                start := #{character := 0, line := 14}
+                'end' := #{character := 0, line := 15},
+                start := #{character := 0, line := 15}
             }
         }
     ] = Changes.
@@ -315,12 +318,40 @@ extract_function_case(Config) ->
                 >>,
             range :=
                 #{
-                    'end' := #{character := 0, line := 14},
-                    start := #{character := 0, line := 14}
+                    'end' := #{character := 0, line := 15},
+                    start := #{character := 0, line := 15}
                 }
         }
     ] = Changes.
 
+-spec extract_function_list_comp(config()) -> ok.
+extract_function_list_comp(Config) ->
+    Uri = ?config(extract_function_uri, Config),
+    execute_command_refactor_extract(Uri, {13, 4}, {13, 32}),
+    [#{edit := #{changes := #{Uri := Changes}}}] = get_edits_from_meck_history(),
+    [
+        #{
+            range :=
+                #{
+                    start := #{line := 13, character := 4},
+                    'end' := #{line := 13, character := 32}
+                },
+            newText := <<"new_function(A, B, C)">>
+        },
+        #{
+            range :=
+                #{
+                    start := #{line := 15, character := 0},
+                    'end' := #{line := 15, character := 0}
+                },
+            newText :=
+                <<
+                    "new_function(A, B, C) ->\n"
+                    "    [X || X <- [A, B, C], X > 1].\n\n"
+                >>
+        }
+    ] = Changes.
+
 -spec extract_function_tuple(config()) -> ok.
 extract_function_tuple(Config) ->
     Uri = ?config(extract_function_uri, Config),
@@ -343,8 +374,8 @@ extract_function_tuple(Config) ->
                 >>,
             range :=
                 #{
-                    'end' := #{character := 0, line := 14},
-                    start := #{character := 0, line := 14}
+                    'end' := #{character := 0, line := 15},
+                    start := #{character := 0, line := 15}
                 }
         }
     ] = Changes.