Merge pull request #305 from inaka/brujo.284.consistent_param_pattern…

…_matching Fix #284: New Rule: Param Pattern Matching
inaka · Mar 2, 2023 · 70504fb · 70504fb
2 parents d90a43b + cb10984
commit 70504fb
Show file tree

Hide file tree

Showing 13 changed files with 262 additions and 37 deletions.
diff --git 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/config/test.config b/config/test.config
@@ -6,7 +6,8 @@
            [{elvis_text_style, line_length, #{limit => 80, skip_comments => false}},
             {elvis_style, nesting_level, #{level => 3}},
             {elvis_style, invalid_dynamic_call, #{ignore => [elvis]}},
-            {elvis_style, no_macros}],
+            {elvis_style, no_macros},
+            {elvis_style, param_pattern_matching, #{side => right}}],
        ruleset => erl_files},
      #{dirs => ["../../_build/test/lib/elvis_core/test/examples"],
        filter => "*.hrl",

diff --git a/doc_rules/elvis_style/param_pattern_matching.md 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 throughout your code and always use the same style.
+
+> Works on `.beam` file? Yes!
+
+## Options
+
+- `side :: left | right`.
+  - default: `right`.
+
+## Example
+
+```erlang
+{elvis_style, param_pattern_matching, #{side => left}}
+```
diff --git a/src/elvis_code.erl b/src/elvis_code.erl
@@ -76,7 +76,7 @@ content_zipper(Root) ->
 -spec all_zipper(ktn_code:tree_node()) -> zipper:zipper(_).
 all_zipper(Root) ->
     IsBranch =
-        fun(Node = #{}) -> ktn_code:content(Node) =/= [] orelse maps:is_key(node_attrs, Node) end,
+        fun(#{} = Node) -> ktn_code:content(Node) =/= [] orelse maps:is_key(node_attrs, Node) end,
     Children =
         fun (#{content := Content, node_attrs := NodeAttrs}) ->
                 Content
@@ -124,7 +124,7 @@ find_by_location(Root, Location) ->
             {ok, Node}
     end.
 
-is_at_location(Node = #{attrs := #{location := {Line, NodeCol}}}, {Line, Column}) ->
+is_at_location(#{attrs := #{location := {Line, NodeCol}}} = Node, {Line, Column}) ->
     Text = ktn_code:attr(text, Node),
     Length = length(Text),
     NodeCol =< Column andalso Column < NodeCol + Length;
@@ -168,7 +168,7 @@ print_node(Node) ->
     print_node(Node, 0).
 
 -spec print_node(ktn_code:tree_node(), integer()) -> ok.
-print_node(Node = #{type := Type}, CurrentLevel) ->
+print_node(#{type := Type} = Node, CurrentLevel) ->
     Type = ktn_code:type(Node),
     Indentation = lists:duplicate(CurrentLevel * 4, $\s),
     Content = ktn_code:content(Node),
@@ -230,19 +230,19 @@ level_increment(#{type := Type}) ->
 %% @doc Returns an anonymous Fun to be flatmapped over node content, as
 %% appropriate for the exported function whose name is the argument given.
 make_extractor_fun(exported_functions) ->
-    fun (Node = #{type := export}) ->
+    fun (#{type := export} = Node) ->
             ktn_code:attr(value, Node);
         (_) ->
             []
     end;
 make_extractor_fun(exported_types) ->
-    fun (Node = #{type := export_type}) ->
+    fun (#{type := export_type} = Node) ->
             ktn_code:attr(value, Node);
         (_) ->
             []
     end;
 make_extractor_fun(function_names) ->
-    fun (Node = #{type := function}) ->
+    fun (#{type := function} = Node) ->
             [ktn_code:attr(name, Node)];
         (_) ->
             []

diff --git a/src/elvis_config.erl b/src/elvis_config.erl
@@ -106,7 +106,7 @@ normalize(Config) when is_list(Config) ->
     lists:map(fun do_normalize/1, Config).
 
 %% @private
-do_normalize(Config = #{src_dirs := Dirs}) ->
+do_normalize(#{src_dirs := Dirs} = Config) ->
     %% NOTE: Provided for backwards compatibility.
     %% Rename 'src_dirs' key to 'dirs'.
     Config1 = maps:remove(src_dirs, Config),
@@ -117,31 +117,31 @@ do_normalize(Config) ->
 -spec dirs(Config :: configs() | config()) -> [string()].
 dirs(Config) when is_list(Config) ->
     lists:flatmap(fun dirs/1, Config);
-dirs(_RuleGroup = #{dirs := Dirs}) ->
+dirs(#{dirs := Dirs}) ->
     Dirs;
 dirs(#{}) ->
     [].
 
 -spec ignore(configs() | config()) -> [string()].
 ignore(Config) when is_list(Config) ->
     lists:flatmap(fun ignore/1, Config);
-ignore(_RuleGroup = #{ignore := Ignore}) ->
+ignore(#{ignore := Ignore}) ->
     lists:map(fun ignore_to_regexp/1, Ignore);
 ignore(#{}) ->
     [].
 
 -spec filter(configs() | config()) -> [string()].
 filter(Config) when is_list(Config) ->
     lists:flatmap(fun filter/1, Config);
-filter(_RuleGroup = #{filter := Filter}) ->
+filter(#{filter := Filter}) ->
     Filter;
 filter(#{}) ->
     ?DEFAULT_FILTER.
 
 -spec files(RuleGroup :: configs() | config()) -> [elvis_file:file()].
 files(RuleGroup) when is_list(RuleGroup) ->
     lists:map(fun files/1, RuleGroup);
-files(_RuleGroup = #{files := Files}) ->
+files(#{files := Files}) ->
     Files;
 files(#{}) ->
     [].
@@ -194,9 +194,9 @@ resolve_files(RuleGroup, Files) ->
 %%      end  'filter' key, or if not specified uses '*.erl'.
 %% @end
 -spec resolve_files(config()) -> config().
-resolve_files(RuleGroup = #{files := _Files}) ->
+resolve_files(#{files := _Files} = RuleGroup) ->
     RuleGroup;
-resolve_files(RuleGroup = #{dirs := Dirs}) ->
+resolve_files(#{dirs := Dirs} = RuleGroup) ->
     Filter = filter(RuleGroup),
     Files = elvis_file:find_files(Dirs, Filter),
     resolve_files(RuleGroup, Files).
@@ -209,7 +209,7 @@ resolve_files(RuleGroup = #{dirs := Dirs}) ->
 apply_to_files(Fun, Config) when is_list(Config) ->
     ApplyFun = fun(RuleGroup) -> apply_to_files(Fun, RuleGroup) end,
     lists:map(ApplyFun, Config);
-apply_to_files(Fun, RuleGroup = #{files := Files}) ->
+apply_to_files(Fun, #{files := Files} = RuleGroup) ->
     NewFiles = lists:map(Fun, Files),
     RuleGroup#{files => NewFiles}.
 

diff --git a/src/elvis_file.erl b/src/elvis_file.erl
@@ -16,11 +16,11 @@
 
 %% @doc Returns a tuple with the contents of the file and the file itself.
 -spec src(file()) -> {binary(), file()} | {error, enoent}.
-src(File = #{content := Content, encoding := _}) ->
+src(#{content := Content, encoding := _} = File) ->
     {Content, File};
-src(File = #{content := Content}) ->
+src(#{content := Content} = File) ->
     {Content, File#{encoding => find_encoding(Content)}};
-src(File = #{path := Path}) ->
+src(#{path := Path} = File) ->
     case file:read_file(Path) of
         {ok, Content} ->
             Encoding = find_encoding(Content),
@@ -49,27 +49,27 @@ parse_tree(Config, Target) ->
                  file(),
                  elvis_core:rule_config()) ->
                     {ktn_code:tree_node(), file()}.
-parse_tree(_Config, File = #{parse_tree := ParseTree0}, RuleConfig) ->
+parse_tree(_Config, #{parse_tree := ParseTree0} = File, RuleConfig) ->
     Ignore = maps:get(ignore, RuleConfig, []),
     Mod = module(File),
     {filter_tree_for(ParseTree0, Mod, Ignore), File};
-parse_tree(Config, File = #{path := Path, content := Content}, RuleConfig) ->
+parse_tree(Config, #{path := Path, content := Content} = File, RuleConfig) ->
     Ext = filename:extension(Path),
     ExtStr = elvis_utils:to_str(Ext),
     Mod = module(File),
     Ignore = maps:get(ignore, RuleConfig, []),
     ParseTree = resolve_parse_tree(ExtStr, Content, Mod, Ignore),
     File1 = maybe_add_abstract_parse_tree(Config, File, Mod, Ignore),
     parse_tree(Config, File1#{parse_tree => ParseTree}, RuleConfig);
-parse_tree(Config, File0 = #{path := _Path}, RuleConfig) ->
+parse_tree(Config, #{path := _Path} = File0, RuleConfig) ->
     {_, File} = src(File0),
     parse_tree(Config, File, RuleConfig);
 parse_tree(_Config, File, _RuleConfig) ->
     throw({invalid_file, File}).
 
 %% @doc Loads and adds all related file data.
 -spec load_file_data(elvis_config:configs() | elvis_config:config(), file()) -> file().
-load_file_data(Config, File0 = #{path := _Path}) ->
+load_file_data(Config, #{path := _Path} = File0) ->
     {_, File1} = src(File0),
     {_, File2} = parse_tree(Config, File1),
     File2.
@@ -87,14 +87,13 @@ find_files(Dirs, Pattern) ->
             <- lists:usort(
                    lists:flatmap(Fun, Dirs))].
 
-dir_to(Filter, _Dir = ".") ->
+dir_to(Filter, ".") ->
     Filter;
 dir_to(Filter, Dir) ->
     filename:join(Dir, Filter).
 
 file_in(ExpandedFilter, Files) ->
-    lists:filter(fun(_File = #{path := Path}) -> lists:member(Path, ExpandedFilter) end,
-                 Files).
+    lists:filter(fun(#{path := Path}) -> lists:member(Path, ExpandedFilter) end, Files).
 
 %% @doc Filter files based on the glob provided.
 -spec filter_files([file()], [string()], string(), [string()]) -> [file()].
@@ -162,7 +161,7 @@ find_encoding(Content) ->
          Ignore :: [elvis_style:ignorable()],
          Res :: file().
 maybe_add_abstract_parse_tree(#{ruleset := beam_files},
-                              File = #{path := Path},
+                              #{path := Path} = File,
                               Mod,
                               Ignore) ->
     AbstractParseTree = get_abstract_parse_tree(Path, Mod, Ignore),

diff --git 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
@@ -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 => right};
 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,68 @@ 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) ->
+    is_clause(Zipper) andalso is_function_or_fun(zipper:up(Zipper)).
+
+is_clause(Zipper) ->
+    ktn_code:type(
+        zipper:node(Zipper))
+    == clause.
+
+is_function_or_fun(Zipper) ->
+    lists:member(
+        ktn_code:type(
+            zipper:node(Zipper)),
+        [function, 'fun']).
+
 -spec consistent_generic_type(elvis_config:config(),
                               elvis_file:file(),
                               empty_rule_config()) ->

diff --git a/src/elvis_utils.erl b/src/elvis_utils.erl
@@ -62,7 +62,7 @@ context(List, CtxCount) ->
 
 context([], _Past, _CtxCount, Results) ->
     lists:reverse(Results);
-context([Current | Future], Past, CtxCount = {PrevCount, NextCount}, Results) ->
+context([Current | Future], Past, {PrevCount, NextCount} = CtxCount, Results) ->
     Prev = lists:sublist(Past, PrevCount),
     Next = lists:sublist(Future, NextCount),
     Item = {Current, lists:reverse(Prev), Next},