search

nrepl / piggieback

nREPL support for ClojureScript REPLs
480 stars 48 forks source link

Use nrepl pretty printing support #108

Closed ak-coram closed 4 years ago

ak-coram commented 5 years ago

Hi,

I'm trying to fix CIDER issue #2667 and believe it could be resolved by also relying on the pretty-printing support of the newer nREPL versions in piggieback. These changes work for me, so please let me know if there are issues with this approach. Thanks!

ak-coram commented 5 years ago

I see this breaks most of the tests, so this may not be a viable approach at all. Could someone give a bit of feedback please?

bbatsov commented 5 years ago

I was under the impression the new middleware in nREPL was supposed to handle the responses from piggieback automatically. @cichli Can you enlighten us? :-)

ak-coram commented 5 years ago

A bit of background information:

I started encountering this issue with the figwheel-main repl. I've seen that :nrepl.middleware.print/keys was not set when using the CLJS repl, even though the nREPL docs state that it should be set by default for the eval and load-file ops. I've added it to piggieback just by following what nrepl.middleware.interruptible-eval does. Now the print middleware received every result as a string value and "pretty printed" it (effectively printing it twice), so I wrapped the piggieback eval result in clojure.tools.reader.edn/read-string so that the print middleware can print it correctly. That made all of it work as expected while also using all the other print configuration from CIDER.

I've also seen the comment about :printed-value being removed from nREPL, so I thought it would be fine to return a form instead of a string (while also removing :printed-value).

I hope this helps!

ak-coram commented 5 years ago

I've noticed another issue: user defined tagged literals don't work with this change. Since the mechanism for defining them is different for Clojure and ClojureScript, I imagine that this would only work if both the Clojure and the ClojureScript environments had compatible reader and printer functions defined. I'm not sure how this was handled previously either.

SevereOverfl0w commented 5 years ago

You're right, the edn reader is not appropriate for use in this context. edn is a strict subset of clojure, so can't parse a lot of things that will potentially be in that value (like functions)

SevereOverfl0w commented 5 years ago

I decided to check what clojurescript's browser env returns from evaluations, it looks like it does this: https://github.com/clojure/clojurescript/blob/r1.10.520-1-g230e46ae/src/main/clojure/cljs/repl/browser.clj#L297-L313 note the read-string call.

The delegating repl env is, well, delegating. So that should be fine!

I had a poke, and found this in repl: https://github.com/clojure/clojurescript/blob/b38ded99dc0967a48824d55ea644bee86b4eae5b/src/main/clojure/cljs/repl.cljc#L500-L502

This string may or may not be readable by the Clojure reader.

So we could attempt a read and then set it to the string otherwise.

ak-coram commented 5 years ago

@SevereOverfl0w: thank you for your feedback, I've implemented falling back on strings when the reader fails. Sadly this happens whenever there is a value anywhere in the result which the reader can't handle. At least it's much better than not having pretty printing at all or getting errors for unsupported values.

Could we maybe leverage cljs.pprint and do the pretty printing directly in ClojureScript instead? Or maybe implement a parser and pretty printer which don't rely on actually reading the values? Neither of these seems straightforward.

SevereOverfl0w commented 5 years ago

I don't think you should catch that specific exception, and rethrow the rest. In older versions of clojure, the exception did not look like that. I think it's fine just to fallback whenever there's an exception and never rethrow.

reworking things to have a clojurescript version of the pprint is beyond my knowledge for how the pprint was intended to work.

ak-coram commented 5 years ago

This is just an idea, but what if we always returned a string from the ClojureScript repl? We would be guaranteed to be able to read it in Clojure and we could return the pretty-printed results through it when pretty printing is enabled. For example:

