tools.nrepl

Tracking source form positions in eval

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: 0.2.3
  • Fix Version/s: None
  • Component/s: None
  • Labels:

Description

Dev tools writers using nREPL's eval op face the following problem - using the `eval` op you cannot pass information regarding the filename and the file position of the form you're evaluating and therefore afterwards you can't use find its definition. Because of this most CIDER users reload their source buffers all the time with `load-file`. Another implication of this problem is that exception backtraces have meaningless information about the position of the problem (usually something like `NO_FILE:1`). I've noticed that LightTable has a custom middleware for this, ritz used to have one as well, probably others too. Now I'm contemplating doing something similar for CIDER, but it seems to me this is basic functionality that should be supported out-of-the-box.

I'd like to propose that the default eval middleware be augmented to support some form of source form tracking to spare tool writers from having to reinvent the wheel.

Activity

Hide
Chas Emerick added a comment -

I agree that this should be baked in. `:line` and `:column` are some optional slots eval could accept (but should not require), with the obvious effect on the reader passed along to the compiler. This may be trickier than it sounds, especially if you're aiming to keep things at parity for both Clojure and ClojureScript (as code to be eval'd is read well before it hits interruptible_eval, in piggieback).

In any case, patch quite welcome.

Show
Chas Emerick added a comment - I agree that this should be baked in. `:line` and `:column` are some optional slots eval could accept (but should not require), with the obvious effect on the reader passed along to the compiler. This may be trickier than it sounds, especially if you're aiming to keep things at parity for both Clojure and ClojureScript (as code to be eval'd is read well before it hits interruptible_eval, in piggieback). In any case, patch quite welcome.
Hide
Bozhidar Batsov added a comment -

`:filepath` should be another optional slot. I don't do much ClojureScript (and I don't know much about piggieback) so getting something working for Clojure would be a good enough start for me. I'll try to cook some patch soon.

Show
Bozhidar Batsov added a comment - `:filepath` should be another optional slot. I don't do much ClojureScript (and I don't know much about piggieback) so getting something working for Clojure would be a good enough start for me. I'll try to cook some patch soon.
Hide
Bozhidar Batsov added a comment -

Chas, can you give me some pointers about how to approach the task. I don't think we can use `clojure.main/repl` for this and I'm not sure what makes sense and what doesn't. One implementation of the idea can be found here https://github.com/pallet/ritz/blob/develop/nrepl-middleware/src/ritz/nrepl/middleware/tracking_eval.clj but I'm not sure whether it's good or bad.

Seems to me there should be a simpler way to provide a context for the evaluation, but I'm far from an expert in the area.

Show
Bozhidar Batsov added a comment - Chas, can you give me some pointers about how to approach the task. I don't think we can use `clojure.main/repl` for this and I'm not sure what makes sense and what doesn't. One implementation of the idea can be found here https://github.com/pallet/ritz/blob/develop/nrepl-middleware/src/ritz/nrepl/middleware/tracking_eval.clj but I'm not sure whether it's good or bad. Seems to me there should be a simpler way to provide a context for the evaluation, but I'm far from an expert in the area.
Hide
Chas Emerick added a comment -

Now I remember what I was planning the last time I thought about it: the easiest way AFAICT would be to use load-file, and simply pad out the top-level form being evaluated. So if you had this file:

(ns foo.bar
  (:require clojure.set))

(defn x [] 5)

    (defn y [] 6)

and the user loads `y`, you'd send this in a load-file message with appropriate file path information:

(ns foo.bar)




    (defn y [] 6)

