Use load-file op in monroe-load-file.

[technomancy]

Let's use the declarative version instead of just shooting Clojure code across the wire.

Also remove reference to tramp-compat-file-local-name which doesn't appear to exist.

[technomancy]

Oh dear; it appears the reference Clojure nrepl server has a broken load-file op: https://github.com/nrepl/nrepl/blob/master/src/clojure/nrepl/middleware/load_file.clj#L61 where it just tries to evaluate the filename as if it were code.

[technomancy]

OK so I just re-read the spec and it turns out the spec is ... reeeeeally weird:

:file Full contents of a file of code.

https://nrepl.org/nrepl/ops.html#load-file

Apparently it's the client's responsibility to read the whole file and send it across the wire? So the load-file op is basically just a worse version of eval. Bizarre! So monroe is still doing the wrong thing, but in a way that just happens to work.

I honestly think this needs to be fixed in the spec; sending the whole contents of the file across the wire makes no sense.

[sanel]

Thank you!

cc @bbatsov: wasn't nREPL load-file behavior left to implemetation? Can't recall the original spec and I might be wrong... What would be difference between load-file and just eval-ing everything? I find load-file much faster.

[bbatsov]

Apparently it's the client's responsibility to read the whole file and send it across the wire? So the load-file op is basically just a worse version of eval. Bizarre! So monroe is still doing the wrong thing, but in a way that just happens to work.

If I recall correctly load-file exists mostly because it was much easier to have location metadata attached to the vars if you feed a whole file to the compiler. These days eval is much improved and load-file is implemented in terms of load-file, so from time to time I wonder if load-file is even needed (other than for historical compatibility reasons).

[bbatsov]

See nrepl/nrepl@c27681c for details.

Apparently it's the client's responsibility to read the whole file and send it across the wire?

Indeed. Looking at the code this has always been the case.

[technomancy]

If I recall correctly load-file exists mostly because it was much easier to have location metadata attached to the vars if you feed a whole file to the compiler.

I think it still makes sense to have this as a top-level op! For example, maybe you want to load a file that you haven't opened in the editor, or maybe you want to load a file that's on the other side of the network boundary and your client can't access it.

But the current parameter doesn't make sense. It should take the filename as a parameter and actually use that argument.

These days eval is much improved and load-file is implemented in terms of load-file, so from time to time I wonder if load-file is even needed (other than for historical compatibility reasons).

I think what you're saying is that the eval implementation in either the original Clojure server implementation (or maybe in cider?) has improved. This does not seem relevant to whether the spec should have a load-file op.

I would propose that the spec be updated to treat the current behavior as deprecated and recommend that file-path be interpreted as the path to the file to be loaded. Unfortunately the spec is rather vague on the current meaning of this parameter; it says "Source-path-relative path of the source file" but it isn't clear whether this means relative to the root of the project or relative to some kind of load-path. (The example loosely implies that it's the latter, but in a way that would not be comprehensible to anyone who hasn't used Clojure.)

But this is starting to go a bit off-topic. Should we continue the discussion in the nrepl issue tracker?

[bbatsov]

But this is starting to go a bit off-topic. Should we continue the discussion in the nrepl issue tracker?

Yeah, that would be best.

[technomancy]

I've got a new version of the spec here that addresses this: nrepl/spec.nrepl.org#1

@sanel as an nREPL implementer, your input on the spec would be appreciated.

Depending on how that goes I'll update it here.