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

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

[Kevin Ryde]

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

When `unload-feature' looks in hooks for functions that it's going to
unload, it doesn't seem to look in buffer-local values, other than for
the current buffer.

Evalling the code in try-foo.el below loads then unloads foo.el.  It
gets an error

    void-function foo-message

where I hoped unload-feature might have purged that `foo-message' from
`after-change-functions'.

I suppose looking in all buffers is more work for unload-feature, but
would be a good protection against bad things happening later.  I expect
some of the standard hooks like `after-change-functions' are used
buffer-local most of the time.

If instead it's an intentional omission (to save work) then the elisp
manual and the docstring could note it so that modes or packages using
buffer-local hook settings can take steps to undo.



[-- Attachment #2: foo.el --]
[-- Type: application/emacs-lisp, Size: 225 bytes --]

[-- Attachment #3: try-foo.el --]
[-- Type: application/emacs-lisp, Size: 197 bytes --]

[-- Attachment #4: Type: text/plain, Size: 1077 bytes --]




In GNU Emacs 23.1.1 (i486-pc-linux-gnu, GTK+ Version 2.16.5)
 of 2009-09-14 on raven, modified by Debian
configured using `configure  '--build=i486-linux-gnu' '--host=i486-linux-gnu' '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib' '--localstatedir=/var/lib' '--infodir=/usr/share/info' '--mandir=/usr/share/man' '--with-pop=yes' '--enable-locallisppath=/etc/emacs23:/etc/emacs:/usr/local/share/emacs/23.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/23.1/site-lisp:/usr/share/emacs/site-lisp:/usr/share/emacs/23.1/leim' '--with-x=yes' '--with-x-toolkit=gtk' '--with-toolkit-scroll-bars' 'build_alias=i486-linux-gnu' 'host_alias=i486-linux-gnu' 'CFLAGS=-DDEBIAN -g -O2' 'LDFLAGS=-g' 'CPPFLAGS=''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_AU
  value of $XMODIFIERS: nil
  locale-coding-system: iso-latin-1-unix
  default-enable-multibyte-characters: t

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

[Juanma Barranquero]

On Sat, Jan 2, 2010 at 22:06, Kevin Ryde <user42@zip.com.au> wrote:

> When `unload-feature' looks in hooks for functions that it's going to
> unload, it doesn't seem to look in buffer-local values, other than for
> the current buffer.
[...]
> I suppose looking in all buffers is more work for unload-feature, but
> would be a good protection against bad things happening later.

Yes, unload-feature should look at buffer-local values, too.

For the moment being, a package like foo should define a function (foo
in the function name matches the feature name you're unloading):

(defun foo-unload-function ()
  ;; do whatever is necessary; in foo's case:
  (with-current-buffer (get-buffer "foo-bufer")
    (remove-hook 'after-change-functions 'foo-message t))
  ;; continue standard unloading
  nil)

that will be automatically called upon unloading foo.el; in most
cases, it should return nil to allow unload-feature to continue the
normal unloading process.

    Juanma

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

[Juanma Barranquero]

> Evalling the code in try-foo.el below loads then unloads foo.el.  It
> gets an error
>
>    void-function foo-message
>
> where I hoped unload-feature might have purged that `foo-message' from
> `after-change-functions'.
>
> I suppose looking in all buffers is more work for unload-feature, but
> would be a good protection against bad things happening later.  I expect
> some of the standard hooks like `after-change-functions' are used
> buffer-local most of the time.

You're right that some hooks are used buffer-locally most of the time,
but it's also true that in many cases they are used from a major mode,
i.e., in your case, if foo.el defined a foo-mode and set
`after-change-functions' locally in foo-mode buffers, unload-feature
would do OK (with the current trunk, not any released Emacsen).

As it is, foo.el is doing something non-standard, and unload-feature
cannot try to revert by itself every non-standard thing packages do.
That's why FEATURE-unload-function exists. So, in this case, the right
fix would be adding this function to foo.el:

(defun foo-unload-function ()
  "Unload foo.el."
  (ignore-errors
    (with-current-buffer (get-buffer "foo-buffer")
      (remove-hook 'after-change-functions 'foo-message t)))
  ;; continue standard unloading
  nil)

That said, there are improvements that could be made to unload-feature
(for example, trying to automatically deactivate minor modes being
undefined), but I'm not sure that looking in every buffer-local
variable of every live buffer is a sensible thing to do. I'm closing
this one because it is not really a bug. We can open a wishlist bug
for unload-feature is you want.

    Juanma

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

[Kevin Ryde]

Juanma Barranquero <lekktu@gmail.com> writes:
>
> but I'm not sure that looking in every buffer-local
> variable of every live buffer

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.

Or conversely, it's undesirable to leave behind an unbound func in a
hook, and the same undesirability as to a buffer-local value as a global
value.

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

[Juanma Barranquero]

On Fri, Jul 15, 2011 at 02:26, Kevin Ryde <user42@zip.com.au> wrote:

> Or conversely, it's undesirable to leave behind an unbound func in a
> hook, and the same undesirability as to a buffer-local value as a global
> value.

But the usual case is that these buffer-local values are set via major
modes also defined in the same package, and so they are automatically
removed when the major modes are disabled (i.e., when the buffers are
switched to other major modes). The only case where a buffer-local
value is left behind is when the package's code sets it in
non-standard ways, and in this case, it's the package responsability
to define a FEATURE-unload-function to undo the changes.

The philosophy behind unload-feature is: we try to automatically undo
the easy/standard things, and give the package the opportunity to undo
the hard/unstandard things itself. And I think it's the right
approach.

    Juanma

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

[Štěpán Němec]

Juanma Barranquero <lekktu@gmail.com> writes:

> On Fri, Jul 15, 2011 at 02:26, Kevin Ryde <user42@zip.com.au> wrote:
>
>> Or conversely, it's undesirable to leave behind an unbound func in a
>> hook, and the same undesirability as to a buffer-local value as a global
>> value.
>
> But the usual case is that these buffer-local values are set via major
> modes also defined in the same package, and so they are automatically
> removed when the major modes are disabled (i.e., when the buffers are
> switched to other major modes). The only case where a buffer-local
> value is left behind is when the package's code sets it in
> non-standard ways, and in this case, it's the package responsability
> to define a FEATURE-unload-function to undo the changes.
>
> The philosophy behind unload-feature is: we try to automatically undo
> the easy/standard things, and give the package the opportunity to undo
> the hard/unstandard things itself. And I think it's the right
> approach.

1) If your reasoning about hooks being added via modes were correct, you
wouldn't have to remove even the global hook additions. If it's faulty
(which is probably the case), both global and local hooks need to be
managed, as Kevin said.

2) The `unload-feature' docstring says it undoes "any additions that the
library has made to hook variables", but that's apparently not what's
really happening, so if things stay as they are, the doc string should
be corrected.

3) Are local hook additions really such a "hard/unstandard" thing to
undo?

  Štěpán

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

[Juanma Barranquero]

On Fri, Jul 15, 2011 at 10:52, Štěpán Němec <stepnem@gmail.com> wrote:

> 1) If your reasoning about hooks being added via modes were correct, you
> wouldn't have to remove even the global hook additions.

Why? Global additions are not removed when the buffer's major mode is
switched. Local variables, including local values of hooks, are.

> If it's faulty
> (which is probably the case), both global and local hooks need to be
> managed, as Kevin said.

I'm more of the opinion that both should be un-managed. A look at the
sources is enough to see that hook removal is currently ugly and
ad-hoc, and ugly too.

> 2) The `unload-feature' docstring says it undoes "any additions that the
> library has made to hook variables", but that's apparently not what's
> really happening, so if things stay as they are, the doc string should
> be corrected.

Yes. The docstring for unload-feature has always promised more than it
could reasonably accomplish. Yours is only one example.

> 3) Are local hook additions really such a "hard/unstandard" thing to
> undo?

Local hook additions aren't. And they are correctly treated right now.
What we are discussing is local hooks in buffers whose major mode
wasn't also defined in the same feature being unloaded. For example,
things like

;;; my-mode.el

(define-derived-mode my-mode ...)

(defun my-mode-this ()  ...)

(defun my-mode-that ()  ...)

(defun my-mode-hook ()  ...)

(with-current-buffer (get-buffer "some poor unsuspecting buffer"))
   ;;; do not set the buffer major mode to my-mode, but
   (add-hook 'some-hook 'my-mode-hook nil t))

