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

[frazze-jobb]

records that have field default values containing variables that are "free" was unsafe in functions that have variables with the same name. This commit creates init function for records to protect the variables in the default value.

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

r_init() will only initialize fields that will not be updated e.g.
foo(X)->#r{f=X} --> foo(X)->(r_init_f()){f=X}.
r_init_f will only initialize g and h with its default value, f will be initialized to undefined.

r_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}.

closes #9317

[github-actions]

CT Test Results

    2 files     97 suites   1h 8m 18s ⏱️
2 194 tests 2 146 ✅ 47 💤 1 ❌
2 561 runs  2 511 ✅ 49 💤 1 ❌

For more details on these failures, see this check.

Results for commit 952de98.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• Windows Installer

// Erlang/OTP Github Action Bot

[ilya-klyuchnikov]

Now, as "free vars" are allowed in record fields, these free vars can affect each other.

-module(z).

-export([mk1/0]).

-record(a, {
  a = X = id(1),
  b = X = id(2)
}).

id(X) -> X.

mk1() ->
  #a{}.

1> z:mk1().
** exception error: no match of right hand side value 2
     in function  z:'rec_init$^0'/0 (z.erl, line 13)

[frazze-jobb]

Now, as "free vars" are allowed in record fields, these free vars can affect each other.

Thanks, I'll get to it soon!

[frazze-jobb]

Now, as "free vars" are allowed in record fields, these free vars can affect each other.

Thanks, I'll get to it soon!

Actually, I spoke to soon. This is as intended. Just like how it works if you try to update the record like this: #r0{a=X=id(1),b=X=id(2)} that would not work either.

But I will update the linter so that it warns for this.

[elbrujohalcon]

Any chance of not including the new function in the error description?

So, from the other comment…

1> z:mk1().
** exception error: no match of right hand side value 2
     in function  z:'rec_init$^0'/0 (z.erl, line 13)

I would've preferred to see something like…

1> z:mk1().
** exception error: no match of right hand side value 2
     in function  z:mk1/0 (z.erl, line 13)

[frazze-jobb]

Any chance of not including the new function in the error description?

Tail calls, like in this case, makes it invisible in the stacktrace.
Instead I suggest:
in record default value (z.erl, line 13)

Would that work for you?

[michalmuskala]

Another option would be to explicitly prevent the compiler from tail calling into this helper function, and always emit a full call with building a stack frame

[ilya-klyuchnikov]

With the current implementation it's possible to crash erlc:

-module(z).

-export([mk1/0]).

-record(a, {
  a = X = 1,
  b = X
}).

mk1() ->
  #a{a = 2}.

erlc z.erl

Function: 'rec_init$^0'/0
Sub pass ssa_opt_type_start
z.erl: internal error in pass beam_ssa_opt:
exception error: no match of right hand side value #{}
  in function  beam_ssa_type:concrete_type/2 (beam_ssa_type.erl, line 2522)
  in call from beam_ssa_type:simplify_arg/3 (beam_ssa_type.erl, line 1890)
  in call from beam_ssa_type:'-simplify_args/3-lc$^0/1-0-'/3 (beam_ssa_type.erl, line 1883)
  in call from beam_ssa_type:'-simplify_args/3-lc$^0/1-0-'/3 (beam_ssa_type.erl, line 1883)
  in call from beam_ssa_type:simplify/5 (beam_ssa_type.erl, line 1101)
  in call from beam_ssa_type:sig_is/7 (beam_ssa_type.erl, line 309)
  in call from beam_ssa_type:sig_bs/8 (beam_ssa_type.erl, line 252)
  in call from beam_ssa_type:sig_function_1/4 (beam_ssa_type.erl, line 221)

The code after expansion is:

-file("z.erl", 1).

-module(z).

-export([mk1/0]).

-record(a,{a = X = 1, b = X}).

mk1() ->
    begin
        REC0 = 'rec_init$^0'(),
        case REC0 of
            {a, _, _} ->
                setelement(2, REC0, 2);
            _ ->
                error({badrecord, REC0})
        end
    end.



'rec_init$^0'() ->
    {a, undefined, X}.

[bjorng]

The test case erl_expand_records_SUITE:init/1 fails.

[frazze-jobb]

I think my attempt at being clever about this failed, and a naiver approach is better.
I am reverting back to that.

-module(z).

-export([mk1/0]).

-record(a, {
  a = X = 1,
  b = X,
  c = 3
}).

mk1() ->
  #a{a = 2}.

In the above case, you will apply the initialize values, (a=2 in this case) #a{2,X,3}, check if there are variables left in the expression.
If it is, you will run the rec_init() function and apply the default values first, followed by a record update. i.e. (rec_init())#a{a=2}.

mk1() ->
  #a{a = 2, b = 1}.

In this case after the initializing values are applied, you have #a{2,1,3}, there are no variables, so its not necessary to run the rec_init() function.

If you have side effects (which you shouldn't) in your records default value, you have to think a second time if that really is what you want. Since they might be running when you do not expect it.

[bjorng]

So far I've looked at erl_error and erl_expand_record.

My comments are very nit-picky. We care very much about a consistent code-style in the compiler (and compiler-adjacent modules in STDLIB).

[bjorng]

Suggested change

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

[bjorng]

Is there any test of this added code?

[frazze-jobb]

Yes a testcase has been added

[bjorng]

A tip: there is no need create and call a fun here. You can just use a case or an if here.

[bjorng]

I don't think this is correct. Only variables defined in the function head will be shadowed. Variables defined in the function body will be matched against variables having the same name in the enclosing function.

In my suggested simplified function, I don't try to handle funs; I think they are used too infrequently to be worth the effort.

[ilya-klyuchnikov]

"Bound" variables from previous fields are not propagated correctly to next fields.

This definition is accepted:

-record(r1, {
  a = X = 1,
  c = X
}).

While this definition is rejected:

-record(r1, {
  a = X = 1,
  b,
  c = X
}).

rec.erl:10:7: variable 'X' is unbound
%   10|   c = X

[michalmuskala]

Thinking about it a bit more, I actually find it very surprising we want to allow variables "leaking" across fields in default values - we don't allow that in regular record creation!

For example this is an error:

-record(foo, {a, b}).

foo() -> #foo{a = X = 1, b = X}.

Why should moving this into the default expression somehow make this compile and OK? This sounds quite inconsistent to me.

[ilya-klyuchnikov]

I actually find it very surprising we want to allow variables "leaking" across fields in default values

I agree, - this introduces one more "rule" about scoping of variables that is already quite complicated and sometimes surprising.

[frazze-jobb]

Thinking about it a bit more, I actually find it very surprising we want to allow variables "leaking" across fields in default values - we don't allow that in regular record creation!

For example this is an error:

-record(foo, {a, b}).

foo() -> #foo{a = X = 1, b = X}.

Why should moving this into the default expression somehow make this compile and OK? This sounds quite inconsistent to me.

Okey, I must have been confused because I thought this was valid! I'll fix the behavior next week then.