From a74b5de31f676d3a106687a3b972901c22784bff Mon Sep 17 00:00:00 2001 From: "F. Jason Park" Date: Tue, 17 Oct 2023 23:36:12 -0700 Subject: [PATCH] Warn about top-level erc-update-modules calls * doc/misc/erc.texi (Modules): Describe unfavorable practices enacted by third-party modules, like running `erc-update-modules' on load. * lisp/erc/erc.el (erc-modules): Clarify comment in `custom-set' function. (erc--warn-about-aberrant-modules): Tweak warning message. (erc--requiring-module-mode-p): New internal variable. (erc--find-mode): Guard against recursive `erc-update-module' invocations. (Bug#57955) --- doc/misc/erc.texi | 81 ++++++++++++++++++++++++++++------------------- lisp/erc/erc.el | 26 +++++++++++---- 2 files changed, 67 insertions(+), 40 deletions(-) diff --git a/doc/misc/erc.texi b/doc/misc/erc.texi index 3bfa240cacc..10902eac33f 100644 --- a/doc/misc/erc.texi +++ b/doc/misc/erc.texi @@ -412,16 +412,18 @@ One way to add functionality to ERC is to customize which of its many modules are loaded. There is a spiffy customize interface, which may be reached by typing -@kbd{M-x customize-option @key{RET} erc-modules @key{RET}}. -When removing a module outside of the Custom ecosystem, you may wish -to ensure it's disabled by invoking its associated minor-mode toggle -with a nonpositive prefix argument, for example, @kbd{C-u - M-x +@kbd{M-x customize-option @key{RET} erc-modules @key{RET}}. When +removing a module outside of Customize, you may wish to ensure it's +disabled by invoking its associated minor-mode toggle with a +nonpositive prefix argument, for example, @kbd{C-u - M-x erc-spelling-mode @key{RET}}. Additionally, if you plan on loading third-party modules that perform atypical setup on activation, you may need to arrange for calling @code{erc-update-modules} in your init file. Examples of such setup might include registering an @code{erc-before-connect} hook, advising @code{erc-open}, and -modifying @code{erc-modules} itself. +modifying @code{erc-modules} itself. On Emacs 29 and greater, you can +also run @code{erc-update-modules} indirectly, via @code{(setopt +erc-modules erc-modules)}. The following is a list of available modules. @@ -652,41 +654,54 @@ buffers belonging to their connection (when called interactively). And unlike global toggles, none of these ever mutates @code{erc-modules}. - +@c FIXME add section to Advanced chapter for creating modules, and +@c move this there. @anchor{Module Loading} -@subheading Module Loading +@subheading 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{} in @code{erc-modules} lacks a corresponding -@code{erc--mode} command, ERC will attempt to load the library -@code{erc-} prior to connecting. If this fails, ERC signals an -error. Users wanting to define modules in an init files should -@code{(provide 'erc-)} 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. +@code{define-erc-module} (also shown below). For historical reasons, +ERC falls back to @code{require}ing features. For example, if some +module @code{my-module} in @code{erc-modules} lacks a corresponding +@code{erc-my-module-mode} command, ERC will attempt to load the +library @code{erc-my-module} prior to connecting. If this fails, ERC +signals an error. Users defining personal modules in an init file +should @code{(provide 'erc-my-module)} somewhere to placate ERC. +Dynamically generating modules on the fly is not supported. + +Some packages have been known to autoload a module's definition +instead of its minor-mode command, which severs 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. As +such, packages should only supply module-related autoload cookies with +an actual @code{autoload} form for their module's minor-mode command, +like so: + +@lisp +;;;###autoload(autoload 'erc-my-module-mode "erc-my-module" nil t) +(define-erc-module my-module nil + "My doc string." + ((add-hook 'erc-insert-post-hook #'erc-my-module-on-insert-post)) + ((remove-hook 'erc-insert-post-hook #'erc-my-module-on-insert-post))) +@end lisp + +@noindent +As implied earlier, packages can usually omit such cookies entirely so +long as their module's prefixed name matches that of its defining +library and the library's provided feature. + +Finally, packages have also been observed to run +@code{erc-update-modules} in top-level forms, forcing ERC to take +special precautions to avoid recursive invocations. Another +unfortunate practice is mutating @code{erc-modules} itself upon +loading @code{erc}, possibly by way of an autoload. 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 5bf6496e926..a4c52389367 100644 --- a/lisp/erc/erc.el +++ b/lisp/erc/erc.el @@ -2047,6 +2047,7 @@ removed from the list will be disabled." (when (symbol-value f) (message "Disabling `erc-%s'" module) (funcall f 0)) + ;; Disable local module in all ERC buffers. (unless (or (custom-variable-p f) (not (fboundp 'erc-buffer-filter))) (erc-buffer-filter (lambda () @@ -2055,8 +2056,8 @@ removed from the list will be disabled." (kill-local-variable f))))))))) ;; 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. + ;; Don't initialize modules on load, even though the rare + ;; third-party module may need it. (when (fboundp 'erc-update-modules) (unless erc--inside-mode-toggle-p (erc-update-modules)))) @@ -2129,12 +2130,17 @@ Except ignore all local modules, which were introduced in ERC 5.5." (defun erc--warn-about-aberrant-modules () (when (and erc--aberrant-modules (not erc--target)) (erc-button--display-error-notice-with-keys-and-warn - "The following modules exhibited strange loading behavior: " + "The following modules likely engage in unfavorable loading practices: " (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))) +(defvar erc--requiring-module-mode-p nil + "Non-nil while doing (require \\='erc-mymod) for `mymod' in `erc-modules'. +Used for inhibiting potentially recursive `erc-update-modules' +invocations by third-party packages.") + (defun erc--find-mode (sym) (setq sym (erc--normalize-module-symbol sym)) (if-let ((mode (intern-soft (concat "erc-" (symbol-name sym) "-mode"))) @@ -2144,10 +2150,16 @@ Except ignore all local modules, which were introduced in ERC 5.5." (symbol-file mode) (ignore (cl-pushnew sym erc--aberrant-modules))))) mode - (and (require (or (get sym 'erc--feature) - (intern (concat "erc-" (symbol-name sym)))) - nil 'noerror) - (setq mode (intern-soft (concat "erc-" (symbol-name sym) "-mode"))) + (and (or (and erc--requiring-module-mode-p + ;; Also likely non-nil: (eq sym (car features)) + (cl-pushnew sym erc--aberrant-modules)) + (let ((erc--requiring-module-mode-p t)) + (require (or (get sym 'erc--feature) + (intern (concat "erc-" (symbol-name sym)))) + nil 'noerror)) + (memq sym erc--aberrant-modules)) + (or mode (setq mode (intern-soft (concat "erc-" (symbol-name sym) + "-mode")))) (fboundp mode) mode))) -- 2.39.2