Rewrite merge of clause variable tables (in case, try, etc)

erl_lint:icrt_export/4 has been rewritten to make the code really
follow the scoping rules of Erlang, and not just in most situations
by accident.

 * The function should not depend on calling unused_vars/3 because that
   function does not return variables which begins with an underscore,
   something that only matters when emitting warnings. This could cause
   a compiler crash if such a variable was reused afterwards.

 * The variable tables from each clause are first merged together,
   lists:merge/1 is safe to use because they are orddicts and thus
   already sorted. This list is then traversed parallelly to the old
   variable table, again taking advantage of their sorted order.

 * The function does not emit warnings itself, there is no need to pass
   around the lint state. In the same vein, vtunsafe/3 has been rewritten
   to do more things by itself, given that all of its calls were similar.
   Finally, compiled-out code has been removed.

 * This reverts the code in 9ce148b1059e4da746a11f1d80a653340216c468,
   which fixed the compiler crash and made erl_lint remember unsafe
   variables, but forget about unused variables in the process.

 * Other places of the code which relied on the old clunky behaviour were
   also updated: unused and unsafe old variables are forgotten when
   merging fun clauses and boolean shortcircuiting operators do not rely
   on icrt_export/3 anymore.

2 files changed, 199 insertions, 76 deletions

diff --git a/lib/stdlib/src/erl_lint.erl b/lib/stdlib/src/erl_lint.erl
index 269e4b34cf..49f1f3a6ee 100644
--- a/lib/stdlib/src/erl_lint.erl
+++ b/lib/stdlib/src/erl_lint.erl
@@ -2065,8 +2065,8 @@ expr({'receive',Line,Cs,To,ToEs}, Vt, St0) ->
     {Cvt,St3} = icrt_clauses(Cs, Vt, St2),
     %% Csvts = [vtnew(Tevt, Vt)|Cvt],           %This is just NEW variables!
     Csvts = [Tevt|Cvt],
-    {Rvt,St4} = icrt_export(Csvts, Vt, {'receive',Line}, St3),
-    {vtmerge([Tvt,Tevt,Rvt]),St4};
+    Rvt = icrt_export(Csvts, Vt, {'receive',Line}),
+    {vtmerge([Tvt,Tevt,Rvt]),St3};
 expr({'fun',Line,Body}, Vt, St) ->
     %%No one can think funs export!
     case Body of
@@ -2177,21 +2177,20 @@ expr({'try',Line,Es,Scs,Ccs,As}, Vt, St0) ->
     %% passes cannot handle exports in combination with 'after'.
     {Evt0,St1} = exprs(Es, Vt, St0),
     TryLine = {'try',Line},
-    Uvt = vtunsafe(vtnames(vtnew(Evt0, Vt)), TryLine, []),
-    Evt1 = vtupdate(Uvt, vtsubtract(Evt0, Uvt)),
+    Uvt = vtunsafe(TryLine, Evt0, Vt),
+    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),
+    Rvt1 = vtupdate(vtunsafe(TryLine, Rvt0, Vt), Rvt0),
     Evt2 = vtmerge(Evt1, Rvt1),
     {Avt0,St} = exprs(As, vtupdate(Evt2, Vt), St2),
-    Avt1 = vtupdate(vtunsafe(vtnames(vtnew(Avt0, Vt)), TryLine, []), Avt0),
+    Avt1 = vtupdate(vtunsafe(TryLine, Avt0, Vt), Avt0),
     Avt = vtmerge(Evt2, Avt1),
     {Avt,St};
 expr({'catch',Line,E}, Vt, St0) ->
     %% No new variables added, flag new variables as unsafe.
-    {Evt,St1} = expr(E, Vt, St0),
-    Uvt = vtunsafe(vtnames(vtnew(Evt, Vt)), {'catch',Line}, []),
-    {vtupdate(Uvt,vtupdate(Evt, Vt)),St1};
+    {Evt,St} = expr(E, Vt, St0),
+    {vtupdate(vtunsafe({'catch',Line}, Evt, Vt), Evt),St};
 expr({match,_Line,P,E}, Vt, St0) ->
     {Evt,St1} = expr(E, Vt, St0),
     {Pvt,Bvt,St2} = pattern(P, vtupdate(Evt, Vt), St1),
@@ -2204,9 +2203,8 @@ expr({op,Line,Op,L,R}, Vt, St0) when Op =:= 'orelse'; Op =:= 'andalso' ->
     {Evt1,St1} = expr(L, Vt, St0),
     Vt1 = vtupdate(Evt1, Vt),
     {Evt2,St2} = expr(R, Vt1, St1),