(provide 'my-mode)

;;;; end of my-mode.el


and that kind of thing is unfrequent enough that the better fix is

(defun my-mode-unload-function ()
   (ignore-errors
      (with-current-buffer (get-buffer "some poor unsuspecting buffer")
         (remove-hook 'some-hook 'my-mode-hook t)))
   nil)

Both Kevin and you seem to think that unload-feature should do
everything and that having to define FEATURE-unload-function is a bug
or something like that. It is not.  I'm all for making unload-feature
smarter, as long as it does not trample on the programmer's ability to
do. I can perfectly well imagine unloading a package but setting a
hook with an autoloading function from that same package.

    Juanma

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

[Štěpán Němec]

Juanma Barranquero <lekktu@gmail.com> writes:

> On Fri, Jul 15, 2011 at 10:52, Štěpán Němec <stepnem@gmail.com> wrote:
>
>> 1) If your reasoning about hooks being added via modes were correct, you
>> wouldn't have to remove even the global hook additions.
>
> Why? Global additions are not removed when the buffer's major mode is
> switched. Local variables, including local values of hooks, are.

Note I omitted the "major" part, i.e., it's not uncommon for minor modes
to make global hook additions. Sorry if that's not really related to the
problem at hand.

>> If it's faulty
>> (which is probably the case), both global and local hooks need to be
>> managed, as Kevin said.
>
> I'm more of the opinion that both should be un-managed. A look at the
> sources is enough to see that hook removal is currently ugly and
> ad-hoc, and ugly too.

