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

Using load-file to emulate source-tracking eval will not work with piggieback 0.2.1 #1088

Closed
cichli opened this issue Apr 27, 2015 · 18 comments

Comments

@cichli
Copy link
Member

cichli commented Apr 27, 2015

See nrepl/piggieback@46e30fd. The file slot is now ignored, and instead the contents of file-path are always loaded from disk. This means that the interactive eval commands that rely on load-file will not do the expected thing at all when using piggieback 0.2.1.

I guess, ideally, the source-tracking eval patch (or something equivalent) will be merged to nREPL, and then the equivalent changes made in piggieback also - but I think this will potentially require upstream changes in ClojureScript itself. The string <cljs repl> is passed as the file name in a lot of places in cljs.repl, so piggieback would need some way of dynamically binding this.

Paging @cemerick... any thoughts on this?

@cemerick
Copy link
Contributor

I haven't merged the patch on NREPL-59 yet because I've continued to unravel the consequences of the ClojureScript REPL redesign from a couple months ago. The change in piggieback 0.2.1 is a part of this. It's really a fundamental break from the contract of the load-file op, but (a) it works well in the 99.999% scenario of local ClojureScript development, and (b) I couldn't see any other way forward given the "use cljs.repl" constraint.

There's lots of background I could go into with more time, but the tl;dr is that the "supported" API to the ClojureScript REPL offers two options: load files from disk (with line/column metadata), or evaluate expressions interactively (without such metadata). A lot of this flows from the demands of source maps and the whole-file expectations of REPL environment impls, key default implementations of the various REPL environments protocols, (and more fundamentally) the fact that ClojureScript straddles two contexts: compiler in JVM, runtime "over there" (note that this doesn't change even given a node-hosted compiler, as such a thing would continue to architecturally support dumping compilation output and source maps for consumption by a separate JS env).

Anyway, for any particular "edge case" compiler-dependent interaction like this, there's always the option of interfacing directly with the ClojureScript compiler…which piggieback did in the past, and which did not work out so well as it (me) was outpaced by upstream changes. Being able to say that piggieback works with and reuses cljs.repl/repl* comprehensively is great in terms of maintaining fundamental parity with whatever David & co. do. Of course, this ties our hands significantly in terms of supporting behaviours and capabilities that it doesn't envision, unless someone is very committed to fast-following upstream's REPL behaviour in a theoretical piggieback that touches the compiler directly. (There is apparently going to be a "compiler API" that might make this somewhat more tractable, but that's TBD.)

Absent someone stepping up to that role (or getting upstream to take patches to support this or whatever other "edge case" interactions come up), I'd suggest simply not attempting to support source tracking of file-sourced expression evaluations.

@cichli
Copy link
Member Author

cichli commented Apr 27, 2015

Thanks for the quick and detailed response! Yeah, I agree that the moving from using internals like evaluate-form to just using repl* directly has been a change for the better (less breakage between releases, proper special fn support, etc).

I understand what you mean by things like source maps (and just how CLJS REPLs work more like an interface to the compiler than an actual dynamic runtime) make doing this difficult in CLJS (possibly more so than it is worth).

It's also worth considering that with the advent of the :watch option for REPLs and tools like Figwheel, evaluating individual forms in files is less useful when developing CLJS, since you can just save the file and have the file on disk be recompiled automatically.

So perhaps, we should just focus on source-tracking eval in nREPL only. The file/line/column slots for eval requests will obviously be ignored by piggieback, but this is a caveat that we can document. I don't know about other nREPL clients, but FWIW in CIDER when we do an actual load-file (i.e. not an emulated source-tracking eval), we ensure that the file is saved on disk beforehand, so the subset of the contract that piggieback provides is sufficient for us (@bbatsov correct me if I'm wrong :-)).

@bbatsov any thoughts on what to do in the meantime?

@bbatsov
Copy link
Member

bbatsov commented Apr 27, 2015

So perhaps, we should just focus on source-tracking eval in nREPL only. The file/line/column slots for eval requests will obviously be ignored by piggieback, but this is a caveat that we can document. I don't know about other nREPL clients, but FWIW in CIDER when we do an actual load-file (i.e. not an emulated source-tracking eval), we ensure that the file is saved on disk beforehand, so the subset of the contract that piggieback provides is sufficient for us (@bbatsov correct me if I'm wrong :-)).

You're not.

@bbatsov any thoughts on what to do in the meantime?

My take is that the current situation is pretty problematic and I'll take source-tracking implemented in nREPL over our current hack any day of the week. While feature parity between nREPL & piggieback is highly desirable, our inability to ensure it right now shouldn't be the obstacle to adding support for this in nREPL.