-    Vt2 = vtmerge(Evt2, Vt1),
-    {Vt3,St3} = icrt_export([Vt1,Vt2], Vt1, {Op,Line}, St2),
-    {vtmerge(Evt1, Vt3),St3};
+    Evt3 = vtupdate(vtunsafe({Op,Line}, Evt2, Vt1), Evt2),
+    {vtmerge(Evt1, Evt3),St2};
 expr({op,_Line,_Op,L,R}, Vt, St) ->
     expr_list([L,R], Vt, St);                   %They see the same variables
 %% The following are not allowed to occur anywhere!
@@ -2946,11 +2944,12 @@ check_local_opaque_types(St) ->
     dict:fold(FoldFun, St, Ts).
 
 %% icrt_clauses(Clauses, In, ImportVarTable, State) ->
-%%      {NewVts,State}.
+%%      {UpdVt,State}.
 
 icrt_clauses(Cs, In, Vt, St0) ->
     {Csvt,St1} = icrt_clauses(Cs, Vt, St0),
-    icrt_export(Csvt, Vt, In, St1).
+    UpdVt = icrt_export(Csvt, Vt, In),
+    {UpdVt,St1}.
 
 %% icrt_clauses(Clauses, ImportVarTable, State) ->
 %%      {NewVts,State}.
@@ -2960,26 +2959,73 @@ icrt_clauses(Cs, Vt, St) ->
 
 icrt_clause({clause,_Line,H,G,B}, Vt0, St0) ->
     {Hvt,Binvt,St1} = head(H, Vt0, St0),
-    Vt1 = vtupdate(Hvt, vtupdate(Binvt, Vt0)),
-    {Gvt,St2} = guard(G, Vt1, St1),
+    Vt1 = vtupdate(Hvt, Binvt),
+    {Gvt,St2} = guard(G, vtupdate(Vt1, Vt0), St1),
     Vt2 = vtupdate(Gvt, Vt1),
-    {Bvt,St3} = exprs(B, Vt2, St2),
+    {Bvt,St3} = exprs(B, vtupdate(Vt2, Vt0), St2),
     {vtupdate(Bvt, Vt2),St3}.
 
