Repository: livebook-dev/kino
Pull request #363 (2023-11-08 14:32:37)

Make remote execution cell work regardless of Elixir and OTP versions
🐈‍⬛

mrluc: Haha, seems like an obvious way to fix this in retrospect! There's elixir on the other side, so do version-safe rpc of just Code.eval_string, on a string of user's code, instrumented.

```elixir
...
    {nil, binding} =
      :erpc.call(node, Code, :eval_string, [   #<-- this line right here
        eval_string,
        [input: {string, binding, opts}],
        [file: @relative_file, line: line]
      ])

    {result, diagnostics} = binding[:output]

    for diagnostic <- diagnostics do
      Code.print_diagnostic(diagnostic)
    end

    case result do
      {:ok, value} ->
        value
...
```

DECE

Patch:
diff --git a/lib/assets/remote_execution_cell/main.css b/lib/assets/remote_execution_cell/main.css
index 0b0ed6d..8853471 100644
--- a/lib/assets/remote_execution_cell/main.css
+++ b/lib/assets/remote_execution_cell/main.css
@@ -202,71 +202,3 @@ select.input {
 .hidden-checkbox:hover {
   cursor: pointer;
 }
-
-.icon-button {
-  background: none;
-  border: none;
-  cursor: pointer;
-  padding: 4px;
-  display: flex;
-  align-items: center;
-  justify-content: center;
-  border-radius: 100%;
-  color: var(--gray-500);
-  line-height: 1;
-}
-
-.icon-button:hover {
-  color: var(--gray-900);
-}
-
-.icon-button:focus {
-  outline: none;
-}
-
-.icon-button:disabled {
-  color: var(--gray-300);
-  cursor: default;
-}
-
-a.icon-button {
-  text-decoration: none;
-}
-
-a.icon-button img {
-  height: 1.3rem;
-}
-
-.ri {
-  font-size: 1.25rem;
-  vertical-align: middle;
-  line-height: 1;
-}
-
-.help-section {
-  padding: 16px;
-  font-size: 0.875rem;
-  line-height: 1.5em;
-  color: var(--gray-700);
-  border: solid 1px var(--gray-300);
-  background-color: rgba(248, 250, 252, 0.3);
-}
-
-.help-section > *:not(:first-child) {
-  margin-top: 20px;
-}
-
-.help-section h3 {
-  margin-bottom: 8px;
-  font-size: 1.125em;
-}
-
-.help-section a {
-  color: var(--gray-900);
-  font-weight: 500;
-  text-decoration: underline;
-}
-
-.help-section a:hover {
-  text-decoration: none;
-}
diff --git a/lib/assets/remote_execution_cell/main.js b/lib/assets/remote_execution_cell/main.js
index e0a8182..17bea23 100644
--- a/lib/assets/remote_execution_cell/main.js
+++ b/lib/assets/remote_execution_cell/main.js
@@ -4,9 +4,6 @@ export function init(ctx, payload) {
   ctx.importCSS(
     "https://fonts.googleapis.com/css2?family=Inter:wght@400;500&display=swap"
   );
-  ctx.importCSS(
-    "https://cdn.jsdelivr.net/npm/remixicon@2.5.0/fonts/remixicon.min.css"
-  );
   ctx.importCSS("main.css");
 
   const BaseInput = {
@@ -214,12 +211,6 @@ export function init(ctx, payload) {
             inputClass="input input--xs"
             :inline
           />
-          <button type="button" @click="toggleHelpBox" class="icon-button grow">
-            <i class="ri ri-questionnaire-line" aria-hidden="true"></i>
-          </button>
-        </div>
-        <div class="help-section" v-if="showHelpBox">
-          <p>The Erlang/OTP version of both nodes need to match and the remote node must run Elixir v1.15.7 or later.</p>
         </div>
       </form>
     </div>
@@ -228,7 +219,6 @@ export function init(ctx, payload) {
     data() {
       return {
         fields: payload.fields,
-        showHelpBox: false,
       };
     },
 
@@ -248,9 +238,6 @@ export function init(ctx, payload) {
           : this.fields["cookie"];
         ctx.setSmartCellEditorIntellisenseNode(node, cookie);
       },
-      toggleHelpBox() {
-        this.showHelpBox = !this.showHelpBox;
-      },
     },
 
     mounted() {
diff --git a/lib/kino/remote_execution_cell.ex b/lib/kino/remote_execution_cell.ex
index 7d78cea..f2911d5 100644
--- a/lib/kino/remote_execution_cell.ex
+++ b/lib/kino/remote_execution_cell.ex
@@ -76,18 +76,15 @@ defmodule Kino.RemoteExecutionCell do
   def to_source(%{"use_cookie_secret" => false, "cookie" => ""}), do: ""
   def to_source(%{"use_cookie_secret" => true, "cookie_secret" => ""}), do: ""
 
-  def to_source(%{"code" => code} = attrs) do
-    code = Code.string_to_quoted(code)
-    to_source(attrs, code)
-  end
-
-  defp to_source(%{"assign_to" => var} = attrs, {:ok, code}) do
+  def to_source(%{"code" => code, "assign_to" => var} = attrs) do
     var = if Kino.SmartCell.valid_variable_name?(var), do: var
     call = build_call(code) |> build_var(var)
     cookie = build_set_cookie(attrs)
     node = build_node(attrs)
 
     quote do
+      require Kino.RPC
+
       node = unquote(node)
       Node.set_cookie(node, unquote(cookie))
       unquote(call)
@@ -95,18 +92,17 @@ defmodule Kino.RemoteExecutionCell do
     |> Kino.SmartCell.quoted_to_string()
   end
 
-  defp to_source(%{"code" => code}, {:error, _reason}) do
-    "# Invalid code for RPC, reproducing the error below\n" <>
-      Kino.SmartCell.quoted_to_string(
-        quote do
-          Code.string_to_quoted!(unquote(code))
-        end
-      )
-  end
-
   defp build_call(code) do
     quote do
-      :erpc.call(node, fn -> unquote(code) end)
+      Kino.RPC.eval_string(node, unquote(quoted_code(code)), file: __ENV__.file)
+    end
+  end
+
+  defp quoted_code(code) do
+    if String.contains?(code, "\n") do
+      {:<<>>, [delimiter: ~s["""]], [code <> "\n"]}
+    else
+      code
     end
   end
 
diff --git a/lib/kino/rpc.ex b/lib/kino/rpc.ex
new file mode 100644
index 0000000..836bd31
--- /dev/null
+++ b/lib/kino/rpc.ex
@@ -0,0 +1,128 @@
+defmodule Kino.RPC do
+  @moduledoc """
+  Functions for working with remote nodes.
+  """
+
+  @relative_file Path.relative_to_cwd(__ENV__.file)
+
+  @doc """
+  Evaluates the contents given by `string` on the given `node`.
+
+  Returns the value returned from evaluation.
+
+  The code is analyzed for variable references, they are automatically
+  extracted from the caller binding and passed to the evaluation. This
+  means that the evaluated string actually has closure semantics.
+
+  The code is parsed and expanded on the remote node. Also, errors
+  and exists are captured and propagated to the caller.
+
+  See `Code.eval_string/3` for available `opts`.
+  """
+  defmacro eval_string(node, string, opts \\ []) do
+    unless is_binary(string) do
+      raise ArgumentError,
+            "Kino.RPC.eval_string/3 expects a string literal as the second argument"
+    end
+
+    used_var_names = used_var_names(string, __CALLER__)
+
+    binding = for name <- used_var_names, do: {name, Macro.var(name, nil)}
+
+    quote do
+      Kino.RPC.__remote_eval_string__(
+        unquote(node),
+        unquote(string),
+        unquote(binding),
+        unquote(opts)
+      )
+    end
+  end
+
+  defp used_var_names(string, env) do
+    # TODO: only keep :emit_warnings once we require Elixir v1.16+
+    case Code.string_to_quoted(string, emit_warnings: false, warn_on_unnecessary_quotes: false) do
+      {:ok, ast} ->
+        # This is a simple heuristic, we traverse the unexpanded AST
+        # and look for any variable node. This means we may have false
+        # positives if there are macros, but in our use case this is
+        # acceptable. We may also have false negatives in very specific
+        # edge cases, such as calling `binding()`, but these are even
+        # more unlikely.
+
+        names = Map.new(Macro.Env.vars(env))
+
+        ast
+        |> Macro.prewalk(MapSet.new(), fn
+          {name, _, nil} = node, acc when is_map_key(names, name) ->
+            {node, MapSet.put(acc, name)}
+
+          node, acc ->
+            {node, acc}
+        end)
+        |> elem(1)
+
+      {:error, _} ->
+        []
+    end
+  end
+
+  @doc false
+  def __remote_eval_string__(node, string, binding, opts) do
+    opts = Keyword.validate!(opts, [:file, :line])
+
+    # We do a nested evaluation to catch errors and capture diagnostics.
+    # Also, note that `eval_string` returns both result and binding,
+    # so in order to minimize the data sent between nodes, we bind the
+    # result and diagnostics to `output` and we rebind `input` to `nil`.
+
+    line = __ENV__.line + 4
+
+    eval_string =
+      """
+      output =
+        Code.with_diagnostics([log: false], fn ->
+          {string, binding, opts} = input
+
+          try do
+            quoted = Code.string_to_quoted!(string, opts)
+            {value, _binding} = Code.eval_quoted(quoted, binding, opts)
+            {:ok, value}
+          catch
+            kind, error ->
+              {:error, kind, error, __STACKTRACE__}
+          end
+        end)
+
+      input = nil
+      """
+
+    {nil, binding} =
+      :erpc.call(node, Code, :eval_string, [
+        eval_string,
+        [input: {string, binding, opts}],
+        [file: @relative_file, line: line]
+      ])
+
+    {result, diagnostics} = binding[:output]
+
+    for diagnostic <- diagnostics do
+      Code.print_diagnostic(diagnostic)
+    end
+
+    case result do
+      {:ok, value} ->
+        value
+
+      {:error, :error, error, stacktrace} ->
+        error = Exception.normalize(:error, error, stacktrace)
+        reraise error, stacktrace
+
+      {:error, :throw, value, _stacktrace} ->
+        throw(value)
+
+      {:error, :exit, reason, _stacktrace} ->
+        exit(reason)
+    end
+  end
+end


Test patch:
diff --git a/test/kino/remote_execution_cell_test.exs b/test/kino/remote_execution_cell_test.exs
index 6f3b6ec..c031a40 100644
--- a/test/kino/remote_execution_cell_test.exs
+++ b/test/kino/remote_execution_cell_test.exs
@@ -29,9 +29,10 @@ defmodule Kino.RemoteExecutionCellTest do
     {_kino, source} = start_smart_cell!(RemoteExecutionCell, @fields)
 
     assert source == """
+           require Kino.RPC
            node = :name@node
            Node.set_cookie(node, :"node-cookie")
-           :erpc.call(node, fn -> :ok end)\
+           Kino.RPC.eval_string(node, ":ok", file: __ENV__.file)\
            """
   end
 
@@ -40,9 +41,10 @@ defmodule Kino.RemoteExecutionCellTest do
     {_kino, source} = start_smart_cell!(RemoteExecutionCell, attrs)
 
     assert source == """
+           require Kino.RPC
            node = :name@node
            Node.set_cookie(node, :"node-cookie")
-           result = :erpc.call(node, fn -> :ok end)\
+           result = Kino.RPC.eval_string(node, ":ok", file: __ENV__.file)\
            """
   end
 
@@ -51,9 +53,10 @@ defmodule Kino.RemoteExecutionCellTest do
     {_kino, source} = start_smart_cell!(RemoteExecutionCell, attrs)
 
     assert source == """
+           require Kino.RPC
            node = :name@node
            Node.set_cookie(node, String.to_atom(System.fetch_env!("LB_COOKIE_SECRET")))
-           :erpc.call(node, fn -> :ok end)\
+           Kino.RPC.eval_string(node, ":ok", file: __ENV__.file)\
            """
   end
 
@@ -62,9 +65,10 @@ defmodule Kino.RemoteExecutionCellTest do
     {_kino, source} = start_smart_cell!(RemoteExecutionCell, attrs)
 
     assert source == """
+           require Kino.RPC
            node = :name@node
            Node.set_cookie(node, :"cookie-value")
-           :erpc.call(node, fn -> :ok end)\
+           Kino.RPC.eval_string(node, ":ok", file: __ENV__.file)\
            """
   end
 
@@ -84,56 +88,56 @@ defmodule Kino.RemoteExecutionCellTest do
       assert RemoteExecutionCell.to_source(attrs) == ""
     end
 
-    test "emites Code.string_to_quoted! when the code is invalid" do
-      attrs = %{@fields | "code" => "1 + "}
-
-      assert RemoteExecutionCell.to_source(attrs) == """
-             # Invalid code for RPC, reproducing the error below
-             Code.string_to_quoted!("1 + ")\
-             """
-    end
-
     test "generates an erpc call when there's valid code" do
       code1 = %{@fields | "code" => "1 + 1"}
       code2 = %{@fields | "code" => "1 == 1"}
       code3 = %{@fields | "code" => "a = 1\na + a"}
 
       assert RemoteExecutionCell.to_source(@fields) == """
+             require Kino.RPC
              node = :name@node
              Node.set_cookie(node, :"node-cookie")
-             :erpc.call(node, fn -> :ok end)\
+             Kino.RPC.eval_string(node, ":ok", file: __ENV__.file)\
              """
 
       assert RemoteExecutionCell.to_source(code1) == """
+             require Kino.RPC
              node = :name@node
              Node.set_cookie(node, :"node-cookie")
-             :erpc.call(node, fn -> 1 + 1 end)\
+             Kino.RPC.eval_string(node, "1 + 1", file: __ENV__.file)\
              """
 
       assert RemoteExecutionCell.to_source(code2) == """
+             require Kino.RPC
              node = :name@node
              Node.set_cookie(node, :"node-cookie")
-             :erpc.call(node, fn -> 1 == 1 end)\
+             Kino.RPC.eval_string(node, "1 == 1", file: __ENV__.file)\
              """
 
-      assert RemoteExecutionCell.to_source(code3) == """
+      assert RemoteExecutionCell.to_source(code3) == ~s'''
+             require Kino.RPC
              node = :name@node
              Node.set_cookie(node, :"node-cookie")
 
-             :erpc.call(node, fn ->
+             Kino.RPC.eval_string(
+               node,
+               """
                a = 1
                a + a
-             end)\
-             """
+               """,
+               file: __ENV__.file
+             )\
+             '''
     end
 
     test "assign to a variable" do
       attrs = %{@fields | "assign_to" => "result"}
 
       assert RemoteExecutionCell.to_source(attrs) == """
+             require Kino.RPC
              node = :name@node
              Node.set_cookie(node, :"node-cookie")
-             result = :erpc.call(node, fn -> :ok end)\
+             result = Kino.RPC.eval_string(node, ":ok", file: __ENV__.file)\
              """
     end
 
@@ -141,9 +145,10 @@ defmodule Kino.RemoteExecutionCellTest do
       attrs = %{@fields | "assign_to" => "invalid result"}
 
       assert RemoteExecutionCell.to_source(attrs) == """
+             require Kino.RPC
              node = :name@node
              Node.set_cookie(node, :"node-cookie")
-             :erpc.call(node, fn -> :ok end)\
+             Kino.RPC.eval_string(node, ":ok", file: __ENV__.file)\
              """
     end
 
@@ -151,9 +156,10 @@ defmodule Kino.RemoteExecutionCellTest do
       attrs = %{@fields | "use_cookie_secret" => true, "cookie_secret" => "COOKIE_SECRET"}
 
       assert RemoteExecutionCell.to_source(attrs) == """
+             require Kino.RPC
              node = :name@node
              Node.set_cookie(node, String.to_atom(System.fetch_env!("LB_COOKIE_SECRET")))
-             :erpc.call(node, fn -> :ok end)\
+             Kino.RPC.eval_string(node, ":ok", file: __ENV__.file)\
              """
     end
 
@@ -166,9 +172,10 @@ defmodule Kino.RemoteExecutionCellTest do
       attrs = %{@fields | "use_node_secret" => true, "node_secret" => "NODE_SECRET"}
 
       assert RemoteExecutionCell.to_source(attrs) == """
+             require Kino.RPC
              node = String.to_atom(System.fetch_env!("LB_NODE_SECRET"))
              Node.set_cookie(node, :"node-cookie")
-             :erpc.call(node, fn -> :ok end)\
+             Kino.RPC.eval_string(node, ":ok", file: __ENV__.file)\
              """
     end
 
@@ -187,9 +194,10 @@ defmodule Kino.RemoteExecutionCellTest do
       }
 
       assert RemoteExecutionCell.to_source(attrs) == """
+             require Kino.RPC
              node = String.to_atom(System.fetch_env!("LB_NODE_SECRET"))
              Node.set_cookie(node, String.to_atom(System.fetch_env!("LB_COOKIE_SECRET")))
-             :erpc.call(node, fn -> :ok end)\
+             Kino.RPC.eval_string(node, ":ok", file: __ENV__.file)\
              """
     end
   end
@@ -212,9 +220,10 @@ defmodule Kino.RemoteExecutionCellTest do
       {_kino, source} = start_smart_cell!(RemoteExecutionCell, %{})
 
       assert source == """
+             require Kino.RPC
              node = :name@node@global
              Node.set_cookie(node, :"node-cookie-global")
-             :erpc.call(node, fn -> :ok end)\
+             Kino.RPC.eval_string(node, ":ok", file: __ENV__.file)\
              """
     end
 
@@ -227,9 +236,10 @@ defmodule Kino.RemoteExecutionCellTest do
       {_kino, source} = start_smart_cell!(RemoteExecutionCell, %{})
 
       assert source == """
+             require Kino.RPC
              node = :name@node@global
              Node.set_cookie(node, String.to_atom(System.fetch_env!("LB_COOKIE_SECRET_GLOBAL")))
-             :erpc.call(node, fn -> :ok end)\
+             Kino.RPC.eval_string(node, ":ok", file: __ENV__.file)\
              """
     end
 
@@ -242,9 +252,10 @@ defmodule Kino.RemoteExecutionCellTest do
       {_kino, source} = start_smart_cell!(RemoteExecutionCell, %{})
 
       assert source == """
+             require Kino.RPC
              node = String.to_atom(System.fetch_env!("LB_NODE_SECRET_GLOBAL"))
              Node.set_cookie(node, :"node-cookie-global")
-             :erpc.call(node, fn -> :ok end)\
+             Kino.RPC.eval_string(node, ":ok", file: __ENV__.file)\
              """
     end
 
@@ -252,9 +263,10 @@ defmodule Kino.RemoteExecutionCellTest do
       {_kino, source} = start_smart_cell!(RemoteExecutionCell, %{"node" => "name@node@attrs"})
 
       assert source == """
+             require Kino.RPC
              node = :name@node@attrs
              Node.set_cookie(node, :"node-cookie-global")
-             :erpc.call(node, fn -> :ok end)\
+             Kino.RPC.eval_string(node, ":ok", file: __ENV__.file)\
              """
     end
 
@@ -266,9 +278,10 @@ defmodule Kino.RemoteExecutionCellTest do
       {_kino, source} = start_smart_cell!(RemoteExecutionCell, %{})
 
       assert source == """
+             require Kino.RPC
              node = :edited@node@name
              Node.set_cookie(node, :"node-cookie-global")
-             :erpc.call(node, fn -> :ok end)\
+             Kino.RPC.eval_string(node, ":ok", file: __ENV__.file)\
              """
     end
   end
diff --git a/test/kino/rpc_test.exs b/test/kino/rpc_test.exs
new file mode 100644
index 0000000..de51fe4
--- /dev/null
+++ b/test/kino/rpc_test.exs
@@ -0,0 +1,32 @@
+defmodule Kino.RPCTest do
+  use ExUnit.Case, async: true
+
+  require Kino.RPC
+
+  test "captures caller binding" do
+    x = 1
+    y = 1
+
+    assert Kino.RPC.eval_string(node(), "x + y") == 2
+
+    assert Kino.RPC.eval_string(node(), "{x, binding()}") == {1, [x: 1]}
+  end
+
+  test "propagates errors" do
+    assert_raise ArgumentError, "error", fn ->
+      Kino.RPC.eval_string(node(), """
+      raise ArgumentError, "error"
+      """)
+    end
+
+    assert_raise TokenMissingError, fn ->
+      Kino.RPC.eval_string(node(), "1 +")
+    end
+
+    assert ExUnit.CaptureIO.capture_io(:stderr, fn ->
+             assert_raise CompileError, fn ->
+               Kino.RPC.eval_string(node(), "unknown + 1")
+             end
+           end) =~ ~s/undefined variable "unknown"/
+  end
+end