bug#21440: 25.0.50; Manual: FEATURE-unload-hook in (info "(elisp) Coding Conventions")

bug#21440: 25.0.50; Manual: FEATURE-unload-hook in (info "(elisp) Coding Conventions")

[Michael Heerdegen]

Hi,

In (info "(elisp) Coding Conventions"):

   • If loading the file adds functions to hooks, define a function
     ‘FEATURE-unload-hook’, where FEATURE is the name of the feature the
     package provides, and make it undo any such changes.  Using
     ‘unload-feature’ to unload the file will run this function.  *Note
     Unloading.


A problematic tip:

 - it is unnecessary for that reason. AFAIK FEATURE's functions are
 removed automatically from hooks by `unload-feature'.

 - When you follow the advice, `unload-feature' will skip removing
 entries from `auto-mode-alist', so you would have to do that yourself.

 - Isn't `FEATURE-unload-function' the preferred thing to use today?
 The doc string of `unload-feature' doesn't even mention
 FEATURE-unload-hook variables.  Seems it is discouraged to be used.


BTW, I miss a comment what I should do if a feature added functions to
the buffer local binding of a hook.  Does `unload-feature' take care of
that (doesn't seem so)?  Nothing about this in (info "(elisp) Unloading").


Thanks,

Michael.



In GNU Emacs 25.0.50.19 (x86_64-unknown-linux-gnu, GTK+ Version 3.16.6)
 of 2015-09-07
Repository revision: af629df80605614c645ba6539bb98d4f1e990925
Windowing system distributor 'The X.Org Foundation', version 11.0.11702000
System Description:	Debian GNU/Linux testing (stretch)

bug#21440: 25.0.50; Manual: FEATURE-unload-hook in (info "(elisp) Coding Conventions")

[Stefan Kangas]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Hi,
>
> In (info "(elisp) Coding Conventions"):
>
>    • If loading the file adds functions to hooks, define a function
>      ‘FEATURE-unload-hook’, where FEATURE is the name of the feature the
>      package provides, and make it undo any such changes.  Using
>      ‘unload-feature’ to unload the file will run this function.  *Note
>      Unloading.
>
> A problematic tip:
>
>  - it is unnecessary for that reason. AFAIK FEATURE's functions are
>  removed automatically from hooks by `unload-feature'.
>
>  - When you follow the advice, `unload-feature' will skip removing
>  entries from `auto-mode-alist', so you would have to do that yourself.
>
>  - Isn't `FEATURE-unload-function' the preferred thing to use today?
>  The doc string of `unload-feature' doesn't even mention
>  FEATURE-unload-hook variables.  Seems it is discouraged to be used.

The above analysis seems correct to me.  There should be no need to use
`FEATURE-load-hook', in fact from reading `unload-feature' it seems
worse.  But I don't know too much about this, so I'm not sure if there's
some fundamental aspect I'm overlooking.

If the above is indeed true, we should probably just remove the advice
from coding conventions.  And it should be announced in NEWS, probably
together with some warnings emitted by `unload-feature' if there is such
a `FEATURE-unload-hook' defined.

Is there anyone else who could provide some insight here?

Best regards,
Stefan Kangas

bug#21440: 25.0.50; Manual: FEATURE-unload-hook in (info "(elisp) Coding Conventions")

[Michael Albinus]

Stefan Kangas <stefan@marxist.se> writes:

Hi,

>> In (info "(elisp) Coding Conventions"):
>>
>>    • If loading the file adds functions to hooks, define a function
>>      ‘FEATURE-unload-hook’, where FEATURE is the name of the feature the
>>      package provides, and make it undo any such changes.  Using
>>      ‘unload-feature’ to unload the file will run this function.  *Note
>>      Unloading.
>>
>> A problematic tip:
>>
>>  - it is unnecessary for that reason. AFAIK FEATURE's functions are
>>  removed automatically from hooks by `unload-feature'.
>>
>>  - When you follow the advice, `unload-feature' will skip removing
>>  entries from `auto-mode-alist', so you would have to do that yourself.
>>
>>  - Isn't `FEATURE-unload-function' the preferred thing to use today?
>>  The doc string of `unload-feature' doesn't even mention
>>  FEATURE-unload-hook variables.  Seems it is discouraged to be used.
>
> The above analysis seems correct to me.  There should be no need to use
> `FEATURE-load-hook', in fact from reading `unload-feature' it seems
> worse.  But I don't know too much about this, so I'm not sure if there's
> some fundamental aspect I'm overlooking.
>
> If the above is indeed true, we should probably just remove the advice
> from coding conventions.  And it should be announced in NEWS, probably
> together with some warnings emitted by `unload-feature' if there is such
> a `FEATURE-unload-hook' defined.
>
> Is there anyone else who could provide some insight here?

As a test, I've renamed `tramp-unload-hook' and all
`tramp-FEATURE-unload-hook' variables to `tramp-unload-function' and
`tramp-FEATURE-unload-function', respectively. The related test,
`tramp-test45-unload', fails now. I haven't debugged further, but
throwing a warning when `FEATURE-unload-hook' is used, looks premature
to me.

> Best regards,
> Stefan Kangas

Best regards, Michael.

bug#21440: 25.0.50; Manual: FEATURE-unload-hook in (info "(elisp) Coding Conventions")

[Lars Ingebrigtsen]

Michael Albinus <michael.albinus@gmx.de> writes:

>>>    • If loading the file adds functions to hooks, define a function
>>>      ‘FEATURE-unload-hook’, where FEATURE is the name of the feature the
>>>      package provides, and make it undo any such changes.  Using
>>>      ‘unload-feature’ to unload the file will run this function.  *Note
>>>      Unloading.

[...]

> As a test, I've renamed `tramp-unload-hook' and all
> `tramp-FEATURE-unload-hook' variables to `tramp-unload-function' and
> `tramp-FEATURE-unload-function', respectively. The related test,
> `tramp-test45-unload', fails now. I haven't debugged further, but
> throwing a warning when `FEATURE-unload-hook' is used, looks premature
> to me.

The quoted text no longer exists in Emacs 28 -- it now refers to
-unload-function.  Because:

commit 8da810f91b11a258a7ed0f5315292143072881d8
Author:     Eli Zaretskii <eliz@gnu.org>
AuthorDate: Mon Nov 7 19:39:54 2016 +0200
Commit:     Eli Zaretskii <eliz@gnu.org>
CommitDate: Mon Nov 7 19:39:54 2016 +0200

    Don't refer to obsolete FEATURE-unload-hook
    
    * doc/lispref/tips.texi (Coding Conventions): Refer to
    FEATURE-unload-function rather than its obsolete variant
    FEATURE-unload-hook.  (Bug#24890)

But I don't know whether -unload-function has the same problems that
-unload-hook had?  And I see that there's still a lot of things called
-unload-hook in Emacs...

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

bug#21440: 25.0.50; Manual: FEATURE-unload-hook in (info "(elisp) Coding Conventions")

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> But I don't know whether -unload-function has the same problems that
> -unload-hook had?

At least now (docstring of `unload-feature'):

| If a function `FEATURE-unload-function' is defined, this function
| calls it with no arguments, before doing anything else.  That function
| can do whatever is appropriate to undo the loading of the library.  If
| `FEATURE-unload-function' returns non-nil, that suppresses the
| standard unloading of the library.  Otherwise the standard unloading
| proceeds.

So it can now be controlled whether standard unloading stuff will still
be performed (last problem mentioned in my report -> solved).

This questionable paragraph is sill in the manual however:

   • If loading the file adds functions to hooks, define a function
     ‘FEATURE-unload-function’, where FEATURE is the name of the feature
     the package provides, and make it undo any such changes.  Using
     ‘unload-feature’ to unload the file will run this function.  *Note
     Unloading::.

I guess it is a bit outdated and this had been automated long ago,
although partially heuristically.  Instead it could say what typical
non-standard changes need to be handled in an unload-function.  It is
unrealistic that every author cares about implementing a non-heuristic
version of the automatic unloading code.

Or, say shortly what unloading actually already does of its own, and
that the library developer should care about the rest.


Thanks,

Michael.

bug#21440: 25.0.50; Manual: FEATURE-unload-hook in (info "(elisp) Coding Conventions")

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Mon, 12 Jul 2021 05:56:50 +0200
> Cc: Michael Albinus <michael.albinus@gmx.de>, 21440@debbugs.gnu.org,
>  Stefan Kangas <stefan@marxist.se>
> 
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
> > But I don't know whether -unload-function has the same problems that
> > -unload-hook had?
> 
> At least now (docstring of `unload-feature'):
> 
> | If a function `FEATURE-unload-function' is defined, this function
> | calls it with no arguments, before doing anything else.  That function
> | can do whatever is appropriate to undo the loading of the library.  If
> | `FEATURE-unload-function' returns non-nil, that suppresses the
> | standard unloading of the library.  Otherwise the standard unloading
> | proceeds.
> 
> So it can now be controlled whether standard unloading stuff will still
> be performed (last problem mentioned in my report -> solved).
> 
> This questionable paragraph is sill in the manual however:
> 
>    • If loading the file adds functions to hooks, define a function
>      ‘FEATURE-unload-function’, where FEATURE is the name of the feature
>      the package provides, and make it undo any such changes.  Using
>      ‘unload-feature’ to unload the file will run this function.  *Note
>      Unloading::.
> 
> I guess it is a bit outdated and this had been automated long ago,
> although partially heuristically.

I don't understand why you say this is outdated.  What did I miss?

> Instead it could say what typical non-standard changes need to be
> handled in an unload-function.

Isn't that what it says?

bug#21440: 25.0.50; Manual: FEATURE-unload-hook in (info "(elisp) Coding Conventions")

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> At least now (docstring of `unload-feature'):
>> 
>> | If a function `FEATURE-unload-function' is defined, this function
>> | calls it with no arguments, before doing anything else.  That function
>> | can do whatever is appropriate to undo the loading of the library.  If
>> | `FEATURE-unload-function' returns non-nil, that suppresses the
>> | standard unloading of the library.  Otherwise the standard unloading
>> | proceeds.
>> 
>> So it can now be controlled whether standard unloading stuff will still
>> be performed (last problem mentioned in my report -> solved).

Ah, right.

>> This questionable paragraph is sill in the manual however:
>> 
>>    • If loading the file adds functions to hooks, define a function
>>      ‘FEATURE-unload-function’, where FEATURE is the name of the feature
>>      the package provides, and make it undo any such changes.  Using
>>      ‘unload-feature’ to unload the file will run this function.  *Note
>>      Unloading::.
>> 
>> I guess it is a bit outdated and this had been automated long ago,
>> although partially heuristically.
>
> I don't understand why you say this is outdated.  What did I miss?
>
>> Instead it could say what typical non-standard changes need to be
>> handled in an unload-function.
>
> Isn't that what it says?

Well, it recommends writing an unload function, although this is rarely
necessary.  So it should say something about when it's necessary.

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

bug#21440: 25.0.50; Manual: FEATURE-unload-hook in (info "(elisp) Coding Conventions")

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> > I don't understand why you say this is outdated.  What did I miss?
> >
> >> Instead it could say what typical non-standard changes need to be
> >> handled in an unload-function.
> >
> > Isn't that what it says?
>
> Well, it recommends writing an unload function, although this is rarely
> necessary.  So it should say something about when it's necessary.

Yes, exactly.

The second paragraph of `unload-feature' has some good examples (should
we add advices to that list btw?).


Michael.

bug#21440: 25.0.50; Manual: FEATURE-unload-hook in (info "(elisp) Coding Conventions")

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

>> Well, it recommends writing an unload function, although this is rarely
>> necessary.  So it should say something about when it's necessary.
>
> Yes, exactly.

I've now clarified that section...

> The second paragraph of `unload-feature' has some good examples 

... but I don't think we need to go into details about what
unload-feature does in the Coding Conventions section in the manual.

> (should we add advices to that list btw?).

Hm...  does unload-feature remove advices?  Let's see..

Well, there's this:

(defun loadhist--unload-function (x)
  (let ((fun (cdr x)))
    (when (fboundp fun)
      (when (fboundp 'ad-unadvise)
	(ad-unadvise fun))

Hm...  but is that what it does?

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

bug#21440: 25.0.50; Manual: FEATURE-unload-hook in (info "(elisp) Coding Conventions")

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I've now clarified that section...

Thanks, Lars.

> > (should we add advices to that list btw?).
>
> Hm...  does unload-feature remove advices?  Let's see..
>
> Well, there's this:
>
> (defun loadhist--unload-function (x)
>   (let ((fun (cdr x)))
>     (when (fboundp fun)
>       (when (fboundp 'ad-unadvise)
> 	(ad-unadvise fun))
>
> Hm...  but is that what it does?

I don't think so.  AFAIU this does not even effect nadvice advices, and
treats only advises of functions defined by the feature.

Michael.

bug#21440: 25.0.50; Manual: FEATURE-unload-hook in (info "(elisp) Coding Conventions")

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> > Hm...  does unload-feature remove advices?  Let's see..
> >
> > Well, there's this:
> >
> > (defun loadhist--unload-function (x)
> > [...]
> >
> > Hm...  but is that what it does?
>
> I don't think so.  AFAIU this does not even effect nadvice advices,
> and treats only advises of functions defined by the feature.

Here is a simple test file:

#+begin_src emacs-lisp
;;; foo.el --- ...  -*- lexical-binding: t -*-

(defun my-foo--around-ad (f &rest args)
  17
  (apply f args))

(advice-add 'make-frame-command :around #'my-foo--around-ad)

(provide 'foo)
;;; foo.el ends here
#+end_src

Load it, then M-x unload-feature foo RET -- and after that, C-x 5 2
errors.


Michael.

bug#21440: 25.0.50; Manual: FEATURE-unload-hook in (info "(elisp) Coding Conventions")

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Load it, then M-x unload-feature foo RET -- and after that, C-x 5 2
> errors.

Right; it would indeed be good if that was fixed by `unload-feature'.
Could you open a new bug report for that (as it's a somewhat separate
issue from what this one is about)?

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

bug#21440: 25.0.50; Manual: FEATURE-unload-hook in (info "(elisp) Coding Conventions")

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Right; it would indeed be good if that was fixed by `unload-feature'.
> Could you open a new bug report for that (as it's a somewhat separate
> issue from what this one is about)?

Done - see Bug#49674.

Michael.