Skip to content

Commit

Permalink
Make options in the -compile attribute take precedence
Browse files Browse the repository at this point in the history
Change the compiler option processing order so that options given
in the `compile()` attribute takes precedence over options given
to the compiler, which in turn takes precedence over options given
in the environment.

This order makes most sense, as each module might need customized
options.

While at it, remove the undocumented `strict_record_updates` /
`no_strict_record_updates` options. Their naming no longer make any
sense, because record updates are always strict (that is, the source
record must have the correct tag and size). Incorporate the
behavior of `strict_record_updates` to update the record by matching
and building a new tuple into the `dialyzer` option. When dialyzer
is not used, records are updated using `setelement/3`, which is more
efficient in the JIT.

(This is the second attempt to fix erlang#6979, as erlang#8093 did not really
work.)
  • Loading branch information
bjorng committed Mar 4, 2024
1 parent ab7b354 commit af10a79
Show file tree
Hide file tree
Showing 5 changed files with 280 additions and 35 deletions.
2 changes: 1 addition & 1 deletion lib/compiler/src/compile.erl
Original file line number Diff line number Diff line change
Expand Up @@ -792,7 +792,7 @@ Module:format_error(ErrorDescriptor)
CompRet :: comp_ret().

file(File, Opts) when is_list(Opts) ->
do_compile({file,File}, Opts++env_default_opts());
do_compile({file,File}, env_default_opts() ++ Opts);
file(File, Opt) ->
file(File, [Opt|?DEFAULT_OPTIONS]).

Expand Down
252 changes: 247 additions & 5 deletions lib/compiler/test/compile_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
bc_options/1, deterministic_include/1, deterministic_paths/1,
compile_attribute/1, message_printing/1, other_options/1,
transforms/1, erl_compile_api/1, types_pp/1, bs_init_writable/1,
annotations_pp/1
annotations_pp/1, option_order/1
]).

suite() -> [{ct_hooks,[ts_install_cth]}].
Expand All @@ -59,7 +59,8 @@ all() ->
env_compiler_options, custom_debug_info, bc_options,
custom_compile_info, deterministic_include, deterministic_paths,
compile_attribute, message_printing, other_options, transforms,
erl_compile_api, types_pp, bs_init_writable, annotations_pp].
erl_compile_api, types_pp, bs_init_writable, annotations_pp,
option_order].

groups() ->
[].
Expand Down Expand Up @@ -906,10 +907,10 @@ strict_record(Config) when is_list(Config) ->
{ok,M} = c:c(M, [no_strict_record_tests|Opts]),
Turtle = test_sloppy(),

%% The option first given wins.
{ok,M} = c:c(M, [no_strict_record_tests,strict_record_tests|Opts]),
Turtle = test_sloppy(),
%% The option last given wins.
{ok,M} = c:c(M, [strict_record_tests,no_strict_record_tests|Opts]),
Turtle = test_sloppy(),
{ok,M} = c:c(M, [no_strict_record_tests,strict_record_tests|Opts]),
Turtle = test_strict(),

%% Default (possibly influenced by ERL_COMPILER_OPTIONS).
Expand Down Expand Up @@ -2120,6 +2121,146 @@ get_annotations(Key, [_|Lines]) ->
get_annotations(_, []) ->
[].

option_order(Config) ->
Ts = [{spec1,
~"""
-compile(nowarn_missing_spec).
foo() -> ok.
""",
[], %Environment
[warn_missing_spec],
[]},
{spec2,
~"""
foo() -> ok.
""",
[{"ERL_COMPILER_OPTIONS", "warn_missing_spec"}],
[nowarn_missing_spec],
[]},
{spec3,
~"""
-compile(nowarn_missing_spec).
foo() -> ok.
""",
[{"ERL_COMPILER_OPTIONS", "nowarn_missing_spec"}],
[warn_missing_spec],
[]},
{spec4,
~"""
-compile(warn_missing_spec).
foo() -> ok.
""",
[{"ERL_COMPILER_OPTIONS", "nowarn_missing_spec"}],
[],
{warnings,[{{2,1},erl_lint,{missing_spec,{foo,0}}}]}
},
{spec5,
~"""
-compile([warn_missing_spec,nowarn_missing_spec]).
foo() -> ok.
""",
[{"ERL_COMPILER_OPTIONS", "nowarn_missing_spec"}],
[warn_missing_spec],
[]},
{records1,
~"""
-record(r, {x,y}).
rec_test(#r{x=X,y=Y}) -> X + Y.
""",
[],
[strict_record_tests],
fun(M) ->
try M:rec_test({r,1,2,3}) of
3 ->
fail()
catch
error:function_clause ->
ok
end
end},
{records2,
~"""
-record(r, {x,y}).
rec_test(R) -> R#r.x + R#r.y.
""",
[],
[no_strict_record_tests],
fun(M) ->
3 = M:rec_test({r,1,2,3}),
ok
end},
{records3,
~"""
-compile(no_strict_record_tests).
-record(r, {x,y}).
rec_test(R) -> R#r.x + R#r.y.
""",
[],
[strict_record_tests],
fun(M) ->
3 = M:rec_test({r,1,2,3}),
ok
end},
{records4,
~"""
-record(r, {x,y}).
rec_test(#r{x=X,y=Y}) -> X + Y.
""",
[{"ERL_COMPILER_OPTIONS", "strict_record_tests"}],
[],
fun(M) ->
try M:rec_test({r,1,2,3}) of
3 ->
fail()
catch
error:function_clause ->
ok
end
end},
{records5,
~"""
-record(r, {x,y}).
rec_test(R) -> R#r.x + R#r.y.
""",
[{"ERL_COMPILER_OPTIONS", "strict_record_tests"}],
[no_strict_record_tests],
fun(M) ->
3 = M:rec_test({r,1,2,3}),
ok
end},
{records6,
~"""
-compile(no_strict_record_tests).
-record(r, {x,y}).
rec_test(R) -> R#r.x + R#r.y.
""",
[{"ERL_COMPILER_OPTIONS", "strict_record_tests"}],
[],
fun(M) ->
3 = M:rec_test({r,1,2,3}),
ok
end},
{records7,
~"""
-record(r, {x,y}).
rec_test(R) -> R#r.x + R#r.y.
""",
[{"ERL_COMPILER_OPTIONS", "no_strict_record_tests"}],
[no_strict_record_tests, strict_record_tests],
fun(M) ->
try M:rec_test({r,1,2,3}) of
3 ->
fail()
catch
error:{badrecord,{r,1,2,3}} ->
ok
end
end}

],
run(Config, Ts),
ok.

