bug#57397: cl-letf blindly macroexpands places

[Michael Heerdegen]

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

> > Was it necessary to really expand symbol macros to fix that bug, or is
> > the purpose only to handle the following `symbolp' test correctly?
>
> No the problem shows up in the `gv-letplace` that follows immediately,
> so by the time we get to the `symbolp` test it's too late.
> But I suspect that the better fix is to skip the macroexpand call here
> and to change `gv-get` so as to do a `macroexpand-1` call even if its
> arg is a `symbolp`.

Ok, you have obviously more insight here, so can you maybe...take over
this part?

> > [`gv-synthetic-place'] seems to work quite as well (using the same
> > body and gv-spec) when defined as a function.
>
> The only downside is that the code is less efficient (the getter has to
> construct the closure of the setter, then call `gv-synthetic-place`
> which then just throws it away) but that should be easy to fix with
> a compiler macro.

Ok - does this look correct?

From 585981019e32ff4aa1a7ce4614428744d1b55332 Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Wed, 31 Aug 2022 03:13:09 +0200
Subject: [PATCH] Turn gv-synthetic-place into a function

This fixes Bug#57397.

* lisp/emacs-lisp/gv.el (gv-synthetic-place): Make a function and add
trivial compiler macro to avoid decreasing efficiency.
---
 lisp/emacs-lisp/gv.el | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lisp/emacs-lisp/gv.el b/lisp/emacs-lisp/gv.el
index eaab6439ad..9c3f77d2cc 100644
--- a/lisp/emacs-lisp/gv.el
+++ b/lisp/emacs-lisp/gv.el
@@ -532,13 +532,13 @@ plist-get
        (funcall do `(error . ,args)
                 (lambda (v) `(progn ,v (error . ,args))))))

-(defmacro gv-synthetic-place (getter setter)
+(defun gv-synthetic-place (getter setter)
   "Special place described by its setter and getter.
 GETTER and SETTER (typically obtained via `gv-letplace') get and
-set that place.  I.e. This macro allows you to do the \"reverse\" of what
-`gv-letplace' does.
-This macro only makes sense when used in a place."
-  (declare (gv-expander funcall))
+set that place.  I.e. this function allows you to do the
+\"reverse\" of what `gv-letplace' does.  This function only makes
+sense when used in a place."
+  (declare (gv-expander funcall) (compiler-macro (lambda (_) getter)))
   (ignore setter)
   getter)

--
2.30.2

BTW, I had trouble understanding the paragraph about the compiler-macro
declare specs in (info "(elisp) Declare Form"), in particular the calling
convention:

| [...] When encountering a call to the function, of the form ‘(FUNCTION
| ARGS...)’, the macro expander will call EXPANDER with that form as
| well as with ARGS...

not only because of the colons, but also because it's...wrong?  EXPANDER
is called with one argument, and the other formal arguments are
available (bound) to the corresponding argument forms, right?

Could you then maybe rephrase a bit [I don't want to, my English is not
good enough.  I'm able to do it but it always takes much too long to
find a good wording.]

TIA,

Michael.