load-file turns this into a call like (clojure.lang.Compiler/load "(ns foo.bar)\n\n\n\n\n (defn y [] 6)" "foo/bar.clj" "bar.clj". That doesn't change *ns*, and gives you proper metadata.

I've only skimmed Ritz's implementation, but it does a lot of clever things, and is tied to Clojure. Perhaps there's reasons why my (perhaps hacky?) approach isn't suitable in certain situations, but it should be pretty future-proof, and should work transparently with ClojureScript/piggieback. I'd rather dummy up "files" to be loaded than overriding LineNumberingPushbackReader method impls, etc.

Show
Chas Emerick added a comment - Now I remember what I was planning the last time I thought about it: the easiest way AFAICT would be to use load-file, and simply pad out the top-level form being evaluated. So if you had this file:
(ns foo.bar
  (:require clojure.set))

(defn x [] 5)

    (defn y [] 6)
and the user loads `y`, you'd send this in a load-file message with appropriate file path information:
(ns foo.bar)




    (defn y [] 6)
load-file turns this into a call like (clojure.lang.Compiler/load "(ns foo.bar)\n\n\n\n\n (defn y [] 6)" "foo/bar.clj" "bar.clj". That doesn't change *ns*, and gives you proper metadata. I've only skimmed Ritz's implementation, but it does a lot of clever things, and is tied to Clojure. Perhaps there's reasons why my (perhaps hacky?) approach isn't suitable in certain situations, but it should be pretty future-proof, and should work transparently with ClojureScript/piggieback. I'd rather dummy up "files" to be loaded than overriding LineNumberingPushbackReader method impls, etc.
Hide
Bozhidar Batsov added a comment -

Your idea sounds great to me. I guess the only question is whether the clients should take care of this or nREPL when passed extra arguments to the eval op.

Show
Bozhidar Batsov added a comment - Your idea sounds great to me. I guess the only question is whether the clients should take care of this or nREPL when passed extra arguments to the eval op.
Hide
Chas Emerick added a comment -

All of the source-file-aware stuff is in load-file, so I'm inclined to put the burden on the client. Even if an op accepted the necessary info, the client would still need to provide it (target namespace + line and column information); going from there to a dummied-up ns form and the corresponding whitespace is so straightforward, I'd rather not complicate the op interface(s).

Show
Chas Emerick added a comment - All of the source-file-aware stuff is in load-file, so I'm inclined to put the burden on the client. Even if an op accepted the necessary info, the client would still need to provide it (target namespace + line and column information); going from there to a dummied-up ns form and the corresponding whitespace is so straightforward, I'd rather not complicate the op interface(s).
Hide
Bozhidar Batsov added a comment -

Roger that! I've just implemented this in cider and it works great. I'll be closing this. Thanks for all your help!

Show
Bozhidar Batsov added a comment - Roger that! I've just implemented this in cider and it works great. I'll be closing this. Thanks for all your help!
Hide
Bozhidar Batsov added a comment -

P.S. You should probably mention this technique in the README for posterity's sake.

Show
Bozhidar Batsov added a comment - P.S. You should probably mention this technique in the README for posterity's sake.
Hide
Bozhidar Batsov added a comment -

P.S. You should probably mention this technique in the README for posterity's sake.

Show
Bozhidar Batsov added a comment - P.S. You should probably mention this technique in the README for posterity's sake.
Hide
Chas Emerick added a comment -

True. Really, the repo should have a separate file for implementors, as there's a lot of stuff in the source and tribal knowledge that isn't documented anywhere....

Show
Chas Emerick added a comment - True. Really, the repo should have a separate file for implementors, as there's a lot of stuff in the source and tribal knowledge that isn't documented anywhere....
Hide
Bozhidar Batsov added a comment -

Btw, looking at this ticket https://github.com/clojure-emacs/cider/issues/781 I'm not sure we can use just (ns ns-name) at the start of the dummy files as it seems this overrides the namespace definition. Perhaps we should always insert the complete ns declaration from the source file in question? Or is there some other smart trick I'm not aware of that'll help avoid this ns clobbering.

Show
Bozhidar Batsov added a comment - Btw, looking at this ticket https://github.com/clojure-emacs/cider/issues/781 I'm not sure we can use just (ns ns-name) at the start of the dummy files as it seems this overrides the namespace definition. Perhaps we should always insert the complete ns declaration from the source file in question? Or is there some other smart trick I'm not aware of that'll help avoid this ns clobbering.
Hide
Chas Emerick added a comment -

load-file only changes *ns* for the duration of the loading process. If it clobbered *ns* otherwise, then loading entire files would change it in the REPL (which doesn't happen)…

Show
Chas Emerick added a comment - load-file only changes *ns* for the duration of the loading process. If it clobbered *ns* otherwise, then loading entire files would change it in the REPL (which doesn't happen)…
Hide
Bozhidar Batsov added a comment - - edited

Are we talking about the same thing? My concern is that swapping:

(ns foo
   (:require [bar :as b]))

with

(ns foo)

changes the ns definition (not the ns itself) in a manner in which you're no longer referring to bar as b anymore.

Show
Bozhidar Batsov added a comment - - edited Are we talking about the same thing? My concern is that swapping:
(ns foo
   (:require [bar :as b]))
with
(ns foo)
changes the ns definition (not the ns itself) in a manner in which you're no longer referring to bar as b anymore.
Hide
Vitalie Spinu added a comment -

Another solution is to allow the `eval` middleware to accept a token that will be appended to the temp file that eval uses for evaluation. Then the client can split the file path and retrive the metadata back. You can even allow for a full path of a temp file as an argument. This should be trivial to implement.

Show
Vitalie Spinu added a comment - Another solution is to allow the `eval` middleware to accept a token that will be appended to the temp file that eval uses for evaluation. Then the client can split the file path and retrive the metadata back. You can even allow for a full path of a temp file as an argument. This should be trivial to implement.
Hide
Bozhidar Batsov added a comment -

The problem with the solution you propose is that *eval* is implemented in terms of Clojure's built *clojure.main/repl* and we obviously can't change it to suit our needs. Don't know about you, but I definitely don't want *eval* to have to be implemented in terms of some custom evaluation logic developed specially for nREPL.

The current solution works well enough and requires zero middleware changes. The only question is does the temp file need the complete ns declaration of the original source file or not.

Show
Bozhidar Batsov added a comment - The problem with the solution you propose is that *eval* is implemented in terms of Clojure's built *clojure.main/repl* and we obviously can't change it to suit our needs. Don't know about you, but I definitely don't want *eval* to have to be implemented in terms of some custom evaluation logic developed specially for nREPL. The current solution works well enough and requires zero middleware changes. The only question is does the temp file need the complete ns declaration of the original source file or not.
Hide
Vitalie Spinu added a comment -

I have just looked at the code and it looks like you are right. I am now confused. Where do "tmp/form-init34343" files come from? I thought they are handled in nREPL.

Show
Vitalie Spinu added a comment - I have just looked at the code and it looks like you are right. I am now confused. Where do "tmp/form-init34343" files come from? I thought they are handled in nREPL.
Hide
Bozhidar Batsov added a comment -

It comes from Clojure itself (or rather its built-in REPL evaluation). These tmp filenames appeared in Clojure 1.6. In older Clojure releases the var metadata had just *NO_FILE* for *:file*.

Show
Bozhidar Batsov added a comment - It comes from Clojure itself (or rather its built-in REPL evaluation). These tmp filenames appeared in Clojure 1.6. In older Clojure releases the var metadata had just *NO_FILE* for *:file*.
Hide
Chas Emerick added a comment -

ns declarations are strictly additive AFAICT:

user=> (ns foo (:require [clojure.set :as set]))
nil
foo=> set/intersection
#<set$intersection clojure.set$intersection@3e4db6a>
foo=> (ns foo)
nil
foo=> set/intersection
#<set$intersection clojure.set$intersection@3e4db6a>

And using load-file, which is effectively what the load-file op duplicates:

chas@t440p:~$ cat > foo.clj
(ns foo (:require [clojure.set :as set]))
chas@t440p:~$ cat > foo2.clj
(ns foo)
chas@t440p:~$ lein repl
nREPL server started on port 40749 on host 127.0.0.1 - nrepl://127.0.0.1:40749

user=> (load-file "foo.clj")
nil
user=> (in-ns 'foo)
#<Namespace foo>
foo=> set/intersection
#<set$intersection clojure.set$intersection@2fcecc1>
foo=> (in-ns 'user)
#<Namespace user>
user=> (load-file "foo2.clj")
nil
user=> (in-ns 'foo)
#<Namespace foo>
foo=> set/intersection
#<set$intersection clojure.set$intersection@2fcecc1>

The load-file op appears to work properly outside of this use case as well, since I can (from cider) remove a :require clause from a file, reload it, and its alias remains in place.

If you seem to need to include the full ns declaration, then something else is amiss somewhere...

Show
Chas Emerick added a comment - ns declarations are strictly additive AFAICT:
user=> (ns foo (:require [clojure.set :as set]))
nil
foo=> set/intersection
#<set$intersection clojure.set$intersection@3e4db6a>
foo=> (ns foo)
nil
foo=> set/intersection
#<set$intersection clojure.set$intersection@3e4db6a>
And using load-file, which is effectively what the load-file op duplicates:
chas@t440p:~$ cat > foo.clj
(ns foo (:require [clojure.set :as set]))
chas@t440p:~$ cat > foo2.clj
(ns foo)
chas@t440p:~$ lein repl
nREPL server started on port 40749 on host 127.0.0.1 - nrepl://127.0.0.1:40749

user=> (load-file "foo.clj")
nil
user=> (in-ns 'foo)
#<Namespace foo>
foo=> set/intersection
#<set$intersection clojure.set$intersection@2fcecc1>
foo=> (in-ns 'user)
#<Namespace user>
user=> (load-file "foo2.clj")
nil
user=> (in-ns 'foo)
#<Namespace foo>
foo=> set/intersection
#<set$intersection clojure.set$intersection@2fcecc1>
The load-file op appears to work properly outside of this use case as well, since I can (from cider) remove a :require clause from a file, reload it, and its alias remains in place. If you seem to need to include the full ns declaration, then something else is amiss somewhere...
Hide
Bozhidar Batsov added a comment -

I couldn't reproduce the problem some people are reporting, so I'm inclined to believe you. I've just merged a patch that uses the full ns declarations all the time, but I may revert it in light of your comment. I wonder if this behaviour changes in different Clojure versions.

Show
Bozhidar Batsov added a comment - I couldn't reproduce the problem some people are reporting, so I'm inclined to believe you. I've just merged a patch that uses the full ns declarations all the time, but I may revert it in light of your comment. I wonder if this behaviour changes in different Clojure versions.
Hide
Chas Emerick added a comment -

Using the full ns declaration isn't necessarily bad, but there is the edge case of people who add :reload and/or :reload-all options to their :require, etc clauses. In such cases, sending a single top-level expr to be loaded will then cause other namespaces to be reloaded, perhaps with side effects (if those other namespaces have impolite top-level forms themselves).

Re: version differences, I'm pretty sure that's not an issue either:

chas@t440p:~/dev/pdfts$ java -cp ~/Downloads/clojure-1.0.0.jar clojure.main
Clojure 1.0.0-
user=> *clojure-version*
{:major 1, :minor 0, :incremental 0, :qualifier ""}
user=> (load-file "foo.clj")
nil
user=> (in-ns 'foo)
#<Namespace foo>
foo=> set/intersection
#<set$intersection__5909 clojure.set$intersection__5909@5b0abc94>
foo=> (in-ns 'user)
#<Namespace user>
user=> (load-file "foo2.clj")
nil
user=> (in-ns 'foo)
#<Namespace foo>
foo=> set/intersection
#<set$intersection__5909 clojure.set$intersection__5909@5b0abc94>

I could believe that there's some bug/oddity with the implementation of the load-file op in this case, but the underlying mechanism is stable and behaves non-destructively.

Show
Chas Emerick added a comment - Using the full ns declaration isn't necessarily bad, but there is the edge case of people who add :reload and/or :reload-all options to their :require, etc clauses. In such cases, sending a single top-level expr to be loaded will then cause other namespaces to be reloaded, perhaps with side effects (if those other namespaces have impolite top-level forms themselves). Re: version differences, I'm pretty sure that's not an issue either:
chas@t440p:~/dev/pdfts$ java -cp ~/Downloads/clojure-1.0.0.jar clojure.main
Clojure 1.0.0-
user=> *clojure-version*
{:major 1, :minor 0, :incremental 0, :qualifier ""}
user=> (load-file "foo.clj")
nil
user=> (in-ns 'foo)
#<Namespace foo>
foo=> set/intersection
#<set$intersection__5909 clojure.set$intersection__5909@5b0abc94>
foo=> (in-ns 'user)
#<Namespace user>
user=> (load-file "foo2.clj")
nil
user=> (in-ns 'foo)
#<Namespace foo>
foo=> set/intersection
#<set$intersection__5909 clojure.set$intersection__5909@5b0abc94>
I could believe that there's some bug/oddity with the implementation of the load-file op in this case, but the underlying mechanism is stable and behaves non-destructively.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated: