Subject: Re: bug#78221: 31.0.50; Improving *-change-functions notifications

From: Stefan Monnier <[email-masked]>
Date: Sat, 03 May 2025 11:14:28 -0400

>> Alan Mackenzie made significant improvements in the past to the C code
>> that runs these hooks, so that we now rarely break our promises in terms
>> of how/when those notifications are sent, but there are still
>> cases lurking.
> By "those promises", I think you mean being that we invoke
> before-change-functions and after-change-functions strictly in matched
> pairs.

No, I mean the looser promise that AFTERs are allowed to touch only text
within the region described by the last BEFORE.

> On 2018-01-06, we formally agreed that these hooks will be invoked as
> follows:
>   o - Every buffer changing primitive starts off with exactly one call
>     to b-c-f,
>   o - after which there will be zero, one, or several calls to a-c-f.
>   o - The b-c-f's region will enclose those of all the a-c-f regions,
>     but need not be the minimal such region.

Exactly.  And it's now documented in the manual.

> I actually think a good strategy would be to invoke these hooks
> strictly in matching pairs in all cases, but I'm sure I said that back
> in 2018 too.

🙂

>> The main problem is that it's very hard to find the places where this
>> occurs and it's not always obvious that the corresponding fix is
>> harmless.  A fix is likely to involve changes visible to the Elisp side,
>> such as new hooks, so it seemed out of scope for bug#78042.
>
> I don't think it's all that hard.  In 2020, I made a list of which
> non-static functions in insdel.c call before/after-change-functions.
> The list, which is likely still valid, or very nearly so, looked like
> this:
>
> insert                             B/A
> insert_and_inherit                 B/A
> insert_char                        B/A
> insert_string                      B/A
> insert_before_markers              B/A
>
> insert_before_markers_and_inherit  B/A
> insert_1_both                      ?/-
> insert_from_string                 B/A
> insert_from_string_before_markers  B/A
> insert_from_gap_1                  -/-
>
> insert_from_gap                    -/-
> insert_from_buffer                 B/A
> del_range                          B/A
> del_range_1                        ?/A
> del_range_byte                     B/A
>
> del_range_both                     ?/A
> del_range_2                        -/-
> replace_range                      ?/A
> replace_range_2                    -/-
> modify_text                        B/-

Thanks, that's very helpful.
[ It lacks the ones coming from text-property modifications.  ]

The ones above that are neither B/A nor -/- are the reason why making
changes in this area is ... delicate.

> But if an alternative is to allow nested calls to the hooks (something
> we don't allow at the moment, even though the rule isn't explicitly
> formulated anywhere),

The rule is implicitly formulated where we document that AFTERs can't
touch text outside of the last BEFORE.

> I think it was a mistake in the beginning to have text property
> modifications trigger before/after-c-f.

Agreed.

> But seeing it's been that way for so long, I don't think it would be
> a good idea to change it now.

FWIW, I'm now running my Emacs with such a change to see if the ELisp
code I use cares.  But yeah, it seems too risky to do it without
providing some backward compatibility.

> before/after-change-functions functions SHOULDN'T themselves make
> changes to the buffer text (apart from text properties).

+1

> I don't think it would be all that hard to instrument insdel.c to
> signal an error when such changes do get made, just tedious.
> Running such an instrumented Emacs on the test suite would catch
> a very great number of violations.

It might be worthwhile adding such runtime tests and emit warnings when
before/after-change-functions make changes the buffer text, tho it's
orthogonal to what we're discussing here (where the nesting is not due
to ill-behaved before/after-change-functions but come straight from the
C code).

> As I proposed in my reply to Stefan M., I don't think finding most/all of
> these situations need be too difficult.  insdel.c could be temporarily
> instrumented to signal an error on detecting an illegitimate buffer
> change.  The difficulty here might be false positives, when a program
> intends to make buffer changes from inside a change hook function.  I
> don't think these are common, though.

I'm running my Emacs sessions with a function added globally to
before/after-change-functions, which checks that we stick to our
promises (basically the `sanity-check-change-functions-*` code in
`test/src/editfns-tests.el`).

But that catches only those occurrences that are triggered by my
usage patterns.

        Stefan