Improve linting of map expressions

Map fields are put in their own function instead of being clauses of expr/3.
Also, invalid map construction expressions now emit one error per ':=' field,
at the location of said field instead of one for the whole expression,
furthermore, such warnings do not stop linting of their key and value
expressions anymore. Ill-formed maps constructions are now also properly
detected in guard expressions.

2 files changed, 90 insertions, 44 deletions

diff --git a/lib/stdlib/src/erl_lint.erl b/lib/stdlib/src/erl_lint.erl
index 9f5be2da37..40523d136d 100644
--- a/lib/stdlib/src/erl_lint.erl
+++ b/lib/stdlib/src/erl_lint.erl
@@ -1373,18 +1373,19 @@ pattern({cons,_Line,H,T}, Vt, Old,  Bvt, St0) ->
 pattern({tuple,_Line,Ps}, Vt, Old, Bvt, St) ->
     pattern_list(Ps, Vt, Old, Bvt, St);
 pattern({map,_Line,Ps}, Vt, Old, Bvt, St) ->
-    pattern_list(Ps, Vt, Old, Bvt, St);
-pattern({map_field_assoc,Line,_,_}, _, _, _, St) ->
-    {[],[],add_error(Line, illegal_pattern, St)};
-pattern({map_field_exact,Line,KP,VP}, Vt, Old, Bvt0, St0) ->
-    %% if the key pattern has variables we should fail
-    case expr(KP,[],St0) of
-	{[],_} ->
-	    pattern(VP, Vt, Old, Bvt0, St0);
-	{[Var|_],_} ->
-	    %% found variables in key expression
-	    {Vt,Old,add_error(Line,{illegal_map_key_variable,element(1,Var)},St0)}
-    end;
+    foldl(fun ({map_field_assoc,L,_,_}, {Psvt,Bvt0,St0}) ->
+                  {Psvt,Bvt0,add_error(L, illegal_pattern, St0)};
+              ({map_field_exact,L,KP,VP}, {Psvt,Bvt0,St0}) ->
+                  case expr(KP, [], St0) of
+                        {[],_} ->
+                            {Pvt,Bvt1,St1} = pattern(VP, Vt, Old, Bvt, St0),
+                            {vtmerge_pat(Pvt, Psvt),vtmerge_pat(Bvt0, Bvt1),
+                             St1};
+                        {[Var|_],_} ->
+                            Error = {illegal_map_key_variable,element(1, Var)},
+                            {Psvt,Bvt0,add_error(L, Error, St0)}
+                  end
+          end, {[],[],St}, Ps);
 %%pattern({struct,_Line,_Tag,Ps}, Vt, Old, Bvt, St) ->
 %%    pattern_list(Ps, Vt, Old, Bvt, St);
 pattern({record_index,Line,Name,Field}, _Vt, _Old, _Bvt, St) ->
@@ -1773,13 +1774,11 @@ gexpr({cons,_Line,H,T}, Vt, St) ->
 gexpr({tuple,_Line,Es}, Vt, St) ->
     gexpr_list(Es, Vt, St);
 gexpr({map,_Line,Es}, Vt, St) ->
-    gexpr_list(Es, Vt, St);
+    map_fields(Es, Vt, check_assoc_fields(Es, St), fun gexpr_list/3);
 gexpr({map,_Line,Src,Es}, Vt, St) ->
-    gexpr_list([Src|Es], Vt, St);
-gexpr({map_field_assoc,_Line,K,V}, Vt, St) ->
-    gexpr_list([K,V], Vt, St);
-gexpr({map_field_exact,_Line,K,V}, Vt, St) ->
-    gexpr_list([K,V], Vt, St);
+    {Svt,St1} = gexpr(Src, Vt, St),
+    {Fvt,St2} = map_fields(Es, Vt, St1, fun gexpr_list/3),
+    {vtmerge(Svt, Fvt),St2};
 gexpr({record_index,Line,Name,Field}, _Vt, St) ->
     check_record(Line, Name, St,
                  fun (Dfs, St1) -> record_field(Field, Name, Dfs, St1) end );
@@ -1997,24 +1996,12 @@ expr({bc,_Line,E,Qs}, Vt, St) ->
     handle_comprehension(E, Qs, Vt, St);
 expr({tuple,_Line,Es}, Vt, St) ->
     expr_list(Es, Vt, St);
-expr({map,Line,Es}, Vt, St) ->
-    {Rvt,St1} = expr_list(Es,Vt,St),
-    case is_valid_map_construction(Es) of
-	true  -> {Rvt,St1};
-	false -> {[],add_error(Line,illegal_map_construction,St1)}
-    end;
+expr({map,_Line,Es}, Vt, St) ->
+    map_fields(Es, Vt, check_assoc_fields(Es, St), fun expr_list/3);
 expr({map,_Line,Src,Es}, Vt, St) ->
