GNU bug report logs - #39680
27.0.60; electric-pair-mode broken by undo

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 39680 in the body.
You can then email your comments to 39680 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: 27.0.60; electric-pair-mode broken by undo
Date: Wed, 19 Feb 2020 19:34:37 +0100

Hello,

Commit e66d5a1c45 (2019-02-19T00:00:44Z!monnier <at> iro.umontreal.ca) might
have introduced a bug.  From emacs -Q:

1. C-x b foo RET
2. M-x electric-pair-mode RET
3. (
    - A closing parenthesis has been inserted.
4. C-b C-f
    - This is to break undo grouping.
5. a
6. C-_
7. (

In Emacs 26.3, buffer foo contains "(())" and point is after the
innermost opening bracket.

In Emacs 27, buffer foo contains "()" and point is after the closing
bracket.  The *Messages* buffer shows:

> cancel-change-group: Undoing to some unrelated state

NB: this can be reproduced with other electric-pair characters, e.g. ".
I found the bug in a Python buffer trying to write a tuple of strings: I
opened a parenthesis, forgot to add quotes, wrote a string, hit undo,
tried to insert a single quote… and got moved past the end parenthesis
instead.

I tried to write a non-regression test; unfortunately what I came up
with does not catch this bug reliably:

(ert-deftest electric-pair-undo-unrelated-state ()
  (with-temp-buffer
    (buffer-enable-undo)
    (electric-pair-mode)
    (let ((last-command-event ?\())
      (self-insert-command 1))
    (undo-boundary)
    (insert "hi there")
    (undo)
    (let ((last-command-event ?\())
      (self-insert-command 1))))

C-x C-e'ing the (with-temp-buffer …) form only triggers the error once
every two evaluations, for some reason.


Thank you for your time.


In GNU Emacs 28.0.50 (build 5, x86_64-pc-linux-gnu, GTK+ Version 3.24.13, cairo version 1.16.0)
 of 2020-02-19 built on my-little-tumbleweed
Repository revision: e1e1bd8f85c53fea9f61b6ec99b461ddd93461b9
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12007000
System Description: openSUSE Tumbleweed

Configured using:
 'configure --with-xwidgets --with-cairo'

Message #8 received at 39680 <at> debbugs.gnu.org (full text, mbox):

From: Alan Mackenzie <acm <at> muc.de>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 39680 <at> debbugs.gnu.org
Subject: Re: bug#39680: 27.0.60; electric-pair-mode broken by undo
Date: Tue, 25 Feb 2020 19:34:51 +0000

Hello, Kévin and Stefan.

On Wed, Feb 19, 2020 at 19:34:37 +0100, Kévin Le Gouguec wrote:
> Hello,

> Commit e66d5a1c45 (2019-02-19T00:00:44Z!monnier <at> iro.umontreal.ca) might
> have introduced a bug.  From emacs -Q:

> 1. C-x b foo RET
> 2. M-x electric-pair-mode RET
> 3. (
>     - A closing parenthesis has been inserted.
> 4. C-b C-f
>     - This is to break undo grouping.
> 5. a
> 6. C-_
> 7. (

> In Emacs 26.3, buffer foo contains "(())" and point is after the
> innermost opening bracket.

> In Emacs 27, buffer foo contains "()" and point is after the closing
> bracket.  The *Messages* buffer shows:

> > cancel-change-group: Undoing to some unrelated state

The cause of this bug is a bug in cancel-change-group, part of the
atomic-change-group group of functions.  The commit you (Kévin) refer to
above substitutes atomic-change-group for a simple insertion and
deletion of a character (for a good reason).

cancel-change-group looks like this:

(defun cancel-change-group (handle)
  "Finish a change group made with `prepare-change-group' (which see).
This finishes the change group by reverting all of its changes."
  (dolist (elt handle)
    (with-current-buffer (car elt)
      (setq elt (cdr elt))
      (save-restriction
        ;; Widen buffer temporarily so if the buffer was narrowed within
        ;; the body of `atomic-change-group' all changes can be undone.
        (widen)
        (let ((old-car (car-safe elt))
              (old-cdr (cdr-safe elt)))
          (unwind-protect
              (progn
                ;; Temporarily truncate the undo log at ELT.
                (when (consp elt)
                  (setcar elt nil) (setcdr elt nil))
                (unless (eq last-command 'undo) (undo-start))   <=======
                ;; Make sure there's no confusion.
============>   (when (and (consp elt) (not (eq elt (last pending-undo-list))))
                  (error "Undoing to some unrelated state"))
                ;; Undo it all.
                (save-excursion
                  (while (listp pending-undo-list) (undo-more 1)))
                ;; Revert the undo info to what it was when we grabbed
                ;; the state.
                (setq buffer-undo-list elt))
            ;; Reset the modified cons cell ELT to its original content.
            (when (consp elt)
              (setcar elt old-car)
              (setcdr elt old-cdr))))))))

On entry to this function, HANDLE has the value:

    ((#<buffer asdf> (2 . 3) nil ("a" . 2) (#<marker at 2 in asdf> . -1)
      nil (2 . 3)))

.  At the first indicated spot above, last-command is indeed 'undo, so
undo-start is not invoked.

Since the undo which undid "a" emptied pending-undo-list,
pending-undo-list has been set to t.

So when the eq is done in the second indicated spot, pending-undo-list
is not ELT (the first cons cell from (cdr handle)).  The function thus
spuriously signals the error "Undoing to some unrelated state".

#########################################################################

I admit I don't fully understand the mechanism of atomic-change-group,
but I see the problem as the EQ comparison on the <======= line.  If
pending-undo-list has been replaced by t (after being exhausted by the
previous 'undo operation), there is no point EQing it with the cons from
HANDLE.

So, as a first approximation to a fix, I added a (consp
pending-undo-list) into this test, as follow:


diff --git a/lisp/subr.el b/lisp/subr.el
index b5ec0de156..8b7d9b5451 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2975,7 +2975,9 @@ cancel-change-group
                 ;; Temporarily truncate the undo log at ELT.
                 (when (consp elt)
                   (setcar elt nil) (setcdr elt nil))
-                (unless (eq last-command 'undo) (undo-start))
+                (unless (and (eq last-command 'undo)
+                             (consp pending-undo-list))
+                  (undo-start))
                 ;; Make sure there's no confusion.
                 (when (and (consp elt) (not (eq elt (last pending-undo-list))))
                   (error "Undoing to some unrelated state"))


Stefan, what is your view on this attempted patch?  Is it sound?

[ .... ]

> Thank you for your time.

Thank you for a good bug report, conveniently reduced to a minimum test
case.

> In GNU Emacs 28.0.50 (build 5, x86_64-pc-linux-gnu, GTK+ Version 3.24.13, cairo version 1.16.0)
>  of 2020-02-19 built on my-little-tumbleweed
> Repository revision: e1e1bd8f85c53fea9f61b6ec99b461ddd93461b9
> Repository branch: master
> Windowing system distributor 'The X.Org Foundation', version 11.0.12007000
> System Description: openSUSE Tumbleweed

> Configured using:
>  'configure --with-xwidgets --with-cairo'

-- 
Alan Mackenzie (Nuremberg, Germany).

Message #11 received at 39680 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Alan Mackenzie <acm <at> muc.de>
Cc: 39680 <at> debbugs.gnu.org,
 Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Subject: Re: bug#39680: 27.0.60; electric-pair-mode broken by undo
Date: Mon, 09 Mar 2020 14:26:21 -0400

> The cause of this bug is a bug in cancel-change-group,

Indeed.
Thanks Alan for the nice analysis (and sorry I didn't get to it earlier).

>                 (unless (eq last-command 'undo) (undo-start))   <=======
>                 ;; Make sure there's no confusion.
> ============>   (when (and (consp elt) (not (eq elt (last pending-undo-list))))

> .  At the first indicated spot above, last-command is indeed 'undo, so
> undo-start is not invoked.

I think there's a bug right here: the fact that the previous command was
`undo` shouldn't really matter.  Worse, we've made changes to the buffer
since that last `undo` so it's plain wrong to pass that old
`pending-undo-list` to `undo-more`.

> Stefan, what is your view on this attempted patch?  Is it sound?

I think we need something like the patch below (not really tested yet).
WDYT?

>> Thank you for your time.
> Thank you for a good bug report, conveniently reduced to a minimum test
> case.

Indeed.  This is pretty delicate code, so a concise and easy to reproduce
test case is very welcome.



        Stefan


diff --git a/lisp/subr.el b/lisp/subr.el
index 13515ca7da..ebc8e320dc 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2972,13 +2972,14 @@ cancel-change-group
 	;; the body of `atomic-change-group' all changes can be undone.
 	(widen)
 	(let ((old-car (car-safe elt))
-	      (old-cdr (cdr-safe elt)))
+	      (old-cdr (cdr-safe elt))
+	      (start-pul pending-undo-list))
           (unwind-protect
               (progn
                 ;; Temporarily truncate the undo log at ELT.
                 (when (consp elt)
                   (setcar elt nil) (setcdr elt nil))
-                (unless (eq last-command 'undo) (undo-start))
+                (setq pending-undo-list buffer-undo-list)
                 ;; Make sure there's no confusion.
                 (when (and (consp elt) (not (eq elt (last pending-undo-list))))
                   (error "Undoing to some unrelated state"))
@@ -2991,7 +2992,13 @@ cancel-change-group
             ;; Reset the modified cons cell ELT to its original content.
             (when (consp elt)
               (setcar elt old-car)
-              (setcdr elt old-cdr))))))))
+              (setcdr elt old-cdr)))
+          ;; Let's not break a sequence of undos just because we
+          ;; tried to make a change and then undid it: preserve
+          ;; the original `pending-undo-list' if it's still valid.
+          (if (eq (undo--last-change-was-undo-p buffer-undo-list)
+                  start-pul)
+              (setq pending-undo-list start-pul)))))))
 
 ;;;; Display-related functions.
 

Message #14 received at 39680 <at> debbugs.gnu.org (full text, mbox):

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Alan Mackenzie <acm <at> muc.de>, 39680 <at> debbugs.gnu.org
Subject: Re: bug#39680: 27.0.60; electric-pair-mode broken by undo
Date: Tue, 10 Mar 2020 07:45:38 +0100

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> Stefan, what is your view on this attempted patch?  Is it sound?
>
> I think we need something like the patch below (not really tested yet).
> WDYT?

FWIW, I applied it and AFAICT I can't reproduce the issue.

>>> Thank you for your time.
>> Thank you for a good bug report, conveniently reduced to a minimum test
>> case.
>
> Indeed.  This is pretty delicate code, so a concise and easy to reproduce
> test case is very welcome.

Happy to help.  Though I wonder why my attempted ERT test only fails
half the time.  Reproduced here for context:

(ert-deftest electric-pair-undo-unrelated-state ()
  (with-temp-buffer
    (buffer-enable-undo)
    (electric-pair-mode)
    (let ((last-command-event ?\())
      (self-insert-command 1))
    (undo-boundary)
    (insert "hi there")
    (undo)
    (let ((last-command-event ?\())
      (self-insert-command 1))))

With your patch, C-x C-e'ing the inner with-temp-buffer form once
triggers no error (an "Undo" shows up in *Messages*); C-x C-e'ing a
second time without moving point causes a "No further undo information"
user error.


At any rate, thanks for the fix!

Message #17 received at 39680 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: Alan Mackenzie <acm <at> muc.de>, 39680 <at> debbugs.gnu.org
Subject: Re: bug#39680: 27.0.60; electric-pair-mode broken by undo
Date: Wed, 11 Mar 2020 11:31:45 -0400

>> I think we need something like the patch below (not really tested yet).
>> WDYT?
>
> FWIW, I applied it and AFAICT I can't reproduce the issue.

Thanks.  I think the patch wasn't quite right and was overly
complicated: `undo` is already responsible for dealing with the
freshness of `pending-undo-list`.  So I think the better patch is the
one below.

Eli, is the patch below OK for `emacs-27`?


        Stefan


[ BTW, I accidentally installed the previous patch on `master`, and I'm
  waiting for "the right solution" to clean up.  ]

diff --git a/lisp/subr.el b/lisp/subr.el
index 5b94343e499..4c69c9dd044 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -2964,13 +2964,18 @@ cancel-change-group
 	;; the body of `atomic-change-group' all changes can be undone.
 	(widen)
 	(let ((old-car (car-safe elt))
-	      (old-cdr (cdr-safe elt)))
+	      (old-cdr (cdr-safe elt))
+	      ;; Use `pending-undo-list' temporarily since `undo-more' needs
+	      ;; it, but restore it afterwards so as not to mess with an
+	      ;; ongoing sequence of `undo's.
+	      (pending-undo-list
+	       ;; Use `buffer-undo-list' unconditionally (bug#39680).
+	       buffer-undo-list))
           (unwind-protect
               (progn
                 ;; Temporarily truncate the undo log at ELT.
                 (when (consp elt)
                   (setcar elt nil) (setcdr elt nil))
-                (unless (eq last-command 'undo) (undo-start))
                 ;; Make sure there's no confusion.
                 (when (and (consp elt) (not (eq elt (last pending-undo-list))))
                   (error "Undoing to some unrelated state"))

Message #20 received at 39680 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: acm <at> muc.de, 39680 <at> debbugs.gnu.org, kevin.legouguec <at> gmail.com
Subject: Re: bug#39680: 27.0.60; electric-pair-mode broken by undo
Date: Wed, 11 Mar 2020 18:20:09 +0200

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Date: Wed, 11 Mar 2020 11:31:45 -0400
> Cc: Alan Mackenzie <acm <at> muc.de>, 39680 <at> debbugs.gnu.org
> 
> Eli, is the patch below OK for `emacs-27`?

Yes, thanks.

Message #25 received at 39680-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: acm <at> muc.de, 39680-done <at> debbugs.gnu.org, kevin.legouguec <at> gmail.com
Subject: Re: bug#39680: 27.0.60; electric-pair-mode broken by undo
Date: Thu, 12 Mar 2020 10:04:11 -0400

>> Eli, is the patch below OK for `emacs-27`?
> Yes, thanks.

Thanks, installed,


        Stefan

Message #32 received at 39680 <at> debbugs.gnu.org (full text, mbox):

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: João Távora <joaotavora <at> gmail.com>
Cc: Alan Mackenzie <acm <at> muc.de>, control <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 39680 <at> debbugs.gnu.org
Subject: bug#39680: Test case
Date: Wed, 20 May 2020 00:05:18 +0200

[Message part 1 (text/plain, inline)]

Finally nailed it, after finding out about ert-simulate-command.

The following patch adds a test that passes on master and emacs-27, and
"successfully fails" when reverting Stefan's fix (commit c1ce9fa7f2).

[0001-Add-test-for-bug-39680.patch (text/x-patch, attachment)]

Message #35 received at 39680 <at> debbugs.gnu.org (full text, mbox):

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: João Távora <joaotavora <at> gmail.com>
Cc: Alan Mackenzie <acm <at> muc.de>, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 39680 <at> debbugs.gnu.org
Subject: Re: bug#39680: Test case
Date: Wed, 20 May 2020 00:26:42 +0200

(Removing control <at> debbugs.gnu.org from the CC list; sorry for this
blunder.)

Kévin Le Gouguec <kevin.legouguec <at> gmail.com> writes:

> Finally nailed it, after finding out about ert-simulate-command.
>
> The following patch adds a test that passes on master and emacs-27, and
> "successfully fails" when reverting Stefan's fix (commit c1ce9fa7f2).

Message #38 received at 39680 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: Alan Mackenzie <acm <at> muc.de>, control <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 39680 <at> debbugs.gnu.org
Subject: Re: bug#39680: Test case
Date: Wed, 20 May 2020 00:17:13 +0100

Kévin Le Gouguec <kevin.legouguec <at> gmail.com> writes:

> Finally nailed it, after finding out about ert-simulate-command.
>
> The following patch adds a test that passes on master and emacs-27, and
> "successfully fails" when reverting Stefan's fix (commit c1ce9fa7f2).

Thanks! pushed to master.

João

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.