File 1421-Add-opportunistic-warnings-for-shadowed-map-patterns.patch of Package erlang
From 8a8ea04b49704d3269e27f1eab58a2830cfe6fae Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B6rn=20Gustavsson?= <bjorn@erlang.org>
Date: Tue, 11 Jun 2024 14:17:07 +0200
Subject: [PATCH] Add opportunistic warnings for shadowed map patterns
The compiler can emit so-called *opportunistic warnings*, which are
warnings emitted when the compiler notices something suspicious during
optimization or code generation. The trouble with opportunistic
warnings is that there is no guarantee that the compiler will notice
all kind of suspicious code. For example, the compiler in Erlang/OTP
27 and earlier would not emit any warnings for the following
functions:
mm_1(#{}) ->
a;
mm_1(#{first := First}) ->
{b,First}.
mm_2(#{first := First}) ->
{b,First};
mm_2(#{first := First, second := Second}) ->
{c,First,Second}.
Since it is easy to arrange clauses in the wrong order and put the
matching for any map before other map patterns, it seems worthwhile
to catch at least some of the map patterns that will shadow subsequent
patterns.
With this commit, the compiler will generate the following warnings:
t.erl:6:1: Warning: this clause cannot match because a previous clause at line 4 matches the same pattern as this clause
% 6| mm_1(#{first := First}) ->
% | ^
t.erl:11:1: Warning: this clause cannot match because a previous clause at line 9 matches the same pattern as this clause
% 11| mm_2(#{first := First, second := Second}) ->
% | ^
Note that these warnings are still opportunistic and there is no
guarantee that the compiler will notice all kind of shadowed map
patterns.
Resolves #8558
---
lib/compiler/src/beam_core_to_ssa.erl | 50 ++++++++++++++++---
lib/compiler/test/warnings_SUITE.erl | 72 ++++++++++++++++++++++++++-
2 files changed, 113 insertions(+), 9 deletions(-)
diff --git a/lib/compiler/src/beam_core_to_ssa.erl b/lib/compiler/src/beam_core_to_ssa.erl
index a1191a79bf..8a87c49ad0 100644
--- a/lib/compiler/src/beam_core_to_ssa.erl
+++ b/lib/compiler/src/beam_core_to_ssa.erl
@@ -169,8 +169,8 @@ module(#c_module{name=#c_literal{val=Mod},exports=Es,attrs=As,defs=Fs}, Options)
-spec format_error(warning()) -> string() | binary().
format_error({nomatch,{shadow,Line}}) ->
- M = io_lib:format(<<"this clause cannot match because a previous clause at line ~p "
- "always matches">>, [Line]),
+ S = ~"this clause cannot match because a previous clause at line ~p matches the same pattern as this clause",
+ M = io_lib:format(S, [Line]),
flatten(M);
format_error({nomatch,shadow}) ->
<<"this clause cannot match because a previous clause always matches">>;
@@ -1502,12 +1502,46 @@ ensure_fixed_size(#cg_bin_end{}) ->
%% At this point all the clauses have the same constructor; we must
%% now separate them according to value.
+match_value(Us0, cg_map=T, Cs0, Def, St0) ->
+ {Cs1,St1} = remove_unreachable(Cs0, St0),
+ {Us1,Cs2,St2} = partition_intersection(Us0, Cs1, St1),
+ do_match_value(Us1, T, Cs2, Def, St2);
match_value(Us0, T, Cs0, Def, St0) ->
- {Us1,Cs1,St1} = partition_intersection(T, Us0, Cs0, St0),
- UCss = group_value(T, Us1, Cs1),
- mapfoldl(fun ({Us,Cs}, St) -> match_clause(Us, Cs, Def, St) end, St1, UCss).
+ do_match_value(Us0, T, Cs0, Def, St0).
+
+do_match_value(Us0, T, Cs0, Def, St0) ->
+ UCss = group_value(T, Us0, Cs0),
+ mapfoldl(fun ({Us,Cs}, St) -> match_clause(Us, Cs, Def, St) end, St0, UCss).
+
+%% remove_unreachable([Clause], State) -> {[Clause],State}
+%% Remove all clauses after a clause that will always match any
+%% map.
+remove_unreachable([#iclause{anno=Anno,pats=Pats,guard=G}=C|Cs0], St0) ->
+ maybe
+ %% Will the first pattern match any map?
+ [#cg_map{es=[]}|Ps] ?= Pats,
+
+ %% Are all following pattern variables, which will always match?
+ true ?= all(fun(#b_var{}) -> true;
+ (_) -> false
+ end, Ps),
+
+ %% Will the guard always succeed?
+ #c_literal{val=true} ?= G,
+
+ %% This clause will always match. Warn and discard all clauses
+ %% that follow.
+ St1 = maybe_add_warning(Cs0, Anno, St0),
+ {[C],St1}
+ else
+ _ ->
+ {Cs,St} = remove_unreachable(Cs0, St0),
+ {[C|Cs],St}
+ end;
+remove_unreachable([], St0) ->
+ {[],St0}.
-%% partition_intersection(Type, Us, [Clause], State) -> {Us,Cs,State}.
+%% partition_intersection(Us, [Clause], State) -> {Us,Cs,State}.
%% Partition a map into two maps with the most common keys to the
%% first map.
%%
@@ -1528,7 +1562,7 @@ match_value(Us0, T, Cs0, Def, St0) ->
%% The intention is to group as many keys together as possible and
%% thus reduce the number of lookups to that key.
-partition_intersection(cg_map, [U|_]=Us, [_,_|_]=Cs0, St0) ->
+partition_intersection([U|_]=Us, [_,_|_]=Cs0, St0) ->
Ps = [clause_val(C) || C <- Cs0],
case find_key_intersection(Ps) of
none ->
@@ -1540,7 +1574,7 @@ partition_intersection(cg_map, [U|_]=Us, [_,_|_]=Cs0, St0) ->
end, Cs0),
{[U|Us],Cs1,St0}
end;
-partition_intersection(_, Us, Cs, St) ->
+partition_intersection(Us, Cs, St) ->
{Us,Cs,St}.
partition_keys(#cg_map{es=Pairs}=Map, Ks) ->
diff --git a/lib/compiler/test/warnings_SUITE.erl b/lib/compiler/test/warnings_SUITE.erl
index 284528714f..982f1f9cfe 100644
--- a/lib/compiler/test/warnings_SUITE.erl
+++ b/lib/compiler/test/warnings_SUITE.erl
@@ -868,7 +868,77 @@ maps(Config) when is_list(Config) ->
{{25,20},v3_core,{map_key_repeated,#{"a" => 1}}},
{{28,21},v3_core,{map_key_repeated,#{"a" => <<"b">>}}},
{{32,21},v3_core,{map_key_repeated,#{<<"a">> => 1}}}
- ]}}
+ ]}},
+ {map_nomatch,
+ ~"""
+ match_map_1(#{}) ->
+ a;
+ match_map_1(#{first := First}) ->
+ {b,First};
+ match_map_1(#{first := First, second := Second}) ->
+ {c,First,Second}.
+
+ match_map_1(#{}, A) ->
+ {a,A};
+ match_map_1(#{first := First}, A) ->
+ {b,A,First};
+ match_map_1(#{first := First, second := Second}, A) ->
+ {c,A,First,Second}.
+
+ match_map_2(#{first := First}) ->
+ {b,First};
+ match_map_2(#{first := First, second := Second}) ->
+ {c,First,Second}.
+
+ match_map_2(#{first := First}, A, B) ->
+ {b,A,B,First};
+ match_map_2(#{first := First, second := Second}, A, B) ->
+ {c,A,B,First,Second}.
+
+ match_map_3([#{} | _]) ->
+ a;
+ match_map_3([#{first := First} | _]) ->
+ {b,First};
+ match_map_3([#{first := First, second := Second} | _]) ->
+ {c,First,Second}.
+
+ match_map_4([#{first := First} | _]) ->
+ {b,First};
+ match_map_4([#{first := First, second := Second} | _]) ->
+ {c,First,Second}.
+ """,
+ [],
+ {warnings,[{{3,1},beam_core_to_ssa,{nomatch,{shadow,1}}},
+ {{10,1},beam_core_to_ssa,{nomatch,{shadow,8}}},
+ {{17,1},beam_core_to_ssa,{nomatch,{shadow,15}}},
+ {{22,1},beam_core_to_ssa,{nomatch,{shadow,20}}},
+ {{27,1},beam_core_to_ssa,{nomatch,{shadow,25}}},
+ {{34,1},beam_core_to_ssa,{nomatch,{shadow,32}}}]}},
+ {map_nowarn,
+ %% There will be no warnings for shadowing for the
+ %% following functions, either because the first clause
+ %% actually can be executed or because the compiler's
+ %% checks are not sufficiently thorough.
+ ~"""
+ %% The compiler does not detect this shadowing.
+ match_map_nowarn_1([#{}]) -> no;
+ match_map_nowarn_1([#{a := A}]) -> {a,A}.
+
+ %% The guard in the first clause can fail.
+ match_map_nowarn_2(#{}, X) when is_integer(X) -> {a,X};
+ match_map_nowarn_2(#{b := B}, X) -> {b,X,B}.
+
+ %% The first clause will fail to match if the second
+ %% argument is not `x`.
+ match_map_nowarn_3(#{}, x) -> a;
+ match_map_nowarn_3(#{b := B}, y) -> {b,B}.
+
+ %% The compiler does not detect this shadowing.
+ match_map_nowarn_4(#{}, x) -> a;
+ match_map_nowarn_4(#{b := B}, x) -> {b,B}.
+ """,
+ [],
+ []}
],
run(Config, Ts),
ok.
--
2.35.3
