cider--goto-expression-start: Do not throw if bop

[benjamin-asdf]

There is currently an edge case when you interactively eval in a source file that has no ns form (and no list form at all).
And you eval something that throws an error. Then cider--find-last-error-location will try to find a list in line 0 at col 0 and throw an error.
Edge case but not nice because you get an "error in process filter" which are slightly harder to figure out.
With this change cider--goto-expression-start will return nil instead of throwing. There is only 1 usage so no other semantic is affected.
cider--find-last-error-location will usually end up returning a list of (point-min) (point-min) , then so nothing is colored.

[bbatsov]

It'd be good to cover this behavior with some tests, as even I'm struggling a bit to understand what exactly is the edge case we're tackling here.

[bbatsov]

ping :-)

[benjamin-asdf]

I added a test that fails with the old version:

cider--goto-expression-start
  Does not throw an error, when file does not contain a list  FAILED (0.09ms)

========================================
cider--goto-expression-start Does not throw an error, when file does not contain a list

...
error: (beginning-of-buffer)

[benjamin-asdf]

I wanted to make a test for cider--find-last-error-location but would have to be an integration test because it uses a subroutine that only works with a cider project.

[benjamin-asdf]

spefically, cider-find-file only works with an active connection or something I think.

[bbatsov]

Thinking a bit more about the issue it seems to me the right place for this check is not the while loop, but rather the first if or even an other if. E.g. if you're at the beginning of the buffer there's no point to look for the start of an expression as you're already there.

[bbatsov]

It'd be good to add a few tests that also cover the normal behavior of this function.

[bbatsov]

I also wonder why this check didn't get triggered in your example with (insert foo). At the beginning of the expression the code should not try to go backward looking for the beginning of the expression.

[benjamin-asdf]

It short circuits to backward-sexp if we are are behind a closing paren. In the example there is only "foo" so no parens.

[benjamin-asdf]

Maybe this version makes the intent clearer:

      
      
(defun cider--goto-next-expression-start ()
  "Find the next opening paren that is not part of a string.
Go to end of buffer, if none is found."
  (while (and (not (eobp))
              (or (not (looking-at-p "[({[]"))
                  ;; TODO this jumps to comments right now
                  (nth 3 (syntax-ppss))))
    (forward-char)))

(defun cider--goto-expression-start ()
  "Go to the beginning a list, vector, map ( or) set outside of a string.
We do so by starting and the current position and proceeding backwards
until we find a delimiters that's not inside a string."
  (cond
   ;; if there is no list form at all, we don't go anywhere
   ((save-excursion
      (goto-char (point-min))
      (cider--goto-next-expression-start)
      (eobp))
    (point))
   ((and (looking-back "[])}]" (line-beginning-position))
         (null (nth 3 (syntax-ppss))))
    (backward-sexp))
   (t
    (while (or
            (not (looking-at-p "[({[]"))
            (nth 3 (syntax-ppss)))
      (backward-char)))))
          

to me it would also make sense to go backwards to the beginning of the buffer, when there is no expression.

[bbatsov]

I'm on the fence about the second version, as then I'd probably need extract a function cider--goto-prev-expression-start as well, plus I'm not sure how much overhead the checking for any expression will add. Maybe it's best to stick to the minimal initial proposal or alternatively just catch this error when it happens.

to me it would also make sense to go backwards to the beginning of the buffer, when there is no expression.

I think it'd be preferable not to move the point in such cases.