-icrt_export(Csvt, Vt, In, St) ->
-    Vt1 = vtmerge(Csvt),
-    All = ordsets:subtract(vintersection(Csvt), vtnames(Vt)),
-    Some = ordsets:subtract(vtnames(Vt1), vtnames(Vt)),
-    Xvt = vtexport(All, In, []),
-    Evt = vtunsafe(ordsets:subtract(Some, All), In, Xvt),
-    Unused = vtmerge([unused_vars(Vt0, Vt, St) || Vt0 <- Csvt]),
-    %% Exported and unsafe variables may be unused:
-    Uvt = vtmerge(Evt, Unused),
-    %% Make exported and unsafe unused variables unused in subsequent code:
-    Vt2 = vtmerge(Uvt, vtsubtract(Vt1, Uvt)),
-    %% Forget about old variables which were not used:
-    Vt3 = vtmerge(vtnew(Vt2, Vt), vt_no_unused(vtold(Vt2, Vt))),
-    {Vt3,St}.
+icrt_export(Vts, Vt, {Tag,Attrs}) ->
+    {_File,Loc} = loc(Attrs),
+    icrt_export(lists:merge(Vts), Vt, {Tag,Loc}, length(Vts), []).
+
+icrt_export([{V,{{export,_},_,_}}|Vs0], [{V,{{export,_}=S0,_,Ls}}|Vt],
+            In, I, Acc) ->
+    %% V was an exported variable and has been used in an expression in at least
+    %% one clause. Its state needs to be merged from all clauses to silence any
+    %% exported var warning already emitted.
+    {VVs,Vs} = lists:partition(fun ({K,_}) -> K =:= V end, Vs0),
+    S = foldl(fun ({_,{S1,_,_}}, AccS) -> merge_state(AccS, S1) end, S0, VVs),
+    icrt_export(Vs, Vt, In, I, [{V,{S,used,Ls}}|Acc]);
+icrt_export([{V,_}|Vs0], [{V,{_,_,Ls}}|Vt], In, I, Acc) ->
+    %% V was either unsafe or bound and has now been reused. It may also have
+    %% been an export but as it was not matched by the previous clause, it means
+    %% it has been changed to 'bound' in at least one clause because it was used
+    %% in a pattern.
+    Vs = lists:dropwhile(fun ({K,_}) -> K =:= V end, Vs0),
+    icrt_export(Vs, Vt, In, I, [{V,{bound,used,Ls}}|Acc]);
+icrt_export([{V1,_}|_]=Vs, [{V2,_}|Vt], In, I, Acc) when V1 > V2 ->
+    %% V2 was already in scope and has not been reused in any clause.
+    icrt_export(Vs, Vt, In, I, Acc);
+icrt_export([{V,_}|_]=Vs0, Vt, In, I, Acc) ->
+    %% V is a new variable.
+    {VVs,Vs} = lists:partition(fun ({K,_}) -> K =:= V end, Vs0),
+    F = fun ({_,{S,U,Ls}}, {AccI,AccS0,AccLs0}) ->
+                AccS = case {S,AccS0} of
+                           {{unsafe,_},{unsafe,_}} ->
+                               %% V was found unsafe in a previous clause, mark
+                               %% it as unsafe for the whole parent expression.
+                               {unsafe,In};
+                           {{unsafe,_},_} ->
+                               %% V was unsafe in a clause, keep that state and
+                               %% generalize it to the whole expression if it
+                               %% is found unsafe in another one.
+                               S;
+                           _ ->
+                               %% V is either bound or exported, keep original
+                               %% state.
+                               AccS0
+                       end,
+                AccLs = case U of
+                            used -> AccLs0;
+                            unused -> merge_lines(AccLs0, Ls)
+                        end,
+                {AccI + 1,AccS,AccLs}
+        end,
+    %% Initial state is exported from the current expression.
+    {Count,S1,Ls} = foldl(F, {0,{export,In},[]}, VVs),
+    S = case Count of
+            I ->
+                %% V was found in all clauses, keep computed state.
+                S1;
+            _ ->
+                %% V was not bound in some clauses, mark as unsafe.
+                {unsafe,In}
+        end,
+    U = case Ls of [] -> used; _ -> unused end,
+    icrt_export(Vs, Vt, In, I, [{V,{S,U,Ls}}|Acc]);
+icrt_export([], _, _, _, Acc) ->
+    reverse(Acc).
 
 handle_comprehension(E, Qs, Vt0, St0) ->
     {Vt1, Uvt, St1} = lc_quals(Qs, Vt0, St0),
@@ -3077,7 +3123,8 @@ fun_clauses(Cs, Vt, St) ->
                               {Cvt,St1} = fun_clause(C, Vt, St0),
                               {vtmerge(Cvt, Bvt0),St1}
                       end, {[],St#lint{recdef_top = false}}, Cs),
-    {vt_no_unused(vtold(Bvt, Vt)),St2#lint{recdef_top = OldRecDef}}.
+    Uvt = vt_no_unsafe(vt_no_unused(vtold(Bvt, Vt))),
+    {Uvt,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
@@ -3191,19 +3238,24 @@ pat_binsize_var(V, Line, Vt, Bvt, St) ->
 %%  exported vars are probably safe, warn only if warn_export_vars is
 %%  set.
 
-expr_var(V, Line, Vt, St0) ->
+expr_var(V, Line, Vt, St) ->
     case orddict:find(V, Vt) of
         {ok,{bound,_Usage,Ls}} ->
-            {[{V,{bound,used,Ls}}],St0};
+            {[{V,{bound,used,Ls}}],St};
         {ok,{{unsafe,In},_Usage,Ls}} ->
             {[{V,{bound,used,Ls}}],
-             add_error(Line, {unsafe_var,V,In}, St0)};
+             add_error(Line, {unsafe_var,V,In}, St)};
         {ok,{{export,From},_Usage,Ls}} ->
-            {[{V,{bound,used,Ls}}],
-             exported_var(Line, V, From, St0)};
+            case is_warn_enabled(export_vars, St) of
+                true ->
+                    {[{V,{bound,used,Ls}}],
+                     add_warning(Line, {exported_var,V,From}, St)};
+                false ->
+                    {[{V,{{export,From},used,Ls}}],St}
+            end;
         error ->
             {[{V,{bound,used,[Line]}}],
-             add_error(Line, {unbound_var,V}, St0)}
+             add_error(Line, {unbound_var,V}, St)}
     end.
 
 exported_var(Line, V, From, St) ->
@@ -3267,17 +3319,12 @@ vtupdate(Uvt, Vt0) ->
                           {S, merge_used(U1, U2), merge_lines(L1, L2)}
                   end, Uvt, Vt0).
 
