Sign Up
Log In
Log In
or
Sign Up
Places
All Projects
Status Monitor
Collapse sidebar
home:Ledest:erlang:23
erlang
8091-Reduce-confusion-by-rephrasing-some-compil...
Overview
Repositories
Revisions
Requests
Users
Attributes
Meta
File 8091-Reduce-confusion-by-rephrasing-some-compiler-warning.patch of Package erlang
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
Locations
Projects
Search
Status Monitor
Help
OpenBuildService.org
Documentation
API Documentation
Code of Conduct
Contact
Support
@OBShq
Terms
openSUSE Build Service is sponsored by
The Open Build Service is an
openSUSE project
.
Sign Up
Log In
Places
Places
All Projects
Status Monitor