Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use nrepl pretty printing support #108

Merged
merged 10 commits into from
May 9, 2020

Conversation

ak-coram
Copy link
Contributor

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
Copy link
Contributor Author

ak-coram commented Aug 12, 2019

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
Copy link
Contributor

bbatsov commented Aug 12, 2019

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
Copy link
Contributor Author

ak-coram commented Aug 12, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

I decided to check what clojurescript's browser env returns from evaluations, it looks like it does this: https:/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:/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
Copy link
Contributor Author

ak-coram commented Aug 14, 2019

@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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

ak-coram commented Aug 14, 2019

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
Copy link
Contributor

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

@ak-coram
Copy link
Contributor Author

@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
Copy link
Contributor Author

ak-coram commented Aug 14, 2019

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
Copy link
Contributor

bbatsov commented Aug 22, 2019

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
Copy link
Contributor Author

ak-coram commented Sep 1, 2019

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

@bbatsov
Copy link
Contributor

bbatsov commented Sep 7, 2019

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
Copy link
Contributor Author

ak-coram commented Oct 5, 2019

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
Copy link
Contributor

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
Copy link
Contributor Author

ak-coram commented Oct 5, 2019

@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
Copy link
Contributor Author

ak-coram commented Oct 5, 2019

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

`(let [sb# (goog.string.StringBuffer.)
sbw# (cljs.core/StringBufferWriter. sb#)
form# ~form]
(cljs.pprint/pprint form# sbw#)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this approach is that we're effectively hardcoding pretty-printing, and nREPL goes to great lengths to ensure that printing is flexible and you can control what exactly do you want to print and how. I think it would be better if we made piggieback respect the nREPL infrastructure instead of forcing one particular way of pretty printing it in.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the downside of reading on the clojure side and allowing nrepl to print it?

@bbatsov
Copy link
Contributor

bbatsov commented Oct 5, 2019

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
Copy link
Contributor

bbatsov commented Oct 5, 2019

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
Copy link
Contributor Author

ak-coram commented Oct 5, 2019

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
Copy link
Contributor

bbatsov commented Oct 5, 2019

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
Copy link
Contributor Author

ak-coram commented Oct 5, 2019

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

  • Functions
user> (fn [x] x)
#function[user/eval23287/fn--23288]
user> (read-string "#function[user/eval23287/fn--23288]")
Execution error at user/eval23291 (REPL:27).
No reader function for tag function
  • Objects
user> (java.time.LocalDate/now)
#object[java.time.LocalDate 0x2647cbb8 "2019-10-05"]
user> (read-string "#object[java.time.LocalDate 0x2647cbb8 \"2019-10-05\"]")
Execution error at user/eval23295 (REPL:35).
No reader function for tag object

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
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
  • CLJS Functions
c.user> (fn [x] x)
#object[Function]
user> (read-string "#object[Function]")
Execution error at user/eval23346 (REPL:62).
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
Copy link
Contributor Author

ak-coram commented Oct 5, 2019

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.

@shen-tian
Copy link
Contributor

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.

(edn-reader/read-string
{:default ->UnknownTaggedLiteral}
result))
:nrepl.middleware.print/keys #{:value}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to update piggieback to :require print/wrap-print`

@shen-tian
Copy link
Contributor

A good theory of what's broken:

  • Piggieback now need to be below wrap-print in the stack, but we didn't declare that dependency.
  • I've observed some non-deterministic behaviour in middleware ordering in nREPL, as many ordering meet the declared requirements. Not sure what causes the randomness.
  • Thus, if there are under declared requirements, the system is not reliably functional, thus tests fail (as they should)

shen-tian@3750bf4 <- this commit made all the tests go green in my fork.

@ak-coram
Copy link
Contributor Author

ak-coram commented May 7, 2020

Thank you @bbatsov and @shen-tian for your help! It really seems we're
getting close to resolving this after all this time.

it'd be nice to document the pretty-printing and its limitations in the README.