-%% vtexport([Variable], From, VarTable) -> VarTable.
-%% vtunsafe([Variable], From, VarTable) -> VarTable.
-%%  Add the variables to VarTable either as exported from From or as unsafe.
-
-vtexport(Vs, {InTag,FileLine}, Vt0) ->
-    {_File,Line} = loc(FileLine),
-    vtupdate([{V,{{export,{InTag,Line}},unused,[]}} || V <- Vs], Vt0).
+%% vtunsafe(From, UpdVarTable, VarTable) -> UnsafeVarTable.
+%%  Return all new variables in UpdVarTable as unsafe.
 
-vtunsafe(Vs, {InTag,FileLine}, Vt0) ->
+vtunsafe({Tag,FileLine}, Uvt, Vt) ->
     {_File,Line} = loc(FileLine),
-    vtupdate([{V,{{unsafe,{InTag,Line}},unused,[]}} || V <- Vs], Vt0).
+    [{V,{{unsafe,{Tag,Line}},U,Ls}} || {V,{_,U,Ls}} <- vtnew(Uvt, Vt)].
 
 %% vtmerge(VarTable, VarTable) -> VarTable.
 %%  Merge two variables tables generating a new vartable. Give priority to
@@ -3330,8 +3377,6 @@ vtsubtract(New, Old) ->
 vtold(New, Old) ->
     orddict:filter(fun (V, _How) -> orddict:is_key(V, Old) end, New).
 
-vtnames(Vt) -> [ V || {V,_How} <- Vt ].
-
 vt_no_unsafe(Vt) -> [V || {_,{S,_U,_L}}=V <- Vt,
                           case S of
                               {unsafe,_} -> false;
@@ -3340,29 +3385,6 @@ vt_no_unsafe(Vt) -> [V || {_,{S,_U,_L}}=V <- Vt,
 
 vt_no_unused(Vt) -> [V || {_,{_,U,_L}}=V <- Vt, U =/= unused].
 
-%% vunion(VarTable1, VarTable2) -> [VarName].
-%% vunion([VarTable]) -> [VarName].
-%% vintersection(VarTable1, VarTable2) -> [VarName].
-%% vintersection([VarTable]) -> [VarName].
-%%  Union/intersection of names of vars in VarTable.
-
--ifdef(NOTUSED).
-vunion(Vs1, Vs2) -> ordsets:union(vtnames(Vs1), vtnames(Vs2)).
-
-vunion(Vss) -> foldl(fun (Vs, Uvs) ->
-                             ordsets:union(vtnames(Vs), Uvs)
-                     end, [], Vss).
-
-vintersection(Vs1, Vs2) -> ordsets:intersection(vtnames(Vs1), vtnames(Vs2)).
--endif.
-
-vintersection([Vs]) ->
-    vtnames(Vs);                %Boundary conditions!!!
-vintersection([Vs|Vss]) ->
-    ordsets:intersection(vtnames(Vs), vintersection(Vss));
-vintersection([]) ->
-    [].
-
 %% copy_expr(Expr, Line) -> Expr.
 %%  Make a copy of Expr converting all line numbers to Line.
 
diff --git a/lib/stdlib/test/erl_lint_SUITE.erl b/lib/stdlib/test/erl_lint_SUITE.erl
index 5d189006a1..049ca8e963 100644
--- a/lib/stdlib/test/erl_lint_SUITE.erl
+++ b/lib/stdlib/test/erl_lint_SUITE.erl
@@ -42,6 +42,7 @@
 	  unused_vars_warn_rec/1,
 	  unused_vars_warn_fun/1, 
 	  unused_vars_OTP_4858/1,
+          unused_unsafe_vars_warn/1,
 	  export_vars_warn/1,
 	  shadow_vars/1,
 	  unused_import/1,
@@ -97,7 +98,7 @@ groups() ->
     [{unused_vars_warn, [],
       [unused_vars_warn_basic, unused_vars_warn_lc,
        unused_vars_warn_rec, unused_vars_warn_fun,
-       unused_vars_OTP_4858]},
+       unused_vars_OTP_4858, unused_unsafe_vars_warn]},
      {on_load, [], [on_load_successful, on_load_failing]}].
 
 init_per_suite(Config) ->
@@ -729,6 +730,48 @@ unused_vars_OTP_4858(Config) when is_list(Config) ->
     ?line [] = run(Config, Ts),
     ok.
 
