Re: Unbalanced change hooks (part 2)

[Alan Mackenzie]

Hello, Eli.

On Mon, Aug 01, 2016 at 04:07:50PM +0300, Eli Zaretskii wrote:
> > Date: Sun, 31 Jul 2016 21:26:35 +0000
> > Cc: address@hidden, address@hidden, address@hidden
> > From: Alan Mackenzie <address@hidden>

[ .... ]

> > I'm in favour of fixing what is wrong, rather than implementing
> > workarounds somewhere else.  Workarounds are rarely fully satisfactory.

> I'm not suggesting a workaround.  I'm suggesting a fix for a design
> flaw in CC Mode -- its assumption that before-change-functions and
> after-change-functions are always invoked in balanced pairs.  CC Mode
> must not depend on that assumption, because it's false.

The flaw lies between the Emacs core and the Elisp manual.  The latter
quite clearly implies that each buffer change is accompanied by a single
before-change-functions call and a single after-change-functions call.

If Emacs is not going to be fixed to match its spec, then the doc will
need to be changed to match Emacs.  It will need to say, in some fashion,
that the two change hooks are called indeterminately.

[ .... ]

> In practice, we need to make sure that every modification triggers at
> least one call to before-change-functions and at least one call to
> after-change-functions.  The numbers of these calls do not have to
> match.

And in such calls, what are the parameters BEG, END, and OLD-LEN supposed
to mean?

> I had my share of hacking that code [insdel.c] and its callers.  It's
> impossible to convey everything I learned while doing that in a
> discussion such as this one.  I say more about this below.  If that's
> still not enough, you will have to trust me on that.

OK.  In exchange, I'm asking you to trust me when I say that the sort of
change you're expecting in CC Mode is impractical - months of work,
rather than weeks.

[ .... ]

> Just don't assume that there will be a c-before-change call for each
> c-after-change call, that's all.

What you seem to be saying is that before-change-functions isn't very
useful.  Since lots of packages use it, you seem to be saying that these
packages are broken, just as CC Mode is broken.

> Specifically, in the case in point, I believe that the "unbalanced"
> call to c-after-change clearly tells you that text has been deleted,
> so I think it should be enough for you to do the job of the "missing"
> c-before-change call as well.

No.  CC Mode also needs to know what was there before the change.  It
cannot guess.  I think it's fair to ask you to suggest a way for CC Mode
to get that deleted buffer section, somehow.

It is a perfectly reasonable requirement for a generic piece of Lisp code
to be able to get full details of any buffer change.

I've had a few wierd ideas myself on this, one of the first being that CC
Mode will maintain a shadow buffer for each of its buffers.  The shadow
will be identical to the main buffer in all ways, apart from when changes
are under way.  After a buffer change, CC Mode will consult the shadow
buffer to see what used to be there, and then copy the change from the
main buffer to keep it up to date.  I don't think this is a good way to
go, so I'm asking you to suggest something better.

> > > I said "basically unchanged".  Trivial refactoring doesn't count.  And
> > > any changes in this function were in response to issues common to all
> > > the callers.  The case in point isn't such a case.

> > What I am proposing is only a little more than trivial refactoring.

> No, it's far from that.  It fundamentally changes the list of things
> that are done in many operations that modify buffers in various ways,
> and the order in which these things are done.  We shouldn't make such
> changes at this point in Emacs development, unless we identify a clear
> bug that affects all the callers of these functions.

> This case is not such a case.  It is a case of a single Lisp package
> that made incorrect assumptions about the way the modification hooks
> are called.  So the right place to fix this is in that Lisp package.

Here is a list of lisp files which use before-change-functions.
Currently all of them are broken due to the unreliability of
before-change-functions:

