bug#53632: Function definition history

bug#53632: Function definition history

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Tags: patch

I don't much like the code we have in Fdefalias that tries to keep track
of definitions so as to be able to undo them later.
It's too ad-hoc for its own good.

The patch below tries to make it a bit better defined.
We used to store in `load-history` when an autoload is redefined as
a non-autoload and in the `autoload` symbol property we used to store
the autoload data that used to be used before it got overriden.

I suggest to replace that info with something slightly more complete.
In the patch below I store the history of the function definition of
a symbol in its `function-history` symbol property.  This history is
stored as a list of the form (... VAL(n+1) FILE(n+1) VALn FILEn ...)
where VALn is the value set by FILEn.  To make this list cheap in the
default case, the latest value is not stored in the list (since it's in
the `symbol-function`) and neither is the first file.  So if there's
only been a single definition (the most common case), the list is empty
and the property is just not present at all.  If a function was first
defined as an autoload and then overriden by the actual function
definition, then the list will hold (FILE2 AUTOLOAD), i.e. the name of
the file that provided the actual function definition and the autoload
that was used before that.
[ Note: the name of the file that provided the first definition can be
  recovered if really needed by checking all entries in `load-history`.
  In the patch below I have not needed it.  ]

This makes it possible to handle correctly things like unloading
`cl-loaddefs.el` which should remove the autoloads that are still
autoloads and leave untouched the functions whose autoload have been
replaced by the actual function definition.

In my tests it increased the size of the .pdmp by about 2KB (on a 32bit
build).

The patch also gets rid of the `autoload` vs `defun` distinction in
`load-history` which seems unnecessary (a significant part of the
motivation for this patch was to get rid of the special handling of
autoloads in this part of the code).  At least I couldn't find any place
in the code which took advantage of that distinction.

Comments?  Objections?


        Stefan


In GNU Emacs 29.0.50 (build 1, i686-pc-linux-gnu, GTK+ Version 2.24.33, cairo version 1.16.0)
 of 2022-01-14 built on ceviche
Repository revision: 161657c1e1598b41c82fcc740ec13b539b013191
Repository branch: work
Windowing system distributor 'The X.Org Foundation', version 11.0.12013000
System Description: Debian GNU/Linux bookworm/sid

Configured using:
 'configure -C --enable-checking --with-modules --enable-check-lisp-object-type
 'CFLAGS=-Wall -g3 -Og -Wno-pointer-sign'
 PKG_CONFIG_PATH=/home/monnier/lib/pkgconfig'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: function-history.patch --]
[-- Type: text/patch, Size: 11903 bytes --]

