Syntax-ppss flush should happen in after-change hook

Syntax-ppss flush should happen in after-change hook

[Vitalie Spinu]

Hi,

Currently ppss flush happens in before-change hook. This leads to a real
possibility that syntax-propertize is called before the actual change and not
refreshed after the change. This could occur in at least two scenarios.

First, when a hook placed after syntax-ppss-flush-cache in
before-change-functions invokes `syntax-propertize` (for example with a
search). Second is when the actual function responsible for a change invokes
`syntax-propertize`.

Here is a real case of uncomment-region in markdown-mode:

  markdown-syntax-propertize(63 823)
  syntax-propertize(823)
  internal--syntax-propertize(64)
  parse-partial-sexp(63 #<marker at 77 in test.tmd> nil nil nil t)
  comment-search-forward(#<marker at 77 in test.tmd> t)
  uncomment-region-default-1(63 77 nil)
  #f(compiled-function () #<bytecode 0x280781d>)()
  combine-change-calls-1(63 77 #f(compiled-function () #<bytecode 0x280781d>))
  uncomment-region-default(63 77 nil)
  uncomment-region(63 77 nil)

As markdown mode caches its syntax properties during syntax-propertize which are
subsequently used by font-lock, the font-lock eventually uses "pre-change"
properties during the fortification. This results in all sort of hard to trace
bugs (mostly end of buffer errors).

I think placing the flush in after-change-functions would solve most of core of
the issue leaving the negligible possibility that there will be a hook before
the flush which would break on outdated properties.


  Vitalie

Re: Syntax-ppss flush should happen in after-change hook

[Stefan Monnier]

> Currently ppss flush happens in before-change hook. This leads to a real
> possibility that syntax-propertize is called before the actual change and not
> refreshed after the change. This could occur in at least two scenarios.
>
> First, when a hook placed after syntax-ppss-flush-cache in
> before-change-functions invokes `syntax-propertize` (for example with a
> search).

If syntax-ppss-flush-cache is on after-change-functions then the risk is
when a hook function is placed before it.  Given the fact that add-hook
defaults to placing functions at the head and that
after-change-functions are much more commonly used than
before-change-functions, the risks associated with placing it on
after-change-functions are at least as high.

FWIW, syntax-ppss-flush-cache was originally placed on
after-change-functions and later changed to before-change-functions
because of exactly that problem.

> Second is when the actual function responsible for a change invokes
> `syntax-propertize`.
>
> Here is a real case of uncomment-region in markdown-mode:
>
>   markdown-syntax-propertize(63 823)
>   syntax-propertize(823)
>   internal--syntax-propertize(64)
>   parse-partial-sexp(63 #<marker at 77 in test.tmd> nil nil nil t)
>   comment-search-forward(#<marker at 77 in test.tmd> t)
>   uncomment-region-default-1(63 77 nil)
>   #f(compiled-function () #<bytecode 0x280781d>)()
>   combine-change-calls-1(63 77 #f(compiled-function () #<bytecode 0x280781d>))
>   uncomment-region-default(63 77 nil)
>   uncomment-region(63 77 nil)

I do see that combine-change-calls is involved and indeed
combine-change-calls seems just fundamentally problematic in this
respect (the problem would appear as well if we moved
syntax-ppss-flush-cache on after-change-functions since it would mean
that the syntax-ppss cache is left inconsistent during the execution of
its body).

The interaction between the syntax-ppss cache (and the syntax-propertize
properties as well) and the changes that can take place during
combine-change-calls can't be done right with a coarse "run something at
the beginning and something else at the end".

A crude solution could be the ad-hoc hack below.  We could make it less
ad-hoc by adding a way for any hook function (on b-c-f and a-c-f) to
indicate whether it should stay active within combine-change-calls,
e.g. with a symbol property `do-not-combine-change-function`.


        Stefan


diff --git a/lisp/subr.el b/lisp/subr.el
index 3a901b3174..abdc116e5f 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3834,7 +3834,10 @@ combine-change-calls-1
 	(if (eq buffer-undo-list t)
 	    (setq result (funcall body))
 	  (let (;; (inhibit-modification-hooks t)
-                before-change-functions after-change-functions)
+                (before-change-functions
+                 (if (memq #'syntax-ppss-flush-cache before-change-functions)
+                     '(syntax-ppss-flush-cache)))
+                after-change-functions)
 	    (setq result (funcall body)))
 	  (let ((ap-elt
 		 (list 'apply

Re: Syntax-ppss flush should happen in after-change hook

[Vitalie Spinu]

>> On Sat, Mar 16 2019 11:53, Stefan Monnier wrote:

>> Currently ppss flush happens in before-change hook. This leads to a real
>> possibility that syntax-propertize is called before the actual change and not
>> refreshed after the change. This could occur in at least two scenarios.
>>
>> First, when a hook placed after syntax-ppss-flush-cache in
>> before-change-functions invokes `syntax-propertize` (for example with a
>> search).

> If syntax-ppss-flush-cache is on after-change-functions then the risk is
> when a hook function is placed before it.  

So it's inconsistent state for the hooks vs inconsistent state for all the emacs
functions after the change.

> Given the fact that add-hook defaults to placing functions at the head and
> that after-change-functions are much more commonly used than
> before-change-functions

One simple solution is to add a sentinel to the before-change-functions to
explicitly check for syntax-ppss-flush-chache being the first hook:

  (unless (eq (car after-change-functions) 'syntax-ppss-flush-cache)
    (delq 'syntax-ppss-flush-cache after-change-functions)
    (add-hook 'after-change-functions 'syntax-ppss-flush-cache nil t))

> A crude solution could be the ad-hoc hack below.  We could make it less
> ad-hoc by adding a way for any hook function (on b-c-f and a-c-f) to
> indicate whether it should stay active within combine-change-calls,
> e.g. with a symbol property `do-not-combine-change-function`.

AFAICS this will not help with the main part of the problem (second scenario in
OP). The problem is not that some inconsistent state happens during the
combine-change-calls but that after the change ppss is left in inconsistent
state. Combine-change-calls happened in my example, but currently any buffer
change results in inconsistent syntax state.

As far as I understand, just before the actual change the cache is in consistent
state, so currently, if the changing function requires syntax properties then
before-change flush enforces extra syntax-propertize for nothing. In my example
that search need not have had triggered syntax-propertize would the flush not
have had happened.


  Vitalie

Re: Syntax-ppss flush should happen in after-change hook

[Stefan Monnier]

> One simple solution is to add a sentinel to the before-change-functions to
> explicitly check for syntax-ppss-flush-chache being the first hook:
>
>   (unless (eq (car after-change-functions) 'syntax-ppss-flush-cache)
>     (delq 'syntax-ppss-flush-cache after-change-functions)
>     (add-hook 'after-change-functions 'syntax-ppss-flush-cache nil t))

That would require using both b-f-c and a-f-c.
I'd much rather "do it right" and change add-hook to understand a notion
of depth/priority/order.

>> A crude solution could be the ad-hoc hack below.  We could make it less
>> ad-hoc by adding a way for any hook function (on b-c-f and a-c-f) to
>> indicate whether it should stay active within combine-change-calls,
>> e.g. with a symbol property `do-not-combine-change-function`.
>
> AFAICS this will not help with the main part of the problem (second
> scenario in OP).  The problem is not that some inconsistent state
> happens during the combine-change-calls but that after the change ppss
> is left in inconsistent state.

I believe it does help the problem at hand: the final state of the cache
will be correct (and the state of the cache is correct during the
combine-change-calls as well).

> Combine-change-calls happened in my example, but currently any buffer
> change results in inconsistent syntax state.

Then I must have misunderstood something.
Can you give more details about the problem scenario you have in mind?

> As far as I understand, just before the actual change the cache is in
> consistent state, so currently, if the changing function requires
> syntax properties then

At the level of b-c-f and a-c-f the only "changing functions" are things
like `insert` or `delete-region` and they should not trigger
syntax-propertize or syntax-ppss (other than via b-c-f and a-c-f,
obviously).

It's only when you start using combine-change-calls (or
combine-after-change-calls) that the code the performs the change
between a-c-f and b-c-f can run arbitrarly fancy Elisp code.

> before-change flush enforces extra syntax-propertize for nothing. In
> my example that search need not have had triggered syntax-propertize
> would the flush not have had happened.

But the search is not done as part of the "change function", so (modulo
combine-change-calls) it either done before b-c-f or after a-c-f, never
between the two.


        Stefan

Re: Syntax-ppss flush should happen in after-change hook

[Vitalie Spinu]

>> On Sat, Mar 16 2019 14:09, Stefan Monnier wrote:

> I'd much rather "do it right" and change add-hook to understand a notion of
> depth/priority/order.

Interesting. This would be useful for this particular case, but are there other
use cases that would benefit from it?

>> Combine-change-calls happened in my example, but currently any buffer
>> change results in inconsistent syntax state.

> Then I must have misunderstood something.
> Can you give more details about the problem scenario you have in mind?

Sorry, I didn't put it rightly. I meant all those functions that trigger
"syntax-propertize", but looks like I was actually confused about the number of
such functions.

> It's only when you start using combine-change-calls (or
> combine-after-change-calls) that the code the performs the change
> between a-c-f and b-c-f can run arbitrarly fancy Elisp code.

I see. Then, as long as combine-change-calls leaves the correct state everything
should be fine. Then your patch looks right, indeed. It does fix my
markdown-mode example for sure.

  Vitalie

Re: Syntax-ppss flush should happen in after-change hook

[Stefan Monnier]

>> I'd much rather "do it right" and change add-hook to understand a notion of
>> depth/priority/order.
>
> Interesting. This would be useful for this particular case, but are
> there other use cases that would benefit from it?

Since it's proved useful for add-function, I expect it would be useful
for add-hook as well.  Off hand, I can only mention
electric--sort-post-self-insertion-hook as a concrete example, tho.

>>> Combine-change-calls happened in my example, but currently any buffer
>>> change results in inconsistent syntax state.
>
>> Then I must have misunderstood something.
>> Can you give more details about the problem scenario you have in mind?
>
> Sorry, I didn't put it rightly.  I meant all those functions that
> trigger "syntax-propertize", but looks like I was actually confused
> about the number of such functions.
>
>> It's only when you start using combine-change-calls (or
>> combine-after-change-calls) that the code the performs the change
>> between a-c-f and b-c-f can run arbitrarly fancy Elisp code.
>
> I see. Then, as long as combine-change-calls leaves the correct state
> everything should be fine.  Then your patch looks right, indeed.
> It does fix my markdown-mode example for sure.

OK, thanks for testing,


        Stefan