nrepl icon indicating copy to clipboard operation
nrepl copied to clipboard

Extend the "describe" op with some extra runtime information

Open bbatsov opened this issue 5 years ago • 24 comments
trafficstars

Is your feature request related to a problem? Please describe.

Today it's hard to tell what's the runtime of the server you're connecting to (e.g. Clojure, ClojureScript, ClojureCLR, babashka, or some non-Clojure language like Racket, fennel, etc.).

One practical problem is that client often implement some functionality in terms of evaluating runtime-specific code, which often means that one client can't be used with different nREPL servers simply because it makes some assumptions that are not universally true.

If clients could easily infer that runtime of the server they can support easily different runtimes.

Describe the solution you'd like

I think we should agree that every nREPL server should return some extra metadata in the describe op:

  • runtime/language (e.g. Clojure)
  • protocol version (tracking the version number of the reference nREPL implementation) - not sure how useful this can be, but it won't hurt. I guess potentially this can be nrepl-version.
  • name of the server implementation (e.g. Ogion)
  • version of the server implementation (this should be different from nrepl-version I guess)

E.g. you can have something like:

{:runtime "racket"
  :protocol-version "0.7"
  :implementation-name "ogion"
  :implementation-version "0.1.0"}

Describe alternatives you've considered

I didn't manage to think of a good alternative.

@nrepl/nrepl @technomancy @borkdude setzer22 Let me know how this sounds to you.

bbatsov avatar Apr 05 '20 10:04 bbatsov

In the case of babashka, should the runtime be "clojure" or "babashka"? Babashka mimics JVM Clojure as close as possible, but some things aren't supported like defprotocol. I'm not sure what to put as the nREPL protocol version. I just use bencode and reverse engineered what was necessary to make CIDER's cider-connect and lein repl :connect work.

{:runtime "babashka" ;; or "clojure" ?
  :protocol-version "??"
  :implementation-name "??"
  :implementation-version "??"}

borkdude avatar Apr 05 '20 10:04 borkdude

I like this idea, except I really don't think "protocol-version" should be included at this point.

Conflating the protocol with the reference implementation is a very bad idea. If we come up with an independent document describing the protocol on its own and version that separately, then it could make sense to include this in the description, but I'm not aware of any such thing at this time.

technomancy avatar Apr 05 '20 20:04 technomancy