diff --git a/lisp/help-fns.el b/lisp/help-fns.el
index 98a1b11e08..36c7966919 100644
--- a/lisp/help-fns.el
+++ b/lisp/help-fns.el
@@ -33,6 +33,7 @@
 ;;; Code:
 
 (require 'cl-lib)
+(require 'seq)
 (require 'help-mode)
 (require 'radix-tree)
 (eval-when-compile (require 'subr-x))   ;For when-let.
@@ -678,19 +679,9 @@ help-fns--globalized-minor-mode
     (terpri)))
 
 ;; We could use `symbol-file' but this is a wee bit more efficient.
-(defun help-fns--autoloaded-p (function file)
-  "Return non-nil if FUNCTION has previously been autoloaded.
-FILE is the file where FUNCTION was probably defined."
-  (let* ((file (file-name-sans-extension (file-truename file)))
-	 (load-hist load-history)
-	 (target (cons t function))
-	 found)
-    (while (and load-hist (not found))
-      (and (stringp (caar load-hist))
-	   (equal (file-name-sans-extension (caar load-hist)) file)
-	   (setq found (member target (cdar load-hist))))
-      (setq load-hist (cdr load-hist)))
-    found))
+(defun help-fns--autoloaded-p (function)
+  "Return non-nil if FUNCTION has previously been autoloaded."
+  (seq-some #'autoloadp (get function 'function-history)))
 
 (defun help-fns--interactive-only (function)
   "Insert some help blurb if FUNCTION should only be used interactively."
@@ -873,13 +864,13 @@ help-fns-function-description-header
   "Print a line describing FUNCTION to `standard-output'."
   (pcase-let* ((`(,_real-function ,def ,aliased ,real-def)
                 (help-fns--analyze-function function))
-               (file-name (find-lisp-object-file-name function (if aliased 'defun
-                                                                 def)))
+               (file-name (find-lisp-object-file-name
+                           function (if aliased 'defun def)))
                (beg (if (and (or (byte-code-function-p def)
                                  (keymapp def)
                                  (memq (car-safe def) '(macro lambda closure)))
                              (stringp file-name)
-                             (help-fns--autoloaded-p function file-name))
+                             (help-fns--autoloaded-p function))
                         (concat
                          "an autoloaded " (if (commandp def)
                                               "interactive "))
diff --git a/lisp/loadhist.el b/lisp/loadhist.el
index 48058f4053..8f634afaca 100644
--- a/lisp/loadhist.el
+++ b/lisp/loadhist.el
@@ -157,38 +157,30 @@ unload--set-major-mode
           ;; mode, or proposed is not nil and not major-mode, and so we use it.
           (funcall (or proposed 'fundamental-mode)))))))
 
+(defvar loadhist-unload-filename nil)
+
 (cl-defgeneric loadhist-unload-element (x)
-  "Unload an element from the `load-history'."
+  "Unload an element from the `load-history'.
+The variable `loadhist-unload-filename' holds the name of the file we're
+unloading."
   (message "Unexpected element %S in load-history" x))
 
-;; In `load-history', the definition of a previously autoloaded
-;; function is represented by 2 entries: (t . SYMBOL) comes before
-;; (defun . SYMBOL) and says we should restore SYMBOL's autoload when
-;; we undefine it.
-;; So we use this auxiliary variable to keep track of the last (t . SYMBOL)
-;; that occurred.
-(defvar loadhist--restore-autoload nil
-  "If non-nil, is a symbol for which to try to restore a previous autoload.")
-
-(cl-defmethod loadhist-unload-element ((x (head t)))
-  (setq loadhist--restore-autoload (cdr x)))
-
-(defun loadhist--unload-function (x)
-  (let ((fun (cdr x)))
-    (when (fboundp fun)
-      (when (fboundp 'ad-unadvise)
-	(ad-unadvise fun))
-      (let ((aload (get fun 'autoload)))
-	(defalias fun
-          (if (and aload (eq fun loadhist--restore-autoload))
-	      (cons 'autoload aload)
-            nil)))))
-  (setq loadhist--restore-autoload nil))
-
 (cl-defmethod loadhist-unload-element ((x (head defun)))
-  (loadhist--unload-function x))
-(cl-defmethod loadhist-unload-element ((x (head autoload)))
-  (loadhist--unload-function x))
+  (let ((fun (cdr x))
+        (hist (get fun 'function-history)))
+    (cond
+     ((null hist) (defalias fun nil))
+     ((equal (car hist) loadhist-unload-filename)
+      (put fun 'function-history (cddr hist))
+      (defalias fun (cadr hist)))
+     (t
+      ;; Unloading a file whose definition is "inactive" (i.e. has been
+      ;; overridden by another file): just remove it from the history,
+      ;; so future unloading of that other file has a chance to DTRT.
+      (let* ((tmp (plist-member hist loadhist-unload-filename))
+             (pos (- (length hist) (length tmp))))
+        (cl-assert (> pos 1))
+        (setcdr (nthcdr (1- pos) hist) (cdr tmp)))))))
 
 (cl-defmethod loadhist-unload-element ((_ (head require))) nil)
 (cl-defmethod loadhist-unload-element ((_ (head defface))) nil)
@@ -257,6 +249,7 @@ unload-feature
 	       (prin1-to-string dependents) file))))
   (let* ((unload-function-defs-list (feature-symbols feature))
          (file (pop unload-function-defs-list))
+         (loadhist-unload-filename file)
 	 (name (symbol-name feature))
          (unload-hook (intern-soft (concat name "-unload-hook")))
 	 (unload-func (intern-soft (concat name "-unload-function"))))
diff --git a/src/data.c b/src/data.c
index a5a76a2755..8cd13f3a77 100644
--- a/src/data.c
+++ b/src/data.c
@@ -859,6 +859,43 @@ DEFUN ("fset", Ffset, Sfset, 2, 2, 0,
   return definition;
 }
 
+static void
+add_to_function_history (Lisp_Object symbol, Lisp_Object olddef)
+{
+  eassert (!NILP (olddef));
+
+  Lisp_Object past = Fget (symbol, Qfunction_history);
+  Lisp_Object file = Qnil;
+  /* FIXME: Sadly, `Vload_file_name` gives less precise information
+     (it's sometimes non-nil when it shoujld be nil).  */
+  Lisp_Object tail = Vcurrent_load_list;
+  FOR_EACH_TAIL_SAFE (tail)
+    if (NILP (XCDR (tail)) && STRINGP (XCAR (tail)))
+      file = XCAR (tail);
+
+  Lisp_Object tem = Fplist_member (past, file);
+  if (!NILP (tem))
+    { /* New def from a file used before.
+         Overwrite the previous record associated with this file.  */
+      if (EQ (tem, past))
+        /* The new def is from the same file as the last change, so
+           there's nothing to do: unloading the file should revert to
+           the status before the last change rather than before this load.  */
+        return;
+      Lisp_Object pastlen = Flength (past);
+      Lisp_Object temlen = Flength (tem);
+      EMACS_INT tempos = XFIXNUM (pastlen) - XFIXNUM (temlen);
+      eassert (tempos > 1);
+      Lisp_Object prev = Fnthcdr (make_fixnum (tempos - 2), past);
+      /* Remove the previous info for this file.
+         E.g. change `hist` from (... OTHERFILE DEF3 THISFILE DEF2 ...)
+         to (... OTHERFILE DEF2). */
+      XSETCDR (prev, XCDR (tem));
+    }
+  /* Push new def from new file.  */
+  Fput (symbol, Qfunction_history, Fcons (file, Fcons (olddef, past)));
+}
+
 void
 defalias (Lisp_Object symbol, Lisp_Object definition)
 {
@@ -867,28 +904,24 @@ defalias (Lisp_Object symbol, Lisp_Object definition)
     if (!will_dump_p () || !autoload)
       { /* Only add autoload entries after dumping, because the ones before are
 	   not useful and else we get loads of them from the loaddefs.el.  */
-        Lisp_Object function = XSYMBOL (symbol)->u.s.function;
-
-	if (AUTOLOADP (function))
-	  /* Remember that the function was already an autoload.  */
-	  LOADHIST_ATTACH (Fcons (Qt, symbol));
-	LOADHIST_ATTACH (Fcons (autoload ? Qautoload : Qdefun, symbol));
-
-        if (!NILP (Vautoload_queue) && !NILP (function))
-          Vautoload_queue = Fcons (Fcons (symbol, function), Vautoload_queue);
-
-        if (AUTOLOADP (function))
-          Fput (symbol, Qautoload, XCDR (function));
+	LOADHIST_ATTACH (Fcons (Qdefun, symbol));
       }
   }
 
-  { /* Handle automatic advice activation.  */
-    Lisp_Object hook = Fget (symbol, Qdefalias_fset_function);
-    if (!NILP (hook))
-      call2 (hook, symbol, definition);
-    else
-      Ffset (symbol, definition);
-  }
+  Lisp_Object olddef = XSYMBOL (symbol)->u.s.function;
+  if (!NILP (olddef))
+    {
+      if (!NILP (Vautoload_queue))
+        Vautoload_queue = Fcons (symbol, Vautoload_queue);
+      add_to_function_history (symbol, olddef);
+    }
+
+  /* Handle automatic advice activation.  */
+  Lisp_Object hook = Fget (symbol, Qdefalias_fset_function);
+  if (!NILP (hook))
+    call2 (hook, symbol, definition);
+  else
+    Ffset (symbol, definition);
 }
 
 DEFUN ("defalias", Fdefalias, Sdefalias, 2, 3, 0,
@@ -4171,6 +4204,7 @@ #define PUT_ERROR(sym, tail, msg)			\
 
   DEFSYM (Qinteractive_form, "interactive-form");
   DEFSYM (Qdefalias_fset_function, "defalias-fset-function");
+  DEFSYM (Qfunction_history, "function-history");
 
   DEFSYM (Qbyte_code_function_p, "byte-code-function-p");
 
diff --git a/src/eval.c b/src/eval.c
index b083a00a79..cf38e76718 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -2236,35 +2236,40 @@ DEFUN ("autoload", Fautoload, Sautoload, 2, 5, 0,
       && !AUTOLOADP (XSYMBOL (function)->u.s.function))
     return Qnil;
 
+  ptrdiff_t count = SPECPDL_INDEX ();
+  if (will_dump_p)
+    /* Only add autoload entries after dumping, because the ones before are
+	   not useful and else we get loads of them from the loaddefs.el.  */
+    specbind (Qcurrent_load_list, Qnil);
+
   if (!NILP (Vpurify_flag) && EQ (docstring, make_fixnum (0)))
     /* `read1' in lread.c has found the docstring starting with "\
        and assumed the docstring will be provided by Snarf-documentation, so it
        passed us 0 instead.  But that leads to accidental sharing in purecopy's
        hash-consing, so we use a (hopefully) unique integer instead.  */
     docstring = make_ufixnum (XHASH (function));
-  return Fdefalias (function,
-		    list5 (Qautoload, file, docstring, interactive, type),
-		    Qnil);
+  Lisp_Object tem
+    = Fdefalias (function,
+		 list5 (Qautoload, file, docstring, interactive, type),
+		 Qnil);
+  unbind_to (count, Qnil);
+  return tem;
 }
 
 static void
 un_autoload (Lisp_Object oldqueue)
 {
-  Lisp_Object queue, first, second;
-
   /* Queue to unwind is current value of Vautoload_queue.
      oldqueue is the shadowed value to leave in Vautoload_queue.  */
-  queue = Vautoload_queue;
+  Lisp_Object queue = Vautoload_queue;
   Vautoload_queue = oldqueue;
   while (CONSP (queue))
     {
-      first = XCAR (queue);
-      second = Fcdr (first);
-      first = Fcar (first);
-      if (EQ (first, make_fixnum (0)))
-	Vfeatures = second;
+      Lisp_Object first = XCAR (queue);
+      if (CONSP (first) && EQ (XCAR (first), make_fixnum (0)))
+	Vfeatures = XCDR (first);
       else
-	Ffset (first, second);
+	Ffset (first, Fcar (Fcdr (Fget (first, Qfunction_history))));
       queue = XCDR (queue);
     }
 }
diff --git a/src/lread.c b/src/lread.c
index 9910db27de..ada137ff19 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -5240,12 +5248,9 @@ syms_of_lread (void)
 The remaining ENTRIES in the alist element describe the functions and
 variables defined in that file, the features provided, and the
 features required.  Each entry has the form `(provide . FEATURE)',
-`(require . FEATURE)', `(defun . FUNCTION)', `(autoload . SYMBOL)',
-`(defface . SYMBOL)', `(define-type . SYMBOL)',
-`(cl-defmethod METHOD SPECIALIZERS)', or `(t . SYMBOL)'.
-Entries like `(t . SYMBOL)' may precede a `(defun . FUNCTION)' entry,
-and mean that SYMBOL was an autoload before this file redefined it
-as a function.  In addition, entries may also be single symbols,
+`(require . FEATURE)', `(defun . FUNCTION)', `(defface . SYMBOL)',
+ `(define-type . SYMBOL)', or `(cl-defmethod METHOD SPECIALIZERS)'.
+In addition, entries may also be single symbols,
 which means that symbol was defined by `defvar' or `defconst'.
 
 During preloading, the file name recorded is relative to the main Lisp

bug#53632: Function definition history

[Eli Zaretskii]

> Date: Sun, 30 Jan 2022 00:07:57 -0500
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> I suggest to replace that info with something slightly more complete.
> In the patch below I store the history of the function definition of
> a symbol in its `function-history` symbol property.  This history is
> stored as a list of the form (... VAL(n+1) FILE(n+1) VALn FILEn ...)
> where VALn is the value set by FILEn.  To make this list cheap in the
> default case, the latest value is not stored in the list (since it's in
> the `symbol-function`) and neither is the first file.  So if there's
> only been a single definition (the most common case), the list is empty
> and the property is just not present at all.  If a function was first
> defined as an autoload and then overriden by the actual function
> definition, then the list will hold (FILE2 AUTOLOAD), i.e. the name of
> the file that provided the actual function definition and the autoload
> that was used before that.
> [ Note: the name of the file that provided the first definition can be
>   recovered if really needed by checking all entries in `load-history`.
>   In the patch below I have not needed it.  ]
> 
> This makes it possible to handle correctly things like unloading
> `cl-loaddefs.el` which should remove the autoloads that are still
> autoloads and leave untouched the functions whose autoload have been
> replaced by the actual function definition.
> 
> In my tests it increased the size of the .pdmp by about 2KB (on a 32bit
> build).
> 
> The patch also gets rid of the `autoload` vs `defun` distinction in
> `load-history` which seems unnecessary (a significant part of the
> motivation for this patch was to get rid of the special handling of
> autoloads in this part of the code).  At least I couldn't find any place
> in the code which took advantage of that distinction.
> 
> Comments?  Objections?

Please make sure to document this in the ELisp manual.

bug#53632: Function definition history

[Lars Ingebrigtsen]

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

> I suggest to replace that info with something slightly more complete.
> In the patch below I store the history of the function definition of
> a symbol in its `function-history` symbol property.  This history is
> stored as a list of the form (... VAL(n+1) FILE(n+1) VALn FILEn ...)
> where VALn is the value set by FILEn.

I like it.

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

bug#53632: Function definition history

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Please make sure to document this in the ELisp manual.

Done, thanks, pushed,


        Stefan

bug#53632: Function definition history

[Glenn Morris]

Since 1d1b664fb, tramp-test47-unload fails.
Ref eg https://hydra.nixos.org/build/166143250
  `tramp-unload-file-name-handlers' still bound

bug#53632: Function definition history

[Michael Albinus]

Glenn Morris <rgm@gnu.org> writes:

Hi,

> Since 1d1b664fb, tramp-test47-unload fails.
> Ref eg https://hydra.nixos.org/build/166143250
>   `tramp-unload-file-name-handlers' still bound

This is due to a changed behavior of `unload-feature'. Prior this patch,
there is

--8<---------------cut here---------------start------------->8---
# emacs -Q -l tramp
(unload-feature 'tramp 'force)
(mapatoms
 (lambda (x)
   (and (functionp x)
	(string-match-p "^tramp" (symbol-name x))
        (message "%s" x))))
=>
tramp-register-archive-file-name-handler
tramp-archive-autoload-file-name-handler
--8<---------------cut here---------------end--------------->8---

This is OK, because tramp-archive.el is not unloaded via unloading
tramp.el.

Now, this behavior has changed. With the same recipe, we get

--8<---------------cut here---------------start------------->8---
=>
tramp-unload-file-name-handlers
tramp-unload-tramp
tramp-register-archive-file-name-handler
tramp-archive-autoload-file-name-handler
tramp-register-autoload-file-name-handlers
tramp-autoload-file-name-handler
--8<---------------cut here---------------end--------------->8---

Best regards, Michael.

bug#53632: Function definition history

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Now, this behavior has changed. With the same recipe, we get
>
> --8<---------------cut here---------------start------------->8---
> =>
> tramp-unload-file-name-handlers
> tramp-unload-tramp
> tramp-register-archive-file-name-handler
> tramp-archive-autoload-file-name-handler
> tramp-register-autoload-file-name-handlers
> tramp-autoload-file-name-handler
> --8<---------------cut here---------------end--------------->8---

Indeed, the behavior is changed, but AFAICT it's "better" in that it
gives us a state closer to the one we had before `tramp.el` was loaded:
the above functions are predefined in `loaddefs.el`
via `;;;###autoload` cookies, so it's normal that they're defined when
`tramp` is not loaded.

So I think the problem is in the test rather than in the unload code.
The patch below fixes the test for me.

BTW, I notice that the test uses `functionp` so it doesn't pay attention
to whether macros are properly unloaded (I noticed because I thought it
was odd that the above list didn't include
`tramp-archive-autoload-file-name-regexp` which is similarly predefined
in `loaddefs.el`).


        Stefan


diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
index b41824a6cf3..1f9eea4c5f7 100644
--- a/test/lisp/net/tramp-tests.el
+++ b/test/lisp/net/tramp-tests.el
@@ -202,8 +202,8 @@ tramp--test-instrument-test-case
 	  (trace-buffer (tramp-trace-buffer-name tramp-test-vec))
 	  (debug-ignored-errors
 	   (append
-	    '("^make-symbolic-link not supported$"
-	      "^error with add-name-to-file")
+	    '("\\`make-symbolic-link not supported\\'"
+	      "\\`error with add-name-to-file")
 	    debug-ignored-errors))
 	  inhibit-message)
      (unwind-protect
@@ -326,7 +326,7 @@ tramp-test01-file-name-syntax
 	  (let (tramp-mode)
 	    (should-not (tramp-tramp-file-p "/method:user@host:")))
 	  ;; `tramp-ignored-file-name-regexp' suppresses Tramp.
-	  (let ((tramp-ignored-file-name-regexp "^/method:user@host:"))
+	  (let ((tramp-ignored-file-name-regexp "\\`/method:user@host:"))
 	    (should-not (tramp-tramp-file-p "/method:user@host:")))
 	  ;; Methods shall be at least two characters on MS Windows,
 	  ;; except the default method.
@@ -3664,7 +3664,7 @@ tramp--test-ignore-add-name-to-file-error
   `(condition-case err
        (progn ,@body)
      (file-error
-      (unless (string-match-p "^error with add-name-to-file"
+      (unless (string-match-p "\\`error with add-name-to-file"
 			      (error-message-string err))
 	(signal (car err) (cdr err))))))
 
@@ -6070,7 +6070,7 @@ tramp-test39-detect-external-change
 		      (when create-lockfiles
 			(should (string-match-p
 				 (format
-				  "^%s changed on disk; really edit the buffer\\?"
+				  "\\`%s changed on disk; really edit the buffer\\?"
 				  (if (tramp--test-crypt-p)
 				      ".+" (file-name-nondirectory tmp-name)))
 				 captured-messages))
@@ -6183,7 +6183,7 @@ tramp--test-ftp-p
 This does not support globbing characters in file names (yet)."
   ;; Globbing characters are ??, ?* and ?\[.
   (string-match-p
-   "ftp$" (file-remote-p tramp-test-temporary-file-directory 'method)))
+   "ftp\\'" (file-remote-p tramp-test-temporary-file-directory 'method)))
 
 (defun tramp--test-fuse-p ()
   "Check, whether an FUSE file system isused."
@@ -6215,7 +6215,7 @@ tramp--test-ksh-p
   ;; We must refill the cache.  `file-truename' does it.
   (file-truename tramp-test-temporary-file-directory)
   (string-match-p
-   "ksh$" (tramp-get-connection-property tramp-test-vec "remote-shell" "")))
+   "ksh\\'" (tramp-get-connection-property tramp-test-vec "remote-shell" "")))
 
 (defun tramp--test-macos-p ()
   "Check, whether the remote host runs macOS."
@@ -6263,7 +6263,7 @@ tramp--test-share-p
   "Check, whether the method needs a share."
   (and (tramp--test-gvfs-p)
        (string-match-p
-	"^\\(afp\\|davs?\\|smb\\)$"
+	"\\`\\(afp\\|davs?\\|smb\\)\\'"
 	(file-remote-p tramp-test-temporary-file-directory 'method))))
 
 (defun tramp--test-sshfs-p ()
@@ -7206,11 +7206,20 @@ tramp-test47-unload
    (lambda (x)
      (and (or (and (boundp x) (null (local-variable-if-set-p x)))
 	      (and (functionp x) (null (autoloadp (symbol-function x)))))
-	  (string-match-p "^tramp" (symbol-name x))
+	  (string-match-p "\\`tramp" (symbol-name x))
 	  ;; `tramp-completion-mode' is autoloaded in Emacs < 28.1.
 	  (not (eq 'tramp-completion-mode x))
-	  (not (string-match-p "^tramp\\(-archive\\)?--?test" (symbol-name x)))
-	  (not (string-match-p "unload-hook$" (symbol-name x)))
+	  ;; Some functions aren't autoloads but they are similarly
+	  ;; predefined before `Tramp' is loaded (bug#53632):
+	  (not (memq x '(tramp-unload-file-name-handlers
+	                 tramp-unload-tramp
+	                 tramp-register-archive-file-name-handler
+	                 tramp-archive-autoload-file-name-handler
+	                 tramp-register-autoload-file-name-handlers
+	                 tramp-autoload-file-name-handler)))
+	  (not (string-match-p "\\`tramp\\(-archive\\)?--?test"
+	                       (symbol-name x)))
+	  (not (string-match-p "unload-hook\\'" (symbol-name x)))
 	  (ert-fail (format "`%s' still bound" x)))))
   ;; The defstruct `tramp-file-name' and all its internal functions
   ;; shall be purged.
@@ -7225,8 +7234,8 @@ tramp-test47-unload
   (mapatoms
    (lambda (x)
      (and (boundp x)
-	  (string-match-p "-\\(hook\\|function\\)s?$" (symbol-name x))
-	  (not (string-match-p "unload-hook$" (symbol-name x)))
+	  (string-match-p "-\\(hook\\|function\\)s?\\'" (symbol-name x))
+	  (not (string-match-p "unload-hook\\'" (symbol-name x)))
 	  (consp (symbol-value x))
 	  (ignore-errors (all-completions "tramp" (symbol-value x)))
 	  (ert-fail (format "Hook `%s' still contains Tramp function" x))))))
@@ -7237,7 +7246,7 @@ tramp-test-all
   (interactive "p")
   (funcall
    (if interactive #'ert-run-tests-interactively #'ert-run-tests-batch)
-   "^tramp"))
+   "\\`tramp"))
 
 ;; TODO:
 

bug#53632: Function definition history

[Michael Albinus]

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

Hi Stefan,

> Indeed, the behavior is changed, but AFAICT it's "better" in that it
> gives us a state closer to the one we had before `tramp.el` was loaded:
> the above functions are predefined in `loaddefs.el`
> via `;;;###autoload` cookies, so it's normal that they're defined when
> `tramp` is not loaded.

OK, so be it.

> So I think the problem is in the test rather than in the unload code.
> The patch below fixes the test for me.

The patch essential is to list those functions, and to make an
exception. Will work, yes. However, I fear that this is error-prone:
Whenever there is another such function with an autoload cookie in the
future, the test will fail, again.

Perhaps we shall give these functions a symbol property, say
`tramp-autoload', and test for that property in order to filter out?
This should be more robust.

> BTW, I notice that the test uses `functionp` so it doesn't pay attention
> to whether macros are properly unloaded (I noticed because I thought it
> was odd that the above list didn't include
> `tramp-archive-autoload-file-name-regexp` which is similarly predefined
> in `loaddefs.el`).

Good point, we shall test also for `macrop'. Will extend the test.

>         Stefan

Best regards, Michael.

bug#53632: Function definition history

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> So I think the problem is in the test rather than in the unload code.
>> The patch below fixes the test for me.
>
> The patch essential is to list those functions, and to make an
> exception. Will work, yes. However, I fear that this is error-prone:
> Whenever there is another such function with an autoload cookie in the
> future, the test will fail, again.
>
> Perhaps we shall give these functions a symbol property, say
> `tramp-autoload', and test for that property in order to filter out?
> This should be more robust.

Maybe a simpler option is to give them a recognizable name
(e.g. including "preloaded" or "AL" or somesuch in their name)?

In any case, those parts of the test47 are actually testing
loadhist.el rather than Tramp, so I'd recommend you don't worry too much
about those functions.

I'd focus instead on the parts that verify proper functioning of the code
you put in `tramp-unload-hook`.


        Stefan


PS: BTW, why is `tramp-unload-file-name-handlers` preloaded?
It doesn't seem like it can be meaningfully used before Tramp is loaded.

bug#53632: Function definition history

[Michael Albinus]

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

Hi Stefan,

> Maybe a simpler option is to give them a recognizable name
> (e.g. including "preloaded" or "AL" or somesuch in their name)?
>
> In any case, those parts of the test47 are actually testing
> loadhist.el rather than Tramp, so I'd recommend you don't worry too much
> about those functions.
>
> I'd focus instead on the parts that verify proper functioning of the code
> you put in `tramp-unload-hook`.

Will see. In fact, it tests already the functionality of the
unload-hooks, like cleaning up other hooks, and alike.

> PS: BTW, why is `tramp-unload-file-name-handlers` preloaded?
> It doesn't seem like it can be meaningfully used before Tramp is loaded.

The name is a little bit misleading, it is rather a
`tramp-deactivate-file-name-handlers' functionality.

It is autoloaded, because it is used in `tramp-autoload-file-name-handler',
which is also autoloaded.

>         Stefan

Best regards, Michael.

bug#53632: Function definition history

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Michael Albinus [2022-02-04 18:43:44] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> Maybe a simpler option is to give them a recognizable name
>> (e.g. including "preloaded" or "AL" or somesuch in their name)?
>>
>> In any case, those parts of the test47 are actually testing
>> loadhist.el rather than Tramp, so I'd recommend you don't worry too much
>> about those functions.
>>
>> I'd focus instead on the parts that verify proper functioning of the code
>> you put in `tramp-unload-hook`.
>
> Will see. In fact, it tests already the functionality of the
> unload-hooks, like cleaning up other hooks, and alike.

Indeed it already does, and I just meant to clarify that I didn't mean
to throw out the whole test47.

>> PS: BTW, why is `tramp-unload-file-name-handlers` preloaded?
>> It doesn't seem like it can be meaningfully used before Tramp is loaded.
>
> The name is a little bit misleading, it is rather a
> `tramp-deactivate-file-name-handlers' functionality.
>
> It is autoloaded, because it is used in `tramp-autoload-file-name-handler',
> which is also autoloaded.

I guess the part I don't understand is why we need it there.
It's called right before loading `tramp.el` but `tramp.el` will
re-execute it anyway when it runs `tramp-register-file-name-handlers`,
so it seems redundant.


        Stefan

bug#53632: Function definition history

[Michael Albinus]

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

Hi Stefan,

>>> Maybe a simpler option is to give them a recognizable name
>>> (e.g. including "preloaded" or "AL" or somesuch in their name)?
>>>
>>> In any case, those parts of the test47 are actually testing
>>> loadhist.el rather than Tramp, so I'd recommend you don't worry too much
>>> about those functions.
>>>
>>> I'd focus instead on the parts that verify proper functioning of the code
>>> you put in `tramp-unload-hook`.
>>
>> Will see. In fact, it tests already the functionality of the
>> unload-hooks, like cleaning up other hooks, and alike.
>
> Indeed it already does, and I just meant to clarify that I didn't mean
> to throw out the whole test47.

Yep. I've fixed this now in master with commit 3a8e140ad1. I've decided
against renaming of the functions, because this would result in a
backward compatibility nightmare.

>>> PS: BTW, why is `tramp-unload-file-name-handlers` preloaded?
>>> It doesn't seem like it can be meaningfully used before Tramp is loaded.
>>
>> The name is a little bit misleading, it is rather a
>> `tramp-deactivate-file-name-handlers' functionality.
>>
>> It is autoloaded, because it is used in `tramp-autoload-file-name-handler',
>> which is also autoloaded.
>
> I guess the part I don't understand is why we need it there.
> It's called right before loading `tramp.el` but `tramp.el` will
> re-execute it anyway when it runs `tramp-register-file-name-handlers`,
> so it seems redundant.

Well, I don't remember all the details. But there are subtle problems
when you change the regexp used in the file name handler alist, so I
finally came with this approach. Really, I don't want to reopen this can
of worms, I'm happy it works as it is.

>         Stefan

Best regards, Michael.

bug#53632: Function definition history

[Glenn Morris]

Michael Albinus wrote:

> Yep. I've fixed this now in master with commit 3a8e140ad1.

tramp-test47-unload continues to fail as of 9e420cd893?

bug#53632: Function definition history

[Michael Albinus]

Glenn Morris <rgm@gnu.org> writes:

Hi Glenn,

>> Yep. I've fixed this now in master with commit 3a8e140ad1.
>
> tramp-test47-unload continues to fail as of 9e420cd893?

Oops, commit 3a8e140ad1 contains only tramp.el and
tramp-archive.el. I've forgotten to commit tramp-tests.el. Done now with
commit ddc734432b.

Sorry for the trouble. Best regards, Michael.