-    expr_list([Src|Es], Vt, St);
-expr({map_field_assoc,Line,K,V}, Vt, St) ->
-    case is_valid_map_key(K,St) of
-	true  -> expr_list([K,V], Vt, St);
-	{false,Var} -> {[],add_error(Line,{illegal_map_key_variable,Var},St)}
-    end;
-expr({map_field_exact,Line,K,V}, Vt, St) ->
-    case is_valid_map_key(K,St) of
-	true  -> expr_list([K,V], Vt, St);
-	{false,Var} -> {[],add_error(Line,{illegal_map_key_variable,Var},St)}
-    end;
+    {Svt,St1} = expr(Src, Vt, St),
+    {Fvt,St2} = map_fields(Es, Vt, St1, fun expr_list/3),
+    {vtupdate(Svt, Fvt),St2};
 expr({record_index,Line,Name,Field}, _Vt, St) ->
     check_record(Line, Name, St,
                  fun (Dfs, St1) -> record_field(Field, Name, Dfs, St1) end);
@@ -2222,6 +2209,25 @@ record_expr(Line, Rec, Vt, St0) ->
     St1 = warn_invalid_record(Line, Rec, St0),
     expr(Rec, Vt, St1).
 
+check_assoc_fields([{map_field_exact,Line,_,_}|Fs], St) ->
+    check_assoc_fields(Fs, add_error(Line, illegal_map_construction, St));
+check_assoc_fields([{map_field_assoc,_,_,_}|Fs], St) ->
+    check_assoc_fields(Fs, St);
+check_assoc_fields([], St) ->
+    St.
+
+map_fields([{Tag,Line,K,V}|Fs], Vt, St, F) when Tag =:= map_field_assoc;
+                                                Tag =:= map_field_exact ->
+    St1 = case is_valid_map_key(K, St) of
+              true -> St;
+              {false,Var} -> add_error(Line, {illegal_map_key_variable,Var}, St)
+          end,
+    {Pvt,St2} = F([K,V], Vt, St1),
+    {Vts,St3} = map_fields(Fs, Vt, St2, F),
+    {vtupdate(Pvt, Vts),St3};
+map_fields([], Vt, St, _) ->
+  {Vt,St}.
+
 %% warn_invalid_record(Line, Record, State0) -> State
 %% Adds warning if the record is invalid.
 
@@ -2274,13 +2280,6 @@ is_valid_call(Call) ->
         _ -> true
     end.
 
-%% check_map_construction
-%% Only #{ K => V }, i.e. assoc is a valid construction
-is_valid_map_construction([{map_field_assoc,_,_,_}|Es]) ->
-    is_valid_map_construction(Es);
-is_valid_map_construction([]) -> true;
-is_valid_map_construction(_)  -> false.
-
 is_valid_map_key(K,St) ->
     case expr(K,[],St) of
 	{[],_} -> true;
diff --git a/lib/stdlib/test/erl_lint_SUITE.erl b/lib/stdlib/test/erl_lint_SUITE.erl
index 1614a2722f..f93697731e 100644
--- a/lib/stdlib/test/erl_lint_SUITE.erl
+++ b/lib/stdlib/test/erl_lint_SUITE.erl
@@ -61,7 +61,8 @@
 	  on_load_successful/1, on_load_failing/1, 
 	  too_many_arguments/1,
 	  basic_errors/1,bin_syntax_errors/1,
-          predef/1
+          predef/1,
+          maps/1
         ]).
 
 % Default timetrap timeout (set in init_per_testcase).
@@ -88,7 +89,7 @@ all() ->
      otp_5878, otp_5917, otp_6585, otp_6885, otp_10436, otp_11254,export_all,
      bif_clash, behaviour_basic, behaviour_multiple,
      otp_7550, otp_8051, format_warn, {group, on_load},
-     too_many_arguments, basic_errors, bin_syntax_errors, predef].
+     too_many_arguments, basic_errors, bin_syntax_errors, predef, maps].
 
 groups() -> 
     [{unused_vars_warn, [],
@@ -3259,6 +3260,52 @@ predef(Config) when is_list(Config) ->
      {47,erl_lint,{deprecated_type,{tid,0},{ets,tid},"OTP 18.0"}}] = W2,
     ok.
 
+maps(Config) ->
+    %% TODO: test key patterns, not done because map patterns are going to be
+    %% changed a lot.
+    Ts = [{illegal_map_construction,
+           <<"t() ->
+                  #{ a := b,
+                     c => d,
+                     e := f
+                  }#{ a := b,
+                      c => d,
+                      e := f };
+              t() when is_map(#{ a := b,
+                                 c => d
+                              }#{ a := b,
+                                  c => d,
+                                  e := f }) ->
+                  ok.
+            ">>,
+           [],
+           {errors,[{2,erl_lint,illegal_map_construction},
+                    {4,erl_lint,illegal_map_construction},
+                    {8,erl_lint,illegal_map_construction}],
+            []}},
+          {illegal_pattern,
+           <<"t(#{ a := A,
+                   c => d,
+                   e := F,
+                   g := 1 + 1,
+                   h := _,
+                   i := (_X = _Y),
+                   j := (x ! y) }) ->
+                  {A,F}.
+            ">>,
+           [],
+           {errors,[{2,erl_lint,illegal_pattern},
+                    {7,erl_lint,illegal_pattern}],
+            []}},
+          {error_in_illegal_map_construction,
+           <<"t() -> #{ a := X }.">>,
+           [],
+           {errors,[{1,erl_lint,illegal_map_construction},
+                    {1,erl_lint,{unbound_var,'X'}}],
+            []}}],
+    [] = run(Config, Ts),
+    ok.
+
 run(Config, Tests) ->
     F = fun({N,P,Ws,E}, BadL) ->
                 case catch run_test(Config, P, Ws) of