plexus / unrepl.el

Mozilla Public License 2.0
16 stars 2 forks source link

History management proposal #5

Open volrath opened 7 years ago

volrath commented 7 years ago

For the sake of comparison (and to be sure I am not overlooking anything,) I'm going to write here the way history is currently being managed:

History is a list that has 'history as its head, and a list of alists as its tail. Each alist is composed of position, form, and handler initially, after sending a line through the socket io stream. Then an id and a result are included to said alist, taken from the :read and :eval unrepl responses respectively.

In order to correlate a :read response to an element of the history list, we are using position as the "look up field" in the list and we are comparing it with the :read's :offset.

The problem at hand is that position is a buffer-based measurement of the client's offset, and in some cases it might differ from unrepl :read's :offset, like when sending uncompleted forms:

unrepl.replG__1421=> (+ 1 1) (+ 3
#_=> 2  ;; at this point, everything is "ok", emacs position is 0, read offset is 0
4)  ;; <- closing the open form
9  ;; problem!

;; :read :offset is 7 (only took into account the first form)
;; emacs position is 13 (took the size of the whole first line, including the incompleted form)

;; last :read message is:
[:read {:from [1 8], :to [4 1], :offset 7, :len 11} 2]
;; last history list, right after sending the message is:
(history ((position . 13) (form . "2 4)") (handler . unrepl-repl-eval-callback)) 
         ((result . 2) (id . 1) (position . 0) (form . "(+ 1 1) (+ 3") (handler . unrepl-repl-eval-callback)))

;; resulting history list after getting :read message is:
(history ((position . 13) (form . "2 4)") (handler . unrepl-repl-eval-callback)) 
         ((id . 2) (result . 2) (id . 1) (position . 0) (form . "(+ 1 1) (+ 3") (handler . unrepl-repl-eval-callback)))

So there are two problems happening here.

  1. The :id 2 was associated to the wrong alist in history, basically because there wasn't a possible correlation between :offset and :position.
  2. The second string sent to the server included 2, which was the result of the first line sent.

For the sake of simplicity, I'm going to talk only about the former.

Proposal

The way that a REPL works, by definition, is a sequential thing: it Reads, Evals, Prints and then it starts again. There's no way for a REPL to evaluate two different "instructions" at once, if a string is sent to the REPL while it's evaluating something else, it gets queued and deferred until the loop starts again. This guarantees a deterministic order of reads and evaluations:

user=> (do (Thread/sleep 10000) 1)   ;; 10 seconds start here
(do (Thread/sleep 5000) 2)
(do (Thread/sleep 5000) 3)
1 ;; and end here, then 5 seconds start here.
user=> 2  ;; and end here, then 5 more seconds...
user=> 3  ;; and done.

Same happens with unrepl's read/eval messages.

Since this is a deterministic FIFO process, we don't really need position/:offset to associate an unrepl group id to a sent form/line, we only need a humble queue (list): each :read message that comes from unrepl is related to the first "not associated" alist from the list (by "not associated" I mean an alist without id.)

Since history is not a queue but a stack, the logic changes a little bit: each time unrepl-eval gets called, it sends the current line and adds it to the top of the history stack; each time a :read message comes through (unrepl--handle-read), we look for the last "not associated" alist in history and set the id to the :read :id. Something like

