Skip to content

Commit

Permalink
stdlib: allow variables in record definitions
Browse files Browse the repository at this point in the history 
create an init function e.g. rec_init$^0, for each record
with definitions containing variables.

e.g.
-record(r, {f = fun(X)->case X of {y, Y} -> Y; _ -> X end, g=..., h=abc}).
foo(X)->\#r{}. --> foo(X)->(rec_init()){}.

rec_init() will initialize all fields with the default values
e.g.
foo(X)->\#r{f=X} --> foo(X)->(rec_init()){f=X}.

This means that some fields will be initialized twice.
This also means that if you have side effects in the record definition
they might be evaluated even when the corresponding field is set to a new expression.

rec_init() functions will not be generated if all fields of the record that contains
"free" variables are initialized by the user.
e.g.
foo(X)->\#r{f=X,g=X}. --> foo(X)->{r,X,X,abc}.

- removes lint error for variables in definitions
- updates erl_lint_SUITE and erl_expand_records_SUITE to work with this new behavior
  • Loading branch information
@frazze-jobb
frazze-jobb committed Feb 13, 2025
1 parent befaffa commit cf5cbab
Show file tree
Hide file tree
Showing 6 changed files with 190 additions and 46 deletions.
17 changes: 15 additions & 2 deletions lib/stdlib/src/erl_error.erl
View file
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
75 changes: 72 additions & 3 deletions lib/stdlib/src/erl_expand_records.erl
View file
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];
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,34 @@ 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);
%% Initialize the record
R_init = [{atom,Anno,Name} |
record_inits(record_fields(Name, Anno0, St), Is)],
Vars = lists:flatten(traverse_af(Is, fun save_vars/2)),
%% Check if there are variables in the initialized record
%% if there are, we need to initialize the record using a generated function
case free_variables(R_init, Vars) of
true ->
%% Initialize the record with only the default values
R_default_init = [{atom,Anno,Name} |
record_inits(record_fields(Name, Anno, St),[])],
{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
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
51 changes: 50 additions & 1 deletion lib/stdlib/test/erl_expand_records_SUITE.erl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,56 @@ init(Config) when is_list(Config) ->
t() ->
catch #{ok => ok || #r1{}},
ok.
"""
""",
~"""
-record(r0, {a=[X||X<-[cucumber,banan]],
b=case {cucumber,banan} of X -> X; _ -> ok end,
c=fun()->{Y,_} = {cucumber,banan}, Y end}).
-record(r1, {a=[X||X<-[side_effect(a)]],
b=[X||X<-[side_effect(b)]]}).
-record(r2, {a=X=1,
b=X}).
-record(r3, {a = case X = 1 of _ -> X end,
b = case Y = 2 of X -> Y; _ -> X end}).
-record(r4, {a= begin X = side_effect(a), X end,
b= begin X = side_effect(b), X end}).
-record(r5, {a = case side_effect(a) of Y when Y =:= ok -> Y; Y -> Y end,
b = case Y of ok -> 1; _ -> 2 end}).
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 default initialization is done on fields that are
%% initialized with another value
#r1{a=hello,b=[ok]}=#r1{a=hello},
Ok1 = receive
{side_effect, a} -> ok;
{side_effect, b} -> nok
end,
Ok2 = receive
{side_effect, b} -> ok
after 100 -> nok
end,
Ok1 = Ok2 = ok,
#r2{a=1,b=1}=#r2{},
#r2{a=undefined, b=1}=#r2{a=undefined},
#r2{a=2, b=2}=#r2{_=2},
#r3{a=1, b=1}=#r3{},
#r3{a=3, b=1}=#r3{a=3},
#r4{a=ok, b=ok}=#r4{},
ok = receive {side_effect, a} -> ok; _ -> nok end,
ok = receive {side_effect, b} -> ok; _ -> nok end,
#r4{a=ok, b=ok}=#r4{a=ok},
ok = receive {side_effect, a} -> ok; _ -> nok end,
ok = receive {side_effect, b} -> ok; _ -> nok end,
#r5{a=ok, b = 1} = #r5{a=ok},
ok = receive {side_effect, a} -> ok; _ -> nok end,
ok.
"""
],
run(Config, Ts),
ok.
Expand Down
34 changes: 21 additions & 13 deletions lib/stdlib/test/erl_lint_SUITE.erl
View file
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
Loading

0 comments on commit cf5cbab

Please sign in to comment.