bug#71814: define-globalized-minor-mode Should predicate variable be defined before?

bug#71814: define-globalized-minor-mode Should predicate variable be defined before?

[Elijah G.]

Hello, i've noticed when defining a globalized minor mode using the
define-globalized-minor-mode macro, gives a byte-compile warning about
the auto-generated :predicate variable not being defined.

I found that it's because the macro defines the predicate user option
after defining the minor mode:

         (put ',global-mode 'globalized-minor-mode t)
         :autoload-end
         (defvar-local ,MODE-major-mode nil))
       ;; The actual global minor-mode
       (define-minor-mode ,global-mode
         ,(concat (format "Toggle %s in all buffers.\n" pretty-name)
                  (internal--format-docstring-line
                   ...)

	 ;; Setup hook to handle future mode changes and new buffers.
	 (if ,global-mode
	     (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))
	   (with-current-buffer buf
             (if ,global-mode (funcall ,turn-on-function)
               (when ,mode (,mode -1)))))
         ,@body)

       ,(when predicate
          `(defcustom ,MODE-predicate ,(car predicate)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

My question is, should it be defined before defining the minor mode?
if not, then why it's defined like that?

I made a little patch for solve this, since i think it would be it would
be beneficial for packages that use that macro (also i barely remember
that there are some built-in packages that defines the user-option
before using the macro for solve this issue).

Thanks.
--8<---------------cut here---------------start------------->8---
diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index ba0f8bad393..69cc1332282 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -525,6 +525,36 @@ define-globalized-minor-mode
          (put ',global-mode 'globalized-minor-mode t)
          :autoload-end
          (defvar-local ,MODE-major-mode nil))
+       ,(when predicate
+          `(defcustom ,MODE-predicate ,(car predicate)
+             ,(format "Which major modes `%s' is switched on in.
+This variable can be either t (all major modes), nil (no major modes),
+or a list of modes and (not modes) to switch use this minor mode or
+not.  For instance
+
+  (c-mode (not message-mode mail-mode) text-mode)
+
+means \"use this mode in all modes derived from `c-mode', don't use in
+modes derived from `message-mode' or `mail-mode', but do use in other
+modes derived from `text-mode'\".  An element with value t means \"use\"
+and nil means \"don't use\".  There's an implicit nil at the end of the
+list."
+                      mode)
+             :type '(choice
+                     (const :tag "Enable in all major modes" t)
+                     (const :tag "Don't enable in any major mode" nil)
+                     (repeat
+                      :tag "Rules (earlier takes precedence)..."
+                      (choice
+                       (const :tag "Enable in all (other) modes" t)
+                       (const :tag "Don't enable in any (other) mode" nil)
+                       (symbol :value fundamental-mode
+                               :tag "Enable in major mode")
+                       (cons :tag "Don't enable in major modes"
+                             (const :tag "Don't enable in..." not)
+                             (repeat (symbol :value fundamental-mode
+                                             :tag "Major mode"))))))
+             ,@group))
        ;; The actual global minor-mode
        (define-minor-mode ,global-mode
          ,(concat (format "Toggle %s in all buffers.\n" pretty-name)
@@ -565,37 +595,6 @@ define-globalized-minor-mode
                (when ,mode (,mode -1)))))
          ,@body)
 
-       ,(when predicate
-          `(defcustom ,MODE-predicate ,(car predicate)
-             ,(format "Which major modes `%s' is switched on in.
-This variable can be either t (all major modes), nil (no major modes),
-or a list of modes and (not modes) to switch use this minor mode or
-not.  For instance
-
-  (c-mode (not message-mode mail-mode) text-mode)
-
-means \"use this mode in all modes derived from `c-mode', don't use in
-modes derived from `message-mode' or `mail-mode', but do use in other
-modes derived from `text-mode'\".  An element with value t means \"use\"
-and nil means \"don't use\".  There's an implicit nil at the end of the
-list."
-                      mode)
-             :type '(choice
-                     (const :tag "Enable in all major modes" t)
-                     (const :tag "Don't enable in any major mode" nil)
-                     (repeat
-                      :tag "Rules (earlier takes precedence)..."
-                      (choice
-                       (const :tag "Enable in all (other) modes" t)
-                       (const :tag "Don't enable in any (other) mode" nil)
-                       (symbol :value fundamental-mode
-                               :tag "Enable in major mode")
-                       (cons :tag "Don't enable in major modes"
-                             (const :tag "Don't enable in..." not)
-                             (repeat (symbol :value fundamental-mode
-                                             :tag "Major mode"))))))
-             ,@group))
-
        ;; Autoloading define-globalized-minor-mode autoloads everything
        ;; up-to-here.
        :autoload-end
--8<---------------cut here---------------end--------------->8---

--
E.G. from Gnus The Emacs Newsreader and E-mail client

bug#71814: define-globalized-minor-mode Should predicate variable be defined before?

[Stefan Kangas]

"Elijah G." <eg642616@gmail.com> writes:

> Hello, i've noticed when defining a globalized minor mode using the
> define-globalized-minor-mode macro, gives a byte-compile warning about
> the auto-generated :predicate variable not being defined.

Thanks for the bug report and proposed fix.

Could you please provide a recipe to reproduce the problem?

bug#71814: define-globalized-minor-mode Should predicate variable be defined before?

[Elijah G]

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

El vie., 28 de junio de 2024 9:27 p. m., Stefan Kangas <
stefankangas@gmail.com> escribió:

> "Elijah G." <eg642616@gmail.com> writes:
>
> > Hello, i've noticed when defining a globalized minor mode using the
> > define-globalized-minor-mode macro, gives a byte-compile warning about
> > the auto-generated :predicate variable not being defined.
>
> Thanks for the bug report and proposed fix.
>
> Could you please provide a recipe to reproduce the problem?
>

Sure.

1. create a .el file
2. insert this snippet into to file:
```
(define-minor-mode test-mode "")
(define-globalized-minor-mode global-test-mode
  test-mode #'ignore
  :group 'test
  :predicate '(prog-mode text-mode))
```
3. byte-compile the file
4. in the compile log buffer should appear this warning:
`Warning: reference to free variable ‘global-test-modes’`
which is the variable generated by the macro.

>

[-- Attachment #2: Type: text/html, Size: 1817 bytes --]

bug#71814: define-globalized-minor-mode Should predicate variable be defined before?

[Stefan Kangas]

Elijah G <eg642616@gmail.com> writes:

> 1. create a .el file
> 2. insert this snippet into to file:
> ```
> (define-minor-mode test-mode "")
> (define-globalized-minor-mode global-test-mode
>   test-mode #'ignore
>   :group 'test
>   :predicate '(prog-mode text-mode))
> ```
> 3. byte-compile the file
> 4. in the compile log buffer should appear this warning:
> `Warning: reference to free variable ‘global-test-modes’`
> which is the variable generated by the macro.

Thanks, I've now fixed this on emacs-30 (commit a65b6aac6b5).