Fix the compatibility problems proof found

dialtone/src/dialtone_backend.erl

@@ -9,6 +9,10 @@
 %% resources they'd need to clean up. Output streaming needs no callback:
 %% evaluated code writes through its group leader, which dialtone owns.
 %%
+%% A backend that evaluates several forms in one eval reports the value of
+%% each form but the last with {@link emit_value/1} as it goes; the last one
+%% is the `value' that eval/3 returns.
+%%
 %% Optional callbacks gate op advertisement: a backend without `complete/3'
 %% simply yields a server that doesn't announce (or answer) `completions'.
 -module(dialtone_backend).
@@ -23,6 +27,8 @@
 %% "arglists-str", "file", "line"; values binary or integer.
 -type info() :: #{binary() => binary() | integer()}.
 
+-export([emit_value/1]).
+
 -export_type([state/0, eval_meta/0, ok_result/0, err_result/0,
               candidate/0, info/0]).
 
@@ -56,3 +62,11 @@
 
 -optional_callbacks([load_file/3, complete/3, lookup/3, format_exception/4,
                      io_opts/0]).
+
+%% @doc Send a value to the client right away, ahead of the one eval/3
+%% returns. Only meaningful inside eval/3, where the group leader is the
+%% session's IO server.
+-spec emit_value(unicode:chardata()) -> ok.
+emit_value(Value) ->
+    _ = io:request(group_leader(), {dialtone_value, Value}),
+    ok.

dialtone/src/dialtone_backend_erlang.erl

@@ -3,7 +3,9 @@
 %%
 %% Code is scanned/parsed/evaluated with the stdlib erl_scan/erl_parse/
 %% erl_eval pipeline. Dot-terminated forms evaluate in sequence, threading
-%% bindings; the last value is reported. A missing final dot is forgiven
+%% bindings, and each form's value is reported as it's computed, like the
+%% shell prints them (load-file reports only the last one). A missing final
+%% dot is forgiven
 %% (editors send bare expressions like "1 + 2"). Evaluating module
 %% definitions is not supported by erl_eval - load-file (M3) compiles module
 %% sources via compile:forms instead.
