File 0156-Eliminate-incorrect-warning-for-useless-bu... of Package erlang

Overview Repositories Revisions Requests Users Attributes Meta

File 0156-Eliminate-incorrect-warning-for-useless-building.patch of Package erlang (Revision eb6a0d2011426594176daad72577cf6b)

Currently displaying revision eb6a0d2011426594176daad72577cf6b , Show latest

From 476df89a43034ca1d24c464ad01c4602f9059080 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Gustavsson?= <bjorn@erlang.org>
Date: Thu, 27 May 2021 05:40:36 +0200
Subject: [PATCH] Eliminate incorrect warning for useless building

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.
---
 lib/compiler/src/sys_core_fold.erl   | 40 ++++++++++++++++------------
 lib/compiler/test/warnings_SUITE.erl | 14 +++++++++-
 2 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/lib/compiler/src/sys_core_fold.erl b/lib/compiler/src/sys_core_fold.erl
index 2fd2655eb3..01e9987196 100644
--- a/lib/compiler/src/sys_core_fold.erl
+++ b/lib/compiler/src/sys_core_fold.erl
@@ -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([], [], _) -> {[],[],[]}.
 
@@ -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) ->
@@ -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) ->
@@ -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.
diff --git a/lib/compiler/test/warnings_SUITE.erl b/lib/compiler/test/warnings_SUITE.erl
index 6aebf61b09..82ab077cc9 100644
--- a/lib/compiler/test/warnings_SUITE.erl
+++ b/lib/compiler/test/warnings_SUITE.erl
@@ -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.
-- 
2.26.2

Places

File 0156-Eliminate-incorrect-warning-for-useless-building.patch of Package erlang (Revision eb6a0d2011426594176daad72577cf6b)

Places