bug#57397: 29.0.50; cl-letf blindly macroexpands places

bug#57397: 29.0.50; cl-letf blindly macroexpands places

[Michael Heerdegen]

Hello,

background: to get rid of warnings about the recently obsoleted
generalized variables in my init file... because I'm lazy and wanted to
try (and there were only two) I blindly replaced them with synthetic
places - like this:

#+begin_src emacs-lisp
(defun test1 (b query)
  (cl-letf (((buffer-local-value 'buffer-save-without-query b)
             (or buffer-save-without-query query)))
    (with-current-buffer b (save-buffer))))
;; ~~>
(defun test2 (b query)
  (cl-letf (((gv-synthetic-place
              (buffer-local-value 'buffer-save-without-query b)
              (lambda (v) `(setq-local buffer-save-without-query ,v)))
             (or buffer-save-without-query query)))
    (with-current-buffer b (save-buffer))))
#+end_src

[ I know it is an nonsense example wrt what it does and how it does it,
that doesn't matter. ]

This should get rid of the warning - but in this case it doesn't (in
the other it did).  The reason is that Stefan made cl-letf (the helper
cl--letf more precisely), macroexpand each place:

| 91a7f934ac * lisp/emacs-lisp/cl-macs.el: Fix bug#26073.

to support symbol macros better:

#+begin_src emacs-lisp
(defun cl--letf (bindings simplebinds binds body)
  ;; It's not quite clear what the semantics of cl-letf should be...10..
  (if (null bindings)
      (if (and (null binds) (null simplebinds)) (macroexp-progn body)..20..)
    (let* ((binding (car bindings))
           (place (macroexpand (car binding) macroexpand-all-environment)));<--!!
      (gv-letplace (getter setter) place..12..))))
#+end_src

I think this is an error in the general case.  Maybe it's enough to
expand only symbol macros?  It's at least always wrong when (car PLACE)
is a macro name with a gv spec defined.

TIA,

Michael.


In GNU Emacs 29.0.50 (build 2, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0, Xaw scroll bars) of 2022-08-25 built on drachen
Repository revision: bd5b704447ac5ec0559a824209bb01b271d29959
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Debian GNU/Linux 11 (bullseye)

Configured using:
 'configure --with-x-toolkit=lucid'

bug#57397: cl-letf blindly macroexpands places

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

> to support symbol macros better:
>
> #+begin_src emacs-lisp
> (defun cl--letf (bindings simplebinds binds body)
>   ;; It's not quite clear what the semantics of cl-letf should be...10..
>   (if (null bindings)
>       (if (and (null binds) (null simplebinds)) (macroexp-progn body)..20..)
>     (let* ((binding (car bindings))
>            (place (macroexpand (car binding) macroexpand-all-environment)));<--!!
>       (gv-letplace (getter setter) place..12..))))
> #+end_src
>
> I think this is an error in the general case.  It's at least always
> wrong when (car PLACE) is a macro name with a gv spec defined.

Indeed, defining a gv spec for a macro is fiddly.

> Maybe it's enough to expand only symbol macros?

Yes, that should still cover the original need in bug#26073 without
breaking your use case.

But regardless of this, we should probably turn `gv-synthetic-place`
into a function so it's more robust.


        Stefan

bug#57397: cl-letf blindly macroexpands places

[Michael Heerdegen]

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

> > Maybe it's enough to expand only symbol macros?
>
> Yes, that should still cover the original need in bug#26073 without
> breaking your use case.

Was it necessary to really expand symbol macros to fix that bug, or is
the purpose only to handle the following `symbolp' test correctly?

> But regardless of this, we should probably turn `gv-synthetic-place`
> into a function so it's more robust.

Why is it a macro?  Seems to work quite as well (using the same body and
gv-spec) when defined as a function.

Michael.

bug#57397: cl-letf blindly macroexpands places

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

>> > Maybe it's enough to expand only symbol macros?
>> Yes, that should still cover the original need in bug#26073 without
>> breaking your use case.
> 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`.

>> But regardless of this, we should probably turn `gv-synthetic-place`
>> into a function so it's more robust.
> Why is it a macro?

Beats me.
I tried to ask the original author but he was not available for comments.

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


        Stefan

bug#57397: cl-letf blindly macroexpands places

[Michael Heerdegen]

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

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?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Turn-gv-synthetic-place-into-a-function.patch --]
[-- Type: text/x-diff, Size: 1441 bytes --]

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


[-- Attachment #3: Type: text/plain, Size: 729 bytes --]


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.

bug#57397: cl-letf blindly macroexpands places

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

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

Pushed to `master`.

> Ok - does this look correct?

Looks good, yes.

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

There are two cases: one is when EXPANDER is of the form (lambda ...)
and the other is when it's not (in which case it'll be a symbol naming
a function defined elsewhere).

When you write

    (defun my-foo (arg1 &optional arg2)
      (declare (compiler-macro (lambda (whole) ..blabla..)))
      ..toto..)

it is macro expanded to something more or less equivalent to:

    (defun my-foo (arg1 arg2)
      (declare (compiler-macro my-foo--expander))
      ..toto..)
    (defun my-foo--expander (whole arg1 &optional arg2)
      ..blabla..)

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

I can see the source of your confusion, but I'm not sure how to write it
better, without making it much more verbose (and risk making it yet
more confusing).

Would something like the patch below help?


        Stefan


diff --git a/doc/lispref/functions.texi b/doc/lispref/functions.texi
index 983dfe2ec59..8e34fdf3640 100644
--- a/doc/lispref/functions.texi
+++ b/doc/lispref/functions.texi
@@ -2476,11 +2476,11 @@ Declare Form
 expander will call @var{expander} with that form as well as with
 @var{args}@dots{}, and @var{expander} can either return a new expression to use
 instead of the function call, or it can return just the form unchanged,
-to indicate that the function call should be left alone.  @var{expander} can
-be a symbol, or it can be a form @code{(lambda (@var{arg}) @var{body})} in
-which case @var{arg} will hold the original function call expression, and the
-(unevaluated) arguments to the function can be accessed using the function's
-formal arguments.
+to indicate that the function call should be left alone.
+
+To avoid syntactic redundancy, when @var{expander} is of the form
+@code{(lambda (@var{arg}) @var{body})} the function's formal arguments
+are automatically added to the lambda's list of arguments.
 
 @item (gv-expander @var{expander})
 Declare @var{expander} to be the function to handle calls to the macro (or

bug#57397: cl-letf blindly macroexpands places

[Michael Heerdegen]

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

> diff --git a/doc/lispref/functions.texi b/doc/lispref/functions.texi
> index 983dfe2ec59..8e34fdf3640 100644
> --- a/doc/lispref/functions.texi
> +++ b/doc/lispref/functions.texi
> @@ -2476,11 +2476,11 @@ Declare Form
>  expander will call @var{expander} with that form as well as with
>  @var{args}@dots{}, and @var{expander} can either return a new expression to use
>  instead of the function call, or it can return just the form unchanged,
> -to indicate that the function call should be left alone.  @var{expander} can
> -be a symbol, or it can be a form @code{(lambda (@var{arg}) @var{body})} in
> -which case @var{arg} will hold the original function call expression, and the
> -(unevaluated) arguments to the function can be accessed using the function's
> -formal arguments.
> +to indicate that the function call should be left alone.
> +
> +To avoid syntactic redundancy, when @var{expander} is of the form
> +@code{(lambda (@var{arg}) @var{body})} the function's formal arguments
> +are automatically added to the lambda's list of arguments.

Yes, that would have helped a lot.  A definitive improvement.

But could we maybe describe that simpler like "when expander is a lambda
form [...]"?  - Because AFAIU these arguments are added to _any_
argument list - with other words, implicitly hint that it's an error to
specify function arguments in the lambda arglist explicitly, or to
provide an empty arglist.

Michael.

bug#57397: cl-letf blindly macroexpands places

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

> But could we maybe describe that simpler like "when expander is a lambda
> form [...]"?  - Because AFAIU these arguments are added to _any_
> argument list - with other words, implicitly hint that it's an error to
> specify function arguments in the lambda arglist explicitly, or to
> provide an empty arglist.

OK, thanks, done,


        Stefan

bug#57397: cl-letf blindly macroexpands places

[Eli Zaretskii]

> Cc: 57397@debbugs.gnu.org, Lars Ingebrigtsen <larsi@gnus.org>
> Date: Wed, 28 Sep 2022 12:29:20 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> > But could we maybe describe that simpler like "when expander is a lambda
> > form [...]"?  - Because AFAIU these arguments are added to _any_
> > argument list - with other words, implicitly hint that it's an error to
> > specify function arguments in the lambda arglist explicitly, or to
> > provide an empty arglist.
> 
> OK, thanks, done,

IMO, the text which was installed looks devoid of any useful
information:

          When EXPANDER is a lambda form it should be of the form
          ‘(lambda (ARG) BODY)’ because the function’s formal arguments
          are automatically added to the lambda’s list of arguments.

Isn't every lambda form _always_ of that form?

And "because" is out of the blue: there's no cause and effect relation
here that I can identify.

The original text was somewhat more self-explanatory:

 To avoid syntactic redundancy, when @var{expander} is of the form
 @code{(lambda (@var{arg}) @var{body})} the function's formal arguments
 are automatically added to the lambda's list of arguments.

This explains the reason, and actually reverses the cause and the
effect, which then make sense.  (Although the issue with the form of
lambda still stands.)

What am I missing?

bug#57397: cl-letf blindly macroexpands places

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> The original text was somewhat more self-explanatory:
>
>  To avoid syntactic redundancy, when @var{expander} is of the form
>  @code{(lambda (@var{arg}) @var{body})} the function's formal arguments
>  are automatically added to the lambda's list of arguments.
>
> This explains the reason, and actually reverses the cause and the
> effect, which then make sense.  (Although the issue with the form of
> lambda still stands.)

Hmm - I liked that wording more, too.

> What am I missing?

We needed to say more clearly that the given lambda form must have an
argument list of exactly one argument.

Michael.

bug#57397: cl-letf blindly macroexpands places

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> We needed to say more clearly that the given lambda form must have an
> argument list of exactly one argument.

I think you should say exactly that, in those words.

bug#57397: cl-letf blindly macroexpands places

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  57397@debbugs.gnu.org,
>   larsi@gnus.org
> Date: Wed, 28 Sep 2022 19:10:59 +0200
> 
> We needed to say more clearly that the given lambda form must have an
> argument list of exactly one argument.

Then just say it, in these very words.

bug#57397: cl-letf blindly macroexpands places

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Then just say it, in these very words.

Ok...could you please rethink your change then, Stefan?

Thanks,

Michael.

bug#57397: cl-letf blindly macroexpands places

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

Michael Heerdegen [2022-09-28 19:56:32] wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
>> Then just say it, in these very words.
> Ok...could you please rethink your change then, Stefan?

Hopefully better now,


        Stefan

bug#57397: 29.0.50; cl-letf blindly macroexpands places

[Michael Heerdegen]

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

> >> Then just say it, in these very words.
> > Ok...could you please rethink your change then, Stefan?
>
> Hopefully better now,
>
>
>         Stefan

Yes, better.  And I think we are done, so I am closing my report.

Thanks to all,

Michael.