bug#65017: 29.1; Byte compiler interaction with cl-lib function objects, removes symbol-function

[Alan Mackenzie <acm@muc.de>]

Hello, Stefan and Mattias.

On Sat, Aug 05, 2023 at 18:53:48 -0400, Stefan Monnier wrote:
> >> I don't know why `symbols-with-pos-enabled` is non-nil at that point (I
> >> thought we only enabled it wile byte-compiling), ....

> > This is not quite the case.  symbols-with-pos-enabled gets erroneously
> > bound to t in internal-macroexpand-for-load (emacs-lisp/macroexp.el).

> Aha!

> > diff --git a/lisp/emacs-lisp/macroexp.el b/lisp/emacs-lisp/macroexp.el
> > index b05aba3e1a7..ea838f5b7b2 100644
> > --- a/lisp/emacs-lisp/macroexp.el
> > +++ b/lisp/emacs-lisp/macroexp.el
> > @@ -799,8 +799,7 @@ macroexp--debug-eager
 
> >  (defun internal-macroexpand-for-load (form full-p)
> >    ;; Called from the eager-macroexpansion in readevalloop.
> > -  (let ((symbols-with-pos-enabled t)
> > -        (print-symbols-bare t))
> > +  (let ((print-symbols-bare t))
> >      (cond
> >       ;; Don't repeat the same warning for every top-level element.
> >       ((eq 'skip (car macroexp--pending-eager-loads)) form)

> Looks good to me.  AFAICT this binding was added at some point where it
> seemed like a good idea but we later figured better places to do it,
> and we just didn't remove it because it seemed "harmless" (or because
> we just didn't think of it).

There is another unneeded binding of symbols-with-pos-enabled in
macroexp.el, and several redundant bindings of print-symbols-bare in
bytecomp.el (which are commented as such), and one of print-symbols-bare
in macroexp.el.  The patch below removes these bindings.  make bootstrap
and make check still work OK, with it.  A compile-defun in a function
with an error produces the correct error message.

I suggest installing this patch into master.

> > Stefan, it would still be nice for cl--labels-convert-cache to get
> > initialised each time it gets used.

> No, the problem is not initialization, as I pointed out.  The problem is
> that this `eq` should not consider a symbol equal to a sympos *ever*
> (contrary to most other uses of `eq` in macros).

Are you sure?  Why not?  If cl--labels-convert-cache is being used
inside the byte compiler, it surely needs to consider #<symbol foo at
42> and #<symbol foo at 60> as eq?  There is no mechanism to make these
two SWPs eq whilst excluding their eq with the bare symbol.



diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index ac040799a22..b9d1948e555 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -489,8 +489,7 @@ byte-compile-recurse-toplevel
   ;; 3.2.3.1, "Processing of Top Level Forms".  The semantics are very
   ;; subtle: see test/lisp/emacs-lisp/bytecomp-tests.el for interesting
   ;; cases.
-  (let ((print-symbols-bare t))         ; Possibly redundant binding.
-    (setf form (macroexp-macroexpand form byte-compile-macro-environment)))
+  (setf form (macroexp-macroexpand form byte-compile-macro-environment))
   (if (eq (car-safe form) 'progn)
       (cons (car form)
             (mapcar (lambda (subform)
@@ -568,11 +567,10 @@ byte-compile-initial-macro-environment
                               ;; Don't compile here, since we don't know
                               ;; whether to compile as byte-compile-form
                               ;; or byte-compile-file-form.
-                              (let* ((print-symbols-bare t) ; Possibly redundant binding.
-                                     (expanded
-                                      (macroexpand--all-toplevel
-                                       form
-                                       macroexpand-all-environment)))
+                              (let ((expanded
+                                     (macroexpand--all-toplevel
+                                      form
+                                      macroexpand-all-environment)))
                                 (eval (byte-run-strip-symbol-positions
                                        (bytecomp--copy-tree expanded))
                                       lexical-binding)
@@ -2489,8 +2487,7 @@ byte-compile-output-file-form
     ;; Spill output for the native compiler here
     (push (make-byte-to-native-top-level :form form :lexical lexical-binding)
           byte-to-native-top-level-forms))
-  (let ((print-symbols-bare t)          ; Possibly redundant binding.
-        (print-escape-newlines t)
+  (let ((print-escape-newlines t)
         (print-length nil)
         (print-level nil)
         (print-quoted t)
@@ -2524,8 +2521,7 @@ byte-compile-output-docform
   ;; in the input buffer (now current), not in the output buffer.
   (let ((dynamic-docstrings byte-compile-dynamic-docstrings))
     (with-current-buffer byte-compile--outbuffer
-      (let (position
-            (print-symbols-bare t))     ; Possibly redundant binding.
+      (let (position)
         ;; Insert the doc string, and make it a comment with #@LENGTH.
         (when (and (>= (nth 1 info) 0) dynamic-docstrings)
           (setq position (byte-compile-output-as-comment
@@ -2621,8 +2617,7 @@ byte-compile-flush-pending
               byte-compile-jump-tables nil))))
 
 (defun byte-compile-preprocess (form &optional _for-effect)
-  (let ((print-symbols-bare t))         ; Possibly redundant binding.
-    (setq form (macroexpand-all form byte-compile-macro-environment)))
+  (setq form (macroexpand-all form byte-compile-macro-environment))
   ;; FIXME: We should run byte-optimize-form here, but it currently does not
   ;; recurse through all the code, so we'd have to fix this first.
   ;; Maybe a good fix would be to merge byte-optimize-form into
diff --git a/lisp/emacs-lisp/macroexp.el b/lisp/emacs-lisp/macroexp.el
index b05aba3e1a7..47d663b5d4a 100644
--- a/lisp/emacs-lisp/macroexp.el
+++ b/lisp/emacs-lisp/macroexp.el
@@ -107,8 +107,7 @@ macroexp--all-clauses
 
 (defun macroexp--compiler-macro (handler form)
   (condition-case-unless-debug err
-      (let ((symbols-with-pos-enabled t))
-        (apply handler form (cdr form)))
+      (apply handler form (cdr form))
     (error
      (message "Warning: Optimization failure for %S: Handler: %S\n%S"
               (car form) handler err)
@@ -799,40 +798,38 @@ macroexp--debug-eager
 
 (defun internal-macroexpand-for-load (form full-p)
   ;; Called from the eager-macroexpansion in readevalloop.
-  (let ((symbols-with-pos-enabled t)
-        (print-symbols-bare t))
-    (cond
-     ;; Don't repeat the same warning for every top-level element.
-     ((eq 'skip (car macroexp--pending-eager-loads)) form)
-     ;; If we detect a cycle, skip macro-expansion for now, and output a warning
-     ;; with a trimmed backtrace.
-     ((and load-file-name (member load-file-name macroexp--pending-eager-loads))
-      (let* ((bt (delq nil
-                       (mapcar #'macroexp--trim-backtrace-frame
-                               (macroexp--backtrace))))
-             (elem `(load ,(file-name-nondirectory load-file-name)))
-             (tail (member elem (cdr (member elem bt)))))
-        (if tail (setcdr tail (list '…)))
-        (if (eq (car-safe (car bt)) 'macroexpand-all) (setq bt (cdr bt)))
-        (if macroexp--debug-eager
-            (debug 'eager-macroexp-cycle)
-          (error "Eager macro-expansion skipped due to cycle:\n  %s"
-                 (mapconcat #'prin1-to-string (nreverse bt) " => ")))
-        (push 'skip macroexp--pending-eager-loads)
-        form))
-     (t
-      (condition-case err
-          (let ((macroexp--pending-eager-loads
-                 (cons load-file-name macroexp--pending-eager-loads)))
-            (if full-p
-                (macroexpand--all-toplevel form)
-              (macroexpand form)))
-        (error
-         ;; Hopefully this shouldn't happen thanks to the cycle detection,
-         ;; but in case it does happen, let's catch the error and give the
-         ;; code a chance to macro-expand later.
-         (error "Eager macro-expansion failure: %S" err)
-         form))))))
+  (cond
+   ;; Don't repeat the same warning for every top-level element.
+   ((eq 'skip (car macroexp--pending-eager-loads)) form)
+   ;; If we detect a cycle, skip macro-expansion for now, and output a warning
+   ;; with a trimmed backtrace.
+   ((and load-file-name (member load-file-name macroexp--pending-eager-loads))
+    (let* ((bt (delq nil
+                     (mapcar #'macroexp--trim-backtrace-frame
+                             (macroexp--backtrace))))
+           (elem `(load ,(file-name-nondirectory load-file-name)))
+           (tail (member elem (cdr (member elem bt)))))
+      (if tail (setcdr tail (list '…)))
+      (if (eq (car-safe (car bt)) 'macroexpand-all) (setq bt (cdr bt)))
+      (if macroexp--debug-eager
+          (debug 'eager-macroexp-cycle)
+        (error "Eager macro-expansion skipped due to cycle:\n  %s"
+               (mapconcat #'prin1-to-string (nreverse bt) " => ")))
+      (push 'skip macroexp--pending-eager-loads)
+      form))
+   (t
+    (condition-case err
+        (let ((macroexp--pending-eager-loads
+               (cons load-file-name macroexp--pending-eager-loads)))
+          (if full-p
+              (macroexpand--all-toplevel form)
+            (macroexpand form)))
+      (error
+       ;; Hopefully this shouldn't happen thanks to the cycle detection,
+       ;; but in case it does happen, let's catch the error and give the
+       ;; code a chance to macro-expand later.
+       (error "Eager macro-expansion failure: %S" err)
+       form)))))
 
 ;; ¡¡¡ Big Ugly Hack !!!
 ;; src/bootstrap-emacs is mostly used to compile .el files, so it needs


>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).