Subject: Re: bug#57955: 29.0.50; Allow session-local ERC modules

From: "J.P." <[email-masked]>
Date: Sun, 08 Oct 2023 21:02:02 -0700

It turns out these changes removed some behavior involving the loading
of modules that's long been part of ERC's implicit interface. Basically,
when encountering a third-party module `mymod', ERC has always attempted
to `require' the (possibly nonexistent) feature `erc-mymod', and to do
so unconditionally. However, this bug changed that policy to instead
only attempt such loading if the corresponding command `erc-mymod-mode'
is undefined. That was likely unwise because some packages do
questionable things like

  ;;;###autoload
  (eval-after-load 'erc
    '(define-erc-module mymod nil "Doc" () ()))

which fails to provide the vital `symbol-file' association between the
minor-mode command and the library (because the command itself isn't
autoloaded). Such uses ignore the decades-old example in the doc string
of `define-erc-module', which clearly recommends

  ;;;###autoload(autoload 'erc-mymode-mode "erc-mymode")
  (define-erc-module mymod nil "Doc" () ())

as the surefire approach. That said, in cases where the library's name
matches the (prefixed) module name, no autoload cookie is necessary.
Unfortunately, many of these packages are in maintenance mode, with
authors unwilling to respond to suggestions to update such aberrant
uses. Thus, I think it's in ERC's best interest to accommodate them as
it always has. Please see the second patch below. Thanks.

>From 01c7045757c6109e07053dcf3a712b74a1d34d41 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <[email-masked]>
Date: Thu, 5 Oct 2023 00:16:46 -0700
Subject: [PATCH 1/2] [5.6] Honor nil values in erc--restore-initialize-priors

* lisp/erc/erc.el (erc--restore-initialize-priors): Don't produce
invalid empty `setq' when given VARS that initialize to nil.  This
function is mainly used by local modules, which were first introduced
in 5.5 (bug#57955).
* test/lisp/erc/erc-tests.el (erc--restore-initialize-priors): Fix
expected expansion.  (Bug#60936)
---
 lisp/erc/erc.el            | 17 ++++++++---------
 test/lisp/erc/erc-tests.el | 17 +++++++----------
 2 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index fb236f1f189..16651b41eef 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1366,16 +1366,15 @@ erc--target-priors
 (defmacro erc--restore-initialize-priors (mode &rest vars)
   "Restore local VARS for MODE from a previous session."
   (declare (indent 1))
-  (let ((existing (make-symbol "existing"))
+  (let ((priors (make-symbol "priors"))
+        (initp (make-symbol "initp"))
         ;;
-        restore initialize)
-    (while-let ((k (pop vars)) (v (pop vars)))
-      (push `(,k (alist-get ',k ,existing)) restore)
-      (push `(,k ,v) initialize))
-    `(if-let* ((,existing (or erc--server-reconnecting erc--target-priors))
-               ((alist-get ',mode ,existing)))
-         (setq ,@(mapcan #'identity (nreverse restore)))
-       (setq ,@(mapcan #'identity (nreverse initialize))))))
+        forms)
+    (while-let ((k (pop vars)))
+      (push `(,k (if ,initp (alist-get ',k ,priors) ,(pop vars))) forms))
+    `(let* ((,priors (or erc--server-reconnecting erc--target-priors))
+            (,initp (and ,priors (alist-get ',mode ,priors))))
+       (setq ,@(mapcan #'identity (nreverse forms))))))
 
 (defun erc--target-from-string (string)
   "Construct an `erc--target' variant from STRING."
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index 8a68eca6196..64b503832f3 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -796,18 +796,15 @@ erc--valid-local-channel-p
       (should (erc--valid-local-channel-p "&local")))))
 
 (ert-deftest erc--restore-initialize-priors ()
-  ;; This `pcase' expands to 100+k.  Guess we could do something like
-  ;; (and `(,_ ((,e . ,_) . ,_) . ,_) v) first and then return a
-  ;; (equal `(if-let* ((,e ...)...)...) v) to cut it down to < 1k.
   (should (pcase (macroexpand-1 '(erc--restore-initialize-priors erc-my-mode
                                    foo (ignore 1 2 3)
-                                   bar #'spam))
-            (`(if-let* ((,e (or erc--server-reconnecting erc--target-priors))
-                        ((alist-get 'erc-my-mode ,e)))
-                  (setq foo (alist-get 'foo ,e)
-                        bar (alist-get 'bar ,e))
-                (setq foo (ignore 1 2 3)
-                      bar #'spam))
+                                   bar #'spam
+                                   baz nil))
+            (`(let* ((,p (or erc--server-reconnecting erc--target-priors))
+                     (,q (and ,p (alist-get 'erc-my-mode ,p))))
+                (setq foo (if ,q (alist-get 'foo ,p) (ignore 1 2 3))
+                      bar (if ,q (alist-get 'bar ,p) #'spam)
+                      baz (if ,q (alist-get 'baz ,p) nil)))
              t))))
 
 (ert-deftest erc--target-from-string ()
-- 
2.41.0

>From 8e85f0748859590f2e94339e3dab300f9de9f728 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <[email-masked]>
Date: Fri, 6 Oct 2023 17:34:04 -0700
Subject: [PATCH 2/2] [5.6] Sort and dedupe when loading modules in erc-open

* doc/misc/erc.texi: Add new subheading "Module Loading" under
"Modules" chapter.
* lisp/erc/erc.el (erc--sort-modules): New utility function to sort
and dedupe modules.
(erc-modules): In `custom-set' function, factor out collation step
into separate utility `erc--sort-modules'.
(erc-update-modules): Call `erc--update-modules' with `erc-modules'.
(erc--aberrant-modules): New variable, a list of symbols whose modules
are suspected of being incorrectly defined.
(erc--find-mode): Make heuristic more robust by always checking for a
mode activation command rather than just a state variable.  This fixes
a compatibility bug, new in 5.6, affecting third-party modules that
autoload their module definition instead of its corresponding
activation-toggle command.
(erc--update-modules): Add new positional argument `modules'.
(erc--setup-buffer-hook): Add new default member,
`erc--warn-about-aberrant-modules'.
(erc-open): Pass sorted `erc-modules' to `erc--update-modules'.
* test/lisp/erc/erc-tests.el (erc--sort-modules): New test.
(erc-tests--update-modules): New fixture.
(erc--update-modules): Remove and rework as three separate tests
dedicated to specific contexts.  The existing one had poor coverage
and was difficult if not impossible to follow.
(erc--update-modules/unknown, erc--update-modules/local,
erc--update-modules/realistic): New tests.  (Bug#57955)
---
 doc/misc/erc.texi          |  35 ++++++++
 lisp/erc/erc.el            |  52 ++++++++----
 test/lisp/erc/erc-tests.el | 163 ++++++++++++++++++++++++++-----------
 3 files changed, 183 insertions(+), 67 deletions(-)

diff --git a/doc/misc/erc.texi b/doc/misc/erc.texi
index 3297d8b17f0..3bfa240cacc 100644
--- a/doc/misc/erc.texi
+++ b/doc/misc/erc.texi
@@ -653,6 +653,41 @@ Modules
 @code{erc-modules}.

+@anchor{Module Loading}
+@subheading Module Loading
+@cindex module loading
+
+ERC loads internal modules in alphabetical order and third-party
+modules as they appear in @code{erc-modules}.  When defining your own
+module, take care to ensure ERC can find it.  An easy way to do that
+is by mimicking the example in the doc string for
+@code{define-erc-module}.  For historical reasons, ERC also falls back
+to @code{require}ing features.  For example, if some module
+@code{<mymod>} in @code{erc-modules} lacks a corresponding
+@code{erc-<mymod>-mode} command, ERC will attempt to load the library
+@code{erc-<mymod>} prior to connecting.  If this fails, ERC signals an
+error.  Users wanting to define modules in an init files should
+@code{(provide 'erc-<my-mod>)} somewhere to placate ERC.  Dynamically
+generating modules on the fly is not supported.
+
+Sometimes, packages attempt to autoload a module's definition instead
+of its minor-mode command, which breaks the link between the library
+and the module.  This means that enabling the mode by invoking its
+command toggle isn't enough to load its defining library.  Such
+packages should instead only supply autoload cookies featuring an
+explicit @code{autoload} form for their module's minor-mode command.
+As mentioned above, packages can also usually avoid autoload cookies
+entirely so long as their module's prefixed name matches that of its
+defining library and the latter's provided feature.
+
+Packages have also been seen to specify unnecessary top-level
+@code{eval-after-load} forms, which end up being ineffective in most
+cases.  Another unfortunate practice is mutating @code{erc-modules}
+itself in an autoloaded form.  Doing this tricks Customize into
+displaying the widget for @code{erc-modules} incorrectly, with
+built-in modules moved from the predefined checklist to the
+user-provided free-form area.
+
 @c PRE5_4: Document every option of every module in its own subnode

diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index 16651b41eef..60d745e1268 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -2004,6 +2004,13 @@ erc-migrate-modules
   ;; each item is in the format '(old . new)
   (delete-dups (mapcar #'erc--normalize-module-symbol mods)))
 
+(defun erc--sort-modules (modules)
+  (let (built-in third-party)
+    (dolist (mod modules)
+      (setq mod (erc--normalize-module-symbol mod))
+      (cl-pushnew mod (if (get mod 'erc--module) built-in third-party)))
+    `(,@(sort built-in #'string-lessp) ,@(nreverse third-party))))
+
 (defcustom erc-modules '( autojoin button completion fill imenu irccontrols
                           list match menu move-to-prompt netsplit
                           networks noncommands readonly ring stamp track)
@@ -2039,16 +2046,10 @@ erc-modules
                                           (when (symbol-value f)
                                             (funcall f 0))
                                           (kill-local-variable f)))))))))
-         (let (built-in third-party)
-           (dolist (v val)
-             (setq v (erc--normalize-module-symbol v))
-             (if (get v 'erc--module)
-                 (push v built-in)
-               (push v third-party)))
-           ;; Calling `set-default-toplevel-value' complicates testing
-           (set sym (append (sort built-in #'string-lessp)
-                            (nreverse third-party))))
+         ;; Calling `set-default-toplevel-value' complicates testing.
+         (set sym (erc--sort-modules val))
          ;; this test is for the case where erc hasn't been loaded yet
+         ;; FIXME explain how this ^ can occur or remove comment.
          (when (fboundp 'erc-update-modules)
            (unless erc--inside-mode-toggle-p
              (erc-update-modules))))
@@ -2112,15 +2113,29 @@ erc-modules
 (defun erc-update-modules ()
   "Enable minor mode for every module in `erc-modules'.
 Except ignore all local modules, which were introduced in ERC 5.5."
-  (erc--update-modules)
+  (erc--update-modules erc-modules)
   nil)
 
+(defvar erc--aberrant-modules nil
+  "Modules suspected of being improperly loaded.")
+
+(defun erc--warn-about-aberrant-modules ()
+  (when erc--aberrant-modules
+    (erc-button--display-error-notice-with-keys-and-warn
+     "The following modules exhibited strange loading behavior: "
+     (mapconcat (lambda (s) (format "`%s'" s)) erc--aberrant-modules ", ")
+     ". Please contact ERC with \\[erc-bug] if you believe this to be untrue."
+     " See Info:\"(erc) Module Loading\" for more.")
+    (setq erc--aberrant-modules nil)))
+
 (defun erc--find-mode (sym)
   (setq sym (erc--normalize-module-symbol sym))
-  (if-let* ((mode (intern-soft (concat "erc-" (symbol-name sym) "-mode")))
-            ((or (boundp mode)
-                 (and (fboundp mode)
-                      (autoload-do-load (symbol-function mode) mode)))))
+  (if-let ((mode (intern-soft (concat "erc-" (symbol-name sym) "-mode")))
+           ((and (fboundp mode)
+                 (autoload-do-load (symbol-function mode) mode)))
+           ((or (get sym 'erc--module)
+                (symbol-file mode)
+                (ignore (cl-pushnew sym erc--aberrant-modules)))))
       mode
     (and (require (or (get sym 'erc--feature)
                       (intern (concat "erc-" (symbol-name sym))))
@@ -2129,9 +2144,9 @@ erc--find-mode
          (fboundp mode)
          mode)))
 
-(defun erc--update-modules ()
+(defun erc--update-modules (modules)
   (let (local-modes)
-    (dolist (module erc-modules local-modes)
+    (dolist (module modules local-modes)
       (if-let ((mode (erc--find-mode module)))
           (if (custom-variable-p mode)
               (funcall mode 1)
@@ -2158,7 +2173,7 @@ erc--updating-modules-p
 confidently call (erc-foo-mode 1) without having to learn
 anything about the dependency's implementation.")
 
-(defvar erc--setup-buffer-hook nil
+(defvar erc--setup-buffer-hook '(erc--warn-about-aberrant-modules)
   "Internal hook for module setup involving windows and frames.")
 
 (defvar erc--display-context nil
@@ -2315,7 +2330,8 @@ erc-open
     (setq old-point (point))
     (setq delayed-modules
           (erc--merge-local-modes (let ((erc--updating-modules-p t))
-                                    (erc--update-modules))
+                                    (erc--update-modules
+                                     (erc--sort-modules erc-modules)))
                                   (or erc--server-reconnecting
                                       erc--target-priors)))
 
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index 64b503832f3..0b88ad9cfa9 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -2293,65 +2293,130 @@ erc--find-group--real
   (should (eq (erc--find-group 'smiley nil) 'erc))
   (should (eq (erc--find-group 'unmorse nil) 'erc)))
 
-(ert-deftest erc--update-modules ()
-  (let (calls
-        erc-modules
-        erc-kill-channel-hook erc-kill-server-hook erc-kill-buffer-hook)
+(ert-deftest erc--sort-modules ()
+  (should (equal (erc--sort-modules '(networks foo fill bar fill stamp bar))
+                 ;; Third-party mods appear in original order.
+                 '(fill networks stamp foo bar))))
+
+(defun erc-tests--update-modules (fn)
+  (let* ((calls nil)
+         (custom-modes nil)
+         (on-load nil)
+
+         (get-calls (lambda () (prog1 (nreverse calls) (setq calls nil))))
+
+         (add-onload (lambda (m k v)
+                       (put (intern m) 'erc--feature k)
+                       (push (cons k (lambda () (funcall v m))) on-load)))
 
-    ;; This `lbaz' module is unknown, so ERC looks for it via the
-    ;; symbol proerty `erc--feature' and, failing that, by
-    ;; `require'ing its "erc-" prefixed symbol.
-    (should-not (intern-soft "erc-lbaz-mode"))
+         (mk-cmd (lambda (module)
+                   (let ((mode (intern (format "erc-%s-mode" module))))
+                     (fset mode (lambda (n) (push (cons mode n) calls))))))
+
+         (mk-builtin (lambda (module-string)
+                       (let ((s (intern module-string)))
+                         (put s 'erc--module s))))
+
+         (mk-global (lambda (module)
+                      (push (intern (format "erc-%s-mode" module))
+                            custom-modes))))
 
     (cl-letf (((symbol-function 'require)
                (lambda (s &rest _)
-                 (when (eq s 'erc--lbaz-feature)
-                   (fset (intern "erc-lbaz-mode") ; local module
-                         (lambda (n) (push (cons 'lbaz n) calls))))
-                 (push s calls)))
-
-              ;; Local modules
-              ((symbol-function 'erc-lbar-mode)
-               (lambda (n) (push (cons 'lbar n) calls)))
-              ((get 'lbaz 'erc--feature) 'erc--lbaz-feature)
-
-              ;; Global modules
-              ((symbol-function 'erc-gfoo-mode)
-               (lambda (n) (push (cons 'gfoo n) calls)))
-              ((get 'erc-gfoo-mode 'standard-value) 'ignore)
+                 ;; Simulate library being loaded, things defined.
+                 (when-let ((h (alist-get s on-load))) (funcall h))
+                 (push (cons 'req s) calls)))
+
+              ;; Spoof global module detection.
+              ((symbol-function 'custom-variable-p)
+               (lambda (v) (memq v custom-modes))))
+
+      (funcall fn get-calls add-onload mk-cmd mk-builtin mk-global))
+    (should-not erc--aberrant-modules)))
+
+(ert-deftest erc--update-modules/unknown ()
+  (erc-tests--update-modules
+
+   (lambda (get-calls _ mk-cmd _ mk-global)
+
+     (ert-info ("Baseline")
+       (let* ((erc-modules '(foo))
+              (obarray (obarray-make))
+              (err (should-error (erc--update-modules erc-modules))))
+         (should (equal (cadr err) "`foo' is not a known ERC module"))
+         (should (equal (funcall get-calls)
+                        `((req . ,(intern-soft "erc-foo")))))))
+
+     ;; Module's mode command exists but lacks an associated file.
+     (ert-info ("Bad autoload flagged as suspect")
+       (should-not erc--aberrant-modules)
+       (let* ((erc--aberrant-modules nil)
+              (obarray (obarray-make))
+              (erc-modules (list (intern "foo"))))
+
+         ;; Create a mode activation command.
+         (funcall mk-cmd "foo")
+
+         ;; Make the mode var global.
+         (funcall mk-global "foo")
+
+         ;; No local modules to return.
+         (should-not (erc--update-modules erc-modules))
+         (should (equal (mapcar #'prin1-to-string erc--aberrant-modules)
+                        '("foo")))
+         ;; ERC requires the library via prefixed module name.
+         (should (equal (mapcar #'prin1-to-string (funcall get-calls))
+                        `("(req . erc-foo)" "(erc-foo-mode . 1)"))))))))
+
+;; A local module (here, `lo2') lacks a mode toggle, so ERC tries to
+;; load its defining library, first via the symbol property
+;; `erc--feature', and then via an "erc-" prefixed symbol.
+(ert-deftest erc--update-modules/local ()
+  (erc-tests--update-modules
+
+   (lambda (get-calls add-onload mk-cmd mk-builtin mk-global)
+
+     (let* ((obarray (obarray-make 20))
+            (erc-modules (mapcar #'intern '("glo" "lo1" "lo2"))))
+
+       ;; Create a global and a local module.
+       (mapc mk-cmd '("glo" "lo1"))
+       (mapc mk-builtin '("glo" "lo1"))
+       (funcall mk-global "glo")
+       (funcall add-onload "lo2" 'explicit-feature-lib mk-cmd)
+
+       ;; Returns local modules.
+       (should (equal (mapcar #'symbol-name (erc--update-modules erc-modules))
+                      '("erc-lo2-mode" "erc-lo1-mode")))
+
+       ;; Requiring `erc-lo2' defines `erc-lo2-mode'.
+       (should (equal (mapcar #'prin1-to-string (funcall get-calls))
+                      `("(erc-glo-mode . 1)"
+                        "(req . explicit-feature-lib)")))))))
+
+(ert-deftest erc--update-modules/realistic ()
+  (let ((calls nil)
+        ;; Module `pcomplete' "resolves" to `completion'.
+        (erc-modules '(pcomplete autojoin networks)))
+    (cl-letf (((symbol-function 'require)
+               (lambda (s &rest _) (push (cons 'req s) calls)))
+
+              ;; Spoof global module detection.
+              ((symbol-function 'custom-variable-p)
+               (lambda (v)
+                 (memq v '(erc-autojoin-mode erc-networks-mode
+                                             erc-completion-mode))))
+              ;; Mock and spy real builtins.
               ((symbol-function 'erc-autojoin-mode)
                (lambda (n) (push (cons 'autojoin n) calls)))
-              ((get 'erc-autojoin-mode 'standard-value) 'ignore)
               ((symbol-function 'erc-networks-mode)
                (lambda (n) (push (cons 'networks n) calls)))
-              ((get 'erc-networks-mode 'standard-value) 'ignore)
               ((symbol-function 'erc-completion-mode)
-               (lambda (n) (push (cons 'completion n) calls)))
-              ((get 'erc-completion-mode 'standard-value) 'ignore))
-
-      (ert-info ("Unknown module")
-        (setq erc-modules '(lfoo))
-        (should-error (erc--update-modules))
-        (should (equal (pop calls) 'erc-lfoo))
-        (should-not calls))
+               (lambda (n) (push (cons 'completion n) calls))))
 
-      (ert-info ("Local modules")
-        (setq erc-modules '(gfoo lbar lbaz))
-        ;; Don't expose the mode here
-        (should (equal (mapcar #'symbol-name (erc--update-modules))
-                       '("erc-lbaz-mode" "erc-lbar-mode")))
-        ;; Lbaz required because unknown.
-        (should (equal (nreverse calls) '((gfoo . 1) erc--lbaz-feature)))
-        (fmakunbound (intern "erc-lbaz-mode"))
-        (unintern (intern "erc-lbaz-mode") obarray)
-        (setq calls nil))
-
-      (ert-info ("Global modules") ; `pcomplete' resolved to `completion'
-        (setq erc-modules '(pcomplete autojoin networks))
-        (should-not (erc--update-modules)) ; no locals
-        (should (equal (nreverse calls)
-                       '((completion . 1) (autojoin . 1) (networks . 1))))
-        (setq calls nil)))))
+      (should-not (erc--update-modules erc-modules)) ; no locals
+      (should (equal (nreverse calls)
+                     '((completion . 1) (autojoin . 1) (networks . 1)))))))
 
 (ert-deftest erc--merge-local-modes ()
   (cl-letf (((get 'erc-b-mode 'erc-module) 'b)
-- 
2.41.0