Question about defcustom and load-history

Question about defcustom and load-history

[Mauro Aranda]

Hello everybody.

Working in a little project of mine, I found myself in the need of exploring
'load-history', after loading a file.

I noticed that for files that define customizable variables, there's a
duplicate entry for each one of them.  However, symbols defined with
'defvar'
appear only once.  Does anybody know why is that?
It took me a little by surprise to see that.

For example, after loading comint, looking at the values for the file
comint, you'll see
something like:
    (require . ring)
    (require . ansi-color)
    (require . regexp-opt)
    comint-prompt-regexp
    comint-prompt-read-only
    comint-prompt-read-only

Best regards,
Mauro.

Re: Question about defcustom and load-history

[Tassilo Horn]

Mauro Aranda <maurooaranda@gmail.com> writes:

Hi Mauro,

> I noticed that for files that define customizable variables, there's a
> duplicate entry for each one of them.  However, symbols defined with
> 'defvar'
> appear only once.  Does anybody know why is that?

I'm not sure but I'd suspect this code in the end of
`custom-declare-variable'.

--8<---------------cut here---------------start------------->8---
  ;; Use defvar to set the docstring as well as the special-variable-p flag.
  ;; FIXME: We should reproduce more of `defvar's behavior, such as the warning
  ;; when the var is currently let-bound.
  (if (not (default-boundp symbol))
      ;; Don't use defvar to avoid setting a default-value when undesired.
      (when doc (put symbol 'variable-documentation doc))
    (eval `(defvar ,symbol nil ,@(when doc (list doc)))))
  (push symbol current-load-list)
  (run-hooks 'custom-define-hook)
  symbol)
--8<---------------cut here---------------end--------------->8---

In the falsy case, i.e., when the variable has an initial default value,
a `defvar' form is evaled resulting in one entry, and then the symbol is
pushed again to `current-load-list' from where it probably gets to
`load-history'.

I wonder how you could have a non-default-bound custom variable anyway?
Maybe using something like

  (defvar foo)
  (defcustom foo nil ...)
  
?

Anyway, I have no clue if your observation is itensional or
accidentally...

Bye,
Tassilo

Re: Question about defcustom and load-history

[Stefan Monnier]

> --8<---------------cut here---------------start------------->8---
>   ;; Use defvar to set the docstring as well as the special-variable-p flag.
>   ;; FIXME: We should reproduce more of `defvar's behavior, such as the warning
>   ;; when the var is currently let-bound.
>   (if (not (default-boundp symbol))
>       ;; Don't use defvar to avoid setting a default-value when undesired.
>       (when doc (put symbol 'variable-documentation doc))
>     (eval `(defvar ,symbol nil ,@(when doc (list doc)))))
>   (push symbol current-load-list)
>   (run-hooks 'custom-define-hook)
>   symbol)
> --8<---------------cut here---------------end--------------->8---
>
> In the falsy case, i.e., when the variable has an initial default value,
> a `defvar' form is evaled resulting in one entry, and then the symbol is
> pushed again to `current-load-list' from where it probably gets to
> `load-history'.

Indeed the manual handling of current-load-list should likely be moved
to the true branch of the `if`.

> I wonder how you could have a non-default-bound custom variable anyway?

By using appropriate :set and :get functions that store&fetch the value
from elsewhere (e.g. another variable, a frame-parameter, a file, some
symbol property, younameit).

I can't seem to find a case where we do that, tho, so maybe we should
declare such uses invalid.


        Stefan

Re: Question about defcustom and load-history

[Mauro Aranda]

>> In the falsy case, i.e., when the variable has an initial default value,
>> a `defvar' form is evaled resulting in one entry, and then the symbol is
>> pushed again to `current-load-list' from where it probably gets to
>> `load-history'.
>
> Indeed the manual handling of current-load-list should likely be moved
> to the true branch of the `if`.

Hello Tassilo and Stefan,

Thank you both for your answers.

After reading Tassilo explanation, I went on to have a look at
'custom.el' and found out that if a custom variable has
'custom-initialize-default' as the :initialize function, a 'defvar' is
evaled too.  So, those custom variables appear thrice in 'load-history'.

For example, loading 'ansi-color', you'll see
'ansi-color-faces-vector' and 'ansi-color-names-vector' triplicated.

This doesn't look intentional to me.  Should I file a bug report?

Best regards,
Mauro.

El dom., 31 mar. 2019 a las 5:05, Stefan Monnier (<monnier@iro.umontreal.ca>)
escribió:

> > --8<---------------cut here---------------start------------->8---
> >   ;; Use defvar to set the docstring as well as the special-variable-p
> flag.
> >   ;; FIXME: We should reproduce more of `defvar's behavior, such as the
> warning
> >   ;; when the var is currently let-bound.
> >   (if (not (default-boundp symbol))
> >       ;; Don't use defvar to avoid setting a default-value when
> undesired.
> >       (when doc (put symbol 'variable-documentation doc))
> >     (eval `(defvar ,symbol nil ,@(when doc (list doc)))))
> >   (push symbol current-load-list)
> >   (run-hooks 'custom-define-hook)
> >   symbol)
> > --8<---------------cut here---------------end--------------->8---
> >
> > In the falsy case, i.e., when the variable has an initial default value,
> > a `defvar' form is evaled resulting in one entry, and then the symbol is
> > pushed again to `current-load-list' from where it probably gets to
> > `load-history'.
>
> Indeed the manual handling of current-load-list should likely be moved
> to the true branch of the `if`.
>
> > I wonder how you could have a non-default-bound custom variable anyway?
>
> By using appropriate :set and :get functions that store&fetch the value
> from elsewhere (e.g. another variable, a frame-parameter, a file, some
> symbol property, younameit).
>
> I can't seem to find a case where we do that, tho, so maybe we should
> declare such uses invalid.
>
>
>         Stefan
>
>
>

Re: Question about defcustom and load-history

[Stefan Monnier]

Mauro Aranda <maurooaranda@gmail.com> writes:
>>> In the falsy case, i.e., when the variable has an initial default value,
>>> a `defvar' form is evaled resulting in one entry, and then the symbol is
>>> pushed again to `current-load-list' from where it probably gets to
>>> `load-history'.
>>
>> Indeed the manual handling of current-load-list should likely be moved
>> to the true branch of the `if`.
>
> Hello Tassilo and Stefan,
>
> Thank you both for your answers.
>
> After reading Tassilo explanation, I went on to have a look at
> 'custom.el' and found out that if a custom variable has
> 'custom-initialize-default' as the :initialize function, a 'defvar' is
> evaled too.  So, those custom variables appear thrice in 'load-history'.
>
> For example, loading 'ansi-color', you'll see
> 'ansi-color-faces-vector' and 'ansi-color-names-vector' triplicated.
>
> This doesn't look intentional to me.  Should I file a bug report?

