File 4051-Reduce-confusion-by-rephrasing-some-compil... of Package erlang

Overview Repositories Revisions Requests Users Attributes Meta

File 4051-Reduce-confusion-by-rephrasing-some-compiler-warning.patch of Package erlang (Revision a8b85727c0162baf491871a68c67a1ad)

Currently displaying revision a8b85727c0162baf491871a68c67a1ad , Show latest

From 8ecc648df22dbfe9dd68a3e2b968ba2b6a2ca7fe Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Gustavsson?= <bjorn@erlang.org>
Date: Thu, 4 Feb 2021 07:25:22 +0100
Subject: [PATCH] Reduce confusion by rephrasing some compiler warnings

Consider this module:

-module(w).
    -export([ll/0, sum/0, tt/0, ignored/2]).

ll() ->
        L = {a,b,c},
        length(L).

sum() ->
        A = a,
        B = b,
        A + B.

tt() ->
        (try nan after whatever end) + 42.

ignored(X, Y) ->
        X + Y,
        ok.

When compiling this module using https://github.com/erlang/otp/pull/3020,
the following warnings are emitted:

w.erl:6:5: Warning: this expression will fail with a 'badarg' exception
    %    6|     length(L).
    %     |     ^

w.erl:11:7: Warning: this expression will fail with a 'badarith' exception
    %   11|     A + B.
    %     |       ^

w.erl:14:34: Warning: this expression will fail with a 'badarith' exception
    %   14|     (try nan after whatever end) + 42.
    %     |                                  ^

w.erl:17:7: Warning: the result of the expression is ignored (suppress the warning by assigning the expression to the _ variable)
    %   17|     X + Y,
    %     |       ^

The last three warnings are slightly confusing, because the warnings
say "this expression" or "the expression", but the column position
points out the `+` operator in the middle of the expresson instead
of the beginning of the expression.

For the second warning, it would be better to point out the variable `A`:

w.erl:11:5: Warning: this expression will fail with a 'badarith' exception
    %   11|     A + B.
    %     |     ^

However, to produce that warning would need a major rethinking of the
compiler pass, `sys_core_fold`, that produces that kind of warning.
`sys_core_fold` is an optimization pass that opportunistically emits
warnings while optimizing the code. When the warning for line 11 is
emitted, the original code has been lost and the line and column
number for the variable `A` is no longer available.

The third warning does not seem possible to fix by moving the position of the caret:

w.erl:14:34: Warning: this expression will fail with a 'badarith' exception
    %   14|     (try nan after whatever end) + 42.
    %     |                                  ^

Pointing to the atom `nan` would be confusing.

Considering that the second warning would need a lot of work and that there is no good
solution for the third warning, I instead chose to rephrase the warnings like this:

w.erl:6:5: Warning: the call to length/1 will fail with a 'badarg' exception
    %    6|     length(L).
    %     |     ^

w.erl:11:7: Warning: evaluation of operator '+'/2 will fail with a 'badarith' exception
    %   11|     A + B.
    %     |       ^

w.erl:14:34: Warning: evaluation of operator '+'/2 will fail with a 'badarith' exception
    %   14|     (try nan after whatever end) + 42.
    %     |                                  ^

w.erl:17:7: Warning: the result of evaluating operator '+'/2 is ignored (suppress the warning by assigning the expression to the _ variable)
    %   17|     X + Y,
    %     |       ^
---
 lib/compiler/src/sys_core_fold.erl   | 91 +++++++++++++++++++++-------
 lib/compiler/test/warnings_SUITE.erl | 44 +++++++++-----
 2 files changed, 100 insertions(+), 35 deletions(-)

