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.

Sign up for GitHub

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

stdlib: create an init function for records with complex default values #9373

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+158 −46
Open

stdlib: create an init function for records with complex default values #9373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
544d835
stdlib: create an init function for records with complex default values
frazze-jobb Jan 30, 2025
5bd3a4d
stdlib: fix review comments
frazze-jobb Feb 3, 2025
fc096c6
fix: throw warnings when record fields use previously bound variables
frazze-jobb Feb 4, 2025
5e9f9b8
fix: output "record default value" instead of function rec_init$^
frazze-jobb Feb 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions lib/stdlib/src/erl_error.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,10 +536,20 @@ location(L) ->
sep(1, S) -> S;
sep(_, S) -> [$\n | S].

is_rec_init(F) when is_atom(F) ->
case atom_to_binary(F) of
<<"rec_init$^", _/binary>> -> true;
_ -> false
end;
is_rec_init(_) -> false.

origin(1, M, F, A) ->
case is_op({M, F}, n_args(A)) of
{yes, F} -> <<"in operator ">>;
no -> <<"in function ">>
no -> case is_rec_init(F) of
true -> <<"in record">>;
_ -> <<"in function ">>
end
end;
origin(_N, _M, _F, _A) ->
<<"in call from">>.
Expand Down Expand Up @@ -625,7 +635,10 @@ printable_list(_, As) ->
io_lib:printable_list(As).

mfa_to_string(M, F, A, Enc) ->
io_lib:fwrite(<<"~ts/~w">>, [mf_to_string({M, F}, A, Enc), A]).
case is_rec_init(F) of
true -> <<"default value">>;
_ -> io_lib:fwrite(<<"~ts/~w">>, [mf_to_string({M, F}, A, Enc), A])
end.

mf_to_string({M, F}, A, Enc) ->
case erl_internal:bif(M, F, A) of
Expand Down
76 changes: 73 additions & 3 deletions lib/stdlib/src/erl_expand_records.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ Section [The Abstract Format](`e:erts:absform.md`) in ERTS User's Guide.
strict_ra=[], % Strict record accesses
checked_ra=[], % Successfully accessed records
dialyzer=false, % Compiler option 'dialyzer'
rec_init_count=0, % Number of generated record init functions
new_forms=#{}, % New forms
strict_rec_tests=true :: boolean()
}).

