Fix reentrancy of Code.eval_*

lib/elixir/src/elixir.erl

@@ -304,6 +304,9 @@ eval_forms(Tree, Binding, OrigE) ->
   eval_forms(Tree, Binding, OrigE, []).
 eval_forms(Tree, Binding, OrigE, Opts) ->
   Prune = proplists:get_value(prune_binding, Opts, false),
+  %% We save and restore the previous dbg_callback so nested eval calls in
+  %% the same process do not clobber the outer callback.
+  PreviousDbg = erlang:get({elixir, dbg_callback}),
   case proplists:get_value(dbg_callback, Opts) of
     undefined -> ok;
     DbgCallback -> erlang:put({elixir, dbg_callback}, DbgCallback)
@@ -332,7 +335,10 @@ eval_forms(Tree, Binding, OrigE, Opts) ->
         {Value, DumpedBinding, NewE#{versioned_vars := DumpedVars}}
     end
   after
-    erlang:erase({elixir, dbg_callback})
+    case PreviousDbg of
+      undefined -> erlang:erase({elixir, dbg_callback});
+      _ -> erlang:put({elixir, dbg_callback}, PreviousDbg)
+    end
   end.
 
 %% Evaluate Erlang code with careful handling of local and external functions
@@ -341,20 +347,27 @@ erl_eval(Expr, Binding, Env) ->
   LocalHandler = {value, fun ?MODULE:eval_local_handler/2},
   ExternalHandler = {value, fun ?MODULE:eval_external_handler/3},
 
+  %% ?elixir_eval_env is used by the external handler.
+  %%
+  %% The reason why we use the process dictionary to pass the environment
+  %% is because we want to avoid passing closures to erl_eval, as that
+  %% would effectively tie the eval code to the Elixir version and it is
+  %% best if it depends solely on Erlang/OTP.
+  %%
+  %% The downside is that functions that escape the eval context will no
+  %% longer have the original environment they came from.
+  %%
+  %% We save and restore the previous env so nested eval calls in the same
+  %% process do not clobber the outer env.
+  PreviousEvalEnv = erlang:get(?elixir_eval_env),
+  erlang:put(?elixir_eval_env, Env),
   try
-    %% ?elixir_eval_env is used by the external handler.
-    %%
-    %% The reason why we use the process dictionary to pass the environment
-    %% is because we want to avoid passing closures to erl_eval, as that
-    %% would effectively tie the eval code to the Elixir version and it is
-    %% best if it depends solely on Erlang/OTP.
-    %%
-    %% The downside is that functions that escape the eval context will no
-    %% longer have the original environment they came from.
-    erlang:put(?elixir_eval_env, Env),
     erl_eval:expr(Expr, Binding, LocalHandler, ExternalHandler)
   after
-    erlang:erase(?elixir_eval_env)
+    case PreviousEvalEnv of
+      undefined -> erlang:erase(?elixir_eval_env);
+      _ -> erlang:put(?elixir_eval_env, PreviousEvalEnv)
+    end
   end.
 
 eval_local_handler(FunName, Args) ->

lib/elixir/test/elixir/code_test.exs

@@ -288,6 +288,41 @@ defmodule CodeTest do
         assert {1, _binding} = Code.eval_string("dbg(1)", [])
       end)
     end
+
+    test "nested eval preserves outer :dbg_callback" do
+      opts = [dbg_callback: {__MODULE__, :dbg_callback_add_one, []}]
+
+      assert {{2, []}, _binding} =
+               Code.eval_string(
+                 """
+                 Code.eval_string("dbg(1)")
+                 """,
+                 [],
+                 opts
+               )
+    end
+
+    test "nested eval preserves outer env in exception stacktrace" do
+      env = %{Code.env_for_eval([]) | file: "outer_file.ex"}
+
+      stacktrace =
+        try do
+          Code.eval_string(
+            """
+            Code.eval_string("1 + 1")
+            raise "boom"
+            """,
+            [],
+            env
+          )
+        rescue
+          _ -> __STACKTRACE__
+        end
+
+      assert Enum.any?(stacktrace, fn
+               {_, _, _, meta} -> Keyword.get(meta, :file) == ~c"outer_file.ex"
+             end)
+    end
   end
 
   describe "eval_quoted/1" do