Merge pull request #1453 from bjorng/bjorn/catch-warning/OTP-14401

Warn for potentially unsafe use of get_stacktrace/0

4 files changed, 158 insertions, 14 deletions

diff --git a/erts/doc/src/erlang.xml b/erts/doc/src/erlang.xml
index 6f70ae4a52..6d165e9eff 100644
--- a/erts/doc/src/erlang.xml
+++ b/erts/doc/src/erlang.xml
@@ -1813,8 +1813,9 @@ true</pre>
       <fsummary>Get the call stack back-trace of the last exception.</fsummary>
       <type name="stack_item"/>
       <desc>
-        <p>Gets the call stack back-trace (<em>stacktrace</em>) of the
-          last exception in the calling process as a list of
+        <p>Gets the call stack back-trace (<em>stacktrace</em>) for an
+          exception that has just been caught
+	  in the calling process as a list of
           <c>{<anno>Module</anno>,<anno>Function</anno>,<anno>Arity</anno>,<anno>Location</anno>}</c>
           tuples. Field <c><anno>Arity</anno></c> in the first tuple can be the
           argument list of that function call instead of an arity integer,
@@ -1822,6 +1823,29 @@ true</pre>
         <p>If there has not been any exceptions in a process, the
           stacktrace is <c>[]</c>. After a code change for the process,
           the stacktrace can also be reset to <c>[]</c>.</p>
+	<p><c>erlang:get_stacktrace/0</c> is only guaranteed to return
+	a stacktrace if called (directly or indirectly) from within the
+	scope of a <c>try</c> expression. That is, the following call works:</p>
+<pre>
+try Expr
+catch
+  C:R ->
+   {C,R,erlang:get_stacktrace()}
+end</pre>
+	<p>As does this call:</p>
+<pre>
+try Expr
+catch
+  C:R ->
+   {C,R,helper()}
+end
+
+helper() ->
+  erlang:get_stacktrace().</pre>
+
+        <warning><p>In a future release,
+        <c>erlang:get_stacktrace/0</c> will return <c>[]</c> if called
+        from outside a <c>try</c> expression.</p></warning>
         <p>The stacktrace is the same data as operator <c>catch</c>
           returns, for example:</p>
         <pre>
diff --git a/lib/compiler/doc/src/compile.xml b/lib/compiler/doc/src/compile.xml
index fed68037c1..10164890f2 100644
--- a/lib/compiler/doc/src/compile.xml
+++ b/lib/compiler/doc/src/compile.xml
@@ -629,6 +629,14 @@ module.beam: module.erl \
             <p>Turns off warnings for unused record types. Default is to 
 	    emit warnings for unused locally defined record types.</p>
           </item>
+
+	  <tag><c>nowarn_get_stacktrace</c></tag>
+          <item>
+            <p>Turns off warnings for using <c>get_stacktrace/0</c> in a context
+	    where it will probably not work in a future release. For example,
+	    by default there will be a warning if <c>get_stacktrace/0</c> is
+	    used following a <c>catch</c> expression.</p>
+          </item>
         </taglist>
 
 	<p>Another class of warnings is generated by the compiler
diff --git a/lib/stdlib/src/erl_lint.erl b/lib/stdlib/src/erl_lint.erl
index 7c40058dd8..d53a31db0d 100644
--- a/lib/stdlib/src/erl_lint.erl
+++ b/lib/stdlib/src/erl_lint.erl
@@ -92,6 +92,14 @@ value_option(Flag, Default, On, OnVal, Off, OffVal, Opts) ->
               :: dict:dict(ta(), line())
          }).
 
+
+%% Are we outside or inside a catch or try/catch?
+-type catch_scope() :: 'none'
+                     | 'after_old_catch'
+                     | 'after_try'
+                     | 'wrong_part_of_try'
+                     | 'try_catch'.
+
 %% Define the lint state record.
 %% 'called' and 'exports' contain {Line, {Function, Arity}},
 %% the other function collections contain {Function, Arity}.
@@ -135,7 +143,9 @@ value_option(Flag, Default, On, OnVal, Off, OffVal, Opts) ->
                types = dict:new()               %Type definitions
                    :: dict:dict(ta(), #typeinfo{}),
                exp_types=gb_sets:empty()        %Exported types
-                   :: gb_sets:set(ta())
+                   :: gb_sets:set(ta()),
+               catch_scope = none               %Inside/outside try or catch
+                   :: catch_scope()
               }).
 
 -type lint_state() :: #lint{}.
@@ -223,7 +233,15 @@ format_error({redefine_old_bif_import,{F,A}}) ->
 format_error({redefine_bif_import,{F,A}}) ->
     io_lib:format("import directive overrides auto-imported BIF ~w/~w~n"
 		  " - use \"-compile({no_auto_import,[~w/~w]}).\" to resolve name clash", [F,A,F,A]);