%%%
%%% Utilities.
%%%
Expand Down Expand Up @@ -2149,3 +2290,104 @@ is_lfe_module(File, Ext) ->
"lfe_" ++ _ -> true;
_ -> false
end.

%% Compiles a test module and returns the list of errors and warnings.

run(Config, Tests) ->
F = fun({N,P,Env,Ws,Run}, _BadL) when is_function(Run, 1) ->
case catch run_test(Config, P, Env, Ws, Run) of
ok ->
ok;
Bad ->
io:format("~nTest ~p failed. Expected~n ~p~n"
"but got~n ~p~n", [N, ok, Bad]),
fail()
end;
({N,P,Env,Ws,Expected}, BadL)
when is_list(Expected); is_tuple(Expected) ->
io:format("### ~s\n", [N]),
case catch run_test(Config, P, Env, Ws, none) of
Expected ->
BadL;
Bad ->
io:format("~nTest ~p failed. Expected~n ~p~n"
"but got~n ~p~n", [N, Expected, Bad]),
fail()
end
end,
lists:foldl(F, [], Tests).

run_test(Conf, Test0, Env, Options, Run) ->
run_test_putenv(Env),
Module = "warnings" ++ test_lib:uniq(),
Filename = Module ++ ".erl",
DataDir = proplists:get_value(priv_dir, Conf),
Test1 = ["-module(", Module, "). -file( \"", Filename, "\", 1). ", Test0],
Test = iolist_to_binary(Test1),
File = filename:join(DataDir, Filename),
Opts = [binary,export_all,return|Options],
ok = file:write_file(File, Test),

%% Compile once just to print all warnings (and cover more code).
_ = compile:file(File, [binary,export_all,report|Options]),

%% Test result of compilation.
{ok, Mod, Beam, Warnings} = compile:file(File, Opts),
_ = file:delete(File),

if
is_function(Run, 1) ->
{module,Mod} = code:load_binary(Mod, "", Beam),
ok = Run(Mod),
run_test_unsetenv(Env),
true = code:delete(Mod),
_ = code:purge(Mod),
ok;
Run =:= none ->
run_test_unsetenv(Env),
Res = get_warnings(Warnings),
case Res of
[] ->
[];
{warnings, Ws} ->
print_warnings(Ws, Test),
Res
end
end.

run_test_putenv(Env) ->
_ = [_ = os:putenv(Name, Value) || {Name,Value} <- Env],
ok.

run_test_unsetenv(Env) ->
_ = [_ = os:unsetenv(Name) || {Name,_Value} <- Env],
ok.

get_warnings([]) ->
[];
get_warnings(WsL) ->
case WsL of
[{_File,Ws}] -> {warnings, Ws};
_ -> {warnings, WsL}
end.

print_warnings(Warnings, Source) ->
Lines = binary:split(Source, <<"\n">>, [global]),
Cs = [print_warning(W, Lines) || W <- Warnings],
io:put_chars(Cs),
ok.

print_warning({{LineNum,Column},Mod,Data}, Lines) ->
Line0 = lists:nth(LineNum, Lines),
<<Line1:(Column-1)/binary,_/binary>> = Line0,
Spaces = re:replace(Line1, <<"[^\t]">>, <<" ">>, [global]),
CaretLine = [Spaces,"^"],
[io_lib:format("~p:~p: ~ts\n",
[LineNum,Column,Mod:format_error(Data)]),
Line0, "\n",
CaretLine, "\n\n"];
print_warning(_, _) ->
[].

