Skip to content

Commit

Permalink
Merge pull request #300 from inaka/264-opaque-records
Browse files Browse the repository at this point in the history
Fix #264: New Rule: Private Data Types
  • Loading branch information
maco authored Mar 2, 2023
2 parents 3aad6ae + 3e30af8 commit c0993ef
Show file tree
Hide file tree
Showing 9 changed files with 211 additions and 22 deletions.
3 changes: 2 additions & 1 deletion RULES.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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

Expand Down
22 changes: 22 additions & 0 deletions doc_rules/elvis_style/private_data_types.md
Original file line number Diff line number Diff line change
@@ -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}
```
6 changes: 4 additions & 2 deletions src/elvis_rulesets.erl
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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]);
Expand Down
79 changes: 63 additions & 16 deletions src/elvis_style.erl
Original file line number Diff line number Diff line change
Expand Up @@ -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]).
Expand All @@ -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 "
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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}),
Expand All @@ -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) ->
Expand All @@ -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) ->
Expand Down
20 changes: 20 additions & 0 deletions test/examples/fail_private_data_types.erl
Original file line number Diff line number Diff line change
@@ -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.
17 changes: 17 additions & 0 deletions test/examples/pass_private_data_types.erl
Original file line number Diff line number Diff line change
@@ -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.

16 changes: 16 additions & 0 deletions test/examples/pass_private_data_types2.erl
Original file line number Diff line number Diff line change
@@ -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.
17 changes: 17 additions & 0 deletions test/examples/pass_private_data_types_elvis_attr.erl
Original file line number Diff line number Diff line change
@@ -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.
53 changes: 50 additions & 3 deletions test/style_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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]).

Expand Down Expand Up @@ -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) ->
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
Expand Down

0 comments on commit c0993ef

Please sign in to comment.