@@ -18,7 +20,11 @@ init(_Opts) ->
     %% so lookup can read their debug_info chunk (they exist only in memory).
     {ok, #{bindings => erl_eval:new_bindings(), beams => #{}}}.
 
-eval(Code, Meta, #{bindings := Bindings} = State) ->
+eval(Code, Meta, State) ->
+    eval(Code, Meta, State, true).
+
+%% EmitAll says whether forms before the last report their values too.
+eval(Code, Meta, #{bindings := Bindings} = State, EmitAll) ->
     StartLoc = {maps:get(line, Meta, 1), maps:get(column, Meta, 1)},
     Str = unicode:characters_to_list(Code),
     case erl_scan:string(Str, StartLoc) of
@@ -29,7 +35,7 @@ eval(Code, Meta, #{bindings := Bindings} = State) ->
         {ok, Tokens, EndLoc} ->
             case parse_forms(ensure_dot(Tokens, EndLoc)) of
                 {ok, ExprLists} ->
-                    eval_forms(ExprLists, Bindings, State);
+                    eval_forms(ExprLists, Bindings, State, EmitAll);
                 {error, ErrResult} ->
                     {error, ErrResult, State}
             end;
@@ -45,11 +51,15 @@ load_file(Contents, Meta, State) ->
     Path = maps:get(path, Meta, maps:get(name, Meta, <<"nrepl-load-file">>)),
     Str = unicode:characters_to_list(Contents),
     case erl_scan:string(Str) of
+        {ok, [], _EndLoc} ->
+            %% Nothing but whitespace and comments: same as an empty eval.
+            eval(Contents, #{file => Path}, State, false);
         {ok, Tokens, EndLoc} ->
             Forms = split_dots(ensure_dot(Tokens, EndLoc)),
             case is_module_source(Forms) of
                 true -> compile_module(Forms, Path, State);
-                false -> eval(Contents, #{file => Path}, State)
+                %% Like nREPL, load-file reports just the last form's value.
+                false -> eval(Contents, #{file => Path}, State, false)
             end;
         {error, {Loc, Mod, Desc}, _} ->
             {error, syntax_error(Loc, Mod, Desc), State}
@@ -374,14 +384,19 @@ split_dots([Token | Rest], Current, Forms) ->
 
 %% Runtime errors raise out of erl_eval and are rendered by the worker's
 %% catch-all (dialtone_err) - by design, so users see real stacktraces.
-eval_forms(ExprLists, Bindings0, State) ->
-    {Value, Bindings} =
-        lists:foldl(fun(Exprs, {_PrevValue, B}) ->
-                            {value, V, B2} = erl_eval:exprs(Exprs, B),
-                            {V, B2}
-                    end, {ok, Bindings0}, ExprLists),
-    {ok, #{value => io_lib:format("~tp", [Value])},
-     State#{bindings := Bindings}}.
+eval_forms([Exprs], Bindings0, State, _EmitAll) ->
+    {value, Value, Bindings} = erl_eval:exprs(Exprs, Bindings0),
+    {ok, #{value => format_value(Value)}, State#{bindings := Bindings}};
+eval_forms([Exprs | Rest], Bindings0, State, EmitAll) ->
+    {value, Value, Bindings} = erl_eval:exprs(Exprs, Bindings0),
+    case EmitAll of
+        true -> dialtone_backend:emit_value(format_value(Value));
+        false -> ok
+    end,
+    eval_forms(Rest, Bindings, State, EmitAll).
+
+format_value(Value) ->
+    io_lib:format("~tp", [Value]).
 
 syntax_error(Loc, Mod, Desc) ->
     Where = case Loc of

dialtone/src/dialtone_conn.erl

@@ -94,7 +94,7 @@ drain(Buffer, State) ->
 
 handle_message(Req, State) ->
     case maps:get(<<"op">>, Req, undefined) of
-        undefined -> send_self(dialtone_msg:response(Req, #{<<"status">> => ['unknown-op', done]}), State);
+        undefined -> send_self(dialtone_msg:unknown_op(Req), State);
         <<"describe">> -> describe(Req, State);
         <<"clone">> -> clone(Req, State);
         <<"ls-sessions">> -> ls_sessions(Req, State);
@@ -103,7 +103,7 @@ handle_message(Req, State) ->
                 Op =:= <<"interrupt">>; Op =:= <<"completions">>; Op =:= <<"lookup">> ->
             route_to_session(Req, State);
         _UnknownOp ->
-            send_self(dialtone_msg:response(Req, #{<<"status">> => ['unknown-op', done]}), State)
+            send_self(dialtone_msg:unknown_op(Req), State)
     end.
 
 describe(Req, #conn{backend = {BMod, _}} = State) ->
@@ -156,20 +156,22 @@ ls_sessions(Req, State) ->
               State).
 
 %% close is idempotent: closing an unknown (or absent) session still answers
-%% done. A live session tears down its own in-flight work and sends the done
+%% session-closed, so clients waiting for it (vim-fireplace) don't loop. A
+%% live session tears down its own in-flight work and sends the reply
 %% itself - routing close through the async request path like any other
 %% session op keeps message flow one-directional (conn never blocks on a
 %% session that might be blocking on the conn).
 close(Req, State) ->
     case maps:get(<<"session">>, Req, undefined) of
         undefined ->
-            send_self(dialtone_msg:response(Req, #{<<"status">> => [done]}), State);
+            send_self(dialtone_msg:response(Req, #{<<"status">> => ['session-closed', done]}),
+                      State);
         Id ->
             case dialtone_sessions:lookup(Id) of
                 {ok, Pid} ->
                     dialtone_session:request(Pid, Req, self());
                 error ->
-                    send_self(dialtone_msg:response(Req, #{<<"status">> => [done]}),
+                    send_self(dialtone_msg:response(Req, #{<<"status">> => ['session-closed', done]}),
                               State)
             end
     end.

dialtone/src/dialtone_io_server.erl

@@ -84,6 +84,18 @@ io_request(From, ReplyAs, {put_chars, Chars}, State) ->
     io_request(From, ReplyAs, {put_chars, latin1, Chars}, State);
 io_request(From, ReplyAs, {put_chars, M, F, A}, State) ->
     io_request(From, ReplyAs, {put_chars, latin1, M, F, A}, State);
+%% A value from a form that isn't the last one in an eval, sent by the
+%% backend (see dialtone_backend:emit_value/1). It goes the same way as
+%% output, so it reaches the client in step with the code's output.
+io_request(From, ReplyAs, {dialtone_value, Value}, #io{sink = Sink} = State) ->
+    Reply = case unicode:characters_to_binary(Value) of
+                Bin when is_binary(Bin) ->
+                    emit_value(dialtone_msg:truncate_value(Bin), Sink);
+                _ ->
+                    {error, {no_translation, unicode, utf8}}
+            end,
+    io_reply(From, ReplyAs, Reply),
+    State;
 io_request(From, ReplyAs, {get_until, Encoding, Prompt, M, F, A}, State) ->
     enqueue_read(From, ReplyAs, {until, Encoding, M, F, A, []}, Prompt, State);
 io_request(From, ReplyAs, {get_line, Encoding, Prompt}, State) ->
@@ -137,6 +149,11 @@ do_put_chars(Encoding, Chars, #io{sink = Sink}) ->
             {error, {no_translation, Encoding, utf8}}
     end.
 
+emit_value(_Bin, undefined) ->
+    ok;
+emit_value(Bin, {Conn, Req}) ->
+    dialtone_msg:reply(Conn, Req, #{<<"value">> => Bin}).
+
 emit_out(_Bin, undefined) ->
     ok;
 emit_out(Bin, {Conn, Req}) ->

dialtone/src/dialtone_msg.erl

@@ -11,13 +11,33 @@
 %% design, and results of an eval whose client went away have nowhere to go.
 -module(dialtone_msg).
 
--export([reply/3, reply_done/3, reply_done/4, send/2, response/2]).
+-export([reply/3, reply_done/3, reply_done/4, send/2, response/2, unknown_op/1,
+         truncate_value/1]).
+
+%% Crude safety net against a multi-gigabyte value taking the editor down;
+%% a proper client-negotiated print quota can replace it later.
+-define(MAX_VALUE_BYTES, 8 * 1024 * 1024).
 
 %% @doc Build a response map echoing id/session from the request.
 -spec response(map(), map()) -> map().
 response(Req, Extra) ->
     maps:merge(maps:with([<<"id">>, <<"session">>], Req), Extra).
 
+%% @doc The response to an op the server doesn't support. It echoes the op,
+%% which Calva uses to tell the user what's missing.
+-spec unknown_op(map()) -> map().
+unknown_op(Req) ->
+    response(Req, maps:merge(maps:with([<<"op">>], Req),
+                             #{<<"status">> => ['unknown-op', done]})).
+
+%% @doc Cut a rendered value down to a size clients can cope with.
+-spec truncate_value(binary()) -> binary().
+truncate_value(Bin) when byte_size(Bin) > ?MAX_VALUE_BYTES ->
+    Head = binary:part(Bin, 0, ?MAX_VALUE_BYTES),
+    <<Head/binary, "... (value truncated by dialtone)">>;
+truncate_value(Bin) ->
+    Bin.
+
 %% @doc Send an intermediate (non-terminal) response for a request.
 -spec reply(pid(), map(), map()) -> ok.
 reply(Conn, Req, Extra) ->

dialtone/src/dialtone_session.erl

@@ -17,10 +17,6 @@
 -export([start_link/3, start_ephemeral/2, request/3, get_bstate/1]).
 -export([init/1, handle_call/3, handle_cast/2, handle_info/2, terminate/2]).
 
-%% Crude safety net against a multi-gigabyte value taking the editor down;
-%% a proper client-negotiated print quota can replace it later.
--define(MAX_VALUE_BYTES, 8 * 1024 * 1024).
-
 -record(session, {id :: binary(),
                   backend :: {module(), map()},
                   bstate :: term(),
@@ -71,10 +67,25 @@ handle_call(get_bstate, _From, State) ->
 
 handle_cast({request, #{<<"op">> := <<"close">>} = Req, ReplyTo}, State) ->
     interrupt_all(State),
-    dialtone_msg:reply_done(ReplyTo, Req, #{}),
+    %% Unregister before replying: once a client sees session-closed, its
+    %% next request must get unknown-session instead of reaching this
+    %% process while it shuts down.
+    ok = case State#session.ephemeral of
+             true -> ok;
+             false -> dialtone_sessions:remove(State#session.id)
+         end,
+    %% vim-fireplace only forgets a session once it sees session-closed.
+    dialtone_msg:reply_done(ReplyTo, Req, ['session-closed'], #{}),
     {stop, normal, State#session{running = undefined, queue = queue:new()}};
 handle_cast({request, Req, ReplyTo}, State) ->
-    {noreply, handle_request(maps:get(<<"op">>, Req), Req, ReplyTo, State)}.
+    case handle_request(maps:get(<<"op">>, Req), Req, ReplyTo, State) of
+        %% A throwaway session whose request was answered without starting
+        %% an eval (no-code, completions, ...) has nothing left to do.
+        #session{ephemeral = true, running = undefined} = State1 ->
+            {stop, normal, State1};
+        State1 ->
+            {noreply, State1}
+    end.
 
 handle_info({eval_result, Ref, Result},
             #session{running = #{ref := Ref, req := Req, reply_to := ReplyTo,
@@ -115,6 +126,10 @@ handle_request(Op, Req, ReplyTo, State)
     case validate(Op, Req) of
         ok ->
             enqueue_or_run({Req, ReplyTo}, State);
+        no_code ->
+            %% Nothing was evaluated, so this isn't an eval-error.
+            dialtone_msg:reply_done(ReplyTo, Req, ['no-code'], #{}),
+            State;
         {error, Err} ->
             dialtone_msg:reply_done(ReplyTo, Req, ['eval-error'],
                                     #{<<"err">> => Err, <<"ex">> => <<"bad-request">>}),
@@ -143,7 +158,8 @@ handle_request(<<"lookup">>, Req, ReplyTo, State) ->
               end, Req, ReplyTo, State).
 
 validate(<<"eval">>, #{<<"code">> := Code}) when is_binary(Code) -> ok;
-validate(<<"eval">>, _) -> {error, <<"eval op requires a code field">>};
+validate(<<"eval">>, #{<<"code">> := _}) -> {error, <<"the code field must be a string">>};
+validate(<<"eval">>, _) -> no_code;
 validate(<<"load-file">>, #{<<"file">> := File}) when is_binary(File) -> ok;
 validate(<<"load-file">>, _) -> {error, <<"load-file op requires a file field">>}.
 
@@ -158,7 +174,7 @@ ro_meta(Req) ->
 read_only(F, Args, Render, Req, ReplyTo, #session{backend = {BMod, _}} = State) ->
     case erlang:function_exported(BMod, F, length(Args) + 1) of
         false ->
-            dialtone_msg:reply_done(ReplyTo, Req, ['unknown-op'], #{});
+            dialtone_msg:send(ReplyTo, dialtone_msg:unknown_op(Req));
         true ->
             try apply(BMod, F, Args ++ [State#session.bstate]) of
                 Result ->
@@ -211,7 +227,7 @@ eval_meta(Req) ->
 deliver_result(Req, ReplyTo, Result, #session{bstate = OldBState}) ->
     case Result of
         {ok, OkMap, NewBState} ->
-            Value = truncate(unicode:characters_to_binary(maps:get(value, OkMap))),
+            Value = dialtone_msg:truncate_value(unicode:characters_to_binary(maps:get(value, OkMap))),
             Extra = case OkMap of
                         #{ns := Ns} -> #{<<"ns">> => unicode:characters_to_binary(Ns)};
                         _ -> #{}
@@ -231,12 +247,6 @@ deliver_result(Req, ReplyTo, Result, #session{bstate = OldBState}) ->
             OldBState
     end.
 
-truncate(Bin) when byte_size(Bin) > ?MAX_VALUE_BYTES ->
-    Head = binary:part(Bin, 0, ?MAX_VALUE_BYTES),
-    <<Head/binary, "... (value truncated by dialtone)">>;
-truncate(Bin) ->
-    Bin.
-
 next(#session{ephemeral = true, running = undefined} = State) ->
     %% One request served; this throwaway session is done.
     {stop, normal, State};

dialtone/src/dialtone_sessions.erl

@@ -5,7 +5,7 @@
 
 -behaviour(gen_server).
 
--export([start_link/0, new/2, lookup/1, list/0]).
+-export([start_link/0, new/2, lookup/1, list/0, remove/1]).
 -export([init/1, handle_call/3, handle_cast/2, handle_info/2]).
 
 -define(TAB, ?MODULE).
@@ -30,10 +30,21 @@ lookup(Id) ->
 list() ->
     [Id || {Id, _Pid} <- ets:tab2list(?TAB)].
 
+%% @doc Forget a session right away instead of when its process goes down,
+%% so a client that's been told the session is closed can't reach it any
+%% more. Called by the session itself while handling close.
+remove(Id) ->
+    gen_server:call(?MODULE, {remove, Id}).
+
 init([]) ->
     ?TAB = ets:new(?TAB, [named_table, protected, {read_concurrency, true}]),
     {ok, #{}}.
 
+handle_call({remove, Id}, _From, Monitors) ->
+    %% The monitor stays: the DOWN that follows deletes the row again, which
+    %% is harmless.
+    true = ets:delete(?TAB, Id),
+    {reply, ok, Monitors};
 handle_call({new, Backend, InitialBState}, _From, Monitors) ->
     Id = dialtone_uuid:v4(),
     case dialtone_session_sup:start_session(Id, Backend, InitialBState) of