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

2 files changed, 49 insertions, 20 deletions

diff --git a/lib/stdlib/src/erl_lint.erl b/lib/stdlib/src/erl_lint.erl
index 8f07750b9b..9284f08b30 100644
--- a/lib/stdlib/src/erl_lint.erl
+++ b/lib/stdlib/src/erl_lint.erl
@@ -1953,12 +1953,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) ->
@@ -2012,8 +2010,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!!
@@ -2111,12 +2108,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};
@@ -2150,10 +2147,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),
@@ -2843,7 +2841,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),
@@ -2856,7 +2856,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}
@@ -2920,7 +2924,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
@@ -3181,6 +3185,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 4dc7a44064..0bcf9b3e01 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.