Understanding end-of-defun

Understanding end-of-defun

[Yuan Fu]

I’m looking at end-of-defun’s definition, and these two lines looks weird:

Line 570 in lisp.el:

(unless (zerop arg)
          (beginning-of-defun-raw (- arg))
          (funcall end-of-defun-function))

Presumably this makes us go to the next arg’h begining-of-defun, and goes to the end of that defun. However, what if beginning-of-defun-raw couldn’t find any defun beyond point, didn’t move point, and returned nil? At that point calling end-of-defun-function breaks the assumption that we only call it when point is at the beginning of a defun. Am I missing something?

Yuan

Re: Understanding end-of-defun

[Stefan Monnier]

> Presumably this makes us go to the next arg’h begining-of-defun, and goes to
> the end of that defun. However, what if beginning-of-defun-raw couldn’t find
> any defun beyond point, didn’t move point, and returned nil? At that point
> calling end-of-defun-function breaks the assumption that we only call it
> when point is at the beginning of a defun. Am I missing something?

No, you're quite right.  We should double check that
`beginning-of-defun-raw` was successful (and do something else if not).


        Stefan

Re: Understanding end-of-defun

[Yuan Fu]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]



> On Oct 25, 2022, at 7:55 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>> Presumably this makes us go to the next arg’h begining-of-defun, and goes to
>> the end of that defun. However, what if beginning-of-defun-raw couldn’t find
>> any defun beyond point, didn’t move point, and returned nil? At that point
>> calling end-of-defun-function breaks the assumption that we only call it
>> when point is at the beginning of a defun. Am I missing something?
> 
> No, you're quite right.  We should double check that
> `beginning-of-defun-raw` was successful (and do something else if not).

How about:

Yuan




[-- Attachment #2: end-of-defun.diff --]
[-- Type: application/octet-stream, Size: 2167 bytes --]

diff --git a/lisp/emacs-lisp/lisp.el b/lisp/emacs-lisp/lisp.el
index acae1a0b0a9..edc3a306815 100644
--- a/lisp/emacs-lisp/lisp.el
+++ b/lisp/emacs-lisp/lisp.el
@@ -543,6 +543,7 @@ end-of-defun
         (push-mark))
     (if (or (null arg) (= arg 0)) (setq arg 1))
     (let ((pos (point))
+          (success nil)
           (beg (progn (when end-of-defun-moves-to-eol
                         (end-of-line 1))
                       (beginning-of-defun-raw 1) (point)))
@@ -567,9 +568,12 @@ end-of-defun
             (setq arg (1- arg))
           ;; We started from after the end of the previous function.
           (goto-char pos))
+        ;; At this point, point either didn't move (because we started
+        ;; in between two defun's), or is at the end of a defun
+        ;; (because we started in the middle of a defun).
         (unless (zerop arg)
-          (beginning-of-defun-raw (- arg))
-          (funcall end-of-defun-function)))
+          (when (setq success (beginning-of-defun-raw (- arg)))
+            (funcall end-of-defun-function))))
        ((< arg 0)
         ;; Moving backward.
         (if (< (point) pos)
@@ -579,16 +583,18 @@ end-of-defun
           ;; We started from inside a function.
           (goto-char beg))
         (unless (zerop arg)
-          (beginning-of-defun-raw (- arg))
-	  (setq beg (point))
-          (funcall end-of-defun-function))))
+          (when (setq success (beginning-of-defun-raw (- arg)))
+            (setq beg (point))
+            (funcall end-of-defun-function)))))
       (funcall skip)
