GNU bug report logs - #78042
31.0.50; Improper sequence of `before/after-change-functions` calls

Previous Next

To reply to this bug, email your comments to 78042 AT debbugs.gnu.org.
There is no need to reopen the bug first.

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

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: bug-gnu-emacs <at> gnu.org
Subject: 31.0.50; Improper sequence of `before/after-change-functions` calls
Date: Thu, 24 Apr 2025 11:38:59 -0400

Package: Emacs
Version: 31.0.50


I just found another case where we break our promises w.r.t
`before/after-change-functions`:

    src/emacs -Q src/regexp-emacs.c
    M-: (decode-coding-region 123 (point-max) 'windows-1252) RET

[ The details of the file and buffer positions don't really
  matter, AFAIK.  ]

I first get a run of `before-change-functions` for 123...point-max, as
expected at the beginning and a run of `after-change-functions` for
123...point-max, as expected at the end, but between the two I get
another run of `before-change-functions` for 123..16497:

    #0  signal_before_change
        (start_int=start_int <at> entry=123, end_int=end_int <at> entry=16497, preserve_ptr=preserve_ptr <at> entry=0x0) at insdel.c:2157
    #1  0x0000555555702c3a in prepare_to_modify_buffer_1
        (start=start <at> entry=123, end=end <at> entry=16497, preserve_ptr=preserve_ptr <at> entry=0x0) at insdel.c:2024
    #2  0x00005555557c9f36 in modify_text_properties (buffer=MISSING, 
        buffer <at> entry=XIL(0x555555dfac0d), start=MISSING, end=MISSING) at textprop.c:85
    #3  0x00005555557cb5cc in add_text_properties_1
        (start=make_fixnum(123), end=make_fixnum(16497), properties=XIL(0x7fffffffce33), object=XIL(0x555555dfac0d), set_type=set_type <at> entry=TEXT_PROPERTY_REPLACE, destructive=destructive <at> entry=true) at textprop.c:1236
    #4  0x00005555557cb9b8 in Fadd_text_properties (start=MISSING, 
        start <at> entry=make_fixnum(123), end=MISSING, 
        end <at> entry=make_fixnum(16497), properties=MISSING, object=MISSING, 
        object <at> entry=XIL(0x555555dfac0d)) at textprop.c:1308
    #5  0x00005555557cba12 in Fput_text_property
        (start=make_fixnum(123), end=end <at> entry=make_fixnum(16497), property=MISSING, 
        property <at> entry=XIL(0x55b0), value=MISSING, 
        value <at> entry=XIL(0x2aaa9984e428), object=XIL(0x555555dfac0d))
        at textprop.c:1326
    #6  0x0000555555644852 in produce_charset
        (coding=coding <at> entry=0x7fffffffd080, charbuf=charbuf <at> entry=0x555556a7ef88, pos=pos <at> entry=16497) at coding.c:7277
    #7  0x00005555556448e8 in produce_annotation
        (coding=coding <at> entry=0x7fffffffd080, pos=16497, pos <at> entry=123)
        at coding.c:7320
    #8  0x000055555564578f in decode_coding (coding=coding <at> entry=0x7fffffffd080)
        at coding.c:7421
    #9  0x000055555564cc32 in decode_coding_object
        (coding=coding <at> entry=0x7fffffffd080, src_object=src_object <at> entry=XIL(0x555555dfac0d), from=from <at> entry=123, from_byte=from_byte <at> entry=123, to=to <at> entry=161558, to_byte=to_byte <at> entry=161558, dst_object=XIL(0x555555dfac0d))
        at coding.c:8183
    #10 0x000055555564e1c7 in code_convert_region
        (start=make_fixnum(123), end=make_fixnum(161558), coding_system=XIL(0x2aaa9984e428), dst_object=XIL(0x555555dfac0d), encodep=encodep <at> entry=false, norecord=norecord <at> entry=false) at coding.c:9510

There's probably also a corresponding run of `after-change-functions`,
I haven't checked, but the problem is that the "final run of
`after-change-functions` for 123...point-max is now "invalid" in the
sense that it modifies a range of the buffer beyond the last run of
`before-change-functions`, contrary to what we promise.


