bug#78042: Improper sequence of `before/after-change-functions` calls

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 78042@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?