Fine with me, as long as the documentation reflects this.

>> 2) The `unload-feature' docstring says it undoes "any additions that the
>> library has made to hook variables", but that's apparently not what's
>> really happening, so if things stay as they are, the doc string should
>> be corrected.
>
> Yes. The docstring for unload-feature has always promised more than it
> could reasonably accomplish. Yours is only one example.

Please do update it then.

[...]

> Both Kevin and you seem to think that unload-feature should do
> everything and that having to define FEATURE-unload-function is a bug
> or something like that. It is not.  I'm all for making unload-feature
> smarter, as long as it does not trample on the programmer's ability to
> do. I can perfectly well imagine unloading a package but setting a
> hook with an autoloading function from that same package.

Right... I do know about unload functions and use them where
appropriate. The important thing is that the documentation needs to
describe what actually happens, so whatever you decide to do about this,
please update the documentation (which, as you confirmed, needs to be
done anyway).

  Štěpán

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

[Juanma Barranquero]

On Fri, Jul 15, 2011 at 18:08, Štěpán Němec <stepnem@gmail.com> wrote:

> Note I omitted the "major" part, i.e., it's not uncommon for minor modes
> to make global hook additions. Sorry if that's not really related to the
> problem at hand.

Currently, minor modes are not automatically turned off; packages that
define minor modes *need* a FEATURE-unload-function. See allout.el,
hi-lock.el, hl-line.el, linum.el, etc. Turning off the minor mode
should remove these hooks.

> The important thing is that the documentation needs to
> describe what actually happens, so whatever you decide to do about this,
> please update the documentation (which, as you confirmed, needs to be
> done anyway).

I agree that the documentation should better reflect what
unload-feature actually does, but I won't be the one writing it. I
suck at that.

    Juanma

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

[Stefan Monnier]

> 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.


        Stefan

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

[Kevin Ryde]

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.

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.

(Actually the way unload-feature seems not much used and not getting
much attention from packages makes me wonder if it's worth bothering.
But if unload-feature did a little more then it would increase its
usefulness, perhaps to the point where it would be used more :-)

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

[Štěpán Němec]

[-- 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

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

[Stefan Monnier]

> @@ -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

Maybe instead of `(dolist (buffer (buffer-list))` within that big
`mapatoms` within `(dolist (func removables)` (which is O(B*F*V) where
B is the number of buffers, F is the number of functions and V is the
number of hook vars), we should instead do it as:

    (dolist (buffer (buffer-list))
      (dolist (varvar (buffer-local-variables buffer))
        (when <var is a hook>
          (dolist (func removables)
            (remove-hook <var> func t)))))

If we need it to go faster maybe we could also arrange for (add-hook
V F ..)  to do (cl-pushnew V (get F 'hooks-where-it-has-been-put)).
So we could do

    (let ((relevant-hooks
           (mapcan (lambda (f) (get F 'hooks-where-it-has-been-put)) funcs)))
      (dolist (buffer (buffer-list))
        (dolist (varvar (buffer-local-variables buffer))
          (when (memq var relevant-hooks)
            (dolist (func removables)
              (remove-hook <var> func t)))))


-- Stefan

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

[Štěpán Němec]

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

On Mon, 06 Apr 2020 14:06:02 -0400
Stefan Monnier wrote:

>> @@ -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
>
> Maybe instead of `(dolist (buffer (buffer-list))` within that big
> `mapatoms` within `(dolist (func removables)` (which is O(B*F*V) where
> B is the number of buffers, F is the number of functions and V is the
> number of hook vars), we should instead do it as:
>
>     (dolist (buffer (buffer-list))
>       (dolist (varvar (buffer-local-variables buffer))
                         ^^^^^^^^^^^^^^^^^^^^^^

Ha! Didn't know/think about that one, and indeed it makes a world of
difference. Thanks!

So I guess my defeatism will not prevail after all. Updated [2/3] attached.

-- 
Štěpán


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

From fd9b67b10fcc9596fc1eeae50bb351ef235eea68 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 v2] 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 | 9 +++++++++
 2 files changed, 12 insertions(+)

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..81576679c3 100644
--- a/lisp/loadhist.el
+++ b/lisp/loadhist.el
@@ -300,6 +300,15 @@ unload-feature
 			    (memq x unload-feature-special-hooks)))
 	       (dolist (func removables)
 	         (remove-hook x func)))))
+          (save-current-buffer
+            (dolist (buffer (buffer-list))
+              (pcase-dolist (`(,sym . ,val) (buffer-local-variables buffer))
+                (when (or (and (consp val)
+                               (string-match "-hooks?\\'" (symbol-name sym)))
+                          (memq sym unload-feature-special-hooks))
+                  (set-buffer buffer)
+                  (dolist (func removables)
+                    (remove-hook sym func t))))))
           ;; Remove any feature-symbols from auto-mode-alist as well.
           (dolist (func removables)
             (setq auto-mode-alist
-- 
2.26.0

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

[Juanma Barranquero]

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

On Mon, Apr 6, 2020 at 7:24 PM Štěpán Němec <stepnem@gmail.com> wrote:

> 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;

Anything automatically done in the unload-hook is just an ad hoc fix for
things the
module author knows how to do better than us. FEATURE-unload-function has
already
been there for a few years. I don't remember right now whether we suggest
in the
mode-creation documentation to use it, but certainly that's something
module authors
should do, and the automatic unloading is just a last-resort feature for
those old
modules that don't. There's no point IMHO to make the hands off approach
work better.

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

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

[Štěpán Němec]

On Mon, 06 Apr 2020 22:39:50 +0200
Juanma Barranquero wrote:

> On Mon, Apr 6, 2020 at 7:24 PM Štěpán Němec <stepnem@gmail.com> wrote:
>
>> 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;
>
> Anything automatically done in the unload-hook is just an ad hoc fix
> for things the module author knows how to do better than us.

One common scenario where this doesn't quite hold IIUC is minor modes
which users are supposed to put on various hooks themselves: the library
author has no way of dealing with that, short of doing something like
`unload-feature' does, although checking for just a few known symbols
from an unload function instead of the brute-force approach of the
latter would arguably be more effective.

> FEATURE-unload-function has already been there for a few years. I
> don't remember right now whether we suggest in the mode-creation
> documentation to use it,

We do (lispref/tips.texi). Unfortunately, most elisp libraries in the
wild seem to be written by people who either haven't read it, or have
remained resistent to most of its edificatory influence.

> but certainly that's something module authors should do, and the
> automatic unloading is just a last-resort feature for those old
> modules that don't.

Actually, IME the older, the better behaved. I can't remember last time
I saw a newish package with an unload function (while I do remember
those without one which left my Emacs broken when I tried unloading
them).

> There's no point IMHO to make the hands off approach work better.

I don't know what you mean by "hands off" here, but in any case, while I
used to argue for handling as much as possible in `unload-feature',
these days I don't feel strongly about it. So even though this
particular issue (local hooks) does seem solvable (thanks again to
Stefan!) without making anything much worse or uglier than it already
is, I remain of two minds on whether it is the best thing to do or not.

-- 
Štěpán

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

[Juanma Barranquero]

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

On Mon, Apr 6, 2020 at 11:27 PM Štěpán Němec <stepnem@gmail.com> wrote:

> One common scenario where this doesn't quite hold IIUC is minor modes
> which users are supposed to put on various hooks themselves: the library
> author has no way of dealing with that, short of doing something like
> `unload-feature' does, although checking for just a few known symbols
> from an unload function instead of the brute-force approach of the
> latter would arguably be more effective.

Some minor modes are designed to be put in a few hooks, so these can be
checked by the package itself. But you're right that there are occasions
where
the automatic mechanisms are necessary.

> We do (lispref/tips.texi). Unfortunately, most elisp libraries in the
> wild seem to be written by people who either haven't read it, or have
> remained resistent to most of its edificatory influence.

In these cases, I would consider any problem unloading the library as a
reportable bug. One good thing of FEATURE-unload-function is that it is
totally backwards-compatible. Any library can define such a function and
does not need to worry about being run in older Emacsen.

> Actually, IME the older, the better behaved. I can't remember last time
> I saw a newish package with an unload function (while I do remember
> those without one which left my Emacs broken when I tried unloading
> them).

Well, I think what happens is that many package developers just don't
think at all about the package being unloaded. Truth be told, I'm not sure
unload-feature is used that often. I spent some effort in making it work
with the libraries included in the Emacs distribution, but I think I've used
unload-feature myself perhaps a couple of times, other than when testing it.

> I don't know what you mean by "hands off" here,

I meant the case where the package maintainer isn't willing to make the
effort to add FEATURE-unload-function.

> but in any case, while I
> used to argue for handling as much as possible in `unload-feature',
> these days I don't feel strongly about it. So even though this
> particular issue (local hooks) does seem solvable (thanks again to
> Stefan!) without making anything much worse or uglier than it already
> is, I remain of two minds on whether it is the best thing to do or not.

Well, I'm not against making unload-feature work better, if at all possible.
It's just that I see as an imperfect solution because the knowledge to
unload a package is, mostly, in the package author's hands. It's
somewhat of a waste to have to second-guess them.

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

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

[Lars Ingebrigtsen]

Štěpán Němec <stepnem@gmail.com> writes:

> So I guess my defeatism will not prevail after all. Updated [2/3] attached.

The patch no longer applied to the trunk, and it referred to a variable
called `removables' that I couldn't find.  So I've respun it (included
below), but not tested.

Any comments?

diff --git a/etc/NEWS b/etc/NEWS
index b4f29ab783..993cb3ca85 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -215,6 +215,9 @@ preserving markers, properties and overlays.  The new variable
 number of seconds that 'revert-buffer-with-fine-grain' should spend
 trying to be non-destructive.
 
+---
+** 'unload-feature' now also tries to undo additions to buffer-local hooks.
+
 \f
 * Changes in Specialized Modes and Packages in Emacs 28.1
 
diff --git a/lisp/loadhist.el b/lisp/loadhist.el
index a1ff2f6270..6f2e313f71 100644
--- a/lisp/loadhist.el
+++ b/lisp/loadhist.el
@@ -297,6 +297,16 @@ unload-feature
 	       (when (and (eq (car-safe y) 'defun)
 			  (not (get (cdr y) 'autoload)))
 		 (remove-hook x (cdr y)))))))
+        ;; Handle buffer-local hooks.
+        (save-current-buffer
+          (dolist (buffer (buffer-list))
+            (pcase-dolist (`(,sym . ,val) (buffer-local-variables buffer))
+              (when (or (and (consp val)
+                             (string-match "-hooks?\\'" (symbol-name sym)))
+                        (memq sym unload-feature-special-hooks))
+                (set-buffer buffer)
+                (dolist (func unload-function-defs-list)
+                  (remove-hook sym func t))))))
 	;; Remove any feature-symbols from auto-mode-alist as well.
 	(dolist (y unload-function-defs-list)
 	  (when (and (eq (car-safe y) 'defun)


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Štěpán Němec]

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

On Wed, 30 Sep 2020 20:44:04 +0200
Lars Ingebrigtsen wrote:

> The patch no longer applied to the trunk,

That's just the usual etc/NEWS conflict, I think.

> and it referred to a variable called `removables' that I couldn't
> find.

It's a 3-patch series; the variable is introduced by the previous patch.

For convenience, I've attached the whole series rebased on top of
current master.

Given the lack of further feedback, I haven't pushed (for pushing) this,
as I kinda liked Stefan's other suggestion, too ("If we need it to go
faster maybe we could also arrange for (add-hook V F ..) to do
(cl-pushnew V (get F 'hooks-where-it-has-been-put)).", from

https://lists.gnu.org/archive/html/bug-gnu-emacs/2020-04/msg00163.html
jwvsghgcxi5.fsf-monnier+emacs@gnu.org
)

But this version seems fast enough, so maybe we can push this and be
done with it, at least for the time being.

-- 
Štěpán


[-- 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 0d41a85c1881cd24a384e89227b3079bb62d5e30 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.28.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: 1963 bytes --]

From c50b6dec73739e5c3e1f18774b98adae213cf76c 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 | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index c571fa95d1..3739b1c67e 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1743,6 +1743,9 @@ to lexical binding, where dynamic (special) variables bound in one
 file can affect code in another.  For details, see the manual section
 '(Elisp) Converting to Lexical Binding'.
 
+---
+** '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..81576679c3 100644
--- a/lisp/loadhist.el
+++ b/lisp/loadhist.el
@@ -300,6 +300,15 @@ unload-feature
 			    (memq x unload-feature-special-hooks)))
 	       (dolist (func removables)
 	         (remove-hook x func)))))
+          (save-current-buffer
+            (dolist (buffer (buffer-list))
+              (pcase-dolist (`(,sym . ,val) (buffer-local-variables buffer))
+                (when (or (and (consp val)
+                               (string-match "-hooks?\\'" (symbol-name sym)))
+                          (memq sym unload-feature-special-hooks))
+                  (set-buffer buffer)
+                  (dolist (func removables)
+                    (remove-hook sym func t))))))
           ;; Remove any feature-symbols from auto-mode-alist as well.
           (dolist (func removables)
             (setq auto-mode-alist
-- 
2.28.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 eb9a34d1e1e5527686a41561db5fd2797edb5074 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 aa6ef307b1..e5364152d5 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 81576679c3..8ac575e8e3 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.28.0

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

[Lars Ingebrigtsen]

Štěpán Němec <stepnem@gmail.com> writes:

> But this version seems fast enough, so maybe we can push this and be
> done with it, at least for the time being.

Yeah, I think so -- your patch looks like a good idea, and we can always
tweak this more later, so please go ahead and push the patch series.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Štěpán Němec]

On Tue, 20 Oct 2020 13:13:00 +0200
Lars Ingebrigtsen wrote:

> Štěpán Němec <stepnem@gmail.com> writes:
>
>> But this version seems fast enough, so maybe we can push this and be
>> done with it, at least for the time being.
>
> Yeah, I think so -- your patch looks like a good idea, and we can always
> tweak this more later, so please go ahead and push the patch series.

Done, thank you.

0e9e36747f (unload-feature: Improve logic (don't repeat computation))
2020-04-06T13:25:41+02:00!stepnem@gmail.com

5c266a71c1 (unload-feature: Handle local hooks (bug#5293))
2020-04-06T13:30:11+02:00!stepnem@gmail.com

8dc8ab6b42 (unload-feature: Correct doc string to match info manual and reality)
2020-04-06T17:05:33+02:00!stepnem@gmail.com

-- 
Štěpán