Expand Down Expand Up @@ -95,6 +97,12 @@ forms([{function,Anno,N,A,Cs0} | Fs0], St0) ->
forms([F | Fs0], St0) ->
{Fs,St} = forms(Fs0, St0),
{[F | Fs], St};
forms([], #exprec{new_forms=FsN}=St) ->
{[{'function', Anno,
maps:get(Def,FsN),
0,
[{'clause', Anno, [], [], [Def]}]}
|| {_,Anno,_}=Def <- maps:keys(FsN)], St};
forms([], St) -> {[],St}.

clauses([{clause,Anno,H0,G0,B0} | Cs0], St0) ->
Expand Down Expand Up @@ -262,6 +270,42 @@ not_a_tuple({op,_,_,_}) -> true;
not_a_tuple({op,_,_,_,_}) -> true;
not_a_tuple(_) -> false.

traverse_af(AF, Fun) ->
traverse_af(AF, Fun, []).
traverse_af(AF, Fun, Acc) when is_list(AF) ->
[ traverse_af(Ast, Fun, Fun(Ast,Acc)) || Ast <- AF];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[ traverse_af(Ast, Fun, Fun(Ast,Acc)) || Ast <- AF];
[traverse_af(Ast, Fun, Fun(Ast,Acc)) || Ast <- AF];

traverse_af(AF, Fun, Acc) when is_tuple(AF) ->
%% Iterate each tuple element, if the element is an AF, traverse it
[[(fun (List) when is_list(List) ->
traverse_af(List, Fun, Acc);
(Tuple) when is_tuple(Tuple)->
case erl_anno:is_anno(Tuple) of
true -> [];
false -> traverse_af(Tuple, Fun, Fun(Tuple,Acc))
end;
(_) -> []
end)(Term) || Term <- tuple_to_list(AF)],Acc];
traverse_af(_, _, Acc) -> Acc.
save_vars({var, _, Var}, _) -> Var;
save_vars(_, Acc) -> Acc.
free_variables(AF, Acc) ->
try
_=traverse_af(AF, fun free_variables1/2, Acc),
false
catch
throw:{error,unsafe_variable} -> true
end.
free_variables1({'fun',_anno,{clauses, _}}, Acc) ->
{function,Acc}; %% tag that we are in a 'fun' now that can define new variables
free_variables1({clause,_anno,Pattern,_guards,_body}, {function,Acc}) ->
lists:flatten(traverse_af(Pattern, fun save_vars/2, [])++Acc);
free_variables1({var, _, Var}, Acc) ->
case lists:member(Var, Acc) of
true -> Acc;
false -> throw({error, unsafe_variable})
end;
free_variables1(_, Acc) -> Acc.

record_test_in_body(Anno, Expr, Name, St0) ->
%% As Expr may have side effects, we must evaluate it
%% first and bind the value to a new variable.
Expand Down Expand Up @@ -335,9 +379,35 @@ expr({record_index,Anno,Name,F}, St) ->
expr(I, St);
expr({record,Anno0,Name,Is}, St) ->
Anno = mark_record(Anno0, St),
expr({tuple,Anno,[{atom,Anno0,Name} |
record_inits(record_fields(Name, Anno0, St), Is)]},
St);
R_init = [{atom,Anno,Name} |
record_inits(record_fields(Name, Anno0, St), Is)],
Vars = lists:flatten(traverse_af(Is, fun save_vars/2)),
%% If R_init contains free variables that was not bound via Is
case free_variables(R_init, Vars) of
true ->
IsUndefined = [{RF, AnnoRF, Field, {atom, AnnoRF, 'undefined'}} || {record_field=RF, AnnoRF, Field, _} <- Is],
R_default_init = [{atom,Anno,Name} |
record_inits(record_fields(Name, Anno0, St),IsUndefined)],
%% add a function to the module that returns the
%% initialized record, we generate different init functions
%% depending on which fields that will override the default value
frazze-jobb marked this conversation as resolved.
Show resolved Hide resolved
{Def, St1} = expr({tuple,Anno,R_default_init},St),
Map=St1#exprec.new_forms,
{FName,St2} = case maps:get(Def, Map, undefined) of
undefined->
C=St1#exprec.rec_init_count,
NewName=list_to_atom("rec_init$^" ++ integer_to_list(C)),
{NewName, St1#exprec{rec_init_count=C+1, new_forms=Map#{Def=>NewName}}};
OldName -> {OldName,St1}
end,
%% replace the init record expression with a call expression
%% to the newly added function and a record update
expr({record, Anno0, {call,Anno,{atom,Anno,FName},[]}, Name, Is},St2);
false ->
%% No free variables means that we can just
%% output the record as a tuple.
expr({tuple,Anno,R_init},St)
end;
expr({record_field,_A,R,Name,F}, St) ->
Anno = erl_parse:first_anno(R),
get_record_field(Anno, R, F, Name, St);
Expand Down
45 changes: 20 additions & 25 deletions lib/stdlib/src/erl_lint.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,6 @@ value_option(Flag, Default, On, OnVal, Off, OffVal, Opts) ->
errors=[] :: [{file:filename(),error_info()}], %Current errors
warnings=[] :: [{file:filename(),error_info()}], %Current warnings
file = "" :: string(), %From last file attribute
recdef_top=false :: boolean(), %true in record initialisation
%outside any fun or lc
xqlc= false :: boolean(), %true if qlc.hrl included
called= [] :: [{fa(),anno()}], %Called functions
fun_used_vars = undefined %Funs used vars
Expand Down Expand Up @@ -451,6 +449,9 @@ format_error_1({unbound_var,V,GuessV}) ->
format_error_1({unsafe_var,V,{What,Where}}) ->
{~"variable ~w unsafe in ~w ~s",
[V,What,format_where(Where)]};
format_error_1({exported_var,V,{{record_field,R,F},Where}}) ->
{~"variable ~w exported from #~w.~w ~s",
[V,R,F,format_where(Where)]};
format_error_1({exported_var,V,{What,Where}}) ->
{~"variable ~w exported from ~w ~s",
[V,What,format_where(Where)]};
Expand All @@ -469,8 +470,6 @@ format_error_1({shadowed_var,V,In}) ->
{~"variable ~w shadowed in ~w", [V,In]};
format_error_1({unused_var, V}) ->
{~"variable ~w is unused", [V]};
format_error_1({variable_in_record_def,V}) ->
{~"variable ~w in record definition", [V]};
format_error_1({stacktrace_guard,V}) ->
{~"stacktrace variable ~w must not be used in a guard", [V]};
format_error_1({stacktrace_bound,V}) ->
Expand Down Expand Up @@ -3073,7 +3072,7 @@ record_def(Anno, Name, Fs0, St0) ->
case is_map_key(Name, St0#lint.records) of
true -> add_error(Anno, {redefine_record,Name}, St0);
false ->
{Fs1,St1} = def_fields(normalise_fields(Fs0), Name, St0),
{Fs1,_,St1} = def_fields(normalise_fields(Fs0), Name, St0),
St2 = St1#lint{records=maps:put(Name, {Anno,Fs1},
St1#lint.records)},
Types = [T || {typed_record_field, _, T} <- Fs0],
Expand All @@ -3086,27 +3085,30 @@ record_def(Anno, Name, Fs0, St0) ->
%% record and set State.

def_fields(Fs0, Name, St0) ->
foldl(fun ({record_field,Af,{atom,Aa,F},V}, {Fs,St}) ->
foldl(fun ({record_field,Af,{atom,Aa,F},V}, {Fs,Vt0,St}) ->
case exist_field(F, Fs) of
true -> {Fs,add_error(Af, {redefine_field,Name,F}, St)};
true -> {Fs,Vt0,add_error(Af, {redefine_field,Name,F}, St)};
false ->
St1 = St#lint{recdef_top = true},
{_,St2} = expr(V, [], St1),
{Vt1,St2} = expr(V, Vt0, St),
%% Everything that was bound is exported to the next field
Vt2 = lists:map(
fun({Var,{bound,Usage,Ls}}) ->
{Var, {{export, {{'record_field', Name, F}, Af}}, Usage,Ls}};
(X) -> X end, Vt1),
%% Warnings and errors found are kept, but
%% updated calls, records, etc. are discarded.
St3 = St1#lint{warnings = St2#lint.warnings,
St3 = St#lint{warnings = St2#lint.warnings,
errors = St2#lint.errors,
called = St2#lint.called,
recdef_top = false},
called = St2#lint.called},
%% This is one way of avoiding a loop for
%% "recursive" definitions.
NV = case St2#lint.errors =:= St1#lint.errors of
NV = case St2#lint.errors =:= St#lint.errors of
true -> V;
false -> {atom,Aa,undefined}
end,
{[{record_field,Af,{atom,Aa,F},NV}|Fs],St3}
{[{record_field,Af,{atom,Aa,F},NV}|Fs],Vt2,St3}
end
end, {[],St0}, Fs0).
end, {[],[],St0}, Fs0).

%% normalise_fields([RecDef]) -> [Field].
%% Normalise the field definitions to always have a default value. If
Expand Down Expand Up @@ -4067,10 +4069,7 @@ comprehension_expr(E, Vt, St) ->
%% in ShadowVarTable (these are local variables that are not global variables).

lc_quals(Qs, Vt0, St0) ->
OldRecDef = St0#lint.recdef_top,
{Vt,Uvt,St} = lc_quals(Qs, Vt0, [], St0#lint{recdef_top = false}),
{Vt,Uvt,St#lint{recdef_top = OldRecDef}}.

lc_quals(Qs, Vt0, [], St0).
lc_quals([{zip,_Anno,Gens} | Qs], Vt0, Uvt0, St0) ->
St1 = are_all_generators(Gens,St0),
{Vt,Uvt,St} = handle_generators(Gens,Vt0,Uvt0,St1),
Expand Down Expand Up @@ -4205,13 +4204,12 @@ fun_clauses(Cs, Vt, St) ->
fun_clauses1(Cs, Vt, St).

fun_clauses1(Cs, Vt, St) ->
OldRecDef = St#lint.recdef_top,
{Bvt,St2} = foldl(fun (C, {Bvt0, St0}) ->
{Cvt,St1} = fun_clause(C, Vt, St0),
{vtmerge(Cvt, Bvt0),St1}
end, {[],St#lint{recdef_top = false}}, Cs),
end, {[],St}, Cs),
Uvt = vt_no_unsafe(vt_no_unused(vtold(Bvt, Vt))),
{Uvt,St2#lint{recdef_top = OldRecDef}}.
{Uvt,St2}.

fun_clause({clause,_Anno,H,G,B}, Vt0, St0) ->
{Hvt,Hnew,St1} = head(H, Vt0, [], St0), % No imported pattern variables
Expand Down Expand Up @@ -4289,9 +4287,6 @@ pat_var(V, Anno, Vt, New, St0) ->
{[{V,{bound,used,Ls}}],[],
%% As this is matching, exported vars are risky.
add_warning(Anno, {exported_var,V,From}, St)};
error when St0#lint.recdef_top ->
{[],[{V,{bound,unused,[Anno]}}],
add_error(Anno, {variable_in_record_def,V}, St0)};
error ->
%% add variable to NewVars, not yet used
{[],[{V,{bound,unused,[Anno]}}],St0}
Expand Down
29 changes: 28 additions & 1 deletion lib/stdlib/test/erl_expand_records_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,34 @@
t() ->
catch #{ok => ok || #r1{}},
ok.
"""
""",
~"""
-record(r0, {a=[X||X<-[cucumber,banan]],
b=case {cucumber,banan} of X -> X; _ -> ok end,
c=fun()->{X,_} = {cucumber,banan}, X end}).
-record(r1, {a=[X||X<-[side_effect(a)]],
b=[X||X<-[side_effect(b)]]}).
side_effect(X) -> self() ! {side_effect, X}, ok.
t() ->
%% Test that X does not affect default initialization
X = {yes, no},
{yes,no} = X,
#r0{a=[cucumber,banan], b={cucumber,banan}, c=C} = #r0{},
cucumber = C(),
%% Test that initialization is not done on fields to be initialized
%% with other than the default.
#r1{a=hello,b=[ok]}=#r1{a=hello},
Ok1 = receive
{side_effect, b} -> ok;
{side_effect, a} -> nok
end,
Ok2 = receive
{side_effect, b} -> ok;
{side_effect, a} -> nok
after 100 -> ok
end,
Ok1 = Ok2 = ok.
"""
],
run(Config, Ts),
ok.
Expand Down Expand Up @@ -825,7 +852,7 @@
File = filename(Filename, Config),
Opts = [export_all,return,{outdir,?privdir}|Opts0],
ok = file:write_file(File, Test),
{ok, _M, Ws} = compile:file(File, Opts),

Check warning on line 855 in lib/stdlib/test/erl_expand_records_SUITE.erl

View workflow job for this annotation

GitHub Actions / CT Test Results

1 out of 2 runs failed: init 

artifacts/Unit Test Results/stdlib_junit.xml [took 0s]
Raw output 
Test init in erl_expand_records_SUITE failed!
{{badmatch,{error,[{"/buildroot/otp/lib/stdlib/make_test_dir/ct_logs/[email protected]_10.04.04/make_test_dir.stdlib_test.logs/run.2025-02-10_10.04.41/log_private/exprec_test.erl",
                    [{{3,25},erl_lint,{unsafe_var,'X',{'case',{2,17}}}}]}],
                  [{"/buildroot/otp/lib/stdlib/make_test_dir/ct_logs/[email protected]_10.04.04/make_test_dir.stdlib_test.logs/run.2025-02-10_10.04.41/log_private/exprec_test.erl",
                    [{{1,24},erl_lint,export_all}]}]}},
 [{erl_expand_records_SUITE,compile_file,3,
                            [{file,"erl_expand_records_SUITE.erl"},
                             {line,855}]},
  {erl_expand_records_SUITE,'-run/3-fun-0-',3,
                            [{file,"erl_expand_records_SUITE.erl"},
                             {line,830}]},
  {lists,foreach_1,2,[{file,"lists.erl"},{line,2313}]},
  {erl_expand_records_SUITE,init,1,
                            [{file,"erl_expand_records_SUITE.erl"},
                             {line,304}]},
  {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1794}]},
  {test_server,run_test_case_eval1,6,[{file,"test_server.erl"},{line,1303}]},
  {test_server,run_test_case_eval,9,[{file,"test_server.erl"},{line,1235}]}]}
warnings(File, Ws).

compile_file_mod(Config) ->
Expand Down
34 changes: 21 additions & 13 deletions lib/stdlib/test/erl_lint_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,7 +877,22 @@ export_vars_warn(Config) when is_list(Config) ->
Z = X.
">>,
[],
{warnings,[{{7,19},erl_lint,{exported_var,'Z',{'if',{2,19}}}}]}}
{warnings,[{{7,19},erl_lint,{exported_var,'Z',{'if',{2,19}}}}]}},
{exp5,
<<"-record(r0, {a=X=1,
b=X=2}).
-record(r1, {a=case 1 of Z -> X=Z end,
b=case 2 of X -> Y=Z=2 end}).
-record(r2, {a=case 1 of X -> X end,
b=(fun()-> X=2 end)()}).
">>,
[],{warnings,[{{1,22},erl_lint,{unused_record,r0}},
{{2,31},erl_lint,{exported_var,'X',{{record_field,r0,a},{1,34}}}},
{{3,17},erl_lint,{unused_record,r1}},
{{4,41},erl_lint,{exported_var,'X',{'case',{3,31}}}},
{{4,48},erl_lint,{exported_var,'Z',{'case',{3,31}}}},
{{5,17},erl_lint,{unused_record,r2}},
{{6,40},erl_lint,{exported_var,'X',{'case',{5,31}}}}]}}
],
[] = run(Config, Ts),
ok.
Expand Down Expand Up @@ -2846,10 +2861,8 @@ otp_5878(Config) when is_list(Config) ->
t() -> #r2{}.
">>,
[warn_unused_record],
{error,[{{1,44},erl_lint,{variable_in_record_def,'A'}},
{{1,54},erl_lint,{unbound_var,'B'}},
{{2,38},erl_lint,{variable_in_record_def,'A'}}],
[{{1,22},erl_lint,{unused_record,r1}}]}},
{errors,[{{1,54},erl_lint,{unbound_var,'B'}}],
[]}},