-
+format_error({get_stacktrace,wrong_part_of_try}) ->
+    "erlang:get_stacktrace/0 used in the wrong part of 'try' expression. "
+        "(Use it in the block between 'catch' and 'end'.)";
+format_error({get_stacktrace,after_old_catch}) ->
+    "erlang:get_stacktrace/0 used following an old-style 'catch' "
+        "may stop working in a future release. (Use it inside 'try'.)";
+format_error({get_stacktrace,after_try}) ->
+    "erlang:get_stacktrace/0 used following a 'try' expression "
+        "may stop working in a future release. (Use it inside 'try'.)";
 format_error({deprecated, MFA, ReplacementMFA, Rel}) ->
     io_lib:format("~s is deprecated and will be removed in ~s; use ~s",
 		  [format_mfa(MFA), Rel, format_mfa(ReplacementMFA)]);
@@ -568,7 +586,10 @@ start(File, Opts) ->
 		      false, Opts)},
 	 {missing_spec_all,
 	  bool_option(warn_missing_spec_all, nowarn_missing_spec_all,
-		      false, Opts)}
+		      false, Opts)},
+         {get_stacktrace,
+          bool_option(warn_get_stacktrace, nowarn_get_stacktrace,
+                      true, Opts)}
 	],
     Enabled1 = [Category || {Category,true} <- Enabled0],
     Enabled = ordsets:from_list(Enabled1),
