bug#66218: 29.1.50; `beginning-of-defun' jumps to wrong position in `emacs-lisp-mode'

bug#66218: 29.1.50; `beginning-of-defun' jumps to wrong position in `emacs-lisp-mode'

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Function `beginning-of-defun-raw' uses `syntax-pss' at [1] to
verify a potential BOF in this snippet:

------------------------- snip -------------------------
    (and (let (found)
           (while
               (and (setq found
                          (re-search-backward
                           (if defun-prompt-regexp
			       (concat (if open-paren-in-column-0-is-defun-start
					   "^\\s(\\|" "")
				       "\\(?:" defun-prompt-regexp "\\)\\s(")
			     "^\\s(")
			                      nil 'move arg))
                    (nth 8 (syntax-ppss))))
                           ^^^^^^^^^^^^^ [1]
           found)
	 (progn (goto-char (1- (match-end 0)))
                               ^^^^^^^^^^^^^ [2]
                t)))
------------------------- snip -------------------------

However, if `syntax-pss' calls a `syntax-propertize-function', the latter
can overwrite match data, which is referenced at [2] above.

Here is an "emacs -Q" reproducer that shows the problem (CHANGES
REQUIRED):

------------------------- snip -------------------------
(progn
  ;; CHANGE FILE NAME AS NEEDED.  It is important that the file
  ;; is found first in this test case, or otherwise syntax
  ;; properties may already have been populated.
  (ignore-errors (kill-buffer "shr.el"))
  (with-current-buffer
      (find-file-noselect "~/work/emacs-29/lisp/net/shr.el")
    ;; CHANGE POSITION AS NEEDED, should be line 2044, at BOL
    ;; before the first `cond' in `shr-table-body'
    (goto-char 70719)
    (beginning-of-defun)
    ;; point should be at BOF of `shr-table-body', but is in fact
    ;; somewhere near a Unicode character name
    (pop-to-buffer (current-buffer))))
------------------------- snip -------------------------

Patch attached.  Please let me know if you think that ERTs or
anything else is requried.

In GNU Emacs 29.1.50 (build 4, x86_64-pc-linux-gnu, GTK+ Version
 3.24.24, cairo version 1.16.0) of 2023-09-25 built on sappc2
Repository revision: ca5b48fd76db2ab7c154c2db6a7d4a9fb7857f6c
Repository branch: beginning-of-defun-save-match-data
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

Configured using:
 'configure -C --with-native-compilation'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF
TOOLKIT_SCROLL_BARS X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

Important settings:
  value of $LC_COLLATE: POSIX
  value of $LC_TIME: POSIX
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils comp comp-cstr
warnings icons subr-x rx cl-seq cl-macs gv cl-extra help-mode
cl-loaddefs cl-lib bytecomp byte-compile rmc iso-transl tooltip cconv
eldoc paren electric uniquify ediff-hook vc-hooks lisp-float-type
elisp-mode mwheel term/x-win x-win term/common-win x-dnd tool-bar dnd
fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow
isearch easymenu timer select scroll-bar mouse jit-lock font-lock syntax
font-core term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit xinput2 x multi-tty make-network-process
native-compile emacs)

Memory information:
((conses 16 93935 9790)
 (symbols 48 7139 0)
 (strings 32 26103 2634)
 (string-bytes 1 831368)
 (vectors 16 20039)
 (vector-slots 8 365879 14596)
 (floats 8 41 21)
 (intervals 56 237 0)
 (buffers 976 12))

[-- Attachment #2: 0001-Fix-beginning-of-defun-not-jumping-to-BOF.patch --]
[-- Type: text/x-patch, Size: 945 bytes --]

From cf593fad5b1bc9140203563eac8ddf0d4603fccb Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Tue, 26 Sep 2023 21:36:19 +0200
Subject: [PATCH] Fix beginning-of-defun not jumping to BOF

* lisp/emacs-lisp/lisp.el (beginning-of-defun-raw): Save match data
around a call to `syntax-ppss'.
---
 lisp/emacs-lisp/lisp.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/lisp.el b/lisp/emacs-lisp/lisp.el
index 17d58b1e3c6..b49850e4ef0 100644
--- a/lisp/emacs-lisp/lisp.el
+++ b/lisp/emacs-lisp/lisp.el
@@ -422,7 +422,8 @@ beginning-of-defun-raw
 				       "\\(?:" defun-prompt-regexp "\\)\\s(")
 			     "^\\s(")
 			                      nil 'move arg))
-                    (nth 8 (syntax-ppss))))
+                    (save-match-data
+                      (nth 8 (syntax-ppss)))))
            found)
 	 (progn (goto-char (1- (match-end 0)))
                 t)))
