Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

benjamin-asdf
Copy link
Contributor

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
Copy link
Member

bbatsov commented Jan 17, 2023

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
Copy link
Member

bbatsov commented Feb 1, 2023

ping :-)

@benjamin-asdf
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@@ -602,8 +602,10 @@ until we find a delimiters that's not inside a string."
(if (and (looking-back "[])}]" (line-beginning-position))
(null (nth 3 (syntax-ppss))))
(backward-sexp)
(while (or (not (looking-at-p "[({[]"))
(nth 3 (syntax-ppss)))
(while (and (not (bobp))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -47,4 +47,23 @@
(insert "🍻"))
(expect (cider-provide-file filename) :to-equal "8J+Nuw=="))))

(describe
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(nth 3 (syntax-ppss)))
(while (and (not (bobp))
(or
(not (looking-at-p "[({[]"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@benjamin-asdf benjamin-asdf Feb 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

benjamin-asdf commented Feb 3, 2023

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
Copy link
Member

bbatsov commented Feb 9, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants