From 2a418eeb10f82673a7f7f77f878cc7e786c37d09 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Thu, 6 Jun 2013 01:34:25 +0200 Subject: Fix variable usage tracking in erl_lint When analyzing complex expressions (i.e. comprehensions, cases, tries, ifs and receives), erl_lint does not forget about old unused variables when returning the updated variable table. This causes a bug where old unused variables are not recorded as such: t(X, Y) -> #r{a=[ K || K <- Y ],b=[ K || K <- Y ]}. As erl_lint uses vtmerge_pat/2 to merge the results of the analysis of the two list comprehensions, X is marked as used and the warning is not emitted. The function vtmerge_pat/2 is used instead of the similar vtmerge/2 which does not mark multiple occurrences of a variable as usage to handle cases like the following one: t(X, Y) -> #r{a=A=X,b=A=Y}. Other simpler expressions like conses, tuples and external fun references do not correctly follow this behaviour, e.g. A is not marked as used in the following code: t(X, Y) -> {A=X,A=Y}. This commit fixes both issues and makes erl_lint not return old unused variables in updated tables and makes all compound expressions use vtmerge_pat/2. Reported-by: Anders Ramsell --- lib/stdlib/src/erl_lint.erl | 42 ++++++++++++++++++++++---------------- lib/stdlib/test/erl_lint_SUITE.erl | 27 ++++++++++++++++++++++-- 2 files changed, 49 insertions(+), 20 deletions(-) diff --git a/lib/stdlib/src/erl_lint.erl b/lib/stdlib/src/erl_lint.erl index 68a8534f15..4d02a00af8 100644 --- a/lib/stdlib/src/erl_lint.erl +++ b/lib/stdlib/src/erl_lint.erl @@ -1951,12 +1951,10 @@ expr({string,_Line,_S}, _Vt, St) -> {[],St}; expr({nil,_Line}, _Vt, St) -> {[],St}; expr({cons,_Line,H,T}, Vt, St) -> expr_list([H,T], Vt, St); -expr({lc,_Line,E,Qs}, Vt0, St0) -> - {Vt,St} = handle_comprehension(E, Qs, Vt0, St0), - {vtold(Vt, Vt0),St}; %Don't export local variables -expr({bc,_Line,E,Qs}, Vt0, St0) -> - {Vt,St} = handle_comprehension(E, Qs, Vt0, St0), - {vtold(Vt,Vt0),St}; %Don't export local variables +expr({lc,_Line,E,Qs}, Vt, St) -> + handle_comprehension(E, Qs, Vt, St); +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({record_index,Line,Name,Field}, _Vt, St) -> @@ -2010,8 +2008,7 @@ expr({'fun',Line,Body}, Vt, St) -> %%No one can think funs export! case Body of {clauses,Cs} -> - {Bvt, St1} = fun_clauses(Cs, Vt, St), - {vtupdate(Bvt, Vt), St1}; + fun_clauses(Cs, Vt, St); {function,F,A} -> %% BifClash - Fun expression %% N.B. Only allows BIFs here as well, NO IMPORTS!! @@ -2109,12 +2106,12 @@ expr({'try',Line,Es,Scs,Ccs,As}, Vt, St0) -> {Evt0,St1} = exprs(Es, Vt, St0), TryLine = {'try',Line}, Uvt = vtunsafe(vtnames(vtnew(Evt0, Vt)), TryLine, []), - Evt1 = vtupdate(Uvt, vtupdate(Evt0, Vt)), - {Sccs,St2} = icrt_clauses(Scs++Ccs, TryLine, Evt1, St1), + Evt1 = vtupdate(Uvt, Evt0), + {Sccs,St2} = icrt_clauses(Scs++Ccs, TryLine, vtupdate(Evt1, Vt), St1), Rvt0 = Sccs, Rvt1 = vtupdate(vtunsafe(vtnames(vtnew(Rvt0, Vt)), TryLine, []), Rvt0), Evt2 = vtmerge(Evt1, Rvt1), - {Avt0,St} = exprs(As, Evt2, St2), + {Avt0,St} = exprs(As, vtupdate(Evt2, Vt), St2), Avt1 = vtupdate(vtunsafe(vtnames(vtnew(Avt0, Vt)), TryLine, []), Avt0), Avt = vtmerge(Evt2, Avt1), {Avt,St}; @@ -2148,10 +2145,11 @@ expr({remote,Line,_M,_F}, _Vt, St) -> %% {UsedVarTable,State} expr_list(Es, Vt, St) -> - foldl(fun (E, {Esvt,St0}) -> - {Evt,St1} = expr(E, Vt, St0), - {vtmerge(Evt, Esvt),St1} - end, {[],St}, Es). + {Vt1,St1} = foldl(fun (E, {Esvt,St0}) -> + {Evt,St1} = expr(E, Vt, St0), + {vtmerge_pat(Evt, Esvt),St1} + end, {[],St}, Es), + {vtmerge(vtnew(Vt1, Vt), vtold(Vt1, Vt)),St1}. record_expr(Line, Rec, Vt, St0) -> St1 = warn_invalid_record(Line, Rec, St0), @@ -2848,7 +2846,9 @@ icrt_export(Csvt, Vt, In, St) -> Uvt = vtmerge(Evt, Unused), %% Make exported and unsafe unused variables unused in subsequent code: Vt2 = vtmerge(Uvt, vtsubtract(Vt1, Uvt)), - {Vt2,St}. + %% Forget about old variables which were not used: + Vt3 = vtmerge(vtnew(Vt2, Vt), vt_no_unused(vtold(Vt2, Vt))), + {Vt3,St}. handle_comprehension(E, Qs, Vt0, St0) -> {Vt1, Uvt, St1} = lc_quals(Qs, Vt0, St0), @@ -2861,7 +2861,11 @@ handle_comprehension(E, Qs, Vt0, St0) -> %% Local variables that have not been shadowed. {_,St} = check_unused_vars(Vt2, Vt0, St4), Vt3 = vtmerge(vtsubtract(Vt2, Uvt), Uvt), - {Vt3,St}. + %% Don't export local variables. + Vt4 = vtold(Vt3, Vt0), + %% Forget about old variables which were not used. + Vt5 = vt_no_unused(Vt4), + {Vt5,St}. %% lc_quals(Qualifiers, ImportVarTable, State) -> %% {VarTable,ShadowedVarTable,State} @@ -2925,7 +2929,7 @@ fun_clauses(Cs, Vt, St) -> {Cvt,St1} = fun_clause(C, Vt, St0), {vtmerge(Cvt, Bvt0),St1} end, {[],St#lint{recdef_top = false}}, Cs), - {Bvt,St2#lint{recdef_top = OldRecDef}}. + {vt_no_unused(vtold(Bvt, Vt)),St2#lint{recdef_top = OldRecDef}}. fun_clause({clause,_Line,H,G,B}, Vt0, St0) -> {Hvt,Binvt,St1} = head(H, Vt0, [], St0), % No imported pattern variables @@ -3186,6 +3190,8 @@ vt_no_unsafe(Vt) -> [V || {_,{S,_U,_L}}=V <- Vt, _ -> true end]. +vt_no_unused(Vt) -> [V || {_,{_,U,_L}}=V <- Vt, U =/= unused]. + %% vunion(VarTable1, VarTable2) -> [VarName]. %% vunion([VarTable]) -> [VarName]. %% vintersection(VarTable1, VarTable2) -> [VarName]. diff --git a/lib/stdlib/test/erl_lint_SUITE.erl b/lib/stdlib/test/erl_lint_SUITE.erl index f8345559c4..68945d6f39 100644 --- a/lib/stdlib/test/erl_lint_SUITE.erl +++ b/lib/stdlib/test/erl_lint_SUITE.erl @@ -151,7 +151,16 @@ unused_vars_warn_basic(Config) when is_list(Config) -> {22,erl_lint,{unused_var,'N'}}, {23,erl_lint,{shadowed_var,'N','fun'}}, {28,erl_lint,{unused_var,'B'}}, - {29,erl_lint,{unused_var,'B'}}]}}], + {29,erl_lint,{unused_var,'B'}}]}}, + {basic2, + <<"-record(r, {x,y}). + f({X,Y}) -> {Z=X,Z=Y}; + f([H|T]) -> [Z=H|Z=T]; + f(#r{x=X,y=Y}) -> #r{x=A=X,y=A=Y}. + g({M, F}) -> (Z=M):(Z=F)(); + g({M, F, Arg}) -> (Z=M):F(Z=Arg). + h(X, Y) -> (Z=X) + (Z=Y).">>, + [warn_unused_vars], []}], ?line [] = run(Config, Ts), ok. @@ -537,7 +546,21 @@ unused_vars_warn_rec(Config) when is_list(Config) -> end. ">>, [warn_unused_vars], - {warnings,[{22,erl_lint,{unused_var,'Same'}}]}}], + {warnings,[{22,erl_lint,{unused_var,'Same'}}]}}, + {rec2, + <<"-record(r, {a,b}). + f(X, Y) -> #r{a=[K || K <- Y], b=[K || K <- Y]}. + g(X, Y) -> #r{a=lists:map(fun (K) -> K end, Y), + b=lists:map(fun (K) -> K end, Y)}. + h(X, Y) -> #r{a=case Y of _ when is_list(Y) -> Y end, + b=case Y of _ when is_list(Y) -> Y end}. + i(X, Y) -> #r{a=if is_list(Y) -> Y end, b=if is_list(Y) -> Y end}. + ">>, + [warn_unused_vars], + {warnings,[{2,erl_lint,{unused_var,'X'}}, + {3,erl_lint,{unused_var,'X'}}, + {5,erl_lint,{unused_var,'X'}}, + {7,erl_lint,{unused_var,'X'}}]}}], ?line [] = run(Config, Ts), ok. -- cgit v1.2.1 From d6985f32ff416307f2c4a0d48b4ec7c403b3a962 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Mon, 22 Jul 2013 21:41:31 +0200 Subject: Fix unsafe variable tracking in try expressions Variables used in the body of a try expression were marked as unsafe *and* used, which makes no sense as an unsafe variable can't be used. Function vtsubtract/2 is used to forget usage of such unsafe variables. Reported-by: Paul Davis --- lib/stdlib/src/erl_lint.erl | 2 +- lib/stdlib/test/erl_lint_SUITE.erl | 19 ++++++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/lib/stdlib/src/erl_lint.erl b/lib/stdlib/src/erl_lint.erl index 4d02a00af8..2e791379c1 100644 --- a/lib/stdlib/src/erl_lint.erl +++ b/lib/stdlib/src/erl_lint.erl @@ -2106,7 +2106,7 @@ expr({'try',Line,Es,Scs,Ccs,As}, Vt, St0) -> {Evt0,St1} = exprs(Es, Vt, St0), TryLine = {'try',Line}, Uvt = vtunsafe(vtnames(vtnew(Evt0, Vt)), TryLine, []), - Evt1 = vtupdate(Uvt, Evt0), + Evt1 = vtupdate(Uvt, vtsubtract(Evt0, Uvt)), {Sccs,St2} = icrt_clauses(Scs++Ccs, TryLine, vtupdate(Evt1, Vt), St1), Rvt0 = Sccs, Rvt1 = vtupdate(vtunsafe(vtnames(vtnew(Rvt0, Vt)), TryLine, []), Rvt0), diff --git a/lib/stdlib/test/erl_lint_SUITE.erl b/lib/stdlib/test/erl_lint_SUITE.erl index 68945d6f39..a407a94c32 100644 --- a/lib/stdlib/test/erl_lint_SUITE.erl +++ b/lib/stdlib/test/erl_lint_SUITE.erl @@ -1098,7 +1098,24 @@ unsafe_vars_try(Config) when is_list(Config) -> {10,erl_lint,{unsafe_var,'Ra',{'try',3}}}, {10,erl_lint,{unsafe_var,'Rc',{'try',3}}}, {10,erl_lint,{unsafe_var,'Ro',{'try',3}}}], - []}}], + []}}, + {unsafe_try5, + <<"bang() -> + case 1 of + nil -> + Acc = 2; + _ -> + try + Acc = 3, + Acc + catch _:_ -> + ok + end + end, + Acc. + ">>, + [], + {errors,[{13,erl_lint,{unsafe_var,'Acc',{'try',6}}}],[]}}], ?line [] = run(Config, Ts), ok. -- cgit v1.2.1 From 30b282072f29f754a9c6d4b121cc04825b9d2d26 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Wed, 31 Jul 2013 10:31:51 +0200 Subject: Fix variable usage tracking in some record errors When reporting a field redefinition in a record, erl_lint can forget about some old unused variables. --- lib/stdlib/src/erl_lint.erl | 2 +- lib/stdlib/test/erl_lint_SUITE.erl | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/stdlib/src/erl_lint.erl b/lib/stdlib/src/erl_lint.erl index 2e791379c1..8d809b22df 100644 --- a/lib/stdlib/src/erl_lint.erl +++ b/lib/stdlib/src/erl_lint.erl @@ -2306,7 +2306,7 @@ check_fields(Fs, Name, Fields, Vt, St0, CheckFun) -> check_field({record_field,Lf,{atom,La,F},Val}, Name, Fields, Vt, St, Sfs, CheckFun) -> case member(F, Sfs) of - true -> {Sfs,{Vt,add_error(Lf, {redefine_field,Name,F}, St)}}; + true -> {Sfs,{[],add_error(Lf, {redefine_field,Name,F}, St)}}; false -> {[F|Sfs], case find_field(F, Fields) of diff --git a/lib/stdlib/test/erl_lint_SUITE.erl b/lib/stdlib/test/erl_lint_SUITE.erl index a407a94c32..5ff35cff9a 100644 --- a/lib/stdlib/test/erl_lint_SUITE.erl +++ b/lib/stdlib/test/erl_lint_SUITE.erl @@ -560,7 +560,14 @@ unused_vars_warn_rec(Config) when is_list(Config) -> {warnings,[{2,erl_lint,{unused_var,'X'}}, {3,erl_lint,{unused_var,'X'}}, {5,erl_lint,{unused_var,'X'}}, - {7,erl_lint,{unused_var,'X'}}]}}], + {7,erl_lint,{unused_var,'X'}}]}}, + {rec3, + <<"-record(r, {a}). + t() -> X = 1, #r{a=foo, a=bar}. + ">>, + [warn_unused_vars], + {error,[{2,erl_lint,{redefine_field,r,a}}], + [{2,erl_lint,{unused_var,'X'}}]}}], ?line [] = run(Config, Ts), ok. -- cgit v1.2.1