@cemerick
Copy link
Contributor

@cichli Interesting that cider saves as a side effect of a file load. (I make a habit of not changing my emacs setup.) That's sort of unfortunate IMO, but I might be idiosyncratic in that opinion.

@bbatsov
Copy link
Member

bbatsov commented Apr 30, 2015

@cichli Interesting that cider saves as a side effect of a file load. (I make a habit of not changing my emacs setup.) That's sort of unfortunate IMO, but I might be idiosyncratic in that opinion.

Technically, it asks you to save the file before loading it, which is pretty reasonable IMO. An there's an option you added a while back to simply save the file automatically instead of asking to do so.

@cemerick
Copy link
Contributor

How embarrassing, so I did. #469. I have no recollection of it, but there it is, and the setting remains in my init.el. 😊

@bbatsov
Copy link
Member

bbatsov commented Apr 30, 2015

We'll forget all about it if you merge NREPL-59. ;-)

@bbatsov
Copy link
Member

bbatsov commented May 12, 2015

Seems this is now the only showstopper for CIDER 0.9. I doubt anything will be changed in nREPL soon, so we're left with one of two unpleasant options:

  • kill the load-file altogether (which would have been preference if nREPL support location info at least for Clojure)
  • make the load-file hack conditional (only for Clojure)

So, what should we do?

@bbatsov
Copy link
Member

bbatsov commented May 19, 2015

@cichli @cemerick Thoughts? This is the main blocker of the CIDER release.

@cemerick
Copy link
Contributor

The upshot of my prior comment was effectively the second of your options. Unpleasant or not, I don't expect piggieback to ever support source-tracking eval, NREPL-59 or not.

@bbatsov
Copy link
Member

bbatsov commented May 19, 2015

@cemerick If we merge NREPL-59 we can just kill this hack altogether. This will work on Clojure and won't work on ClojureScript, which is fine by me. Beats having a conditional hack any day of the week.

@bbatsov
Copy link
Member

bbatsov commented May 19, 2015

Not to mention it will remove all the confusing load-file message people now see in their nrepl-messages logs and have trouble comprehending.

@bbatsov
Copy link
Member

bbatsov commented May 19, 2015

Btw, @swannodette, what's your take on all of this?

@swannodette
Copy link

@bbatsov no strong opinions since I'm not an active Cider user. Even after reading this whole thread and the related links I'm not sure what ClojureScript needs to do to support this.

@bbatsov
Copy link
Member

bbatsov commented May 19, 2015

@swannodette Well, this is not an CIDER issue, per se. The problem is that any REPL-based ClojureScript tool can't keep track of the var definition locations, as you cannot pass this location somehow to the ClojureScript REPL at the moment.

This is the most relevant part of the discussion for you. @cemerick can't find a simple way to implement the definition location tracking in piggieback, which blocks us from implementing it in nREPL and CIDER respectively.

There's lots of background I could go into with more time, but the tl;dr is that the "supported" API to the ClojureScript REPL offers two options: load files from disk (with line/column metadata), or evaluate expressions interactively (without such metadata). A lot of this flows from the demands of source maps and the whole-file expectations of REPL environment impls, key default implementations of the various REPL environments protocols, (and more fundamentally) the fact that ClojureScript straddles two contexts: compiler in JVM, runtime "over there" (note that this doesn't change even given a node-hosted compiler, as such a thing would continue to architecturally support dumping compilation output and source maps for consumption by a separate JS env).

Anyway, for any particular "edge case" compiler-dependent interaction like this, there's always the option of interfacing directly with the ClojureScript compiler…which piggieback did in the past, and which did not work out so well as it (me) was outpaced by upstream changes. Being able to say that piggieback works with and reuses cljs.repl/repl* comprehensively is great in terms of maintaining fundamental parity with whatever David & co. do. Of course, this ties our hands significantly in terms of supporting behaviours and capabilities that it doesn't envision, unless someone is very committed to fast-following upstream's REPL behaviour in a theoretical piggieback that touches the compiler directly. (There is apparently going to be a "compiler API" that might make this somewhat more tractable, but that's TBD.)

@swannodette
Copy link

@bbatsov there's just nothing for me to do here yet. Someone else needs to describe something actionable for ClojureScript like enumerating implementation strategies and their tradeoffs. I'm more than happy to sort through ideas once they are in a presentable form and guide someone else's work on a patch to ClojureScript.

@bbatsov
Copy link
Member

bbatsov commented May 19, 2015

Fair enough.

@bbatsov
Copy link
Member

bbatsov commented May 19, 2015

@cichli @gtrak Now we need some actionable idea for implementing what we need. :-)

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

No branches or pull requests

4 participants