Fix #289: New Rule: No match in conditions

RULES.md

@@ -41,6 +41,7 @@ identified with `(since ...)` for convenience purposes.
 - [No If Expression](doc_rules/elvis_style/no_if_expression.md)
 - [No Import](doc_rules/elvis_style/no_import.md)
 - [No Macros](doc_rules/elvis_style/no_macros.md)
+- [No Match in Condition](doc_rules/elvis_style/no_match_in_condition.md)
 - [No Nested try...catch Blocks](doc_rules/elvis_style/no_nested_try_catch.md)
 - [No Single-Clause Case Statements](doc_rules/elvis_style/no_single_clause_case.md)
 - [No Space after #](doc_rules/elvis_style/no_space_after_pount.md)

doc_rules/elvis_style/no_match_in_condition.md

@@ -0,0 +1,36 @@
+# No Match in Condition
+
+(since [3.0.0](https://github.com/inaka/elvis_core/releases/tag/3.0.0))
+
+Don't write code like this:
+
+```erlang
+case #{a := A} = do:something() of
+    #{b := good} -> {a, really, nice, A};
+    #{b := bad} -> {"not", a, good, A}
+end
+```
+
+Do the matching in the clause heads (i.e., outside of the `case` condition):
+
+```erlang
+case do:something() of
+    #{a := A, b := good} -> {a, really, nice, A};
+    #{a := A, b := bad} -> {"not", a, good, A}
+end
+```
+
+While the code as written in the first example is valid, it's much harder to understand
+(particularly for large statements) than the one from the second example.
+
+> Works on `.beam` file? Yes!
+
+## Options
+
+- None.
+
+## Example
+
+```erlang
+{elvis_style, no_match_in_condition, #{}}
+```

src/elvis_rulesets.erl

@@ -38,6 +38,7 @@ rules(hrl_files) ->
                {elvis_style, no_import},
                {elvis_style, no_catch_expressions},
                {elvis_style, no_single_clause_case},
+               {elvis_style, no_match_in_condition},
                {elvis_style, numeric_format},
                {elvis_style, no_specs},
                {elvis_style, no_types}]);
@@ -74,6 +75,7 @@ rules(erl_files) ->
                {elvis_style, no_import},
                {elvis_style, no_catch_expressions},
                {elvis_style, no_single_clause_case},
+               {elvis_style, no_match_in_condition},
                {elvis_style, numeric_format},
                {elvis_style, behaviour_spelling},
                {elvis_style, export_used_types},
@@ -101,6 +103,7 @@ rules(beam_files) ->
                no_import,
                no_catch_expressions,
                no_single_clause_case,
+               no_match_in_condition,
                behaviour_spelling,
                export_used_types,
                max_function_arity,

src/elvis_style.erl

@@ -11,7 +11,8 @@
          no_common_caveats_call/3, no_nested_try_catch/3, no_successive_maps/3,
          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, option/3]).
+         always_shortcircuit/3, consistent_generic_type/3, export_used_types/3,
+         no_match_in_condition/3, option/3]).
 
 -export_type([empty_rule_config/0]).
 -export_type([ignorable/0]).
@@ -25,7 +26,8 @@
               dont_repeat_yourself_config/0, no_call_config/0, no_debug_call_config/0,
               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_single_clause_case_config/0, consistent_variable_casing_config/0,
+              no_match_in_condition_config/0]).
 
 -define(INVALID_MACRO_NAME_REGEX_MSG,
         "The macro named ~p on line ~p does not respect the format "
@@ -114,6 +116,8 @@
         "Usage of catch expression on line ~p is not recommended").
 -define(NO_SINGLE_CLAUSE_CASE_MSG,
         "Case statement with a single clause found on line ~p.").
+-define(NO_MATCH_IN_CONDITION_MSG,
+        "Case statement with a match in its condition found on line ~p.").
 -define(NUMERIC_FORMAT_MSG,
         "Number ~p on line ~p does not respect the format "
         "defined by the regular expression '~p'.").
@@ -214,6 +218,7 @@ default(RuleWithEmptyDefault)
          RuleWithEmptyDefault == no_import;
          RuleWithEmptyDefault == no_catch_expressions;
          RuleWithEmptyDefault == no_single_clause_case;
+         RuleWithEmptyDefault == no_match_in_condition;
          RuleWithEmptyDefault == always_shortcircuit;
          RuleWithEmptyDefault == no_space_after_pound;
          RuleWithEmptyDefault == export_used_types;