{otp_5878_30,
<<"-record(r1, {t = case foo of _ -> 3 end}).
Expand All @@ -2859,9 +2872,7 @@ otp_5878(Config) when is_list(Config) ->
t() -> {#r1{},#r2{},#r3{},#r4{}}.
">>,
[warn_unused_record],
{errors,[{{2,44},erl_lint,{variable_in_record_def,'A'}},
{{3,44},erl_lint,{variable_in_record_def,'A'}}],
[]}},
[]},

{otp_5878_40,
<<"-record(r1, {foo = A}). % A unbound
Expand Down Expand Up @@ -2898,9 +2909,7 @@ otp_5878(Config) when is_list(Config) ->
">>,
[warn_unused_record],
{error,[{{1,39},erl_lint,{unbound_var,'A'}},
{{2,33},erl_lint,{unbound_var,'A'}},
{{4,42},erl_lint,{variable_in_record_def,'A'}},
{{17,44},erl_lint,{variable_in_record_def,'A'}}],
{{2,33},erl_lint,{unbound_var,'A'}}],
[{{8,36},erl_lint,{unused_var,'X'}}]}},

{otp_5878_60,
Expand All @@ -2922,8 +2931,7 @@ otp_5878(Config) when is_list(Config) ->
t() -> #r1{}.
">>,
[warn_unused_record],
{errors,[{{3,40},erl_lint,{unbound_var,'Y'}},
{{4,38},erl_lint,{variable_in_record_def,'Y'}}],
{errors,[{{3,40},erl_lint,{unbound_var,'Y'}}],
[]}},

{otp_5878_80,
Expand Down
3 changes: 1 addition & 2 deletions system/doc/reference_manual/ref_man_records.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@ used.
FieldN [= ExprN]}).
```

The default value for a field is an arbitrary expression, except that it must
not use any variables.
The default value for a field is an arbitrary expression.

A record definition can be placed anywhere among the attributes and function
declarations of a module, but the definition must come before any usage of the
Expand Down
Loading