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.