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