- Stefan

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: 78042 <at> debbugs.gnu.org
Subject: Re: bug#78042: Improper sequence of `before/after-change-functions`
 calls
Date: Thu, 24 Apr 2025 21:35:09 -0400

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

Hmm...

The code that runs those hooks is really littered at odd places, with no
good/standard way to control when to run it and when not.
I suggest the patch below (which includes a corresponding test).
Any objection?


        Stefan

[decode-change-hook.patch (text/x-diff, inline)]

diff --git a/src/coding.c b/src/coding.c
index 63b0dbeb18b..b4209986918 100644
--- a/src/coding.c
+++ b/src/coding.c
@@ -7363,6 +7363,7 @@ decode_coding (struct coding_system *coding)
   struct ccl_spec cclspec;
   int carryover;
   int i;
+  specpdl_ref count = SPECPDL_INDEX ();
 
   USE_SAFE_ALLOCA;
 
@@ -7389,6 +7390,8 @@ decode_coding (struct coding_system *coding)
 
       undo_list = BVAR (current_buffer, undo_list);
       bset_undo_list (current_buffer, Qt);
+      /* The caller should run *-change-functions over the whole region.  */
+      specbind (Qinhibit_modification_hooks, Qt);
     }
 
   coding->consumed = coding->consumed_char = 0;
@@ -7501,7 +7504,7 @@ decode_coding (struct coding_system *coding)
       record_insert (coding->dst_pos, coding->produced_char);
     }
 
-  SAFE_FREE ();
+  SAFE_FREE_UNBIND_TO (count, Qnil);
 }
 
 
