bug#64283: 29.0.91; js-mode's mark-defun does not work correctly when functions have a comment on top

["Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Sun, 25 Jun 2023 15:38:12 +0200
>> From:  Daniel Martín via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> Daniel Martín via "Bug reports for GNU Emacs, the Swiss army knife of
>> text editors" <bug-gnu-emacs@gnu.org> writes:
>> 
>> > This is a regression from Emacs 28.2.
>> >
>> 
>> This is the first commit where this bug happens:
>> 
>> commit 4489450f37deafb013b1f0fc00c89f0973fda14a
>> Author: Yuan Fu <casouri@gmail.com>
>> Date:   Thu Nov 10 15:00:29 2022 -0800
>> 
>>     In end-of-defun, terminate early if no further defun exists
>>     
>>     Before this change, end-of-defun calls end-of-defun-function even if
>>     point is not necessarily at the beginning of a defun (contrary to what
>>     end-of-defun-function's docstring claims).  Now it terminates early
>>     and doesn't call end-of-defun-function.
>>     
>>     * lisp/emacs-lisp/lisp.el (beginning-of-defun-raw): Update docstring
>>     clarifying the return value.
>>     (end-of-defun): Terminate early if beginning-of-defun-raw returns nil.
>
> Yuan, could you please look into this?

What I see is that, after 4489450f37deafb013b1f0fc00c89f0973fda14a,
defun movement may be subtly broken if beginning-of-defun-function does
not return non-nil when it found the beginning of a defun.  One of the
affected modes is js-mode, but who knows if there are more out there.

We could either revert 4489450f37deafb013b1f0fc00c89f0973fda14a, because
of the incompatibilities it may cause (Yuan, what is the bug it tries to
fix?), or maybe adjust js-mode so that it follows the documentation of
beginning-of-defun-function and returns non-nil when it found the
beginning of a defun.  I've attached a patch that follows this second
approach, with some unit tests.  It fixes the bug on my side.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-js-beginning-of-defun-return-non-nil-on-success.patch --]
[-- Type: text/x-patch, Size: 5214 bytes --]

From dbd0a48fba90b9a7f2fb291a3d279b6adf3117f3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=ADn?= <mardani29@yahoo.es>
Date: Sun, 25 Jun 2023 22:17:14 +0200
Subject: [PATCH] Make js-beginning-of-defun return non-nil on success

* lisp/progmodes/js.el (js-beginning-of-defun): The docstring of
beginning-of-defun-function says that this function shall return
non-nil when it found the beginning of a defun.  This is specially
important because the calling code decides when to move point
depending on the return value.  Make js-beginning-of-defun return
non-nil when it found the beginning of a defun.
(js--beginning-of-defun-flat): Same for this helper.
* test/lisp/progmodes/js-tests.el (js-mode-end-of-defun): Add a unit
test.
---
 lisp/progmodes/js.el            | 61 ++++++++++++++++++---------------
 test/lisp/progmodes/js-tests.el | 51 +++++++++++++++++++++++++++
 2 files changed, 85 insertions(+), 27 deletions(-)

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index 414b6eb2baf..a05bd758dbc 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -1024,38 +1024,45 @@ js--beginning-of-defun-flat
   "Helper function for `js-beginning-of-defun'."
   (let ((pstate (js--beginning-of-defun-raw)))
     (when pstate
-      (goto-char (js--pitem-h-begin (car pstate))))))
+      (goto-char (js--pitem-h-begin (car pstate)))
+      t)))
 
 (defun js-beginning-of-defun (&optional arg)
   "Value of `beginning-of-defun-function' for `js-mode'."
   (setq arg (or arg 1))
-  (while (and (not (eobp)) (< arg 0))
-    (cl-incf arg)
-    (when (and (not js-flat-functions)
-               (or (eq (js-syntactic-context) 'function)
-                   (js--function-prologue-beginning)))
-      (js-end-of-defun))
-
-    (if (js--re-search-forward
-         "\\_<function\\_>" nil t)
-        (goto-char (js--function-prologue-beginning))
-      (goto-char (point-max))))
-
-  (while (> arg 0)
-    (cl-decf arg)
-    ;; If we're just past the end of a function, the user probably wants
-    ;; to go to the beginning of *that* function
-    (when (eq (char-before) ?})
-      (backward-char))
-
-    (let ((prologue-begin (js--function-prologue-beginning)))
-      (cond ((and prologue-begin (< prologue-begin (point)))
-             (goto-char prologue-begin))
+  (let ((found))
+    (while (and (not (eobp)) (< arg 0))
+      (cl-incf arg)
+      (when (and (not js-flat-functions)
+                 (or (eq (js-syntactic-context) 'function)
+                     (js--function-prologue-beginning)))
+        (js-end-of-defun))
+
+      (if (js--re-search-forward
+           "\\_<function\\_>" nil t)
+          (progn (goto-char (js--function-prologue-beginning))
+                 (setq found t))
+        (goto-char (point-max))
+        (setq found nil)))
+
+    (while (> arg 0)
+      (cl-decf arg)
+      ;; If we're just past the end of a function, the user probably wants
+      ;; to go to the beginning of *that* function
+      (when (eq (char-before) ?})
+        (backward-char))
 
-            (js-flat-functions
-             (js--beginning-of-defun-flat))
-            (t
-             (js--beginning-of-defun-nested))))))
+      (let ((prologue-begin (js--function-prologue-beginning)))
+        (cond ((and prologue-begin (< prologue-begin (point)))
+               (goto-char prologue-begin)
+               (setq found t))
+
+              (js-flat-functions
+               (setq found (js--beginning-of-defun-flat)))
+              (t
+               (when (js--beginning-of-defun-nested)
+                 (setq found t))))))
+    found))
 
 (defun js--flush-caches (&optional beg _ignored)
   "Flush the `js-mode' syntax cache after position BEG.
diff --git a/test/lisp/progmodes/js-tests.el b/test/lisp/progmodes/js-tests.el
index 00fa78e8891..5db92b08f8a 100644
--- a/test/lisp/progmodes/js-tests.el
+++ b/test/lisp/progmodes/js-tests.el
@@ -237,6 +237,57 @@ "jsx-unclosed-1.jsx"
 (js-deftest-indent "jsx-unclosed-2.jsx")
 (js-deftest-indent "jsx.jsx")
 
+;;;; Navigation tests.
+
+(ert-deftest js-mode-beginning-of-defun ()
+  (with-temp-buffer
+    (insert "function foo() {
+  var value = 1;
+}
+
+/** A comment. */
+function bar() {
+  var value = 1;
+}
+")
+    (js-mode)
+    ;; Move point inside `foo'.
+    (goto-char 18)
+    (beginning-of-defun)
+    (should (bobp))
+    ;; Move point between the two functions.
+    (goto-char 37)
+    (beginning-of-defun)
+    (should (bobp))
+    ;; Move point inside `bar'.
+    (goto-char 73)
+    (beginning-of-defun)
+    ;; Point should move to the beginning of `bar'.
+    (should (equal (point) 56))))
+
+(ert-deftest js-mode-end-of-defun ()
+  (with-temp-buffer
+    (insert "function foo() {
+  var value = 1;
+}
+
+/** A comment. */
+function bar() {
+  var value = 1;
+}
+")
+    (js-mode)
+    (goto-char (point-min))
+    (end-of-defun)
+    ;; end-of-defun from the beginning of the buffer should go to the
+    ;; end of `foo'.
+    (should (equal (point) 37))
+    ;; Move point to the beginning of /** A comment. */
+    (goto-char 38)
+    (end-of-defun)
+    ;; end-of-defun should move point to eob.
+    (should (eobp))))
+
 (provide 'js-tests)
 
 ;;; js-tests.el ends here
-- 
2.40.1