I haven't seen a corresponding bug-report, so I'll reply here.
I installed the patch below into `master` which should fix this problem
for good.


        Stefan


    * lisp/custom.el: Avoid adding vars to load-history multiple times
    
    Avoid the abuse of (eval `(defvar ...)) which tends to end up
    adding redundant entries in `load-history`, as discussed in
    https://lists.gnu.org/r/help-gnu-emacs/2019-03/msg00237.html
    
    (custom-initialize-default): Don't add to load-history.
    (custom-declare-variable): Use internal--define-uninitialized-variable
    and only add the var to load-history once.  Do it before calling
    `initialize` so the special-variable-p flag is set.
    
    * src/eval.c (Finternal__define_uninitialized_variable): New function.
    (Fdefvar, Fdefconst): Use it.
    (syms_of_eval): Defsubr' it.


diff --git a/lisp/custom.el b/lisp/custom.el
index 53b8045f05..29bf9e570a 100644
--- a/lisp/custom.el
+++ b/lisp/custom.el
@@ -56,8 +56,14 @@ custom-initialize-default
 the car of that and use it as the default binding for symbol.
 Otherwise, EXP will be evaluated and used as the default binding for
 symbol."
-  (eval `(defvar ,symbol ,(let ((sv (get symbol 'saved-value)))
-                            (if sv (car sv) exp)))))
+  (condition-case nil
+      (default-toplevel-value symbol)   ;Test presence of default value.
+    (void-variable
+     ;; The var is not initialized yet.
+     (set-default-toplevel-value
+      symbol (eval (let ((sv (get symbol 'saved-value)))
+                     (if sv (car sv) exp))
+                   t)))))
 
 (defun custom-initialize-set (symbol exp)
   "Initialize SYMBOL based on EXP.
@@ -188,18 +194,13 @@ custom-declare-variable
 		(t
 		 (custom-handle-keyword symbol keyword value
 					'custom-variable))))))
+    ;; Set the docstring, record the var on load-history, as well
+    ;; as set the special-variable-p flag.
+    (internal--define-uninitialized-variable symbol doc)
     (put symbol 'custom-requests requests)
     ;; Do the actual initialization.
     (unless custom-dont-initialize
       (funcall initialize symbol default)))
-  ;; Use defvar to set the docstring as well as the special-variable-p flag.
-  ;; FIXME: We should reproduce more of `defvar's behavior, such as the warning
-  ;; when the var is currently let-bound.
-  (if (not (default-boundp symbol))
-      ;; Don't use defvar to avoid setting a default-value when undesired.
-      (when doc (put symbol 'variable-documentation doc))
-    (eval `(defvar ,symbol nil ,@(when doc (list doc)))))
-  (push symbol current-load-list)
   (run-hooks 'custom-define-hook)
   symbol)
 
diff --git a/src/eval.c b/src/eval.c
index 3fd9a40a3a..567c32e0d7 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -715,6 +715,25 @@ DEFUN ("set-default-toplevel-value", Fset_default_toplevel_value,
   return Qnil;
 }
 
+DEFUN ("internal--define-uninitialized-variable",
+       Finternal__define_uninitialized_variable,
+       Sinternal__define_uninitialized_variable, 1, 2, 0,
+       doc: /* Define SYMBOL as a variable, with DOC as its docstring.
+This is like `defvar' and `defconst' but without affecting the variable's
+value.  */)
+  (Lisp_Object symbol, Lisp_Object doc)
+{
+  XSYMBOL (symbol)->u.s.declared_special = true;
+  if (!NILP (doc))
+    {
+      if (!NILP (Vpurify_flag))
+	doc = Fpurecopy (doc);
+      Fput (symbol, Qvariable_documentation, doc);
+    }
+  LOADHIST_ATTACH (symbol);
+  return Qnil;
+}
+
 DEFUN ("defvar", Fdefvar, Sdefvar, 1, UNEVALLED, 0,
        doc: /* Define SYMBOL as a variable, and return SYMBOL.
 You are not required to define a variable in order to use it, but
@@ -754,32 +773,25 @@ usage: (defvar SYMBOL &optional INITVALUE DOCSTRING)  */)
     {
       if (!NILP (XCDR (tail)) && !NILP (XCDR (XCDR (tail))))
 	error ("Too many arguments");
+      Lisp_Object exp = XCAR (tail);
 
       tem = Fdefault_boundp (sym);
+      tail = XCDR (tail);
 
       /* Do it before evaluating the initial value, for self-references.  */
-      XSYMBOL (sym)->u.s.declared_special = true;
+      Finternal__define_uninitialized_variable (sym, CAR (tail));
 
       if (NILP (tem))
-	Fset_default (sym, eval_sub (XCAR (tail)));
+	Fset_default (sym, eval_sub (exp));
       else
 	{ /* Check if there is really a global binding rather than just a let
 	     binding that shadows the global unboundness of the var.  */
 	  union specbinding *binding = default_toplevel_binding (sym);
 	  if (binding && EQ (specpdl_old_value (binding), Qunbound))
 	    {
-	      set_specpdl_old_value (binding, eval_sub (XCAR (tail)));
+	      set_specpdl_old_value (binding, eval_sub (exp));
 	    }
 	}
-      tail = XCDR (tail);
-      tem = Fcar (tail);
-      if (!NILP (tem))
-	{
-	  if (!NILP (Vpurify_flag))
-	    tem = Fpurecopy (tem);
-	  Fput (sym, Qvariable_documentation, tem);
-	}
-      LOADHIST_ATTACH (sym);
     }
   else if (!NILP (Vinternal_interpreter_environment)
 	   && (SYMBOLP (sym) && !XSYMBOL (sym)->u.s.declared_special))
@@ -827,19 +839,12 @@ usage: (defconst SYMBOL INITVALUE [DOCSTRING])  */)
       docstring = XCAR (XCDR (XCDR (args)));
     }
 
+  Finternal__define_uninitialized_variable (sym, docstring);
   tem = eval_sub (XCAR (XCDR (args)));
   if (!NILP (Vpurify_flag))
     tem = Fpurecopy (tem);
-  Fset_default (sym, tem);
-  XSYMBOL (sym)->u.s.declared_special = true;
-  if (!NILP (docstring))
-    {
-      if (!NILP (Vpurify_flag))
-	docstring = Fpurecopy (docstring);
-      Fput (sym, Qvariable_documentation, docstring);
-    }
-  Fput (sym, Qrisky_local_variable, Qt);
-  LOADHIST_ATTACH (sym);
+  Fset_default (sym, tem);      /* FIXME: set-default-toplevel-value? */
+  Fput (sym, Qrisky_local_variable, Qt); /* FIXME: Why?  */
   return sym;
 }
 
@@ -4198,6 +4203,7 @@ alist of active lexical bindings.  */);
   defsubr (&Sdefvaralias);
   DEFSYM (Qdefvaralias, "defvaralias");
   defsubr (&Sdefconst);
+  defsubr (&Sinternal__define_uninitialized_variable);
   defsubr (&Smake_var_non_special);
   defsubr (&Slet);
   defsubr (&SletX);

Re: Question about defcustom and load-history

[Mauro Aranda]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> This doesn't look intentional to me.  Should I file a bug report?
>
> I haven't seen a corresponding bug-report, so I'll reply here.
> I installed the patch below into `master` which should fix this problem
> for good.
>
>
>         Stefan

Thank you Stefan!