bug#24749: Making sure syntax-propertize is called

bug#24749: Making sure syntax-propertize is called

[Stefan Monnier]

Package: Emacs
Version: 25.2.50


Nowadays, syntax-propertize is automatically called ondemand by the sexp
functions, but only if parse-sexp-lookup-properties is non-nil.
See bug#4920 for an example of the problem and the fix we installed.

I suggest we install the patch below instead, so as to fix this bug in
more/all major modes at once.


        Stefan


diff --git a/lisp/subr.el b/lisp/subr.el
index 8c29395..38bfc5f 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -1844,8 +1844,15 @@ run-mode-hooks
 	(push hook delayed-mode-hooks))
     ;; Normal case, just run the hook as before plus any delayed hooks.
     (setq hooks (nconc (nreverse delayed-mode-hooks) hooks))
+    (and syntax-propertize-function
+         (not (buffer-local-p 'parse-sexp-lookup-properties))
+         ;; `syntax-propertize' sets `parse-sexp-lookup-properties' for us, but
+         ;; in order for the sexp primitives to automatically call
+         ;; `syntax-propertize' we need `parse-sexp-lookup-properties' to be
+         ;; set first.
+         (setq-local parse-sexp-lookup-properties t))
     (setq delayed-mode-hooks nil)
-    (apply 'run-hooks (cons 'change-major-mode-after-body-hook hooks))
+    (apply #'run-hooks (cons 'change-major-mode-after-body-hook hooks))
     (if (buffer-file-name)
         (with-demoted-errors "File local-variables error: %s"
           (hack-local-variables 'no-mode)))
diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index f1fa7cb..848e357 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -3728,7 +3728,6 @@ js-mode
   (setq-local prettify-symbols-alist js--prettify-symbols-alist)
 
   (setq-local parse-sexp-ignore-comments t)
-  (setq-local parse-sexp-lookup-properties t)
   (setq-local which-func-imenu-joiner-function #'js--which-func-joiner)
 
   ;; Comments
diff --git a/lisp/progmodes/sh-script.el b/lisp/progmodes/sh-script.el
index 4cb8886..fd87c1e 100644
--- a/lisp/progmodes/sh-script.el
+++ b/lisp/progmodes/sh-script.el
@@ -2214,7 +2214,6 @@ sh-set-shell
                       (funcall mksym "rules")
                       :forward-token  (funcall mksym "forward-token")
                       :backward-token (funcall mksym "backward-token")))
-        (setq-local parse-sexp-lookup-properties t)
 	(if sh-make-vars-local
 	    (sh-make-vars-local))
 	(message "Indentation setup for shell type %s" sh-shell))
diff --git a/lisp/textmodes/texinfo.el b/lisp/textmodes/texinfo.el
index 9f72659..6982fb9 100644
--- a/lisp/textmodes/texinfo.el
+++ b/lisp/textmodes/texinfo.el
@@ -620,7 +620,6 @@ texinfo-mode
   (setq font-lock-defaults
 	'(texinfo-font-lock-keywords nil nil nil backward-paragraph))
   (setq-local syntax-propertize-function texinfo-syntax-propertize-function)
-  (setq-local parse-sexp-lookup-properties t)
   (setq-local add-log-current-defun-function #'texinfo-current-defun-name)
 
   ;; Outline settings.

bug#24749: Making sure syntax-propertize is called

[Dmitry Gutov]

On 21.10.2016 00:14, Stefan Monnier wrote:

> Nowadays, syntax-propertize is automatically called ondemand by the sexp
> functions, but only if parse-sexp-lookup-properties is non-nil.
> See bug#4920 for an example of the problem and the fix we installed.
>
> I suggest we install the patch below instead, so as to fix this bug in
> more/all major modes at once.

Good suggestion. Although can't we just set the default value to t, and 
simply not use it when syntax-propertize-function is not set?

bug#24749: Making sure syntax-propertize is called

[Stefan Monnier]

> Good suggestion. Although can't we just set the default value to t,

Of course, that's another option.

> and simply not use it when syntax-propertize-function is not set?

AFAIK the code already works fine when syntax-propertize-function is nil
but parse-sexp-lookup-properties non-nil.


        Stefan

bug#24749: Making sure syntax-propertize is called

[Dmitry Gutov]

On 21.10.2016 02:12, Stefan Monnier wrote:
>> Good suggestion. Although can't we just set the default value to t,
>
> Of course, that's another option.

If it has no downsides, I think it's a bit simpler, and also easier to 
document in NEWS.

bug#24749: Making sure syntax-propertize is called

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Thu, 20 Oct 2016 17:14:40 -0400
> 
> Package: Emacs
> Version: 25.2.50
> 
> 
> Nowadays, syntax-propertize is automatically called ondemand by the sexp
> functions, but only if parse-sexp-lookup-properties is non-nil.
> See bug#4920 for an example of the problem and the fix we installed.
> 
> I suggest we install the patch below instead, so as to fix this bug in
> more/all major modes at once.

Since when does this issue exist?

The issue seems of rather low importance to me, so unless it's a
regression since Emacs 24.5, I think the fix should go to master, not
to the release branch.

Thanks.

bug#24749: Making sure syntax-propertize is called

[Stefan Monnier]

>> Nowadays, syntax-propertize is automatically called ondemand by the sexp
>> functions, but only if parse-sexp-lookup-properties is non-nil.
>> See bug#4920 for an example of the problem and the fix we installed.
>> I suggest we install the patch below instead, so as to fix this bug in
>> more/all major modes at once.
> Since when does this issue exist?

25.1.

> The issue seems of rather low importance to me, so unless it's a
> regression since Emacs 24.5, I think the fix should go to master, not
> to the release branch.

100% agreement (I just happened to send the bug-report from an Emacs
built from the master branch before it got renumbered).


        Stefan

bug#24749: Making sure syntax-propertize is called

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: 24749@debbugs.gnu.org
> Date: Fri, 21 Oct 2016 11:36:56 -0400
> 
> >> Nowadays, syntax-propertize is automatically called ondemand by the sexp
> >> functions, but only if parse-sexp-lookup-properties is non-nil.
> >> See bug#4920 for an example of the problem and the fix we installed.
> >> I suggest we install the patch below instead, so as to fix this bug in
> >> more/all major modes at once.
> > Since when does this issue exist?
> 
> 25.1.

Then I'm okay with having this on the emacs-25 branch, thanks.

bug#24749: Making sure syntax-propertize is called

[Stefan Monnier]

>> >> Nowadays, syntax-propertize is automatically called ondemand by the sexp
>> >> functions, but only if parse-sexp-lookup-properties is non-nil.
>> >> See bug#4920 for an example of the problem and the fix we installed.
>> >> I suggest we install the patch below instead, so as to fix this bug in
>> >> more/all major modes at once.
>> > Since when does this issue exist?
>> 25.1.
> Then I'm okay with having this on the emacs-25 branch, thanks.

I think I may have described it incorrectly: the issue that it solves is
one that's been around forever.  It's just that 25.1 fixes this issue in
most cases and hence creates a new expectations that the issue is fixed,
but there's still a hole if you try to use navigation commands before
any syntax-propertize call, which this patch intends to fix.

So I don't think this is urgent, and since the fix (whichever we choose)
affects (by nature) "all" modes, I don't think it's safe enough for
emacs-25.


        Stefan

bug#24749: Making sure syntax-propertize is called

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: 24749@debbugs.gnu.org
> Date: Fri, 21 Oct 2016 12:34:27 -0400
> 
> I think I may have described it incorrectly: the issue that it solves is
> one that's been around forever.  It's just that 25.1 fixes this issue in
> most cases and hence creates a new expectations that the issue is fixed,
> but there's still a hole if you try to use navigation commands before
> any syntax-propertize call, which this patch intends to fix.
> 
> So I don't think this is urgent, and since the fix (whichever we choose)
> affects (by nature) "all" modes, I don't think it's safe enough for
> emacs-25.

Fine, then master it is.

bug#24749: Making sure syntax-propertize is called

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
>> Cc: 24749@debbugs.gnu.org
>> Date: Fri, 21 Oct 2016 12:34:27 -0400
>> 
>> I think I may have described it incorrectly: the issue that it solves is
>> one that's been around forever.  It's just that 25.1 fixes this issue in
>> most cases and hence creates a new expectations that the issue is fixed,
>> but there's still a hole if you try to use navigation commands before
>> any syntax-propertize call, which this patch intends to fix.
>> 
>> So I don't think this is urgent, and since the fix (whichever we choose)
>> affects (by nature) "all" modes, I don't think it's safe enough for
>> emacs-25.
>
> Fine, then master it is.

Should this finally be applied to master then?

bug#24749: Making sure syntax-propertize is called

[Stefan Monnier]

Version: 27.1

> Should this finally be applied to master then?

Done,


        Stefan

bug#24749: closed (Re: bug#24749: Making sure syntax-propertize is called)

[Tassilo Horn]

help-debbugs@gnu.org (GNU bug Tracking System) writes:

Hi Stefan,

> Version: 27.1
>
>> Should this finally be applied to master then?
>
> Done,

Now I get this error:

Debugger entered--Lisp error: (void-function buffer-local-p)
  buffer-local-p(parse-sexp-lookup-properties)
  run-mode-hooks(message-mode-hook)
  message-mode()
  message-pop-to-buffer("*unsent wide reply to GNU bug Tracking System*" nil)
  message-reply(nil t)
  gnus-summary-reply((12) t)
  gnus-summary-reply-with-original(nil t)
  gnus-summary-wide-reply-with-original(nil)
  funcall-interactively(gnus-summary-wide-reply-with-original nil)
  call-interactively(gnus-summary-wide-reply-with-original nil nil)
  command-execute(gnus-summary-wide-reply-with-original)

Should it be `local-variable-p' instead of the non-existing
`buffer-local-p'?

Bye,
Tassilo

bug#24749: closed (Re: bug#24749: Making sure syntax-propertize is called)

[Stefan Monnier]

> Debugger entered--Lisp error: (void-function buffer-local-p)

Duh!  That's what I get for taking the original patch I sent, rather
than re-extract it from my local Emacs.
Should be fixed now,


        Stefan

bug#24749: closed (Re: bug#24749: Making sure syntax-propertize is called)

[martin rudalics]

 >> Debugger entered--Lisp error: (void-function buffer-local-p)
 >
 > Duh!  That's what I get for taking the original patch I sent, rather
 > than re-extract it from my local Emacs.

To me `buffer-local-p' sounds better than `local-variable-p'.

martin

bug#24749: Making sure syntax-propertize is called

[Stefan Monnier]

[ Resending after unarchiving the bug.  ]

> Shouldn't this be pushed (or even backported) to emacs 26?

While I clearly think it's an improvement, I don't think it fixes
a regression: it's more like it fixes bugs that have been with us for
"ever".
So, I don't see any urgency.  

It should be pretty safe, tho, so it's probably OK to add it to
emacs-26.  It's Eli's call (and given the situation, I think in his
shoes I probably would say "I'd rather not").

> Some modes (e.g pascal, perl) don't set parse-sexp-lookup-properties
> but have the syntax-propertize-function.  This leads to
> really-hard-to-debug behavior in batch mode.  For some reason in
> interactive session parse-sexp-lookup-properties is set to
> t (font-lock I guess, and maybe something else).

Indeed, font-lock is the most common caller of syntax-propertize, so
that explains why interactive use typically is not affected.

Note: you can also set parse-sexp-lookup-properties's default value to
t and forget about the problem ;-)

> Also, it's a bit confusing that many modes do still set
> parse-sexp-lookup-properties explicitly:
>  
>  lisp/emacs-lisp/syntax.el:293:      (set (make-local-variable 'parse-sexp-lookup-properties) t)
>  lisp/ChangeLog.9:10944:	Don't switch the syntax-table.  Don't set parse-sexp-lookup-properties.
>  lisp/ChangeLog.15:7269:	Set parse-sexp-lookup-properties buffer-locally here.
>  lisp/progmodes/cc-defs.el:1996:	(setq parse-sexp-lookup-properties t
>  lisp/progmodes/cc-mode.el:580:      (set (make-local-variable 'parse-sexp-lookup-properties) t))
>  lisp/progmodes/ruby-mode.el:754:  (setq-local parse-sexp-lookup-properties t)
>  lisp/progmodes/ada-mode.el:1149:    (set (make-local-variable 'parse-sexp-lookup-properties) t))
>  lisp/progmodes/python.el:5352:  (set (make-local-variable 'parse-sexp-lookup-properties) t)
>  lisp/progmodes/cperl-mode.el:1759:	(set (make-local-variable 'parse-sexp-lookup-properties) t)
>  lisp/progmodes/cperl-mode.el:6762:	(set (make-local-variable 'parse-sexp-lookup-properties) t))))
>  lisp/font-lock.el:1545:    (set (make-local-variable 'parse-sexp-lookup-properties) t))
>  lisp/subr.el:1982:         ;; `syntax-propertize' sets `parse-sexp-lookup-properties' for us, but
>  lisp/subr.el:1986:         (setq-local parse-sexp-lookup-properties t))
>  test/lisp/gnus/message-tests.el:47:          (setq-local parse-sexp-lookup-properties t)

While this is likely unneeded in several of those matches, it's also
harmless.  Feel free to remove them in those cases where you can
determine it's indeed not needed (typically because you know the code
won't be used in Emacs<27).


         Stefan

bug#24749: Making sure syntax-propertize is called

[Vitalie Spinu]

>> On Mon, Jun 03 2019 13:47, Stefan Monnier wrote:

> Indeed, font-lock is the most common caller of syntax-propertize, so
> that explains why interactive use typically is not affected.

It's a bit tricky actually. AFAICS font-lock installs it only when
font-lock-syntactic-keywords are defined and syntax-propertize-function is not
defined. The latter condition was undefined for me. So I still have no clear
idea what actually is setting the value. Likely some third party. 

I pulled quite some hair till I figured how things work. So I wonder if a few
words documenting the parity between `syntax-propertize` and
`parse-sexp-lookup-properties` could be added to (preferably)
`syntax-propertize` docstring. Currently the only "doc" is a comment in
`run-mode-hooks`, which is the last place someone will look for it.

╭──────── #1982 ─ /home/vspinu/bin/emacs/lisp/subr.el ──
│          ;; `syntax-propertize' sets `parse-sexp-lookup-properties' for us, but
│          ;; in order for the sexp primitives to automatically call
│          ;; `syntax-propertize' we need `parse-sexp-lookup-properties' to be
│          ;; set first.
╰──────── #1985 ─


> Note: you can also set parse-sexp-lookup-properties's default value to
> t and forget about the problem ;-)

Why not make it the default then?

There is a clear redundancy between the two. If a mode defines
`syntax-propertize` then it must set `parse-sexp-lookup-properties`, and the
other way around.

It seems that the only(?) use case for a nil `parse-sexp-lookup-properties` is
to set it to nil dynamically in specialized lookup code for performance
reasons. Such code does let-bind it to nil already.

  Vitalie

bug#24749: Making sure syntax-propertize is called

[Stefan Monnier]

>> Indeed, font-lock is the most common caller of syntax-propertize, so
>> that explains why interactive use typically is not affected.
> It's a bit tricky actually. AFAICS font-lock installs it only when
> font-lock-syntactic-keywords are defined and syntax-propertize-function is not
> defined.

No, no: what font-lock does is *call* syntax-propertize.
The font-lock-syntactic-keywords stuff is just obsolete backward
compatibility cruft.

> The latter condition was undefined for me.  So I still have no clear
> idea what actually is setting the value.

It's syntax-propertize which sets parse-sexp-lookup-properties ;-)

>> Note: you can also set parse-sexp-lookup-properties's default value to
>> t and forget about the problem ;-)
> Why not make it the default then?

No idea.

> There is a clear redundancy between the two. If a mode defines
> `syntax-propertize` then it must set `parse-sexp-lookup-properties`,

Indeed, which is why syntax-propertize sets it.

> and the other way around.

Actually the other way around isn't quite true: a major mode may decide
to use the syntax-table text-property but set it by hand its own way
rather than via syntax-propertize.

> It seems that the only(?) use case for a nil `parse-sexp-lookup-properties` is
> to set it to nil dynamically in specialized lookup code for performance
> reasons. Such code does let-bind it to nil already.

I can't remember binding it to nil for performance reasons (tho maybe
that's the reason behind yasnippet.el's case), but we do let-bind it
temporarily to nil in a few cases as a hackish way to have a "different
view" of the buffer which happens to fit our needs.  E.g. this is done
in font-latex.el and sm-c-mode.el.


        Stefan

bug#24749: Making sure syntax-propertize is called

[npostavs]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> It seems that the only(?) use case for a nil `parse-sexp-lookup-properties` is
>> to set it to nil dynamically in specialized lookup code for performance
>> reasons. Such code does let-bind it to nil already.
>
> I can't remember binding it to nil for performance reasons (tho maybe
> that's the reason behind yasnippet.el's case), but we do let-bind it
> temporarily to nil in a few cases as a hackish way to have a "different
> view" of the buffer which happens to fit our needs.  E.g. this is done
> in font-latex.el and sm-c-mode.el.

A "different view" of the buffer is the reason for yasnippet.el's use of
it too: it's parsing braces in "snippet syntax", which is not related to
the major mode's syntax.  (Really, parsing of the snippet syntax should
be done in a separate buffer to remove the need for this, but the
parsing code is intertwined with the evaluation of lisp in some snippet
constructs, so it's tricky to do that.)

bug#24749: Making sure syntax-propertize is called

[Vitalie Spinu]

>> On Mon, Jun 03 2019 15:45, Stefan Monnier wrote:

> It's syntax-propertize which sets parse-sexp-lookup-properties ;-)

Arh. Right. It's a chicken-egg then, syntax-propertize is triggered by search
when parse-sexp-lookup-properties are t, but parse-sexp-lookup-properties is set
by syntax-propertize. So an external tool (font-lock) was needed to
syntax-propertize for the first time before this patch. Things are not
particularly elegant, are they?

>>> Note: you can also set parse-sexp-lookup-properties's default value to
>>> t and forget about the problem ;-)
>> Why not make it the default then?

> No idea.

Would you consider a patch which sets it to t by default and removes the
auto-resets? It's hard to imagine that it would break anything.

>> and the other way around.

> Actually the other way around isn't quite true: a major mode may decide
> to use the syntax-table text-property but set it by hand its own way
> rather than via syntax-propertize.

Ok, but then setting parse-sexp-lookup-properties to t by default would not harm
such a mode. 

Instead, one can imagine a scenario when a mode doesn't want to use syntax
tables but would like to use syntax-propertize to install various text
properties (to be used in font-lock or just caching buffer structures
etc). Current situation harms such modes by re-setting
parse-sexp-lookup-properties to t on various occasions.

>> It seems that the only(?) use case for a nil `parse-sexp-lookup-properties` is
>> to set it to nil dynamically in specialized lookup code for performance
>> reasons. Such code does let-bind it to nil already.

> I can't remember binding it to nil for performance reasons (tho maybe
> that's the reason behind yasnippet.el's case), but we do let-bind it
> temporarily to nil in a few cases as a hackish way to have a "different
> view" of the buffer which happens to fit our needs.  E.g. this is done
> in font-latex.el and sm-c-mode.el.

I am setting it to nil while searching for mode boundaries in polymode. First,
it doesn't make sense to use local syntax tables in search because different
inner modes might have a different opinion of syntax. Second, I don't want to
trigger syntax-propertize for performance reasons. The bounds of the forward
search is normally eob and that would mean syntax-propertizing the entire buffer
on every mode boundary lookup.


  Vitale

bug#24749: Making sure syntax-propertize is called

[Stefan Monnier]

> Arh. Right. It's a chicken-egg then, syntax-propertize is triggered by
> search when parse-sexp-lookup-properties are t, but
> parse-sexp-lookup-properties is set by syntax-propertize. So an
> external tool (font-lock) was needed to syntax-propertize for the
> first time before this patch. Things are not particularly elegant,
> are they?

I see you understand ;-)

> Would you consider a patch which sets it to t by default and removes the
> auto-resets?  It's hard to imagine that it would break anything.

It's not my call to make, but I won't oppose it, no.

> Ok, but then setting parse-sexp-lookup-properties to t by default
> would not harm such a mode.

No, and I can't think of a case where it would cause harm other than
maybe a potential possible slowdown.

> I am setting it to nil while searching for mode boundaries in
> polymode. First, it doesn't make sense to use local syntax tables in
> search because different inner modes might have a different opinion of
> syntax.

That also means that the regexps you use there presumably don't use
syntax-dependent constructs like \s?, \<, and friends, right?

> Second, I don't want to trigger syntax-propertize for
> performance reasons.  The bounds of the forward search is normally eob
> and that would mean syntax-propertizing the entire buffer on every
> mode boundary lookup.

If your regexp doesn't use syntax-dependent constructs, then the regexp
search shouldn't call syntax-propertize, IIRC (and similarly,
syntax-propertize wouldn't be applied to the whole buffer if a match is
found before EOB).


        Stefan

bug#24749: Making sure syntax-propertize is called

[Vitalie Spinu]

>> On Tue, Jun 04 2019 09:08, Stefan Monnier wrote:

>> I am setting it to nil while searching for mode boundaries in
>> polymode. First, it doesn't make sense to use local syntax tables in
>> search because different inner modes might have a different opinion of
>> syntax.

> That also means that the regexps you use there presumably don't use
> syntax-dependent constructs like \s?, \<, and friends, right?

Ideally yes, but I cannot control for what users use. To be frank I was not even
paying attention to this in my own regexps so far. I think constructs like \\w
or \\s- are pretty common.

>> Second, I don't want to trigger syntax-propertize for performance reasons.
>> The bounds of the forward search is normally eob and that would mean
>> syntax-propertizing the entire buffer on every mode boundary lookup.

> If your regexp doesn't use syntax-dependent constructs, then the regexp
> search shouldn't call syntax-propertize, 

Good to know. I was not aware of this. 

Relatedly, I just noticed that seemingly innocuous dabrev-expand (hippie-expand)
trigger syntax-propertize for the entire buffer. In large buffers, especially if
you have several of these, the delay is noticeable.

> IIRC (and similarly, syntax-propertize wouldn't be applied to the whole buffer
> if a match is found before EOB).

In the multi-mode context some types of chunks might not be present in the
buffer, and the rare ones are commonly not. So the search ends up at eob.


  Vitalie

bug#24749: Making sure syntax-propertize is called

[Stefan Monnier]

> Relatedly, I just noticed that seemingly innocuous dabrev-expand
> (hippie-expand) trigger syntax-propertize for the entire buffer.

Probably because of a search with \< and such.  The fact that this
depends on the syntax-table text-property is a misfeature in most cases :-(

>> IIRC (and similarly, syntax-propertize wouldn't be applied to the
>> whole buffer if a match is found before EOB).
> In the multi-mode context some types of chunks might not be present in the
> buffer, and the rare ones are commonly not. So the search ends up at eob.

I was mere mentioning a detail of the behavior.  Indeed, if your search
fails then it will have checked to EOB.  So the behavior may depend on
details like the difference between:

    (re-search-forward "FOO\\|BAR" ...)
or
    (or (re-search-forward "FOO" ...)
        (re-search-forward "BAR" ...))


-- Stefan