Add refactoring code action to extract function (#1506)

plux · web-flow · commit 908d9e8f41a7 · 2024-04-25T10:28:11.000+02:00
diff --git a/apps/els_core/src/els_poi.erl b/apps/els_core/src/els_poi.erl
@@ -43,6 +43,7 @@
     | import_entry
     | include
     | include_lib
+    | keyword_expr
     | macro
     | module
     | parse_transform
diff --git a/apps/els_core/src/els_utils.erl b/apps/els_core/src/els_utils.erl
@@ -26,7 +26,8 @@
     jaro_distance/2,
     is_windows/0,
     system_tmp_dir/0,
-    race/2
+    race/2,
+    uniq/1
 ]).
 
 %%==============================================================================
@@ -295,6 +296,30 @@ race(Funs, Timeout) ->
         error(timeout)
     end.
 
+%% uniq/1: return a new list with the unique elements of the given list
+-spec uniq(List1) -> List2 when
+    List1 :: [T],
+    List2 :: [T],
+    T :: term().
+
+uniq(L) ->
+    uniq(L, #{}).
+
+-spec uniq(List1, Map) -> List2 when
+    Map :: map(),
+    List1 :: [T],
+    List2 :: [T],
+    T :: term().
+uniq([X | Xs], M) ->
+    case is_map_key(X, M) of
+        true ->
+            uniq(Xs, M);
+        false ->
+            [X | uniq(Xs, M#{X => true})]
+    end;
+uniq([], _) ->
+    [].
+
 %%==============================================================================
 %% Internal functions
 %%==============================================================================
diff --git a/apps/els_lsp/priv/code_navigation/src/extract_function.erl b/apps/els_lsp/priv/code_navigation/src/extract_function.erl
@@ -0,0 +1,16 @@
+-module(extract_function).
+-export([f/2]).
+
+f(A, B) ->
+    C = 1,
+    F = A + B + C,
+    G = case A of
+            1 -> one;
+            _ -> other
+        end,
+    H = [X || X <- [A, B, C], X > 1],
+    I = {A, B, A},
+    ok.
+
+other_function() ->
+    hello.
diff --git a/apps/els_lsp/src/els_code_action_provider.erl b/apps/els_lsp/src/els_code_action_provider.erl
@@ -13,6 +13,8 @@
 %%==============================================================================
 -spec handle_request(any()) -> {response, any()}.
 handle_request({document_codeaction, Params}) ->
+    %% TODO: Make code actions run async?
+    %% TODO: Extract document here
     #{
         <<"textDocument">> := #{<<"uri">> := Uri},
         <<"range">> := RangeLSP,
@@ -30,7 +32,8 @@ handle_request({document_codeaction, Params}) ->
 code_actions(Uri, Range, #{<<"diagnostics">> := Diagnostics}) ->
     lists:usort(
         lists:flatten([make_code_actions(Uri, D) || D <- Diagnostics]) ++
-            wrangler_handler:get_code_actions(Uri, Range)
+            wrangler_handler:get_code_actions(Uri, Range) ++
+            els_code_actions:extract_function(Uri, Range)
     ).
 
 -spec make_code_actions(uri(), map()) -> [map()].
diff --git a/apps/els_lsp/src/els_code_actions.erl b/apps/els_lsp/src/els_code_actions.erl
@@ -1,5 +1,6 @@
 -module(els_code_actions).
 -export([
+    extract_function/2,
     create_function/4,
     export_function/4,
     fix_module_name/4,
@@ -197,6 +198,59 @@ 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),
+    %% We only want to extract if selection is large enough
+    %% and cursor is inside a function
+    case
+        large_enough_range(From, To) andalso
+            not contains_function_clause(Document, Line) andalso
+            els_dt_document:wrapping_functions(Document, Line, Column) /= []
+    of
+        true ->
+            [
+                #{
+                    title => <<"Extract function">>,
+                    kind => <<"refactor.extract">>,
+                    command => make_extract_function_command(Range, Uri)
+                }
+            ];
+        false ->
+            []
+    end.
+
+-spec make_extract_function_command(range(), uri()) -> map().
+make_extract_function_command(Range, Uri) ->
+    els_command:make_command(
+        <<"Extract function">>,
+        <<"refactor.extract">>,
+        [#{uri => Uri, range => Range}]
+    ).
+
+-spec contains_function_clause(
+    els_dt_document:item(),
+    non_neg_integer()
+) -> boolean().
+contains_function_clause(Document, Line) ->
+    POIs = els_dt_document:get_element_at_pos(Document, Line, 1),
+    lists:any(
+        fun
+            (#{kind := 'function_clause'}) ->
+                true;
+            (_) ->
+                false
+        end,
+        POIs
+    ).
+
+-spec large_enough_range(pos(), pos()) -> boolean().
+large_enough_range({Line, FromC}, {Line, ToC}) when (ToC - FromC) < 2 ->
+    false;
+large_enough_range(_From, _To) ->
+    true.
+
 -spec undefined_callback(uri(), range(), binary(), [binary()]) -> [map()].
 undefined_callback(Uri, _Range, _Data, [_Function, Behaviour]) ->
     Title = <<"Add missing callbacks for: ", Behaviour/binary>>,
diff --git a/apps/els_lsp/src/els_document_highlight_provider.erl b/apps/els_lsp/src/els_document_highlight_provider.erl
@@ -25,7 +25,7 @@ handle_request({document_highlight, Params}) ->
     } = Params,
     {ok, Document} = els_utils:lookup_document(Uri),
     Highlights =
-        case els_dt_document:get_element_at_pos(Document, Line + 1, Character + 1) of
+        case valid_highlight_pois(Document, Line, Character) of
             [POI | _] -> find_highlights(Document, POI);
             [] -> null
         end,
@@ -35,6 +35,18 @@ handle_request({document_highlight, Params}) ->
         %% overwrites them for more transparent Wrangler forms.
     end.
 
+-spec valid_highlight_pois(els_dt_document:item(), integer(), integer()) ->
+    [els_poi:poi()].
+valid_highlight_pois(Document, Line, Character) ->
+    POIs = els_dt_document:get_element_at_pos(Document, Line + 1, Character + 1),
+    [
+        P
+     || #{kind := Kind} = P <- POIs,
+        Kind /= keyword_expr,
+        Kind /= module,
+        Kind /= function_clause
+    ].
+
 %%==============================================================================
 %% Internal functions
 %%==============================================================================
diff --git a/apps/els_lsp/src/els_execute_command_provider.erl b/apps/els_lsp/src/els_execute_command_provider.erl
@@ -24,6 +24,7 @@ options() ->
         <<"show-behaviour-usages">>,
         <<"suggest-spec">>,
         <<"function-references">>,
+        <<"refactor.extract">>,
         <<"add-behaviour-callbacks">>
     ],
     #{
@@ -106,6 +107,14 @@ execute_command(<<"suggest-spec">>, [
         },
     els_server:send_request(Method, Params),
     [];
+execute_command(<<"refactor.extract">>, [
+    #{
+        <<"uri">> := Uri,
+        <<"range">> := Range
+    }
+]) ->
+    ok = extract_function(Uri, Range),
+    [];
 execute_command(<<"add-behaviour-callbacks">>, [
     #{
         <<"uri">> := Uri,
@@ -197,6 +206,137 @@ execute_command(Command, Arguments) ->
     end,
     [].
 
+-spec extract_function(uri(), range()) -> ok.
+extract_function(Uri, Range) ->
+    {ok, [#{text := Text} = Document]} = els_dt_document:lookup(Uri),
+    ExtractRange = extract_range(Document, Range),
+    #{from := {FromL, FromC} = From, to := {ToL, ToC}} = ExtractRange,
+    ExtractString0 = els_text:range(Text, From, {ToL, ToC}),
+    %% Trim whitespace
+    ExtractString = string:trim(ExtractString0, both, " \n\r\t"),
+    %% Trim trailing termination symbol
+    ExtractStringTrimmed = string:trim(ExtractString, trailing, ",.;"),
+    Method = <<"workspace/applyEdit">>,
+    case els_dt_document:wrapping_functions(Document, FromL, FromC) of
+        [WrappingFunPOI | _] when ExtractStringTrimmed /= <<>> ->
+            %% WrappingFunPOI is the function that we are currently in
+            #{
+                data := #{
+                    wrapping_range :=
+                        #{
+                            from := {FunBeginLine, _},
+                            to := {FunEndLine, _}
+                        }
+                }
+            } = WrappingFunPOI,
+            %% Get args needed for the new function
+            Args = get_args(ExtractRange, Document, FromL, FunBeginLine),
+            ArgsBin = unicode:characters_to_binary(string:join(Args, ", ")),
+            FunClause = <<"new_function(", ArgsBin/binary, ")">>,
+            %% Place the new function after the current function
+            EndSymbol = end_symbol(ExtractString),
+            NewRange = els_protocol:range(
+                #{from => {FunEndLine + 1, 1}, to => {FunEndLine + 1, 1}}
+            ),
+            FunBody = unicode:characters_to_list(
+                <<FunClause/binary, " ->\n", ExtractStringTrimmed/binary, ".">>
+            ),
+            {ok, FunBodyFormatted, _} = erlfmt:format_string(FunBody, []),
+            NewFun = unicode:characters_to_binary(FunBodyFormatted ++ "\n"),
+            Changes = [
+                #{
+                    newText => <<FunClause/binary, EndSymbol/binary>>,
+                    range => els_protocol:range(ExtractRange)
+                },
+                #{
+                    newText => NewFun,
+                    range => NewRange
+                }
+            ],
+            Params = #{edit => #{changes => #{Uri => Changes}}},
+            els_server:send_request(Method, Params);
+        _ ->
+            ?LOG_INFO("No wrapping function found"),
+            ok
+    end.
+
+-spec end_symbol(binary()) -> binary().
+end_symbol(ExtractString) ->
+    case binary:last(ExtractString) of
+        $. -> <<".">>;
+        $, -> <<",">>;
+        $; -> <<";">>;
+        _ -> <<>>
+    end.
+
+%% @doc Find all variables defined in the function before the current.
+%%      If they are used inside the selected range, they need to be
+%%      sent in as arguments to the new function.
+-spec get_args(
+    els_poi:poi_range(),
+    els_dt_document:item(),
+    non_neg_integer(),
+    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),
+    els_utils:uniq([
+        atom_to_list(Id)
+     || Id <- VarsInside,
+        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 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
+        true ->
+            case sort_by_range_size([P || #{kind := keyword_expr} = P <- POIs]) of
+                [] ->
+                    PoiRange;
+                [{_Size, #{range := SmallestRange}} | _] ->
+                    SmallestRange
+            end;
+        false ->
+            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]).
+
+-spec range_size(_) -> _.
+range_size(#{range := #{from := {FromL, FromC}, to := {ToL, ToC}}}) ->
+    {ToL - FromL, ToC - FromC}.
+
 -spec spec_text(binary()) -> binary().
 spec_text(<<"-callback", Rest/binary>>) ->
     <<"-spec", Rest/binary>>;
diff --git a/apps/els_lsp/src/els_parser.erl b/apps/els_lsp/src/els_parser.erl
@@ -272,13 +272,28 @@ do_points_of_interest(Tree) ->
                 type_application(Tree);
             record_type ->
                 record_type(Tree);
-            _ ->
+            Type when
+                Type == block_expr;
+                Type == case_expr;
+                Type == if_expr;
+                Type == implicit_fun;
+                Type == maybe_expr;
+                Type == receive_expr;
+                Type == try_expr
+            ->
+                keyword_expr(Type, Tree);
+            _Other ->
                 []
         end
     catch
         throw:syntax_error -> []
     end.
 
+-spec keyword_expr(atom(), tree()) -> [els_poi:poi()].
+keyword_expr(Type, Tree) ->
+    Pos = erl_syntax:get_pos(Tree),
+    [poi(Pos, keyword_expr, Type)].
+
 -spec application(tree()) -> [els_poi:poi()].
 application(Tree) ->
     case application_mfa(Tree) of
diff --git a/apps/els_lsp/test/els_code_action_SUITE.erl b/apps/els_lsp/test/els_code_action_SUITE.erl
diff --git a/apps/els_lsp/test/els_execute_command_SUITE.erl b/apps/els_lsp/test/els_execute_command_SUITE.erl
diff --git a/apps/els_lsp/test/els_workspace_symbol_SUITE.erl b/apps/els_lsp/test/els_workspace_symbol_SUITE.erl
