diff --git a/RULES.md b/RULES.md index 8b8f2045..808aea39 100644 --- a/RULES.md +++ b/RULES.md @@ -16,6 +16,7 @@ identified with `(since ...)` for convenience purposes. - [Consistent Generic Type](doc_rules/elvis_style/consistent_generic_type.md) - [Consistent Variable Casing](doc_rules/elvis_style/consistent_variable_casing.md) - [Don't Repeat Yourself](doc_rules/elvis_style/dont_repeat_yourself.md) +- [Export Used Types](doc_rules/elvis_style/export_used_types.md) - [Function Naming Convention](doc_rules/elvis_style/function_naming_convention.md) - [God Modules](doc_rules/elvis_style/god_modules.md) - [Invalid Dynamic Calls](doc_rules/elvis_style/invalid_dynamic_call.md) @@ -56,10 +57,10 @@ identified with `(since ...)` for convenience purposes. - [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) +- [Private Data Types](doc_rules/elvis_style/private_data_types.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) -- [Export Used Types](doc_rules/elvis_style/export_used_types.md) ## Project rules diff --git a/doc_rules/elvis_style/private_data_types.md b/doc_rules/elvis_style/private_data_types.md new file mode 100644 index 00000000..1db108aa --- /dev/null +++ b/doc_rules/elvis_style/private_data_types.md @@ -0,0 +1,22 @@ +# Private Data Types + +(since [3.0.0](https://github.com/inaka/elvis_core/releases/tag/3.0.0)) + +Exporting functions' internally-defined data types, in order to consume those +types externally, results in tightly-coupled code. Modules should be responsible +for defining their own internal data types. If these are needed outside the +modules, they should be made +[opaque](https://www.erlang.org/doc/reference_manual/opaques.html). + +> "Works on `.beam` file? Yes!" + +## Options + +- `apply_to :: [record | map | tuple]` + - default: `record`. + +## Example + +```erlang +{elvis_style, private_data_types} +``` diff --git a/src/elvis_rulesets.erl b/src/elvis_rulesets.erl index 1ae39b22..4f82b170 100644 --- a/src/elvis_rulesets.erl +++ b/src/elvis_rulesets.erl @@ -81,7 +81,8 @@ rules(erl_files) -> {elvis_style, export_used_types}, {elvis_style, max_function_arity}, {elvis_style, max_anonymous_function_arity}, - {elvis_style, param_pattern_matching}]); + {elvis_style, param_pattern_matching}, + {elvis_style, private_data_types}]); rules(beam_files) -> lists:map(fun(Rule) -> {elvis_style, Rule, elvis_style:default(Rule)} end, [nesting_level, @@ -109,7 +110,8 @@ rules(beam_files) -> export_used_types, max_function_arity, max_anonymous_function_arity, - param_pattern_matching]); + param_pattern_matching, + private_data_types]); 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 d48b827e..e54ddf69 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, param_pattern_matching/3, option/3]). + no_match_in_condition/3, param_pattern_matching/3, private_data_types/3, option/3]). -export_type([empty_rule_config/0]). -export_type([ignorable/0]). @@ -28,7 +28,7 @@ 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]). + param_pattern_matching_config/0, private_data_type_config/0]). -define(INVALID_MACRO_NAME_REGEX_MSG, "The macro named ~p on line ~p does not respect the format " @@ -135,6 +135,9 @@ "Found usage of type ~p/0 on line ~p. Please use ~p/0, instead."). -define(EXPORT_USED_TYPES_MSG, "Type ~p/~p, defined on line ~p, is used by an exported function but not exported itself"). +-define(PRIVATE_DATA_TYPES_MSG, + "Private data type ~p/~p, defined on line ~p, is exported. Either don't export it or make " + "it an opaque type."). %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% %% Default values @@ -204,6 +207,8 @@ default(param_pattern_matching) -> #{side => right}; default(consistent_generic_type) -> #{preferred_type => term}; +default(private_data_types) -> + #{apply_to => [record]}; default(RuleWithEmptyDefault) when RuleWithEmptyDefault == macro_module_names; RuleWithEmptyDefault == no_macros; @@ -1265,8 +1270,6 @@ always_shortcircuit(Config, Target, RuleConfig) -> export_used_types(Config, Target, RuleConfig) -> TreeRootNode = get_root(Config, Target, RuleConfig), ExportedFunctions = elvis_code:exported_functions(TreeRootNode), - AllTypes = - elvis_code:find(fun is_type_attribute/1, TreeRootNode, #{traverse => all, mode => node}), ExportedTypes = elvis_code:exported_types(TreeRootNode), SpecNodes = elvis_code:find(fun is_spec_attribute/1, TreeRootNode, #{traverse => all, mode => node}), @@ -1282,23 +1285,14 @@ export_used_types(Config, Target, RuleConfig) -> elvis_code:find(fun(Node) -> ktn_code:type(Node) =:= user_type end, Spec, #{mode => node, traverse => all}), + % yes, on a -type line, the arity is based on `args`, but on + % a -spec line, it's based on `content` [{Name, length(Vars)} || #{attrs := #{name := Name}, content := Vars} <- Types] end, ExportedSpecs)), UnexportedUsedTypes = lists:subtract(UsedTypes, ExportedTypes), - - % Get line numbers for all types - LineNumbers = - lists:foldl(fun (#{attrs := #{location := {Line, _}, name := Name}, - node_attrs := #{args := Args}}, - Acc) -> - maps:put({Name, length(Args)}, Line, Acc); - (_, Acc) -> - Acc - end, - #{}, - AllTypes), + LineNumbers = map_type_declarations_to_line_numbers(TreeRootNode), % Report lists:map(fun({Name, Arity} = Info) -> @@ -1307,10 +1301,63 @@ export_used_types(Config, Target, RuleConfig) -> end, UnexportedUsedTypes). +get_type_of_type(#{type := type_attr, + node_attrs := #{type := #{attrs := #{name := TypeOfType}}}}) -> + TypeOfType; +get_type_of_type(_) -> + undefined. + +-type data_type() :: record | map | tuple. +-type private_data_type_config() :: #{apply_to => [data_type()]}. + +-spec private_data_types(elvis_config:config(), + elvis_file:file(), + private_data_type_config()) -> + [elvis_result:item()]. +private_data_types(Config, Target, RuleConfig) -> + TypesToCheck = option(apply_to, RuleConfig, private_data_types), + TreeRootNode = get_root(Config, Target, RuleConfig), + ExportedTypes = elvis_code:exported_types(TreeRootNode), + LineNumbers = map_type_declarations_to_line_numbers(TreeRootNode), + + PublicDataTypes = public_data_types(TypesToCheck, TreeRootNode, ExportedTypes), + + lists:map(fun({Name, Arity} = Info) -> + Line = maps:get(Info, LineNumbers, unknown), + elvis_result:new(item, ?PRIVATE_DATA_TYPES_MSG, [Name, Arity, Line], Line) + end, + PublicDataTypes). + +public_data_types(TypesToCheck, TreeRootNode, ExportedTypes) -> + Fun = fun(Node) -> lists:member(get_type_of_type(Node), TypesToCheck) end, + Types = + [name_arity_from_type_line(Node) + || Node <- elvis_code:find(Fun, TreeRootNode, #{traverse => all, mode => node})], + lists:filter(fun({Name, Arity}) -> lists:member({Name, Arity}, ExportedTypes) end, Types). + %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% %% Private %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% +-spec name_arity_from_type_line(ktn_code:tree_node()) -> {atom(), integer()}. +name_arity_from_type_line(#{attrs := #{name := Name}, node_attrs := #{args := Args}}) -> + {Name, length(Args)}. + +-spec map_type_declarations_to_line_numbers(ktn_code:tree_node()) -> + #{{atom(), number()} => number()}. +map_type_declarations_to_line_numbers(TreeRootNode) -> + AllTypes = + elvis_code:find(fun is_type_attribute/1, TreeRootNode, #{traverse => all, mode => node}), + lists:foldl(fun (#{attrs := #{location := {Line, _}, name := Name}, + node_attrs := #{args := Args}}, + Acc) -> + maps:put({Name, length(Args)}, Line, Acc); + (_, Acc) -> + Acc + end, + #{}, + AllTypes). + specific_or_default(same, Regex) -> Regex; specific_or_default(RegexEnclosed, _Regex) -> diff --git a/test/examples/fail_private_data_types.erl b/test/examples/fail_private_data_types.erl new file mode 100644 index 00000000..069467e8 --- /dev/null +++ b/test/examples/fail_private_data_types.erl @@ -0,0 +1,20 @@ +-module(fail_private_data_types). + +-record(my_rec, {a :: integer(), b :: integer(), c :: integer()}). + +-type my_rec() :: #my_rec{}. +-type my_tuple() :: {bitstring(), bitstring()}. +-type my_map() :: map(). + +-export_type([my_rec/0]). +-export_type([my_tuple/0]). +-export_type([my_map/0]). + +-export([hello/0]). + +-spec hello() -> ok. +hello() -> + my_fun(#my_rec{a = 1, b = 2, c = 3}, {<<"hello">>, <<"world">>}, #{a => 1}). + +-spec my_fun(my_rec(), my_tuple(), my_map()) -> ok. +my_fun(_Rec, _Tup, _Map) -> ok. diff --git a/test/examples/pass_private_data_types.erl b/test/examples/pass_private_data_types.erl new file mode 100644 index 00000000..bcc8c247 --- /dev/null +++ b/test/examples/pass_private_data_types.erl @@ -0,0 +1,17 @@ +-module(pass_private_data_types). + +-record(my_rec, {a :: integer(), b :: integer(), c :: integer()}). + +-opaque my_rec() :: #my_rec{}. + +-export_type([my_rec/0]). + +-export([hello/0]). + +-spec hello() -> ok. +hello() -> + my_fun(#my_rec{a = 1, b = 2, c = 3}). + +-spec my_fun(my_rec()) -> ok. +my_fun(_Rec) -> ok. + diff --git a/test/examples/pass_private_data_types2.erl b/test/examples/pass_private_data_types2.erl new file mode 100644 index 00000000..73712733 --- /dev/null +++ b/test/examples/pass_private_data_types2.erl @@ -0,0 +1,16 @@ +-module(pass_private_data_types2). + +-record(my_rec, {a :: integer(), b :: integer(), c :: integer()}). + +-type my_rec() :: #my_rec{}. +-type my_tuple() :: {bitstring(), bitstring()}. +-type my_map() :: map(). + +-export([hello/0]). + +-spec hello() -> ok. +hello() -> + my_fun(#my_rec{a = 1, b = 2, c = 3}, {<<"hello">>, <<"world">>}, #{a => 1}). + +-spec my_fun(my_rec(), my_tuple(), my_map()) -> ok. +my_fun(_Rec, _Tup, _Map) -> ok. diff --git a/test/examples/pass_private_data_types_elvis_attr.erl b/test/examples/pass_private_data_types_elvis_attr.erl new file mode 100644 index 00000000..107139ca --- /dev/null +++ b/test/examples/pass_private_data_types_elvis_attr.erl @@ -0,0 +1,17 @@ +-module(pass_private_data_types_elvis_attr). +-elvis([{elvis_style, private_data_types, #{apply_to => [record, tuple]}}]). + +-record(my_rec, {a :: integer(), b :: integer(), c :: integer()}). + +-type my_rec() :: #my_rec{}. +-type my_tuple() :: {bitstring(), bitstring()}. +-type my_map() :: map(). + +-export([hello/0]). + +-spec hello() -> ok. +hello() -> + my_fun(#my_rec{a = 1, b = 2, c = 3}, {<<"hello">>, <<"world">>}, #{a => 1}). + +-spec my_fun(my_rec(), my_tuple(), my_map()) -> ok. +my_fun(_Rec, _Tup, _Map) -> ok. diff --git a/test/style_SUITE.erl b/test/style_SUITE.erl index a1070325..e89b3021 100644 --- a/test/style_SUITE.erl +++ b/test/style_SUITE.erl @@ -23,7 +23,8 @@ 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_param_pattern_matching/1]). + verify_no_match_in_condition/1, verify_param_pattern_matching/1, + verify_private_data_types/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, @@ -39,7 +40,8 @@ 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_param_pattern_matching/1]). + verify_elvis_attr_behaviour_spelling/1, verify_elvis_attr_param_pattern_matching/1, + verify_elvis_attr_private_data_types/1]). %% Non-rule -export([results_are_ordered_by_line/1, oddities/1]). @@ -78,7 +80,8 @@ 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_behaviour_spelling, verify_param_pattern_matching]}]. + verify_no_match_in_condition, verify_behaviour_spelling, verify_param_pattern_matching, + verify_private_data_types]}]. -spec init_per_suite(config()) -> config(). init_per_suite(Config) -> @@ -1581,6 +1584,46 @@ verify_export_used_types(Config) -> [#{line_num := 3}] = elvis_core_apply_rule(Config, elvis_style, export_used_types, #{}, PathFail). +-spec verify_private_data_types(config()) -> any(). +verify_private_data_types(Config) -> + Ext = proplists:get_value(test_file_ext, Config, "erl"), + PathPass = "pass_private_data_types2." ++ Ext, + [] = + elvis_core_apply_rule(Config, + elvis_style, + private_data_types, + #{apply_to => [record, map, tuple]}, + PathPass), + PathPass2 = "pass_private_data_types2." ++ Ext, + [] = + elvis_core_apply_rule(Config, + elvis_style, + private_data_types, + #{apply_to => [record, map, tuple]}, + PathPass2), + % Default applies only to records + PathFail = "fail_private_data_types." ++ Ext, + [#{line_num := _}] = + elvis_core_apply_rule(Config, elvis_style, private_data_types, #{}, PathFail), + [#{line_num := _}] = + elvis_core_apply_rule(Config, + elvis_style, + private_data_types, + #{apply_to => [tuple]}, + PathFail), + [#{line_num := _}] = + elvis_core_apply_rule(Config, + elvis_style, + private_data_types, + #{apply_to => [map]}, + PathFail), + [#{line_num := _}, #{line_num := _}, #{line_num := _}] = + elvis_core_apply_rule(Config, + elvis_style, + private_data_types, + #{apply_to => [record, tuple, map]}, + PathFail). + -spec results_are_ordered_by_line(config()) -> true. results_are_ordered_by_line(_Config) -> ElvisConfig = elvis_test_utils:config(), @@ -1717,6 +1760,10 @@ verify_elvis_attr_behaviour_spelling(Config) -> verify_elvis_attr_param_pattern_matching(Config) -> verify_elvis_attr(Config, "pass_param_pattern_matching_elvis_attr"). +-spec verify_elvis_attr_private_data_types(config()) -> true. +verify_elvis_attr_private_data_types(Config) -> + verify_elvis_attr(Config, "pass_private_data_types_elvis_attr"). + %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% %% Private %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%