bug#38694: 26.1; update-*-autoloads fails with local generated-autoload-file

bug#38694: 26.1; update-*-autoloads fails with local generated-autoload-file

[Braun Gábor]

Let the current directory have only the following two files:

------------------ test-foo.el -------------------------
;;;###autoload
(defun test-foo-fun ()
  "FOO: a function just for testing."
  t)

;; Local Variables:
;; generated-autoload-file: "foo-autoloads.el"
;; End:
--------------------------------------------------------


------------------ test-bar.el -------------------------
;;;###autoload
(defun test-bar-fun ()
  "BAR: a noop test function."
  nil)
--------------------------------------------------------

1) Do the following:

emacs -Q test-foo.el

M-x update-file-autoloads RET
test-bar.el RET
test-autoloads.el RET


It produces an error message, which I copied from buffer *Messages*:

autoload-generate-file-autoloads: DIRECTORY/test-bar.el:0:0: error: 
wrong-type-argument: (stringp nil)

where in place of DIRECTORY there is the directory the files
above reside.

2) Similar as above but try generating autoloads for the whole
directory:

emacs -Q test-foo.el

M-x update-directory-autoloads RET RET
test-autoloads.el RET


This time the error message is (again from buffer *Messages*):

autoload-generate-file-autoloads: test-bar.el:0:0: error: wrong-type-
argument: (stringp nil)


IMHO, the root of the problem is that update-file-autoloads and
update-directory-autoloads bind generated-autoload-file buffer locally
(as it is a file local variable in test-foo.el) but intend to bind the
default value.  Note that the docstrings of both functions
mention the case of generated-autoload-file being buffer-local, so
they ought to support this case.

As the documented behaviour of these functions do not
depend on the current buffer, I propose to simply switch to a
temporary buffer so that the local settings don't interfere.
This is a preemptive measure of other similar latent problems.

Admittedly, creating a temporary buffer just to bind variables'
default value seems to be wasteful, and I am very interested in
learning a better, more idiomatic approach.

All in all, the patch is the following, adding wrappers
`(with-temp-buffer ...)'.  (The code has not been reindented, 
reformatted
solely to keep the patch readable.)

--- lisp/emacs-lisp/autoload.el
+++ lisp/emacs-lisp/autoload.el
@@ -916,6 +916,9 @@ update-file-autoloads
   (interactive (list (read-file-name "Update autoloads for file: ")
                     current-prefix-arg
                     (read-file-name "Write autoload definitions to 
file: ")))
+  ;; The temporary buffer is only to bind the default value of e.g.,
+  ;; `generated-autoload-file'.  The current buffer is irrelevant.
+  (with-temp-buffer
   (let* ((generated-autoload-file (or outfile generated-autoload-file))
         (autoload-modified-buffers nil)
         ;; We need this only if the output file handles more than one 
input.
@@ -927,6 +930,7 @@ update-file-autoloads
       (if (called-interactively-p 'interactive)
           (message "Autoload section for %s is up to date." file)))
     (if no-autoloads file)))
+  )

 (defun autoload-find-destination (file load-name)
   "Find the destination point of the current buffer's autoloads.
@@ -1028,6 +1032,9 @@ update-directory-autoloads
 file binds `generated-autoload-file' as a file-local variable,
 write its autoloads into the specified file instead."
   (interactive "DUpdate autoloads from directory: ")
+  ;; The temporary buffer is only to bind default value of e.g.,
+  ;; `generated-autoload-file'.  The current buffer is irrelevant.
+  (with-temp-buffer
   (let* ((files-re (let ((tmp nil))
                     (dolist (suf (get-load-suffixes))
                        ;; We don't use module-file-suffix below because
@@ -1142,6 +1149,7 @@ update-directory-autoloads
       ;; In case autoload entries were added to other files because of
       ;; file-local autoload-generated-file settings.
       (autoload-save-buffers))))
+  )

 (define-obsolete-function-alias 'update-autoloads-from-directories
     'update-directory-autoloads "22.1")



Best wishes,

     Gábor


In GNU Emacs 26.1 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.5)
 of 2019-09-23, modified by Debian built on x86-grnet-01
Windowing system distributor 'The X.Org Foundation', version 
11.0.12004000
System Description:	Debian GNU/Linux 10 (buster)

Recent messages:
Generating autoloads for ~/.emacs.d/autoload-test/test-foo.el...
(New file)
Generating autoloads for ~/.emacs.d/autoload-test/test-foo.el...done
Quit
Generating autoloads for ~/.emacs.d/autoload-test/test-bar.el...
(New file)
Generating autoloads for ~/.emacs.d/autoload-test/test-bar.el...done
scroll-down-command: Beginning of buffer
C-c M-w is undefined
scroll-up-command: End of buffer

Configured using:
 'configure --build x86_64-linux-gnu --prefix=/usr
 --sharedstatedir=/var/lib --libexecdir=/usr/lib
 --localstatedir=/var/lib --infodir=/usr/share/info
 --mandir=/usr/share/man --enable-libsystemd --with-pop=yes
 --enable-locallisppath=/etc/emacs:/usr/local/share/emacs/26.1/site-
lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/26.1/site-lisp:/
usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-mailutils --build
 x86_64-linux-gnu --prefix=/usr --sharedstatedir=/var/lib
 --libexecdir=/usr/lib --localstatedir=/var/lib
 --infodir=/usr/share/info --mandir=/usr/share/man --enable-libsystemd
 --with-pop=yes
 --enable-locallisppath=/etc/emacs:/usr/local/share/emacs/26.1/site-
lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/26.1/site-lisp:/
usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-mailutils --with-x=yes
 --with-x-toolkit=gtk3 --with-toolkit-scroll-bars 'CFLAGS=-g -O2
 -fdebug-prefix-map=/build/emacs-StqULU/emacs-26.1+1=. -fstack-
protector-strong
 -Wformat -Werror=format-security -Wall' 'CPPFLAGS=-Wdate-time
 -D_FORTIFY_SOURCE=2' LDFLAGS=-Wl,-z,relro'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS NOTIFY
ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 THREADS LIBSYSTEMD LCMS2

Important settings:
  value of $LANG: hu_HU.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  diff-auto-refine-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny seq byte-opt bytecomp
byte-compile cconv dired dired-loaddefs format-spec rfc822 mml mml-sec
password-cache epa derived epg epg-config gnus-util rmail rmail-loaddefs
mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils
mailheader sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr
mail-utils cl-macs gv pcase ediff-ptch ediff-merg ediff-wind ediff-diff
ediff-mult ediff-help ediff-init cl-loaddefs cl-lib ediff-util ediff
jka-compr thingatpt find-func autoload radix-tree lisp-mnt vc-git
diff-mode easymenu easy-mmode elec-pair time-date mule-util tooltip
eldoc electric uniquify ediff-hook vc-hooks lisp-float-type mwheel
term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode elisp-mode
lisp-mode prog-mode register page menu-bar rfn-eshadow isearch timer
select scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting move-toolbar gtk x-toolkit x multi-tty
make-network-process emacs)

Memory information:
((conses 16 116513 9893)
 (symbols 48 22483 1)
 (miscs 40 106 201)
 (strings 32 34406 1337)
 (string-bytes 1 943432)
 (vectors 16 17409)
 (vector-slots 8 538130 13522)
 (floats 8 69 226)
 (intervals 56 380 0)
 (buffers 992 20))

bug#38694: 26.1; update-*-autoloads fails with local generated-autoload-file

[Lars Ingebrigtsen]

Braun Gábor <braungb88@gmail.com> writes:

> Let the current directory have only the following two files:

[...]

> ;; Local Variables:
> ;; generated-autoload-file: "foo-autoloads.el"
> ;; End:

> M-x update-file-autoloads RET
> test-bar.el RET
> test-autoloads.el RET
>
> It produces an error message, which I copied from buffer *Messages*:
>
> autoload-generate-file-autoloads: DIRECTORY/test-bar.el:0:0: error: 
> wrong-type-argument: (stringp nil)

I can confirm this bug.

[...]

> IMHO, the root of the problem is that update-file-autoloads and
> update-directory-autoloads bind generated-autoload-file buffer locally
> (as it is a file local variable in test-foo.el) but intend to bind the
> default value.  Note that the docstrings of both functions
> mention the case of generated-autoload-file being buffer-local, so
> they ought to support this case.
>
> As the documented behaviour of these functions do not
> depend on the current buffer, I propose to simply switch to a
> temporary buffer so that the local settings don't interfere.
> This is a preemptive measure of other similar latent problems.
>
> Admittedly, creating a temporary buffer just to bind variables'
> default value seems to be wasteful, and I am very interested in
> learning a better, more idiomatic approach.

I don't think there is, and that's perhaps unfortunate.  What we have
here is a pretty unusual situation, though.

> +  ;; The temporary buffer is only to bind the default value of e.g.,
> +  ;; `generated-autoload-file'.  The current buffer is irrelevant.
> +  (with-temp-buffer
>    (let* ((generated-autoload-file (or outfile generated-autoload-file))

But I don't think this is the right way to fix this?  We want to heed
the buffer-local value of generated-autoload-file, and we want to bind
the global value of generated-autoload-file, I think?  Otherwise, what
would be the point of

> ;; Local Variables:
> ;; generated-autoload-file: "foo-autoloads.el"
> ;; End:

I'm not very familiar with this code, though, so I may be misreading it.

So the patch would be something like

+  (let ((autoload-file (or outfile generated-autoload-file)))
+    (with-temp-buffer
+      (let* ((generated-autoload-file autoload-file)

Or more complete (with indentation changes) below.

Does that makes sense?

diff --git a/lisp/emacs-lisp/autoload.el b/lisp/emacs-lisp/autoload.el
index 2eef451200..ad39f5642b 100644
--- a/lisp/emacs-lisp/autoload.el
+++ b/lisp/emacs-lisp/autoload.el
@@ -924,17 +924,20 @@ update-file-autoloads
   (interactive (list (read-file-name "Update autoloads for file: ")
 		     current-prefix-arg
 		     (read-file-name "Write autoload definitions to file: ")))
-  (let* ((generated-autoload-file (or outfile generated-autoload-file))
-	 (autoload-modified-buffers nil)
-	 ;; We need this only if the output file handles more than one input.
-	 ;; See https://debbugs.gnu.org/22213#38 and subsequent.
-	 (autoload-timestamps t)
-         (no-autoloads (autoload-generate-file-autoloads file)))
-    (if autoload-modified-buffers
-        (if save-after (autoload-save-buffers))
-      (if (called-interactively-p 'interactive)
-          (message "Autoload section for %s is up to date." file)))
-    (if no-autoloads file)))
+  (let ((autoload-file (or outfile generated-autoload-file)))
+    (with-temp-buffer
+      (let* ((generated-autoload-file autoload-file)
+	     (autoload-modified-buffers nil)
+	     ;; We need this only if the output file handles more than
+	     ;; one input.  See https://debbugs.gnu.org/22213#38 and
+	     ;; subsequent.
+	     (autoload-timestamps t)
+             (no-autoloads (autoload-generate-file-autoloads file)))
+        (if autoload-modified-buffers
+            (if save-after (autoload-save-buffers))
+          (if (called-interactively-p 'interactive)
+              (message "Autoload section for %s is up to date." file)))
+        (if no-autoloads file)))))
 
 (defun autoload-find-destination (file load-name)
   "Find the destination point of the current buffer's autoloads.


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

bug#38694: 26.1; update-*-autoloads fails with local generated-autoload-file

[Lars Ingebrigtsen]

This has now been fixed in Emacs 28 with the introduction of the new
`make-directory-autoloads' command.

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