-- 
2.30.2

bug#66218: 29.1.50; `beginning-of-defun' jumps to wrong position in `emacs-lisp-mode'

[No Wayman]

Possibly related to bug#60768?

bug#66218: 29.1.50; `beginning-of-defun' jumps to wrong position in `emacs-lisp-mode'

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

No Wayman <iarchivedmywholelife@gmail.com> writes:

> Possibly related to bug#60768?

Actually, yes: With above patch your bug seems fixed.  The preconditions
for this bug are also met by your bug: Syntax properties previously not
loaded plus the substring "###" which triggers overwriting of the match
data.

bug#66218: 29.1.50; `beginning-of-defun' jumps to wrong position in `emacs-lisp-mode'

[Stefan Kangas]

forcemerge 60768 66218
tags 60768 + patch
thanks

Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:

> No Wayman <iarchivedmywholelife@gmail.com> writes:
>
>> Possibly related to bug#60768?
>
> Actually, yes: With above patch your bug seems fixed.  The preconditions
> for this bug are also met by your bug: Syntax properties previously not
> loaded plus the substring "###" which triggers overwriting of the match
> data.

Thanks, I'm therefore merging those bugs.

bug#66218: 29.1.50; `beginning-of-defun' jumps to wrong position in `emacs-lisp-mode'

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:

> However, if `syntax-pss' calls a `syntax-propertize-function', the latter
> can overwrite match data, which is referenced at [2] above.

> Patch attached.

This seems to be such a low-hanging fruit, so:

Bump.

Eli, could you please review and commit to emacs-29?

Thanks.

bug#66218: 29.1.50; `beginning-of-defun' jumps to wrong position in `emacs-lisp-mode'

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Fri, 06 Oct 2023 21:26:24 +0200
> 
> Jens Schmidt <jschmidt4gnu@vodafonemail.de> writes:
> 
> > However, if `syntax-pss' calls a `syntax-propertize-function', the latter
> > can overwrite match data, which is referenced at [2] above.
> 
> > Patch attached.
> 
> This seems to be such a low-hanging fruit, so:
> 
> Bump.
> 
> Eli, could you please review and commit to emacs-29?

I'm afraid I cannot reproduce the original problem using the current
emacs-29 branch.  Typing C-M-a from buffer position 70719 in shr.el
from emacs-29 lands me at the beginning of shr-table-body, as
expected.

bug#66218: 29.1.50; `beginning-of-defun' jumps to wrong position in `emacs-lisp-mode'

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
>> Cc: Eli Zaretskii <eliz@gnu.org>
>> Date: Fri, 06 Oct 2023 21:26:24 +0200
>
> I'm afraid I cannot reproduce the original problem using the current
> emacs-29 branch.  Typing C-M-a from buffer position 70719 in shr.el
> from emacs-29 lands me at the beginning of shr-table-body, as
> expected.

Thanks for looking into this, but it still reproduces for me.

It is important to execute the reproducer *without* actually getting the
file to the glass, so hitting C-M-a in the file itself would not show
the issue.  Please try exactly like described below, starting off the
root of the development directory:

------------------------- snip -------------------------
[emacs-29]$ pwd
/home/jschmidt/work/emacs-29
[emacs-29]$ ls -al README
-rw-r--r-- 1 jschmidt jschmidt 6125 Aug  1 00:06 README
[emacs-29]$ git rev-parse HEAD
8f23a02a9ea1fbc4213cae5664dcb9bf6b5205f6
[emacs-29]$ make -j8
make actual-all || make advice-on-failure make-target=all exit-status=$?
make[1]: Entering directory '/home/jschmidt/work/emacs-29'
[...]
make[1]: Leaving directory '/home/jschmidt/work/emacs-29'
[emacs-29]$ ./src/emacs -Q
------------------------- snip -------------------------

Paste the following form into the scratch buffer with C-y:

