cider-eval-toplevel-inside-comment-form breaks autocomplete inside comment form

[dpsutton]

Example:

(defn thing [])

(comment
  (th|)
  )

if point is at | you would expect the autocompletion to suggest thing. And it does if cider-eval-toplevel-inside-comment-form is nil. But if this is truthy you are greeted with the following message:

Company: backend company-capf error "Args out of range: "(th)", 0, 12" with args (candidates th)

The problem is in cider-completion-get-context-at-point which gets the context for the completion which includes in relevant part:

      (let* ((pref-end (point))
             (pref-start (cider-completion-symbol-start-pos))
             (context (cider-defun-at-point))  ;; aware of comment forms
             (_ (beginning-of-defun))          ;; unconditionally goes to top level
             (expr-start (point)))
        (concat (when pref-start (substring context 0 (- pref-start expr-start)))
                "__prefix__"
                (substring context (- pref-end expr-start))))

The problem is that it uses cider-defun-at-point and then beginning-of-defun. The former is aware that top level may be one level deep inside of a comment form, the later goes to the top level open parens no questions asked. This breaks assumptions about the substring it will get.

A simple fix is to only use "beginning of defun" aware functions:

      (let* ((pref-end (point))
             (pref-start (cider-completion-symbol-start-pos))
             (context-bounds (cider-defun-at-point t))    ;; get beginning and ending char positions
             (context (apply #'buffer-substring-no-properties context-bounds))
             (expr-start (car context-bounds)))
        (concat (when pref-start (substring context 0 (- pref-start expr-start)))
                "__prefix__"
                (substring context (- pref-end expr-start))))

But if we wish to keep the top-level-defun inside of comment forms feature, we need to eradicate all usages of beginning-of-defun in the codebase since it is not aware that top level has a bit more logic to it. I count 11 non-comment usages of this. It should be mechanical but I didn't realize this feature had this price originally.

I think it is fine and we should change (beginning-of-defun) to some type of cider-beginning-of-defun but I just wanted to make sure this was acceptable beforehand.

[bbatsov]

That's a bit more nuanced than this:

• beginning-of-defun is also used in clojure-mode, so a custom replacement should likely live there
• changing this might break some third-party modes relying on beginning-of-defun
• is end-of-defun affected as well

Overall that's just another manifestation of the big (and old) problem that clojure-mode.el inherits a lot of things from lisp-mode, instead of having it's own implementation of them. That made the initial implementation easier, but also inflexible. One day we should have a clojure- version of all sexp navigation functions.

[dpsutton]

@bbatsov thanks for the thoughts. if this notion of top level defun inside of a comment form should live on it should definitely go inside clojure mode. But holding onto this feature kinda obligates it. I wasn't sure of your thoughts on whether this more complicated notion of top-level defun was something you wanted to take on.

Alternatively, we could wait and see where these issues crop up. This one is easily solves as well as just dynamically binding the var to nil so the completion context is the entire comment form which should be fine.

[bbatsov]

Btw, turns out there's a much simpler fix. I just remembered that major modes can override this the internal of the beginning-of-defun and end-of-defun. So, we just need to move this defcustom together with a more flexible definition of beginning and end of defun to clojure-mode and we'll all set. That will fix the problem and not affect any 3rd party packages.

See the definition of cider-repl-mode and you'll understand what I'm talking about.

[dpsutton]

Great insight! I'll get to that early next week unless someone wants to tackle it before then.