add-hook and defvar

add-hook and defvar

[Artur Malabarba]

While it's nice that `add-hook' tries to do the right thing even when
the variable isn't defined yet, that's a bit of a false assurance.
Although it's documented in the docstring, it seems undesirable that
it sets the variable. This means that if the hook's defvar were to
specify an initial value, this value won't take effect because of that
preceding `add-hook' invocation.

In this situation, add-hook doesn't so much "add" as it actually sets
the value of the hook.

The following patch is one way of having `add-hook' always add,
without overriding the "future" initial value of the hook. It is not
complete, it's intended as a possible outline (hopefully a
conversation starter).

The big disadvantage is that it's not backwards compatible. Any code
out there that relies on the variable being set by `add-hook' will
break.
There's also the disadvantage that it adds a symbol-property check to
every `defvar' call.

Any thoughts?

---
 lisp/subr.el | 68 ++++++++++++++++++++++++++++++++----------------------------
 src/eval.c   | 11 +++++++++-
 2 files changed, 46 insertions(+), 33 deletions(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index deadca6..a9a7aef 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -1389,38 +1389,42 @@ functions of the global value as well as in
the local value.
 HOOK should be a symbol, and FUNCTION may be any valid function.  If
 HOOK is void, it is first set to nil.  If HOOK's value is a single
 function, it is changed to a list of functions."
-  (or (boundp hook) (set hook nil))
-  (or (default-boundp hook) (set-default hook nil))
-  (if local (unless (local-variable-if-set-p hook)
-          (set (make-local-variable hook) (list t)))
-    ;; Detect the case where make-local-variable was used on a hook
-    ;; and do what we used to do.
-    (unless (and (consp (symbol-value hook)) (memq t (symbol-value hook)))
-      (setq local t)))
-  (let ((hook-value (if local (symbol-value hook) (default-value hook))))
-    ;; If the hook value is a single function, turn it into a list.
-    (when (or (not (listp hook-value)) (functionp hook-value))
-      (setq hook-value (list hook-value)))
-    ;; Do the actual addition if necessary
-    (unless (member function hook-value)
-      (when (stringp function)
-    (setq function (purecopy function)))
-      (setq hook-value
-        (if append
-        (append hook-value (list function))
-          (cons function hook-value))))
-    ;; Set the actual variable
-    (if local
-    (progn
-      ;; If HOOK isn't a permanent local,
-      ;; but FUNCTION wants to survive a change of modes,
-      ;; mark HOOK as partially permanent.
-      (and (symbolp function)
-           (get function 'permanent-local-hook)
-           (not (get hook 'permanent-local))
-           (put hook 'permanent-local 'permanent-local-hook))
-      (set hook hook-value))
-      (set-default hook hook-value))))
+  (if (not (boundp hook))
+      ;; If the hook is not defined yet, store the value until it is.
+      (push (list function append (when local (current-buffer)))
+            (get hook 'pre-hooked-functions))
+
+    ;; Everything below this line wasn't changed (indentation only).
+    (if local (unless (local-variable-if-set-p hook)
+                (set (make-local-variable hook) (list t)))
+      ;; Detect the case where make-local-variable was used on a hook
+      ;; and do what we used to do.
+      (unless (and (consp (symbol-value hook)) (memq t (symbol-value hook)))
+        (setq local t)))
+    (let ((hook-value (if local (symbol-value hook) (default-value hook))))
+      ;; If the hook value is a single function, turn it into a list.
+      (when (or (not (listp hook-value)) (functionp hook-value))
+        (setq hook-value (list hook-value)))
+      ;; Do the actual addition if necessary
+      (unless (member function hook-value)
+        (when (stringp function)
+          (setq function (purecopy function)))
+        (setq hook-value
+              (if append
+                  (append hook-value (list function))
+                (cons function hook-value))))
+      ;; Set the actual variable
+      (if local
+          (progn
+            ;; If HOOK isn't a permanent local,
+            ;; but FUNCTION wants to survive a change of modes,
+            ;; mark HOOK as partially permanent.
+            (and (symbolp function)
+                 (get function 'permanent-local-hook)
+                 (not (get hook 'permanent-local))
+                 (put hook 'permanent-local 'permanent-local-hook))
+            (set hook hook-value))
+        (set-default hook hook-value)))))

 (defun remove-hook (hook function &optional local)
   "Remove from the value of HOOK the function FUNCTION.
diff --git a/src/eval.c b/src/eval.c
index e828da9..119ffe0 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -729,7 +729,7 @@ To define a user option, use `defcustom' instead
of `defvar'.
 usage: (defvar SYMBOL &optional INITVALUE DOCSTRING)  */)
   (Lisp_Object args)
 {
-  Lisp_Object sym, tem, tail;
+  Lisp_Object sym, tem, tail, pre_hooked;

   sym = XCAR (args);
   tail = XCDR (args);
@@ -763,6 +763,15 @@ usage: (defvar SYMBOL &optional INITVALUE DOCSTRING)  */)
         tem = Fpurecopy (tem);
       Fput (sym, Qvariable_documentation, tem);
     }
+      /* In the final version, this should probably be
+         Qpre_hooked_functions. */
+      pre_hooked = Fget (sym, intern ("pre-hooked-functions"));;
+      if (!NILP (pre_hooked))
+        {
+          /* do something like add-hook on each element of the
+             pre_hooked list. */
+        }
+
       LOADHIST_ATTACH (sym);
     }
   else if (!NILP (Vinternal_interpreter_environment)
-- 
2.3.0

RE: add-hook and defvar

[Drew Adams]

> The big disadvantage is that it's not backwards compatible. Any code
> out there that relies on the variable being set by `add-hook' will
> break.

From that alone it does not sound like a good idea to me.  `add-hook'
has been around forever.  The amount of 3rd-party code that calls it
and expects the longstanding behavior must be non-neglible.

Just one opinion.

Re: add-hook and defvar

[Robin Templeton]

Artur Malabarba <bruce.connor.am@gmail.com> writes:

> While it's nice that `add-hook' tries to do the right thing even when
> the variable isn't defined yet, that's a bit of a false assurance.
> Although it's documented in the docstring, it seems undesirable that
> it sets the variable. This means that if the hook's defvar were to
> specify an initial value, this value won't take effect because of that
> preceding `add-hook' invocation.

This seems like a bug in the program defining the hook variable, not a
problem with `add-hook' per se; programs should always allow for the
possibility that the user has set their own value for a variable and
that the default value in the defvar form is ignored. I would expect the
program to call `add-hook' itself if the default hook functions were
important.

> In this situation, add-hook doesn't so much "add" as it actually sets
> the value of the hook.
>
> The following patch is one way of having `add-hook' always add,
> without overriding the "future" initial value of the hook. It is not
> complete, it's intended as a possible outline (hopefully a
> conversation starter).
>
> The big disadvantage is that it's not backwards compatible. Any code
> out there that relies on the variable being set by `add-hook' will
> break.
> There's also the disadvantage that it adds a symbol-property check to
> every `defvar' call.
>
> Any thoughts?

Another potential problem is that `add-hook' followed by `run-hooks'
will do nothing if the hook variable has not been defined. If the
behavior of `add-hook' were changed, I would prefer it if it simply
required the variable to be defined; that would avoid the above
disadvantages, would make it consistent with functions like
`add-to-list', and would normally ensure that the hook is initialized as
expected.

my 2¢,
robin
-- 
Inteligenta persono lernas la lingvon Esperanton rapide kaj facile.
Esperanto estas moderna, kultura lingvo por la mondo. Simpla, fleksebla,
belsona, Esperanto estas la praktika solvo de la problemo de universala
interkompreno. Lernu la interlingvon Esperanton!

Re: add-hook and defvar

[Artur Malabarba]

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

On Feb 21, 2015 8:35 PM, "Robin Templeton" <robin@terpri.org> wrote:
>
> Artur Malabarba <bruce.connor.am@gmail.com> writes:
>
> > While it's nice that `add-hook' tries to do the right thing even when
> > the variable isn't defined yet, that's a bit of a false assurance.
> > Although it's documented in the docstring, it seems undesirable that
> > it sets the variable. This means that if the hook's defvar were to
> > specify an initial value, this value won't take effect because of that
> > preceding `add-hook' invocation.
>
> This seems like a bug in the program defining the hook variable, not a
> problem with `add-hook' per se; programs should always allow for the
> possibility that the user has set their own value for a variable and
> that the default value in the defvar form is ignored.

The problem is not that the hook will stop working if it gets overriden,
it's more about the fact that the the user probably doesn't know that
`add-hook' is actually behaving like `override-hook'. I've been a
user/developer for years and I only just found out this year.

The fact that add-hook works even before the variable is defined gives the
impression that it's always safe to use. But it's actually quite the
opposite. It would have just been better if it behaved like all other
functions of this type, throwing an error if the variable isn't defined.

>I would expect the
> program to call `add-hook' itself if the default hook functions were
> important.

I'm not too worried about hook-functions that are vital, as the developer
can always just move these out of the hooks. But I am worried that the
developer trying to provide default behaviour for hooks has no sage way of
doing that.

If the program calls add-hook as you suggest, then the user who actually
*wants* to completely override the hook (by using `setq') won't be able to,
because you're manually adding these other values to it.

Defining a variable with an initial value is the standard way of offering a
default behaviour. And it's a problem that such a widely used function
doesn't play well with that.

> Another potential problem is that `add-hook' followed by `run-hooks'
> will do nothing if the hook variable has not been defined.

Yes, but isn't it a bug to run-hooks a variable that wasn't explicitly
defined?

> If the
> behavior of `add-hook' were changed, I would prefer it if it simply
> required the variable to be defined; that would avoid the above
> disadvantages, would make it consistent with functions like
> `add-to-list', and would normally ensure that the hook is initialized as
> expected.

I'm fine for that too. We could also provide a new function like
add-to-hook in order to provide backwards compatibility, and slowly
obsolete add-hook.

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

Re: add-hook and defvar

[Stefan Monnier]

> There's also the disadvantage that it adds a symbol-property check to
> every `defvar' call.

That's a fairly significant disadvantage.
Another problem is that defvar may simply never be called.


        Stefan

Re: add-hook and defvar

[Artur Malabarba]

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

On Feb 22, 2015 4:18 PM, "Stefan Monnier" <monnier@iro.umontreal.ca> wrote:
>
> > There's also the disadvantage that it adds a symbol-property check to
> > every `defvar' call.
>
> That's a fairly significant disadvantage.

Yes. We can also just throw an error if the variable isn't defined, like
every other function out there.

We can do that as a new function, `add-to-hook', and slowly obsolete
add-hook.

> Another problem is that defvar may simply never be called.

But isn't it a bug to run-hooks a variable that was never explicitly
defined?

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

Re: add-hook and defvar

[Stefan Monnier]

>> Another problem is that defvar may simply never be called.
> But isn't it a bug to run-hooks a variable that was never explicitly
> defined?

All hook functions have always treated "unbound" as equal to nil, so no,
it's not considered a bug to run a hook that's still unbound.

Basically, given that "unbound == nil", what is a bug is to defvar
a hook with a non-nil default value, unless that hook is predefined
(i.e. is never unbound).


        Stefan

Re: add-hook and defvar

[Philipp Stephani]

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

Stefan Monnier <monnier@iro.umontreal.ca> schrieb am Mon Feb 23 2015 at
23:32:20:

> >> Another problem is that defvar may simply never be called.
> > But isn't it a bug to run-hooks a variable that was never explicitly
> > defined?
>
> All hook functions have always treated "unbound" as equal to nil, so no,
> it's not considered a bug to run a hook that's still unbound.
>
> Basically, given that "unbound == nil", what is a bug is to defvar
> a hook with a non-nil default value, unless that hook is predefined
> (i.e. is never unbound).
>
>
Is this something the byte compiler could warn about? At least normal hooks
by convention end in "-hook", so a warning could be emitted every time such
a variable is defined that doesn't have nil as default.

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

Re: add-hook and defvar

[Stefan Monnier]

> Is this something the byte compiler could warn about? At least normal hooks
> by convention end in "-hook", so a warning could be emitted every time such
> a variable is defined that doesn't have nil as default.

Yes, we could do that.  Patch welcome,


        Stefan

Re: add-hook and defvar

[Philipp Stephani]

[-- Attachment #1.1: Type: text/plain, Size: 375 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> schrieb am Mi., 25. Feb. 2015 um
03:56 Uhr:

> > Is this something the byte compiler could warn about? At least normal
> hooks
> > by convention end in "-hook", so a warning could be emitted every time
> such
> > a variable is defined that doesn't have nil as default.
>
> Yes, we could do that.  Patch welcome,
>
>
>
Added a patch.

[-- Attachment #1.2: Type: text/html, Size: 674 bytes --]

[-- Attachment #2: 0001-Warn-if-a-hook-variable-is-given-a-non-nil-init-form.txt --]
[-- Type: text/plain, Size: 966 bytes --]

From 8aad74d2cb12a7afd6067b8737894c3a4b905a20 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Thu, 5 Mar 2015 21:06:07 +0100
Subject: [PATCH] Warn if a hook variable is given a non-nil init form

---
 lisp/emacs-lisp/bytecomp.el | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index e929c02..b9db322 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -2311,6 +2311,9 @@ list that represents a doc string reference.
 (defun byte-compile-file-form-defvar (form)
   (let ((sym (nth 1 form)))
     (byte-compile--declare-var sym)
+    (and (string-suffix-p "-hook" (symbol-name sym))
+         (nth 2 form)
+         (byte-compile-warn "hook variable `%s' has non-nil init value" sym))
     (if (eq (car form) 'defconst)
         (push sym byte-compile-const-variables)))
   (if (and (null (cddr form))		;No `value' provided.
-- 
2.2.0.rc0.207.ga3a616c

Re: add-hook and defvar

[Artur Malabarba]

This should probably do the same if the variable name ends with
"-functions" (since IIUC that's the correct way of naming a hook where
the functions should take arguments).

2015-03-05 17:28 GMT-03:00 Philipp Stephani <p.stephani2@gmail.com>:
>
>
> Stefan Monnier <monnier@iro.umontreal.ca> schrieb am Mi., 25. Feb. 2015 um
> 03:56 Uhr:
>
>> > Is this something the byte compiler could warn about? At least normal
>> > hooks
>> > by convention end in "-hook", so a warning could be emitted every time
>> > such
>> > a variable is defined that doesn't have nil as default.
>>
>> Yes, we could do that.  Patch welcome,
>>
>>
>
> Added a patch.

Re: add-hook and defvar

[Philipp Stephani]

[-- Attachment #1.1: Type: text/plain, Size: 933 bytes --]

Ack. Modified patch attached.

Btw, do I still need to write explicit changelog entries or is that already
done automatically?
Artur Malabarba <bruce.connor.am@gmail.com> schrieb am Do., 5. März 2015 um
22:45 Uhr:

> This should probably do the same if the variable name ends with
> "-functions" (since IIUC that's the correct way of naming a hook where
> the functions should take arguments).
>
> 2015-03-05 17:28 GMT-03:00 Philipp Stephani <p.stephani2@gmail.com>:
> >
> >
> > Stefan Monnier <monnier@iro.umontreal.ca> schrieb am Mi., 25. Feb. 2015
> um
> > 03:56 Uhr:
> >
> >> > Is this something the byte compiler could warn about? At least normal
> >> > hooks
> >> > by convention end in "-hook", so a warning could be emitted every time
> >> > such
> >> > a variable is defined that doesn't have nil as default.
> >>
> >> Yes, we could do that.  Patch welcome,
> >>
> >>
> >
> > Added a patch.
>

[-- Attachment #1.2: Type: text/html, Size: 1449 bytes --]

[-- Attachment #2: 0001-Warn-if-a-hook-variable-is-given-a-non-nil-init-form.txt --]
[-- Type: text/plain, Size: 1036 bytes --]

From ea7361c10c4deb13c25fefec705a05403506b860 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Thu, 5 Mar 2015 21:06:07 +0100
Subject: [PATCH] Warn if a hook variable is given a non-nil init form

---
 lisp/emacs-lisp/bytecomp.el | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index e929c02..22491a2 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -2311,6 +2311,10 @@ list that represents a doc string reference.
 (defun byte-compile-file-form-defvar (form)
   (let ((sym (nth 1 form)))
     (byte-compile--declare-var sym)
+    (and (or (string-suffix-p "-hook" (symbol-name sym))
+             (string-suffix-p "-functions" (symbol-name sym)))
+         (nth 2 form)
+         (byte-compile-warn "hook variable `%s' has non-nil init value" sym))
     (if (eq (car form) 'defconst)
         (push sym byte-compile-const-variables)))
   (if (and (null (cddr form))		;No `value' provided.
-- 
2.2.0.rc0.207.ga3a616c

Re: add-hook and defvar

[Artur Malabarba]

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

> Btw, do I still need to write explicit changelog entries?

Yes, M-x add-changelog-entry

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

Re: add-hook and defvar

[Stefan Monnier]

>> > Is this something the byte compiler could warn about? At least
>> > normal hooks by convention end in "-hook", so a warning could be
>> > emitted every time such a variable is defined that doesn't have nil
>> > as default.
>> Yes, we could do that.  Patch welcome,
> Added a patch.

After trying it out, I'm not sure we want it: there are various places
where we use a non-nil default value, and while it's not ideal, it's not
really clear what would be a better choice.

If there was a simple way to silence those warnings in those cases, it's
would be fine, but I can't think of a simple way to do that.


        Stefan