Remove runtime-specific code / improve nREPL?

[jackrusher]

Branching off from our conversation around the nbb bug in this issue:

#3061 (comment)

I have recently implemented an nREPL for a new evaluation context, and I found a few odd things with how Cider uses the nREPL protocol. For example, the nREPL "spec" claims that the op for completions is completions, but Cider uses complete. Presumably, all the other clients in the Clojure space do likewise, so I guess we should fix the spec.

In addition, when the server is missing ops that Cider wants, it sends JVM-specific forms to eval to try to get a similar effect to the op. This means that when someone is building an nREPL server in a non-JVM context they must detect those forms via string comparison and take evasive action or just send endless exception messages to Cider.

None of this is great for tool builders, so I was hoping that maybe we could clean things up a bit and make the standard more of a standard. 😊

[borkdude]

I notice that when connecting with CIDER to a custom nREPL server, it wants to evaluate the "version" op.

:msg {"op" "version", "prefix-rewriting" #object["[B" 0xb48f9be "[B@b48f9be"], "insert-newline-after-require" #object["[B" 0x7c6db4d1 "[B@7c6db4d1"], "debug" #object["[B" 0x58a5ab43 "[B@58a5ab43"], "session" "e9a00b57-d5c8-42c2-bf86-35a885ad048c", "id" "7"}

I do not see any documention about this op (https://nrepl.org/nrepl/ops.html), nor does my nREPL server say in its "describe" message that it supports such an op.

@bbatsov Would it be best to create sub-issues and track them in this meta issue? You can simply do this by creating a list like so:

• Remove runtime-specific code / improve nREPL? #3193

I have trouble finding out where cider.el requests something with "op" "version".

(-->
  id                           "8"
  op                           "version"
  session                      "6513be93-8f83-4c34-8f30-9989df20c9fb"
  time-stamp                   "2022-05-03 12:33:36.178101000"
  debug                        "false"
  insert-newline-after-require "true"
  prefix-rewriting             "false"
)

[borkdude]

I think it would already be a major step forwards if:

• CIDER does not try to send any ops that aren't declared as supported in "describe"
• When CIDER tries to eval something like (System/getProperty "java.class.path"), it should handle such things very gracefully, accepting that the server might not be able to process that request successfully

[bbatsov]

CIDER does not try to send any ops that aren't declared as supported in "describe"

I don't quite get this, as if you try to invoke an unsupported op you'll just get an error.

When CIDER tries to eval something like (System/getProperty "java.class.path"), it should handle such things very gracefully, accepting that the server might not be able to process that request successfully

Yeah, I totally agree. Probably all such evals should be guarded with some runtime checks, as those are pretty simple (the runtime info is attached to the connection).

At the protocol level it'd be good to know what ops do you think are missing and we can discuss them in more detail.

I have recently implemented an nREPL for a new evaluation context, and I found a few odd things with how Cider uses the nREPL protocol. For example, the nREPL "spec" claims that the op for completions is completions, but Cider uses complete. Presumably, all the other clients in the Clojure space do likewise, so I guess we should fix the spec.

That's different for historical reasons, as the CIDER op predated the nREPL op by 7-8 years. As namespacing the CIDER ops would have taken a lot of time and would have been very painful for the end users I was forced to choose a different name for the nREPL op to avoid conflicts. I still plan to namespace the CIDER ops to reduce the confusion with them, but I don't have a timeline for this (e.g. complete will become cider/complete and so on) CIDER works with completions just fine if complete is not supported.

[borkdude]

I don't quite get this, as if you try to invoke an unsupported op you'll just get an error.

Well, this depends on the nREPL server. I implemented an nREPL server under the assumption that the client will only ask for ops that are supported, as described in the "describe" response - isn't that what the describe response is for?

[borkdude]

https://nrepl.org/nrepl/ops.html#describe

Produce a machine- and human-readable directory and documentation for the operations supported by an nREPL endpoint.

[bbatsov]

I'll mention here that the conversation for nREPL improvements is taking place mostly here nrepl/nrepl#273 right now.

As for this particular ticket - I'll take it upon myself to make the problematic "tooling" evals in CIDER less problematic by either removing them or making them no ops.

[bbatsov]

Well, this depends on the nREPL server. I implemented an nREPL server under the assumption that the client will only ask for ops that are supported, as described in the "describe" response - isn't that what the describe response is for?

@borkdude Yeah, totally. E.g. CIDER would take the response from this op (describe) and attach it to each connection so it'd cache the info about known ops and have a faster way of checking if some op is supported by the nREPL server or no. It would typically check first if an op's available and as a fallback here and there eval some platform-specific code. As mentioned above I'll track down all usages of "cider-tooling-eval" and try to reduce them or at the very least make them not blow up.

But perhaps in this context what you mean is that the "eval" op shouldn't be used for tooling purposes? (which is basically the crux of the ticket, at least the part "remove runtime-specific code")

[borkdude]

I would say eval should be only a last resort (eval makes the client bound to the server language/runtime). If eval is used for tooling purposes as some sort of fallback, the client should always take into account that the other side may fail executing that code (as there may be a different language/runtime than the client expects there to be).

[bbatsov]

Agreed. As an example - here's what I imagine the typical fix on CIDER's side would be like - 7fc5688

[bbatsov]

Btw, it seems that CIDER does only 4 "tooling" evals in its entire codebase currently, which means the problem is simpler to solve than I imagined it'd be. 3 of those usages are related to changing the namespace of REPL buffers and one is the lookup op fallback that we probably don't need much a this point, as I hope most nREPL servers provide it.

[borkdude]

@bbatsov Amazing! Does this mean that nbb / Hubble users can use cider-connect and or cider-connect-cljs (whichever is best) possibly?

[bbatsov]

It seems to me that cider-connect should work fine for them, as from what I got they are both native ClojureScript implementations that don't need the "upgrade magic" that's needed for the hosted ClojureScript that cider-connect-cljs was designed for.

[borkdude]

That's right.