bug#5293: 23.1; unload-feature on buffer-local hooks
Kevin Ryde
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.
Juanma Barranquero
> 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
Juanma Barranquero
> gets an error
>
> void-function foo-message
>
> where I hoped unload-feature might have purged that `foo-message' from
> `after-change-functions'.
>
> 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
Kevin Ryde
>
> 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.
Juanma Barranquero
> 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
Štěpán Němec
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
Juanma Barranquero
> 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
Štěpán Němec
> On Fri, Jul 15, 2011 at 10:52, Štěpán Němec <ste...@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
Juanma Barranquero
> 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
Stefan Monnier
> 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
Kevin Ryde
thanks
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 :-)
Štěpán Němec
Kevin Ryde wrote:
> reopen 5293
> thanks
>
> Stefan Monnier <mon...@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.
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.)
Stefan Monnier
> ;; 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
Štěpán Němec
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
Juanma Barranquero
> 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;
Štěpán Němec
Juanma Barranquero wrote:
> On Mon, Apr 6, 2020 at 7:24 PM Štěpán Němec <ste...@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.
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,
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.
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.
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
Juanma Barranquero
On Mon, Apr 6, 2020 at 11:27 PM Štěpán Němec <ste...@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.
> wild seem to be written by people who either haven't read it, or have
> remained resistent to most of its edificatory influence.
> 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).
> I don't know what you mean by "hands off" here,
> 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.