From 27638722318c8eff918ec52a2d3454a334c866ce Mon Sep 17 00:00:00 2001 From: Brujo Benavides Rodriguez Date: Thu, 2 Mar 2023 14:20:56 +0100 Subject: [PATCH] Fix #284: New Rule: Param Pattern Matching --- RULES.md | 1 + .../elvis_style/param_pattern_matching.md | 21 ++++++ src/elvis_rulesets.erl | 6 +- src/elvis_style.erl | 74 +++++++++++++++++-- test/examples/left_param_pattern_matching.erl | 24 ++++++ .../pass_operator_spaces_elvis_attr.erl | 10 +-- ...pass_param_pattern_matching_elvis_attr.erl | 19 +++++ .../examples/right_param_pattern_matching.erl | 41 ++++++++++ test/style_SUITE.erl | 57 +++++++++++++- 9 files changed, 238 insertions(+), 15 deletions(-) create mode 100644 doc_rules/elvis_style/param_pattern_matching.md create mode 100644 test/examples/left_param_pattern_matching.erl create mode 100644 test/examples/pass_param_pattern_matching_elvis_attr.erl create mode 100644 test/examples/right_param_pattern_matching.erl diff --git a/RULES.md b/RULES.md index b5916d34..8b8f2045 100644 --- a/RULES.md +++ b/RULES.md @@ -55,6 +55,7 @@ identified with `(since ...)` for convenience purposes. - [No Types](doc_rules/elvis_style/no_types.md) - [Numeric Format](doc_rules/elvis_style/numeric_format.md) - [Operator Spaces](doc_rules/elvis_style/operator_spaces.md) +- [Param Pattern Matching](doc_rules/elvis_style/param_pattern_matching.md) - [State Record and Type](doc_rules/elvis_style/state_record_and_type.md) - [Used Ignored Variable](doc_rules/elvis_style/used_ignored_variable.md) - [Variable Naming Convention](doc_rules/elvis_style/variable_naming_convention.md) diff --git a/doc_rules/elvis_style/param_pattern_matching.md b/doc_rules/elvis_style/param_pattern_matching.md new file mode 100644 index 00000000..ab73a952 --- /dev/null +++ b/doc_rules/elvis_style/param_pattern_matching.md @@ -0,0 +1,21 @@ +# Param Pattern-Matching + +(since [3.0.0](https://github.com/inaka/elvis_core/releases/tag/3.0.0)) + +When capturing a parameter using pattern matching you can either put the parameter +name on the left (`Param = #{pattern := ToMatch}`) or right (`#{pattern := ToMatch} = Param`) side +of the pattern that you use in the function clause. +This rule will make sure you are consistent through your code and use always the same style. + +> Works on `.beam` file? Yes! + +## Options + +- `side :: left | right`. + - default: `left`. + +## Example + +```erlang +{elvis_style, param_pattern_matching, #{side => right}} +``` diff --git a/src/elvis_rulesets.erl b/src/elvis_rulesets.erl index 8b96e2f7..1ae39b22 100644 --- a/src/elvis_rulesets.erl +++ b/src/elvis_rulesets.erl @@ -80,7 +80,8 @@ rules(erl_files) -> {elvis_style, behaviour_spelling}, {elvis_style, export_used_types}, {elvis_style, max_function_arity}, - {elvis_style, max_anonymous_function_arity}]); + {elvis_style, max_anonymous_function_arity}, + {elvis_style, param_pattern_matching}]); rules(beam_files) -> lists:map(fun(Rule) -> {elvis_style, Rule, elvis_style:default(Rule)} end, [nesting_level, @@ -107,7 +108,8 @@ rules(beam_files) -> behaviour_spelling, export_used_types, max_function_arity, - max_anonymous_function_arity]); + max_anonymous_function_arity, + param_pattern_matching]); rules(rebar_config) -> lists:map(fun(Rule) -> {elvis_project, Rule, elvis_project:default(Rule)} end, [no_branch_deps, protocol_for_deps]); diff --git a/src/elvis_style.erl b/src/elvis_style.erl index 5127636b..6a2b7dae 100644 --- a/src/elvis_style.erl +++ b/src/elvis_style.erl @@ -12,7 +12,7 @@ atom_naming_convention/3, no_throw/3, no_dollar_space/3, no_author/3, no_import/3, no_catch_expressions/3, no_single_clause_case/3, numeric_format/3, behaviour_spelling/3, always_shortcircuit/3, consistent_generic_type/3, export_used_types/3, - no_match_in_condition/3, option/3]). + no_match_in_condition/3, param_pattern_matching/3, option/3]). -export_type([empty_rule_config/0]). -export_type([ignorable/0]). @@ -27,7 +27,8 @@ no_common_caveats_call_config/0, atom_naming_convention_config/0, no_author_config/0, no_import_config/0, no_catch_expressions_config/0, numeric_format_config/0, no_single_clause_case_config/0, consistent_variable_casing_config/0, - no_match_in_condition_config/0]). + no_match_in_condition_config/0, behaviour_spelling_config/0, + param_pattern_matching_config/0]). -define(INVALID_MACRO_NAME_REGEX_MSG, "The macro named ~p on line ~p does not respect the format " @@ -121,9 +122,12 @@ -define(NUMERIC_FORMAT_MSG, "Number ~p on line ~p does not respect the format " "defined by the regular expression '~p'."). --define(BEHAVIOUR_SPELLING, +-define(BEHAVIOUR_SPELLING_MSG, "The behavior/behaviour in line ~p is misspelt, please use the " "~p spelling."). +-define(PARAM_PATTERN_MATCHING_MSG, + "Variable ~ts, used to match a parameter in line ~p is placed on " + "the wrong side of the match. It was expected on the ~p side."). -define(ALWAYS_SHORTCIRCUIT_MSG, "Non-shortcircuiting operator (~p) found in line ~p. " "It's recommended to use ~p, instead."). @@ -196,6 +200,8 @@ default(numeric_format) -> float_regex => same}; default(behaviour_spelling) -> #{spelling => behaviour}; +default(param_pattern_matching) -> + #{side => left}; default(consistent_generic_type) -> #{preferred_type => term}; default(RuleWithEmptyDefault) @@ -1125,7 +1131,12 @@ numeric_format(Config, Target, RuleConfig) -> IntNodes, check_numeric_format(FloatRegex, FloatNodes, [])). --spec behaviour_spelling(elvis_config:config(), elvis_file:file(), empty_rule_config()) -> +-type behaviour_spelling_config() :: + #{ignore => [ignorable()], spelling => behaviour | behavior}. + +-spec behaviour_spelling(elvis_config:config(), + elvis_file:file(), + behaviour_spelling_config()) -> [elvis_result:item()]. behaviour_spelling(Config, Target, RuleConfig) -> Spelling = option(spelling, RuleConfig, behaviour_spelling), @@ -1143,11 +1154,64 @@ behaviour_spelling(Config, Target, RuleConfig) -> fun(Node) -> {Line, _} = ktn_code:attr(location, Node), Info = [Line, Spelling], - elvis_result:new(item, ?BEHAVIOUR_SPELLING, Info, Line) + elvis_result:new(item, ?BEHAVIOUR_SPELLING_MSG, Info, Line) end, lists:map(ResultFun, InconsistentBehaviorNodes) end. +-type param_pattern_matching_config() :: #{ignore => [ignorable()], side => left | right}. + +-spec param_pattern_matching(elvis_config:config(), + elvis_file:file(), + param_pattern_matching_config()) -> + [elvis_result:item()]. +param_pattern_matching(Config, Target, RuleConfig) -> + Side = option(side, RuleConfig, param_pattern_matching), + Root = get_root(Config, Target, RuleConfig), + + FunctionClausePatterns = + lists:flatmap(fun(Clause) -> ktn_code:node_attr(pattern, Clause) end, + elvis_code:find(fun is_function_clause/1, + Root, + #{mode => zipper, traverse => all})), + + MatchesInFunctionClauses = + lists:filter(fun(Pattern) -> ktn_code:type(Pattern) == match end, FunctionClausePatterns), + + lists:filtermap(fun(Match) -> + case lists:map(fun ktn_code:type/1, ktn_code:content(Match)) of + [var, var] -> + false; + [var, _] when Side == right -> + {Line, _} = ktn_code:attr(location, Match), + [Var, _] = ktn_code:content(Match), + VarName = ktn_code:attr(name, Var), + Info = [VarName, Line, Side], + {true, + elvis_result:new(item, ?PARAM_PATTERN_MATCHING_MSG, Info, Line)}; + [_, var] when Side == left -> + {Line, _} = ktn_code:attr(location, Match), + [_, Var] = ktn_code:content(Match), + VarName = ktn_code:attr(name, Var), + Info = [VarName, Line, Side], + {true, + elvis_result:new(item, ?PARAM_PATTERN_MATCHING_MSG, Info, Line)}; + _ -> + false + end + end, + MatchesInFunctionClauses). + +is_function_clause(Zipper) -> + clause + == ktn_code:type( + zipper:node(Zipper)) + andalso lists:member( + ktn_code:type( + zipper:node( + zipper:up(Zipper))), + [function, 'fun']). + -spec consistent_generic_type(elvis_config:config(), elvis_file:file(), empty_rule_config()) -> diff --git a/test/examples/left_param_pattern_matching.erl b/test/examples/left_param_pattern_matching.erl new file mode 100644 index 00000000..3cb4c9f5 --- /dev/null +++ b/test/examples/left_param_pattern_matching.erl @@ -0,0 +1,24 @@ +-module(left_param_pattern_matching). + +-export([single/1, multiple/1, different_param/3, different_clause/1]). + +single(Simple = {left, side, assignment}) -> + fun(SimpleToo = {left, side, assignment}) -> Simple == SimpleToo end. + +multiple(Multiple = Assignments = {on, the, left, side}) -> + fun(MultipleToo = AssignmentsToo = {on, the, left, side}) -> + {Multiple, Assignments} == {MultipleToo, AssignmentsToo} + end. + +different_param(happens_on, TheSecond = #{param := of_the}, function) -> + fun(happens_on, TheSecondToo = #{param := of_the}, function) -> TheSecond == TheSecondToo + end. + +different_clause("it doesn't happen on the first clause") -> + not_here; +different_clause(But = "it does in the second one") -> + fun ("it doesn't happen on the first clause") -> + not_here; + (ButToo = "it does in the second one") -> + But == ButToo + end. diff --git a/test/examples/pass_operator_spaces_elvis_attr.erl b/test/examples/pass_operator_spaces_elvis_attr.erl index 7814de3b..030ad6cc 100644 --- a/test/examples/pass_operator_spaces_elvis_attr.erl +++ b/test/examples/pass_operator_spaces_elvis_attr.erl @@ -55,7 +55,7 @@ function7() -> RegExp = "^.{1,20}$" , re:run(Name , RegExp , [unicode]). -tag_filters(DocName , #{conn := Conn} = State) -> +tag_filters(DocName , State = #{conn := Conn}) -> TableName = atom_to_list(DocName) , Sql = ["SELECT " " 'tag' AS \"type\", " @@ -80,7 +80,7 @@ unicode_characters() -> <<"ß"/utf8>> = <<"\\o337">> , ok. -windows_newlines() -> - <<_/bytes>> = <<"Foo" , - "bar">> , - ok. +windows_newlines() -> + <<_/bytes>> = <<"Foo" , + "bar">> , + ok. diff --git a/test/examples/pass_param_pattern_matching_elvis_attr.erl b/test/examples/pass_param_pattern_matching_elvis_attr.erl new file mode 100644 index 00000000..8bffe871 --- /dev/null +++ b/test/examples/pass_param_pattern_matching_elvis_attr.erl @@ -0,0 +1,19 @@ +-module(pass_param_pattern_matching_elvis_attr). + +-elvis([{elvis_style, param_pattern_matching, #{side => right}}]). + +-export([my_fun/1, my_fun/3]). + +my_fun(#{[variable, goes] := [on, the]} = Right) -> + fun (#{[variable, goes] := [on, the]} = RightToo) -> + Right == RightToo; + ({can, be, Left} = {_If, _It, "is not a single variable"}) -> + Left + end; +my_fun({can, be, Left} = {_If, _It, "is not a single variable"}) -> + Left. + +my_fun(works, {as, well, on} = TheOther, parameters) -> + fun(works, {as, well, on} = TheOtherToo, parameters) -> + TheOther == TheOtherToo + end. diff --git a/test/examples/right_param_pattern_matching.erl b/test/examples/right_param_pattern_matching.erl new file mode 100644 index 00000000..56e72109 --- /dev/null +++ b/test/examples/right_param_pattern_matching.erl @@ -0,0 +1,41 @@ +-module(right_param_pattern_matching). + +-export([single/1, multiple/1, different_param/3, different_clause/1]). +-export([regular_matching_works_fine/0]). + +single({right, side, assignment} = Simple) -> + fun({right, side, assignment} = SimpleToo) -> Simple == SimpleToo end. + +%% This is not a match. According to Erlang precedence rules, the following code is actually +%% equivalent to: {on, the, right, side} = (Multiple = Assignments). +%% It's a tuple against a match, not a tuple against a variable against another variable +%% That's why no warnings are emitted for this line even if side is 'left'. +multiple({on, the, right, side} = Multiple = Assignments) -> + fun({on, the, right, side} = MultipleToo = AssignmentsToo) -> + {Multiple, Assignments} == {MultipleToo, AssignmentsToo} + end. + +different_param(happens_on, #{the_second := param_of} = The, function) -> + fun(happens_on, #{the_second := param_of} = TheToo, function) -> The == TheToo + end. + +different_clause("it doesn't happen on the first clause") -> + not_here; +different_clause("it does in the second one" = AsYoda) -> + fun ("it doesn't happen on the first clause") -> + not_here; + ("it does in the second one" = AsYodaToo) -> + AsYoda == AsYodaToo + end. + +regular_matching_works_fine() -> + This = works:fine(), + case This of + This = #{is := also} -> + valid; + _ -> + fun() -> + Like = This, + valid:as_well(Like) + end + end. diff --git a/test/style_SUITE.erl b/test/style_SUITE.erl index 516c78de..50059f98 100644 --- a/test/style_SUITE.erl +++ b/test/style_SUITE.erl @@ -27,7 +27,7 @@ verify_no_single_clause_case/1, verify_numeric_format/1, verify_behaviour_spelling/1, verify_always_shortcircuit/1, verify_consistent_generic_type/1, verify_no_types/1, verify_no_specs/1, verify_export_used_types/1, verify_consistent_variable_casing/1, - verify_no_match_in_condition/1]). + verify_no_match_in_condition/1, verify_param_pattern_matching/1]). %% -elvis attribute -export([verify_elvis_attr_atom_naming_convention/1, verify_elvis_attr_numeric_format/1, verify_elvis_attr_dont_repeat_yourself/1, verify_elvis_attr_function_naming_convention/1, @@ -43,7 +43,7 @@ verify_elvis_attr_no_tabs/1, verify_elvis_attr_no_trailing_whitespace/1, verify_elvis_attr_operator_spaces/1, verify_elvis_attr_state_record_and_type/1, verify_elvis_attr_used_ignored_variable/1, verify_elvis_attr_variable_naming_convention/1, - verify_elvis_attr_behaviour_spelling/1]). + verify_elvis_attr_behaviour_spelling/1, verify_elvis_attr_param_pattern_matching/1]). %% Non-rule -export([results_are_ordered_by_line/1, oddities/1]). @@ -82,7 +82,7 @@ groups() -> verify_no_author, verify_no_import, verify_always_shortcircuit, verify_no_catch_expressions, verify_no_single_clause_case, verify_no_macros, verify_export_used_types, verify_max_anonymous_function_arity, verify_max_function_arity, - verify_no_match_in_condition]}]. + verify_no_match_in_condition, verify_behaviour_spelling, verify_param_pattern_matching]}]. -spec init_per_suite(config()) -> config(). init_per_suite(Config) -> @@ -756,6 +756,53 @@ verify_behaviour_spelling(Config) -> #{spelling => behavior}, PathPass1). +-spec verify_param_pattern_matching(config()) -> any(). +verify_param_pattern_matching(Config) -> + Ext = proplists:get_value(test_file_ext, Config, "erl"), + + PathRight = "right_param_pattern_matching." ++ Ext, + PathLeft = "left_param_pattern_matching." ++ Ext, + + [#{info := ['Simple' | _]}, + #{info := ['SimpleToo' | _]}, + #{info := ['The' | _]}, + #{info := ['TheToo' | _]}, + #{info := ['AsYoda' | _]}, + #{info := ['AsYodaToo' | _]}] = + elvis_core_apply_rule(Config, + elvis_style, + param_pattern_matching, + #{side => left}, + PathRight), + + [#{info := ['Simple' | _]}, + #{info := ['SimpleToo' | _]}, + #{info := ['Multiple' | _]}, + #{info := ['MultipleToo' | _]}, + #{info := ['TheSecond' | _]}, + #{info := ['TheSecondToo' | _]}, + #{info := ['But' | _]}, + #{info := ['ButToo' | _]}] = + elvis_core_apply_rule(Config, + elvis_style, + param_pattern_matching, + #{side => right}, + PathLeft), + + [] = + elvis_core_apply_rule(Config, + elvis_style, + param_pattern_matching, + #{side => right}, + PathRight), + + [] = + elvis_core_apply_rule(Config, + elvis_style, + param_pattern_matching, + #{side => left}, + PathLeft). + -spec verify_consistent_generic_type(config()) -> any(). verify_consistent_generic_type(Config) -> Ext = proplists:get_value(test_file_ext, Config, "erl"), @@ -1672,6 +1719,10 @@ verify_elvis_attr_variable_naming_convention(Config) -> verify_elvis_attr_behaviour_spelling(Config) -> verify_elvis_attr(Config, "pass_behaviour_spelling_elvis_attr"). +-spec verify_elvis_attr_param_pattern_matching(config()) -> true. +verify_elvis_attr_param_pattern_matching(Config) -> + verify_elvis_attr(Config, "pass_param_pattern_matching_elvis_attr"). + %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% %% Private %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%