Yes, I agree this PR is a bit lacking in terms of documentation. Would
you prefer to deal with this in the same PR or should I open a new
one?

shen-tian/piggieback@3750bf4 <- this commit made all the tests go green in my fork.

Thanks, I've merged it. The implicit dependency on wrap-print is a
perfectly reasonable explanation for the sporadic test failures.

@codecov-io
Copy link

codecov-io commented May 7, 2020

Codecov Report

Merging #108 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #108   +/-   ##
=======================================
  Coverage   83.33%   83.33%           
=======================================
  Files           1        1           
  Lines          12       12           
  Branches        1        1           
=======================================
  Hits           10       10           
  Misses          1        1           
  Partials        1        1           
Impacted Files Coverage Δ
src/cider/piggieback.clj 83.33% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5d8b25...a574d42. Read the comment docs.

@bbatsov
Copy link
Contributor

bbatsov commented May 7, 2020

Yes, I agree this PR is a bit lacking in terms of documentation.

Let's add it to this PR.

@bbatsov
Copy link
Contributor

bbatsov commented May 7, 2020

You'll also have to rebase on top of the latest master.

@@ -32,8 +32,6 @@
:dependencies [[org.clojure/clojure "1.11.0-master-SNAPSHOT"]
[org.clojure/clojurescript "1.10.439"]]}

:nrepl-0.4 {:dependencies [[nrepl/nrepl "0.4.5"]]}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll have to mention in the README/Changelog that Piggieback now requires nREPL 0.6+.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll have to mention in the README/Changelog that Piggieback now requires nREPL 0.6+.

Updated the README & CHANGELOG with new nREPL version requirement changes.

@@ -203,12 +211,45 @@
(readers/source-logging-push-back-reader
(java.io.StringReader. form-str))))))

(defn- wrap-pprint [form]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add some explanation about the naming here, as wrap-something is the standard convention for naming middleware functions in nREPL and this might confuse some people.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add some explanation about the naming here, as wrap-something is the standard convention for naming middleware functions in nREPL and this might confuse some people.

Added a docstring to clarify that this wraps an s-expression with cljs.pprint/pprint. We can also change the name to something else if you prefer.

ak-coram and others added 4 commits May 7, 2020 22:43
Try to read the CLJS evaluation result in CLJ and rely on the nREPL
printing middleware to print it if reading succeeds. The type
UnknownTaggedLiteral is introduced to be able to read and print tagged
literals which are specific to CLJS such as #queue (or any
user-defined ones). The output may vary based on the printing function
used (for example fipp and puget don't rely on the supplied
print-method for UnknownTaggedLiteral).

Reading can still fail in some cases, so a fallback is also included
by trying to detect if pretty printing is enabled
(nrepl.middleware.print/print not set to "cider.nrepl.pprint/pr") and
if that is the case, wrapping the CLJS form with cljs.pprint/pprint
(instead of the default cljs.core.pr-str). This way pretty printing
can still be turned on or off, even when reading fails.
@ak-coram
Copy link
Contributor Author

ak-coram commented May 7, 2020

Rebased on top of the latest master.

@bbatsov
Copy link
Contributor

bbatsov commented May 8, 2020

We only need some coverage of the print functionality in the README and the changelog and we're good to go.

@ak-coram
Copy link
Contributor Author

ak-coram commented May 8, 2020

We only need some coverage of the print functionality in the README and the changelog and we're good to go.

Sounds great, I'll try to find time on the weekend to do a write-up for the README.

@ak-coram
Copy link
Contributor Author

ak-coram commented May 9, 2020

@bbatsov: I've tried to summarize the parts of this discussion which are relevant to the current solution. Please let me know what you think. Thanks!

@bbatsov bbatsov merged commit aa037fd into nrepl:master May 9, 2020
@bbatsov
Copy link
Contributor

bbatsov commented May 9, 2020

Looks good. I'll tweak the docs a bit before cutting a release, but overall you've done great!

I'm happy that we finally managed to wrap this up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants