"Eager macro-expansion failure: (excessive-variable-binding)"

"Eager macro-expansion failure: (excessive-variable-binding)"

[Lars Ingebrigtsen]

Doing a bootstrap today I get:

  ELC      ../lisp/abbrev.elc
Local variable error: (error "Eager macro-expansion failure: (excessive-variable-binding)")
  ELC      ../lisp/bindings.elc
Local variable error: (error "Eager macro-expansion failure: (excessive-variable-binding)")
  ELC      ../lisp/buff-menu.elc
Local variable error: (error "Eager macro-expansion failure: (excessive-variable-binding)")
  ELC      ../lisp/button.elc
Local variable error: (error "Eager macro-expansion failure: (excessive-variable-binding)")
(etc etc)

I haven't tried debugging this -- perhaps it's obvious to somebody
what's triggering this?  And would it perhaps be possible to improve
this error message to make it give a hint about where it's finding the
problem?

Re: "Eager macro-expansion failure: (excessive-variable-binding)"

[Mattias Engdegård]

14 sep. 2022 kl. 14.18 skrev Lars Ingebrigtsen <larsi@gnus.org>:

>  ELC      ../lisp/abbrev.elc
> Local variable error: (error "Eager macro-expansion failure: (excessive-variable-binding)")

I got it too, debugged it and have now raised the limits. This came out:

The addition of vc-git-annotate-switches to .dir-locals.el in 3ddf1a9 had the effect of triggering the load of a raft of vc packages during byte-compilation, because that variable has the autoloaded `vc-git-annotate-switches-safe-p` as its `safe-local-variable` property.

This isn't ideal (probably makes build times go up a bit, maybe Lars can see it on his graphs?) but fixing the build was the first order of the day, and raising limits to those previously only used for nativecomp builds seemed to be a prudent move since they are effectively "pre-approved" in some sense.

I suggest we do something about the vc-git-annotate-switches problem anyway for general performance reasons, but that we keep the higher limits because what deity are we trying to please by having them too low for comfort anyway?

No blame on Stefan Kangas for adding the variable; it was done with the best of intentions and consequences difficult to foresee.

Re: "Eager macro-expansion failure: (excessive-variable-binding)"

[Lars Ingebrigtsen]

Mattias Engdegård <mattias.engdegard@gmail.com> writes:

> This isn't ideal (probably makes build times go up a bit, maybe Lars
> can see it on his graphs?)

Hm...  yes, it seems like this (or something else recently) increased
the "make -j32" build time from 1:08m to 1:17m, which is pretty
significant.  I'm doing further rebuilds to see if that's indeed the
culprit.

> I suggest we do something about the vc-git-annotate-switches problem
> anyway for general performance reasons,

It's a general issue with adding variables that are supposed to be safe
file-local in this way, I guess?  Perhaps we should instead `put' the
safe-local stuff in a central file that doesn't trigger loading the file
where the variable is defined?

Re: "Eager macro-expansion failure: (excessive-variable-binding)"

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Hm...  yes, it seems like this (or something else recently) increased
> the "make -j32" build time from 1:08m to 1:17m, which is pretty
> significant.  I'm doing further rebuilds to see if that's indeed the
> culprit.

Yes, but via ldefs-boot.el which was done a bit later.

The following patch brings build times back down to 1:08s.

diff --git a/lisp/ldefs-boot.el b/lisp/ldefs-boot.el
index 909ecf773c..fd2c68b98d 100644
--- a/lisp/ldefs-boot.el
+++ b/lisp/ldefs-boot.el
@@ -33564,7 +33564,7 @@ "vc-filewise"
 this list might be extended in the future.
 
 (fn SWITCHES)")
-(put 'vc-git-annotate-switches 'safe-local-variable #'vc-git-annotate-switches-safe-p)
+(put 'vc-git-annotate-switches 'safe-local-variable (lambda (switches) (equal switches "-w")))
  (defun vc-git-registered (file)
   "Return non-nil if FILE is registered with git."
   (if (vc-find-root file ".git")       ; Short cut.
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 8d8ea33f8b..0764282676 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -135,7 +135,7 @@ vc-git-annotate-switches
 		 (string :tag "Argument String")
 		 (repeat :tag "Argument List" :value ("") string))
   :version "25.1")
-;;;###autoload(put 'vc-git-annotate-switches 'safe-local-variable #'vc-git-annotate-switches-safe-p)
+;;;###autoload(put 'vc-git-annotate-switches 'safe-local-variable (lambda (switches) (equal switches "-w")))
 
 (defcustom vc-git-log-switches nil
   "String or list of strings specifying switches for Git log under VC."

Re: "Eager macro-expansion failure: (excessive-variable-binding)"

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: "Emacs-devel@gnu.org" <Emacs-devel@gnu.org>
> Date: Wed, 14 Sep 2022 17:38:00 +0200
> 
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
> > Hm...  yes, it seems like this (or something else recently) increased
> > the "make -j32" build time from 1:08m to 1:17m, which is pretty
> > significant.  I'm doing further rebuilds to see if that's indeed the
> > culprit.
> 
> Yes, but via ldefs-boot.el which was done a bit later.
> 
> The following patch brings build times back down to 1:08s.

Perhaps we should simply remove the variable from .dir-locals.el?  I
mean, through how many hoops do we want to jump for the sake of saving
some of us from doing this in their init files?  I think we went
overboard with this one.

Re: "Eager macro-expansion failure: (excessive-variable-binding)"

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Perhaps we should simply remove the variable from .dir-locals.el?  I
> mean, through how many hoops do we want to jump for the sake of saving
> some of us from doing this in their init files?  I think we went
> overboard with this one.

Well, it's fixed now, so...

Re: "Eager macro-expansion failure: (excessive-variable-binding)"

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: mattias.engdegard@gmail.com,  Emacs-devel@gnu.org
> Date: Wed, 14 Sep 2022 18:52:43 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Perhaps we should simply remove the variable from .dir-locals.el?  I
> > mean, through how many hoops do we want to jump for the sake of saving
> > some of us from doing this in their init files?  I think we went
> > overboard with this one.
> 
> Well, it's fixed now, so...

I cannot say I like the fix.  It's too much ado about a single obscure
variable.

Re: "Eager macro-expansion failure: (excessive-variable-binding)"

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I cannot say I like the fix.  It's too much ado about a single obscure
> variable.

Like I said earlier, I wonder whether we should do something in general
with the `safe-local-variable' things to avoid these issues for all
variables -- to make it easier to use them without pulling in the files
they're defined in.  It could make Emacs more efficient in general in
the long run.

I'm not sure what that would look like, though.  Putting them into
ldefs-boot.el "manually" this way isn't pretty...

Re: "Eager macro-expansion failure: (excessive-variable-binding)"

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: mattias.engdegard@gmail.com,  Emacs-devel@gnu.org
> Date: Wed, 14 Sep 2022 19:03:04 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I cannot say I like the fix.  It's too much ado about a single obscure
> > variable.
> 
> Like I said earlier, I wonder whether we should do something in general
> with the `safe-local-variable' things to avoid these issues for all
> variables -- to make it easier to use them without pulling in the files
> they're defined in.  It could make Emacs more efficient in general in
> the long run.

I'm not objected to finding a better solution, if we can find it.  I'm
talking about this particular variable and the mess that you needed to
go through to allow it being in .dir-locals.el without slowing down
the build.  It's unjustified.  I think we should delete it for now,
and revisit this when the infrastructure you are thinking about is
available.

Re: "Eager macro-expansion failure: (excessive-variable-binding)"

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I'm not objected to finding a better solution, if we can find it.  I'm
> talking about this particular variable and the mess that you needed to
> go through to allow it being in .dir-locals.el without slowing down
> the build.  It's unjustified.  I think we should delete it for now,
> and revisit this when the infrastructure you are thinking about is
> available.

It might just be as simple as having a guideline here ("don't
`safe-local-variable' autoloaded functions), but I don't know how
realistic that would be.  I'm grepping a bit, and there aren't too many
of these.  For instance:

;;;###autoload(put 'time-stamp-time-zone 'safe-local-variable 'time-stamp-zone-type-p)
;;;###autoload
(defun time-stamp-zone-type-p (zone)
  "Return non-nil if ZONE is of the correct type for a timezone rule.
Valid ZONE values are described in the documentation of `format-time-string'."
  (or (memq zone '(nil t wall))
      (stringp zone)
      (and (consp zone)
           (integerp (car zone))
           (consp (cdr zone))
           (stringp (cadr zone)))
      (integerp zone)))

Which is the identical problem.  But in this case, the function isn't
totally trivial, so perhaps we don't want that in ldefs-boot.el.

That's the only non-trivial case I've found after some cursory grepping.
Most of the other things here already do it correctly.  Like:

;;;###autoload(put 'reftex-vref-is-default 'safe-local-variable (lambda (x) (or (stringp x) (symbolp x))))

So our change here would be to transform all the things like:

;;;###autoload(put 'checkdoc-ispell-list-words 'safe-local-variable #'checkdoc-list-of-strings-p)
;;;###autoload
(defun checkdoc-list-of-strings-p (obj)
  "Return t when OBJ is a list of strings."
  ;; this is a function so it might be shared by checkdoc-proper-noun-list
  ;; and/or checkdoc-ispell-lisp-words in the future
  (and (listp obj)
       (not (memq nil (mapcar #'stringp obj)))))

into something similar.

Unless somebody has a different idea.