diff --git a/RULES.md b/RULES.md index 979908b0..df78f2c2 100644 --- a/RULES.md +++ b/RULES.md @@ -62,6 +62,7 @@ identified with `(since ...)` for convenience purposes. - [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) +- [No Init Lists](doc_rules/elvis_style/no_init_lists.md) - [Prefer Unquoted Atoms](doc_rules/elvis_text_style/prefer_unquoted_atoms.md) ## `.gitignore` rules diff --git a/doc_rules/elvis_style/no_init_lists.md b/doc_rules/elvis_style/no_init_lists.md new file mode 100644 index 00000000..d22f0089 --- /dev/null +++ b/doc_rules/elvis_style/no_init_lists.md @@ -0,0 +1,17 @@ +# No Init Lists + +Do not use a list as the parameter for the `init/1` callback when implementing `gen_*` behaviours. +It's semantically clearer to use a tuple or a map. +[More info](https://erlangforums.com/t/args-in-gen-init-1/3169/5) + +> Works on `.beam` files? Yes! + +## Options + +- None. + +## Example + +```erlang +{elvis_style, no_init_lists, #{}} +``` diff --git a/src/elvis_rulesets.erl b/src/elvis_rulesets.erl index f5b0fd63..4d1d4714 100644 --- a/src/elvis_rulesets.erl +++ b/src/elvis_rulesets.erl @@ -97,6 +97,7 @@ rules(erl_files_strict) -> max_function_length, max_module_length, no_call, + no_init_lists, no_common_caveats_call, no_macros, state_record_and_type]]); @@ -119,6 +120,7 @@ rules(beam_files) -> no_debug_call, no_if_expression, no_import, + no_init_lists, no_match_in_condition, no_nested_try_catch, no_single_clause_case, diff --git a/src/elvis_style.erl b/src/elvis_style.erl index 2fafbdd3..456c899b 100644 --- a/src/elvis_style.erl +++ b/src/elvis_style.erl @@ -12,7 +12,8 @@ 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, param_pattern_matching/3, private_data_types/3, option/3]). + no_match_in_condition/3, param_pattern_matching/3, private_data_types/3, option/3, + no_init_lists/3]). -export_type([empty_rule_config/0]). -export_type([ignorable/0]). @@ -28,8 +29,10 @@ 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, behaviour_spelling_config/0, - param_pattern_matching_config/0, private_data_type_config/0]). + param_pattern_matching_config/0, private_data_type_config/0, no_init_lists_config/0]). +-define(NO_INIT_LISTS_MSG, + "Do not use a list as the parameter for the 'init' callback at position ~p."). -define(INVALID_MACRO_NAME_REGEX_MSG, "The macro named ~p on line ~p does not respect the format " "defined by the regular expression '~p'."). @@ -147,6 +150,9 @@ %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% -spec default(Rule :: atom()) -> DefaultRuleConfig :: term(). +default(no_init_lists) -> + #{behaviours => + [gen_server, gen_statem, gen_fsm, supervisor, supervisor_bridge, gen_event]}; default(macro_names) -> #{regex => "^[A-Z](_?[A-Z0-9]+)*$"}; default(operator_spaces) -> @@ -1112,6 +1118,79 @@ atom_naming_convention(Config, Target, RuleConfig) -> AtomNodes = elvis_code:find(fun is_atom_node/1, Root, #{traverse => all, mode => node}), check_atom_names(Regex, RegexEnclosed, AtomNodes, []). +-type no_init_lists_config() :: #{behaviours => [atom()]}. + +-spec no_init_lists(elvis_config:config(), elvis_file:file(), no_init_lists_config()) -> + [elvis_result:item()]. +no_init_lists(Config, Target, RuleConfig) -> + Root = get_root(Config, Target, RuleConfig), + + ListInitClauses = + case is_relevant_behaviour(Root, RuleConfig) of + true -> + IsInit1Function = + fun(Node) -> + ktn_code:type(Node) == function + andalso ktn_code:attr(name, Node) == init + andalso ktn_code:attr(arity, Node) == 1 + end, + + case elvis_code:find(IsInit1Function, Root) of + [] -> + []; + [Init1Fun] -> + Content = ktn_code:content(Init1Fun), + ListAttrClauses = + lists:filtermap(fun(X) -> filter_list_clause_location(X) end, Content), + case length(ListAttrClauses) =:= length(Content) of + true -> + ListAttrClauses; + false -> + [] + end + end; + false -> + [] + end, + + ResultFun = + fun(Location) -> + Info = [Location], + Msg = ?NO_INIT_LISTS_MSG, + elvis_result:new(item, Msg, Info, Location) + end, + + lists:map(ResultFun, ListInitClauses). + +is_relevant_behaviour(Root, RuleConfig) -> + ConfigBehaviors = option(behaviours, RuleConfig, no_init_lists), + IsBehaviour = fun(Node) -> ktn_code:type(Node) == behaviour end, + Behaviours = elvis_code:find(IsBehaviour, Root), + lists:any(fun(Elem) -> Elem =:= true end, + lists:map(fun(BehaviourNode) -> + lists:member( + ktn_code:attr(value, BehaviourNode), ConfigBehaviors) + end, + Behaviours)). + +filter_list_clause_location(Clause) -> + [Attribute] = ktn_code:node_attr(pattern, Clause), + case is_list_node(Attribute) of + true -> + {true, ktn_code:attr(location, Clause)}; + false -> + false + end. + +is_list_node(#{type := cons}) -> + true; +is_list_node(#{type := nil}) -> + true; +is_list_node(#{type := match, content := Content}) -> + lists:any(fun(Elem) -> is_list_node(Elem) end, Content); +is_list_node(_) -> + false. + -spec no_throw(elvis_config:config(), elvis_file:file(), empty_rule_config()) -> [elvis_result:item()]. no_throw(Config, Target, RuleConfig) -> diff --git a/test/examples/no_init_lists_examples/example_behaviour.erl b/test/examples/no_init_lists_examples/example_behaviour.erl new file mode 100644 index 00000000..e8b5fa63 --- /dev/null +++ b/test/examples/no_init_lists_examples/example_behaviour.erl @@ -0,0 +1,3 @@ +-module(example_behaviour). + +-callback example() -> atom(). diff --git a/test/examples/no_init_lists_examples/fail_no_init_lists.erl b/test/examples/no_init_lists_examples/fail_no_init_lists.erl new file mode 100644 index 00000000..d08048ff --- /dev/null +++ b/test/examples/no_init_lists_examples/fail_no_init_lists.erl @@ -0,0 +1,15 @@ +-module(fail_no_init_lists). + +-behaviour(gen_server). + +-export([start_link/1, init/1, handle_cast/2, handle_call/3]). + +start_link(AParam) -> + gen_server:start_link(?MODULE, AParam, []). + +init([_AParam]) -> + ok. + +handle_cast(_, _) -> ok. + +handle_call(_, _, _) -> ok. diff --git a/test/examples/no_init_lists_examples/fail_no_init_lists2.erl b/test/examples/no_init_lists_examples/fail_no_init_lists2.erl new file mode 100644 index 00000000..6d33a93e --- /dev/null +++ b/test/examples/no_init_lists_examples/fail_no_init_lists2.erl @@ -0,0 +1,15 @@ +-module(fail_no_init_lists2). + +-behaviour(gen_server). + +-export([start_link/1, init/1, handle_cast/2, handle_call/3]). + +start_link(AParam) -> + gen_server:start_link(?MODULE, AParam, []). + +init(_A = [1, 2, 3]) -> + ok. + +handle_cast(_, _) -> ok. + +handle_call(_, _, _) -> ok. diff --git a/test/examples/no_init_lists_examples/fail_no_init_lists3.erl b/test/examples/no_init_lists_examples/fail_no_init_lists3.erl new file mode 100644 index 00000000..e091a1ac --- /dev/null +++ b/test/examples/no_init_lists_examples/fail_no_init_lists3.erl @@ -0,0 +1,21 @@ +-module(fail_no_init_lists3). + +-behaviour(gen_server). + +-export([start_link/1, init/1, handle_cast/2, handle_call/3]). + +start_link(AParam) -> + gen_server:start_link(?MODULE, AParam, []). + +init([_]) -> + ok; + +init(_A = [1, 2, 3]) -> + ok; + +init([undefined]) -> + ok. + +handle_cast(_, _) -> ok. + +handle_call(_, _, _) -> ok. diff --git a/test/examples/no_init_lists_examples/fail_no_init_lists4.erl b/test/examples/no_init_lists_examples/fail_no_init_lists4.erl new file mode 100644 index 00000000..df738c39 --- /dev/null +++ b/test/examples/no_init_lists_examples/fail_no_init_lists4.erl @@ -0,0 +1,15 @@ +-module(fail_no_init_lists4). + +-behaviour(gen_server). +-behaviour(example_behaviour). + +-export([init/1, handle_cast/2, handle_cast/3, handle_call/3]). +-export([example/0]). + +init([]) -> {error, "should not be a list"}. + +handle_cast(_, _) -> ok. +handle_cast(_, _, _) -> ok. +handle_call(_, _, _) -> ok. + +example() -> ok. diff --git a/test/examples/no_init_lists_examples/fail_no_init_lists5.erl b/test/examples/no_init_lists_examples/fail_no_init_lists5.erl new file mode 100644 index 00000000..07f34ca2 --- /dev/null +++ b/test/examples/no_init_lists_examples/fail_no_init_lists5.erl @@ -0,0 +1,12 @@ +-module(fail_no_init_lists5). + +-behaviour(gen_fsm). + +-export([init/1, handle_sync_event/4, handle_event/3]). + +init([]) -> {error, "Don't use list for init/1"}. + +handle_sync_event(_, _, _, _) -> ok. + +handle_event(_, _, _) -> ok. + diff --git a/test/examples/no_init_lists_examples/fail_no_init_lists6.erl b/test/examples/no_init_lists_examples/fail_no_init_lists6.erl new file mode 100644 index 00000000..38cbd9be --- /dev/null +++ b/test/examples/no_init_lists_examples/fail_no_init_lists6.erl @@ -0,0 +1,7 @@ +-module(fail_no_init_lists6). + +-behaviour(supervisor). + +-export([init/1]). + +init([]) -> {error, "Don't use list for init/1"}. diff --git a/test/examples/no_init_lists_examples/fail_no_init_lists7.erl b/test/examples/no_init_lists_examples/fail_no_init_lists7.erl new file mode 100644 index 00000000..6a0e4f33 --- /dev/null +++ b/test/examples/no_init_lists_examples/fail_no_init_lists7.erl @@ -0,0 +1,9 @@ +-module(fail_no_init_lists7). + +-behaviour(supervisor_bridge). + +-export([init/1, terminate/2]). + +init([]) -> {error, "Don't use list for init/1"}. + +terminate(_, _) -> ok. diff --git a/test/examples/no_init_lists_examples/fail_no_init_lists8.erl b/test/examples/no_init_lists_examples/fail_no_init_lists8.erl new file mode 100644 index 00000000..3c48ccfe --- /dev/null +++ b/test/examples/no_init_lists_examples/fail_no_init_lists8.erl @@ -0,0 +1,11 @@ +-module(fail_no_init_lists8). + +-behaviour(gen_event). + +-export([init/1, handle_event/2, handle_call/2]). + +init([]) -> {error, "Don't use list for init/1"}. + +handle_event(_, _) -> ok. + +handle_call(_, _) -> ok. diff --git a/test/examples/no_init_lists_examples/pass_no_init_lists.erl b/test/examples/no_init_lists_examples/pass_no_init_lists.erl new file mode 100644 index 00000000..c9a0ea69 --- /dev/null +++ b/test/examples/no_init_lists_examples/pass_no_init_lists.erl @@ -0,0 +1,21 @@ +-module(pass_no_init_lists). + +-behaviour(gen_server). + +-export([start_link/0, init/1, handle_cast/2, handle_call/3]). + +start_link() -> + gen_server:start_link(?MODULE, 1, []). + +init(1) -> + ok; + +init([undefined]) -> + ok; + +init([_]) -> + ok. + +handle_cast(_, _) -> ok. + +handle_call(_, _, _) -> ok. diff --git a/test/examples/no_init_lists_examples/pass_no_init_lists2.erl b/test/examples/no_init_lists_examples/pass_no_init_lists2.erl new file mode 100644 index 00000000..8ab684b4 --- /dev/null +++ b/test/examples/no_init_lists_examples/pass_no_init_lists2.erl @@ -0,0 +1,18 @@ +-module(pass_no_init_lists2). + +-behaviour(gen_server). + +-export([start_link/0, init/1, init/2, handle_cast/2, handle_call/3]). + +start_link() -> + gen_server:start_link(?MODULE, #{a => map}, []). + +init(#{}) -> + ok. + +init([_], undefined) -> + ok. + +handle_cast(_, _) -> ok. + +handle_call(_, _, _) -> ok. diff --git a/test/examples/no_init_lists_examples/pass_no_init_lists3.erl b/test/examples/no_init_lists_examples/pass_no_init_lists3.erl new file mode 100644 index 00000000..dd9fc4f7 --- /dev/null +++ b/test/examples/no_init_lists_examples/pass_no_init_lists3.erl @@ -0,0 +1,18 @@ +-module(pass_no_init_lists3). + +-behaviour(gen_server). + +-export([start_link/0, init/0, init/1, handle_cast/2, handle_call/3]). + +start_link() -> + gen_server:start_link(?MODULE, {1, 2}, []). + +init({1, 2}) -> + ok. + +init() -> + ok. + +handle_cast(_, _) -> ok. + +handle_call(_, _, _) -> ok. diff --git a/test/examples/no_init_lists_examples/pass_no_init_lists4.erl b/test/examples/no_init_lists_examples/pass_no_init_lists4.erl new file mode 100644 index 00000000..288e3e98 --- /dev/null +++ b/test/examples/no_init_lists_examples/pass_no_init_lists4.erl @@ -0,0 +1,8 @@ +-module(pass_no_init_lists4). + +-export([start_link/1, init/1]). + +start_link(AParam) -> + gen_server:start_link(?MODULE, [AParam], []). + +init([_AParam]) -> ok. diff --git a/test/examples/no_init_lists_examples/pass_no_init_lists5.erl b/test/examples/no_init_lists_examples/pass_no_init_lists5.erl new file mode 100644 index 00000000..c12e4f1f --- /dev/null +++ b/test/examples/no_init_lists_examples/pass_no_init_lists5.erl @@ -0,0 +1,8 @@ +-module(pass_no_init_lists5). + +-behaviour(example_behaviour). + +-export([init/1, example/0]). + +init([]) -> {error, "can be a list"}. +example() -> ok. diff --git a/test/style_SUITE.erl b/test/style_SUITE.erl index 89154d98..54649661 100644 --- a/test/style_SUITE.erl +++ b/test/style_SUITE.erl @@ -26,7 +26,7 @@ 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_param_pattern_matching/1, - verify_private_data_types/1, verify_unquoted_atoms/1]). + verify_private_data_types/1, verify_unquoted_atoms/1, verify_no_init_lists/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, @@ -1521,6 +1521,42 @@ verify_atom_naming_convention(Config) -> FailPath) end. +-spec verify_no_init_lists(config()) -> any(). +verify_no_init_lists(Config) -> + Ext = proplists:get_value(test_file_ext, Config, "erl"), + + ExamplesDir = "no_init_lists_examples/", + FailPath = ExamplesDir ++ "fail_no_init_lists." ++ Ext, + FailPath2 = ExamplesDir ++ "fail_no_init_lists2." ++ Ext, + FailPath3 = ExamplesDir ++ "fail_no_init_lists3." ++ Ext, + FailPath4 = ExamplesDir ++ "fail_no_init_lists4." ++ Ext, + FailPath5 = ExamplesDir ++ "fail_no_init_lists5." ++ Ext, + FailPath6 = ExamplesDir ++ "fail_no_init_lists6." ++ Ext, + FailPath7 = ExamplesDir ++ "fail_no_init_lists7." ++ Ext, + FailPath8 = ExamplesDir ++ "fail_no_init_lists8." ++ Ext, + + [_] = elvis_core_apply_rule(Config, elvis_style, no_init_lists, #{}, FailPath), + [_] = elvis_core_apply_rule(Config, elvis_style, no_init_lists, #{}, FailPath2), + [_, _, _] = elvis_core_apply_rule(Config, elvis_style, no_init_lists, #{}, FailPath3), + [_] = elvis_core_apply_rule(Config, elvis_style, no_init_lists, #{}, FailPath4), + [_] = elvis_core_apply_rule(Config, elvis_style, no_init_lists, #{}, FailPath5), + [_] = elvis_core_apply_rule(Config, elvis_style, no_init_lists, #{}, FailPath6), + [_] = elvis_core_apply_rule(Config, elvis_style, no_init_lists, #{}, FailPath7), + [_] = elvis_core_apply_rule(Config, elvis_style, no_init_lists, #{}, FailPath8), + + PassPath = ExamplesDir ++ "pass_no_init_lists." ++ Ext, + PassPath2 = ExamplesDir ++ "pass_no_init_lists2." ++ Ext, + PassPath3 = ExamplesDir ++ "pass_no_init_lists3." ++ Ext, + PassPath4 = ExamplesDir ++ "pass_no_init_lists4." ++ Ext, + PassPath5 = ExamplesDir ++ "pass_no_init_lists5." ++ Ext, + + [] = elvis_core_apply_rule(Config, elvis_style, no_init_lists, #{}, PassPath), + [] = elvis_core_apply_rule(Config, elvis_style, no_init_lists, #{}, PassPath2), + [] = elvis_core_apply_rule(Config, elvis_style, no_init_lists, #{}, PassPath3), + [] = elvis_core_apply_rule(Config, elvis_style, no_init_lists, #{}, PassPath4), + [] = elvis_core_apply_rule(Config, elvis_style, no_init_lists, #{}, PassPath5), + ok. + -spec verify_no_throw(config()) -> any(). verify_no_throw(Config) -> _Group = proplists:get_value(group, Config, erl_files),