-      (while (and (< arg 0) (>= (point) pos))
+      (while (and (< arg 0) (>= (point) pos) success)
         ;; We intended to move backward, but this ended up not doing so:
         ;; Try harder!
         (goto-char beg)
-        (beginning-of-defun-raw (- arg))
-        (if (>= (point) beg)
+        (setq success (beginning-of-defun-raw (- arg)))
+        ;; If we successfully moved pass point, or there is no further
+        ;; defun beginnings anymore, stop.
+        (if (or (>= (point) beg) (not success))
 	    (setq arg 0)
 	  (setq beg (point))
           (funcall end-of-defun-function)

Re: Understanding end-of-defun

[Stefan Monnier]

>> No, you're quite right.  We should double check that
>> `beginning-of-defun-raw` was successful (and do something else if not).
> How about:

I haven't had a chance to try it out, but it looks good to me, tho
`beginning-of-defun-raw` doesn't mention anything about its return value
in the its docstring, so we may want to tweak that docstring (and
review its code to make sure that it indeed returns the value we need
in all cases, tho I suspect you've done so already).


        Stefan

Re: Understanding end-of-defun

[Yuan Fu]

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]



> On Oct 27, 2022, at 8:25 AM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>>> No, you're quite right.  We should double check that
>>> `beginning-of-defun-raw` was successful (and do something else if not).
>> How about:
> 
> I haven't had a chance to try it out, but it looks good to me, tho
> `beginning-of-defun-raw` doesn't mention anything about its return value
> in the its docstring, so we may want to tweak that docstring (and
> review its code to make sure that it indeed returns the value we need
> in all cases, tho I suspect you've done so already).

(Sorry for the delay) Yes, it returns nil in all cases. Pretty sure that’s the intent from the very beginning (looking at git logs). How about this patch?

Yuan


[-- Attachment #2: end-of-defun.diff --]
[-- Type: application/octet-stream, Size: 2683 bytes --]

diff --git a/lisp/emacs-lisp/lisp.el b/lisp/emacs-lisp/lisp.el
index acae1a0b0a9..c8d05a084bb 100644
--- a/lisp/emacs-lisp/lisp.el
+++ b/lisp/emacs-lisp/lisp.el
@@ -375,7 +375,10 @@ beginning-of-defun-raw
 is non-nil.
 
 If variable `beginning-of-defun-function' is non-nil, its value
-is called as a function to find the defun's beginning."
+is called as a function to find the defun's beginning.
+
+Return non-nil if this function successfully found the beginning
+of a defun, nil if it failed to find one."
   (interactive "^p")   ; change this to "P", maybe, if we ever come to pass ARG
                       ; to beginning-of-defun-function.
   (unless arg (setq arg 1))
@@ -543,6 +546,7 @@ end-of-defun
         (push-mark))
     (if (or (null arg) (= arg 0)) (setq arg 1))
     (let ((pos (point))
+          (success nil)
           (beg (progn (when end-of-defun-moves-to-eol
                         (end-of-line 1))
                       (beginning-of-defun-raw 1) (point)))
@@ -567,9 +571,12 @@ end-of-defun
             (setq arg (1- arg))
           ;; We started from after the end of the previous function.
           (goto-char pos))
+        ;; At this point, point either didn't move (because we started
+        ;; in between two defun's), or is at the end of a defun
+        ;; (because we started in the middle of a defun).
         (unless (zerop arg)
-          (beginning-of-defun-raw (- arg))
-          (funcall end-of-defun-function)))
+          (when (setq success (beginning-of-defun-raw (- arg)))
+            (funcall end-of-defun-function))))
        ((< arg 0)
         ;; Moving backward.
         (if (< (point) pos)
@@ -579,16 +586,18 @@ end-of-defun
           ;; We started from inside a function.
           (goto-char beg))
         (unless (zerop arg)
-          (beginning-of-defun-raw (- arg))
-	  (setq beg (point))
-          (funcall end-of-defun-function))))
+          (when (setq success (beginning-of-defun-raw (- arg)))
+            (setq beg (point))
+            (funcall end-of-defun-function)))))
       (funcall skip)
-      (while (and (< arg 0) (>= (point) pos))
+      (while (and (< arg 0) (>= (point) pos) success)
         ;; We intended to move backward, but this ended up not doing so:
         ;; Try harder!
         (goto-char beg)
-        (beginning-of-defun-raw (- arg))
-        (if (>= (point) beg)
+        (setq success (beginning-of-defun-raw (- arg)))
+        ;; If we successfully moved pass point, or there is no further
+        ;; defun beginnings anymore, stop.
+        (if (or (>= (point) beg) (not success))
 	    (setq arg 0)
 	  (setq beg (point))
           (funcall end-of-defun-function)

Re: Understanding end-of-defun

[Stefan Monnier]

>> I haven't had a chance to try it out, but it looks good to me, tho
>> `beginning-of-defun-raw` doesn't mention anything about its return value
>> in the its docstring, so we may want to tweak that docstring (and
>> review its code to make sure that it indeed returns the value we need
>> in all cases, tho I suspect you've done so already).
>
> (Sorry for the delay) Yes, it returns nil in all cases. Pretty sure that’s
> the intent from the very beginning (looking at git logs). How about
> this patch?

LGTM, thank you!


        Stefan


PS: I'm very happy to work with you: I just babble about and you
    go and do the grunt work.  Half kidding, obviously, but really,
    I appreciate it, thank you.

Re: Understanding end-of-defun

[Yuan Fu]

> On Nov 10, 2022, at 2:49 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
>>> I haven't had a chance to try it out, but it looks good to me, tho
>>> `beginning-of-defun-raw` doesn't mention anything about its return value
>>> in the its docstring, so we may want to tweak that docstring (and
>>> review its code to make sure that it indeed returns the value we need
>>> in all cases, tho I suspect you've done so already).
>> 
>> (Sorry for the delay) Yes, it returns nil in all cases. Pretty sure that’s
>> the intent from the very beginning (looking at git logs). How about
>> this patch?
> 
> LGTM, thank you!

I pushed this to feature/tree-sitter. I looked at tests for end-of-defun, the only one I found runs fine. I could add a test for this change but I don’t know what we’ll test for.

> PS: I'm very happy to work with you: I just babble about and you
>    go and do the grunt work.  Half kidding, obviously, but really,
>    I appreciate it, thank you.
> 

Oh thank you! That made my day :-)

Yuan