bug#39680: 27.0.60; electric-pair-mode broken by undo

bug#39680: 27.0.60; electric-pair-mode broken by undo

[Kévin Le Gouguec]

Hello,

Commit e66d5a1c45 (2019-02-19T00:00:44Z!monnier@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'

bug#39680: 27.0.60; electric-pair-mode broken by undo

[Alan Mackenzie]

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@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).

bug#39680: 27.0.60; electric-pair-mode broken by undo

[Stefan Monnier]

> 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)))))))
 \f
 ;;;; Display-related functions.
 

bug#39680: 27.0.60; electric-pair-mode broken by undo

[Kévin Le Gouguec]

Stefan Monnier <monnier@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!

bug#39680: 27.0.60; electric-pair-mode broken by undo

[Stefan Monnier]

>> 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"))

bug#39680: 27.0.60; electric-pair-mode broken by undo

[Eli Zaretskii]

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

Yes, thanks.

bug#39680: 27.0.60; electric-pair-mode broken by undo

[Stefan Monnier]

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

Thanks, installed,


        Stefan

bug#39680: Test case

[Kévin Le Gouguec]

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

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).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-test-for-bug-39680.patch --]
[-- Type: text/x-patch, Size: 1414 bytes --]

From c83fbd50f0f0999814f09706fc908df5c22c5a47 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Tue, 19 May 2020 23:17:04 +0200
Subject: [PATCH] Add test for bug#39680

* test/lisp/electric-tests.el (electric-pair-undo-unrelated-state):
New test.
---
 test/lisp/electric-tests.el | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/test/lisp/electric-tests.el b/test/lisp/electric-tests.el
index 56d1bdb110..67f474cbd5 100644
--- a/test/lisp/electric-tests.el
+++ b/test/lisp/electric-tests.el
@@ -546,6 +546,24 @@ js-mode-braces-with-layout-and-indent
       (electric-pair-delete-pair 1)
       (should (equal "" (buffer-string))))))
 
+\f
+;;; Undoing
+(ert-deftest electric-pair-undo-unrelated-state ()
+  "Make sure `electric-pair-mode' does not confuse `undo' (bug#39680)."
+  (with-temp-buffer
+    (buffer-enable-undo)
+    (electric-pair-local-mode)
+    (let ((last-command-event ?\())
+      (ert-simulate-command '(self-insert-command 1)))
+    (undo-boundary)
+    (let ((last-command-event ?a))
+      (ert-simulate-command '(self-insert-command 1)))
+    (undo-boundary)
+    (ert-simulate-command '(undo))
+    (let ((last-command-event ?\())
+      (ert-simulate-command '(self-insert-command 1)))
+    (should (string= (buffer-string) "(())"))))
+
 \f
 ;;; Electric newlines between pairs
 ;;; TODO: better tests
-- 
2.26.2

bug#39680: Test case

[Kévin Le Gouguec]

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

Kévin Le Gouguec <kevin.legouguec@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).

bug#39680: Test case

[João Távora]

Kévin Le Gouguec <kevin.legouguec@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