Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New elvis_style rule: no_init_lists #367

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions RULES.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,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)
- [Not Use List In Init Functions](doc_rules/elvis_style/no_init_lists.md)
bormilan marked this conversation as resolved.
Show resolved Hide resolved
- [Prefer Unquoted Atoms](doc_rules/elvis_text_style/prefer_unquoted_atoms.md)

## `.gitignore` rules
Expand Down
17 changes: 17 additions & 0 deletions doc_rules/elvis_style/no_init_lists.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# No Init Lists

This warns you if you use list as a parameter in an init function in a gen_* module.
[Reasoning](https://erlangforums.com/t/args-in-gen-init-1/3169/4?u=elbrujohalcon)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This warns you if you use list as a parameter in an init function in a gen_* module.
[Reasoning](https://erlangforums.com/t/args-in-gen-init-1/3169/4?u=elbrujohalcon)
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/4?u=elbrujohalcon).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's safe to remove ?u=elbrujohalcon from the link.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right I removed and changed the last id from 4 to 5 to point the link to the right comment.


> Works on `.beam` file? Not really! (it consumes results Ok, but these might be unexpected, since
the files are pre-processed)
bormilan marked this conversation as resolved.
Show resolved Hide resolved

## Options

- None.

## Example

```erlang
{elvis_style, no_init_lists, #{}}
```
1 change: 1 addition & 0 deletions src/elvis_rulesets.erl
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,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]]);
Expand Down
40 changes: 38 additions & 2 deletions src/elvis_style.erl
Original file line number Diff line number Diff line change
Expand Up @@ -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]).
Expand All @@ -30,6 +31,10 @@
no_match_in_condition_config/0, behaviour_spelling_config/0,
param_pattern_matching_config/0, private_data_type_config/0]).

-hank([{unnecessary_function_arguments, [{no_init_lists, 3}]}]).

-define(NO_INIT_LISTS_MSG,
"Don't use a list as a parameter in 'init' function at position ~p.").
bormilan marked this conversation as resolved.
Show resolved Hide resolved
-define(INVALID_MACRO_NAME_REGEX_MSG,
"The macro named ~p on line ~p does not respect the format "
"defined by the regular expression '~p'.").
Expand Down Expand Up @@ -247,7 +252,8 @@ default(RuleWithEmptyDefault)
RuleWithEmptyDefault == always_shortcircuit;
RuleWithEmptyDefault == no_space_after_pound;
RuleWithEmptyDefault == export_used_types;
RuleWithEmptyDefault == consistent_variable_casing ->
RuleWithEmptyDefault == consistent_variable_casing;
RuleWithEmptyDefault == no_init_lists ->
#{}.

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
Expand Down Expand Up @@ -1029,6 +1035,36 @@ 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, []).

-spec no_init_lists(elvis_config:config(), elvis_file:file(), empty_rule_config()) ->
[elvis_result:item()].
no_init_lists(_Config, Target, _RuleConfig) ->
Root = get_root(#{}, Target, #{}),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
no_init_lists(_Config, Target, _RuleConfig) ->
Root = get_root(#{}, Target, #{}),
no_init_lists(Config, Target, RuleConfig) ->
Root = get_root(Config, Target, RuleConfig),

Did you need to use empty maps there for a reason, @bormilan ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right again, I just left it in...


IsFunction = fun(Node) -> ktn_code:type(Node) == function end,
FunctionNodes = elvis_code:find(IsFunction, Root),

PairFun =
fun(FunctionNode) ->
Name = ktn_code:attr(name, FunctionNode),
Location = ktn_code:attr(location, FunctionNode),
[Content] = ktn_code:content(FunctionNode),
Attributes = ktn_code:node_attr(pattern, Content),
{Name, Location, [Attr || #{type := Type} = Attr <- Attributes, Type == cons]}
end,

FunListAttributeInfos = lists:map(PairFun, FunctionNodes),

FilterFun = fun({Name, _, C}) -> length(C) > 0 andalso Name =:= init end,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure but… if C is the list of arguments… the it has to be exactly 1.

Suggested change
FilterFun = fun({Name, _, C}) -> length(C) > 0 andalso Name =:= init end,
FilterFun = fun({Name, _, Args}) -> length(Args) =:= 1 andalso Name =:= init end,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right it's a dumb fault.

FunListAttributes = lists:filter(FilterFun, FunListAttributeInfos),

ResultFun =
fun({_, Location, _}) ->
Info = [Location],
Msg = ?NO_INIT_LISTS_MSG,
elvis_result:new(item, Msg, Info, Location)
end,
lists:map(ResultFun, FunListAttributes).

-spec no_throw(elvis_config:config(), elvis_file:file(), empty_rule_config()) ->
[elvis_result:item()].
no_throw(Config, Target, RuleConfig) ->
Expand Down
9 changes: 9 additions & 0 deletions test/examples/fail_verify_no_init_lists.erl
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
-module(fail_verify_no_init_lists).

-export([start_link/1, init/1]).

start_link(AParam) ->
gen_server:start_link(?MODULE, [AParam], []).

init([_AParam]) ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should actually work… It should not fail since it's not implementing any behaviour.
If the file is not implementing a behaviour that requires an init/1 callback, we don't care how init/1 is implemented.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, to start we should just go with a few hard-coded Erlang behaviours; then we can extend as time goes by (they don't come by that often, in any case).

ok.
9 changes: 9 additions & 0 deletions test/examples/pass_verify_no_init_lists.erl
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as Brujo's before; this file is irrelevant for the rule since it's not implementing a behaviour (not explicitly, at least).

Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
-module(pass_verify_no_init_lists).

-export([start_link/0, init/1]).

start_link() ->
gen_server:start_link(?MODULE, undefined, []).

init(_) ->
ok.
14 changes: 13 additions & 1 deletion test/style_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
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_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,
Expand Down Expand Up @@ -1448,6 +1448,18 @@ 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"),

FailPath = "fail_verify_no_init_lists." ++ Ext,

[_] = elvis_core_apply_rule(Config, elvis_style, no_init_lists, #{}, FailPath),

PassPath = "pass_verify_no_init_lists." ++ Ext,

[] = elvis_core_apply_rule(Config, elvis_style, no_init_lists, #{}, PassPath).

-spec verify_no_throw(config()) -> any().
verify_no_throw(Config) ->
_Group = proplists:get_value(group, Config, erl_files),
Expand Down