./wid-edit.el:1292:  (add-hook 'before-change-functions 'widget-before-change 
nil t)
./emacs-lisp/syntax.el:495:             (add-hook 'before-change-functions
./whitespace.el:2184:    (add-hook 'before-change-functions 
#'whitespace-buffer-changed nil t)
./allout-widgets.el:594:        (add-hook 'before-change-functions 
'allout-widgets-before-change-handler
./allout-widgets.el:741:          (add-hook 'before-change-functions
./obsolete/lazy-lock.el:591:    (add-hook 'before-change-functions 
'lazy-lock-arrange-before-change nil t))
./org/org-table.el:3954:  (add-hook 'before-change-functions 
'org-table-remove-rectangle-highlight))
./allout.el:2049:      (add-hook 'before-change-functions 
'allout-before-change-handler nil t)
./progmodes/cc-mode.el:641:  (add-hook 'before-change-functions 
'c-before-change nil t)
./progmodes/js.el:3740:  (add-hook 'before-change-functions #'js--flush-caches 
t t)
./progmodes/compile.el:2115:  (add-hook 'before-change-functions 
'compilation--flush-parse nil t)
./cedet/semantic/grammar.el:1347:  (add-hook 'before-change-functions
./textmodes/tex-mode.el:707:      (add-hook 'before-change-functions
./textmodes/sgml-mode.el:926:        (add-hook 'before-change-functions
./emulation/viper-cmd.el:2307:  (add-hook 'before-change-functions 
'viper-before-change-sentinel t)

[ .... ]

> > I'm still trying to work out what they are, beyond a generalised
> > opposition to any change at this level of the code.

> Without a good reason, yeah, that's pretty much it.

> It's not some paranoia, mind you: I've been working on fixing quite a
> few subtle bugs which originated due to such changes for the last few
> years, so I know what I'm talking about.  Please trust my experience
> and the conclusions I drew from it.

Please tell me how CC Mode can get full details of any buffer change,
then I'll be (reasonably) happy.

[ .... ]

> What I'd like you to try instead is eliminate from CC Mode the
> incorrect assumption about pairwise calls to before- and
> after-change-functions.  That is the correct way towards solving this
> problem.

CC Mode needs full details of all buffer changes.  Redesigning it so that
it doesn't doesn't bear thinking about - it would be far more work than
making the change hooks get called properly.

How about the following idea: we deprecate before-change-functions, which
is broken anyway, and isn't going to get fixed.  Functions in
after-change-functions would somehow get access to the old deleted
portion of the buffer, complete with text properties, overlays, ...?
This shouldn't be too hard, given that such stuff is stored in the undo
list anyway, even though in an unsuitable form.

> > Programs other than CC Mode use before-change-functions.  It would be
> > more effort to construct an actual failure than simply to fix the
> > problem.

> But before-change-functions _are_ being called in the scenario of the
> bugs we are discussing.  They just aren't balanced with calls to
> after-change-functions, that's all.

No.  In the scenario of bug #24094 there was no call to
before-change-functions whatsoever.  (Discovered by instrumenting
c-before-change and c-after-change for edebug.)

> See, in this fragment of insert-file-contents:

>       if (same_at_end != same_at_start)
>       {
>         invalidate_buffer_caches (current_buffer,
>                                   BYTE_TO_CHAR (same_at_start),
>                                   same_at_end_charpos);
>         del_range_byte (same_at_start, same_at_end, 0);
>         temp = GPT;
>         eassert (same_at_start == GPT_BYTE);
>         same_at_start = GPT_BYTE;
>       }
>       else
>       {
>         temp = same_at_end_charpos;
>       }
>       /* Insert from the file at the proper position.  */
>       SET_PT_BOTH (temp, same_at_start);
>       same_at_start_charpos
>       = buf_bytepos_to_charpos (XBUFFER (conversion_buffer),
>                                 same_at_start - BEGV_BYTE
>                                 + BUF_BEG_BYTE (XBUFFER (conversion_buffer)));
>       eassert (same_at_start_charpos == temp - (BEGV - BEG));
>       inserted_chars
>       = (buf_bytepos_to_charpos (XBUFFER (conversion_buffer),
>                                  same_at_start + inserted - BEGV_BYTE
>                                 + BUF_BEG_BYTE (XBUFFER (conversion_buffer)))
>          - same_at_start_charpos);
>       /* This binding is to avoid ask-user-about-supersession-threat
>        being called in insert_from_buffer (via in
>        prepare_to_modify_buffer).  */
>       specbind (intern ("buffer-file-name"), Qnil);
>       insert_from_buffer (XBUFFER (conversion_buffer),
>                         same_at_start_charpos, inserted_chars, 0);

> the before-change-functions are called from insert_from_buffer.  So
> it's not like the replacement would be missed by the
> before-change-functions.

It was missed in the recipe for bug #24094.  For an atomic user action
(namely C-x C-f test.c++ followed by typing "yes" to the prompt to reload
the file) there was no invocation of before-change-functions and one of
after-change-functions.  This is the bug I am complaining of.

[ .... ]

> > > > It should calculate the two variables c-new-BEG and c-new-END (which
> > > > bound the active region) in c-before-change, and these get modified in
> > > > c-after-change.

> > > Sorry, I don't understand.  Why can't you compute these two values in
> > > the c-after-change?

> > In the general case because information gets lost at a change; things
> > like where the beginning of a statement was, where deleted template
> > delimiters used to be, what the bounds of a macro or string used to be
> > (before backslashes were deleted).

> I'm sure CC Mode is well equipped to recalculate all of these if
> needed.  It does that when a file is first visited.

An empty buffer is a fully defined state.  A buffer immediately following
a deletion is in an undefined state, which requires to know what was
deleted to make it defined again.

[ .... ]

> > Because a proper fix is the Right Thing to do.

> Well, we clearly disagree about that.  I think there's no problem in
> insdel.c or its callers, and the cause of this bug is the incorrect
> assumption in CC Mode that before- and after-change-functions will be
> called in balanced pairs.  So from my POV, TRT would be to eliminate
> that assumption, which should make CC Mode more robust in the long
> run, as I'm not sure this is the only case where that assumption
> breaks.

Please provide some means by which CC Mode has access to the full details
of a buffer change.  That's all I'm asking for now (even if I'm getting a
bit repetitive).

[ .... ]

> Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).