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

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

[Eric Marsden]

The byte-compiler seems to erroneously remove the symbol-function for 
equal in the
code show below.

--- file "perturb.el" ---
(require 'cl-lib)

(defun foo ()
   (cl-flet ((bar (v) (list v)))
     (make-hash-table :test #'equal)))
---


--- file "use.el" ---
(require 'cl-lib)
(require 'ert)

(defun test ()
   (cl-flet ((foo (x) (list x)))
     (should (equal nil 42))))
---

% emacs -Q --batch --eval '(byte-compile-file "perturb.el")' -l use.el 
-f test
Error: invalid-function (#<symbol equal at 95>)
   mapbacktrace(#f(compiled-function (evald func args flags) #<bytecode 
-0x84e95e6e2517821>))
   debug-early-backtrace()
   debug-early(error (invalid-function #<symbol equal at 95>))
   #<symbol equal at 95>(nil 42)
   apply(#<symbol equal at 95> (nil 42))
   (setq value-2 (apply fn-0 args-1))
   (unwind-protect (setq value-2 (apply fn-0 args-1)) (setq 
form-description-4 (nconc (list '(should (equal nil 42))) (list :form 
(cons fn-0 args-1)) (if (eql value-2 'ert-form-evaluation-aborted-3) nil 
(list :value value-2)) (if (eql value-2 'ert-form-evaluation-aborted-3) 
nil (let* ((-explainer- (and t (ert--get-explainer 'equal)))) (if 
-explainer- (list :explanation (apply -explainer- args-1)) nil))))) 
(ert--signal-should-execution form-description-4))
   (if (unwind-protect (setq value-2 (apply fn-0 args-1)) (setq 
form-description-4 (nconc (list '(should (equal nil 42))) (list :form 
(cons fn-0 args-1)) (if (eql value-2 'ert-form-evaluation-aborted-3) nil 
(list :value value-2)) (if (eql value-2 'ert-form-evaluation-aborted-3) 
nil (let* ((-explainer- (and t (ert--get-explainer 'equal)))) (if 
-explainer- (list :explanation (apply -explainer- args-1)) nil))))) 
(ert--signal-should-execution form-description-4)) nil (ert-fail 
form-description-4))
   (let (form-description-4) (if (unwind-protect (setq value-2 (apply 
fn-0 args-1)) (setq form-description-4 (nconc (list '(should (equal nil 
42))) (list :form (cons fn-0 args-1)) (if (eql value-2 
'ert-form-evaluation-aborted-3) nil (list :value value-2)) (if (eql 
value-2 'ert-form-evaluation-aborted-3) nil (let* ((-explainer- (and t 
(ert--get-explainer 'equal)))) (if -explainer- (list :explanation (apply 
-explainer- args-1)) nil))))) (ert--signal-should-execution 
form-description-4)) nil (ert-fail form-description-4)))
   (let ((value-2 'ert-form-evaluation-aborted-3)) (let 
(form-description-4) (if (unwind-protect (setq value-2 (apply fn-0 
args-1)) (setq form-description-4 (nconc (list '(should (equal nil 42))) 
(list :form (cons fn-0 args-1)) (if (eql value-2 
'ert-form-evaluation-aborted-3) nil (list :value value-2)) (if (eql 
value-2 'ert-form-evaluation-aborted-3) nil (let* ((-explainer- (and t 
(ert--get-explainer 'equal)))) (if -explainer- (list :explanation (apply 
-explainer- args-1)) nil))))) (ert--signal-should-execution 
form-description-4)) nil (ert-fail form-description-4))) value-2)
   (let* ((fn-0 #'#<symbol equal at 95>) (args-1 (condition-case err 
(let ((signal-hook-function #'ert--should-signal-hook)) (list nil 42)) 
(error (progn (setq fn-0 #'signal) (list (car err) (cdr err))))))) (let 
((value-2 'ert-form-evaluation-aborted-3)) (let (form-description-4) (if 
(unwind-protect (setq value-2 (apply fn-0 args-1)) (setq 
form-description-4 (nconc (list '(should (equal nil 42))) (list :form 
(cons fn-0 args-1)) (if (eql value-2 'ert-form-evaluation-aborted-3) nil 
(list :value value-2)) (if (eql value-2 'ert-form-evaluation-aborted-3) 
nil (let* ((-explainer- (and t (ert--get-explainer 'equal)))) (if 
-explainer- (list :explanation (apply -explainer- args-1)) nil))))) 
(ert--signal-should-execution form-description-4)) nil (ert-fail 
form-description-4))) value-2))
   (progn (let* ((fn-0 #'#<symbol equal at 95>) (args-1 (condition-case 
err (let ((signal-hook-function #'ert--should-signal-hook)) (list nil 
42)) (error (progn (setq fn-0 #'signal) (list (car err) (cdr err))))))) 
(let ((value-2 'ert-form-evaluation-aborted-3)) (let 
(form-description-4) (if (unwind-protect (setq value-2 (apply fn-0 
args-1)) (setq form-description-4 (nconc (list '(should (equal nil 42))) 
(list :form (cons fn-0 args-1)) (if (eql value-2 
'ert-form-evaluation-aborted-3) nil (list :value value-2)) (if (eql 
value-2 'ert-form-evaluation-aborted-3) nil (let* ((-explainer- (and t 
(ert--get-explainer 'equal)))) (if -explainer- (list :explanation (apply 
-explainer- args-1)) nil))))) (ert--signal-should-execution 
form-description-4)) nil (ert-fail form-description-4))) value-2)))
   (let* ((--cl-foo-- #'(lambda (x) (list x)))) (progn (let* ((fn-0 
#'#<symbol equal at 95>) (args-1 (condition-case err (let 
((signal-hook-function #'ert--should-signal-hook)) (list nil 42)) (error 
(progn (setq fn-0 #'signal) (list (car err) (cdr err))))))) (let 
((value-2 'ert-form-evaluation-aborted-3)) (let (form-description-4) (if 
(unwind-protect (setq value-2 (apply fn-0 args-1)) (setq 
form-description-4 (nconc (list '(should (equal nil 42))) (list :form 
(cons fn-0 args-1)) (if (eql value-2 'ert-form-evaluation-aborted-3) nil 
(list :value value-2)) (if (eql value-2 'ert-form-evaluation-aborted-3) 
nil (let* ((-explainer- (and t (ert--get-explainer 'equal)))) (if 
-explainer- (list :explanation (apply -explainer- args-1)) nil))))) 
(ert--signal-should-execution form-description-4)) nil (ert-fail 
form-description-4))) value-2))))
   test()
   command-line-1(("--eval" "(byte-compile-file \"perturb.el\")" "-l" 
"use.el" "-f" "test"))
   command-line()
   normal-top-level()
Invalid function: #<symbol equal at 95>


The byte-compiler seems to have erroneously removed the symbol-function 
for equal.

In GNU Emacs 29.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.38,
  cairo version 1.16.0) of 2023-08-01, modified by Debian built on
  x86-ubc-02
Windowing system distributor 'The X.Org Foundation', version 11.0.12201009
System Description: Debian GNU/Linux trixie/sid

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

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3
THREADS TIFF TOOLKIT_SCROLL_BARS TREE_SITTER WEBP X11 XDBE XIM XINPUT2
XPM GTK3 ZLIB

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

[Mattias Engdegård]

> Error: invalid-function (#<symbol equal at 95>)

That is a symbol-with-position somehow leaking out.
We can simplify your nice little test case to

------- first file -----------
(require 'cl-macs)
(defun zeta () (cl-flet () #'equal))
------- second file ---------
(defun eta () (cl-flet () (funcall #'equal 12 34)))
------------------------------

and indeed, the leak is in cl--labels-convert-cache which will contain `equal` as a symbol-with-pos after byte-compiling the first file, and this causes trouble in the second file.

cl--labels-convert-cache contains

  (#<symbol equal at 49> function #<symbol equal at 49>)

and the function `eta` is consequently defined as

  (closure (t) nil (progn (#<symbol equal at 49> 12 34)))

where 49 is the position of `equal` in the first file.

Stefan and Alan should have a word here but I doubt we should hack this in cl-macs.el somehow, should we?
Making Ffuncall (etc) tolerant of symbol-with-pos isn't appealing either.

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

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

> We can simplify your nice little test case to
>
> ------- first file -----------
> (require 'cl-macs)
> (defun zeta () (cl-flet () #'equal))
> ------- second file ---------
> (defun eta () (cl-flet () (funcall #'equal 12 34)))
> ------------------------------
>
> and indeed, the leak is in cl--labels-convert-cache which will contain
> `equal` as a symbol-with-pos after byte-compiling the first file, and this
> causes trouble in the second file.
>
> cl--labels-convert-cache contains
>
>   (#<symbol equal at 49> function #<symbol equal at 49>)
>
> and the function `eta` is consequently defined as
>
>   (closure (t) nil (progn (#<symbol equal at 49> 12 34)))
>
> where 49 is the position of `equal` in the first file.

Thanks Mattias.

AFAICT the problem is that OT1H `symbols-with-pos-enabled` is
non-nil while loading the second file, but OTOH positions aren't
stripped from the resulting code.

So in `cl--labels-convert` when we check

    (eq f (car cl--labels-convert-cache))

we get when `f` is just `equal` whereas the cache contains
the sympos, but that sympos is not stripped later on.

I don't know why `symbols-with-pos-enabled` is non-nil at that point (I
thought we only enabled it wile byte-compiling), but assuming there's
a good reason for it, I tried to work around the problem with the patch
below which is conceptually correct (indeed we should only return
(cdr cl--labels-convert-cache) only in the case where `f` is exactly
the very same object as (car cl--labels-convert-cache)).

Sadly, it doesn't seem to help for a reason that escapes me.
The *Messages* buffer says:

    For information about GNU Emacs and the GNU system, type C-h C-a.
    Compiling .../tmp/foo1.el...
    Self-rewrite #<symbol equal at 82> to #'#<symbol equal at 82> (t)
    Compiling .../tmp/foo1.el...done
    Wrote .../tmp/foo1.elc
    Self-rewrite equal to #'#<symbol equal at 82> (t)
    Self-rewrite #<symbol equal at 82> to #'#<symbol equal at 82> (t)

where the middle "self-rewrite" is the culprit.
Apparently

    (let ((symbols-with-pos-enabled nil))
      (eq f (car cl--labels-convert-cache)))

returned non-nil when `f` was the bare `equal` and the `car` returned
the sympos.  How can that be?  I thought `eq` should really be the
good old "pointer equality" when `symbols-with-pos-enabled` is nil.

What am I missing?


        Stefan


diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index 0a3181561bd..bc71f565f3b 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -2037,7 +2039,12 @@
    ;; *after* handling `function', but we want to stop macroexpansion from
    ;; being applied infinitely, so we use a cache to return the exact `form'
    ;; being expanded even though we don't receive it.
-   ((eq f (car cl--labels-convert-cache)) (cdr cl--labels-convert-cache))
+   ((let ((symbols-with-pos-enabled nil))
+      (eq f (car cl--labels-convert-cache)))
+    (let ((print-symbols-bare nil))
+      (message "Self-rewrite %S to %S (%S)" f (cdr cl--labels-convert-cache)
+               symbols-with-pos-enabled))
+    (cdr cl--labels-convert-cache))
    (t
     (let* ((found (assq f macroexpand-all-environment))
            (replacement (and found

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

[Mattias Engdegård]

3 aug. 2023 kl. 16.43 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

>    (let ((symbols-with-pos-enabled nil))
>      (eq f (car cl--labels-convert-cache)))

As far as the LAP peephole optimiser is concerned, eq commutes with unbind so that let-binding will vanish.

(I'm more annoyed by the fact that `equal` doesn't work like `eq` for symbols when symbols-with-pos-enabled is nil.)

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

[Alan Mackenzie]

Hello, Mattias, and Stefan.

On Thu, Aug 03, 2023 at 11:39:33 +0200, Mattias Engdegård wrote:
> > Error: invalid-function (#<symbol equal at 95>)

> That is a symbol-with-position somehow leaking out.
> We can simplify your nice little test case to

> ------- first file -----------
> (require 'cl-macs)
> (defun zeta () (cl-flet () #'equal))
> ------- second file ---------
> (defun eta () (cl-flet () (funcall #'equal 12 34)))
> ------------------------------

> and indeed, the leak is in cl--labels-convert-cache which will contain `equal` as a symbol-with-pos after byte-compiling the first file, and this causes trouble in the second file.

> cl--labels-convert-cache contains

>   (#<symbol equal at 49> function #<symbol equal at 49>)

> and the function `eta` is consequently defined as

>   (closure (t) nil (progn (#<symbol equal at 49> 12 34)))

> where 49 is the position of `equal` in the first file.

First thoughts:

It would seem cl--labels-convert-cache is failing to get initialised,
somewhere.  Perhaps this is a problem of cache invalidation.  The
variable lacks a doc-string, which might otherwise have documented where
it is meant to be valid.  What does this variable mean?

cl--label-convert is defined as "Special macro-expander to rename
(function F) references in `cl-labels'.".  What does "rename (function
F) references" mean?  Is the term "name" in this context defined
anywhere?

> Stefan and Alan should have a word here but I doubt we should hack
> this in cl-macs.el somehow, should we?

If that's where the bug is, that's what we should fix.

> Making Ffuncall (etc) tolerant of symbol-with-pos isn't appealing
> either.

Definitely not!

-- 
Alan Mackenzie (Nuremberg, Germany).

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

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

>>    (let ((symbols-with-pos-enabled nil))
>>      (eq f (car cl--labels-convert-cache)))
> As far as the LAP peephole optimiser is concerned, eq commutes with unbind
> so that let-binding will vanish.

Hmm... so a bug in the optimizer because the new `eq` semantics breaks
a previous assumption  :-(

The positive side is that this optimization is less important for
lexbind code.


        Stefan

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

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

> cl--label-convert is defined as "Special macro-expander to rename
> (function F) references in `cl-labels'.".  What does "rename (function
> F) references" mean?  Is the term "name" in this context defined
> anywhere?

`cl--label-convert` is the macro expander for (function F) used inside
`cl-labels` so that

    (cl-labels ((my-foo () toto))
      #'my-foo)

gets turned into

    (let ((bar (lambda () toto)))
      bar)

So it "renames" (function my-foo) to the corresponding variable `bar`.

But for most (function BLA) the code should be left as-is, which a macro
can't do directly, so `cl--labels-convert-cache` is a hack which lets us
receive a handle to the overall (function BLA) form so we can return it
as-is rather than having to rebuild a *new* (function BLA) which would
just make the macro-expander call us right-back.

>> Making Ffuncall (etc) tolerant of symbol-with-pos isn't appealing
>> either.
> Definitely not!

I think we all agree on that.


        Stefan

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

[Alan Mackenzie]

Hello, Stefan.

On Thu, Aug 03, 2023 at 10:43:16 -0400, Stefan Monnier wrote:
> > We can simplify your nice little test case to

> > ------- first file -----------
> > (require 'cl-macs)
> > (defun zeta () (cl-flet () #'equal))
> > ------- second file ---------
> > (defun eta () (cl-flet () (funcall #'equal 12 34)))
> > ------------------------------

> > and indeed, the leak is in cl--labels-convert-cache which will contain
> > `equal` as a symbol-with-pos after byte-compiling the first file, and this
> > causes trouble in the second file.

> > cl--labels-convert-cache contains

> >   (#<symbol equal at 49> function #<symbol equal at 49>)

> > and the function `eta` is consequently defined as

> >   (closure (t) nil (progn (#<symbol equal at 49> 12 34)))

> > where 49 is the position of `equal` in the first file.

> Thanks Mattias.

> AFAICT the problem is that OT1H `symbols-with-pos-enabled` is
> non-nil while loading the second file, but OTOH positions aren't
> stripped from the resulting code.

As I suggested in my other post, it might be a "simple" problem of cache
invalidation.  Why is the value from the first compilation hanging around
until the second one?

> So in `cl--labels-convert` when we check

>     (eq f (car cl--labels-convert-cache))

> we get when `f` is just `equal` whereas the cache contains
> the sympos, but that sympos is not stripped later on.

> I don't know why `symbols-with-pos-enabled` is non-nil at that point (I
> thought we only enabled it wile byte-compiling), ....

I believe that is indeed the case.

> .... but assuming there's a good reason for it, I tried to work around
> the problem with the patch below which is conceptually correct (indeed
> we should only return (cdr cl--labels-convert-cache) only in the case
> where `f` is exactly the very same object as (car
> cl--labels-convert-cache)).

> Sadly, it doesn't seem to help for a reason that escapes me.

Setting symbols-with-pos-enabled to nil does nothing other than disable
the mechanisms which handle symbols with position.  Any such symbols
which exist will continue to be swp, but will no longer be EQ to the base
symbol, or other swp's with the same base symbol.

> The *Messages* buffer says:

>     For information about GNU Emacs and the GNU system, type C-h C-a.
>     Compiling .../tmp/foo1.el...
>     Self-rewrite #<symbol equal at 82> to #'#<symbol equal at 82> (t)
>     Compiling .../tmp/foo1.el...done
>     Wrote .../tmp/foo1.elc
>     Self-rewrite equal to #'#<symbol equal at 82> (t)
>     Self-rewrite #<symbol equal at 82> to #'#<symbol equal at 82> (t)

> where the middle "self-rewrite" is the culprit.
> Apparently

>     (let ((symbols-with-pos-enabled nil))
>       (eq f (car cl--labels-convert-cache)))

> returned non-nil when `f` was the bare `equal` and the `car` returned
> the sympos.  How can that be?  I thought `eq` should really be the
> good old "pointer equality" when `symbols-with-pos-enabled` is nil.

> What am I missing?


>         Stefan


> diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
> index 0a3181561bd..bc71f565f3b 100644
> --- a/lisp/emacs-lisp/cl-macs.el
> +++ b/lisp/emacs-lisp/cl-macs.el
> @@ -2037,7 +2039,12 @@
>     ;; *after* handling `function', but we want to stop macroexpansion from
>     ;; being applied infinitely, so we use a cache to return the exact `form'
>     ;; being expanded even though we don't receive it.
> -   ((eq f (car cl--labels-convert-cache)) (cdr cl--labels-convert-cache))
> +   ((let ((symbols-with-pos-enabled nil))
> +      (eq f (car cl--labels-convert-cache)))
> +    (let ((print-symbols-bare nil))
> +      (message "Self-rewrite %S to %S (%S)" f (cdr cl--labels-convert-cache)
> +               symbols-with-pos-enabled))
> +    (cdr cl--labels-convert-cache))
>     (t
>      (let* ((found (assq f macroexpand-all-environment))
>             (replacement (and found

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Mattias Engdegård]

3 aug. 2023 kl. 18.36 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> Hmm... so a bug in the optimizer because the new `eq` semantics breaks
> a previous assumption  :-(
> 
> The positive side is that this optimization is less important for
> lexbind code.

Yes, but as it only applies to one particular variable that isn't bound very often, I'm loath to remove it. For that matter it's not just about dynamic variable bindings but other constructs using unbind.

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

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

> As I suggested in my other post, it might be a "simple" problem of cache
> invalidation.  Why is the value from the first compilation hanging around
> until the second one?

That's a side problem.  If absolutely needed, we could add some ad-hoc
invalidation to work around the core problem, but I'd rather fix the
core problem.


        Stefan

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

[Mattias Engdegård]

3 aug. 2023 kl. 18.53 skrev Mattias Engdegård <mattias.engdegard@gmail.com>:

>> Hmm... so a bug in the optimizer because the new `eq` semantics breaks
>> a previous assumption  :-(
>> 
>> The positive side is that this optimization is less important for
>> lexbind code.
> 
> Yes, but as it only applies to one particular variable that isn't bound very often, I'm loath to remove it. For that matter it's not just about dynamic variable bindings but other constructs using unbind.

I checked and this particular optimisation (eq-unbind commutation) doesn't affect the generated bytecode in the Emacs tree anywhere, so it may be a candidate for removal after all.

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

[Alan Mackenzie]

Hello, Stefan.

On Thu, Aug 03, 2023 at 13:30:43 -0400, Stefan Monnier wrote:
> > As I suggested in my other post, it might be a "simple" problem of cache
> > invalidation.  Why is the value from the first compilation hanging around
> > until the second one?

> That's a side problem.  If absolutely needed, we could add some ad-hoc
> invalidation to work around the core problem, but I'd rather fix the
> core problem.

Are you sure?  What is the core problem?

As I see it, if the function cl--labels-convert is called during byte
compilation, cl--labels-convert-cache is going to get symbols with
position.  This is expected and is what we want - such a symbol might
easily give the position element of an error occurring in expanded code.

What we don't want is this symbol with position hanging around after the
end of the byte compilation that gives it its context.  If I understood
your other, explaining, post right, cl--labels-convert-cache is only
valid within a particular invocation of cl-flet or cl-labels.  So why
would it not be a complete bug fix to initialise this variable to nil at
the start of every cl-flet or cl-labels?  Or, more likely, bind the
variable to nil, to allow it to function better in nested invocations of
either of these macros?

What am I missing, here?

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Alan Mackenzie]

Hello, Stefan.

On Thu, Aug 03, 2023 at 12:41:55 -0400, Stefan Monnier wrote:
> > cl--label-convert is defined as "Special macro-expander to rename
> > (function F) references in `cl-labels'.".  What does "rename (function
> > F) references" mean?  Is the term "name" in this context defined
> > anywhere?

> `cl--label-convert` is the macro expander for (function F) used inside
> `cl-labels` so that

>     (cl-labels ((my-foo () toto))
>       #'my-foo)

> gets turned into

>     (let ((bar (lambda () toto)))
>       bar)

> So it "renames" (function my-foo) to the corresponding variable `bar`.

> But for most (function BLA) the code should be left as-is, which a macro
> can't do directly, so `cl--labels-convert-cache` is a hack which lets us
> receive a handle to the overall (function BLA) form so we can return it
> as-is rather than having to rebuild a *new* (function BLA) which would
> just make the macro-expander call us right-back.

OK, thanks.

[ .... ]

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

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

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

>> That's a side problem.  If absolutely needed, we could add some ad-hoc
>> invalidation to work around the core problem, but I'd rather fix the
>> core problem.
>
> Are you sure?

Yes.

> What is the core problem?

The core problem is that we want to stop infinite macroexpansion of the
same term.  I.e. we want to stop macroexpansion from turning

    (function FOO)
=>
    (function FOO)
=>
    (function FOO)
=>
    ...

so we tweak the macro-expander for `function` so that if FOO is exaction
the same as last time, we return the exact list (function FOO) we built
last time, rather than building a new one.

If `eq` works as god intended (i.e. it performs a pointer-equality
test), then this is perfectly safe because the cache always has the form

    (#1=<SYM> . (function #1#))

so using the `cdr` of the cache returns something perfectly equivalent
to `(function ,f) just with the added twist that it's the exact same
list we return last time so may stop the macroexpansion.  So it's safe
the use the cache even when it's stale.

But with `symbols-with-pos-enabled` the `eq` test can end up using the
cache when FOO is not 100% the same as <SYM> and so we can end up
replacing `equal` or #<symbol equal at 467> with (function #<symbol
equal at 81>) neither of which is really correct, the second of which
can happen even if we try and flush the cache.

> As I see it, if the function cl--labels-convert is called during byte
> compilation, cl--labels-convert-cache is going to get symbols with
> position.  This is expected and is what we want - such a symbol might
> easily give the position element of an error occurring in expanded code.

Exactly, which is why the `eq` needs to pay attention to the
position information.

> So why would it not be a complete bug fix to initialise this variable
> to nil at the start of every cl-flet or cl-labels?

Because it could/would still replace some sympos with others within the
same `cl-label`.


        Stefan

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

[Alan Mackenzie]

Hello, Stefan and Mattias.

On Thu, Aug 03, 2023 at 13:30:43 -0400, Stefan Monnier wrote:
> > As I suggested in my other post, it might be a "simple" problem of cache
> > invalidation.  Why is the value from the first compilation hanging around
> > until the second one?

> That's a side problem.  If absolutely needed, we could add some ad-hoc
> invalidation to work around the core problem, but I'd rather fix the
> core problem.

Sorry about my last post.  I now see what the core problem is, namely
that (equal 'equal #<symbol equal at 49>) is returning non-nil.

I think the cause is in internal_equal in src/fns.c, where the following
code appears:

  /* A symbol with position compares the contained symbol, and is
     `equal' to the corresponding ordinary symbol.  */
  if (SYMBOL_WITH_POS_P (o1))
    o1 = SYMBOL_WITH_POS_SYM (o1);
  if (SYMBOL_WITH_POS_P (o2))
    o2 = SYMBOL_WITH_POS_SYM (o2);

This piece of code converts symbols with positions to ordinary symbols
without first checking symbols-with-pos-enabled.  I think this is the
bug.

I will work on a patch to fix it (which shouldn't take long).

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

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

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

> Sorry about my last post.  I now see what the core problem is, namely
> that (equal 'equal #<symbol equal at 49>) is returning non-nil.

This is not really the core problem IIUC since `cl-macs.el` uses `eq`
rather than `equal` so changing `equal` won't make much of
a difference here.

I'm not sure whether the above should return nil, or non-nil, or the value
of `symbols-with-pos-enabled`, to be honest, but I guess returning non-nil
has worked fine until now, so I think we'd be better off staying with that.

I'd even like it to try and replace uses of `eq/eql` with `equal` in
those cases where we want to overlook differences in symbol-positions, so
that we can eventually get rid of `symbols-with-pos-enabled` which
I consider as a wart.


        Stefan

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

[Eli Zaretskii]

> Cc: acm@muc.de, 65017@debbugs.gnu.org,
>  Eric Marsden <eric.marsden@risk-engineering.org>
> Date: Thu, 3 Aug 2023 21:10:56 +0000
> From: Alan Mackenzie <acm@muc.de>
> 
> I think the cause is in internal_equal in src/fns.c, where the following
> code appears:
> 
>   /* A symbol with position compares the contained symbol, and is
>      `equal' to the corresponding ordinary symbol.  */
>   if (SYMBOL_WITH_POS_P (o1))
>     o1 = SYMBOL_WITH_POS_SYM (o1);
>   if (SYMBOL_WITH_POS_P (o2))
>     o2 = SYMBOL_WITH_POS_SYM (o2);
> 
> This piece of code converts symbols with positions to ordinary symbols
> without first checking symbols-with-pos-enabled.  I think this is the
> bug.
> 
> I will work on a patch to fix it (which shouldn't take long).

Thanks, but when you have a solution in hand, please also check its
effect on performance.  AFAIR, this part was tuned for optimal
performance, back when symbols with positions were introduced; it
would be a pity to lose performance due to fixing this bug, if that
can be avoided.

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

[Alan Mackenzie]

Hello, Stefan.

On Thu, Aug 03, 2023 at 17:46:44 -0400, Stefan Monnier wrote:
> > Sorry about my last post.  I now see what the core problem is, namely
> > that (equal 'equal #<symbol equal at 49>) is returning non-nil.

> This is not really the core problem IIUC since `cl-macs.el` uses `eq`
> rather than `equal` so changing `equal` won't make much of
> a difference here.

No, it wasn't the cause of this bug.  It's a separate bug in its own
right, though.

> I'm not sure whether the above should return nil, or non-nil, or the value
> of `symbols-with-pos-enabled`, to be honest, but I guess returning non-nil
> has worked fine until now, so I think we'd be better off staying with that.

I've lost the context, somewhat, but the key thing is that the notion of
symbol with position isn't really defined when symbols-with-pos-enabled
is nil.  Returning non-nil for (equal 'foo #<symbol foo at 42>) in this
case is like saying 'foo is equal to an undefined entity.  This is
asking for the sort of trouble we're seeing in this bug.

> I'd even like it to try and replace uses of `eq/eql` with `equal` in
> those cases where we want to overlook differences in symbol-positions, so
> that we can eventually get rid of `symbols-with-pos-enabled` which
> I consider as a wart.

OK.  I don't think you can do this, because you'd have to replace lots
of eq's in macros with equal.  We don't control all these macros.
That's aside from the massive disruption this would cause to bytecomp.el
and friends.  I don't think you should do this, since
symbols-with-pos-enabled, ugly though it may be, is working.  Also,
you'd have to be careful not to slow Emacs down.

Now, let's diagnose bug#65017!

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Mattias Engdegård]

3 aug. 2023 kl. 23.46 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> I'd even like it to try and replace uses of `eq/eql` with `equal` in
> those cases where we want to overlook differences in symbol-positions, so
> that we can eventually get rid of `symbols-with-pos-enabled` which
> I consider as a wart.

I agree it would be wonderful if we could restore `eq` to its former simplicity and speed but is that easily achievable at this point? For example, what about macros that compare arguments with `eq`?

Separate data structures for locations might be an option worth exploring, keeping the actual s-expressions unadorned. Consider a reader mode that also produces a table mapping cons cells read to their locations, for example.

(By the way, I removed the unwind-eq commutation on master for now.)

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

[Alan Mackenzie]

Hello, Mattias.

On Fri, Aug 04, 2023 at 12:14:14 +0200, Mattias Engdegård wrote:
> 3 aug. 2023 kl. 23.46 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> > I'd even like it to try and replace uses of `eq/eql` with `equal` in
> > those cases where we want to overlook differences in
> > symbol-positions, so that we can eventually get rid of
> > `symbols-with-pos-enabled` which I consider as a wart.

> I agree it would be wonderful if we could restore `eq` to its former
> simplicity and speed but is that easily achievable at this point? For
> example, what about macros that compare arguments with `eq`?

> Separate data structures for locations might be an option worth
> exploring, keeping the actual s-expressions unadorned. Consider a
> reader mode that also produces a table mapping cons cells read to their
> locations, for example.

Using a hash table, or something similar?  This is all very well, but the
garbage collecter, for every collected object, would have to check
whether that object is in that table, and if so remove it.  This would
slow down garbage collection, possibly by a lot.  The slow down would be
less if there were a variable saying whether or not this table is active.
Something very like symbols-with-pos-enabled.  ;-)

[ .... ]

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Alan Mackenzie]

Hello, Stefan, Mattias, and Eric.

On Thu, Aug 03, 2023 at 10:43:16 -0400, Stefan Monnier wrote:

[ .... ]

> AFAICT the problem is that OT1H `symbols-with-pos-enabled` is
> non-nil while loading the second file, but OTOH positions aren't
> stripped from the resulting code.

> So in `cl--labels-convert` when we check

>     (eq f (car cl--labels-convert-cache))

[ .... ]

> 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).
This is the cause of the bug; in cl--labels-convert it causes the first
eq to return non-nil when comparing 'equal to #<symbol equal at 194>.

Removing that binding fixes the problem.  This does not affect make
bootstrap or make check.

The necessary patch is:



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)


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

Please try out the patch, and if all is well I can commit it and close
the bug.

[ .... ]

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Mattias Engdegård]

4 aug. 2023 kl. 13.11 skrev Alan Mackenzie <acm@muc.de>:

> Using a hash table, or something similar?  This is all very well, but the
> garbage collecter, for every collected object, would have to check
> whether that object is in that table, and if so remove it.  This would
> slow down garbage collection, possibly by a lot.

The table could have weak keys, and there are known GC algorithms for dealing with those.
It doesn't actually need to be a hash table at all -- we only need to optimise for fast insertion and small size, not lookup (for displaying compiler diagnostics, for example).

This is a very half-baked idea; I haven't thought it through at all. For example, we may need a way to transfer location from one sexp to a newly built one, and that may complicate the inner workings of the compiler (or not).

It's clear that symbols-with-pos is a neat hack that is surprisingly effective, with almost no changes to Lisp code needed at all. It definitely has improved diagnostics. Unfortunately it makes everything a bit slower, not just the compiler, but symbols-with-pos will serve until we come up with something better.

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

[Eli Zaretskii]

> Cc: Mattias Engdegård <mattias.engdegard@gmail.com>,
>  65017@debbugs.gnu.org, Eric Marsden <eric.marsden@risk-engineering.org>
> Date: Fri, 4 Aug 2023 13:22:58 +0000
> From: Alan Mackenzie <acm@muc.de>
> 
> symbols-with-pos-enabled gets erroneously
> bound to t in internal-macroexpand-for-load (emacs-lisp/macroexp.el).
> This is the cause of the bug; in cl--labels-convert it causes the first
> eq to return non-nil when comparing 'equal to #<symbol equal at 194>.

Why "erroneously"? what are the rules for binding that variable to a
non-nil value?  I don't see any of that documented in the "Symbols
with Position" node of the ELisp manual.

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

[Alan Mackenzie]

Hello, Eli.

On Fri, Aug 04, 2023 at 08:35:30 +0300, Eli Zaretskii wrote:
> > Cc: acm@muc.de, 65017@debbugs.gnu.org,
> >  Eric Marsden <eric.marsden@risk-engineering.org>
> > Date: Thu, 3 Aug 2023 21:10:56 +0000
> > From: Alan Mackenzie <acm@muc.de>
> > 
> > I think the cause is in internal_equal in src/fns.c, where the following
> > code appears:
> > 
> >   /* A symbol with position compares the contained symbol, and is
> >      `equal' to the corresponding ordinary symbol.  */
> >   if (SYMBOL_WITH_POS_P (o1))
> >     o1 = SYMBOL_WITH_POS_SYM (o1);
> >   if (SYMBOL_WITH_POS_P (o2))
> >     o2 = SYMBOL_WITH_POS_SYM (o2);
> > 
> > This piece of code converts symbols with positions to ordinary symbols
> > without first checking symbols-with-pos-enabled.  I think this is the
> > bug.
> > 
> > I will work on a patch to fix it (which shouldn't take long).

I have raised bug #65051 for this (which already contains a patch).

> Thanks, but when you have a solution in hand, please also check its
> effect on performance.  AFAIR, this part was tuned for optimal
> performance, back when symbols with positions were introduced; it
> would be a pity to lose performance due to fixing this bug, if that
> can be avoided.

I don't think we need worry, here.  In the generated code, in the normal
non-compilation scenario, two tests are being replaced by one test.

The current two tests are on variables which will already be in
registers, but they perform fairly involved bit twiddling.

The new test is a simple zero/non-zero test on a variable which, whilst
not yet in a register, will certainly be in the processor's cache.
Also, that variable will shortly be needed again, in the Lisp_Cons case.

I'll try a few timings, though.  I'll be surprised indeed if there're
any measurable differences.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Alan Mackenzie]

Hello, Eli.

On Fri, Aug 04, 2023 at 17:04:36 +0300, Eli Zaretskii wrote:
> > Cc: Mattias Engdegård <mattias.engdegard@gmail.com>,
> >  65017@debbugs.gnu.org, Eric Marsden <eric.marsden@risk-engineering.org>
> > Date: Fri, 4 Aug 2023 13:22:58 +0000
> > From: Alan Mackenzie <acm@muc.de>

> > symbols-with-pos-enabled gets erroneously
> > bound to t in internal-macroexpand-for-load (emacs-lisp/macroexp.el).
> > This is the cause of the bug; in cl--labels-convert it causes the first
> > eq to return non-nil when comparing 'equal to #<symbol equal at 194>.

> Why "erroneously"? what are the rules for binding that variable to a
> non-nil value?

internal-macroexpand-for-load isn't being called in the context of a
byte compilation.  It might create a symbol with position which wrongly
matches, or fails to match, another symbol.  This is what has happened
in this bug.

> I don't see any of that documented in the "Symbols with Position" node
> of the ELisp manual.

Well, there is the sentence: "These objects are intended for use by the
byte compiler, which records in them the position of each symbol
occurrence and uses those positions in warning and error messages.".

Do you think this should be firmed up to something like:  "These objects
are for the use of the byte compiler, which records in them the position
of each symbol occurrence and uses those positions in warning and error
messages.  They shouldn't normally be used otherwise."?

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Eli Zaretskii]

> Date: Fri, 4 Aug 2023 14:49:56 +0000
> Cc: monnier@iro.umontreal.ca, mattias.engdegard@gmail.com,
>   65017@debbugs.gnu.org, eric.marsden@risk-engineering.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > > symbols-with-pos-enabled gets erroneously
> > > bound to t in internal-macroexpand-for-load (emacs-lisp/macroexp.el).
> > > This is the cause of the bug; in cl--labels-convert it causes the first
> > > eq to return non-nil when comparing 'equal to #<symbol equal at 194>.
> 
> > Why "erroneously"? what are the rules for binding that variable to a
> > non-nil value?
> 
> internal-macroexpand-for-load isn't being called in the context of a
> byte compilation.  It might create a symbol with position which wrongly
> matches, or fails to match, another symbol.  This is what has happened
> in this bug.

If internal-macroexpand-for-load is "verboten" from being called by
the byte-compiler, I'd expect an assertion in it to that effect.
Because someone, some day, might easily forget and call that function
in the byte-compiler.

Btw, why was this binding added there to begin with?

> > I don't see any of that documented in the "Symbols with Position" node
> > of the ELisp manual.
> 
> Well, there is the sentence: "These objects are intended for use by the
> byte compiler, which records in them the position of each symbol
> occurrence and uses those positions in warning and error messages.".
> 
> Do you think this should be firmed up to something like:  "These objects
> are for the use of the byte compiler, which records in them the position
> of each symbol occurrence and uses those positions in warning and error
> messages.  They shouldn't normally be used otherwise."?

Something like that, perhaps even stronger.  And maybe an explanation
what kind of problems could using them outside of the byte compiler
cause.

Thanks.

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

[Alan Mackenzie]

Hello, Eli.

On Fri, Aug 04, 2023 at 18:22:29 +0300, Eli Zaretskii wrote:
> > Date: Fri, 4 Aug 2023 14:49:56 +0000
> > Cc: monnier@iro.umontreal.ca, mattias.engdegard@gmail.com,
> >   65017@debbugs.gnu.org, eric.marsden@risk-engineering.org
> > From: Alan Mackenzie <acm@muc.de>

> > > > symbols-with-pos-enabled gets erroneously
> > > > bound to t in internal-macroexpand-for-load (emacs-lisp/macroexp.el).
> > > > This is the cause of the bug; in cl--labels-convert it causes the first
> > > > eq to return non-nil when comparing 'equal to #<symbol equal at 194>.

> > > Why "erroneously"? what are the rules for binding that variable to a
> > > non-nil value?

> > internal-macroexpand-for-load isn't being called in the context of a
> > byte compilation.  It might create a symbol with position which wrongly
> > matches, or fails to match, another symbol.  This is what has happened
> > in this bug.

> If internal-macroexpand-for-load is "verboten" from being called by
> the byte-compiler, I'd expect an assertion in it to that effect.
> Because someone, some day, might easily forget and call that function
> in the byte-compiler.

I don't think there's any such prohibition in this case.  The function is
called only from readevalloop in src/lread.c as part of loading a .el
file.  It is probable that an eval-when-compile could cause a .el file to
be loaded during a byte compilation.  This would call
internal-macroexpand-for-load with symbols-with-pos-enabled non-nil, I
think.

> Btw, why was this binding added there to begin with?

A good question.  It was back in January 2022, I was getting eager macro
expansion failures while trying to bootstrap (my development version of)
Emacs.  Binding that variable made those failures go away.  It turns out,
that was not the correct fix.  Somehow that erroneous binding stayed in
the code and got committed.

> > > I don't see any of that documented in the "Symbols with Position" node
> > > of the ELisp manual.

> > Well, there is the sentence: "These objects are intended for use by the
> > byte compiler, which records in them the position of each symbol
> > occurrence and uses those positions in warning and error messages.".

> > Do you think this should be firmed up to something like:  "These objects
> > are for the use of the byte compiler, which records in them the position
> > of each symbol occurrence and uses those positions in warning and error
> > messages.  They shouldn't normally be used otherwise."?

> Something like that, perhaps even stronger.  And maybe an explanation
> what kind of problems could using them outside of the byte compiler
> cause.

OK.  Maybe ".... They shouldn't normally be used otherwise.  Doing so can
cause unexpected results with basic Emacs functions such as `eq' and
`equal'."?

> Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Eli Zaretskii]

> Date: Fri, 4 Aug 2023 16:43:12 +0000
> Cc: monnier@iro.umontreal.ca, mattias.engdegard@gmail.com,
>   65017@debbugs.gnu.org, eric.marsden@risk-engineering.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > If internal-macroexpand-for-load is "verboten" from being called by
> > the byte-compiler, I'd expect an assertion in it to that effect.
> > Because someone, some day, might easily forget and call that function
> > in the byte-compiler.
> 
> I don't think there's any such prohibition in this case.  The function is
> called only from readevalloop in src/lread.c as part of loading a .el
> file.  It is probable that an eval-when-compile could cause a .el file to
> be loaded during a byte compilation.  This would call
> internal-macroexpand-for-load with symbols-with-pos-enabled non-nil, I
> think.

But then, if the caller binds this variable non-nil, the problem will
again rear its ugly head?

> > > Do you think this should be firmed up to something like:  "These objects
> > > are for the use of the byte compiler, which records in them the position
> > > of each symbol occurrence and uses those positions in warning and error
> > > messages.  They shouldn't normally be used otherwise."?
> 
> > Something like that, perhaps even stronger.  And maybe an explanation
> > what kind of problems could using them outside of the byte compiler
> > cause.
> 
> OK.  Maybe ".... They shouldn't normally be used otherwise.  Doing so can
> cause unexpected results with basic Emacs functions such as `eq' and
> `equal'."?

That's a good start, thanks.

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

[Alan Mackenzie]

Hello, Eli

On Fri, Aug 04, 2023 at 08:35:30 +0300, Eli Zaretskii wrote:
> > Cc: acm@muc.de, 65017@debbugs.gnu.org,
> >  Eric Marsden <eric.marsden@risk-engineering.org>
> > Date: Thu, 3 Aug 2023 21:10:56 +0000
> > From: Alan Mackenzie <acm@muc.de>

> Thanks, but when you have a solution in hand, please also check its
> effect on performance.  AFAIR, this part was tuned for optimal
> performance, back when symbols with positions were introduced; it
> would be a pity to lose performance due to fixing this bug, if that
> can be avoided.

This should really be in bug #65051, but:

I've run 

    time src/emacs -Q -batch --eval '(let ((a 1) (b 1) (times 10000000)) (while (> times 0) (equal a b) (setq times (1- times))))'

with different values for a and b (and slightly different quoting
syntax, sometimes).  I get the following results, reporting the "user:
time" from GNU/Linux:

New code:
    a, b = 1, 1: 1.760s 1.760s 1,746s
    a, b = 1, 3: 1.796s 1,772s 1.777s
           1.0, 1.0: 1,757s 1.776s 1.751s
	   1.0, 1.1: 1.792s 1.760s 1.779s
	   '(a b c), '(a b c): 2.041s 2.042s 2.039s
	   '(a b c), '(a b d): 2.096s 2.071s 2.084s
	   "1", "1": 1.841s 1.860s 1.845s
	   "1", "3": 1.865s 1.846s 1.869s

Old code:
   a, b = 1, 1: 1.744s 1.757s 1.762s
   a, b = 1, 3: 1.755s 1,777s 1.759s
          1.0, 1.0: 1.787s 1.748s 1.775s
	  1.0, 1.1: 1.762s 1.770s 1.774s
	  '(a b c), '(a b c): 2.021s 2.057s 2.019s
	  '(a b c), '(a b d): 2.046s 2.090s 2.100s
	  "1", "1": 1.854s 1.900s 1.884s
	  "1", "3": 1.849s 1.833s 1.838s

I think it's fair to say that the new code is not slower than the old
code, to within the measuring limits of these simple tests.  Any
differences, such as they are, are in the second and third decimal
places, and vary more between different measurements, than between the
Old code and New code.  They are surely too small to matter.

-- 
Alan Mackenzie (Nuremberg, Germany).

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

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

>> I'd even like it to try and replace uses of `eq/eql` with `equal` in
>> those cases where we want to overlook differences in symbol-positions, so
>> that we can eventually get rid of `symbols-with-pos-enabled` which
>> I consider as a wart.
> I agree it would be wonderful if we could restore `eq` to its former
> simplicity and speed but is that easily achievable at this point?

I didn't say it would be easy.  But I think it's feasible (tho it will
take many years).

> For example, what about macros that compare arguments with `eq`?

Yes, these are the cases where we need to "replace uses of `eq/eql` with `equal`".

> Separate data structures for locations might be an option worth exploring,
> keeping the actual s-expressions unadorned. Consider a reader mode that also
> produces a table mapping cons cells read to their locations, for example.

When Alan looked at it, the cost seemed prohibitive.

BTW, a related option would be to develop a new kind of macro-definition
along the lines of what's used in Scheme, where the macro author doesn't
need to worry about such issues because the macro knows which parts of
the data it manipulates are chunks of code (potentially adorned with
metainfo) and can take care of extracting the underlying unadorned code.


        Stefan

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

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

> I've lost the context, somewhat, but the key thing is that the notion of
> symbol with position isn't really defined when symbols-with-pos-enabled
> is nil.  Returning non-nil for (equal 'foo #<symbol foo at 42>) in this
> case is like saying 'foo is equal to an undefined entity.  This is
> asking for the sort of trouble we're seeing in this bug.

I don't like this idea that we should pretend sympos objects don't exist
(or are "undefined") when `symbols-with-pos-enabled` is nil.

> and friends.  I don't think you should do this, since
> symbols-with-pos-enabled, ugly though it may be, is working.

As I said, I consider it as a wart, IOW a technical debt.
We *should* come up with a plan to reimburse this debt, regardless if it
takes a long time to pay it back.


        Stefan

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

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

>> 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).

> 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).


        Stefan

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

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

>> internal-macroexpand-for-load isn't being called in the context of a
>> byte compilation.  It might create a symbol with position which wrongly
>> matches, or fails to match, another symbol.  This is what has happened
>> in this bug.
>
> If internal-macroexpand-for-load is "verboten" from being called by
> the byte-compiler, I'd expect an assertion in it to that effect.

It's not "verboten".  Removing the binding just says that if you want to
pass adorned code to that function, it's the caller's responsability to
let-bind the variable around the call.

> Because someone, some day, might easily forget and call that function
> in the byte-compiler.

The byte-compiler already let-binds that variables, so it won't be a problem.

> Btw, why was this binding added there to begin with?

IIUC it took some trial and error to get to understand where that var
needs to be bound (as well as where is the right spot to strip the
sympos), and this is just one binding that was never removed after we
found a better place for it.


        Stefan

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

[Eli Zaretskii]

> Date: Sat, 5 Aug 2023 20:22:32 +0000
> Cc: monnier@iro.umontreal.ca, mattias.engdegard@gmail.com,
>   65017@debbugs.gnu.org, eric.marsden@risk-engineering.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > Thanks, but when you have a solution in hand, please also check its
> > effect on performance.  AFAIR, this part was tuned for optimal
> > performance, back when symbols with positions were introduced; it
> > would be a pity to lose performance due to fixing this bug, if that
> > can be avoided.
> 
> This should really be in bug #65051, but:
> 
> I've run 
> 
>     time src/emacs -Q -batch --eval '(let ((a 1) (b 1) (times 10000000)) (while (> times 0) (equal a b) (setq times (1- times))))'
> 
> with different values for a and b (and slightly different quoting
> syntax, sometimes).  I get the following results, reporting the "user:
> time" from GNU/Linux:
> 
> New code:
>     a, b = 1, 1: 1.760s 1.760s 1,746s
>     a, b = 1, 3: 1.796s 1,772s 1.777s
>            1.0, 1.0: 1,757s 1.776s 1.751s
> 	   1.0, 1.1: 1.792s 1.760s 1.779s
> 	   '(a b c), '(a b c): 2.041s 2.042s 2.039s
> 	   '(a b c), '(a b d): 2.096s 2.071s 2.084s
> 	   "1", "1": 1.841s 1.860s 1.845s
> 	   "1", "3": 1.865s 1.846s 1.869s
> 
> Old code:
>    a, b = 1, 1: 1.744s 1.757s 1.762s
>    a, b = 1, 3: 1.755s 1,777s 1.759s
>           1.0, 1.0: 1.787s 1.748s 1.775s
> 	  1.0, 1.1: 1.762s 1.770s 1.774s
> 	  '(a b c), '(a b c): 2.021s 2.057s 2.019s
> 	  '(a b c), '(a b d): 2.046s 2.090s 2.100s
> 	  "1", "1": 1.854s 1.900s 1.884s
> 	  "1", "3": 1.849s 1.833s 1.838s
> 
> I think it's fair to say that the new code is not slower than the old
> code, to within the measuring limits of these simple tests.  Any
> differences, such as they are, are in the second and third decimal
> places, and vary more between different measurements, than between the
> Old code and New code.  They are surely too small to matter.

Thanks, this LGTM.

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

[Mattias Engdegård]

6 aug. 2023 kl. 00.40 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

>> Separate data structures for locations might be an option worth exploring,
>> keeping the actual s-expressions unadorned. Consider a reader mode that also
>> produces a table mapping cons cells read to their locations, for example.
> 
> When Alan looked at it, the cost seemed prohibitive.

I'm not aware of the details but think that it may be worth revisiting.

The only serious difficulty a priori appears to be keeping locations in transformed code, but most of our diagnostics are issued on code before Lisp optimisations so this should mostly matter to macro-expansion.

> BTW, a related option would be to develop a new kind of macro-definition
> along the lines of what's used in Scheme, where the macro author doesn't
> need to worry about such issues because the macro knows which parts of
> the data it manipulates are chunks of code (potentially adorned with
> metainfo) and can take care of extracting the underlying unadorned code.

Yes, Scheme doesn't have much problems with user code manipulating Lisp as data.
But what do actual Lisp compilers do? Do they perform a slow re-read when until they get to the location they want, when they need to issue a diagnostic? (Relint does something similar, in fact.)

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

[Alan Mackenzie]

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).

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

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

>>> Separate data structures for locations might be an option worth exploring,
>>> keeping the actual s-expressions unadorned. Consider a reader mode that also
>>> produces a table mapping cons cells read to their locations, for example.
>> When Alan looked at it, the cost seemed prohibitive.
> I'm not aware of the details but think that it may be worth revisiting.

Feel free :-)

> The only serious difficulty a priori appears to be keeping locations in
> transformed code, but most of our diagnostics are issued on code before Lisp
> optimisations so this should mostly matter to macro-expansion.

We shouldn't emit errors/warnings from byte-opt, indeed, so that part
doesn't need to preserve the info, but we still need to preserve it
at least through macro-expansion and cconv.

> Yes, Scheme doesn't have much problems with user code manipulating Lisp as data.
> But what do actual Lisp compilers do? Do they perform a slow re-read when
> until they get to the location they want, when they need to issue
> a diagnostic? (Relint does something similar, in fact.)

Scheme's macro system has a notion of "syntactic object" (i.e. an sexp
combined with metainfo), and the macros make it clear when you're
looking inside a syntactic object or when you're building a new
syntactic object.

Don't know what Common Lisp systems do with `defmacro`, sorry.
I know Gambit Scheme has "defmacro" (under the name `define-macro`) but
it treats it as "second class citizen" in the sense that when using
a macro defined with `define-macro`, the metainfo is just lost.


        Stefan

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

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

> I suggest installing this patch into master.

LGTM, thanks.

>> > 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?

Yes.

> Why not?

Not sure how to explain it any further than what I already described.

> 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?

No, it should not treat them equal (when it does, it introduces an
incorrect sympos and can thus lead to error messages pointing at the
wrong place).

> There is no mechanism to make these two SWPs eq whilst excluding their
> eq with the bare symbol.

We luckily don't need such a mechanism here: we just need to use
"BASE_EQ" :-)


        Stefan

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

[Alan Mackenzie]

Hello, Stefan.

On Mon, Aug 07, 2023 at 22:44:29 -0400, Stefan Monnier wrote:
> > I suggest installing this patch into master.

> LGTM, thanks.

> >> > 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?

> Yes.

What about two SWPs with the same symbol but different positions?  If
they aren't considered EQ here, there will never be a match for the
first arm of the cond form in cl--labels-convert; then
cl--labels-convert-cache will get written, but never used.

And if, somehow, it does get used (the current code, I think), then (as
you write below) the argument F will get replaced by an F with the wrong
position.  Am I right, here?

Why must the F get replaced by a different F?  There must surely be a
way, a simpler way than the current cl--labels-convert, to retain the
current F (hence, not corrupting its position)?

[ .... ]

> > 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?

> No, it should not treat them equal (when it does, it introduces an
> incorrect sympos and can thus lead to error messages pointing at the
> wrong place).

Then isn't what is wrong here the introduction of the incorrect SWP
rather than treating the two SWPs as EQ?

This is obscure, difficult code.  :-(

We should think about committing a fix to the original bug, sometime,
too.

[ .... ]

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

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

[Alan Mackenzie]

Hello, Eric.

On Wed, Aug 02, 2023 at 12:28:24 +0200, Eric Marsden wrote:
> The byte-compiler seems to erroneously remove the symbol-function for 
> equal in the
> code show below.

> --- file "perturb.el" ---
> (require 'cl-lib)

> (defun foo ()
>    (cl-flet ((bar (v) (list v)))
>      (make-hash-table :test #'equal)))
> ---


> --- file "use.el" ---
> (require 'cl-lib)
> (require 'ert)

> (defun test ()
>    (cl-flet ((foo (x) (list x)))
>      (should (equal nil 42))))
> ---

> % emacs -Q --batch --eval '(byte-compile-file "perturb.el")' -l use.el 
> -f test
> Error: invalid-function (#<symbol equal at 95>)
>    mapbacktrace(#f(compiled-function (evald func args flags) #<bytecode 
> -0x84e95e6e2517821>))
>    debug-early-backtrace()
>    debug-early(error (invalid-function #<symbol equal at 95>))
>    #<symbol equal at 95>(nil 42)
>    apply(#<symbol equal at 95> (nil 42))
>    (setq value-2 (apply fn-0 args-1))
>    (unwind-protect (setq value-2 (apply fn-0 args-1)) (setq 
> form-description-4 (nconc (list '(should (equal nil 42))) (list :form 
> (cons fn-0 args-1)) (if (eql value-2 'ert-form-evaluation-aborted-3) nil 
> (list :value value-2)) (if (eql value-2 'ert-form-evaluation-aborted-3) 
> nil (let* ((-explainer- (and t (ert--get-explainer 'equal)))) (if 
> -explainer- (list :explanation (apply -explainer- args-1)) nil))))) 
> (ert--signal-should-execution form-description-4))
>    (if (unwind-protect (setq value-2 (apply fn-0 args-1)) (setq 
> form-description-4 (nconc (list '(should (equal nil 42))) (list :form 
> (cons fn-0 args-1)) (if (eql value-2 'ert-form-evaluation-aborted-3) nil 
> (list :value value-2)) (if (eql value-2 'ert-form-evaluation-aborted-3) 
> nil (let* ((-explainer- (and t (ert--get-explainer 'equal)))) (if 
> -explainer- (list :explanation (apply -explainer- args-1)) nil))))) 
> (ert--signal-should-execution form-description-4)) nil (ert-fail 
> form-description-4))

[ .... ]

>    test()
>    command-line-1(("--eval" "(byte-compile-file \"perturb.el\")" "-l" 
> "use.el" "-f" "test"))
>    command-line()
>    normal-top-level()
> Invalid function: #<symbol equal at 95>


> The byte-compiler seems to have erroneously removed the symbol-function 
> for equal.

The bug is now fixed in the master branch, and I'm closing the bug with
this post.

> In GNU Emacs 29.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.38,
>   cairo version 1.16.0) of 2023-08-01, modified by Debian built on
>   x86-ubc-02
> Windowing system distributor 'The X.Org Foundation', version 11.0.12201009
> System Description: Debian GNU/Linux trixie/sid

The patch I posted on Sunday, although correct, doesn't apply cleanly to
Emacs 29.1.  So I'm giving you a version of the patch which does work on
Emacs-29, and will enable you to fix the bug in your own copy of Emacs.

After applying the patch, it will be advisable/necessary to rebuild your
Emacs with

    $ make -j16 bootstrap # adjust the -j16 for your processor.

..  I think you'll know how to do this, but if you want any help with
applying the patch or running the bootstrap, feel free to send me
private email.

Here's the patch:



diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index d093d95a775..a9deb53db03 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -483,8 +483,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)
@@ -526,12 +525,11 @@ 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
-                                      (byte-run-strip-symbol-positions
-                                       (macroexpand--all-toplevel
-                                        form
-                                        macroexpand-all-environment))))
+                              (let ((expanded
+                                     (byte-run-strip-symbol-positions
+                                      (macroexpand--all-toplevel
+                                       form
+                                       macroexpand-all-environment))))
                                 (eval expanded lexical-binding)
                                 expanded)))))
     (with-suppressed-warnings
@@ -2436,8 +2434,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)
@@ -2471,8 +2468,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
@@ -2568,8 +2564,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 168de1bf180..6c604a75a33 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)
@@ -787,40 +786,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


-- 
Alan Mackenzie (Nuremberg, Germany).

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

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

>> > Are you sure?
>> Yes.
> What about two SWPs with the same symbol but different positions?  If
> they aren't considered EQ here, there will never be a match for the
> first arm of the cond form in cl--labels-convert; then
> cl--labels-convert-cache will get written, but never used.

Nope: when it gets written, the `function` macro returns:

    (function <THESYMWITHPOS>)

so the macro is immediately called again with the *exact* same
<THESYMWITHPOS>, so the second time the `function` macro is called, the
cache does hit (and it's the only case where it should hit), making the
second call to the macro return the *exact* `eq`-same

    (function <THESYMWITHPOS>)

list which is the trick that stops the infinite macroexpansion loop.

Next time the `function` macro is invoked with a "similar" sympos the
cache should *not* match because we don't want to accidentally replace

    (function <SOMESYMWITHPOS>)
with
    (function <THESYMWITHPOS>)

> And if, somehow, it does get used (the current code, I think), then (as
> you write below) the argument F will get replaced by an F with the wrong
> position.  Am I right, here?

That's right.

> Why must the F get replaced by a different F?  There must surely be a
> way, a simpler way than the current cl--labels-convert, to retain the
> current F (hence, not corrupting its position)?

There might.  The current hack is the best I could come up with.

>> > 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?
>> No, it should not treat them equal (when it does, it introduces an
>> incorrect sympos and can thus lead to error messages pointing at the
>> wrong place).
> Then isn't what is wrong here the introduction of the incorrect SWP
> rather than treating the two SWPs as EQ?

I can't see how to separate one from the other here: the introduction of
the incorrect SWP is due to treating the two SWP as `eq`.

> This is obscure, difficult code.  :-(

Agreed.

> We should think about committing a fix to the original bug,
> sometime, too.

I'm not completely sure we agree yet on what is "the original bug", but
obviously I agree with  your sentence :-)


        Stefan

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

[Alan Mackenzie]

Hello, Stefan.

On Wed, Aug 09, 2023 at 23:41:56 -0400, Stefan Monnier wrote:
> >> > Are you sure?
> >> Yes.
> > What about two SWPs with the same symbol but different positions?  If
> > they aren't considered EQ here, there will never be a match for the
> > first arm of the cond form in cl--labels-convert; then
> > cl--labels-convert-cache will get written, but never used.

> Nope: when it gets written, the `function` macro returns:

>     (function <THESYMWITHPOS>)

> so the macro is immediately called again with the *exact* same
> <THESYMWITHPOS>, so the second time the `function` macro is called, the
> cache does hit (and it's the only case where it should hit), making the
> second call to the macro return the *exact* `eq`-same

>     (function <THESYMWITHPOS>)

> list which is the trick that stops the infinite macroexpansion loop.

OK, thanks, I think I've got that now.

> Next time the `function` macro is invoked with a "similar" sympos the
> cache should *not* match because we don't want to accidentally replace

>     (function <SOMESYMWITHPOS>)
> with
>     (function <THESYMWITHPOS>)

> > And if, somehow, it does get used (the current code, I think), then (as
> > you write below) the argument F will get replaced by an F with the wrong
> > position.  Am I right, here?

> That's right.

OK.  So perhaps binding symbols-with-pos-enabled to nil around that eq
call could be the way to go.

> > Why must the F get replaced by a different F?  There must surely be a
> > way, a simpler way than the current cl--labels-convert, to retain the
> > current F (hence, not corrupting its position)?

> There might.  The current hack is the best I could come up with.

I'm not criticising the hack, not at all!  But it could be better
commented, and the doc string for cl--labels-convert could be more
informative.  The "why" is missing - why is necessary to handle
`function' as a macro?  I think it's to inhibit the processing of
`function' as function somewhere else, but where and why?  I think you
explained it, at least partly, in an earlier post on this thread.

> >> > 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?
> >> No, it should not treat them equal (when it does, it introduces an
> >> incorrect sympos and can thus lead to error messages pointing at the
> >> wrong place).
> > Then isn't what is wrong here the introduction of the incorrect SWP
> > rather than treating the two SWPs as EQ?

> I can't see how to separate one from the other here: the introduction of
> the incorrect SWP is due to treating the two SWP as `eq`.

> > This is obscure, difficult code.  :-(

> Agreed.

> > We should think about committing a fix to the original bug,
> > sometime, too.

> I'm not completely sure we agree yet on what is "the original bug", but
> obviously I agree with  your sentence :-)

I meant bug #65017.  I committed a fix for it yesterday using the patch
I posted here on Sunday, and closed the bug.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

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

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

>> > And if, somehow, it does get used (the current code, I think), then (as
>> > you write below) the argument F will get replaced by an F with the wrong
>> > position.  Am I right, here?
>> That's right.
> OK.  So perhaps binding symbols-with-pos-enabled to nil around that eq
> call could be the way to go.

Indeed, that's the patch I suggested.  I think it's fundamentally right,
but it doesn't work (yet) because it bumps into the optimization bug
introduced by making `eq` depend on `symbols-with-pos-enabled`  :-(

>> > Why must the F get replaced by a different F?  There must surely be a
>> > way, a simpler way than the current cl--labels-convert, to retain the
>> > current F (hence, not corrupting its position)?
>> There might.  The current hack is the best I could come up with.
> I'm not criticising the hack, not at all!  But it could be better
> commented, and the doc string for cl--labels-convert could be more
> informative.

It would help if you could send a (rough) patch showing what comment
you'd have liked to see.

> The "why" is missing - why is necessary to handle `function' as
> a macro?

I couldn't really think of any alternative for it ("it" being to
implement `cl-labels` and `cl-flet`).  FWIW, the pre-cl-lib code did it
differently by duplicating `macroexp--expand-all` wholesale and then
tweaking its handling of `function` in an ad-hoc way.

BTW, there's a standard solution from Common Lisp to get rid of this
hack: implement the `&whole` macro argument.
[Macro Lambda Lists](http://clhs.lisp.se/Body/03_dd.htm)

> I think it's to inhibit the processing of `function' as function
> somewhere else, but where and why?

It's not a function but a special operator, which is thus handled in
a hard-coded way by `macroexp--expand-all`.

>> I'm not completely sure we agree yet on what is "the original bug", but
>> obviously I agree with  your sentence :-)
> I meant bug #65017.  I committed a fix for it yesterday using the patch
> I posted here on Sunday, and closed the bug.

FWIW my notion of "the original bug" should be fixed by the patch below
(modulo the above-mentioned optimization bug which makes the patch
ineffective).

BTW, I pushed an extended version of that patch which additionally does
what you suggested we should do (and which I claimed wouldn't work),
i.e. flush the cache (I originally couldn't see where to flush it).
I believe this does fix the original problem even in the remaining
corner cases, despite the optimization bug.


        Stefan


diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index 0a3181561bd..a405ae67691 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -2037,7 +2037,9 @@ cl--labels-convert
    ;; *after* handling `function', but we want to stop macroexpansion from
    ;; being applied infinitely, so we use a cache to return the exact `form'
    ;; being expanded even though we don't receive it.
-   ((eq f (car cl--labels-convert-cache)) (cdr cl--labels-convert-cache))
+   ((let ((symbols-with-pos-enabled nil))
+      (eq f (car cl--labels-convert-cache)))
+    (cdr cl--labels-convert-cache))
    (t
     (let* ((found (assq f macroexpand-all-environment))
            (replacement (and found

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

[Mattias Engdegård]

12 aug. 2023 kl. 05.28 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> Indeed, that's the patch I suggested.  I think it's fundamentally right,
> but it doesn't work (yet) because it bumps into the optimization bug
> introduced by making `eq` depend on `symbols-with-pos-enabled`  :-(

The one disabled in 44d7fd3805?

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

[Mattias Engdegård]

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

12 aug. 2023 kl. 05.28 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> there's a standard solution from Common Lisp to get rid of this
> hack: implement the `&whole` macro argument.

If we can live without the special `&whole` syntax then it seems rather straightforward (patch), although I'd rather not add the extra specbind until needed for a concrete purpose.

One such purpose, apart from removing the hack, would be better diagnostics in some cases.


[-- Attachment #2: macroexp-whole.diff --]
[-- Type: application/octet-stream, Size: 1681 bytes --]

diff --git a/lisp/emacs-lisp/macroexp.el b/lisp/emacs-lisp/macroexp.el
index 6d5cf8723f9..18eaf005cb4 100644
--- a/lisp/emacs-lisp/macroexp.el
+++ b/lisp/emacs-lisp/macroexp.el
@@ -199,6 +199,15 @@ macroexp--obsolete-warning
            (instead (format-message "; use `%s' instead." instead))
            (t ".")))))
 
+(defvar macroexp--whole-form
+  ;; The current form being macroexpanded.
+  )
+
+(defun macroexp-whole-form ()
+  "The current form being macro-expanded.
+Can only be called during macro-expansion (ie, inside a macro)."
+  macroexp--whole-form)
+
 (defun macroexpand-1 (form &optional environment)
   "Perform (at most) one step of macroexpansion."
   (cond
@@ -219,7 +228,8 @@ macroexpand-1
              ((not (consp def)) form)
              (t
               (if (eq 'macro (car def))
-                  (apply (cdr def) (cdr form))
+                  (let ((macroexp--whole-form form))
+                    (apply (cdr def) (cdr form)))
                 form))))))))
    (t form)))
 
diff --git a/src/eval.c b/src/eval.c
index 9e54d489a3b..0d32b63bf1d 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1161,7 +1161,9 @@ DEFUN ("macroexpand", Fmacroexpand, Smacroexpand, 1, 2, 0,
 	    break;
 	}
       {
-	Lisp_Object newform = apply1 (expander, XCDR (form));
+	specpdl_ref pdl = SPECPDL_INDEX ();
+	specbind (Qmacroexp__whole_form, form);
+	Lisp_Object newform = unbind_to (pdl, apply1 (expander, XCDR (form)));
 	if (EQ (form, newform))
 	  break;
 	else
@@ -4462,4 +4464,5 @@ syms_of_eval (void)
   defsubr (&Sspecial_variable_p);
   DEFSYM (Qfunctionp, "functionp");
   defsubr (&Sfunctionp);
+  DEFSYM (Qmacroexp__whole_form, "macroexp--whole-form");
 }

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

[Alan Mackenzie]

Hello, Stefan.

On Fri, Aug 11, 2023 at 23:28:01 -0400, Stefan Monnier wrote:

[ .... ]

> >> > Why must the F get replaced by a different F?  There must surely be a
> >> > way, a simpler way than the current cl--labels-convert, to retain the
> >> > current F (hence, not corrupting its position)?
> >> There might.  The current hack is the best I could come up with.
> > I'm not criticising the hack, not at all!  But it could be better
> > commented, and the doc string for cl--labels-convert could be more
> > informative.

> It would help if you could send a (rough) patch showing what comment
> you'd have liked to see.

I'll come up with something as soon as I understand it enough.

> > The "why" is missing - why is necessary to handle `function' as
> > a macro?

> I couldn't really think of any alternative for IT ("it" being to
> implement `cl-labels` and `cl-flet`).  FWIW, the pre-cl-lib code did IT
> differently by duplicating `macroexp--expand-all` wholesale and then
> tweaking its handling of `function` in an ad-hoc way.

Your reply doesn't address my question.  It is not clear to me what the
IT in your previous paragraph is.  You may or may not have thought of
some alternative for IT, and previous code may have done IT differently,
but that doesn't help me understand what IT is.  What was the difficulty
in cl-labels and cl-flet for which IT and cl--labels-convert was the
solution?

The code is substituting (function F) with a non-eq (function F).
You're saying this has some effect in macroexp--expand-all.  I can't see
that, yet.  All I see is FORM, (function F), being substituted by a
different (function F) in L327 of macroexp.el.  Then there are the pcase
arms for (function (lambda ....)) and for (function ....).  Are either
of these pcase arms affected by the "expansion" of FORM?  If so, how?
Or am I looking at the wrong place entirely?

[ .... ]

> > I think it's to inhibit the processing of `function' as function
> > somewhere else, but where and why?

> It's not a function but a special operator, which is thus handled in
> a hard-coded way by `macroexp--expand-all`.

Is it the case that this hard-coded handling for function is prevented
by the macro "expansion" of (function F)?  Otherwise, what is the
relevance of this hard-coded handling to cl--labels-convert?

[ .... ]

>         Stefan

[ .... ]

-- 
Alan Mackenzie (Nuremberg, Germany).

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

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

>> Indeed, that's the patch I suggested.  I think it's fundamentally right,
>> but it doesn't work (yet) because it bumps into the optimization bug
>> introduced by making `eq` depend on `symbols-with-pos-enabled`  :-(
> The one disabled in 44d7fd3805?

Ah, sorry, I didn't notice it, thanks for the heads up.


        Stefan

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

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

> The code is substituting (function F) with a non-eq (function F).
> You're saying this has some effect in macroexp--expand-all.  I can't see
> that, yet.  All I see is FORM, (function F), being substituted by a
> different (function F) in L327 of macroexp.el.  Then there are the pcase
> arms for (function (lambda ....)) and for (function ....).  Are either
> of these pcase arms affected by the "expansion" of FORM?  If so, how?
> Or am I looking at the wrong place entirely?

`cl-flet` needs to replace (function LOCALFUN) with LOCALVAR within the
body of the let, for those LOCALFUNs defined in the `cl-flet`.
That's easy to do with a macro.

But it also should leave all other uses of `function` untouched.
That's the part that does not map well to macros since macros are
repeatedly expanded until they return something that's not a macro call.

>> It's not a function but a special operator, which is thus handled in
>> a hard-coded way by `macroexp--expand-all`.
> Is it the case that this hard-coded handling for function is prevented
> by the macro "expansion" of (function F)?

Yes, we first expand the macros and then try to handle the result
which should be one of the hard-coded cases (or is otherwise assumed to
be a function call).


        Stefan

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

[Alan Mackenzie]

Hello, Stefan.

On Sat, Aug 12, 2023 at 14:28:44 -0400, Stefan Monnier wrote:
> >> I couldn't really think of any alternative for IT ("it" being to
> >> implement `cl-labels` and `cl-flet`).  FWIW, the pre-cl-lib code
> >> did IT differently by duplicating `macroexp--expand-all` wholesale
> >> and then tweaking its handling of `function` in an ad-hoc way.

> > Your reply doesn't address my question.  It is not clear to me what
> > the IT in your previous paragraph is.  You may or may not have
> > thought of some alternative for IT, and previous code may have done
> > IT differently, but that doesn't help me understand what IT is.
> > What was the difficulty in cl-labels and cl-flet for which IT and
> > cl--labels-convert was the solution?

> > The code is substituting (function F) with a non-eq (function F).
> > You're saying this has some effect in macroexp--expand-all.  I can't see
> > that, yet.  All I see is FORM, (function F), being substituted by a
> > different (function F) in L327 of macroexp.el.  Then there are the pcase
> > arms for (function (lambda ....)) and for (function ....).  Are either
> > of these pcase arms affected by the "expansion" of FORM?  If so, how?
> > Or am I looking at the wrong place entirely?

> `cl-flet` needs to replace (function LOCALFUN) with LOCALVAR within the
> body of the let, for those LOCALFUNs defined in the `cl-flet`.
> That's easy to do with a macro.

> But it also should leave all other uses of `function` untouched.
> That's the part that does not map well to macros since macros are
> repeatedly expanded until they return something that's not a macro call.

Thanks, that's useful information.  But it doesn't address my questions
in the slightest.  This is the third time I'm asking you for help.  I
thought it would be quicker than figuring everything out on my own.  You
wrote the code, I think.

I don't understand how cl--labels-convert works, down at the car and cdr
level.  I'm asking you to help me, but I'm not sure how sensible it is
to carry on asking repeatedly.  I've replaced two paragraphs you snipped
from your last reply.

Would you please answer these specific questions, now, to help me
understand this difficult mechanism.  Thanks!

> >> It's not a function but a special operator, which is thus handled in
> >> a hard-coded way by `macroexp--expand-all`.
> > Is it the case that this hard-coded handling for function is prevented
> > by the macro "expansion" of (function F)?

> Yes, we first expand the macros and then try to handle the result
> which should be one of the hard-coded cases (or is otherwise assumed to
> be a function call).

Are you talking about the code in macroexp--expand-all, here?  By
"macros", do you mean cl-flet and cl-labels here (as opposed to
function)?  What do you mean by "hard-coded cases"?  Do you mean the
pcase arms in macroexp--expand-all?  If so, which ones?

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

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

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

> Thanks, that's useful information.  But it doesn't address my questions
> in the slightest.

I'm sorry.  I guess I still haven't figured what it is that I assume as
known but which you don't actually know.

> Would you please answer these specific questions, now, to help me
> understand this difficult mechanism.  Thanks!

[ I assume you're talking about the questions below.  ]

>> >> It's not a function but a special operator, which is thus handled in
>> >> a hard-coded way by `macroexp--expand-all`.
>> > Is it the case that this hard-coded handling for function is prevented
>> > by the macro "expansion" of (function F)?
>> Yes, we first expand the macros and then try to handle the result
>> which should be one of the hard-coded cases (or is otherwise assumed to
>> be a function call).
> Are you talking about the code in macroexp--expand-all, here?

Yes.

> By "macros", do you mean cl-flet and cl-labels here (as opposed to
> function)?

I'm talking about any call to an identifier that is "currently" defined
as a macro.  This can be either because the `symbol-function` holds
something of the form `(macro . <DEF>)` or because
`macroexpand-all-environment` has an entry for that identifier.

> What do you mean by "hard-coded cases"?

The face that `macroexp--expand-all` handles the `function` identifier
as follows:

          (pcase form
            [...]
            (`(,(or 'function 'quote) . ,_) form)
            [...]


-- Stefan

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

[Alan Mackenzie]

Hello, Stefan.

On Sun, Aug 13, 2023 at 12:12:02 -0400, Stefan Monnier wrote:
> > Thanks, that's useful information.  But it doesn't address my questions
> > in the slightest.

> I'm sorry.  I guess I still haven't figured what it is that I assume as
> known but which you don't actually know.

I didn't, and don't know the answers to the questions I asked.

> > Would you please answer these specific questions, now, to help me
> > understand this difficult mechanism.  Thanks!

> [ I assume you're talking about the questions below.  ]

I was actually talking about those strings of characters which I had
terminated with the '?' character.  All you've done with them is to snip
them from your reply.  I'm not going to ask you a fourth time.  You
clearly don't want to answer these questions, for some reason.

Maybe I'll get around to working out for myself how this code works,
maybe I won't.  But if it's up to me to fix the broken commenting/doc
strings associated with cl--labels-convert, it's not looking like it'll
get done any time soon.

Thnks for the answers that you did give me, below.

> >> >> It's not a function but a special operator, which is thus handled in
> >> >> a hard-coded way by `macroexp--expand-all`.
> >> > Is it the case that this hard-coded handling for function is prevented
> >> > by the macro "expansion" of (function F)?
> >> Yes, we first expand the macros and then try to handle the result
> >> which should be one of the hard-coded cases (or is otherwise assumed to
> >> be a function call).
> > Are you talking about the code in macroexp--expand-all, here?

> Yes.

> > By "macros", do you mean cl-flet and cl-labels here (as opposed to
> > function)?

> I'm talking about any call to an identifier that is "currently" defined
> as a macro.  This can be either because the `symbol-function` holds
> something of the form `(macro . <DEF>)` or because
> `macroexpand-all-environment` has an entry for that identifier.

> > What do you mean by "hard-coded cases"?

> The face that `macroexp--expand-all` handles the `function` identifier
> as follows:

>           (pcase form
>             [...]
>             (`(,(or 'function 'quote) . ,_) form)
>             [...]


> -- Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).