diff --git a/lib/compiler/src/sys_core_fold.erl b/lib/compiler/src/sys_core_fold.erl
index 287280c18f..c1d4323815 100644
--- a/lib/compiler/src/sys_core_fold.erl
+++ b/lib/compiler/src/sys_core_fold.erl
@@ -760,8 +760,11 @@ useless_call(effect, #c_call{module=#c_literal{val=Mod},
     case erl_bifs:is_safe(Mod, Name, A) of
 	false ->
 	    case erl_bifs:is_pure(Mod, Name, A) of
-		true -> add_warning(Call, result_ignored);
-		false -> ok
+		true ->
+                    Classified = classify_call(Call),
+                    add_warning(Call, {result_ignored,Classified});
+		false ->
+                    ok
 	    end,
 	    no;
 	true ->
@@ -959,7 +962,8 @@ eval_append(Call, X, Y) ->
 %%  a call to erlang:error(Reason).
 %%
 eval_failure(Call, Reason) ->
-    add_warning(Call, {eval_failure,Reason}),
+    Classified = classify_call(Call),
+    add_warning(Call, {eval_failure,Classified,Reason}),
     Call#c_call{module=#c_literal{val=erlang},
 		name=#c_literal{val=error},
 		args=[#c_literal{val=Reason}]}.
@@ -1663,9 +1667,9 @@ eval_case_warn(#c_primop{anno=Anno,
     case keyfind(eval_failure, 1, Anno) of
 	false ->
 	    ok;
-	{eval_failure,Reason} ->
+	{eval_failure,badmap} ->
 	    %% Example: M = not_map, M#{k:=v}
-	    add_warning(Core, {eval_failure,Reason})
+	    add_warning(Core, bad_map_update)
     end;
 eval_case_warn(_) -> ok.
 
@@ -2829,22 +2833,43 @@ is_result_unwanted(Core) ->
 get_warnings() ->
     ordsets:from_list((erase({?MODULE,warnings}))).
 
--type error() :: 'bad_unicode' | 'bin_argument_order'
-	       | 'bin_left_var_used_in_guard' | 'bin_opt_alias'
-	       | 'bin_partition' | 'bin_var_used' | 'bin_var_used_in_guard'
-	       | 'embedded_binary_size' | 'nomatch_clause_type'
-	       | 'nomatch_guard' | 'nomatch_shadow' | 'no_clause_match'
-	       | 'orig_bin_var_used_in_guard' | 'result_ignored'
-	       | 'useless_building'
-	       | {'eval_failure', term()}
-	       | {'no_effect', {'erlang',atom(),arity()}}
-	       | {'nomatch_shadow', integer()}
-	       | {'embedded_unit', _, _}.
+classify_call(Call) ->
+    Mod = cerl:concrete(cerl:call_module(Call)),
+    Name = cerl:concrete(cerl:call_name(Call)),
+    Arity = cerl:call_arity(Call),
+    case is_operator(Mod, Name, Arity) of
+        true ->
+            {operator,{Name,Arity}};
+        false ->
+            case is_auto_imported(Mod, Name, Arity) of
+                true ->
+                    {function,{Name,Arity}};
+                false ->
+                    {function,{Mod,Name,Arity}}
+            end
+    end.
+
+is_operator(erlang, Name, Arity) ->
+    try
+        _ = erl_internal:op_type(Name, Arity),
+        true
+    catch
+        error:_ ->
+            false
+    end;
+is_operator(_, _, _) -> false.
+
+is_auto_imported(erlang, Name, Arity) ->
+    erl_internal:bif(Name, Arity);
+is_auto_imported(_, _, _) -> false.
+
+-type error() :: atom() | tuple().
 
 -spec format_error(error()) -> nonempty_string().
 
-format_error({eval_failure,Reason}) ->
-    flatten(io_lib:format("this expression will fail with a '~p' exception", [Reason]));
+format_error({eval_failure,Call,Reason}) ->
+    flatten(io_lib:format("~ts will fail with a '~p' exception",
+                          [format_call(Call, false),Reason]));
 format_error(embedded_binary_size) ->
     "binary construction will fail with a 'badarg' exception "
 	"(field size for binary/bitstring greater than actual size)";
@@ -2858,6 +2883,8 @@ format_error(bad_unicode) ->
 format_error(bad_float_size) ->
     "binary construction will fail with a 'badarg' exception "
 	"(invalid size for a float segment)";
+format_error(bad_map_update) ->
+    "map update will fail with a 'badmap' exception";
 format_error({nomatch_shadow,Line,{Name, Arity}}) ->
     M = io_lib:format("this clause for ~ts/~B cannot match because a previous "
 		      "clause at line ~p always matches", [Name, Arity, Line]),
@@ -2910,14 +2937,36 @@ format_error({no_effect,{erlang,F,A}}) ->
 			 end
 		 end,
     flatten(io_lib:format(Fmt, Args));
-format_error(result_ignored) ->
-    "the result of the expression is ignored "
-	"(suppress the warning by assigning the expression to the _ variable)";
+format_error({result_ignored,Call}) ->
+    Fmt = "the result of ~ts is ignored "
+	"(suppress the warning by assigning the expression to the _ variable)",
+    flatten(io_lib:format(Fmt, [format_call(Call, true)]));
 format_error(invalid_call) ->
     "invalid function call";
 format_error(useless_building) ->
     "a term is constructed, but never used".
 
+format_call({function,{erlang,make_fun,3}}, _) ->
+    "fun construction";
+format_call({function,Call}, UseProgressiveForm) ->
+    [case UseProgressiveForm of
+         true -> "calling";
+         false -> "the call to"
+     end,
+     $\s,
+     case Call of
+         {M,F,A} ->
+             io_lib:format("~p:~p/~p", [M,F,A]);
+         {F,A} ->
+             io_lib:format("~p/~p", [F,A])
+     end];
+format_call({operator,{F,A}}, UseProgressiveForm) ->
+    Eval = case UseProgressiveForm of
+               true -> "evaluating";
+               false -> "evaluation of"
+           end,
+    io_lib:format(Eval ++ " operator ~p/~p", [F,A]).
+
 -ifdef(DEBUG).
 %% In order for simplify_let/2 to work correctly, the list of
 %% in-scope variables must always be a superset of the free variables
diff --git a/lib/compiler/test/warnings_SUITE.erl b/lib/compiler/test/warnings_SUITE.erl
index 45e43792b4..51c023ddba 100644
--- a/lib/compiler/test/warnings_SUITE.erl
+++ b/lib/compiler/test/warnings_SUITE.erl
@@ -237,12 +237,12 @@ guard(Config) when is_list(Config) ->
 	   {warnings,
 	    [{2,sys_core_fold,no_clause_match},
 	     {2,sys_core_fold,nomatch_guard},
-	     {2,sys_core_fold,{eval_failure,badarg}},
+	     {2,sys_core_fold,{eval_failure, {function,{element,2}}, badarg}},
 	     {4,sys_core_fold,no_clause_match},
 	     {4,sys_core_fold,nomatch_guard},
 	     {6,sys_core_fold,no_clause_match},
 	     {6,sys_core_fold,nomatch_guard},
-	     {6,sys_core_fold,{eval_failure,badarg}}
+	     {6,sys_core_fold,{eval_failure, {function,{element,2}}, badarg}}
 	    ]}}],
     [] = run(Config, Ts),
 
@@ -269,13 +269,13 @@ bad_arith(Config) when is_list(Config) -
 	   [],
 	   {warnings,
 	    [{3,sys_core_fold,nomatch_guard},
-	     {3,sys_core_fold,{eval_failure,badarith}},
+	     {3,sys_core_fold,{eval_failure, {operator,{'+',2}}, badarith}},
 	     {9,sys_core_fold,nomatch_guard},
-	     {9,sys_core_fold,{eval_failure,badarith}},
+	     {9,sys_core_fold,{eval_failure, {operator,{'+',2}}, badarith}},
 	     {9,sys_core_fold,{no_effect,{erlang,is_integer,1}}},
 	     {10,sys_core_fold,nomatch_guard},
-	     {10,sys_core_fold,{eval_failure,badarith}},
-	     {15,sys_core_fold,{eval_failure,badarith}}
+	     {10,sys_core_fold,{eval_failure, {operator,{'+',2}}, badarith}},
+	     {15,sys_core_fold,{eval_failure, {operator,{'+',2}}, badarith}}
 	    ] }}],
     [] = run(Config, Ts),
     ok.
