Skip to content

Commit

Permalink
Eliminate incorrect warning for useless building
Browse files Browse the repository at this point in the history
In OTP 24, there will be a warning for the following code:

    foo(X) ->
        Y = ".",
        fun() ->
                X = Y ++ "."  %Warning: a term is constructed, but never used
        end(),
        ok.

The warning is incorrect, because the built term will be matched to
the previous value of `X`. This commit eliminates the warning.

To explain the bug, I will illustrate how the example program is transformed
by the Core Erlang passes. My illustrative examples will use Erlang instead
of Core Erlang.

The first transformation makes the match `X = Y ++ "."` explicit:

    foo(X) ->
        Y = ".",
        fun() ->
                X1 = Y ++ ".",
                case X1 =:= X of
                    true ->
                        X1;
                    false ->
                        error({badmatch,X})
                end
        end(),
        ok.

The variable `X1` is only needed for the matching. It will be
annotated with a `compiler_generated` annotation to ensure that no
compiler warnings will be generated if it would later turn out that it
would never be used.

Next constants are propagated and constant expressions are evaluated.
Since the fun is only used once, it will be eliminated and its body
inlined. Now we have:

    foo(X) ->
        case ".." =:= X of
            true ->
                "..";
            false ->
                error({badmatch,X})
        end,
        ok.

The final optimization for this example is simplification of expressions
whose values are never used:

    foo(X) ->
        case ".." =:= X of
            true ->
                ok;  %Warning: a term is constructed, but never used
            false ->
                error({badmatch,X})
        end,
        ok.

The `".."` string will never be used, so it will be replaced with `ok`.
At the same time, a warning will be emitted.

The reason for the warning is that the `".."` string does not have a
`compiler_generated` annotation to indicate that it was introduced by
the compiler. The `X1` variable had a `compiler_generated` annotation,
but it was lost when `X1` was replaced with `".."`.

To eliminate the unwanted warning, the `compiler_generated` annotation
must be propagated from the variable to the substituted value.

Thanks to Jose Maria Perez Ramos (@Kuroneer) for noticing this bug and
suggesting a way to fix it.
  • Loading branch information
bjorng committed May 27, 2021
1 parent 045ce64 commit 476df89
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 18 deletions.
40 changes: 23 additions & 17 deletions lib/compiler/src/sys_core_fold.erl
Original file line number Diff line number Diff line change
Expand Up @@ -1074,23 +1074,13 @@ let_substs_1(Vs, #c_values{es=As}, Sub) ->
let_substs_1([V], A, Sub) -> let_subst_list([V], [A], Sub);
let_substs_1(Vs, A, _) -> {Vs,A,[]}.

let_subst_list([V|Vs0], [A0|As0], Sub) ->
let_subst_list([V|Vs0], [A|As0], Sub) ->
{Vs1,As1,Ss} = let_subst_list(Vs0, As0, Sub),
case is_subst(A0) of
case is_subst(A) of
true ->
A = case is_compiler_generated(V) andalso
not is_compiler_generated(A0) of
true ->
%% Propagate the 'compiler_generated' annotation
%% along with the value.
Ann = [compiler_generated|cerl:get_ann(A0)],
cerl:set_ann(A0, Ann);
false ->
A0
end,
{Vs1,As1,sub_subst_var(V, A, Sub) ++ Ss};
false ->
{[V|Vs1],[A0|As1],Ss}
{[V|Vs1],[A|As1],Ss}
end;
let_subst_list([], [], _) -> {[],[],[]}.

Expand Down Expand Up @@ -1333,8 +1323,10 @@ sub_new(#sub{}=Sub) ->

sub_get_var(#c_var{name=V}=Var, #sub{v=S}) ->
case orddict:find(V, S) of
{ok,Val} -> Val;
error -> Var
{ok,Val} ->
propagate_compiler_generated(Var, Val);
error ->
Var
end.

sub_set_var(#c_var{name=V}, Val, Sub) ->
Expand All @@ -1345,9 +1337,11 @@ sub_set_name(V, Val, #sub{v=S,s=Scope,t=Tdb0}=Sub) ->
Tdb = copy_type(V, Val, Tdb1),
Sub#sub{v=orddict:store(V, Val, S),s=sets:add_element(V, Scope),t=Tdb}.

sub_subst_var(#c_var{name=V}, Val, #sub{v=S0}) ->
sub_subst_var(#c_var{name=V}=Var, Val0, #sub{v=S0}) ->
Val = propagate_compiler_generated(Var, Val0),

%% Fold chained substitutions.
[{V,Val}] ++ [ {K,Val} || {K,#c_var{name=V1}} <- S0, V1 =:= V].
[{V,Val}] ++ [{K,Val} || {K,#c_var{name=V1}} <- S0, V1 =:= V].

sub_add_scope(Vs, #sub{s=Scope0}=Sub) ->
Scope = foldl(fun(V, S) when is_integer(V); is_atom(V) ->
Expand All @@ -1373,6 +1367,18 @@ sub_subst_scope_1([], _, Acc) -> Acc.
sub_is_in_scope(#c_var{name=V}, #sub{s=Scope}) ->
sets:is_element(V, Scope).

%% Propagate the 'compiler_generated' annotation (if any)
%% from From to To.
propagate_compiler_generated(From, To) ->
case is_compiler_generated(From) andalso
not is_compiler_generated(To) of
true ->
Ann = [compiler_generated|cerl:get_ann(To)],
cerl:set_ann(To, Ann);
false ->
To
end.

%% warn_no_clause_match(CaseOrig, CaseOpt) -> ok
%% Generate a warning if none of the user-specified clauses
%% will match.
Expand Down
14 changes: 13 additions & 1 deletion lib/compiler/test/warnings_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,19 @@ effect(Config) when is_list(Config) ->
">>,
[],
{warnings,[{{3,16},sys_core_fold,{ignored,useless_building}},
{{3,30},sys_core_fold,{ignored,useless_building}}]}}
{{3,30},sys_core_fold,{ignored,useless_building}}]}},

{propagated_literal,
<<"
foo(X) ->
Y = [$.],
%% There must not be a warning for constructing a term that
%% is never used.
fun() -> X = Y ++ [$.] end(),
ok.
">>,
[],
[]}
],
[] = run(Config, Ts),
ok.
Expand Down

0 comments on commit 476df89

Please sign in to comment.