bug#5293: 23.1; unload-feature on buffer-local hooks

["Štěpán Němec" <stepnem@gmail.com>]

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

On Sat, 06 Aug 2011 11:20:41 +1000
Kevin Ryde wrote:

> reopen 5293
> thanks
>
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>
>>> No, just the hooks.  If it makes sense to remove unloaded funcs from
>>> hook global values then surely the same rationale applies to remove them
>>> from the buffer-local values too.
>>
>> Agreed.  Someone would have to try it out to see if it can be
>> done efficiently.
        ^^^^^^^^^^^ :-D

> Reopened for that, or failing that then for clarifying the docstring.
>
> I imagine there's not normally many hooks or many buffers, or many
> buffer-local variables, whichever one of those was the efficient way to
> scan ... and unload-feature isn't used very much anyway.

Coming back to this after 10 years, it appears that we (I, certainly)
underestimated the computation involved. While the attached patch(es)
work for emacs -Q toy examples like Kevin's or more recently the one
from bug#34686, when I tried M-x load-library allout RET M-x
unload-feature allout RET in my normal Emacs session with ~300 buffers
and ~1000 features, it took my venerable laptop 8 minutes to complete.

Based on that experience, unless someone has better ideas, I suggest we
close this and clarify the (henceforth intentional) lack of attention to
buffer-local hook values in the documentation instead.

Actually, I wonder if ignoring even the global hooks (as opined by
Juanma) and enforcing more widespread usage of FEATURE-unload-function
wouldn't be better; or/also, couldn't stray undefined functions on hooks
be handled similarly to how it's done for errors e.g. in
`post-command-hook', i.e. auto-removed when encountered? I guess
wrapping all hooks like that would be overkill?

(That said, I think [1/3] and [3/3] could/should be applied
nonetheless.)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-unload-feature-Improve-logic-don-t-repeat-computatio.patch --]
[-- Type: text/x-patch, Size: 2412 bytes --]

From 394f2bf6821196a4171fd79fc9a10dc626619a58 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0t=C4=9Bp=C3=A1n=20N=C4=9Bmec?= <stepnem@gmail.com>
Date: Mon, 6 Apr 2020 13:25:41 +0200
Subject: [PATCH 1/3] unload-feature: Improve logic (don't repeat computation)

* lisp/loadhist.el (unload-feature): Don't do the same computation twice.
---
 lisp/loadhist.el | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/lisp/loadhist.el b/lisp/loadhist.el
index a1ff2f6270..60da00cceb 100644
--- a/lisp/loadhist.el
+++ b/lisp/loadhist.el
@@ -287,22 +287,23 @@ unload-feature
 	;; functions which the package might just have installed, and
 	;; there might be other important state, but this tactic
 	;; normally works.
-	(mapatoms
-	 (lambda (x)
-	   (when (and (boundp x)
-		      (or (and (consp (symbol-value x)) ; Random hooks.
-			       (string-match "-hooks?\\'" (symbol-name x)))
-			  (memq x unload-feature-special-hooks)))	; Known abnormal hooks etc.
-	     (dolist (y unload-function-defs-list)
-	       (when (and (eq (car-safe y) 'defun)
-			  (not (get (cdr y) 'autoload)))
-		 (remove-hook x (cdr y)))))))
-	;; Remove any feature-symbols from auto-mode-alist as well.
-	(dolist (y unload-function-defs-list)
-	  (when (and (eq (car-safe y) 'defun)
-		     (not (get (cdr y) 'autoload)))
-	    (setq auto-mode-alist
-		  (rassq-delete-all (cdr y) auto-mode-alist)))))
+        (let ((removables (cl-loop for def in unload-function-defs-list
+                                   when (and (eq (car-safe def) 'defun)
+                                             (not (get (cdr def) 'autoload)))
+                                   collect (cdr def))))
+          (mapatoms
+	   (lambda (x)
+	     (when (and (boundp x)
+		        (or (and (consp (symbol-value x)) ; Random hooks.
+			         (string-match "-hooks?\\'" (symbol-name x)))
+                            ;; Known abnormal hooks etc.
+			    (memq x unload-feature-special-hooks)))
+	       (dolist (func removables)
+	         (remove-hook x func)))))
+          ;; Remove any feature-symbols from auto-mode-alist as well.
+          (dolist (func removables)
+            (setq auto-mode-alist
+                  (rassq-delete-all func auto-mode-alist)))))
 
       ;; Change major mode in all buffers using one defined in the feature being unloaded.
       (unload--set-major-mode)
-- 
2.26.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-unload-feature-Handle-local-hooks.patch --]
[-- Type: text/x-patch, Size: 1749 bytes --]

From d2372157c031b79cad4e3bf331aa6e167ff48199 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0t=C4=9Bp=C3=A1n=20N=C4=9Bmec?= <stepnem@gmail.com>
Date: Mon, 6 Apr 2020 13:30:11 +0200
Subject: [PATCH 2/3] unload-feature: Handle local hooks

Buffer-local hooks were introduced in

1994-09-30T20:47:13+00:00!rms@gnu.org
0e4d378b32 (add-hook): Initialize default value and local value.

but `unload-feature' has not been updated to handle them.

