bug#36136: syntax-ppss fails to invalidate its cache on changes to syntax-table text properties

bug#36136: syntax-ppss fails to invalidate its cache on changes to syntax-table text properties

[Alan Mackenzie]

Hello, Emacs.

The syntax-ppss cache is not invalidated when syntax-table text
properties are set or cleared.  This is because the invalidation
function, syntax-ppss-flush-cache is invoked only as a before-change
function, but typical (?all) syntax-table property changes happen when
before-change-functions is inactive.

This is a bug.

In my debugging of a CC Mode scenario, a buffer change causes a
syntax-table text property change at an earlier part of the buffer.  This
is to do with the change making previously non-matching C++ raw string
identifiers match up.  Font lock follows the syntax-ppss state, which
spuriously records that the end part of the buffer is still in a string.
Hence the non-string part of the buffer is still fontified with
font-lock-string-face.

Suggested fix: the functions set_properties, add_properties,
remove_properties in textprop.c should check for changes to,
specifically, syntax-table properties.  When these changes are detected,
a hook called something like syntax-table-props-change-alert-hook should
be called (with some appropriate position parameters, tbd).
syntax-ppss-flush-cache will be added to this hook.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#36136: [PATCH]: Re: bug#36136: syntax-ppss fails to invalidate its cache on changes to syntax-table text properties

[Alan Mackenzie]

Hello, Emacs.

On Sat, Jun 08, 2019 at 13:17:24 +0000, Alan Mackenzie wrote:
> The syntax-ppss cache is not invalidated when syntax-table text
> properties are set or cleared.  This is because the invalidation
> function, syntax-ppss-flush-cache is invoked only as a before-change
> function, but typical (?all) syntax-table property changes happen when
> before-change-functions is inactive.

> This is a bug.

> In my debugging of a CC Mode scenario, a buffer change causes a
> syntax-table text property change at an earlier part of the buffer.  This
> is to do with the change making previously non-matching C++ raw string
> identifiers match up.  Font lock follows the syntax-ppss state, which
> spuriously records that the end part of the buffer is still in a string.
> Hence the non-string part of the buffer is still fontified with
> font-lock-string-face.

> Suggested fix: the functions set_properties, add_properties,
> remove_properties in textprop.c should check for changes to,
> specifically, syntax-table properties.  When these changes are detected,
> a hook called something like syntax-table-props-change-alert-hook should
> be called (with some appropriate position parameters, tbd).
> syntax-ppss-flush-cache will be added to this hook.

Here is a first draught of a fix to this bug.

It creates a new abnormal hook,
syntax-table-props-change-alert-functions, whose member functions are
called each time a syntax-table text property is changed.

syntax-ppss-flush-cache is inserted into this new hook at loading time
for syntax.el, thus invalidating the syntax-ppss cache each time a
syntax-table text property is changed.

There is a consequential amendment to the doc string of
inhibit-modification-hooks.

This fixes the original problem.

Comments?


diff --git a/lisp/emacs-lisp/syntax.el b/lisp/emacs-lisp/syntax.el
index 9c6d5b5829..673d598de6 100644
--- a/lisp/emacs-lisp/syntax.el
+++ b/lisp/emacs-lisp/syntax.el
@@ -43,6 +43,10 @@
 
 (eval-when-compile (require 'cl-lib))
 
+;; Ensure that the cache gets invalidated when `syntax-table' text
+;; properties get set, even when `inhibit-modification-hooks' is non-nil.
+(add-hook 'syntax-table-props-change-alert-functions #'syntax-ppss-flush-cache)
+
 ;;; Applying syntax-table properties where needed.
 
 (defvar syntax-propertize-function nil
diff --git a/src/insdel.c b/src/insdel.c
index 85fffd8fd1..5e978d1b29 100644
--- a/src/insdel.c
+++ b/src/insdel.c
@@ -2360,9 +2360,11 @@ syms_of_insdel (void)
   Vcombine_after_change_calls = Qnil;
 
   DEFVAR_BOOL ("inhibit-modification-hooks", inhibit_modification_hooks,
-	       doc: /* Non-nil means don't run any of the hooks that respond to buffer changes.
+	       doc: /* Non-nil means don't run most of the hooks that respond to buffer changes.
 This affects `before-change-functions' and `after-change-functions',
-as well as hooks attached to text properties and overlays.
+as well as most hooks attached to text properties and overlays.
+However the hook `syntax-table-props-change-alert-functions' gets run
+regardless of the setting of this variable.
 Setting this variable non-nil also inhibits file locks and checks
 whether files are locked by another Emacs session, as well as
 handling of the active region per `select-active-regions'.  */);
diff --git a/src/textprop.c b/src/textprop.c
index ae42c44185..050bb7b435 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -334,7 +334,10 @@ set_properties (Lisp_Object properties, INTERVAL interval, Lisp_Object object)
 	    record_property_change (interval->position, LENGTH (interval),
 				    XCAR (sym), XCAR (value),
 				    object);
-	  }
+            if (EQ (sym, Qsyntax_table))
+              CALLN (Frun_hook_with_args, Qsyntax_table_props_change_alert_functions,
+                     make_fixnum (interval->position));
+          }
 
       /* For each new property that has no value at all in the old plist,
 	 make an undo record binding it to nil, so it will be removed.  */
@@ -346,6 +349,9 @@ set_properties (Lisp_Object properties, INTERVAL interval, Lisp_Object object)
 	    record_property_change (interval->position, LENGTH (interval),
 				    XCAR (sym), Qnil,
 				    object);
+            if (EQ (sym, Qsyntax_table))
+              CALLN (Frun_hook_with_args, Qsyntax_table_props_change_alert_functions,
+                     make_fixnum (interval->position));
 	  }
     }
 
