From 1876d0d9e69159d278bc94d69ea0beb78903ad24 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Tue, 4 Feb 2014 18:59:51 +0100 Subject: 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. --- lib/stdlib/src/erl_lint.erl | 83 +++++++++++++++++++------------------- lib/stdlib/test/erl_lint_SUITE.erl | 51 ++++++++++++++++++++++- 2 files changed, 90 insertions(+), 44 deletions(-) (limited to 'lib/stdlib') 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 -- cgit v1.2.3