@@ -353,8 +353,8 @@ files(Config) when is_list(Config) ->
            ">>,
            [],
            {warnings,
-            [{"file1",[{17,sys_core_fold,{eval_failure,badarith}}]},
-             {"file2",[{10,sys_core_fold,{eval_failure,badarith}}]}]}}],
+            [{"file1",[{17,sys_core_fold,{eval_failure, {operator,{'/',2}}, badarith}}]},
+             {"file2",[{10,sys_core_fold,{eval_failure, {operator,{'/',2}}, badarith}}]}]}}],
 
     [] = run(Config, Ts),
     ok.
@@ -484,12 +484,12 @@ effect(Config) when is_list(Config) ->
 	   [],
 	   {warnings,[{5,sys_core_fold,{no_effect,{erlang,is_integer,1}}},
 		      {7,sys_core_fold,useless_building},
-		      {9,sys_core_fold,result_ignored},
+		      {9,sys_core_fold,{result_ignored,{function,{abs,1}}}},
 		      {9,sys_core_fold,useless_building},
 		      {13,sys_core_fold,useless_building},
 		      {15,sys_core_fold,useless_building},
 		      {17,sys_core_fold,useless_building},
-		      {19,sys_core_fold,result_ignored},
+		      {19,sys_core_fold,{result_ignored,{operator,{'*',2}}}}]}},
 		      {21,sys_core_fold,useless_building},
 		      {21,sys_core_fold,{no_effect,{erlang,date,0}}},
 		      {21,sys_core_fold,{no_effect,{erlang,node,0}}},
@@ -606,7 +606,7 @@ maps(Config) when is_list(Config) ->
 		 ok.
            ">>,
            [],
-	   {warnings,[{4,sys_core_fold,{eval_failure,badmap}}]}},
+	   {warnings,[{4,sys_core_fold,bad_map_update}]}},
 	   {bad_map_src2,
            <<"
              t() ->
@@ -624,7 +624,7 @@ maps(Config) when is_list(Config) ->
 		 ok.
            ">>,
            [],
-	   {warnings,[{3,sys_core_fold,{eval_failure,badmap}}]}},
+	   {warnings,[{3,sys_core_fold,bad_map_update}]}},
            {ok_map_literal_key,
            <<"
              t() ->
@@ -844,7 +844,7 @@ underscore(Config) when is_list(Config)
                 {3,sys_core_fold,useless_building},
                 {4,sys_core_fold,useless_building},
                 {7,sys_core_fold,result_ignored},
-                {8,sys_core_fold,{no_effect,{erlang,date,0}}},
+                {8,sys_core_fold,{result_ignored,{operator,{'/',2}}}},
                 {11,sys_core_fold,useless_building},
                 {14,sys_core_fold,useless_building}],
 
-- 
2.26.2

Places

File 4051-Reduce-confusion-by-rephrasing-some-compiler-warning.patch of Package erlang (Revision a8b85727c0162baf491871a68c67a1ad)

Places