* lisp/loadhist.el (unload-feature): Handle local hooks.
---
 etc/NEWS         | 3 +++
 lisp/loadhist.el | 6 +++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index 765a923bf7..fa90edc4d1 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -297,6 +297,9 @@ optional argument specifying whether to follow symbolic links.
 ** 'parse-time-string' can now parse ISO 8601 format strings,
 such as "2020-01-15T16:12:21-08:00".
 
+---
+** 'unload-feature' now also tries to undo additions to buffer-local hooks.
+
 \f
 * Changes in Emacs 28.1 on Non-Free Operating Systems
 
diff --git a/lisp/loadhist.el b/lisp/loadhist.el
index 60da00cceb..2fd2d3f6be 100644
--- a/lisp/loadhist.el
+++ b/lisp/loadhist.el
@@ -299,7 +299,11 @@ unload-feature
                             ;; Known abnormal hooks etc.
 			    (memq x unload-feature-special-hooks)))
 	       (dolist (func removables)
-	         (remove-hook x func)))))
+	         (remove-hook x func)
+                 (save-current-buffer
+                   (dolist (buffer (buffer-list))
+                     (set-buffer buffer)
+                     (remove-hook x func t)))))))
           ;; Remove any feature-symbols from auto-mode-alist as well.
           (dolist (func removables)
             (setq auto-mode-alist
-- 
2.26.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-unload-feature-Correct-doc-string-to-match-info-manu.patch --]
[-- Type: text/x-patch, Size: 2700 bytes --]

From a418282b70e514065159a217a4106226395aa98e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0t=C4=9Bp=C3=A1n=20N=C4=9Bmec?= <stepnem@gmail.com>
Date: Mon, 6 Apr 2020 17:05:33 +0200
Subject: [PATCH 3/3] unload-feature: Correct doc string to match info manual
 and reality

'unload-feature' doesn't try to "undo any additions the library has
made" to hooks, it tries to remove functions defined by the library
from hooks, no matter how they got there.

* lisp/loadhist.el (unload-feature): Correct the doc string.
* doc/lispref/loading.texi (Unloading): Clarify, fix typo.
---
 doc/lispref/loading.texi | 4 ++--
 lisp/loadhist.el         | 9 ++++-----
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/doc/lispref/loading.texi b/doc/lispref/loading.texi
index 2894282079..f1ee638357 100644
--- a/doc/lispref/loading.texi
+++ b/doc/lispref/loading.texi
@@ -1063,7 +1063,7 @@ Unloading
 (Loading saves these in the @code{autoload} property of the symbol.)
 
 Before restoring the previous definitions, @code{unload-feature} runs
-@code{remove-hook} to remove functions in the library from certain
+@code{remove-hook} to remove functions defined by the library from certain
 hooks.  These hooks include variables whose names end in @samp{-hook}
 (or the deprecated suffix @samp{-hooks}), plus those listed in
 @code{unload-feature-special-hooks}, as well as
@@ -1071,7 +1071,7 @@ Unloading
 function because important hooks refer to functions that are no longer
 defined.
 
-Standard unloading activities also undoes ELP profiling of functions
+Standard unloading activities also undo ELP profiling of functions
 in that library, unprovides any features provided by the library, and
 cancels timers held in variables defined by the library.
 
diff --git a/lisp/loadhist.el b/lisp/loadhist.el
index 2fd2d3f6be..9718a4840d 100644
--- a/lisp/loadhist.el
+++ b/lisp/loadhist.el
@@ -234,11 +234,10 @@ unload-feature
 is nil, raise an error.
 
 Standard unloading activities include restoring old autoloads for
-functions defined by the library, undoing any additions that the
-library has made to hook variables or to `auto-mode-alist', undoing
-ELP profiling of functions in that library, unproviding any features
-provided by the library, and canceling timers held in variables
-defined by the library.
+functions defined by the library, removing such functions from
+hooks and `auto-mode-alist', undoing their ELP profiling,
+unproviding any features provided by the library, and canceling
+timers held in variables defined by the library.
 
 If a function `FEATURE-unload-function' is defined, this function
 calls it with no arguments, before doing anything else.  That function
-- 
2.26.0