(defun unrepl--history-search-last-not-associated (history index)
  (let ((next (cadr history)))
    (if (or (not next) (alist-get 'id next))
        index
      (unrepl--history-search-last-not-associated (cdr history) (1+ index)))))

(defun unrepl--history-next ()
  (let ((history (unrepl--@history)))
    (set-elt history (unrepl--history-search-last-not-associated history 0))))

(defun unrepl--handle-read (form)
  (seq-let [_ _ id] form
    (push (cons 'id id) (unrepl--history-next))))

I haven't really tried this code, but just to give a general overview of what I'm thinking of. If you agree, I can submit a PR.

plexus commented 7 years ago

Matching up :read events with sent-out forms in order should work, I don't immediately see a problem with going that way. You're welcome to make a PR.

On top of that I would still like to use scan-sexps and only send fully-formed s-expressions, one at a time.

The reason they should be fully formed is that tooling should be allowed to call unrepl-eval in the background. If the REPL is in a state where it's waiting for a half completed form then that will mess things up.

The reason I want to send them one at a time is that I want to add a marker to each history item to where in the buffer results should be inserted.

cgrand commented 7 years ago

If I understand correctly your proposal assumes there's at most one :read per line.

Le sam. 1 juil. 2017 à 18:02, Arne Brasseur notifications@github.com a écrit :

Matching up :read events with sent-out forms in order should work, I don't immediately see a problem with going that way. You're welcome to make a PR.

On top of that I would still like to use scan-sexps and only send fully-formed s-expressions, one at a time.

The reason they should be fully formed is that tooling should be allowed to call unrepl-eval in the background. If the REPL is in a state where it's waiting for a half completed form then that will mess things up.

The reason I want to send them one at a time is that I want to add a marker to each history item to where in the buffer results should be inserted.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/plexus/unrepl.el/issues/5#issuecomment-312440200, or mute the thread https://github.com/notifications/unsubscribe-auth/AAC3sY0RY1w3YslvXejvm4P-Whk9JSElks5sJm2PgaJpZM4OLWJE .

-- On Clojure http://clj-me.cgrand.net/ Clojure Programming http://clojurebook.com Training, Consulting & Contracting http://lambdanext.eu/

volrath commented 7 years ago

@cgrand Yes, does that makes sense? From what I've seen in the unrepl implementation, there's only one :read "ack" each time the repl reads something.

But now I'm thinking that network latency could be a problem. I'm not entirely sure what would happen if the client sends a line, and then another before the first one reaches the server. Would the two lines be read at once? this would break this approach.

Maybe it would make sense to not add things to history until we get server acknowledge in the form of :read messages.

volrath commented 7 years ago

Also, maybe this is something where the unrepl protocol could help, by extending :read to include the line that was read, but I'm not sure of all the implications this could have.

plexus commented 7 years ago

There is one :read for each sexp/form I believe. Another reason why I think we'll make our lives easier by only sending one form at a time.

On Jul 1, 2017 19:04, "Daniel Barreto" notifications@github.com wrote:

Also, maybe this is something where the unrepl protocol could help, by extending :read to include the line that was read, but I'm not sure of all the implications this could have.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/plexus/unrepl.el/issues/5#issuecomment-312443651, or mute the thread https://github.com/notifications/unsubscribe-auth/AAB91H7rl1xryaAsbE-zkqPpxLgjYoykks5sJnwZgaJpZM4OLWJE .

cgrand commented 7 years ago

As long as you h have an accurate definition of form. If someone stumbles in a discrepancy between parsers, everything is going to be out of sync.

Le sam. 1 juil. 2017 à 19:07, Arne Brasseur notifications@github.com a écrit :

There is one :read for each sexp/form I believe. Another reason why I think we'll make our lives easier by only sending one form at a time.

On Jul 1, 2017 19:04, "Daniel Barreto" notifications@github.com wrote:

Also, maybe this is something where the unrepl protocol could help, by extending :read to include the line that was read, but I'm not sure of all the implications this could have.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/plexus/unrepl.el/issues/5#issuecomment-312443651, or mute the thread < https://github.com/notifications/unsubscribe-auth/AAB91H7rl1xryaAsbE-zkqPpxLgjYoykks5sJnwZgaJpZM4OLWJE

.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/plexus/unrepl.el/issues/5#issuecomment-312443799, or mute the thread https://github.com/notifications/unsubscribe-auth/AAC3sXKZYXB5wHP6WJpYWSEF0oNWET4Cks5sJnzegaJpZM4OLWJE .

-- On Clojure http://clj-me.cgrand.net/ Clojure Programming http://clojurebook.com Training, Consulting & Contracting http://lambdanext.eu/

volrath commented 7 years ago

@plexus in the current unrepl implementation is one per form, you are right.

In terms of sending one form/sexp at a time, I agree that it will make it easier to implement and it's a good start, but it's a restriction I haven't seen in any other repl/client.

plexus commented 7 years ago

There is only one other unrepl client so far so there might still be other approaches to explore :)

The end user wouldn't notice, they can type whatever they want in the repl buffer, but we chunk it up before transmission.

Our use case is a bit different than a regular repl, unrepl.el aims to be a basis for tooling, in particular it aims to provide a eval+callback interface. That's not something you can reliably do on top of a regular stream based repl. With unrepl it's possible, but it takes a bit of accounting.

On Jul 1, 2017 19:15, "Daniel Barreto" notifications@github.com wrote:

@plexus https://github.com/plexus in the current unrepl implementation is one per form, you are right.

In terms of sending one form/sexp at a time, I agree that it will make it easier to implement and it's a good start, but it's a restriction I haven't seen in any other repl/client.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/plexus/unrepl.el/issues/5#issuecomment-312444193, or mute the thread https://github.com/notifications/unsubscribe-auth/AAB91G4vtErtqoQ-uwGppcssuRXG9BFFks5sJn6pgaJpZM4OLWJE .

cgrand commented 7 years ago

On having only one another unrepl client around that's true and it means that if you encounter issues implementing it the protocol should be discussed.

One form at a time: when the two parties disagree on the form definition (implementation parity of a (slowly) moving and underspecified target is a weak hypothesis), it will be hard to recover. Disagreeing for highlighting is not a deep issue, disagreeing for bookkeeping is troublesome.

So I believe you shouldn't assume that you always send a well-formed form.

If your stack is a stack of lines then you can use the :from and :to line information to directly index in the stack (but then you would have to refrain yourself from ever changing line information for the session).

Or upon reception of :read messages you convert offset and len into stack-idx and string-idx (and it would work with any stack of strings, it wouldn't be specific to stack of lines anymore).

I believe that storing the id in stack items is not a good choice. Fields are premature denormalization :-)