------------------------- snip -------------------------
(progn
  (with-current-buffer
      (find-file-noselect "lisp/net/shr.el")
    (goto-char 70719)
    (beginning-of-defun)
    ;; point should be at BOF of `shr-table-body', but is in fact
    ;; somewhere near a Unicode character name
    (pop-to-buffer (current-buffer))))
------------------------- snip -------------------------

And immediately after that hit C-x C-e.  It lands me to the position
denoted by "^" (point=68645 of 91908):

------------------------- snip -------------------------
[...]
(defun shr-tag-bdi (dom)
  (insert ?\N{FIRST STRONG ISOLATE})
  (shr-generic dom)
^  (insert ?\N{POP DIRECTIONAL ISOLATE}))

[...]
------------------------- snip -------------------------

If that doesn't help, I can try setting up a reproducer running in batch
mode.

bug#66218: 29.1.50; `beginning-of-defun' jumps to wrong position in `emacs-lisp-mode'

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66218@debbugs.gnu.org
> Date: Sat, 07 Oct 2023 17:47:19 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> >> Cc: Eli Zaretskii <eliz@gnu.org>
> >> Date: Fri, 06 Oct 2023 21:26:24 +0200
> >
> > I'm afraid I cannot reproduce the original problem using the current
> > emacs-29 branch.  Typing C-M-a from buffer position 70719 in shr.el
> > from emacs-29 lands me at the beginning of shr-table-body, as
> > expected.
> 
> Thanks for looking into this, but it still reproduces for me.
> 
> It is important to execute the reproducer *without* actually getting the
> file to the glass, so hitting C-M-a in the file itself would not show
> the issue.

Why not?  If the problem is where you identify it, it should not
matter how beginning-of-defun is invoked.

I'm not being stubborn, mind you, I think it's important that we
understand the issue completely to reason about the solutions.  And I
think we don't yet have a sufficient understanding of the problem, or
at least I don't.

> Please try exactly like described below, starting off the
> root of the development directory:
> 
> ------------------------- snip -------------------------
> [emacs-29]$ pwd
> /home/jschmidt/work/emacs-29
> [emacs-29]$ ls -al README
> -rw-r--r-- 1 jschmidt jschmidt 6125 Aug  1 00:06 README
> [emacs-29]$ git rev-parse HEAD
> 8f23a02a9ea1fbc4213cae5664dcb9bf6b5205f6
> [emacs-29]$ make -j8
> make actual-all || make advice-on-failure make-target=all exit-status=$?
> make[1]: Entering directory '/home/jschmidt/work/emacs-29'
> [...]
> make[1]: Leaving directory '/home/jschmidt/work/emacs-29'
> [emacs-29]$ ./src/emacs -Q
> ------------------------- snip -------------------------
> 
> Paste the following form into the scratch buffer with C-y:
> 
> ------------------------- snip -------------------------
> (progn
>   (with-current-buffer
>       (find-file-noselect "lisp/net/shr.el")
>     (goto-char 70719)
>     (beginning-of-defun)
>     ;; point should be at BOF of `shr-table-body', but is in fact
>     ;; somewhere near a Unicode character name
>     (pop-to-buffer (current-buffer))))
> ------------------------- snip -------------------------
> 
> And immediately after that hit C-x C-e.  It lands me to the position
> denoted by "^" (point=68645 of 91908):

I'm surprised, to say the least, that such a simple bug needs such a
complex reproducer, and any deviation from it fails the recipe.  I
think we must understand why.  And if it turns out that the problem is
so hard to reproduce, maybe we don't have to fix it on emacs-29.

bug#66218: 29.1.50; `beginning-of-defun' jumps to wrong position in `emacs-lisp-mode'

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
>> Cc: 66218@debbugs.gnu.org
>> Date: Sat, 07 Oct 2023 17:47:19 +0200

> I'm surprised, to say the least, that such a simple bug needs such a
> complex reproducer, and any deviation from it fails the recipe.  I
> think we must understand why.

As soon as you find the file and it's displayed in a window, a lot of
things happen even in an "emacs -Q" that set syntax properties in the
buffer.  With these syntax properties already being set, the bug won't
reproduce.

But here is an interactive-only reproducer (which comes close to my
truth, btw, I don't use eldoc-mode or show-paren-mode):

emacs -Q

C-- M-x global-font-lock-mode RET
C-- M-x global-eldoc-mode RET
C-- M-x show-paren-mode RET
C-x C-f lisp/net/shr.el RET
M-x goto-char RET 70719 RET
C-M-a

Ends up on point=68645 of 91908 for me.

> And if it turns out that the problem is so hard to reproduce, maybe we
> don't have to fix it on emacs-29.

Even if the patch is so innocent as adding a `save-match-data' in the
right place?

bug#66218: 29.1.50; `beginning-of-defun' jumps to wrong position in `emacs-lisp-mode'

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66218@debbugs.gnu.org
> Date: Sat, 07 Oct 2023 19:39:52 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> >> Cc: 66218@debbugs.gnu.org
> >> Date: Sat, 07 Oct 2023 17:47:19 +0200
> 
> > I'm surprised, to say the least, that such a simple bug needs such a
> > complex reproducer, and any deviation from it fails the recipe.  I
> > think we must understand why.
> 
> As soon as you find the file and it's displayed in a window, a lot of
> things happen even in an "emacs -Q" that set syntax properties in the
> buffer.  With these syntax properties already being set, the bug won't
> reproduce.

Then I think we have no reason to rush with fixing this on emacs-29.

> But here is an interactive-only reproducer (which comes close to my
> truth, btw, I don't use eldoc-mode or show-paren-mode):
> 
> emacs -Q
> 
> C-- M-x global-font-lock-mode RET
> C-- M-x global-eldoc-mode RET
> C-- M-x show-paren-mode RET
> C-x C-f lisp/net/shr.el RET
> M-x goto-char RET 70719 RET
> C-M-a
> 
> Ends up on point=68645 of 91908 for me.

Yes.

> > And if it turns out that the problem is so hard to reproduce, maybe we
> > don't have to fix it on emacs-29.
> 
> Even if the patch is so innocent as adding a `save-match-data' in the
> right place?

No patch is "innocent" in Emacs, believe me.  But yes, even so.

bug#66218: 29.1.50; `beginning-of-defun' jumps to wrong position in `emacs-lisp-mode'

[Jens Schmidt via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
>> Cc: 66218@debbugs.gnu.org
>> Date: Sat, 07 Oct 2023 19:39:52 +0200

>> Even if the patch is so innocent as adding a `save-match-data' in the
>> right place?
>
> No patch is "innocent" in Emacs, believe me.  But yes, even so.

Please let me summarize (plus one new point):

- This bug is hard to reproduce, also due to its asynchronous nature.

- It might not affect many users, but if one is affected, it might be
  highly confusing.  I could imagine a report like

    "If I press C-M-a at the end of a very long function, it sometimes
    does not jump to the beginning of that function."

  just because under some circumstances the beginning of that function
  might not have been propertized yet.

- The fix is a rather light-weight and IMO obvious change: Use
  `save-match-data' when you want to keep your match data save.

If that does not convince you for Emacs 29, fine with me, let's go for
master, then.  Updated patch attached.

Thanks.

Just for the record: A work-around on Emacs 29 would be to set
`syntax-propertize-function' to nil in `emacs-lisp-mode'.  Or to advise
function `syntax-ppss' to save match data.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-beginning-of-defun-not-jumping-to-BOF.patch --]
[-- Type: text/x-diff, Size: 1227 bytes --]

From 31564be89042e5a4184fa7b8e29d200f30ec596e Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
Date: Tue, 26 Sep 2023 21:36:19 +0200
Subject: [PATCH] Fix beginning-of-defun not jumping to BOF

In batch mode or when font-lock and some other niceties are switched
off, function `syntax-ppss' can modify match data held by function
`beginning-of-defun-raw'.  In that case, `beginning-of-defun' can jump
to some seemingly arbitrary position, and not the actual BOF.

* lisp/emacs-lisp/lisp.el (beginning-of-defun-raw): Save match data
around a call to `syntax-ppss'. (Bug#66218)
---
 lisp/emacs-lisp/lisp.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/lisp.el b/lisp/emacs-lisp/lisp.el
index a4aa79c171e..ee481dc4ed3 100644
--- a/lisp/emacs-lisp/lisp.el
+++ b/lisp/emacs-lisp/lisp.el
@@ -422,7 +422,8 @@ beginning-of-defun-raw
 				       "\\(?:" defun-prompt-regexp "\\)\\s(")
 			     "^\\s(")
 			                      nil 'move arg))
-                    (nth 8 (syntax-ppss))))
+                    (save-match-data
+                      (nth 8 (syntax-ppss)))))
            found)
 	 (progn (goto-char (1- (match-end 0)))
                 t)))
-- 
2.30.2

bug#66218: 29.1.50; `beginning-of-defun' jumps to wrong position in `emacs-lisp-mode'

[Eli Zaretskii]

> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
> Cc: 66218@debbugs.gnu.org
> Date: Sat, 07 Oct 2023 22:31:50 +0200
> 
> If that does not convince you for Emacs 29, fine with me, let's go for
> master, then.  Updated patch attached.
> 
> Thanks.
> 
> Just for the record: A work-around on Emacs 29 would be to set
> `syntax-propertize-function' to nil in `emacs-lisp-mode'.  Or to advise
> function `syntax-ppss' to save match data.

Thanks, installed on master, and closing the bug.