@@ -400,7 +406,10 @@ add_properties (Lisp_Object plist, INTERVAL i, Lisp_Object object,
 	      {
 		record_property_change (i->position, LENGTH (i),
 					sym1, Fcar (this_cdr), object);
-	      }
+                if (EQ (sym1, Qsyntax_table))
+                  CALLN (Frun_hook_with_args, Qsyntax_table_props_change_alert_functions,
+                         make_fixnum (i->position));
+              }
 
 	    /* I's property has a different value -- change it */
 	    if (set_type == TEXT_PROPERTY_REPLACE)
@@ -436,6 +445,9 @@ add_properties (Lisp_Object plist, INTERVAL i, Lisp_Object object,
 	    {
 	      record_property_change (i->position, LENGTH (i),
 				      sym1, Qnil, object);
+              if (EQ (sym1, Qsyntax_table))
+                CALLN (Frun_hook_with_args, Qsyntax_table_props_change_alert_functions,
+                       make_fixnum (i->position));
 	    }
 	  set_interval_plist (i, Fcons (sym1, Fcons (val1, i->plist)));
 	  changed = true;
@@ -470,9 +482,14 @@ remove_properties (Lisp_Object plist, Lisp_Object list, INTERVAL i, Lisp_Object
       while (CONSP (current_plist) && EQ (sym, XCAR (current_plist)))
 	{
 	  if (BUFFERP (object))
-	    record_property_change (i->position, LENGTH (i),
-				    sym, XCAR (XCDR (current_plist)),
-				    object);
+            {
+              record_property_change (i->position, LENGTH (i),
+                                      sym, XCAR (XCDR (current_plist)),
+                                      object);
+              if (EQ (sym, Qsyntax_table))
+                CALLN (Frun_hook_with_args, Qsyntax_table_props_change_alert_functions,
+                       make_fixnum (i->position));
+            }
 
 	  current_plist = XCDR (XCDR (current_plist));
 	  changed = true;
@@ -486,8 +503,13 @@ remove_properties (Lisp_Object plist, Lisp_Object list, INTERVAL i, Lisp_Object
 	  if (CONSP (this) && EQ (sym, XCAR (this)))
 	    {
 	      if (BUFFERP (object))
-		record_property_change (i->position, LENGTH (i),
-					sym, XCAR (XCDR (this)), object);
+                {
+                  record_property_change (i->position, LENGTH (i),
+                                          sym, XCAR (XCDR (this)), object);
+                  if (EQ (sym, Qsyntax_table))
+                    CALLN (Frun_hook_with_args, Qsyntax_table_props_change_alert_functions,
+                           make_fixnum (i->position));
+                }
 
 	      Fsetcdr (XCDR (tail2), XCDR (XCDR (this)));
 	      changed = true;
@@ -2319,6 +2341,20 @@ inherits it if NONSTICKINESS is nil.  The `front-sticky' and
   Vtext_property_default_nonsticky
     = list2 (Fcons (Qsyntax_table, Qt), Fcons (Qdisplay, Qt));
 
+  DEFVAR_LISP ("syntax-table-props-change-alert-functions",
+               Vsyntax_table_props_change_alert_functions,
+               doc: /* List of functions to call each time a `syntax-table' text property
+gets added, removed, or changed.
+
+Each function is passed a single parameter, the buffer position of the
+start of the property change.  The current buffer is the one the
+change has been made in.  These functions get called even when
+`inhibit-modification-hooks' is non-nil.
+
+Note that these functions may NOT themselves make any buffer changes,
+including text property changes.  */);
+  Vsyntax_table_props_change_alert_functions = Qnil;
+
   interval_insert_behind_hooks = Qnil;
   interval_insert_in_front_hooks = Qnil;
   staticpro (&interval_insert_behind_hooks);
@@ -2337,6 +2373,7 @@ inherits it if NONSTICKINESS is nil.  The `front-sticky' and
   DEFSYM (Qrear_nonsticky, "rear-nonsticky");
   DEFSYM (Qmouse_face, "mouse-face");
   DEFSYM (Qminibuffer_prompt, "minibuffer-prompt");
+  DEFSYM (Qsyntax_table_props_change_alert_functions, "syntax-table-props-change-alert-functions");
 
   /* Properties that text might use to specify certain actions.  */
 


-- 
Alan Mackenzie (Nuremberg, Germany).

bug#36136: [PATCH]: Re: bug#36136: syntax-ppss fails to invalidate its cache on changes to syntax-table text properties

[Eli Zaretskii]

> Date: Sat, 8 Jun 2019 20:36:39 +0000
> From: Alan Mackenzie <acm@muc.de>
> 
> > Suggested fix: the functions set_properties, add_properties,
> > remove_properties in textprop.c should check for changes to,
> > specifically, syntax-table properties.  When these changes are detected,
> > a hook called something like syntax-table-props-change-alert-hook should
> > be called (with some appropriate position parameters, tbd).
> > syntax-ppss-flush-cache will be added to this hook.
> 
> Here is a first draught of a fix to this bug.

I have no opinion about the issue and the idea of its proposed
solution, but I do have some comments to the implementation.

> diff --git a/lisp/emacs-lisp/syntax.el b/lisp/emacs-lisp/syntax.el
> index 9c6d5b5829..673d598de6 100644
> --- a/lisp/emacs-lisp/syntax.el
> +++ b/lisp/emacs-lisp/syntax.el
> @@ -43,6 +43,10 @@
>  
>  (eval-when-compile (require 'cl-lib))
>  
> +;; Ensure that the cache gets invalidated when `syntax-table' text
> +;; properties get set, even when `inhibit-modification-hooks' is non-nil.
> +(add-hook 'syntax-table-props-change-alert-functions #'syntax-ppss-flush-cache)

This means that whenever syntax.el is loaded (i.e., always, since
syntax.el is preloaded), this hook will be non-nil.  Is that a good
idea?  I mean, if we always call this function, why do this via a
hook?

>    DEFVAR_BOOL ("inhibit-modification-hooks", inhibit_modification_hooks,
> -	       doc: /* Non-nil means don't run any of the hooks that respond to buffer changes.
> +	       doc: /* Non-nil means don't run most of the hooks that respond to buffer changes.
>  This affects `before-change-functions' and `after-change-functions',
> -as well as hooks attached to text properties and overlays.
> +as well as most hooks attached to text properties and overlays.
> +However the hook `syntax-table-props-change-alert-functions' gets run
> +regardless of the setting of this variable.

If you go to such lengths, please be sure to mention that the new hook
will NOT run if run-hooks is nil.

> +            if (EQ (sym, Qsyntax_table))
> +              CALLN (Frun_hook_with_args, Qsyntax_table_props_change_alert_functions,
> +                     make_fixnum (interval->position));

These calls make set/add/remove_properties slower.  AFAIK, those
functions are hotspots in Emacs, so please make these calls as
efficient as possible.  In particular, I would call run_hook_with_args
directly, bypassing an extra Frun_hook_with_args function call.
Better still, if we abandon the idea of doing this via a hook, calling
what is now a hook function directly will be more efficient still.

Also, what about the safety of this call? what if the hook signals an
error or the user presses C-g while the hook runs?  IOW, should you
use safe_call or some of its variants, and should you inhibit QUIT?
and if so, whether and how should you handle in the code the case when
the hook does signal an error?

> +  DEFVAR_LISP ("syntax-table-props-change-alert-functions",
> +               Vsyntax_table_props_change_alert_functions,
> +               doc: /* List of functions to call each time a `syntax-table' text property
> +gets added, removed, or changed.

The first line of a doc string should be a complete sentence.

> +Each function is passed a single parameter, the buffer position of the
> +start of the property change.

I think you also know the length of the text where you call the hook,
so maybe pass that as well?

> +                          These functions get called even when
> +`inhibit-modification-hooks' is non-nil.

But not when run-hooks is nil.

> +Note that these functions may NOT themselves make any buffer changes,
> +including text property changes.  */);

And how do we enforce this?

> +  Vsyntax_table_props_change_alert_functions = Qnil;

This should have a comment saying the value is assigned in syntax.el,
i.e. it is always non-nil in a dumped Emacs.

bug#36136: [PATCH]: Re: bug#36136: syntax-ppss fails to invalidate its cache on changes to syntax-table text properties

[Alan Mackenzie]

Hello, Eli.

Thanks for criticising my proposed patch.

On Sun, Jun 09, 2019 at 08:56:45 +0300, Eli Zaretskii wrote:
> > Date: Sat, 8 Jun 2019 20:36:39 +0000
> > From: Alan Mackenzie <acm@muc.de>

> > > Suggested fix: the functions set_properties, add_properties,
> > > remove_properties in textprop.c should check for changes to,
> > > specifically, syntax-table properties.  When these changes are detected,
> > > a hook called something like syntax-table-props-change-alert-hook should
> > > be called (with some appropriate position parameters, tbd).
> > > syntax-ppss-flush-cache will be added to this hook.

> > Here is a first draught of a fix to this bug.

> I have no opinion about the issue and the idea of its proposed
> solution, but I do have some comments to the implementation.

[ .... ]

> This means that whenever syntax.el is loaded (i.e., always, since
> syntax.el is preloaded), this hook will be non-nil.  Is that a good
> idea?  I mean, if we always call this function, why do this via a
> hook?

No, it's not a good idea.  Mainly because of ....

[ .... ]

> Also, what about the safety of this call? what if the hook signals an
> error or the user presses C-g while the hook runs?  IOW, should you
> use safe_call or some of its variants, and should you inhibit QUIT?
> and if so, whether and how should you handle in the code the case when
> the hook does signal an error?

.... add-text-properties and friends are called from redisplay.  So it
would be particularly inconvenient for a hook function to throw an error
here - some sophisticated error handling (like there must be for
fontification-functions) would be needed.

You're right.  All that the hook function really needs to do is set some
buffer local syntax-ppss variable indicating the maximum valid position.
This doesn't need a hook, just a new variable in syntax.c and code there
and in syntax-ppss to use it.

[ All other comments noted, and to a large extent incorporated into the
code.]

So, I'll start again.  Thanks!

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#36136: [PATCH]: Re: bug#36136: syntax-ppss fails to invalidate its cache on changes to syntax-table text properties

[Alan Mackenzie]

On Sat, Jun 08, 2019 at 13:17:24 +0000, Alan Mackenzie wrote:
> Hello, Emacs.

> The syntax-ppss cache is not invalidated when syntax-table text
> properties are set or cleared.  This is because the invalidation
> function, syntax-ppss-flush-cache is invoked only as a before-change
> function, but typical (?all) syntax-table property changes happen when
> before-change-functions is inactive.

> This is a bug.

> In my debugging of a CC Mode scenario, a buffer change causes a
> syntax-table text property change at an earlier part of the buffer.  This
> is to do with the change making previously non-matching C++ raw string
> identifiers match up.  Font lock follows the syntax-ppss state, which
> spuriously records that the end part of the buffer is still in a string.
> Hence the non-string part of the buffer is still fontified with
> font-lock-string-face.

> Suggested fix: the functions set_properties, add_properties,
> remove_properties in textprop.c should check for changes to,
> specifically, syntax-table properties.  When these changes are detected,
> a hook called something like syntax-table-props-change-alert-hook should
> be called (with some appropriate position parameters, tbd).
> syntax-ppss-flush-cache will be added to this hook.

> -- 
> Alan Mackenzie (Nuremberg, Germany).

The following patch is simpler than my first proposal, following
feedback from Eli.  It works for me.

Stefan, could you look at this, please?


Make syntax-ppss react to changes in syntax-table text properties

In particular, it trims its cache to the point of such a change.  This fixes
bug #36136.

* src/textprop.c (syntax-propertize--done): New buffer local variable.
(set_properties, add_properties, remove_properties): when a syntax-table text
property is being changed, reduce syntax-propertize--done to the buffer
position.

* lisp/emacs-lisp/syntax.el (syntax-ppss--trim-cache): New function extracted
from syntax-ppss-flush-cache.
(syntax-ppss-flush-cache): Now only modifies syntax-propertize--done and
syntax-ppss--done.
(syntax-ppss): Calls syntax-ppss--trim-cache and sets syntax-propertize--done.


diff --git a/lisp/emacs-lisp/syntax.el b/lisp/emacs-lisp/syntax.el
index 9c6d5b5829..6ca27d8e83 100644
--- a/lisp/emacs-lisp/syntax.el
+++ b/lisp/emacs-lisp/syntax.el
@@ -404,13 +404,10 @@ syntax-ppss-narrow
 (defvar-local syntax-ppss-narrow-start nil
   "Start position of the narrowing for `syntax-ppss-narrow'.")
 
-(define-obsolete-function-alias 'syntax-ppss-after-change-function
-  #'syntax-ppss-flush-cache "27.1")
-(defun syntax-ppss-flush-cache (beg &rest ignored)
-  "Flush the cache of `syntax-ppss' starting at position BEG."
-  ;; Set syntax-propertize to refontify anything past beg.
-  (setq syntax-propertize--done (min beg syntax-propertize--done))
-  ;; Flush invalid cache entries.
+(defun syntax-ppss--trim-cache (beg)
+  "Flush the cache specific to `syntax-ppss' from position BEG.
+
+This doesn't include the cache for `syntax-propertize'."
   (dolist (cell (list syntax-ppss-wide syntax-ppss-narrow))
     (pcase cell
       (`(,last . ,cache)
@@ -432,8 +429,16 @@ syntax-ppss-flush-cache
        ;; (unless cache
        ;;   (remove-hook 'before-change-functions #'syntax-ppss-flush-cache t))
        (setcar cell last)
-       (setcdr cell cache)))
-    ))
+       (setcdr cell cache)))))
+
+(define-obsolete-function-alias 'syntax-ppss-after-change-function
+  #'syntax-ppss-flush-cache "27.1")
+(defun syntax-ppss-flush-cache (beg &rest ignored)
+  "Flush the cache of `syntax-ppss' starting at position BEG."
+  ;; Set syntax-propertize to refontify anything past beg.
+  (setq syntax-propertize--done (min beg syntax-propertize--done))
+  ;; Flush invalid cache entries.
+  (setq syntax-ppss--done (min beg syntax-ppss--done)))
 
 ;;; FIXME: Explain this variable.  Currently only its last (5th) slot is used.
 ;;; Perhaps the other slots should be removed?
@@ -477,8 +482,10 @@ syntax-ppss
 running the hook."
   ;; Default values.
   (unless pos (setq pos (point)))
+  (syntax-ppss--trim-cache syntax-ppss--done)
   (syntax-propertize pos)
   ;;
+  (setq syntax-ppss--done (max syntax-ppss--done pos))
   (with-syntax-table (or syntax-ppss-table (syntax-table))
   (let* ((cell (syntax-ppss--data))
          (ppss-last (car cell))
diff --git a/src/textprop.c b/src/textprop.c
index ae42c44185..be9e34c889 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -334,6 +334,9 @@ set_properties (Lisp_Object properties, INTERVAL interval, Lisp_Object object)
 	    record_property_change (interval->position, LENGTH (interval),
 				    XCAR (sym), XCAR (value),
 				    object);
+            if (EQ (sym, Qsyntax_table)
+                && (interval->position < syntax_ppss__done))
+              syntax_ppss__done = interval->position;
 	  }
 
       /* For each new property that has no value at all in the old plist,
@@ -346,6 +349,9 @@ set_properties (Lisp_Object properties, INTERVAL interval, Lisp_Object object)
 	    record_property_change (interval->position, LENGTH (interval),
 				    XCAR (sym), Qnil,
 				    object);
+            if (EQ (sym, Qsyntax_table)
+                && (interval->position < syntax_ppss__done))
+              syntax_ppss__done = interval->position;
 	  }
     }
 
@@ -400,6 +406,9 @@ add_properties (Lisp_Object plist, INTERVAL i, Lisp_Object object,
 	      {
 		record_property_change (i->position, LENGTH (i),
 					sym1, Fcar (this_cdr), object);
+                if (EQ (sym1, Qsyntax_table)
+                    && (i->position < syntax_ppss__done))
+                  syntax_ppss__done = i->position;
 	      }
 
 	    /* I's property has a different value -- change it */
@@ -436,6 +445,9 @@ add_properties (Lisp_Object plist, INTERVAL i, Lisp_Object object,
 	    {
 	      record_property_change (i->position, LENGTH (i),
 				      sym1, Qnil, object);
+              if (EQ (sym1, Qsyntax_table)
+                  && (i->position < syntax_ppss__done))
+                syntax_ppss__done = i->position;
 	    }
 	  set_interval_plist (i, Fcons (sym1, Fcons (val1, i->plist)));
 	  changed = true;
@@ -470,9 +482,14 @@ remove_properties (Lisp_Object plist, Lisp_Object list, INTERVAL i, Lisp_Object
       while (CONSP (current_plist) && EQ (sym, XCAR (current_plist)))
 	{
 	  if (BUFFERP (object))
-	    record_property_change (i->position, LENGTH (i),
-				    sym, XCAR (XCDR (current_plist)),
-				    object);
+            {
+              record_property_change (i->position, LENGTH (i),
+                                      sym, XCAR (XCDR (current_plist)),
+                                      object);
+              if (EQ (sym, Qsyntax_table)
+                  && (i->position < syntax_ppss__done))
+                syntax_ppss__done = i->position;
+            }
 
 	  current_plist = XCDR (XCDR (current_plist));
 	  changed = true;
@@ -486,8 +503,13 @@ remove_properties (Lisp_Object plist, Lisp_Object list, INTERVAL i, Lisp_Object
 	  if (CONSP (this) && EQ (sym, XCAR (this)))
 	    {
 	      if (BUFFERP (object))
-		record_property_change (i->position, LENGTH (i),
-					sym, XCAR (XCDR (this)), object);
+                {
+                  record_property_change (i->position, LENGTH (i),
+                                          sym, XCAR (XCDR (this)), object);
+                  if (EQ (sym, Qsyntax_table)
+                      && (i->position < syntax_ppss__done))
+                    syntax_ppss__done = i->position;
+                }
 
 	      Fsetcdr (XCDR (tail2), XCDR (XCDR (this)));
 	      changed = true;
@@ -2319,6 +2341,11 @@ inherits it if NONSTICKINESS is nil.  The `front-sticky' and
   Vtext_property_default_nonsticky
     = list2 (Fcons (Qsyntax_table, Qt), Fcons (Qdisplay, Qt));
 
+  DEFVAR_INT ("syntax-ppss--done", syntax_ppss__done,
+              doc: /* Position up to which the `syntax-ppss' cache is valid.  */);
+  syntax_ppss__done = -1;
+  Fmake_variable_buffer_local (intern ("syntax_ppss__done"));
+
   interval_insert_behind_hooks = Qnil;
   interval_insert_in_front_hooks = Qnil;
   staticpro (&interval_insert_behind_hooks);


-- 
Alan Mackenzie (Nuremberg, Germany).

bug#36136: [PATCH]: Re: bug#36136: syntax-ppss fails to invalidate its cache on changes to syntax-table text properties

[Stefan Monnier]

> The following patch is simpler than my first proposal, following
> feedback from Eli.  It works for me.
>
> Stefan, could you look at this, please?

Here I am.

> * src/textprop.c (syntax-propertize--done): New buffer local variable.
> (set_properties, add_properties, remove_properties): when a syntax-table text
> property is being changed, reduce syntax-propertize--done to the buffer
> position.

Hmm... I'm not too fond of adding ad-hoc support for specific
text-properties in (set_properties, add_properties, remove_properties).

> * lisp/emacs-lisp/syntax.el (syntax-ppss--trim-cache): New function extracted
> from syntax-ppss-flush-cache.
> (syntax-ppss-flush-cache): Now only modifies syntax-propertize--done and
> syntax-ppss--done.
> (syntax-ppss): Calls syntax-ppss--trim-cache and sets syntax-propertize--done.

This part looks OK.

I'm not sure if making the cache-flushing more lazy will be a win
overall: it speeds up buffer modifications at the cost of slowing down
syntax-ppss.

To get back to the original problem:

> This is because the invalidation function, syntax-ppss-flush-cache is
> invoked only as a before-change function, but typical (?all)
> syntax-table property changes happen when before-change-functions
> is inactive.

That's why it doesn't have "--" in its name: if you don't want to use
syntax-propertize then you'll probably have to call that function
by hand.  I consider it as perfectly acceptable.


        Stefan

bug#36136: [PATCH]: Re: bug#36136: syntax-ppss fails to invalidate its cache on changes to syntax-table text properties

[Alan Mackenzie]

Hello, Stefan.

On Wed, Jun 12, 2019 at 04:37:25 -0400, Stefan Monnier wrote:
> > The following patch is simpler than my first proposal, following
> > feedback from Eli.  It works for me.

> > Stefan, could you look at this, please?

> Here I am.

Hello!

> > * src/textprop.c (syntax-propertize--done): New buffer local variable.
> > (set_properties, add_properties, remove_properties): when a syntax-table text
> > property is being changed, reduce syntax-propertize--done to the buffer
> > position.

> Hmm... I'm not too fond of adding ad-hoc support for specific
> text-properties in (set_properties, add_properties, remove_properties).

Neither am I, particularly.  But the whole point of syntax-ppss, surely,
is that it should work automatically, without users having to call
syntax-ppss-flush-cache all the time.

The root of the problem is that inhibit-modification-hooks is too blunt
a tool.  Setting it prevents running functions we want to run, just as
much as ones we don't.

I've just had another idea: we introduce a new property called something
like dont-inhibit.   When a function in before/after-change-functions
has this property, it would run, regardless of
inhibit-modification-hooks.  Or possibly, we could introduce new hooks
no-inhibit-before/after-change-functions.

What do you think?

> > * lisp/emacs-lisp/syntax.el (syntax-ppss--trim-cache): New function extracted
> > from syntax-ppss-flush-cache.
> > (syntax-ppss-flush-cache): Now only modifies syntax-propertize--done and
> > syntax-ppss--done.
> > (syntax-ppss): Calls syntax-ppss--trim-cache and sets syntax-propertize--done.

> This part looks OK.

> I'm not sure if making the cache-flushing more lazy will be a win
> overall: it speeds up buffer modifications at the cost of slowing down
> syntax-ppss.

It will probably not make a great deal of difference either way.  Buffer
changes are frequent in Emacs, and so are calls to syntax-ppss in many
major modes.

> To get back to the original problem:

> > This is because the invalidation function, syntax-ppss-flush-cache is
> > invoked only as a before-change function, but typical (?all)
> > syntax-table property changes happen when before-change-functions
> > is inactive.

> That's why it doesn't have "--" in its name: if you don't want to use
> syntax-propertize then you'll probably have to call that function
> by hand.  I consider it as perfectly acceptable.

Well, for CC Mode I'm going to have to do that anyway, since Emacs-26.x
and earlier are already out there and aren't going to change.  This is
going to be tedious and error prone.

But for the future, it would be nice if syntax-ppss could take note of
all buffer changes, rather than just some of them.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#36136: [PATCH]: Re: bug#36136: syntax-ppss fails to invalidate its cache on changes to syntax-table text properties

[Stefan Monnier]

>> Hmm... I'm not too fond of adding ad-hoc support for specific
>> text-properties in (set_properties, add_properties, remove_properties).
> Neither am I, particularly.  But the whole point of syntax-ppss, surely,
> is that it should work automatically, without users having to call
> syntax-ppss-flush-cache all the time.

`grep` seems to argue that "all the time" is an over statement.

I see the following uses in Emacs's bundled files:
- one call in lisp/elec-pair.el [ I think this one would likely be
  better handled some other way (e.g. by setting syntax-ppss-table), tho
  I haven't looked hard enough to have a firm opinion yet ].
- in before-change-functions, of course.
- one call from syntax-propertize.
- one call in font-lock-apply-syntactic-highlight (this is part of the
  font-lock-syntactic-keywords functionality marked obsolete in 24.1).
- one call in fortran-line-length (not because the buffer is changed,
  but because the syntax-propertize-function is changed).

Of these, only the one in syntax-propertize and the one in
font-lock-apply-syntactic-highlight would be made unnecessary by your patch.

I see a few more calls in the elpa.git files, but none of them seems
affected either.

> It will probably not make a great deal of difference either way.  Buffer
> changes are frequent in Emacs, and so are calls to syntax-ppss in many
> major modes.

That's also my impression (which is why I haven't bothered to make
syntax-ppss-flush-cache lazy like your patch does, even tho I've been
tempted to several times).

> Well, for CC Mode I'm going to have to do that anyway, since Emacs-26.x
> and earlier are already out there and aren't going to change.

Indeed.
[ unless you decide to make CC-mode use syntax-propertize, of course ;-) ]

But I also agree that it shouldn't prevent us from looking for
better solutions.

> This is going to be tedious and error prone.

I don't see why.  Usually a single call does the trick (as seen above:
either in the function that sets the syntax-table property, or in the
function that takes care of refreshing things in response to a buffer
modification).


        Stefan

bug#36136: [PATCH]: Re: bug#36136: syntax-ppss fails to invalidate its cache on changes to syntax-table text properties

[Alan Mackenzie]

Hello, Stefan.

On Wed, Jun 12, 2019 at 06:54:25 -0400, Stefan Monnier wrote:
> >> Hmm... I'm not too fond of adding ad-hoc support for specific
> >> text-properties in (set_properties, add_properties, remove_properties).
> > Neither am I, particularly.  But the whole point of syntax-ppss, surely,
> > is that it should work automatically, without users having to call
> > syntax-ppss-flush-cache all the time.

> `grep` seems to argue that "all the time" is an over statement.

CC Mode has to call that function all the time, without any scare
quotes.  In particular, it has to call the thing before each
font-locking action.  It shouldn't have to; it doesn't even make any use
of the package, and is thus messy programming indeed.

The root of the problem is the implicit assumption made by syntax-ppss
that programs would never set syntax-table properties.  This assumption
doesn't hold.

[ .... ]

> > It will probably not make a great deal of difference either way.  Buffer
> > changes are frequent in Emacs, and so are calls to syntax-ppss in many
> > major modes.

> That's also my impression (which is why I haven't bothered to make
> syntax-ppss-flush-cache lazy like your patch does, even tho I've been
> tempted to several times).

> > Well, for CC Mode I'm going to have to do that anyway, since Emacs-26.x
> > and earlier are already out there and aren't going to change.

> Indeed.
> [ unless you decide to make CC-mode use syntax-propertize, of course ;-) ]

I don't think that would work without loss of functionality and loss of
speed (to whatever degree), and even if it could, would be more work than
it's worth.

> But I also agree that it shouldn't prevent us from looking for
> better solutions.

What about my idea of giving a "no inhibit" property to functions on
before/after-change-functions?  This would be easy to implement, though
might have unwanted unexpected consequences.

> > This is going to be tedious and error prone.

> I don't see why.  Usually a single call does the trick (as seen above:
> either in the function that sets the syntax-table property, or in the
> function that takes care of refreshing things in response to a buffer
> modification).

Yes, you're right, it wasn't too bad.  There are many functions in CC
Mode which set the syntax-table property, so adapting each of them
didn't come into consideration.  I amended the existing cache
invalidation routine to keep track of a syntax-table property position,
and pass that position to syntax-ppss-flush-cache at the end of
c-after-change, just before font-lock kicks in.

But, as already said, I shouldn't have to do this.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#36136: [PATCH]: Re: bug#36136: syntax-ppss fails to invalidate its cache on changes to syntax-table text properties

[Stefan Monnier]

>> >> Hmm... I'm not too fond of adding ad-hoc support for specific
>> >> text-properties in (set_properties, add_properties, remove_properties).
>> > Neither am I, particularly.  But the whole point of syntax-ppss, surely,
>> > is that it should work automatically, without users having to call
>> > syntax-ppss-flush-cache all the time.
>> `grep` seems to argue that "all the time" is an over statement.
> CC Mode has to call that function all the time, without any scare
> quotes.

IIUC the end of your message, you did find another spot where to call
syntax-ppss-flush-cache, making it much less problematic, right?

> The root of the problem is the implicit assumption made by syntax-ppss
> that programs would never set syntax-table properties.  This assumption
> doesn't hold.

AFAIK CC-mode is the only exception nowadays.

>> But I also agree that it shouldn't prevent us from looking for
>> better solutions.
>
> What about my idea of giving a "no inhibit" property to functions on
> before/after-change-functions?

I'm not convinced this won't introduce more problems than it intends to
fix (it seems that it will likely lead to the subsequent introduction of
a `no-really-I-do-mean-inhibit-all-this-time` etc...).

> But, as already said, I shouldn't have to do this.

I agree that it would be nice to invalidate the cache in a more
"automatic" way.  But it's a trade-off.  Your new call to
syntax-ppss-flush-cache brings the count of calls that we could
eliminate this way to 3 (one of those being obsolete since 24.1 should
likely disappear soon).  That's still not very many.  And those calls
are pretty efficient I think compared to the more general code which
catches every text-property change to see if it happens to change the
`syntax-table` property.

So the current situation is not ideal conceptually, but it's more
efficient than the alternative both in terms of run-time complexity and
in terms of code size.  It's actually a pretty good deal.
Especially since even with a more automatic approach, there'd still be
other places where we need to call it manually anyway
(e.g. fortran-line-length).


        Stefan

bug#36136: Acknowledgement (syntax-ppss fails to invalidate its cache on changes to syntax-table text properties)

[Alan Mackenzie]

tags 36136 + wontfix
quit

It has been decided not to fix this bug, since it doesn't affect many
packages, and easy (though ugly) workarounds are available, principally
by the affected packages calling syntax-ppss-flush-cache.

-- 
Alan Mackenzie (Nuremberg, Germany).