@@ -1405,8 +1426,9 @@ call_function(Line, F, A, #lint{usage=Usage0,called=Cd,func=Func,file=File}=St)
 %% function(Line, Name, Arity, Clauses, State) -> State.
 
 function(Line, Name, Arity, Cs, St0) ->
-    St1 = define_function(Line, Name, Arity, St0#lint{func={Name,Arity}}),
-    clauses(Cs, St1).
+    St1 = St0#lint{func={Name,Arity},catch_scope=none},
+    St2 = define_function(Line, Name, Arity, St1),
+    clauses(Cs, St2).
 
 -spec define_function(line(), atom(), arity(), lint_state()) -> lint_state().
 
@@ -2338,22 +2360,24 @@ expr({call,Line,F,As}, Vt, St0) ->
 expr({'try',Line,Es,Scs,Ccs,As}, Vt, St0) ->
     %% Currently, we don't allow any exports because later
     %% passes cannot handle exports in combination with 'after'.
-    {Evt0,St1} = exprs(Es, Vt, St0),
+    {Evt0,St1} = exprs(Es, Vt, St0#lint{catch_scope=wrong_part_of_try}),
     TryLine = {'try',Line},
     Uvt = vtunsafe(TryLine, Evt0, Vt),
     Evt1 = vtupdate(Uvt, Evt0),
-    {Sccs,St2} = icrt_clauses(Scs++Ccs, TryLine, vtupdate(Evt1, Vt), St1),
+    {Sccs,St2} = try_clauses(Scs, Ccs, TryLine,
+                             vtupdate(Evt1, Vt), St1),
     Rvt0 = Sccs,
     Rvt1 = vtupdate(vtunsafe(TryLine, Rvt0, Vt), Rvt0),
     Evt2 = vtmerge(Evt1, Rvt1),
     {Avt0,St} = exprs(As, vtupdate(Evt2, Vt), St2),
     Avt1 = vtupdate(vtunsafe(TryLine, Avt0, Vt), Avt0),
     Avt = vtmerge(Evt2, Avt1),
-    {Avt,St};
+    {Avt,St#lint{catch_scope=after_try}};
 expr({'catch',Line,E}, Vt, St0) ->
     %% No new variables added, flag new variables as unsafe.
     {Evt,St} = expr(E, Vt, St0),
-    {vtupdate(vtunsafe({'catch',Line}, Evt, Vt), Evt),St};
+    {vtupdate(vtunsafe({'catch',Line}, Evt, Vt), Evt),
+     St#lint{catch_scope=after_old_catch}};
 expr({match,_Line,P,E}, Vt, St0) ->
     {Evt,St1} = expr(E, Vt, St0),
     {Pvt,Bvt,St2} = pattern(P, vtupdate(Evt, Vt), St1),
@@ -3173,6 +3197,17 @@ is_module_dialyzer_option(Option) ->
                   error_handling,race_conditions,no_missing_calls,
                   specdiffs,overspecs,underspecs,unknown]).
 
+%% try_catch_clauses(Scs, Ccs, In, ImportVarTable, State) ->
+%%      {UpdVt,State}.
+
+try_clauses(Scs, Ccs, In, Vt, St0) ->
+    {Csvt0,St1} = icrt_clauses(Scs, Vt, St0),
+    St2 = St1#lint{catch_scope=try_catch},
+    {Csvt1,St3} = icrt_clauses(Ccs, Vt, St2),
+    Csvt = Csvt0 ++ Csvt1,
+    UpdVt = icrt_export(Csvt, Vt, In, St3),
+    {UpdVt,St3}.
+
 %% icrt_clauses(Clauses, In, ImportVarTable, State) ->
 %%      {UpdVt,State}.
 
@@ -3657,7 +3692,8 @@ has_wildcard_field([]) -> false.
 check_remote_function(Line, M, F, As, St0) ->
     St1 = deprecated_function(Line, M, F, As, St0),
     St2 = check_qlc_hrl(Line, M, F, As, St1),
-    format_function(Line, M, F, As, St2).
+    St3 = check_get_stacktrace(Line, M, F, As, St2),
+    format_function(Line, M, F, As, St3).
 
 %% check_qlc_hrl(Line, ModName, FuncName, [Arg], State) -> State
 %%  Add warning if qlc:q/1,2 has been called but qlc.hrl has not
@@ -3706,6 +3742,23 @@ deprecated_function(Line, M, F, As, St) ->
 	    St
     end.
 
+check_get_stacktrace(Line, erlang, get_stacktrace, [], St) ->
+    case St of
+        #lint{catch_scope=none} ->
+            St;
+        #lint{catch_scope=try_catch} ->
+            St;
+        #lint{catch_scope=Scope} ->
+            case is_warn_enabled(get_stacktrace, St) of
+                false ->
+                    St;
+                true ->
+                    add_warning(Line, {get_stacktrace,Scope}, St)
+            end
+    end;
+check_get_stacktrace(_, _, _, _, St) ->
+    St.
+
 -dialyzer({no_match, deprecated_type/5}).
 
 deprecated_type(L, M, N, As, St) ->
diff --git a/lib/stdlib/test/erl_lint_SUITE.erl b/lib/stdlib/test/erl_lint_SUITE.erl
index 03cad2c093..02524679fa 100644
--- a/lib/stdlib/test/erl_lint_SUITE.erl
+++ b/lib/stdlib/test/erl_lint_SUITE.erl
@@ -65,7 +65,8 @@
          maps/1,maps_type/1,maps_parallel_match/1,
          otp_11851/1,otp_11879/1,otp_13230/1,
          record_errors/1, otp_11879_cont/1,
-         non_latin1_module/1, otp_14323/1]).
+         non_latin1_module/1, otp_14323/1,
+         get_stacktrace/1]).
 
 suite() ->
     [{ct_hooks,[ts_install_cth]},
@@ -85,7 +86,8 @@ all() ->
      too_many_arguments, basic_errors, bin_syntax_errors, predef,
      maps, maps_type, maps_parallel_match,
      otp_11851, otp_11879, otp_13230,
-     record_errors, otp_11879_cont, non_latin1_module, otp_14323].
+     record_errors, otp_11879_cont, non_latin1_module, otp_14323,
+     get_stacktrace].
 
 groups() -> 
     [{unused_vars_warn, [],
@@ -3980,6 +3982,63 @@ otp_14323(Config) ->
     [] = run(Config, Ts),
     ok.
 
+get_stacktrace(Config) ->
+    Ts = [{old_catch,
+           <<"t1() ->
+                  catch error(foo),
+                  erlang:get_stacktrace().
+           ">>,
+           [],
+           {warnings,[{3,erl_lint,{get_stacktrace,after_old_catch}}]}},
+          {nowarn_get_stacktrace,
+           <<"t1() ->
+                  catch error(foo),
+                  erlang:get_stacktrace().
+           ">>,
+           [nowarn_get_stacktrace],
+           []},
+          {try_catch,
+           <<"t1(X) ->
+                  try abs(X) of
+                    _ ->
+                      erlang:get_stacktrace()
+                  catch
+                      _:_ -> ok
+                  end.
+
+              t2() ->
+                  try error(foo)
+                  catch _:_ -> ok
+                  end,
+                  erlang:get_stacktrace().
+
+              t3() ->
+                  try error(foo)
+                  catch _:_ ->
+                    try error(bar)
+                    catch _:_ ->
+                      ok
+                    end,
+                    erlang:get_stacktrace()
+                  end.
+
+              no_warning(X) ->
+                  try
+                     abs(X)
+                  catch
+                     _:_ ->
+                       erlang:get_stacktrace()
+                  end.
+           ">>,
+           [],
+           {warnings,[{4,erl_lint,{get_stacktrace,wrong_part_of_try}},
+                      {13,erl_lint,{get_stacktrace,after_try}},
+                      {22,erl_lint,{get_stacktrace,after_try}}]}}],
+
+    run(Config, Ts),
+    ok.
+
+
 run(Config, Tests) ->
     F = fun({N,P,Ws,E}, BadL) ->
                 case catch run_test(Config, P, Ws) of