@@ -992,8 +997,7 @@ no_throw(Config, Target, RuleConfig) ->
         fun(Node) -> lists:any(fun(T) -> is_call(Node, T) end, [{throw, 1}, {erlang, throw, 1}])
         end,
     Root = get_root(Config, Target, RuleConfig),
-    Opts = #{mode => node, traverse => content},
-    ThrowNodes = elvis_code:find(Zipper, Root, Opts),
+    ThrowNodes = elvis_code:find(Zipper, Root),
     lists:foldl(fun(ThrowNode, AccIn) ->
                    {Line, _} = ktn_code:attr(location, ThrowNode),
                    [elvis_result:new(item, ?NO_THROW_MSG, [Line]) | AccIn]
@@ -1032,8 +1036,7 @@ no_import(Config, Target, RuleConfig) ->
 no_attribute(Attribute, Msg, Config, Target, RuleConfig) ->
     Zipper = fun(Node) -> ktn_code:type(Node) =:= Attribute end,
     Root = get_root(Config, Target, RuleConfig),
-    Opts = #{mode => node, traverse => content},
-    Nodes = elvis_code:find(Zipper, Root, Opts),
+    Nodes = elvis_code:find(Zipper, Root),
     lists:map(fun(Node) ->
                  {Line, _} = ktn_code:attr(location, Node),
                  elvis_result:new(item, Msg, [Line], Line)
@@ -1048,8 +1051,7 @@ no_attribute(Attribute, Msg, Config, Target, RuleConfig) ->
                               [elvis_result:item()].
 no_catch_expressions(Config, Target, RuleConfig) ->
     Root = get_root(Config, Target, RuleConfig),
-    Opts = #{mode => node, traverse => content},
-    CatchNodes = elvis_code:find(fun is_catch_node/1, Root, Opts),
+    CatchNodes = elvis_code:find(fun is_catch_node/1, Root),
     lists:foldl(fun(CatchNode, Acc) ->
                    {Line, _Col} = ktn_code:attr(location, CatchNode),
                    [elvis_result:new(item, ?NO_CATCH_EXPRESSIONS_MSG, [Line]) | Acc]
@@ -1068,8 +1070,7 @@ is_catch_node(Node) ->
                                [elvis_result:item()].
 no_single_clause_case(Config, Target, RuleConfig) ->
     Root = get_root(Config, Target, RuleConfig),
-    Opts = #{mode => node, traverse => content},
-    CaseNodes = elvis_code:find(fun is_single_clause_case_statement/1, Root, Opts),
+    CaseNodes = elvis_code:find(fun is_single_clause_case_statement/1, Root),
     lists:map(fun(CaseNode) ->
                  {Line, _Col} = ktn_code:attr(location, CaseNode),
                  elvis_result:new(item, ?NO_SINGLE_CLAUSE_CASE_MSG, [Line], Line)
@@ -1084,6 +1085,27 @@ is_single_clause_case_statement(Node) ->
                        Clause <- ktn_code:content(SubNode)])
             == 1.
 
+-type no_match_in_condition_config() :: #{ignore => [ignorable()]}.
+
+-spec no_match_in_condition(elvis_config:config(),
+                            elvis_file:file(),
+                            no_match_in_condition_config()) ->
+                               [elvis_result:item()].
+no_match_in_condition(Config, Target, RuleConfig) ->
+    Root = get_root(Config, Target, RuleConfig),
+    CaseNodes = elvis_code:find(fun is_match_in_condition/1, Root),
+    lists:map(fun(CaseNode) ->
+                 {Line, _Col} = ktn_code:attr(location, CaseNode),
+                 elvis_result:new(item, ?NO_MATCH_IN_CONDITION_MSG, [Line], Line)
+              end,
+              CaseNodes).
+
+is_match_in_condition(Node) ->
+    ktn_code:type(Node) == case_expr andalso [] =/= elvis_code:find(fun is_match/1, Node).
+
+is_match(Node) ->
+    ktn_code:type(Node) == match orelse ktn_code:type(Node) == maybe_match.
+
 -type numeric_format_config() ::
     #{ignore => [ignorable()],
       regex => string(),
@@ -1273,7 +1295,9 @@ check_atom_names(Regex, RegexEnclosed, [AtomNode | RemainingAtomNodes], AccIn) -
     IsExceptionClass = is_exception_class(ValueAtomName),
     RE = re_compile_for_atom_type(IsEnclosed, Regex, RegexEnclosed),
     AccOut =
-        case re:run(_Subject = unicode:characters_to_list(AtomName, unicode), RE) of
+        case re:run(
+                 unicode:characters_to_list(AtomName, unicode), RE)
+        of
             _ when IsExceptionClass andalso not IsEnclosed ->
                 AccIn;
             nomatch when not IsEnclosed ->
@@ -1377,7 +1401,7 @@ check_macro_names(Regexp, [MacroNode | RemainingMacroNodes], ResultsIn) ->
     {MacroNameStripped0, MacroNameOriginal} = macro_name_from_node(MacroNode),
     MacroNameStripped = unicode:characters_to_list(MacroNameStripped0, unicode),
     ResultsOut =
-        case re:run(_Subject = MacroNameStripped, RE) of
+        case re:run(MacroNameStripped, RE) of
             nomatch ->
                 Msg = ?INVALID_MACRO_NAME_REGEX_MSG,
                 {Line, _} = ktn_code:attr(location, MacroNode),

test/examples/fail_no_match_in_condition.erl

@@ -0,0 +1,31 @@
+-module(fail_no_match_in_condition).
+
+-export([good/0, bad/0, ugly/0]).
+
+good() ->
+    case do:something() of
+        #{a := A, b := good} ->
+            {a, really, nice, A};
+        #{a := A, b := bad} ->
+            {"not", a, good, A}
+    end.
+
+bad() ->
+    case #{a := A} = do:something() of
+        #{b := good} ->
+            {a, really, nice, A};
+        #{b := bad} ->
+            {"not", a, good, A}
+    end.
+
+ugly() ->
+    case begin
+             #{a := A} = do:something(),
+             B = do:something('else', with, A)
+         end
+    of
+        #{b := good} ->
+            A;
+        #{b := bad} ->
+            B
+    end.

test/examples/pass_no_match_in_condition.erl

@@ -0,0 +1,20 @@
+-module(pass_no_match_in_condition).
+
+-export([try_body/0, good_case/0]).
+
+try_body() ->
+    try
+        It = is:fine(to, have),
+        a:try_statement(with, a, match, in, It)
+    catch
+        _ ->
+            not_bad
+    end.
+
+good_case() ->
+    case it:is(fine) of
+        To = {have, matches} ->
+            in:clause(heads, To);
+        o ->
+            ":P"
+    end.

test/style_SUITE.erl

@@ -26,7 +26,8 @@
          verify_no_author/1, verify_no_import/1, verify_no_catch_expressions/1,
          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_specs/1, verify_export_used_types/1, verify_consistent_variable_casing/1,
+         verify_no_match_in_condition/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,
@@ -80,8 +81,8 @@ groups() ->
        verify_no_successive_maps, verify_atom_naming_convention, verify_no_throw,
        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_export_used_types, verify_max_anonymous_function_arity, verify_max_function_arity,
+       verify_no_match_in_condition]}].
 
 -spec init_per_suite(config()) -> config().
 init_per_suite(Config) ->
@@ -1413,6 +1414,25 @@ verify_no_single_clause_case(Config) ->
                 [#{line_num := 6}, #{line_num := 14}, #{line_num := 16}] = R
         end.
 
+-spec verify_no_match_in_condition(config()) -> any().
+verify_no_match_in_condition(Config) ->
+    Group = proplists:get_value(group, Config, erl_files),
+    Ext = proplists:get_value(test_file_ext, Config, "erl"),
+
+    PassPath = "pass_no_match_in_condition." ++ Ext,
+    [] = elvis_core_apply_rule(Config, elvis_style, no_match_in_condition, #{}, PassPath),
+
+    FailPath = "fail_no_match_in_condition." ++ Ext,
+
+    R = elvis_core_apply_rule(Config, elvis_style, no_match_in_condition, #{}, FailPath),
+    case Group of
+        beam_files ->
+            [_, _] = R;
+        erl_files ->
+            [#{line_num := 14}, #{line_num := 22}] = R
+    end,
+    ok.
+
 -spec verify_numeric_format(config()) -> any().
 verify_numeric_format(Config) ->
     Ext = proplists:get_value(test_file_ext, Config, "erl"),