+unused_unsafe_vars_warn(Config) when is_list(Config) ->
+    Ts = [{unused_unsafe1,
+           <<"t1() ->
+                  UnusedVar1 = unused1,
+                  try
+                      UnusedVar2 = unused2
+                  catch
+                      _:_ ->
+                          ok
+                  end,
+                  ok.
+           ">>,
+           [warn_unused_vars],
+           {warnings,[{2,erl_lint,{unused_var,'UnusedVar1'}},
+                      {4,erl_lint,{unused_var,'UnusedVar2'}}]}},
+          {unused_unsafe2,
+           <<"t2() ->
+                  try
+                      X = 1
+                  catch
+                      _:_ -> ok
+                  end.
+           ">>,
+           [warn_unused_vars],
+           {warnings,[{3,erl_lint,{unused_var,'X'}}]}},
+          {unused_unsafe2,
+           <<"t3(X, Y) ->
+                  X andalso Y.
+           ">>,
+           [warn_unused_vars],
+           []},
+          {unused_unsafe4,
+           <<"t4() ->
+                  _ = (catch X = X = 1),
+                  _ = case ok of _ -> fun() -> ok end end,
+                  fun (X) -> X end.
+           ">>,
+           [warn_unused_vars],
+           []}],
+    run(Config, Ts),
+    ok.
+
 export_vars_warn(doc) ->
     "Warnings for exported variables";
 export_vars_warn(suite) -> [];
@@ -807,7 +850,19 @@ export_vars_warn(Config) when is_list(Config) ->
            [],
            {error,[{9,erl_lint,{unbound_var,'B'}}],
                   [{9,erl_lint,{exported_var,'Y',{'receive',2}}},
-                   {10,erl_lint,{shadowed_var,'B',generate}}]}}
+                   {10,erl_lint,{shadowed_var,'B',generate}}]}},
+
+          {exp4,
+           <<"t(X) ->
+                  if true -> Z = X end,
+                  case X of
+                      1 -> Z;
+                      2 -> X
+                  end,
+                  Z = X.
+           ">>,
+           [],
+           {warnings,[{7,erl_lint,{exported_var,'Z',{'if',2}}}]}}
          ],
     ?line [] = run(Config, Ts),
     ok.
@@ -831,8 +886,15 @@ shadow_vars(Config) when is_list(Config) ->
            ">>,
 	   [nowarn_shadow_vars],
 	   {error,[{9,erl_lint,{unbound_var,'B'}}],
-	    [{9,erl_lint,{exported_var,'Y',{'receive',2}}}]}}],
-    
+	    [{9,erl_lint,{exported_var,'Y',{'receive',2}}}]}},
+          {shadow2,
+           <<"t() ->
+                  _ = (catch MS = MS = 1), % MS used unsafe
+                  _ = case ok of _ -> fun() -> ok end end,
+                  fun (MS) -> MS end. % MS not shadowed here
+           ">>,
+           [],
+           []}],
     ?line [] = run(Config, Ts),
     ok.
 
@@ -957,6 +1019,45 @@ unsafe_vars(Config) when is_list(Config) ->
            [warn_unused_vars],
            {errors,[{3,erl_lint,{unsafe_var,'X',{'if',2}}},
                     {4,erl_lint,{unsafe_var,'X',{'if',2}}}],
+            []}},
+          {unsafe8,
+           <<"t8(X) ->
+                  case X of _ -> catch _Y = 1 end,
+                  _Y."
+           >>,
+           [],
+           {errors,[{3,erl_lint,{unsafe_var,'_Y',{'catch',2}}}],
+            []}},
+           {unsafe9,
+           <<"t9(X) ->
+                  case X of
+                      1 ->
+                          catch A = 1, % unsafe only here
+                          B = 1,
+                          C = 1,
+                          D = 1;
+                      2 ->
+                          A = 2,
+                          % B not bound here
+                          C = 2,
+                          catch D = 2; % unsafe in two clauses
+                      3 ->
+                          A = 3,
+                          B = 3,
+                          C = 3,
+                          catch D = 3; % unsafe in two clauses
+                      4 ->
+                          A = 4,
+                          B = 4,
+                          C = 4,
+                          D = 4
+                  end,
+                  {A,B,C,D}."
+           >>,
+           [],
+           {errors,[{24,erl_lint,{unsafe_var,'A',{'catch',4}}},
+                    {24,erl_lint,{unsafe_var,'B',{'case',2}}},
+                    {24,erl_lint,{unsafe_var,'D',{'case',2}}}],
             []}}
          ],
     ?line [] = run(Config, Ts),