diff --git a/test/src/editfns-tests.el b/test/src/editfns-tests.el
index 2553ad3ec2c..9a27c420f1e 100644
--- a/test/src/editfns-tests.el
+++ b/test/src/editfns-tests.el
@@ -498,10 +498,10 @@ sanity-check-change-functions-verbose
 (defvar sanity-check-change-functions-op nil)
 (defmacro sanity-check-change-functions-with-op (op &rest body)
   (declare (debug t) (indent 1))
-  `(let ((sanity-check-change-functions-op ,op))
-     (sanity-check--message "%S..." sanity-check-change-functions-op)
+  `(let ((sanity-check-change-functions-op (list ,op)))
+     (sanity-check--message "%S..." ,op)
      ,@body
-     (sanity-check--message "%S...done" sanity-check-change-functions-op)))
+     (sanity-check--message "%S...done" ,op)))
 
 (defun sanity-check--message (&rest args)
   (if sanity-check-change-functions-verbose (apply #'message args)))
@@ -530,6 +530,7 @@ sanity-check-change-functions-check-size
     (setq sanity-check-change-functions-buffer-size (buffer-size)))))
 
 (defun sanity-check-change-functions-before (beg end)
+  (push `(BEFORE ,beg ,end) sanity-check-change-functions-op)
   (sanity-check--message "Before: %S %S" beg end)
   (unless (<= (point-min) beg end (point-max))
     (sanity-check-change-functions-error
@@ -540,6 +541,7 @@ sanity-check-change-functions-before
   (setq sanity-check-change-functions-end end))
 
 (defun sanity-check-change-functions-after (beg end len)
+  (push `(AFTER ,beg ,end ,len) sanity-check-change-functions-op)
   (sanity-check--message "After : %S %S (%S)" beg end len)
   (unless (<= (point-min) beg end (point-max))
     (sanity-check-change-functions-error
@@ -565,7 +567,7 @@ sanity-check-change-functions-after
 (defun sanity-check-change-functions-errors ()
   (sanity-check-change-functions-check-size)
   (if sanity-check-change-functions-errors
-      (cons sanity-check-change-functions-op
+      (cons (reverse sanity-check-change-functions-op)
             sanity-check-change-functions-errors)))
 
 (ert-deftest editfns-tests--before/after-change-functions ()
@@ -591,6 +593,24 @@ editfns-tests--before/after-change-functions
         (decode-coding-region beg (point) 'utf-8)
         (should (null (sanity-check-change-functions-errors)))))
 
+    (let ((beg (point)))                ;bug#78042
+      (apply #'insert (make-list 5000 "hell\351 "))
+      (sanity-check-change-functions-with-op 'DECODE-CODING-LARGE-REGION
+        (decode-coding-region beg (point) 'windows-1252)
+        (should-not (sanity-check-change-functions-errors))))
+
+    (let ((beg (point)))                ;bug#78042
+      (sanity-check-change-functions-with-op 'DECODE-CODING-INSERT
+        ;; The `insert' calls make sure we track the buffer-size
+        ;; so as to detect if `decode-coding-string' fails to run the
+        ;; `*-change-functions'.
+        (insert "<")
+        (decode-coding-string "hell\351 " 'windows-1252 nil (current-buffer))
+        (forward-char 6)
+        (insert ">")
+        (should (equal "<hellé >" (buffer-substring beg (point))))
+        (should-not (sanity-check-change-functions-errors))))
+
     (sanity-check-change-functions-with-op 'ENCODE-CODING-STRING
       (encode-coding-string "ééé" 'utf-8 nil (current-buffer))
       (should (null (sanity-check-change-functions-errors))))

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: monnier <at> iro.umontreal.ca, 78042 <at> debbugs.gnu.org
Subject: Re: bug#78042: 31.0.50;
 Improper sequence of `before/after-change-functions` calls
Date: Fri, 25 Apr 2025 09:08:53 +0300

> Cc: monnier <at> iro.umontreal.ca
> Date: Thu, 24 Apr 2025 11:38:59 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> Package: Emacs
> Version: 31.0.50
> 
> 
> I just found another case where we break our promises w.r.t
> `before/after-change-functions`:
> 
>     src/emacs -Q src/regexp-emacs.c
>     M-: (decode-coding-region 123 (point-max) 'windows-1252) RET
> 
> [ The details of the file and buffer positions don't really
>   matter, AFAIK.  ]
> 
> I first get a run of `before-change-functions` for 123...point-max, as
> expected at the beginning and a run of `after-change-functions` for
> 123...point-max, as expected at the end, but between the two I get
> another run of `before-change-functions` for 123..16497:
> 
>     #0  signal_before_change
>         (start_int=start_int <at> entry=123, end_int=end_int <at> entry=16497, preserve_ptr=preserve_ptr <at> entry=0x0) at insdel.c:2157
>     #1  0x0000555555702c3a in prepare_to_modify_buffer_1
>         (start=start <at> entry=123, end=end <at> entry=16497, preserve_ptr=preserve_ptr <at> entry=0x0) at insdel.c:2024
>     #2  0x00005555557c9f36 in modify_text_properties (buffer=MISSING, 
>         buffer <at> entry=XIL(0x555555dfac0d), start=MISSING, end=MISSING) at textprop.c:85
>     #3  0x00005555557cb5cc in add_text_properties_1
>         (start=make_fixnum(123), end=make_fixnum(16497), properties=XIL(0x7fffffffce33), object=XIL(0x555555dfac0d), set_type=set_type <at> entry=TEXT_PROPERTY_REPLACE, destructive=destructive <at> entry=true) at textprop.c:1236
>     #4  0x00005555557cb9b8 in Fadd_text_properties (start=MISSING, 
>         start <at> entry=make_fixnum(123), end=MISSING, 
>         end <at> entry=make_fixnum(16497), properties=MISSING, object=MISSING, 
>         object <at> entry=XIL(0x555555dfac0d)) at textprop.c:1308
>     #5  0x00005555557cba12 in Fput_text_property
>         (start=make_fixnum(123), end=end <at> entry=make_fixnum(16497), property=MISSING, 
>         property <at> entry=XIL(0x55b0), value=MISSING, 
>         value <at> entry=XIL(0x2aaa9984e428), object=XIL(0x555555dfac0d))
>         at textprop.c:1326
>     #6  0x0000555555644852 in produce_charset
>         (coding=coding <at> entry=0x7fffffffd080, charbuf=charbuf <at> entry=0x555556a7ef88, pos=pos <at> entry=16497) at coding.c:7277
>     #7  0x00005555556448e8 in produce_annotation
>         (coding=coding <at> entry=0x7fffffffd080, pos=16497, pos <at> entry=123)
>         at coding.c:7320
>     #8  0x000055555564578f in decode_coding (coding=coding <at> entry=0x7fffffffd080)
>         at coding.c:7421
>     #9  0x000055555564cc32 in decode_coding_object
>         (coding=coding <at> entry=0x7fffffffd080, src_object=src_object <at> entry=XIL(0x555555dfac0d), from=from <at> entry=123, from_byte=from_byte <at> entry=123, to=to <at> entry=161558, to_byte=to_byte <at> entry=161558, dst_object=XIL(0x555555dfac0d))
>         at coding.c:8183
>     #10 0x000055555564e1c7 in code_convert_region
>         (start=make_fixnum(123), end=make_fixnum(161558), coding_system=XIL(0x2aaa9984e428), dst_object=XIL(0x555555dfac0d), encodep=encodep <at> entry=false, norecord=norecord <at> entry=false) at coding.c:9510
> 
> There's probably also a corresponding run of `after-change-functions`,
> I haven't checked, but the problem is that the "final run of
> `after-change-functions` for 123...point-max is now "invalid" in the
> sense that it modifies a range of the buffer beyond the last run of
> `before-change-functions`, contrary to what we promise.

Sorry, I don't think I understand the problem.  Is the problem that we
have "nested" notifications, whereby you have

  before-change-functions A1 B1
  before-change-functions A2 B2
  after-change-functions  A2 B2
  after-change-functions  A1 B1

IOW, _any_ situation where we have nested pairs of notifications is
"breaking our promise"?  If so, how can that be avoided in principle
in Emacs, given its recursive nature, except by some mechanism which
collects all the notifications and rearranges them to "keep our
promise"?

Or what am I missing?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 78042 <at> debbugs.gnu.org
Subject: Re: bug#78042: Improper sequence of `before/after-change-functions`
 calls
Date: Fri, 25 Apr 2025 10:32:32 +0300

> Date: Thu, 24 Apr 2025 21:35:09 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> The code that runs those hooks is really littered at odd places, with no
> good/standard way to control when to run it and when not.
> I suggest the patch below (which includes a corresponding test).
> Any objection?
> 
> 
> diff --git a/src/coding.c b/src/coding.c
> index 63b0dbeb18b..b4209986918 100644
> --- a/src/coding.c
> +++ b/src/coding.c
> @@ -7363,6 +7363,7 @@ decode_coding (struct coding_system *coding)
>    struct ccl_spec cclspec;
>    int carryover;
>    int i;
> +  specpdl_ref count = SPECPDL_INDEX ();
>  
>    USE_SAFE_ALLOCA;
>  
> @@ -7389,6 +7390,8 @@ decode_coding (struct coding_system *coding)
>  
>        undo_list = BVAR (current_buffer, undo_list);
>        bset_undo_list (current_buffer, Qt);
> +      /* The caller should run *-change-functions over the whole region.  */
> +      specbind (Qinhibit_modification_hooks, Qt);
>      }

How do we make sure this requirement is heeded to?

And how do we document this incompatible change in NEWS?

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78042 <at> debbugs.gnu.org
Subject: Re: bug#78042: 31.0.50; Improper sequence of
 `before/after-change-functions` calls
Date: Fri, 25 Apr 2025 12:37:34 -0400

> Sorry, I don't think I understand the problem.  Is the problem that we
> have "nested" notifications, whereby you have
>
>   before-change-functions A1 B1
>   before-change-functions A2 B2
>   after-change-functions  A2 B2
>   after-change-functions  A1 B1
>
> IOW, _any_ situation where we have nested pairs of notifications is
> "breaking our promise"?

Yup: the `before-change-functions A2 B2` promises that there won't be
any changes to the buffer outside of A2...B2 until the next
`before-change-functions`.

> If so, how can that be avoided in principle in Emacs, given its
> recursive nature, except by some mechanism which collects all the
> notifications and rearranges them to "keep our promise"?

There is currently no attempt in the C code to solve this "by
construction" or "by design".  Instead we fix the offenders as we bump
into them.


        Stefan

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78042 <at> debbugs.gnu.org
Subject: Re: bug#78042: Improper sequence of `before/after-change-functions`
 calls
Date: Fri, 25 Apr 2025 13:39:28 -0400

>> diff --git a/src/coding.c b/src/coding.c
>> index 63b0dbeb18b..b4209986918 100644
>> --- a/src/coding.c
>> +++ b/src/coding.c
>> @@ -7363,6 +7363,7 @@ decode_coding (struct coding_system *coding)
>>    struct ccl_spec cclspec;
>>    int carryover;
>>    int i;
>> +  specpdl_ref count = SPECPDL_INDEX ();
>>  
>>    USE_SAFE_ALLOCA;
>>  
>> @@ -7389,6 +7390,8 @@ decode_coding (struct coding_system *coding)
>>  
>>        undo_list = BVAR (current_buffer, undo_list);
>>        bset_undo_list (current_buffer, Qt);
>> +      /* The caller should run *-change-functions over the whole region.  */
>> +      specbind (Qinhibit_modification_hooks, Qt);
>>      }
>
> How do we make sure this requirement is heeded to?

The rest of the code in this function modifies the buffer without
running any `before/after-change-functions` (except for things like
charset properties, which is what I'm trying to suppress), so if a caller
fails to run those hooks we'd know (and the above patch wouldn't make
it worse).

> And how do we document this incompatible change in NEWS?

It's a bug fix, not an incompatible change.


        Stefan

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 78042 <at> debbugs.gnu.org
Subject: Re: bug#78042: 31.0.50; Improper sequence of
 `before/after-change-functions` calls
Date: Fri, 25 Apr 2025 21:39:46 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: 78042 <at> debbugs.gnu.org
> Date: Fri, 25 Apr 2025 12:37:34 -0400
> 
> > Sorry, I don't think I understand the problem.  Is the problem that we
> > have "nested" notifications, whereby you have
> >
> >   before-change-functions A1 B1
> >   before-change-functions A2 B2
> >   after-change-functions  A2 B2
> >   after-change-functions  A1 B1
> >
> > IOW, _any_ situation where we have nested pairs of notifications is
> > "breaking our promise"?
> 
> Yup: the `before-change-functions A2 B2` promises that there won't be
> any changes to the buffer outside of A2...B2 until the next
> `before-change-functions`.
> 
> > If so, how can that be avoided in principle in Emacs, given its
> > recursive nature, except by some mechanism which collects all the
> > notifications and rearranges them to "keep our promise"?
> 
> There is currently no attempt in the C code to solve this "by
> construction" or "by design".  Instead we fix the offenders as we bump
> into them.

I submit that we will never be able to solve all of them.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 78042 <at> debbugs.gnu.org
Subject: Re: bug#78042: Improper sequence of `before/after-change-functions`
 calls
Date: Fri, 25 Apr 2025 21:56:44 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: 78042 <at> debbugs.gnu.org
> Date: Fri, 25 Apr 2025 13:39:28 -0400
> 
> >> diff --git a/src/coding.c b/src/coding.c
> >> index 63b0dbeb18b..b4209986918 100644
> >> --- a/src/coding.c
> >> +++ b/src/coding.c
> >> @@ -7363,6 +7363,7 @@ decode_coding (struct coding_system *coding)
> >>    struct ccl_spec cclspec;
> >>    int carryover;
> >>    int i;
> >> +  specpdl_ref count = SPECPDL_INDEX ();
> >>  
> >>    USE_SAFE_ALLOCA;
> >>  
> >> @@ -7389,6 +7390,8 @@ decode_coding (struct coding_system *coding)
> >>  
> >>        undo_list = BVAR (current_buffer, undo_list);
> >>        bset_undo_list (current_buffer, Qt);
> >> +      /* The caller should run *-change-functions over the whole region.  */
> >> +      specbind (Qinhibit_modification_hooks, Qt);
> >>      }
> >
> > How do we make sure this requirement is heeded to?
> 
> The rest of the code in this function modifies the buffer without
> running any `before/after-change-functions` (except for things like
> charset properties, which is what I'm trying to suppress), so if a caller
> fails to run those hooks we'd know (and the above patch wouldn't make
> it worse).

I was asking how will we be able to make sure the caller does run the
modification hooks.  I don't quite understand what you mean by "we'd
know".  Know how and by what signs?

> > And how do we document this incompatible change in NEWS?
> 
> It's a bug fix, not an incompatible change.

We now require callers to do something they didn't have to do before,
or am I missing something?

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78042 <at> debbugs.gnu.org
Subject: Re: bug#78042: 31.0.50; Improper sequence of
 `before/after-change-functions` calls
Date: Fri, 25 Apr 2025 15:12:18 -0400

>> There is currently no attempt in the C code to solve this "by
>> construction" or "by design".  Instead we fix the offenders as we bump
>> into them.
> I submit that we will never be able to solve all of them.

I'm pretty sure we can solve all the ones we find.
But we may never find them all, indeed.

My request for comments/objection was not about whether it is worth
fixing this bug (there is no doubt about that on my side: it affects at
least Eglot and CC-mode), but whether the approach I used is acceptable
or if you have a better alternative to propose.


        Stefan

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78042 <at> debbugs.gnu.org
Subject: Re: bug#78042: Improper sequence of `before/after-change-functions`
 calls
Date: Fri, 25 Apr 2025 15:22:32 -0400

>> The rest of the code in this function modifies the buffer without
>> running any `before/after-change-functions` (except for things like
>> charset properties, which is what I'm trying to suppress), so if a caller
>> fails to run those hooks we'd know (and the above patch wouldn't make
>> it worse).
>
> I was asking how will we be able to make sure the caller does run the
> modification hooks.

The function where I made the modification inserts bytes into
the buffer but without running the hooks.
So it is *already* the case that the callers must run the hooks
themselves.

> I don't quite understand what you mean by "we'd know".  Know how and by what signs?

We would have noticed it because when those hooks are not run something
*will* misbehave sooner or later, and for a common operation like
`de/encode-coding-region`, it'll be sooner rather than later.

I also know because I'm running my own Emacs with additional checks to
detect when our change-functions break our promises, so that I can fix
those bugs before they bite someone.

>> > And how do we document this incompatible change in NEWS?
>> It's a bug fix, not an incompatible change.
> We now require callers to do something they didn't have to do before,
> or am I missing something?

Yes you're missing that the callers have always had to do that because
in the usual case, there is no nested modifications (because the decode
does not add any text-properties), and the function just inserts text
into the buffer without running any change-functions.


        Stefan

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 78042 <at> debbugs.gnu.org
Subject: Re: bug#78042: 31.0.50; Improper sequence of
 `before/after-change-functions` calls
Date: Sat, 26 Apr 2025 09:22:32 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: 78042 <at> debbugs.gnu.org
> Date: Fri, 25 Apr 2025 15:12:18 -0400
> 
> >> There is currently no attempt in the C code to solve this "by
> >> construction" or "by design".  Instead we fix the offenders as we bump
> >> into them.
> > I submit that we will never be able to solve all of them.
> 
> I'm pretty sure we can solve all the ones we find.
> But we may never find them all, indeed.
> 
> My request for comments/objection was not about whether it is worth
> fixing this bug (there is no doubt about that on my side: it affects at
> least Eglot and CC-mode), but whether the approach I used is acceptable
> or if you have a better alternative to propose.

The only alternative I can think of is to be able to handle nested
notifications, such as what happens in this case, as correct.  What
are the problems due to which we must have paired notifications
without nesting?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 78042 <at> debbugs.gnu.org
Subject: Re: bug#78042: Improper sequence of `before/after-change-functions`
 calls
Date: Sat, 26 Apr 2025 09:43:06 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: 78042 <at> debbugs.gnu.org
> Date: Fri, 25 Apr 2025 15:22:32 -0400
> 
> >> The rest of the code in this function modifies the buffer without
> >> running any `before/after-change-functions` (except for things like
> >> charset properties, which is what I'm trying to suppress), so if a caller
> >> fails to run those hooks we'd know (and the above patch wouldn't make
> >> it worse).
> >
> > I was asking how will we be able to make sure the caller does run the
> > modification hooks.
> 
> The function where I made the modification inserts bytes into
> the buffer but without running the hooks.
> So it is *already* the case that the callers must run the hooks
> themselves.

I don't understand: if you made the changes (by removing the calls to
modification hooks), then the function originally _was_ calling them,
no?

Anyway, at the time I audited the functions in coding.c and verified
that the hooks were always called by some function in the code path.
If we are changing that now, we need to repeat such auditing.

> > I don't quite understand what you mean by "we'd know".  Know how and by what signs?
> 
> We would have noticed it because when those hooks are not run something
> *will* misbehave sooner or later, and for a common operation like
> `de/encode-coding-region`, it'll be sooner rather than later.
> 
> I also know because I'm running my own Emacs with additional checks to
> detect when our change-functions break our promises, so that I can fix
> those bugs before they bite someone.

The problem with such ad-hoc evidence is that it relies on the
assumption that your code and the "usual" cases everyone runs do
exercise all of the relevant code paths.  Such assumptions are not
very reliable in Emacs, IME.

> >> > And how do we document this incompatible change in NEWS?
> >> It's a bug fix, not an incompatible change.
> > We now require callers to do something they didn't have to do before,
> > or am I missing something?
> 
> Yes you're missing that the callers have always had to do that because
> in the usual case, there is no nested modifications (because the decode
> does not add any text-properties), and the function just inserts text
> into the buffer without running any change-functions.

But the "usual-case" notification is about the entire decoded text,
whereas the notifications your patch blocks are about smaller chunks
of text for specific kinds of changes, and thus more fine-grained.  So
the hooks will need to do a more complex job now: they cannot rely on
the fact that notifications for decoding are separate from
notifications about text-property changes, so they will need to
examine the entire decoded region of text and decide what to do with
each chunk of it.  So the existing hook functions might fail to work
correctly after this change, and we must therefore call out this
change in NEWS.

Or maybe we should not install this at all?  What are the problems
these "nested" notifications cause, again?

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78042 <at> debbugs.gnu.org
Subject: Re: bug#78042: 31.0.50; Improper sequence of
 `before/after-change-functions` calls
Date: Sat, 26 Apr 2025 12:35:43 -0400

>> My request for comments/objection was not about whether it is worth
>> fixing this bug (there is no doubt about that on my side: it affects at
>> least Eglot and CC-mode), but whether the approach I used is acceptable
>> or if you have a better alternative to propose.
> The only alternative I can think of is to be able to handle nested
> notifications, such as what happens in this case, as correct.  What
> are the problems due to which we must have paired notifications
> without nesting?

The problem is that it would change the semantics of
`after/before-change-functions`.  E.g. code that needs to match the
before with the after (e.g. CC-mode, Eglot, ...) would now need to be
adjusted to keep a stack of "pending befores" and the C code would need
to be reviewed to try and catch the few places where we currently run
`after-change-functions` several times after running
`before-change-functions` once.

IOW, we'd trade one set of corner-case breakage for another.


        Stefan

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78042 <at> debbugs.gnu.org
Subject: Re: bug#78042: Improper sequence of `before/after-change-functions`
 calls
Date: Sat, 26 Apr 2025 13:27:00 -0400

>> The function where I made the modification inserts bytes into
>> the buffer but without running the hooks.
>> So it is *already* the case that the callers must run the hooks
>> themselves.
>
> I don't understand: if you made the changes (by removing the calls to
> modification hooks), then the function originally _was_ calling them,
> no?

Here's what currently happens:

- the caller calls `before-change-functions`.
- it calls `decode_coding`:
  - in all cases, if dst_object is a buffer, `decode_coding` inserts
    bytes into that buffer.
  - in no case does `decode_coding` take charge to run
    the `before/after-change-functions` needed for those insertions.
  - in some relatively rare cases, `decode_coding` adds some
    text-properties causing nested `before/after-change-functions` calls.
    This is always done on the just-inserted text, so it's *always*
    nested (aka redundant, and harmful).
- the caller then calls `after-change-functions`.

The patch I sent binds `Qinhibit_modification_hooks` to t during the
execution of `decode_coding`, so it affects only nested calls to
`before/after-change-functions` (which apply only to some cases and only
to some parts of the inserted text), not the "normal" calls which are
always performed outside of that function.

> The problem with such ad-hoc evidence is that it relies on the
> assumption that your code and the "usual" cases everyone runs do
> exercise all of the relevant code paths.  Such assumptions are not
> very reliable in Emacs, IME.

No, the evidence is simply that `decode_coding` does not itself do
anything to run the change functions to reflect the text insertion.
So the inhibition that my patch does can affect only those redundant
nested runs.

> But the "usual-case" notification is about the entire decoded text,
> whereas the notifications your patch blocks are about smaller chunks
> of text for specific kinds of changes, and thus more fine-grained.  So
> the hooks will need to do a more complex job now: they cannot rely on
> the fact that notifications for decoding are separate from
> notifications about text-property changes, so they will need to
> examine the entire decoded region of text and decide what to do with
> each chunk of it.

No, any function placed on those hooks has to deal with a wild array of
other cases and simply can never know whether they're called for
text-property changes or anything else.

Instead, (the coders of) those functions always have to expend a great
effort to try and make as few assumptions as possible because those
assumptions *always* end up being broken in one case or another.

> Or maybe we should not install this at all?  What are the problems
> these "nested" notifications cause, again?

I've seen a backtrace in CC-mode (tho I haven't yet been able to figure
out how to reproduce it: it depends on the state of the CC-mode cache),
it causes Eglot to throw its hands up and say "how well, Emacs messed
up, let's start over", etc...

I guess next time I'll just push my patch silently to avoid having to
argue why we should fix our bugs,


        Stefan

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 78042 <at> debbugs.gnu.org
Subject: Re: bug#78042: 31.0.50; Improper sequence of
 `before/after-change-functions` calls
Date: Sat, 26 Apr 2025 21:41:04 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: 78042 <at> debbugs.gnu.org
> Date: Sat, 26 Apr 2025 12:35:43 -0400
> 
> >> My request for comments/objection was not about whether it is worth
> >> fixing this bug (there is no doubt about that on my side: it affects at
> >> least Eglot and CC-mode), but whether the approach I used is acceptable
> >> or if you have a better alternative to propose.
> > The only alternative I can think of is to be able to handle nested
> > notifications, such as what happens in this case, as correct.  What
> > are the problems due to which we must have paired notifications
> > without nesting?
> 
> The problem is that it would change the semantics of
> `after/before-change-functions`.  E.g. code that needs to match the
> before with the after (e.g. CC-mode, Eglot, ...) would now need to be
> adjusted to keep a stack of "pending befores" and the C code would need
> to be reviewed to try and catch the few places where we currently run
> `after-change-functions` several times after running
> `before-change-functions` once.
> 
> IOW, we'd trade one set of corner-case breakage for another.

Except that making us prepared to handle nested notifications will
solve the problem for good.  IOW, fixing _that_ breakage has hope to
fix the problem completely, whereas fixing each case we find as we go
is just an endless rear-guard battle, and those can never be won.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 78042 <at> debbugs.gnu.org
Subject: Re: bug#78042: Improper sequence of `before/after-change-functions`
 calls
Date: Sat, 26 Apr 2025 21:43:39 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: 78042 <at> debbugs.gnu.org
> Date: Sat, 26 Apr 2025 13:27:00 -0400
> 
> I guess next time I'll just push my patch silently to avoid having to
> argue why we should fix our bugs,

That's not fair on so many levels that I don't even know where to
start.

But whatever, have it your way.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 78042 <at> debbugs.gnu.org
Subject: Re: bug#78042: 31.0.50; Improper sequence of
 `before/after-change-functions` calls
Date: Sat, 26 Apr 2025 19:22:42 -0400

>> The problem is that it would change the semantics of
>> `after/before-change-functions`.  E.g. code that needs to match the
>> before with the after (e.g. CC-mode, Eglot, ...) would now need to be
>> adjusted to keep a stack of "pending befores" and the C code would need
>> to be reviewed to try and catch the few places where we currently run
>> `after-change-functions` several times after running
>> `before-change-functions` once.
>> 
>> IOW, we'd trade one set of corner-case breakage for another.
>
> Except that making us prepared to handle nested notifications will
> solve the problem for good.

I don't see why it'll be easier to find all the cases where we currently
call `after-change-functions` several times.

But maybe you're right.  It'll still be a breaking change, so it might
require introducing new hooks to replace
`after/before-change-functions`.  In the mean time, I'll keep fixing the
problems I bump into.


        Stefan

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: 78042-done <at> debbugs.gnu.org
Subject: Re: bug#78042: Improper sequence of `before/after-change-functions`
 calls
Date: Fri, 02 May 2025 17:07:26 -0400

> The code that runs those hooks is really littered at odd places, with no
> good/standard way to control when to run it and when not.
> I suggest the patch below (which includes a corresponding test).
> Any objection?

Pushed, closing,


        Stefan

Previous Next

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