unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#58888: 28.1.90; font-lock-defaults not respected when hack-local-variables unsafe variable dialogue is displayed before setting the defaults
@ 2022-10-30  6:58 Ihor Radchenko
  2022-10-31  2:15 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 14+ messages in thread
From: Ihor Radchenko @ 2022-10-30  6:58 UTC (permalink / raw)
  To: 58888

Following up the discussion in bug#57003.

Consider the following file:

------------
# -*- mode:my/test -*-
This is test with keyword to be fontified.

# Local Variables:
# eval: (setq unsafe-variable t)
# End:
-------------

Then, consider the following major mode:

(define-derived-mode my/test-mode text-mode "Test"
  ""
  (add-hook 'hack-local-variables-hook
	    (lambda ()
	      (setq-local my/test-mode-keywords '(("keyword" . font-lock-keyword-face)))
	      (setq font-lock-defaults '(my/test-mode-keywords)))
            nil 'local))

1. emacs -Q
2. Evaluate the major mode definition
3. Open the file
4. Answer "y" in the unsafe variable prompt
5. Observe "keyword" not being fontified.
6. Expected: "keyword" fontified using font-lock-keyword-face.

I can also reproduce using
(define-derived-mode my/test-mode text-mode "Test"
""
(hack-local-variables)
(setq-local my/test-mode-keywords '(("keyword" . font-lock-keyword-face)))
(setq font-lock-defaults '(my/test-mode-keywords)))

The fontification works as expected with
(define-derived-mode my/test-mode text-mode "Test"
""
(setq-local my/test-mode-keywords '(("keyword" . font-lock-keyword-face)))
(setq font-lock-defaults '(my/test-mode-keywords)))

Also reproduced on Emacs master.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#58888: 28.1.90; font-lock-defaults not respected when hack-local-variables unsafe variable dialogue is displayed before setting the defaults
  2022-10-30  6:58 bug#58888: 28.1.90; font-lock-defaults not respected when hack-local-variables unsafe variable dialogue is displayed before setting the defaults Ihor Radchenko
@ 2022-10-31  2:15 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2022-10-31  7:11   ` Ihor Radchenko
  0 siblings, 1 reply; 14+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-10-31  2:15 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: 58888

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

> ------------
> # -*- mode:my/test -*-
> This is test with keyword to be fontified.
>
> # Local Variables:
> # eval: (setq unsafe-variable t)
> # End:
> -------------
>
> Then, consider the following major mode:
>
> (define-derived-mode my/test-mode text-mode "Test"
>   ""
>   (add-hook 'hack-local-variables-hook
> 	    (lambda ()
> 	      (setq-local my/test-mode-keywords '(("keyword" . font-lock-keyword-face)))
> 	      (setq font-lock-defaults '(my/test-mode-keywords)))
>             nil 'local))
>
> 1. emacs -Q
> 2. Evaluate the major mode definition
> 3. Open the file
> 4. Answer "y" in the unsafe variable prompt
> 5. Observe "keyword" not being fontified.
> 6. Expected: "keyword" fontified using font-lock-keyword-face.

Hmm... AFAICT the problem here is that the implementation of
`global-font-lock-mode` ends up trying to enable `font-lock-mode` in
that file's buffer during execution of the
`after-major-mode-change-hook` of *another* buffer while querying
whether to obey those file-local settings, and then fails to try again
when the hook is run in the desired buffer.

I suspect the patch below might help (requires recompiling
`font-core.el` and re-dumping Emacs), but as the comment in there
explains it might not always be sufficient either.

Hmm...


        Stefan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: easy-mmode.patch --]
[-- Type: text/x-diff, Size: 3698 bytes --]

diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index 7d54a84687b..4d2174fbe6a 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -485,6 +485,8 @@ define-globalized-minor-mode
 	 (extra-keywords nil)
          (MODE-variable mode)
 	 (MODE-buffers (intern (concat global-mode-name "-buffers")))
+	 (MODE-enable-in-buffer
+	  (intern (concat global-mode-name "-enable-in-buffer")))
 	 (MODE-enable-in-buffers
 	  (intern (concat global-mode-name "-enable-in-buffers")))
 	 (MODE-check-buffers
@@ -551,10 +553,10 @@ define-globalized-minor-mode
 	 (if ,global-mode
 	     (progn
 	       (add-hook 'after-change-major-mode-hook
-			 #',MODE-enable-in-buffers)
+			 #',MODE-enable-in-buffer)
 	       (add-hook 'find-file-hook #',MODE-check-buffers)
 	       (add-hook 'change-major-mode-hook #',MODE-cmhh))
-	   (remove-hook 'after-change-major-mode-hook #',MODE-enable-in-buffers)
+	   (remove-hook 'after-change-major-mode-hook #',MODE-enable-in-buffer)
 	   (remove-hook 'find-file-hook #',MODE-check-buffers)
 	   (remove-hook 'change-major-mode-hook #',MODE-cmhh))
 
@@ -601,7 +603,32 @@ define-globalized-minor-mode
        ;; List of buffers left to process.
        (defvar ,MODE-buffers nil)
 
-       ;; The function that calls TURN-ON in each buffer.
+       ;; The function that calls TURN-ON in the current buffer.
+       (defun ,MODE-enable-in-buffer ()
+         ;; Remove ourselves from the list of pending buffers.
+         (setq ,MODE-buffers (delq (current-buffer) ,MODE-buffers))
+         (unless ,MODE-set-explicitly
+           (unless (eq ,MODE-major-mode major-mode)
+             (if ,MODE-variable
+                 (progn
+                   (,mode -1)
+                   (funcall ,turn-on-function))
+               (funcall ,turn-on-function))))
+         (setq ,MODE-major-mode major-mode))
+       (put ',MODE-enable-in-buffer 'definition-name ',global-mode)
+
+       ;; In the normal case, major modes run `after-change-major-mode-hook'
+       ;; which will have called `MODE-enable-in-buffer' for us.  But some
+       ;; major modes don't use `define-derived-mode' and thus fail to run
+       ;; `after-change-major-mode-hook', so have a combination of ugly hacks
+       ;; to try and catch those corner cases by listening to
+       ;; `change-major-mode-hook' to discover potential candidates and then
+       ;; checking in `post-command-hook' and `find-file-hook' if some of those
+       ;; still haven't run `after-change-major-mode-hook'.
+       ;; FIXME: We should try a get rid of this ugly hack and rely purely
+       ;; on `after-change-major-mode-hook' because they can (and do) end up
+       ;; running `MODE-enable-in-buffer' too early (when the major isn't yet
+       ;; fully setup) in some cases (see bug#58888).
        (defun ,MODE-enable-in-buffers ()
          (let ((buffers ,MODE-buffers))
            ;; Clear MODE-buffers to avoid scanning the same list of
@@ -611,14 +638,7 @@ define-globalized-minor-mode
            (dolist (buf buffers)
              (when (buffer-live-p buf)
                (with-current-buffer buf
-                 (unless ,MODE-set-explicitly
-                   (unless (eq ,MODE-major-mode major-mode)
-                     (if ,MODE-variable
-                         (progn
-                           (,mode -1)
-                           (funcall ,turn-on-function))
-                       (funcall ,turn-on-function))))
-                 (setq ,MODE-major-mode major-mode))))))
+                 (,MODE-enable-in-buffer))))))
        (put ',MODE-enable-in-buffers 'definition-name ',global-mode)
 
        (defun ,MODE-check-buffers ()

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* bug#58888: 28.1.90; font-lock-defaults not respected when hack-local-variables unsafe variable dialogue is displayed before setting the defaults
  2022-10-31  2:15 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2022-10-31  7:11   ` Ihor Radchenko
  2024-04-08 12:48     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 14+ messages in thread
From: Ihor Radchenko @ 2022-10-31  7:11 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 58888

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

> Hmm... AFAICT the problem here is that the implementation of
> `global-font-lock-mode` ends up trying to enable `font-lock-mode` in
> that file's buffer during execution of the
> `after-major-mode-change-hook` of *another* buffer while querying
> whether to obey those file-local settings, and then fails to try again
> when the hook is run in the desired buffer.
>
> I suspect the patch below might help (requires recompiling
> `font-core.el` and re-dumping Emacs), but as the comment in there
> explains it might not always be sufficient either.

I tried the patch via make extraclean; make bootstrap
I can still reproduce the original recipe.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#58888: 28.1.90; font-lock-defaults not respected when hack-local-variables unsafe variable dialogue is displayed before setting the defaults
  2022-10-31  7:11   ` Ihor Radchenko
@ 2024-04-08 12:48     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-04-08 13:15       ` Eli Zaretskii
  2024-04-08 19:25       ` Ihor Radchenko
  0 siblings, 2 replies; 14+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-08 12:48 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: 58888

[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]

>> Hmm... AFAICT the problem here is that the implementation of
>> `global-font-lock-mode` ends up trying to enable `font-lock-mode` in
>> that file's buffer during execution of the
>> `after-major-mode-change-hook` of *another* buffer while querying
>> whether to obey those file-local settings, and then fails to try again
>> when the hook is run in the desired buffer.
>>
>> I suspect the patch below might help (requires recompiling
>> `font-core.el` and re-dumping Emacs), but as the comment in there
>> explains it might not always be sufficient either.
>
> I tried the patch via make extraclean; make bootstrap
> I can still reproduce the original recipe.

I pushed that patch to m`aster` because it fixed other cases
(e.g. bug#69431) but I think to fix bug#58888 we need the next step,
which is the patch below.

Can you confirm that it fixes it for you as well?

It will remove support for globalized minor modes in those few remaining
major modes which don't use `run-mode-hooks`.  `run-mode-hooks` was
introduced in Emacs-22 and for many years it was really important to
support modes which don't use it, but nowadays those modes are
vanishingly rare and really should get fixed, so I think it's time to
get rid of those ugly hacks.


        Stefan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: easy-mmode.patch --]
[-- Type: text/x-diff, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#58888: 28.1.90; font-lock-defaults not respected when hack-local-variables unsafe variable dialogue is displayed before setting the defaults
  2024-04-08 12:48     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-08 13:15       ` Eli Zaretskii
  2024-04-08 13:42         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-04-08 19:25       ` Ihor Radchenko
  1 sibling, 1 reply; 14+ messages in thread
From: Eli Zaretskii @ 2024-04-08 13:15 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: yantar92, 58888

> Cc: 58888@debbugs.gnu.org
> Date: Mon, 08 Apr 2024 08:48:36 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> I pushed that patch to m`aster` because it fixed other cases
> (e.g. bug#69431) but I think to fix bug#58888 we need the next step,
> which is the patch below.

ENOPATCH





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#58888: 28.1.90; font-lock-defaults not respected when hack-local-variables unsafe variable dialogue is displayed before setting the defaults
  2024-04-08 13:15       ` Eli Zaretskii
@ 2024-04-08 13:42         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 0 replies; 14+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-08 13:42 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: yantar92, 58888

[-- Attachment #1: Type: text/plain, Size: 331 bytes --]

>> I pushed that patch to m`aster` because it fixed other cases
>> (e.g. bug#69431) but I think to fix bug#58888 we need the next step,
>> which is the patch below.
>
> ENOPATCH

[ Hmm... not sure how I managed to make that empty file.
  I'll put this in the "creativity" column.  ]

I think this one isn't empty,


        Stefan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: easy-mmode.patch --]
[-- Type: text/x-diff, Size: 4043 bytes --]

diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index 095bd5faa03..eaad9646985 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -495,11 +495,6 @@ define-globalized-minor-mode
 	 (MODE-buffers (intern (concat global-mode-name "-buffers")))
 	 (MODE-enable-in-buffer
 	  (intern (concat global-mode-name "-enable-in-buffer")))
-	 (MODE-enable-in-buffers
-	  (intern (concat global-mode-name "-enable-in-buffers")))
-	 (MODE-check-buffers
-	  (intern (concat global-mode-name "-check-buffers")))
-	 (MODE-cmhh (intern (concat global-mode-name "-cmhh")))
 	 (minor-MODE-hook (intern (concat mode-name "-hook")))
 	 (MODE-set-explicitly (intern (concat mode-name "-set-explicitly")))
 	 (MODE-major-mode (intern (concat (symbol-name mode) "-major-mode")))
@@ -559,14 +554,9 @@ define-globalized-minor-mode
 
 	 ;; Setup hook to handle future mode changes and new buffers.
 	 (if ,global-mode
-	     (progn
-	       (add-hook 'after-change-major-mode-hook
-			 #',MODE-enable-in-buffer)
-	       (add-hook 'find-file-hook #',MODE-check-buffers)
-	       (add-hook 'change-major-mode-hook #',MODE-cmhh))
-	   (remove-hook 'after-change-major-mode-hook #',MODE-enable-in-buffer)
-	   (remove-hook 'find-file-hook #',MODE-check-buffers)
-	   (remove-hook 'change-major-mode-hook #',MODE-cmhh))
+	     (add-hook 'after-change-major-mode-hook
+		       #',MODE-enable-in-buffer)
+	   (remove-hook 'after-change-major-mode-hook #',MODE-enable-in-buffer))
 
 	 ;; Go through existing buffers.
 	 (dolist (buf (buffer-list))
@@ -623,47 +613,7 @@ define-globalized-minor-mode
                    (funcall ,turn-on-function))
                (funcall ,turn-on-function))))
          (setq ,MODE-major-mode major-mode))
-       (put ',MODE-enable-in-buffer 'definition-name ',global-mode)
-
-       ;; In the normal case, major modes run `after-change-major-mode-hook'
-       ;; which will have called `MODE-enable-in-buffer' for us.  But some
-       ;; major modes don't use `run-mode-hooks' (customarily used via
-       ;; `define-derived-mode') and thus fail to run
-       ;; `after-change-major-mode-hook'.
-       ;; The functions below try to handle those major modes, with
-       ;; a combination of ugly hacks to try and catch those corner
-       ;; cases by listening to `change-major-mode-hook' to discover
-       ;; potential candidates and then checking in `post-command-hook'
-       ;; and `find-file-hook' if some of those still haven't run
-       ;; `after-change-major-mode-hook'.  FIXME: We should try and get
-       ;; rid of this ugly hack and rely purely on
-       ;; `after-change-major-mode-hook' because they can (and do) end
-       ;; up running `MODE-enable-in-buffer' too early (when the major
-       ;; isn't yet fully setup) in some cases (see bug#58888).
-
-       ;; The function that calls TURN-ON in each buffer.
-       (defun ,MODE-enable-in-buffers ()
-         (let ((buffers ,MODE-buffers))
-           ;; Clear MODE-buffers to avoid scanning the same list of
-           ;; buffers in recursive calls to MODE-enable-in-buffers.
-           ;; Otherwise it could lead to infinite recursion.
-           (setq ,MODE-buffers nil)
-           (dolist (buf buffers)
-             (when (buffer-live-p buf)
-              (with-current-buffer buf
-                (,MODE-enable-in-buffer))))))
-       (put ',MODE-enable-in-buffers 'definition-name ',global-mode)
-
-       (defun ,MODE-check-buffers ()
-	 (,MODE-enable-in-buffers)
-	 (remove-hook 'post-command-hook #',MODE-check-buffers))
-       (put ',MODE-check-buffers 'definition-name ',global-mode)
-
-       ;; The function that catches kill-all-local-variables.
-       (defun ,MODE-cmhh ()
-	 (add-to-list ',MODE-buffers (current-buffer))
-	 (add-hook 'post-command-hook #',MODE-check-buffers))
-       (put ',MODE-cmhh 'definition-name ',global-mode))))
+       (put ',MODE-enable-in-buffer 'definition-name ',global-mode))))
 
 (defun easy-mmode--globalized-predicate-p (predicate)
   (cond

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* bug#58888: 28.1.90; font-lock-defaults not respected when hack-local-variables unsafe variable dialogue is displayed before setting the defaults
  2024-04-08 12:48     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-04-08 13:15       ` Eli Zaretskii
@ 2024-04-08 19:25       ` Ihor Radchenko
  2024-04-10 20:34         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-04-11 13:53         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 2 replies; 14+ messages in thread
From: Ihor Radchenko @ 2024-04-08 19:25 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 58888

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

>> I tried the patch via make extraclean; make bootstrap
>> I can still reproduce the original recipe.
>
> I pushed that patch to m`aster` because it fixed other cases
> (e.g. bug#69431) but I think to fix bug#58888 we need the next step,
> which is the patch below.
>
> Can you confirm that it fixes it for you as well?

Yes, after make bootstrap, I am no longer able to reproduce the recipe.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#58888: 28.1.90; font-lock-defaults not respected when hack-local-variables unsafe variable dialogue is displayed before setting the defaults
  2024-04-08 19:25       ` Ihor Radchenko
@ 2024-04-10 20:34         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-04-11  6:20           ` Eli Zaretskii
  2024-04-11 13:53         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 1 reply; 14+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-10 20:34 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: 58888

[-- Attachment #1: Type: text/plain, Size: 377 bytes --]

>> I pushed that patch to m`aster` because it fixed other cases
>> (e.g. bug#69431) but I think to fix bug#58888 we need the next step,
>> which is the patch below.
>>
>> Can you confirm that it fixes it for you as well?
>
> Yes, after make bootstrap, I am no longer able to reproduce the recipe.

Thanks for confirming.
Eli, any objection to the patch below?


        Stefan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-define-globalized-minor-mode-Require-the-use-of-run-.patch --]
[-- Type: text/x-diff, Size: 5622 bytes --]

From 4fd9a51b4d9f2e2e0c04341eeabb656884059301 Mon Sep 17 00:00:00 2001
From: Stefan Monnier <monnier@iro.umontreal.ca>
Date: Wed, 10 Apr 2024 16:31:58 -0400
Subject: [PATCH] (define-globalized-minor-mode): Require the use of
 `run-mode-hooks`

When `define-globalized-minor-mode` was introduced (Emacs-22),
`run-mode-hooks` was brand new, so we could not expect all major
modes to use it and we had to rely on brittle workarounds to try
and approximate `after-change-major-mode-hook`.

These workarounds have undesirable side effects, and they're not
needed any more now that virtually all major modes have been
changed to use `run-mode-hooks`.

* lisp/emacs-lisp/easy-mmode.el (define-globalized-minor-mode):
Rely only on `after-change-major-mode-hook`.  Fixes bug#58888.
---
 etc/NEWS                      |  6 ++++
 lisp/emacs-lisp/easy-mmode.el | 58 +++--------------------------------
 2 files changed, 10 insertions(+), 54 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index a2a3fe494cb..0da59201e55 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1658,6 +1658,12 @@ documentation and examples.
 \f
 * Incompatible Lisp Changes in Emacs 30.1
 
+** 'define-globalized-minor-mode' requires that modes use 'run-mode-hooks'.
+Minor modes defined with 'define-globalized-minor-mode', such as
+'global-font-lock-mode', don't work any more with major modes which
+don't use 'run-mode-hooks'.  Major modes defined with
+'define-derived-mode' are not affected.
+
 ---
 ** Old derived.el functions removed.
 The following functions have been deleted because they were only used
diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index 095bd5faa03..eaad9646985 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -495,11 +495,6 @@ define-globalized-minor-mode
 	 (MODE-buffers (intern (concat global-mode-name "-buffers")))
 	 (MODE-enable-in-buffer
 	  (intern (concat global-mode-name "-enable-in-buffer")))
-	 (MODE-enable-in-buffers
-	  (intern (concat global-mode-name "-enable-in-buffers")))
-	 (MODE-check-buffers
-	  (intern (concat global-mode-name "-check-buffers")))
-	 (MODE-cmhh (intern (concat global-mode-name "-cmhh")))
 	 (minor-MODE-hook (intern (concat mode-name "-hook")))
 	 (MODE-set-explicitly (intern (concat mode-name "-set-explicitly")))
 	 (MODE-major-mode (intern (concat (symbol-name mode) "-major-mode")))
@@ -559,14 +554,9 @@ define-globalized-minor-mode
 
 	 ;; Setup hook to handle future mode changes and new buffers.
 	 (if ,global-mode
-	     (progn
-	       (add-hook 'after-change-major-mode-hook
-			 #',MODE-enable-in-buffer)
-	       (add-hook 'find-file-hook #',MODE-check-buffers)
-	       (add-hook 'change-major-mode-hook #',MODE-cmhh))
-	   (remove-hook 'after-change-major-mode-hook #',MODE-enable-in-buffer)
-	   (remove-hook 'find-file-hook #',MODE-check-buffers)
-	   (remove-hook 'change-major-mode-hook #',MODE-cmhh))
+	     (add-hook 'after-change-major-mode-hook
+		       #',MODE-enable-in-buffer)
+	   (remove-hook 'after-change-major-mode-hook #',MODE-enable-in-buffer))
 
 	 ;; Go through existing buffers.
 	 (dolist (buf (buffer-list))
@@ -623,47 +613,7 @@ define-globalized-minor-mode
                    (funcall ,turn-on-function))
                (funcall ,turn-on-function))))
          (setq ,MODE-major-mode major-mode))
-       (put ',MODE-enable-in-buffer 'definition-name ',global-mode)
-
-       ;; In the normal case, major modes run `after-change-major-mode-hook'
-       ;; which will have called `MODE-enable-in-buffer' for us.  But some
-       ;; major modes don't use `run-mode-hooks' (customarily used via
-       ;; `define-derived-mode') and thus fail to run
-       ;; `after-change-major-mode-hook'.
-       ;; The functions below try to handle those major modes, with
-       ;; a combination of ugly hacks to try and catch those corner
-       ;; cases by listening to `change-major-mode-hook' to discover
-       ;; potential candidates and then checking in `post-command-hook'
-       ;; and `find-file-hook' if some of those still haven't run
-       ;; `after-change-major-mode-hook'.  FIXME: We should try and get
-       ;; rid of this ugly hack and rely purely on
-       ;; `after-change-major-mode-hook' because they can (and do) end
-       ;; up running `MODE-enable-in-buffer' too early (when the major
-       ;; isn't yet fully setup) in some cases (see bug#58888).
-
-       ;; The function that calls TURN-ON in each buffer.
-       (defun ,MODE-enable-in-buffers ()
-         (let ((buffers ,MODE-buffers))
-           ;; Clear MODE-buffers to avoid scanning the same list of
-           ;; buffers in recursive calls to MODE-enable-in-buffers.
-           ;; Otherwise it could lead to infinite recursion.
-           (setq ,MODE-buffers nil)
-           (dolist (buf buffers)
-             (when (buffer-live-p buf)
-              (with-current-buffer buf
-                (,MODE-enable-in-buffer))))))
-       (put ',MODE-enable-in-buffers 'definition-name ',global-mode)
-
-       (defun ,MODE-check-buffers ()
-	 (,MODE-enable-in-buffers)
-	 (remove-hook 'post-command-hook #',MODE-check-buffers))
-       (put ',MODE-check-buffers 'definition-name ',global-mode)
-
-       ;; The function that catches kill-all-local-variables.
-       (defun ,MODE-cmhh ()
-	 (add-to-list ',MODE-buffers (current-buffer))
-	 (add-hook 'post-command-hook #',MODE-check-buffers))
-       (put ',MODE-cmhh 'definition-name ',global-mode))))
+       (put ',MODE-enable-in-buffer 'definition-name ',global-mode))))
 
 (defun easy-mmode--globalized-predicate-p (predicate)
   (cond
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* bug#58888: 28.1.90; font-lock-defaults not respected when hack-local-variables unsafe variable dialogue is displayed before setting the defaults
  2024-04-10 20:34         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-11  6:20           ` Eli Zaretskii
  2024-04-11 13:27             ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 14+ messages in thread
From: Eli Zaretskii @ 2024-04-11  6:20 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: yantar92, 58888

> Cc: 58888@debbugs.gnu.org
> Date: Wed, 10 Apr 2024 16:34:48 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Eli, any objection to the patch below?

Any idea how many major modes out there don't use run-mode-hooks?  We
are basically breaking those with this change, right?

> Subject: [PATCH] (define-globalized-minor-mode): Require the use of
>  `run-mode-hooks`
> 
> When `define-globalized-minor-mode` was introduced (Emacs-22),
> `run-mode-hooks` was brand new, so we could not expect all major
> modes to use it and we had to rely on brittle workarounds to try
> and approximate `after-change-major-mode-hook`.
> 
> These workarounds have undesirable side effects, and they're not
> needed any more now that virtually all major modes have been
> changed to use `run-mode-hooks`.
> 
> * lisp/emacs-lisp/easy-mmode.el (define-globalized-minor-mode):
> Rely only on `after-change-major-mode-hook`.  Fixes bug#58888.

Please don't quote `like this`.  (I think you should by now have a
commit hook in your init files that replaces the quoting with our
style, because this seems to be ubiquitous in all your writings.)

> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1658,6 +1658,12 @@ documentation and examples.
>  \f
>  * Incompatible Lisp Changes in Emacs 30.1
>  
> +** 'define-globalized-minor-mode' requires that modes use 'run-mode-hooks'.
> +Minor modes defined with 'define-globalized-minor-mode', such as
> +'global-font-lock-mode', don't work any more with major modes which
> +don't use 'run-mode-hooks'.  Major modes defined with
> +'define-derived-mode' are not affected.

IMO, this NEWS entry is not detailed enough.  First, it should mention
that run-mode-hooks is the modern way of running the mode's hook, a
replacement for run-hooks.  And second, it should at least hint on
what will happen with modes which don't use run-mode-hooks, so that
affected users could identify the problem when they see its signs.

Also, I think we should tell in the ELisp manual that run-mode-hooks
is now a must, not just a preference, and we should mention in the
Minor Modes chapter that the globalized minor modes will work
correctly only if the major mode uses run-mode-hooks to run its hooks.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#58888: 28.1.90; font-lock-defaults not respected when hack-local-variables unsafe variable dialogue is displayed before setting the defaults
  2024-04-11  6:20           ` Eli Zaretskii
@ 2024-04-11 13:27             ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-04-11 14:12               ` Eli Zaretskii
  0 siblings, 1 reply; 14+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-11 13:27 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: yantar92, 58888

> Any idea how many major modes out there don't use run-mode-hooks?

Hard to tell.  I did a quick

    grep '^[^;]*kill-all-local-variables' $(grep -L run-mode-hooks **/*.el)

in our own code, and it shows a fairly large number of occurrences, but
once you look at them it's not clear what to make of them.  Many of them
don't bother to define an actual mode for the buffer, they just call
`kill-all-local-variables` then fill the buffer somehow, occasionally do
some major-mode-like setup such as `use-local-map`, and finally display
the result to the user.

> We are basically breaking those with this change, right?

Yes.  More specifically the minor modes defined with
`define-globalized-minor-mode` will stay inactive in them.

> Please don't quote `like this`.  (I think you should by now have a
> commit hook in your init files that replaces the quoting with our
> style, because this seems to be ubiquitous in all your writings.)

I do my best to follow the ugly `...' convention in ELisp file and the
'...' convention in our other files, but not for anything else.
I strongly prefer the Markdown convention over '...' because it's
a convention enshrined in many tools.  I could live with the Org
convention if you prefer, but I strongly feel that we should
use notational conventions that tools are able to use.

> IMO, this NEWS entry is not detailed enough.  First, it should mention
> that run-mode-hooks is the modern way of running the mode's hook, a
> replacement for run-hooks.  And second, it should at least hint on
> what will happen with modes which don't use run-mode-hooks, so that
> affected users could identify the problem when they see its signs.
>
> Also, I think we should tell in the ELisp manual that run-mode-hooks
> is now a must, not just a preference, and we should mention in the
> Minor Modes chapter that the globalized minor modes will work
> correctly only if the major mode uses run-mode-hooks to run its hooks.

I'll get back to this once we decide we do want the change.


        Stefan






^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#58888: 28.1.90; font-lock-defaults not respected when hack-local-variables unsafe variable dialogue is displayed before setting the defaults
  2024-04-08 19:25       ` Ihor Radchenko
  2024-04-10 20:34         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-11 13:53         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-04-11 14:13           ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  1 sibling, 1 reply; 14+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-11 13:53 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: 58888

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

>>> I tried the patch via make extraclean; make bootstrap
>>> I can still reproduce the original recipe.
>> I pushed that patch to m`aster` because it fixed other cases
>> (e.g. bug#69431) but I think to fix bug#58888 we need the next step,
>> which is the patch below.
>> Can you confirm that it fixes it for you as well?
> Yes, after make bootstrap, I am no longer able to reproduce the recipe.

A more tepid solution might be the patch below: the problem will still
bite those modes defined by hand (as opposed to using
`define-derived-mode`), but it should be sufficient for the most
important cases.

If we don't go for the complete removal of the hacks to support modes
that don't use `run-mode-hooks`, maybe we should try and arrange to
missing calls to `run-mode-hooks` and emit warnings accordingly.


        Stefan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: easy-mmode.patch --]
[-- Type: text/x-diff, Size: 884 bytes --]

diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index 095bd5faa03..580cc0115cf 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -651,7 +651,12 @@ define-globalized-minor-mode
            (dolist (buf buffers)
              (when (buffer-live-p buf)
               (with-current-buffer buf
-                (,MODE-enable-in-buffer))))))
+                ;; If `delay-mode-hooks' is set, it indicates that
+                ;; the current buffer's mode is not fully setup yet,
+                ;; and also that `run-mode-hooks' will be run afterwards
+                ;; anyway, so we don't need to keep BUF in MODE-buffers.
+                (unless delay-mode-hooks
+                  (,MODE-enable-in-buffer)))))))
        (put ',MODE-enable-in-buffers 'definition-name ',global-mode)
 
        (defun ,MODE-check-buffers ()

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* bug#58888: 28.1.90; font-lock-defaults not respected when hack-local-variables unsafe variable dialogue is displayed before setting the defaults
  2024-04-11 13:27             ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-11 14:12               ` Eli Zaretskii
  2024-04-13 14:32                 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 14+ messages in thread
From: Eli Zaretskii @ 2024-04-11 14:12 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: yantar92, 58888

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: yantar92@posteo.net,  58888@debbugs.gnu.org
> Date: Thu, 11 Apr 2024 09:27:44 -0400
> 
> I'll get back to this once we decide we do want the change.

How shall we proceed with this decision?





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#58888: 28.1.90; font-lock-defaults not respected when hack-local-variables unsafe variable dialogue is displayed before setting the defaults
  2024-04-11 13:53         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-11 14:13           ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 0 replies; 14+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-11 14:13 UTC (permalink / raw)
  To: Ihor Radchenko; +Cc: 58888

[-- Attachment #1: Type: text/plain, Size: 268 bytes --]

> A more tepid solution might be the patch below: the problem will still
> bite those modes defined by hand (as opposed to using
> `define-derived-mode`), but it should be sufficient for the most
> important cases.

The version below is a bit better.


        Stefan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: easy-mmode.patch --]
[-- Type: text/x-diff, Size: 893 bytes --]

diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index 095bd5faa03..015ab1d3f84 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -661,8 +661,12 @@ define-globalized-minor-mode
 
        ;; The function that catches kill-all-local-variables.
        (defun ,MODE-cmhh ()
-	 (add-to-list ',MODE-buffers (current-buffer))
-	 (add-hook 'post-command-hook #',MODE-check-buffers))
+         ;; If `delay-mode-hooks' is set, it indicates that the current
+         ;; buffer's mode will run `run-mode-hooks' afterwards anyway,
+         ;; so we don't need to keep BUF in MODE-buffers.
+	 (unless delay-mode-hooks
+	   (add-to-list ',MODE-buffers (current-buffer))
+	   (add-hook 'post-command-hook #',MODE-check-buffers)))
        (put ',MODE-cmhh 'definition-name ',global-mode))))
 
 (defun easy-mmode--globalized-predicate-p (predicate)

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* bug#58888: 28.1.90; font-lock-defaults not respected when hack-local-variables unsafe variable dialogue is displayed before setting the defaults
  2024-04-11 14:12               ` Eli Zaretskii
@ 2024-04-13 14:32                 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 0 replies; 14+ messages in thread
From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-13 14:32 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: yantar92, 58888-done

> How shall we proceed with this decision?

I just pushed it to `master`.
Like a wise man once said, let's "brace for impact".


        Stefan






^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2024-04-13 14:32 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-30  6:58 bug#58888: 28.1.90; font-lock-defaults not respected when hack-local-variables unsafe variable dialogue is displayed before setting the defaults Ihor Radchenko
2022-10-31  2:15 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2022-10-31  7:11   ` Ihor Radchenko
2024-04-08 12:48     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-08 13:15       ` Eli Zaretskii
2024-04-08 13:42         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-08 19:25       ` Ihor Radchenko
2024-04-10 20:34         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-11  6:20           ` Eli Zaretskii
2024-04-11 13:27             ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-11 14:12               ` Eli Zaretskii
2024-04-13 14:32                 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-11 13:53         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-11 14:13           ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).