fail() ->
ct:fail(failed).
2 changes: 1 addition & 1 deletion lib/dialyzer/src/dialyzer_utils.erl
Original file line number Diff line number Diff line change
Expand Up @@ -767,7 +767,7 @@ sets_filter([Mod|Mods], ExpTypes) ->

src_compiler_opts() ->
[no_copt, to_core, binary, return_errors,
no_inline, strict_record_tests, strict_record_updates,
no_inline, strict_record_tests,
dialyzer, no_spawn_compiler_process].

-spec format_errors([{module(), string()}]) -> [string()].
Expand Down
57 changes: 30 additions & 27 deletions lib/stdlib/src/erl_expand_records.erl
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,15 @@ Section [The Abstract Format](`e:erts:absform.md`) in ERTS User's Guide.

-import(lists, [map/2,foldl/3,foldr/3,sort/1,reverse/1,duplicate/2]).

-record(exprec, {compile=[], % Compile flags
vcount=0, % Variable counter
calltype=#{}, % Call types
records=#{}, % Record definitions
raw_records=[],% Raw record forms
strict_ra=[], % strict record accesses
checked_ra=[], % successfully accessed records
dialyzer=false % Cached value of compile flag 'dialyzer'
}).
-record(exprec, {vcount=0, % Variable counter
calltype=#{}, % Call types
records=#{}, % Record definitions
raw_records=[], % Raw record forms
strict_ra=[], % Strict record accesses
checked_ra=[], % Successfully accessed records
dialyzer=false, % Compiler option 'dialyzer'
strict_rec_tests=true :: boolean()
}).

-doc """
Expands all records in a module to use explicit tuple operations and adds
Expand All @@ -56,10 +56,10 @@ module has no references to records, attributes, or code.
%% Is is assumed that Fs is a valid list of forms. It should pass
%% erl_lint without errors.
module(Fs0, Opts0) ->
Opts = compiler_options(Fs0) ++ Opts0,
Dialyzer = lists:member(dialyzer, Opts),
Calltype = init_calltype(Fs0),
St0 = #exprec{compile = Opts, dialyzer = Dialyzer, calltype = Calltype},
Opts = Opts0 ++ compiler_options(Fs0),
St0 = #exprec{dialyzer = lists:member(dialyzer, Opts),
calltype = init_calltype(Fs0),
strict_rec_tests = strict_record_tests(Opts)},
{Fs,_St} = forms(Fs0, St0),
Fs.

Expand Down Expand Up @@ -623,7 +623,7 @@ index_expr(F, [_ | Fs], I) -> index_expr(F, Fs, I+1).
%% This expansion must be passed through expr again.

get_record_field(Anno, R, Index, Name, St) ->
case strict_record_tests(St#exprec.compile) of
case St#exprec.strict_rec_tests of
false ->
sloppy_get_record_field(Anno, R, Index, Name, St);
true ->
Expand Down Expand Up @@ -674,15 +674,17 @@ sloppy_get_record_field(Anno, R, Index, Name, St) ->
{remote,Anno,{atom,Anno,erlang},{atom,Anno,element}},
[I,R]}, St).

strict_record_tests([strict_record_tests | _]) -> true;
strict_record_tests([no_strict_record_tests | _]) -> false;
strict_record_tests([_ | Os]) -> strict_record_tests(Os);
strict_record_tests([]) -> true. %Default.
strict_record_tests(Opts) ->
strict_record_tests(Opts, true).

strict_record_updates([strict_record_updates | _]) -> true;
strict_record_updates([no_strict_record_updates | _]) -> false;
strict_record_updates([_ | Os]) -> strict_record_updates(Os);
strict_record_updates([]) -> false. %Default.
strict_record_tests([strict_record_tests | Os], _) ->
strict_record_tests(Os, true);
strict_record_tests([no_strict_record_tests | Os], _) ->
strict_record_tests(Os, false);
strict_record_tests([_ | Os], Bool) ->
strict_record_tests(Os, Bool);
strict_record_tests([], Bool) ->
Bool.

%% pattern_fields([RecDefField], [Match]) -> [Pattern].
%% Build a list of match patterns for the record tuple elements.
Expand Down Expand Up @@ -732,13 +734,14 @@ record_update(R, Name, Fs, Us0, St0) ->
%% to guarantee that it is only evaluated once.
{Var,St2} = new_var(Anno, St1),

%% Honor the `strict_record_updates` option needed by `dialyzer`, otherwise
%% expand everything to chains of `setelement/3` as that's far more
%% efficient in the JIT.
StrictUpdates = strict_record_updates(St2#exprec.compile),
%% If the `dialyzer` option is in effect, update the record by
%% matching out all unmodified fields and building a new tuple.
%% Otherwise expand everything to chains of `setelement/3` as
%% that is far more efficient in the JIT.
Dialyzer = St2#exprec.dialyzer,
{Update,St} =
if
not StrictUpdates, Us =/= [] ->
not Dialyzer, Us =/= [] ->
{record_setel(Var, Name, Fs, Us), St2};
true ->
record_match(Var, Name, Anno, Fs, Us, St2)
Expand Down
Loading

0 comments on commit af10a79

Please sign in to comment.