(defn eval-cljs [repl-env env form opts]
  (read-cljs-string
   (cljs.repl/evaluate-form repl-env
                            env
                            "<cljs repl>"
                            `(cljs.core/with-out-str
                               (cljs.pprint/pprint ~form))
                            ((:wrap opts #'cljs.repl/wrap-fn) form)
                            opts)))

With this my CIDER repl is handling every value I've thrown at it:

c.user> (repeat 10 {:a (fn [])})
({:a #object[Function]}
 {:a #object[Function]}
 {:a #object[Function]}
 {:a #object[Function]}
 {:a #object[Function]}
 {:a #object[Function]}
 {:a #object[Function]}
 {:a #object[Function]} 
 {:a #object[Function]} 
 {:a #object[Function]})

I'm not sure how much of the nREPL printing configuration cljs.pprint supports, but the options that are available could be included in the pprint call as well. As far as I can tell cljs.pprint is already included via cljs.repl, so we don't need to worry about loading it either.

One thing to potentially worry about is the wrapping code affecting stack traces. I can already see a lot of tooling-related namespaces there, so I guess it doesn't make it that much worse than it already is.

Also I'm not sure if we need to do

`(let [form# ~form]
   (cljs.core/with-out-str (cljs.pprint/pprint form#)))

instead to avoid capturing output while evaluating the form via with-out-str.

ak-coram commented 5 years ago

Forgot to mention that the above also breaks repl history (1, e, etc.) since it saves the printed strings instead of the original values. I'm guessing this could be fixed by first allowing the cljs.repl/wrap-fn functions run before printing.

SevereOverfl0w commented 5 years ago

I suspect a println in the code you send will create broken output

ak-coram commented 5 years ago

@SevereOverfl0w: yes, that's why I suggested

`(let [form# ~form]
   (cljs.core/with-out-str (cljs.pprint/pprint form#)))

This evaluates the form before wrapping the results in with-out-str making sure that any output is printed. The only problem with this is that if you do printing inside lazy sequences which are only realized by the pprint call, then you still end up with the printed output inside of the result. Guess we could avoid it by not relying on with-out-str and passing a writer to pprint:

`(let [sb# (goog.string.StringBuffer.)
       sbw# (cljs.core/StringBufferWriter. sb#)
       form# ~form]
   (cljs.pprint/pprint form# sbw#)
   (cljs.core/str sb#))
ak-coram commented 5 years ago

An example with a lazy seq:

print_test

(this is using a writer with pprint)

The history only contains the printed list (and not the outputs from println): 2019-08-14-205008_1217x50_scrot

bbatsov commented 5 years ago

We're getting closer to a solution. I'll try to find some time for this soon, as I'd really love for us to fix it.

ak-coram commented 5 years ago

@bbatsov: please let me know if I can help with anything.

bbatsov commented 5 years ago

Will do. I've been extremely busy the past couple of weeks, but hopefully my schedule will be back to normal after the middle of next week.

ak-coram commented 5 years ago

Hi, I've made another attempt at fixing this and rebased onto the current master branch. Compared to the earlier attempts this version also supports *1 and friends in the repl. One issue I've found is that the expression false somehow evaluates to nil (it looks like it isn't evaluated at all). I'm not sure how much of an issue this is, but I assume it's not related to the pretty-printing anyway.

I'll look into why the tests are failing, but could you please take a look at this approach? Thanks!

c.user> (for [x (range 10)]
          (do (println "Testing")
              (list x x (fn [x] x))))
Testing
Testing
Testing
Testing
Testing
Testing
Testing
Testing
Testing
Testing
((0 0 #object[Function])
 (1 1 #object[Function])
 (2 2 #object[Function])
 (3 3 #object[Function])
 (4 4 #object[Function])
 (5 5 #object[Function])
 (6 6 #object[Function])
 (7 7 #object[Function])
 (8 8 #object[Function]) 
 (9 9 #object[Function]))
c.user> *1
((0 0 #object[Function])
 (1 1 #object[Function])
 (2 2 #object[Function])
 (3 3 #object[Function])
 (4 4 #object[Function])
 (5 5 #object[Function])
 (6 6 #object[Function])
 (7 7 #object[Function])
 (8 8 #object[Function]) 
 (9 9 #object[Function]))
c.user> (throw (js/Error. "Test"))
#object[Error Error: Test]
c.user> *e
#object[Error Error: Test]
c.user> nil
nil
c.user> false
nil
SevereOverfl0w commented 5 years ago

I think false->nil is probably indicative of something going on.

Also, why is there special handling of ns, import, etc? Is that because they must be top level forms?

ak-coram commented 5 years ago

@SevereOverfl0w: thank you for your feedback.

I think false->nil is probably indicative of something going on.

I've tested it with the upstream version of piggieback (0.4.1) and it's also the case there:

c.user> false
nil

Also, why is there special handling of ns, import, etc? Is that because they must be top level forms?

I'm not sure myself: I've based this directly on cljs.repl/wrap-fn (the default wrapper which is currently used) and wanted it to be as similar as possible (but pretty printing return values).

ak-coram commented 5 years ago

The last force-push is for rebasing on top of the latest master version.

bbatsov commented 5 years ago

Hey there, @ak-coram! Sorry for the radio silence - I was super busy most of September and I finally got to looking into this. In general any solution that we adopt should respect how nREPL when it comes to pretty printing, otherwise that'd be super weird.

It seems to me that something's wrong with the middleware ordering for piggieback if the print-keys don't get properly set for the evaluations performed by it.

bbatsov commented 5 years ago

I'm also wondering if piggieback shouldn't leverage the print middleware infrastructure directly the same way interruptible-eval does (although obviously here we'd printing something after it's already evaluated in the ClojureScript REPL and we have the string representation of the result).

Unfortunately I'm not quite sure how @cichli planned for this to work with ClojureScript and he hasn't been around lately so we can't really ask him.

ak-coram commented 5 years ago

Hi @bbatsov! Thank you for the feedback.

I agree that pretty-printing should be configurable, but I also think it should be done in the CLJS environment in order to be able to print simple expressions such as (fn [x] x) or user-defined tagged literals (which understandably may be only defined for ClojureScript in a ClojureScript-only project). I've tried integrating with nREPL pretty-printing initially to fix this issue too, but discovered that reading values in Clojure which are printed by cljs.core/pr-str in ClojureScript doesn't even cover basic common use-cases (it basically only works on built-in data structures and primitive values).

As far as I can tell the current nREPL pretty-printing infrastructure completely runs on the CLJ-side, even if the given printer (such as fipp) supports CLJS. Other printers do not support CLJS at all (such as puget), but as I've mentioned I see more of a problem with reading values in CLJ than with printing them.

There's already some control you have over the current version, because you can provide your own wrapping function via *cljs-repl-options* (the one I added is just replacing the default one, which was already printing expressions via cljs.core/pr-str). I think we could support setting your own printing fn too (akin to cider-pprint-fn), but it would have to support a different set of values for CLJS compared to CLJ.

EDIT: I just realized that reading built-in data structures doesn't work in all cases either (#queue [] for example)

bbatsov commented 5 years ago

reading values in Clojure which are printed by cljs.core/pr-str in ClojureScript doesn't even cover basic common use-cases (it basically only works on built-in data structures and primitive values).

Well, that certainly comes as a surprise to me, but then again - I don't do much about ClojureScript. I always assumed that reading values was something cross-platform, but I guess it's not. Can you give me some concrete examples so I have a better idea about the nature of the problem?

As far as I can tell the current nREPL pretty-printing infrastructure completely runs on the CLJ-side, even if the given printer (such as fipp) supports CLJS. Other printers do not support CLJS at all (such as puget), but as I've mentioned I see more of a problem with reading values in CLJ than with printing them.

Yeah, the the printing logic is running completely on the Clojure side, which seemed reasonable given the expectation for compatibility that I stated earlier.

There's already some control you have over the current version, because you can provide your own wrapping function via cljs-repl-options (the one I added is just replacing the default one, which was already printing expressions via cljs.core/pr-str). I think we could support setting your own printing fn too (akin to cider-pprint-fn), but it would have to support a different set of values for CLJS compared to CLJ.

Yeah, I get this, but I really hate having to provide different interfaces for Clojure and ClojureScript everywhere. You've got no idea how much I hate ClojureScript already because almost everything is slightly different there and half the code has to be duplicated because of this. I assume there's not chance that the ClojureScript REPL would accept a printing function the way the Clojure REPL does, right? This might have been a reasonable solution.

ak-coram commented 5 years ago

It's not just ClojureScript, some Clojure values can be pretty-printed just fine, but can understandably not be read back:

user> (deftype MyType []) user.MyType user> (->MyType)

object[user.MyType 0x3e15eb62 "user.MyType@3e15eb62"]

user> (read-string "#object[user.MyType 0x3e15eb62 \"user.MyType@3e15eb62\"]") Execution error at user/eval23324 (REPL:48). No reader function for tag object


For ClojureScript there's also the aforementioned `#queue` literal:
https://cljs.github.io/api/syntax/queue-literal

* CLJS Objects
```clojure
c.user> (deftype Cheese [])
cljs.user/Cheese
c.user> (Cheese.)
#object[cljs.user.Cheese]
user> (read-string "#object[cljs.user.Cheese]")
Execution error at user/eval23285 (REPL:22).
No reader function for tag object

And of course it's up to the users to define any number of custom tagged literals, that's why EDN (extensible data notation) is extensible. In a project only using ClojureScript they wouldn't bother defining the same literals for Clojure, which would cause reading them to fail too.

I assume there's not chance that the ClojureScript REPL would accept a printing function the way the Clojure REPL does, right? This might have been a reasonable solution.

I don't think so: the interface looks different from the Clojure REPL, but I'm not familiar with the details either. Even if it does, it probably has to run in JavaScript.

Essentially this is similar to what my workaround in this PR does: it wraps the evaluated CLJS form with the pretty-printing code. I don't think there's much we can do here except introduce an option so the users can override the var used for printing in CLJS (with #'cljs.pprint/pprint being the default).

Or writing a pretty-printer which works on printed CLJS values without reading them, but that would defeat the point of being able to use a custom printer anyway :)

ak-coram commented 5 years ago

We also can't be sure that the types implementing IPrintWithWriter can be read in Clojure.

I understand your frustration with having to support CLJS in this way, guess one day we could have an independent nREPL implementation in it and not "piggieback" on the current nREPL. I'm guessing that would alleviate at least some of these issues.

bbatsov commented 5 years ago

I understand your frustration with having to support CLJS in this way, guess one day we could have an independent nREPL implementation in it and not "piggieback" on the current nREPL. I'm guessing that would alleviate at least some of these issues.

That's the dream. :-) Unfortunately so far no one has been willing to invest time in doing it.

Btw, why are you concerned reading back the printed values?

For ClojureScript there's also the aforementioned #queue literal:

I guess for it and other common cases we can just provide some data readers with cider-nrepl (for instance).

ak-coram commented 5 years ago

That's the dream. :-) Unfortunately so far no one has been willing to invest time in doing it.

Sounds like a fun project, but also like a lot of work :)

Btw, why are you concerned reading back the printed values?

I personally like to use a few of my own data readers for my projects, so it would be a shame if using them would break pretty-printing (especially since this seemed to work fine in the past). I also defined IPrintWithWriter for several stateful types for which it makes no practical sense to define readers (but it's still useful to readably print them).

I'm thinking that maybe we can try to read and if it succeeds we can run everything through the nREPL printing middleware just as with Clojure forms. In case it fails it would nice to have a better fallback than cljs.core/pr-str though. Since the output of cljs.pprint/pprint should be just as readable to the reader as cljs.core/pr-str, why not use it as the default instead? This is also something decided internally in piggieback, so Cider and nREPL wouldn't need to be adjusted in any way.

I think this matters a lot to new users, because I doubt they're interested in customizing pretty-printing, but very quickly notice when something is not pretty-printed. We could also detect if cider-print-fn is set to pr (or nil), disable pretty-printing and use cljs.core/pr-str instead.

I guess for it and other common cases we can just provide some data readers with cider-nrepl (for instance).

I thought about this as well, but since users can define their own printers and readers this doesn't seem like a clean solution. Forcing them to define readers for both Clojure and ClojureScript also seems hard to justify without explaining the internals of the current tooling. I've also noticed that most of the current alternatives for pretty-printing don't work with custom writers (print-method, print-dup) anyway.

Of course there's also the overhead of "printing, reading and then printing again" to consider. I think the greatest advantage of the current printer alternatives is speed, which might be completely negated by doing it twice.

Any thoughts on this?

ak-coram commented 5 years ago

I guess for it and other common cases we can just provide some data readers with cider-nrepl (for instance).

Those using a namespace-qualified var for cider-print-fn would also need to handle the dummy types (for example for #js) we pass to them for these values.

bbatsov commented 5 years ago

I'll need some time to process all of this, but in the mean time I'll ping @thheller, as I'm curious to hear his take on the issue. Lately we've been discussing improving/replacing piggieback and perhaps he has some thoughts/ideas in the area of pretty-printing.

ak-coram commented 5 years ago

I'll need some time to process all of this, but in the mean time I'll ping @thheller, as I'm curious to hear his take on the issue. Lately we've been discussing improving/replacing piggieback and perhaps he has some thoughts/ideas in the area of pretty-printing.

@bbatsov: sounds great, let me know if I can help in any way!

thheller commented 5 years ago

I don't have anything useful to contribute really.

Printing (pretty or not) can always produce strings that cannot be read back properly. Configuring printing is hard since it happens in another runtime and you can't just pass a function or modify a binding to control it.

:wrap-fn is sort of your only way to customize printing but in practice the results should be pretty much the same as pr-str and trying to read/pprint the resulting string value in CLJ. There will just be cases that you can't read but adding a default reader fixes most of those.

;; this fails
(clojure.edn/read-string "#object[cljs.user.Cheese]")

;; this is fine
(clojure.edn/read-string
  {:default (fn [tag data] [::lit tag data])}
  "#object[cljs.user.Cheese]")

;; [:user/lit object [cljs.user.Cheese]]

If you supply a :default function that returns some TaggedLiteral wrapper with actual print-method/pprint support you can probably get something that is better than what cljs.pprint would support without having to configure anything in the JS runtime.

ak-coram commented 5 years ago

Hi @thheller,

thanks, I didn't know about :default. If I understand correctly you're proposing something like this:

user> (deftype UnknownTaggedLiteral [tag data])
user.UnknownTaggedLiteral

user> (defmethod print-method UnknownTaggedLiteral
        [^UnknownTaggedLiteral this ^java.io.Writer w]
        (.write w (str "#" (.tag this) (.data this))))
#multifn[print-method 0x22c93dd0]

user> (clojure.pprint/pprint
       (clojure.edn/read-string
        {:default ->UnknownTaggedLiteral}
        "#object[cljs.user.Cheese]"))
#object[cljs.user.Cheese]
nil

I think this might work as a reasonable default, I don't think it works with any printers other than pr and pprint though (at least without further configuration). Also those using their own function for cider-print-fn would need to handle this type (unless they're relying on print-method themselves).

I know this is a bit pedantic, but as you mentioned technically nobody is forced to emit readable outputs via IPrintWithWriter, so we might have a reader failure even with :default:

user> (deftype Point [x y])
user.Point

user> (defmethod print-method Point
        [^Point this ^java.io.Writer w]
        (.write w (str "#<Point[" (.x this) "," (.y this) "]>")))
#multifn[print-method 0x22c93dd0]

user> (->Point 33 44)
#<Point[33,44]>

user> (clojure.pprint/pprint
       (clojure.edn/read-string
        {:default ->UnknownTaggedLiteral}
        "#<Point[33,44]>"))
Execution error at user/eval23243 (REPL:95).
Unreadable form

Therefore I would still suggest to default to cljs.pprint/pprint instead of cljs.core/pr-str when we can't read the results so we have a reasonable fallback.

I think we should also consider the performance implications, since this relies on "printing, reading and printing again" instead of just printing once.

thheller commented 5 years ago

Yes, it is easy to break the default prn but for the most part people don't. Or this might at least give them a reason not to break stuff.

I'm not a big fan of print,read,print either but performance wise it shouldn't be an issue. A human can't read the output of 1000s of evals per second anyways. If you use a faster pprint like fipp it may actually be faster to print,read,fipp than cljs.pprint alone but for structures where this becomes an issue pprint itself probably isn't usable anyways. Large values are problematic in their own right regardless of pretty printing or not.

The benefit of doing more in CLJ is that you have more control over the printing than you do if printing is all done in CLJS.

ak-coram commented 5 years ago

I'm not a big fan of print,read,print either but performance wise it shouldn't be an issue. A human can't read the output of 1000s of evals per second anyways. If you use a faster pprint like fipp it may actually be faster to print,read,fipp than cljs.pprint alone

To my knowledge fipp also works in CLJS, it would be nice to be able to leverage that in the future. I have no idea how the implementation compares to the one for CLJ, but I would be surprised if cljs-fipp is not faster than "cljs-print,clj-read,clj-fipp".

A human can't read the output of 1000s of evals per second anyways.

I agree, but I think it's more of an issue that you have to wait for the first print and read steps to fully complete on a large value (single eval) before you even get to see anything (even when the end result is then truncated to a dozen lines by the second print).

The benefit of doing more in CLJ is that you have more control over the printing than you do if printing is all done in CLJS.

I'm not sure what you mean, I imagine that at least from the perspective of fipp we should have a similar level of configurability for both CLJ and CLJS.

Even cljs.pprint seems to support a very similar configuration to clojure.pprint (such as length, level, right-margin, etc.):

https://cljs.github.io/api/cljs.pprint/

My point is, that out of the 5 supported (built-in) options in Cider (pr, pprint, fipp, puget, zprint) we have 3 (pr, pprint, fipp) which also have implementations in CLJS with nearly identical APIs. For at least these three we could pass the same configuration to the CLJS print call in piggieback to avoid the read and second print steps in CLJ.

For nil (nrepl.middleware.print/*print-fn*) and the other two (puget, zprint) we could still implement what you've suggested. For those who use their own qualified var in cider-print-fn, we can either do the same or we can expect them to also provide an implementation in CLJS (if they want it to work in the CLJS repl).

Not to mention that fipp and puget don't seem to rely on print-method, so we'd have to figure out how to configure them to emit our UnknownTaggedLiteral wrapper correctly (same goes for the custom var users):

user> (deftype UnknownTaggedLiteral [tag data])
user.UnknownTaggedLiteral
user> (defmethod print-method UnknownTaggedLiteral
        [^UnknownTaggedLiteral this ^java.io.Writer w]
        (.write w (str "#" (.tag this) (.data this))))
#multifn[print-method 0x4a6c1f6b]
user> (def x (clojure.edn/read-string
              {:default ->UnknownTaggedLiteral}
              "#object[cljs.user.Cheese]"))
#'user/x
user> (clojure.pprint/pprint x)
#object[cljs.user.Cheese]
nil
user> (require 'fipp.clojure)
nil
user> (fipp.clojure/pprint x)
#object[user.UnknownTaggedLiteral
        "0x1726b743"
        "user.UnknownTaggedLiteral@1726b743"]
nil
user> (require 'puget.printer)
nil
user> (puget.printer/pprint x)
#<user.UnknownTaggedLiteral@6556d7db>
nil
user> (require 'zprint.core)
nil
user> (zprint.core/zprint x)
#object[cljs.user.Cheese]
nil

Is there anything that I'm missing here?

ak-coram commented 5 years ago

Hi, I'd just like to add that I think we've mostly covered what is possible to do here right now and I would be keen to tackle either of the alternatives (read back everything in CLJ / try to offload to CLJS whenever possible) once a decision has been made on what approach to adopt. If anyone has any different suggestions I'd love if you'd chip in.

Anyway, I'd like to thank everyone for even reading this far into this thread :)

bbatsov commented 5 years ago

OK, so I thought about this a bit more and I think we should aim to do as much in Clojure and just ignore the cases where something can't be read back (potentially falling back to some cljs printing in those situations and shipping a few "essential" extra data readers with piggieback itself). My main concern about offloading the printing to piggieback is that it seems to me this will breaking the streaming of values to clients, as at least the current implementation just does the pretty-printing in a single step fully realizing the result in the process.

ak-coram commented 5 years ago

Thank you for you response.

My main concern about offloading the printing to piggieback is that it seems to me this will breaking the streaming of values to clients, as at least the current implementation just does the pretty-printing in a single step fully realizing the result in the process.

As far as I understand the status quo is the following: CLJS results are first fully realized by cljs.core/pr-str (used in the default CLJS wrapper function) and then passed back to CLJ in a repl-specific manner, but always ending up as single string (see cljs.repl/evaluate-form). This string is then sent as is to the client via nREPL.

Now I've looked at some of the implementations of IJavaScriptEnv and they mostly seem to be working directly with strings (and not streams or something alike). Sidenote: cljs.repl.browser is actually using clojure.core/read-string to parse (a wrapped) evaluation result, so I guess you could even execute your own code on the JVM from your connected browser. That's a feature I was unaware of... :)

https://github.com/clojure/clojurescript/blob/65e3e40abaca4bb7dbb5a7dfd9b7405fdcab7b74/src/main/clojure/cljs/repl/browser.clj#L310

So before we even start doing anything with the evaluation result in piggieback, it's neatly wrapped up into a (single-line) string. This would mean that we have no way of sending parts of the response to the client before evaluation (and printing) fully completes (however or wherever we do the pretty-printing). At least this seems to the case with the existing CLJS repl environments.

Of course it would be much nicer if the CLJS repl implementations didn't rely on passing strings, but I think as long as they do we don't have an effective way of streaming the results while the evaluation is going on (it definitely doesn't work now: no matter how big the result, I always get the results in a single message).

Even if all we have is a large string, maybe it would make sense to deliver it through several nREPL responses (so that individual responses are kept relatively small). In this case it could be even an advantage if the result is already pretty-printed: for example for a pretty-printed result (instead of a single-line one) we could send N lines at a time to the client (by wrapping the string in a java.io.BufferedReader or something similar).

I think at least the browser repl is using sockets internally, so maybe one day streaming the results will be possible without keeping everything in memory on the JVM. As for streaming from the nREPL server to the nREPL client, I suspect we can do it either way: whether we do the pretty-printing in Clojure or ClojureScript should be of no consequence.

What do you think?

ak-coram commented 5 years ago

Hi, I've pushed a bit of further work on this issue. Any feedback and testing would be appreciated.

The tests still fail, but I'm afraid I don't know how to handle the older nREPL versions (which seem to make them fail). Could someone give me a few pointers? Thanks!

bbatsov commented 4 years ago

The printing was changed once in 0.5 and then in 0.6, so supporting both would be pretty hard. Generally these days almost everyone is using nREPL 0.6, so I guess we can just drop support for 0.4 and 0.5 and simplify our lives.

ak-coram commented 4 years ago

Ok, that's fine with me. Should I go ahead and update project.clj accordingly or do you want to handle the deprecation in a separate PR? Guess the CI config might need to be updated as well.

I'm still not sure why the Java 8 / Clojure 1.10 tests are already succeeding.

ak-coram commented 4 years ago

I can't reproduce the remaining test failures on my own machine, but on the surface it looks like the result is sometimes not printed by the nREPL middleware (proper-ns-tracking). This doesn't seem to depend on only the Java version (we have failures and successes for both Java 8 and Java 11) or only the Clojure version (failures and successes for both 1.9 and 1.10). Any ideas? Thanks!

bbatsov commented 4 years ago

Damn! I totally dropped the ball on you! Sorry about that!

The failures seem kind of random to me, as I can't imagine they can really be Clojure/JDK version specific. Probably the real problem is that some operations are not happening in a reliable order or something along those lines. Generally @shen-tian is our resident expert when it comes to debugging hard to reproduce issues. Perhaps he can help with some insight?

bbatsov commented 4 years ago

@ak-coram One more thing - it'd be nice to document the pretty-printing and its limitations in the README. Our conversation here is pure gold and we should preserve its essence in the README. :-)

shen-tian commented 4 years ago

Oh dear. Our favourite java.lang.ClassCastException: clojure.lang.PersistentArrayMap cannot be cast to [B

This exception was popping up sporadically in the nREPL test suite for a while. That turned out to be related to having a eval that printed a long message interrupted. The bencode transport is quite sensitive to that, so it would choke and throw that message.

There was some discussion here https://github.com/nrepl/nrepl/issues/132 but think the fix was around how the interrupt worked.

I'm not familiar with the tests in piggieback, so not sure what could be causing that. But does that ring a bell?

shen-tian commented 4 years ago

Actually, that failure mode is just on Clojure 1.11/master. We have a different (and intermittent) failure mode on the other tests.

Given we are also not running tests on nREPL 0.7/Java 13 etc. either, it's probably worth updating the test suite first, and see what shape that's in first. For all we know, Clojure master actually broke the tests...

I can do that in the next few days. That should give us a bit more information to work with

bbatsov commented 4 years ago

I'm not familiar with the tests in piggieback, so not sure what could be causing that. But does that ring a bell?

I don't recall such failures in the past. @bhauman and @cichli did most of the piggieback work and they are way more familiar with the test suite, but unfortunately they've not been around recently.

bbatsov commented 4 years ago

Given we are also not running tests on nREPL 0.7/Java 13 etc. either, it's probably worth updating the test suite first, and see what shape that's in first. For all we know, Clojure master actually broke the tests...

Good point!

For all we know, Clojure master actually broke the tests...

Well, that'd be a relief. :-) Thanks for looking into this so quickly!

shen-tian commented 4 years ago

Just create a PR for adding nREPL 0.7 and Java 13, and the tests look healthy, so it's some changes from the PR that's causing the issues. I'll have a poke later to see if I can spot anything.

shen-tian commented 4 years ago

A good theory of what's broken:

https://github.com/shen-tian/piggieback/commit/3750bf4828ba1e08f8892dff36cd65a0860973bc <- this commit made all the tests go green in my fork.