From 01b9ffe731dba61331f53df1bf85bc3162f44f3a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jonatan=20K=C5=82osko?= <jonatanklosko@gmail.com>
Date: Mon, 21 Mar 2022 19:23:43 +0100
Subject: [PATCH] Keep binding in reversed evaluation order (#1065)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* Keep binding in reversed evaluation order

* Treat rebound names as new

* Add test for the default order

* Optimise binding reorder

* Update lib/livebook/runtime/evaluator.ex

Co-authored-by: José Valim <jose.valim@dashbit.co>

* Apply review comments

Co-authored-by: José Valim <jose.valim@dashbit.co>
---
 lib/livebook/runtime/evaluator.ex        | 19 +++++++++++++++++++
 test/livebook/runtime/evaluator_test.exs | 21 +++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/lib/livebook/runtime/evaluator.ex b/lib/livebook/runtime/evaluator.ex
index f7c7b60e2..14c20b3f8 100644
--- a/lib/livebook/runtime/evaluator.ex
+++ b/lib/livebook/runtime/evaluator.ex
@@ -322,6 +322,7 @@ defmodule Livebook.Runtime.Evaluator do
     {result_context, result, code_error} =
       case eval(code, context.binding, context.env) do
         {:ok, value, binding, env} ->
+          binding = reorder_binding(binding, context.binding)
           result_context = %{binding: binding, env: env, id: random_id()}
           result = {:ok, value}
           {result_context, result, nil}
@@ -454,6 +455,24 @@ defmodule Livebook.Runtime.Evaluator do
   defp code_error?(%CompileError{}), do: true
   defp code_error?(_error), do: false
 
+  defp reorder_binding(binding, prev_binding) do
+    # We keep the order of existing binding entries and move the new
+    # ones to the beginning
+
+    binding_map = Map.new(binding)
+
+    unchanged_binding =
+      Enum.filter(prev_binding, fn {key, prev_val} ->
+        val = binding_map[key]
+        :erts_debug.same(val, prev_val)
+      end)
+
+    unchanged_binding
+    |> Enum.reduce(binding_map, fn {key, _}, acc -> Map.delete(acc, key) end)
+    |> Map.to_list()
+    |> Kernel.++(unchanged_binding)
+  end
+
   # Adapted from https://github.com/elixir-lang/elixir/blob/1c1654c88adfdbef38ff07fc30f6fbd34a542c07/lib/iex/lib/iex/evaluator.ex#L355-L372
   # TODO: Remove else branch once we depend on the versions below
   if System.otp_release() >= "25" and Version.match?(System.version(), "~> 1.14-dev") do
diff --git a/test/livebook/runtime/evaluator_test.exs b/test/livebook/runtime/evaluator_test.exs
index 30d170496..4463452d0 100644
--- a/test/livebook/runtime/evaluator_test.exs
+++ b/test/livebook/runtime/evaluator_test.exs
@@ -310,6 +310,27 @@ defmodule Livebook.Runtime.EvaluatorTest do
     end
   end
 
+  describe "binding order" do
+    test "keeps binding in evaluation order, starting from most recent", %{evaluator: evaluator} do
+      Evaluator.evaluate_code(evaluator, "b = 1", :code_1)
+      Evaluator.evaluate_code(evaluator, "a = 1", :code_2, :code_1)
+      Evaluator.evaluate_code(evaluator, "c = 1", :code_3, :code_2)
+      Evaluator.evaluate_code(evaluator, "x = 1", :code_4, :code_3)
+
+      {:ok, %{binding: binding}} = Evaluator.fetch_evaluation_context(evaluator, :code_4)
+      assert [:x, :c, :a, :b] == Enum.map(binding, &elem(&1, 0))
+    end
+
+    test "treats rebound names as new", %{evaluator: evaluator} do
+      Evaluator.evaluate_code(evaluator, "b = 1", :code_1)
+      Evaluator.evaluate_code(evaluator, "a = 1", :code_2, :code_1)
+      Evaluator.evaluate_code(evaluator, "b = 2", :code_3, :code_2)
+
+      {:ok, %{binding: binding}} = Evaluator.fetch_evaluation_context(evaluator, :code_3)
+      assert [:b, :a] == Enum.map(binding, &elem(&1, 0))
+    end
+  end
+
   # Helpers
 
   # Some of the code passed to Evaluator above is expected