Skip to content

Commit

Permalink
Add opportunistic warnings for shadowed map patterns
Browse files Browse the repository at this point in the history
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
  • Loading branch information
bjorng committed Jun 20, 2024
1 parent ab1f7d4 commit 8a8ea04
Show file tree
Hide file tree
Showing 2 changed files with 113 additions and 9 deletions.
50 changes: 42 additions & 8 deletions lib/compiler/src/beam_core_to_ssa.erl
Original file line number Diff line number Diff line change
Expand Up @@ -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">>;
Expand Down Expand Up @@ -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.
%%
Expand All @@ -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 ->
Expand All @@ -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) ->
Expand Down
72 changes: 71 additions & 1 deletion lib/compiler/test/warnings_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit 8a8ea04

Please sign in to comment.