From b1f91df9a228b2fb035a5b5815dfc8b7a15b8535 Mon Sep 17 00:00:00 2001 From: "F. Jason Park" Date: Sun, 19 May 2024 23:04:49 -0700 Subject: [PATCH] Return nil from more ERC response handlers * etc/ERC-NEWS: Mention that certain aberrant response handlers now return nil. * lisp/erc/erc-backend.el (define-erc-response-handler): Mention that body should explicitly return nil. (erc-server-PART) (erc-server-PING): Return nil. * lisp/erc/erc-sasl.el (erc-sasl--destroy): Return nil. * lisp/erc/erc.el (erc-display-message): Mention in doc string that the return value is undefined. (erc-kill-channel-hook): Fix package-version. * test/lisp/erc/erc-networks-tests.el (erc-networks--set-name): Ensure `erc--route-insertion' returns nil because this influences whether response-handler hooks continue running. * test/lisp/erc/erc-sasl-tests.el (erc-sasl-create-client-ecdsa): Fix regression that made test unusable, although it's still relatively useless and therefore skipped by default. * test/lisp/erc/erc-services-tests.el (erc-services-tests--auth-source-standard) (erc-services-tests--auth-source-announced): Clarify annotations. * test/lisp/erc/erc-tests.el (erc-message): Don't return non-nil in mocked `erc-display-message'. (erc-send-modify-hook): Shadow `erc-send-modify-hook' because `erc-stamp--date-mode' modifies it locally. (cherry picked from commit 8c54a79ec10d21cfc961476d85db06b643260e38) --- etc/ERC-NEWS | 8 ++++++++ lisp/erc/erc-backend.el | 20 ++++++++++++++++---- lisp/erc/erc-sasl.el | 3 ++- lisp/erc/erc.el | 6 ++++-- test/lisp/erc/erc-networks-tests.el | 2 +- test/lisp/erc/erc-sasl-tests.el | 19 +++++++++++++++---- test/lisp/erc/erc-services-tests.el | 16 +++++++++++----- test/lisp/erc/erc-tests.el | 4 +++- 8 files changed, 60 insertions(+), 18 deletions(-) diff --git a/etc/ERC-NEWS b/etc/ERC-NEWS index 62970f52396..acad0f03572 100644 --- a/etc/ERC-NEWS +++ b/etc/ERC-NEWS @@ -685,6 +685,14 @@ The option 'erc-format-nick-function' has been renamed to actual role. So too has the related function 'erc-format-nick', which is now 'erc-determine-speaker-from user'. +*** All default response handlers return nil. +Actually, this isn't yet true, but ERC is moving in this direction. +The goal is to guarantee that trailing members of response hooks, like +'erc-server-005-functions', have an opportunity to run after the +default handler. For now, certain default handlers that may have +previously returned non-nil, like 'erc-server-PONG' and +'erc-server-904', have been updated to return nil in all cases. + *** A template-based approach to formatting inserted chat messages. Predicting and influencing how ERC formats messages containing a leading "" has never been straightforward. The characters diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el index ef99d762a07..90c46eadaf4 100644 --- a/lisp/erc/erc-backend.el +++ b/lisp/erc/erc-backend.el @@ -1566,13 +1566,23 @@ This creates: `erc-server-NAME'. - a function `erc-server-NAME' with body FN-BODY. +\(Note that here, NAME merely refers to the parameter NAME rather than +an actual IRC response or server-sent command.) + If ALIASES is non-nil, each alias in ALIASES is `defalias'ed to `erc-server-NAME'. Alias hook variables are created as `erc-server-ALIAS-functions' and initialized to the same default value as `erc-server-NAME-functions'. -FN-BODY is the body of `erc-server-NAME' it may refer to the two -function arguments PROC and PARSED. +ERC uses FN-BODY as the body of the default response handler +`erc-server-NAME', which handles all incoming IRC \"NAME\" responses, +unless overridden (see below). ERC calls the function with two +arguments, PROC and PARSED, whose symbols (lowercase) are bound to the +current `erc-server-process' and `erc-response' instance within FN-BODY. +Implementers should take care not to shadow them inadvertently. In all +cases, FN-BODY should return nil to allow third parties to run code +after `erc-server-NAME' returns. For historical reasons, ERC does not +currently enforce this, however future versions very well may. If EXTRA-FN-DOC is non-nil, it is inserted at the beginning of the defined function's docstring. @@ -1902,7 +1912,8 @@ add things to `%s' instead." (when (and erc-kill-buffer-on-part buffer) (defvar erc-killing-buffer-on-part-p) (let ((erc-killing-buffer-on-part-p t)) - (kill-buffer buffer))))))) + (kill-buffer buffer)))))) + nil) (define-erc-response-handler (PING) "Handle ping messages." nil @@ -1914,7 +1925,8 @@ add things to `%s' instead." (erc-display-message parsed 'error proc 'PING ?s (erc-time-diff erc-server-last-ping-time (erc-current-time)))) - (setq erc-server-last-ping-time (erc-current-time)))) + (setq erc-server-last-ping-time (erc-current-time))) + nil) (define-erc-response-handler (PONG) "Handle pong messages." nil diff --git a/lisp/erc/erc-sasl.el b/lisp/erc/erc-sasl.el index f1cc68e2620..1998e4f129b 100644 --- a/lisp/erc/erc-sasl.el +++ b/lisp/erc/erc-sasl.el @@ -373,7 +373,8 @@ This doesn't solicit or validate a suite of supported mechanisms." "Destroy process PROC and warn user that their settings are likely faulty." (delete-process proc) (erc--lwarn 'erc-sasl :error - "Disconnected from %s; please review SASL settings" proc)) + "Disconnected from %s; please review SASL settings" proc) + nil) (define-erc-response-handler (902) "Handle an ERR_NICKLOCKED response." nil diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el index 72fe0e9c9c8..3a2f5ae5ba8 100644 --- a/lisp/erc/erc.el +++ b/lisp/erc/erc.el @@ -3987,7 +3987,9 @@ As of ERC 5.6, assume third-party code will use this function instead of lower-level ones, like `erc-insert-line', to insert arbitrary informative messages as if sent by the server. That is, tell modules to treat a \"local\" message for which PARSED is -nil like any other server-sent message." +nil like any other server-sent message. Finally, expect users to +treat the return value of this function as undefined even though +various default response handlers may appear to presume nil." (let* ((erc--msg-props (or erc--msg-props (let ((table (make-hash-table)) @@ -9625,7 +9627,7 @@ See also `format-spec'." erc-networks-shrink-ids-and-buffer-names erc-networks-rename-surviving-target-buffer) "Invoked whenever a channel-buffer is killed via `kill-buffer'." - :package-version '(ERC . "5.5") + :package-version '(ERC . "5.6") ; FIXME sync on release :group 'erc-hooks :type 'hook) diff --git a/test/lisp/erc/erc-networks-tests.el b/test/lisp/erc/erc-networks-tests.el index 90d6f13f2f6..f0a7c37ddf2 100644 --- a/test/lisp/erc/erc-networks-tests.el +++ b/test/lisp/erc/erc-networks-tests.el @@ -1199,7 +1199,7 @@ (erc-mode) (cl-letf (((symbol-function 'erc--route-insertion) - (lambda (&rest r) (push r calls)))) + (lambda (&rest r) (ignore (push r calls))))) (ert-info ("Signals when `erc-server-announced-name' unset") (should-error (erc-networks--set-name nil (make-erc-response))) diff --git a/test/lisp/erc/erc-sasl-tests.el b/test/lisp/erc/erc-sasl-tests.el index afe55f522dd..9c6def9cb38 100644 --- a/test/lisp/erc/erc-sasl-tests.el +++ b/test/lisp/erc/erc-sasl-tests.el @@ -319,16 +319,27 @@ IRX9cyi2wdYg9mUUYyh9GKdBCYHGUJAiCA== :tags '(:unstable) ;; This is currently useless because it just roundtrips shelling out ;; to pkeyutl. - (ert-skip "Placeholder") + (ert-skip "Placeholder for manual debugging") (unless (executable-find "openssl") (ert-skip "System lacks openssl")) + (ert-with-temp-file keyfile :prefix "ecdsa_key" :suffix ".pem" :text erc-sasl-tests-ecdsa-key-file - (let* ((erc-server-current-nick "jilles") - (erc-sasl--options `((password . ,keyfile))) - (client (erc-sasl--create-client 'ecdsa-nist256p-challenge)) + + (erc-mode) + (erc--initialize-markers (point) nil) + (setq erc-server-process (make-process :name "sleep" + :buffer (current-buffer) + :command '("sleep" "1") + :noquery t) + erc-session-username "jilles") + (let ((erc-sasl-mechanism 'ecdsa-nist256p-challenge) + (erc-sasl-password keyfile)) + (erc-sasl-mode +1)) + + (let* ((client (erc-sasl--state-client erc-sasl--state)) (step (sasl-next-step client nil))) (ert-info ("Client's initial request") (should (equal (format "%S" [erc-sasl--ecdsa-first "jilles"]) diff --git a/test/lisp/erc/erc-services-tests.el b/test/lisp/erc/erc-services-tests.el index 9bafba98dc6..126f6d7bbdd 100644 --- a/test/lisp/erc/erc-services-tests.el +++ b/test/lisp/erc/erc-services-tests.el @@ -70,7 +70,7 @@ (defun erc-services-tests--auth-source-standard (search) (setq search (erc-services-tests--wrap-search search)) - (ert-info ("Session wins") + (ert-info ("Session ID wins") (let ((erc-session-server "irc.gnu.org") (erc-server-announced-name "my.gnu.org") (erc-session-port 6697) @@ -92,9 +92,14 @@ (let ((erc-session-server "irc.gnu.org") (erc-server-announced-name "my.gnu.org") (erc-session-port 6697) - erc-network (erc-networks--id (erc-networks--id-create nil))) - (should (string= (funcall search :user "#chan") "baz"))))) + (should (string= (funcall search :user "#chan") "baz")))) + + (ert-info ("Dialed wins") + (let ((erc-session-server "irc.gnu.org") + (erc-session-port 6697) + (erc-networks--id (erc-networks--id-create nil))) + (should (string= (funcall search :user "#chan") "bar"))))) (defun erc-services-tests--auth-source-announced (search) (setq search (erc-services-tests--wrap-search search)) @@ -102,7 +107,8 @@ (erc-server-parameters '(("CHANTYPES" . "&#"))) (erc--target (erc--target-from-string "&chan"))) - (ert-info ("Announced prioritized") + ;; Pretend #chan is just some account name and not a channel. + (ert-info ("Host priorities reversed when target is local") (ert-info ("Announced wins") (let* ((erc-session-server "irc.gnu.org") @@ -113,7 +119,7 @@ (erc-networks--id (erc-networks--id-create nil))) (should (string= (funcall search :user "#chan") "baz")))) - (ert-info ("Peer next") + (ert-info ("Dialed next") (let* ((erc-server-announced-name "irc.gnu.org") (erc-session-port 6697) (erc-network 'GNU.chat) diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el index 999d9f100c9..6a46246725e 100644 --- a/test/lisp/erc/erc-tests.el +++ b/test/lisp/erc/erc-tests.el @@ -330,6 +330,7 @@ (cl-incf counter)))) erc-accidental-paste-threshold-seconds erc-insert-modify-hook + erc-send-modify-hook (erc-last-input-time 0) (erc-modules (remq 'stamp erc-modules)) (erc-send-input-line-function #'ignore) @@ -1268,6 +1269,7 @@ (should-not (erc--valid-local-channel-p "#chan")) (should (erc--valid-local-channel-p "&local"))))) +;; FIXME remove this because it serves no purpose. See bug#71178. (ert-deftest erc--restore-initialize-priors () (unless (>= emacs-major-version 28) (ert-skip "Lisp nesting exceeds `max-lisp-eval-depth'")) @@ -2533,7 +2535,7 @@ erc-kill-channel-hook erc-kill-server-hook erc-kill-buffer-hook) (cl-letf (((symbol-function 'erc-display-message) (lambda (_ _ _ msg &rest args) - (push (apply #'erc-format-message msg args) calls))) + (ignore (push (apply #'erc-format-message msg args) calls)))) ((symbol-function 'erc-server-send) (lambda (line _) (push line calls))) ((symbol-function 'erc-server-buffer) -- 2.39.2