diff --git a/apps/els_core/src/els_poi.erl b/apps/els_core/src/els_poi.erl index 4a82404f..a9661a50 100644 --- a/apps/els_core/src/els_poi.erl +++ b/apps/els_core/src/els_poi.erl @@ -44,6 +44,7 @@ | include | include_lib | keyword_expr + | list_comp | macro | module | nifs diff --git a/apps/els_core/src/els_text.erl b/apps/els_core/src/els_text.erl index d6cec3a0..76589731 100644 --- a/apps/els_core/src/els_text.erl +++ b/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 %%============================================================================== diff --git a/apps/els_lsp/priv/code_navigation/src/extract_function.erl b/apps/els_lsp/priv/code_navigation/src/extract_function.erl index 2ba7276b..3fa34ceb 100644 --- a/apps/els_lsp/priv/code_navigation/src/extract_function.erl +++ b/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. diff --git a/apps/els_lsp/src/els_code_actions.erl b/apps/els_lsp/src/els_code_actions.erl index 1d7db3cb..d77ab281 100644 --- a/apps/els_lsp/src/els_code_actions.erl +++ b/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 diff --git a/apps/els_lsp/src/els_dt_document.erl b/apps/els_lsp/src/els_dt_document.erl index aee88268..39c2ea29 100644 --- a/apps/els_lsp/src/els_dt_document.erl +++ b/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( diff --git a/apps/els_lsp/src/els_execute_command_provider.erl b/apps/els_lsp/src/els_execute_command_provider.erl index 014ada2a..3f2e1cae 100644 --- a/apps/els_lsp/src/els_execute_command_provider.erl +++ b/apps/els_lsp/src/els_execute_command_provider.erl @@ -346,25 +346,41 @@ 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) -> @@ -372,7 +388,7 @@ extract_range(#{text := Text} = Document, 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]). diff --git a/apps/els_lsp/src/els_parser.erl b/apps/els_lsp/src/els_parser.erl index 712515ae..943f06e7 100644 --- a/apps/els_lsp/src/els_parser.erl +++ b/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 -> diff --git a/apps/els_lsp/test/els_code_action_SUITE.erl b/apps/els_lsp/test/els_code_action_SUITE.erl index 1e1a51bd..f9a99817 100644 --- a/apps/els_lsp/test/els_code_action_SUITE.erl +++ b/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}}), [] ), + %% #{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. diff --git a/apps/els_lsp/test/els_execute_command_SUITE.erl b/apps/els_lsp/test/els_execute_command_SUITE.erl index abd5a9f6..31d6ae9c 100644 --- a/apps/els_lsp/test/els_execute_command_SUITE.erl +++ b/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.