I finally started working on this today, so I'll have some update soon. Now I'm leaning towards adding a runtime and name/server (short for "implementation/server name") key at the top-level of the describe map. Servers will still put their relevant versions in the versions map, but clients would know what to look for based on the runtime/server keys (e.g. if runtime is clojure/babashka they'll look for clojure/babashka and java version info). If in the case of fennel they'll look for fennel and lua version info and so on.

@borkdude In your case runtime should be "babaska", "name" whatever you want (e.g. nrepl-babashka) and same with the version:

{:runtime "babashka"
  :name "nrepl-babashka"
  :versions { :babashka "..."
                     :java "..."
                     :nrepl-babashka "..."}}

Let me know how this sounds to you and whether you have some preferences about the names of the extra keys.

bbatsov avatar Jun 02 '20 08:06 bbatsov

Btw, I guess for babashka the runtime can easily be clojure as well, given how highly compatible they are. That's not a big deal in the end of the day I think.

bbatsov avatar Jun 02 '20 08:06 bbatsov

Having a separate version / java from version / babashka doesn't make sense, since you cannot have different Java/GraalVM versions with the same babashka version, i.e it's a derived property. From 0.1.0 on babashka is compiled with GraalVM java11 (right now GraalVM version 20.1.0).

I think the field "name" could have a more descriptive name. What is this going to be used for? In the case of babashka there is only one possibility: [babashka.nrepl]

To summarize, I think for babashka this makes sense:

{:runtime "babashka"
 :versions {:babashka "0.1.0"}}

and that's about it?

borkdude avatar Jun 02 '20 09:06 borkdude

Having a separate version / java from version / babashka doesn't make sense, since you cannot have different Java/GraalVM versions with the same babashka version, i.e it's a derived property. From 0.1.0 on babashka is compiled with GraalVM java11 (right now GraalVM version 20.1.0).

Got it. Well, one less version then.

I think the field "name" could have a more descriptive name. What is this going to be used for? In the case of babashka there is only one possibility: [babashka.nrepl]

For most runtimes there's just one server currently, so potentially we can skip this. There's no guarantee there won't be more servers down the road, but there's also no real need for premature optimization at this point.

and that's about it?

Fine by me.

bbatsov avatar Jun 02 '20 09:06 bbatsov

Btw, doesn't your nREPL server have different versioning from babashka - I guess it still might be nice to include its version so that clients could display it.

bbatsov avatar Jun 02 '20 09:06 bbatsov

@bbatsov True that. We could add :babashka.nrepl "0.0.4" to versions? https://github.com/babashka/babashka.nrepl/blob/c3bfaff12c84d269e0d1c6110c290d5c5a08e002/project.clj#L1

borkdude avatar Jun 02 '20 09:06 borkdude

We have a deal! 🤝

bbatsov avatar Jun 02 '20 09:06 bbatsov

@bbatsov There are other projects that use babashka.nrepl as their nREPL server. Their runtime, like babashka, is based on the sci interpreter. I made a proposal here for these projects:

https://github.com/babashka/babashka.nrepl/issues/17

Is this information going to be used by CIDER? Would it be useful to have one common key so CIDER doesn't have to add cases for each new sci-based project? It's not always clear what a sci-based project will or won't support though, since sci is just an interpreter. What it can provide depends on how people use the interpreter. So it's likely that an nREPL client will still have to try things with resolve etc. to determine what the nREPL server can do.

borkdude avatar Jun 02 '20 09:06 borkdude

Is this information going to be used by CIDER?

Yeah, it will be. The plan is to have CIDER support better non-Clojure nREPL servers See https://github.com/clojure-emacs/cider/issues/2848 for more details. As an immediate step for babashka I wanted to make some changes that would eliminate the warnings they are getting from CIDER today. Unlike a language like Fennel it doesn't really require much internal changes, as most of the Clojure code that CIDER runs will work just fine.

So it's likely that an nREPL client will still have to try things with resolve etc. to determine what the nREPL server can do.

Well, if the runtime is not Clojure-like we can't even try resolve. :-) That's why the runtime info is the most important one.

bbatsov avatar Jun 02 '20 09:06 bbatsov

@bbatsov If that's the goal, maybe for runtime it's better to use some namespaced thing, so the project can describe it's some flavor of Clojure?

:runtime "clojure/babashka"
:runtime "clojure/bootleg"
:runtime "clojure/clojurescript"

So then CIDER knows by the namespace of the key that it can use require?

Or a separate :language key that says "clojure".

borkdude avatar Jun 02 '20 09:06 borkdude

I've been following this conversation with some trepidation, as it reminds me of how the web has struggled to improve because of everyone doing user agent checks. Instead of checking for implementation/version it would be more robust and forward thinking to announce and check for specific capabilities...

plexus avatar Jun 02 '20 09:06 plexus

@plexus Good point. That's exactly why I have avoided communicating a pod version in babashka.pods but so far have only communicated capabilities using the :ops op which is also part of nREPL.

Stating a :language "clojure" describes it's capable of something although you still don't know if the implementation of clojure is complete.

Btw, this was weird:

Screenshot 2020-06-02 11 36 11

borkdude avatar Jun 02 '20 09:06 borkdude

Yeah, maybe we should have a separate key about the language, as with ClojureScript you can have both a Java runtime and a self-hosted JavaScript runtime and you need to be able to differentiate between the two. On the other hand from what I gather some servers like Ogion can handled two languages (Lua and Fennel), even if the server is implemented in Lua and obviously Lua is the actual runtime (as it's Java for regular Clojure). With hosted languages we have two layers of runtime which makes things a bit more complicated.

bbatsov avatar Jun 02 '20 09:06 bbatsov

I've been following this conversation with some trepidation, as it reminds me of how the web has struggled to improve because of everyone doing user agent checks. Instead of checking for implementation/version it would be more robust and forward thinking to announce and check for specific capabilities...

The problem is that many clients patch some missing capabilities by evaluating code and they need to know what exactly to evaluate. If we remove the evaluations from the equation we don't really care about the runtime. If we need to auto-detect a connection type some extra info would be useful as well. But now I'm wondering if I'm not approaching this from the wrong angle. :-)

Stating a :language "clojure" describes it's capable of something although you still don't know if the implementation of clojure is complete.

Yeah, this might work as well.

bbatsov avatar Jun 02 '20 09:06 bbatsov

But now I'm wondering if I'm not approaching this from the wrong angle. :-)

Maybe good to take a step back. Start with a clear problem statement. Then explore alternative solutions. Then choose one minimal solution that could be extended later if necessary.

To restate the problem: CIDER (or other editor) is missing information about the nREPL server. It could try resolve to gain this information, but resolve might not even work. Is that a problem? If the nREPL server sends back an exception on resolve, why not try the next thing to try if the server speaks Fennel or some other language? That might be painful. To relieve this pain, only adding a :language key might be sufficient for now, although there is no guarantee that if an nREPL server says it speaks clojure, that it also supports resolve? At least it narrows down the amount of things to try next. What are the alternatives?

borkdude avatar Jun 02 '20 10:06 borkdude

  1. The target language gets set manually in clients (e.g. on connect)
  2. Clients restrain themselves from implementing any functionality in terms of eval and just rely on nREPL ops for everything

Probably there are other options, but those two are the obvious ones.

bbatsov avatar Jun 02 '20 18:06 bbatsov

Can you elaborate on 2? Not sure I understand.

borkdude avatar Jun 02 '20 18:06 borkdude

E.g. you want to provide code completion by just evaluating code in some code completion library (as opposed to a dedicated op) - that's how Leiningen currently provides code completion in its REPL. That's a common approach many people take, as it's the simplest thing to do in many cases and it's the main issue when it comes to clients portability.

bbatsov avatar Jun 02 '20 18:06 bbatsov

Btw, there's one more nuance we have to keep in mind - how do you probe for capabilities that are not actually ops (e.g. the print middleware doesn't expose any ops, as it just modifies requests and responses). We can have them somewhere in describe I guess, e.g. under some capabilities key or something.

bbatsov avatar Jun 03 '20 08:06 bbatsov

Seems the conversation here got stalled, so I won't change anything for time being. @borkdude can just add this to babashka.nrepl:

:versions {:babashka "0.1.0"
                  :babashka.nrepl "..."}

So I can teach CIDER about it. I played a bit with babashka and CIDER today and all the nil versions and warnings on startup are something I'd like to fix.

bbatsov avatar Jun 12 '20 09:06 bbatsov

Will do!

borkdude avatar Jun 12 '20 09:06 borkdude

Clients restrain themselves from implementing any functionality in terms of eval and just rely on nREPL ops for everything

For the record I'm 100% in favor of this; at the very least implementing functionality using eval should be considered a measure of last resort that you fall back to only once you determine that the op you need is not present.

technomancy avatar Jun 12 '20 16:06 technomancy