Eval into register, kill last eval result

[yuhan0]

A quick naive implementation of the features as discussed in #3143.

I think there are a few complications around *print-level*/*print-length* options and print streaming.
One common use case I can see for this feature is being able to copy some large eval output into a separate buffer / tool and analyse it further - eg. large strings, which Cider's builtin inspector does not handle very well. Probably the most user-friendly thing to do in that case is temporarily override the print length and level options to nil, allowing for the entire result to be copied without truncation.
But this will slow down the use of regular eval commands, where it also makes sense to just copy verbatim what the user sees in the overlay / minibuffer message into the eval register.
Maybe only override print options in the case of an explicit cider-kill-last-result command? Or have it be toggled by universal-argument?
Note: I had tried to implement a similar cider-inspector-copy-val command in the past, but found the need to accumulate large results in chunks due to nrepl's print streaming and gave up halfway. cider-print-quota also comes into this, and whether to use pretty printing.

Defaults:

Should eval-register be enabled by default and clobber users' ?e registers? Presumably a register power user would find it easy enough to disable.
Suggested key binding for kill-last-result: C-c C-v k?

No tests written yet (due to issues running the testing framework)

---

Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

• The commits are consistent with our contribution guidelines
• You've added tests (if possible) to cover your change(s)
• All tests are passing (eldev test)
• All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • byte-compilation, checkdoc, check-declare, packaging metadata, indentation, and trailing whitespace checks.
• You've updated the changelog (if adding/changing user-visible functionality)
• You've updated the user manual (if adding/changing user-visible functionality)

Thanks!

If you're just starting out to hack on CIDER you might find this section of its
manual extremely useful.

[yuhan0]

To elaborate further on the printing issue - say the user has set in Clojure

(set! *print-length* 10)

;; such that 
(range 50) ;; => (0 1 2 3 4 5 6 7 8 9 ...)

The obvious approach of wrapping the form does not work, I'm not sure why.

(binding [*print-length* nil] 
  (range 50))

Using nrepl print options in additional-params also encounters a problem - the nil is transformed into an empty vector by the time it gets passed to the pr function, and throws a type error.

(cider-interactive-eval "(range 100)"
                        nil nil
                        '(("nrepl.middleware.print/print" "cider.nrepl.pprint/pr")
                          ("nrepl.middleware.print/options" (dict "print-length" nil))))
                          
                          
;; Error: class clojure.lang.PersistentVector cannot be cast to class java.lang.Number (clojure.lang.PersistentVector is in unnamed module of loader 'app'; java.lang.Number is in module java.base of loader 'bootstrap')"

Probably this is some encoding quirk / the fact that nil and () are the same in Emacs. Is there a way of getting around this?

[bbatsov]

Should eval-register be enabled by default and clobber users' ?e registers? Presumably a register power user would find it easy enough to disable.

I think that's fine.

Suggested key binding for kill-last-result: C-c C-v k?

Fine by me.

Probably this is some encoding quirk / the fact that nil and () are the same in Emacs. Is there a way of getting around this?

Yeah, that's the problem. There's no way to send nil over the wire, other than us adding some special handling for some value like :nil on the backend side.

[vemv]

It's good timing for cutting a CIDER release, so you might want to get this PR into that train :)

[bbatsov]

Won't this break for bigger results that are streamed in several chunks? Seems to you your register will only retain the last chunk.

[yuhan0]

Sorry for the late response - I spent a while trying to reproduce and understand this chunking behaviour.
It seems that by default CIDER only enables the stream chunking in certain pretty printing commands, and the handlers for other commands operate on the assumption that the result is not chunked.

See cider-eval-defun-to-comment ->
cider-eval-print-with-comment-handler (does not handle chunks)
with option map (cider--nrepl-pr-request-map) (disables streaming)

whereas cider-pprint-eval-defun-to-comment ->
cider-eval-pprint-with-multiline-comment-handler (accumulates chunks)
with option map (cider--nrepl-print-request-map fill-column) (enables streaming)

But one could conceivably call one with the other, with broken results:

(cider-interactive-eval "(range 50)"
                        (cider-eval-print-with-comment-handler
                         (current-buffer) (point-max-marker) cider-comment-prefix)
                        nil
                        '(("nrepl.middleware.print/stream?" "1")
                          ("nrepl.middleware.print/buffer-size" 20)))
;; => )
;; => 43 44 45 46 47 48 49
;; => 6 37 38 39 40 41 42 
;; =>  30 31 32 33 34 35 3
;; => 23 24 25 26 27 28 29
;; => 6 17 18 19 20 21 22 
;; =>  10 11 12 13 14 15 1
;; => (0 1 2 3 4 5 6 7 8 9

Is this a bug in the current code or a valid assumption that the handlers are making? It looks like cider-interactive-eval-handler is currently only called in conjunction with pr-request-map, all other commands which use pretty printing override the handler.

[yuhan0]

Here's what currently happens when the interactive-eval-handler is called with streaming enabled:

(defun cider-eval-defun-with-streaming ()
  "A hypothetical eval command"
  (interactive)
  (cider-interactive-eval
   (cider-defun-at-point)
   nil                                         ; <- the default interactive-eval-handler
   (cider-defun-at-point 'bounds)
   '(("nrepl.middleware.print/stream?" "1")    ; <- but with streaming
     ("nrepl.middleware.print/buffer-size" 20) ; <- and a small chunk size
     ("nrepl.middleware.print/print" "cider.nrepl.pprint/pr"))))

Is this the intended behaviour? Obviously without the artificial delay, the user would only see the very last chunk in their overlay / minibuffer.

Accumulating the result and redisplaying the overlay on each chunk seems like a better solution:

But in the first place, I believe none of the code paths in the current set of commands will lead to such a situation, unless some custom command like the above is defined.

[bbatsov]

Yeah, you make a good point. Streaming of values wasn't enabled intentionally for interactive evaluation as I couldn't figure out a good way to print the result incrementally. I like your idea.

In an ideal world probably we should able to enable/disable streaming everywhere via defcustoms. For huge results definitely streaming results in a much nicer experience as you start seeing results faster.

[bbatsov]

Is this the intended behaviour? Obviously without the artificial delay, the user would only see the very last chunk in their overlay / minibuffer.

And yeah - that's definitely not the intended behavior. :-) As mentioned about the streaming was disabled until we figure out how to best do it for buffer evaluations. Back then I recall I was planning to research potential options and as it usually happens - I never did. :-)

[yuhan0]

Would it make sense to insert this eval-into-register behaviour into every handler, or just the interactive-eval-handler?

Here's the full list:

• cider-insert-eval-handler
• cider-interactive-eval-handler
• cider-load-file-handler
• cider-eval-print-handler
• cider-eval-print-with-comment-handler
• cider-eval-pprint-with-multiline-comment-handler
• cider-popup-eval-handler

Of these, I think it could be left out of the print-handler and popup-eval-handler, since these would be used for potentially large outputs. Accumulating a huge result in memory in addition to printing it out in chunks into an Emacs buffer could be a performance concern.

[yuhan0]

Ok, I added a commit which refactors the handlers to handle streamed values. Not too happy about the code duplication but there doesn't seem to be an clean way to abstract out all the (when cider-eval-register ... expressions

[bbatsov]

Seems I forgot the respond here. The duplication is definitely not great, but I guess we can live with it, as the handler code rarely changes. I think that the ieval-handler and the load-file handler are definitely the most important ones when you might want to save the result to a register, but I guess for consistency it makes sense to extend this to more handlers as suggested by you. And yeah - we should avoid storing huge results in memory, knowing how poorly GC in Emacs performs.

[yuhan0]

Ok, I removed it from the print-handler and popup-eval-handler, on the basis that the results are directly output to a buffer anyway, where they can be interacted with.

[bbatsov]

Yeah, that'd be nice, but it's also not that big of a deal if it misses it.

[yuhan0]

Yeah, that's the problem. There's no way to send nil over the wire, other than us adding some special handling for some value like :nil on the backend side.

That's unfortunate, I was hoping there was some way of encoding a nil that I couldn't find in the bencode(?) docs - still not very familiar with the internals of nrepl communications.

In this particular case maybe -1 would be a better candidate for a sentinel value, but it would still require changes to the logic in cider-nrepl, as well as handling the different keys for the various printing backends (eg. :print-length for pr, :length for pprint).

I do wonder why wrapping the the form in (binding [*print-length* ...]) doesn't affect Cider's interactive eval output? Having that would solve matters in a much cleaner manner.

[bbatsov]

In this particular case maybe -1 would be a better candidate for a sentinel value, but it would still require changes to the logic in cider-nrepl, as well as handling the different keys for the various printing backends (eg. :print-length for pr, :length for pprint).

Or 0 or some keyword like :nil. There are definitely options, but as you noted the backend needs to be made aware of them.

I do wonder why wrapping the the form in (binding [print-length ...]) doesn't affect Cider's interactive eval output? Having that would solve matters in a much cleaner manner.

Probably something on the backend overrides *print-length* (e.g. in the print middleware), but I don't remember the details of the code much at this point.

[bbatsov]

@yuhan0 Do you plan to work more or this or we can merge it? Seems to me that despite its small flaws the functionality would be useful in its present form.

[yuhan0]

Yes, thanks for the reminder! Haven't had much time to look into those issues above, but it should be alright to merge in its current form.

I'll just add a changelog entry and couple of notes to the manual.

[bbatsov]

I'll just add a changelog entry and couple of notes to the manual.

Sounds good to me.

[yuhan0]

Should be good to go! There are some lint and test faliures which I don't think are related to this PR.