Incomplete specification of the info and eldoc ops

[yuhan0]

https://docs.cider.mx/cider-nrepl/nrepl-api/ops.html#info The specification for the info op only mentions the following keys in the return message:

:doc-block-tags-fragments - may be absent
:doc-first-sentence-fragments - may be absent
:doc-fragments - may be absent
:status - `done`

In reality, the frontend (Emacs / cider-doc) relies on various other compulsory (non-nil) keys in the map, such as :name and :ns.

When the info op is requested for ill-formed input (eg. a non-existent var), the cider-nrepl implementation signals this by returning :no-info in its status:

https://github.com/clojure-emacs/cider-nrepl/blob/11ae4eec39b3cffcb70088f7f153a8b8fb147e0e/src/cider/nrepl/middleware/info.clj#L129

This is also undocumented, but relied upon by the frontend to guard against parsing of an empty (malformed) var-info response.

https://github.com/clojure-emacs/cider/blob/810337cee931d9f14aa16550a74516f8337a47f3/cider-client.el#L691 (Relevant call stack: cider-doc-lookup -> cider-create-doc-buffer -> cider-var-info -> cider-sync-request:info)

Similarly, the eldoc op implicitly relies on a :no-eldoc status to signal an empty return value.

The problem occurs with the babashka.nrepl implementation of the protocol, which also claims to provide the info and eldoc ops, but sometimes returns an map missing many 'required' keys such as :name, and a done status. This results in varying degrees of breakage when CIDER is connected to a babashka-type REPL.

Clearly this would be a simple fix on the impl to return the appropriate :no-info / :no-eldoc statuses, but I thought it would be worth properly pinning down the return contracts before raising an issue there.

Otherwise given the current underspecified docs, it could technically be seen as a well-formed response that requires additional logic on the frontend(s) to guard against nil values.

Steps to reproduce (Emacs / Cider 1.15)

• Create a standalone .bb file with the contents:

  
  ;; repro.bb
  (ns repro)

(defn foo "See also: bar" [])

- cider-jack-in with the `babashka` project type (`C-u 3 C-c M-j`)

- `cider-load-buffer`, then `cider-doc` on the `foo` symbol.

- Click the xref link for `bar` in the doc buffer (a non-existent var).

- The elisp error is thrown: `Wrong type argument: stringp, nil`
(Note: `cider-doc` on a non-existent symbol is wrapped in a condition-case, and falls back to a minibuffer prompt on encountering this error)

- Also observe the broken eldoc displayed in calls to non-existent vars:
```clj
(oops  ) ; <- with cursor in form

Environment & Version information

NA

[yuhan0]

Another related error with cider-eldoc + regular JVM Clojure:

;; repro.clj
(ns repro)

(def undocumented ;; <- cursor here
  123)

• Ensure the default config of (setq cider-eldoc-display-for-symbol-at-point t)
• cider-jack-in-clj, cider-load-buffer
• Place cursor on the symbol undocumented, and observe in the echo area: eldoc error: (wrong-type-argument arrayp nil)

Here, the issue is that CIDER expects the eldoc op response to contain a string under the key :docstring, which is unspecified: https://docs.cider.mx/cider-nrepl/nrepl-api/ops.html#eldoc The error is thrown from cider-docstring--format -> replace-regexp-in-string.

Again it's unclear where the responsibility for the fix should lie - on the Elisp side of things, or having the middleware eldoc op guarantee to always return a string-valued :docstring response?

[vemv]

or having the middleware eldoc op guarantee to always return a string-valued :docstring response?

I don't think we can possibly guarantee this. What should the returned value be for a (def undocumented 123)?

Other than that, PR welcome with documentation changes!

In case you weren't familiar, the workflow is editing src/cider/nrepl.clj and then running lein docs.

[yuhan0]

Actually, the contents of the response originate from Orchard, which simply assocs :docstring = nil to the map when the var has no :doc metadata: https://github.com/clojure-emacs/orchard/blob/6e845a7fe364839e99bb4b4f59ecde5f698d0583/src/orchard/eldoc.clj#L63

Ideally, the user-facing Eldoc message should display something like "(not documented)", but would that be an upstream change in Orchard? Or simply propagate the nil and defer the presentation to the frontend? (Do we differentiate between missing vs. empty-string :doc meta?)

(Also, this was extra confusing because I was under the impression that Bencode had no way to encode nils and instead uses empty strings or surrogates like "-1" to pun for a missing value. But it turns out that Clojure nils are simply sent over the wire as empty lists :/ This probably deserves to be a separate issue but I couldn't figure out which repo it belonged to 👀)

[vemv]

"(not documented)"

Not sure if this would be pretty or desirable, from a user perspective.

For clarity, can't we just deal with the empty list Elisp side?

[yuhan0]

can't we just deal with the empty list Elisp side?

That's the bit I was unsure about, relying on the fact that Clojure nil -> Bencode list -> Emacs nil.

Going by the comment here by @bbatsov https://github.com/clojure-emacs/cider/discussions/3711#discussioncomment-9720560

Indeed. Bencode has no representation for nil.

Are there other places in the code that implicitly rely on this sort of empty-list = nil punning? I see a lot of empty-string punning around Cider, ie. checks like (string= var "").

For example the ns-path op returns empty strings when given 'bad' inputs:

(with-current-buffer "repro.clj"
  (cider-nrepl-send-sync-request '("op" "ns-path" "ns" "clorjue.core")))
;; =>
(dict "status" ("done")
      "id" "22"
      "session" "1a73fedf-5506-4623-9ec4-3b93b199f668" 
      "path" ""
      "url" "")

But I don't have a broad enough overview of the shapes of data flowing back and forth through all these various ops, and it doesn't help that the docs are incomplete and vague about input / return types.

[vemv]

That's the bit I was unsure about

Personally I'd advocate for omitting the inclusion of keys that cannot have a meaningul value and return err keys instead.

Regardless of bencode intricacies, it feels generally cleaner.

In many codebases, return types look like this:

{age: Int} | {error: String}

...but one doesn't intentionally include an age of -1 to signal an error, or accidentally include it while :error is also present.

Elisp side, this seems easy to handle - if an expected key is absent or if an err key is present, one handles the error. That's two ways to signal errors, non exclusive. So plenty of chances for consumers to realise what's wrong!

Not sure of @bbatsov agrees with me.