Recursive Fload and eval-after-load forms. (See bug #43116.)

Recursive Fload and eval-after-load forms. (See bug #43116.)

[Alan Mackenzie]

Hello, Emacs.

In bug #43116, the OP has rightly complained that on loading
fortran.elc, his eval-after-load forms get evaluated twice.

The cause of the double evaluation is a custom-menu-create form in
fortran.el, which causes a recursive evaluation of (load "fortran").
The eval-after-load-forms are evaluated both for the "inner" load and
the "outer" load.

What do people think of the following proposal: that the eval-after-load
forms should be evaluated only after the outermost load has completed?
This would be a simple amendment to the function Fload.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Recursive Fload and eval-after-load forms. (See bug #43116.)

[Stefan Monnier]

> The cause of the double evaluation is a custom-menu-create form in
> fortran.el, which causes a recursive evaluation of (load "fortran").

That sounds like a misfeature we should address.

> What do people think of the following proposal: that the eval-after-load
> forms should be evaluated only after the outermost load has completed?
> This would be a simple amendment to the function Fload.

I think it would only hide the problem and would make the specification
of `eval-after-load` needlessly more complex.

Recursive loading of a file should be rare and should always be
considered as a bug/misfeature in itself, so I don't think we should
take extra steps to try and better support it.


        Stefan

Re: Recursive Fload and eval-after-load forms. (See bug #43116.)

[Alan Mackenzie]

Hello, Stefan.

Thanks for such a rapid reply!

On Mon, Aug 31, 2020 at 14:54:54 -0400, Stefan Monnier wrote:
> > The cause of the double evaluation is a custom-menu-create form in
> > fortran.el, which causes a recursive evaluation of (load "fortran").

> That sounds like a misfeature we should address.

Yes.  The idea of a recursive load left me a bit shocked.  Fload bails
out with an error if the nesting of files it's loading has a file more
than three times in the stack.

> > What do people think of the following proposal: that the
> > eval-after-load forms should be evaluated only after the outermost
> > load has completed?  This would be a simple amendment to the function
> > Fload.

> I think it would only hide the problem and would make the specification
> of `eval-after-load` needlessly more complex.

I suppose it would.

> Recursive loading of a file should be rare and should always be
> considered as a bug/misfeature in itself, so I don't think we should
> take extra steps to try and better support it.

Maybe instead we could modify custom-menu-create such that it will only
require its target file if that file isn't currently being loaded.  If it
is currently being loaded, then we only include the customisable
variables that have already been defined.  This shouldn't be too much of
a hardship, since custom variables tend to be defined near the start of
a .el file, and the higher level functions which create the custom menu
tend to be nearer the end of the file.

What do you think?

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Recursive Fload and eval-after-load forms. (See bug #43116.)

[Nonax]

Hi all,

I looked into a matter a bit and, while I don't have a solution myself,
noticed something very strange.  You made me aware of the fact that the
fortran modes (both fortran-mode and f90) use custom-menu-create
directly.  The thing I find exceptional about that is that absolutely no
other major mode uses this function this way, period.  A quick grep
shows that the only other file in the lisp/ directory using it is
eudc.el, and it uses it to bind the return value to a const.

So it seems that it does a rather standard thing (setting up a menu for
a major mode) in a very exceptional way.  I am still quite new to
writing modes, but I guess one could circumvent the direct usage of
custom-menu-create entirely.

I've also been poking around some more to see what modules use it
indirectly, to see how they deal with potential issues.  The function is
used by two more functions in cus-edit: custom-group-menu-create (unused
in the entire lisp/ subdir) and customize-menu-create, which is used by
cus-edit itself once to create Custom-mode-menu.  Almost all other
modules using customize-menu-create (org.el, idlwave.el, reftex.el)
define a function <mode>-create-customize-menu which calls it in return.
 The only other module using it differently is wid-browse.el, which uses
it in a similar way cus-edit itself uses it.

So all in all the hierarchy of files using custom-menu-create at all is
quite small, and it seems the majority does not actually call it at
evaluation time, but instead have it called as a function bound to a
menu point (meaning it will be called upon user input when everything
important already happened).

I would have to do a lot more digging to really get what
custom-menu-create and family really do, but I get the feeling that
fortran-mode kind of (ab)uses it in a way that was not originally
anticipated.  I hope this information proves somewhat useful, but my
pattern recognition tells me it does.

Cheers,
Nonax

On 31/08/2020 20:45, Alan Mackenzie wrote:
> Hello, Emacs.
> 
> In bug #43116, the OP has rightly complained that on loading
> fortran.elc, his eval-after-load forms get evaluated twice.
> 
> The cause of the double evaluation is a custom-menu-create form in
> fortran.el, which causes a recursive evaluation of (load "fortran").
> The eval-after-load-forms are evaluated both for the "inner" load and
> the "outer" load.
> 
> What do people think of the following proposal: that the eval-after-load
> forms should be evaluated only after the outermost load has completed?
> This would be a simple amendment to the function Fload.
> 

Re: Recursive Fload and eval-after-load forms. (See bug #43116.)

[Stefan Monnier]

> Fload bails out with an error if the nesting of files it's loading has
> a file more than three times in the stack.

Indeed, we know that recursive loads do exist and don't always end up
inf-looping, so we only signal an infinite recursive load when it looks
*really* bad ;-)

> Maybe instead we could modify custom-menu-create such that it will only
> require its target file if that file isn't currently being loaded.

We could try that.  Or we could make it return a dynamic menu whose
content is only computed when the menu is actually displayed.

> If it is currently being loaded, then we only include the customisable
> variables that have already been defined.  This shouldn't be too much
> of a hardship, since custom variables tend to be defined near the
> start of a .el file, and the higher level functions which create the
> custom menu tend to be nearer the end of the file.
>
> What do you think?

According to `grep` it would be good enough for the 3 use cases we have
bundled with Emacs.


        Stefan

Re: Recursive Fload and eval-after-load forms. (See bug #43116.)

[Alan Mackenzie]

Hello, Stefan.

On Mon, Aug 31, 2020 at 23:18:35 -0400, Stefan Monnier wrote:
> > Fload bails out with an error if the nesting of files it's loading has
> > a file more than three times in the stack.

> Indeed, we know that recursive loads do exist and don't always end up
> inf-looping, so we only signal an infinite recursive load when it looks
> *really* bad ;-)

> > Maybe instead we could modify custom-menu-create such that it will only
> > require its target file if that file isn't currently being loaded.

> We could try that.  Or we could make it return a dynamic menu whose
> content is only computed when the menu is actually displayed.

> > If it is currently being loaded, then we only include the customisable
> > variables that have already been defined.  This shouldn't be too much
> > of a hardship, since custom variables tend to be defined near the
> > start of a .el file, and the higher level functions which create the
> > custom menu tend to be nearer the end of the file.

> > What do you think?

> According to `grep` it would be good enough for the 3 use cases we have
> bundled with Emacs.

Something like:


diff --git a/lisp/custom.el b/lisp/custom.el
index 885c486c5e..f8e154e7ae 100644
--- a/lisp/custom.el
+++ b/lisp/custom.el
@@ -649,7 +649,8 @@ custom-load-symbol
       (ignore-errors
         (require 'cus-start))
       (dolist (load (get symbol 'custom-loads))
-        (cond ((symbolp load) (ignore-errors (require load)))
+        (cond ((being-loaded-p load))   ; For bug #43116
+              ((symbolp load) (ignore-errors (require load)))
 	      ;; This is subsumed by the test below, but it's much faster.
 	      ((assoc load load-history))
 	      ;; This was just (assoc (locate-library load) load-history)


, where `being-loaded-p' would be a new primitive in lread.c which would
just scan the static variable Vloads_in_progress with Fmember.  Of
course, some doc strings would need amendments, too.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Recursive Fload and eval-after-load forms. (See bug #43116.)

[Stefan Monnier]

> -        (cond ((symbolp load) (ignore-errors (require load)))
> +        (cond ((being-loaded-p load))   ; For bug #43116
> +              ((symbolp load) (ignore-errors (require load)))

Maybe a simpler test that doesn't require a new ad-hoc primitive is to
test whether that group has already been defined?


        Stefan


diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
index 466727630f..8f1e5968b5 100644
--- a/lisp/cus-edit.el
+++ b/lisp/cus-edit.el
@@ -4844,7 +4844,8 @@ custom-menu-create
     (if (and (or (not (boundp 'custom-menu-nesting))
 		 (>= custom-menu-nesting 0))
 	     (progn
-	       (custom-load-symbol symbol)
+	       (unless (get symbol 'group-documentation)
+	         (custom-load-symbol symbol))
 	       (< (length (get symbol 'custom-group)) widget-menu-max-size)))
 	(let ((custom-prefix-list (custom-prefix-add symbol
 						     custom-prefix-list))