bug#56739: 29.0.50; `cl-psetq' and `cl-psetf' fail to recognize symbol macros

bug#56739: 29.0.50; `cl-psetq' and `cl-psetf' fail to recognize symbol macros

[Wing Hei Chan]

The following form produces (2 2) due to the failure of detecting
dependencies involving symbol macros.

(cl-symbol-macrolet ((c a))
   (let ((a 1) (b 2))
     (cl-psetq a b
               b c)
     (list a b)))

It should have behaved the same as the following form without symbol
macros, that is, producing (2 1).

(let ((a 1) (b 2))
   (cl-psetq a b
             b a)
   (list a b))

bug#56739: 29.0.50; `cl-psetq' and `cl-psetf' fail to recognize symbol macros

[Michael Heerdegen]

Wing Hei Chan <whmunkchan@outlook.com> writes:

> The following form produces (2 2) due to the failure of detecting
> dependencies involving symbol macros.
>
> (cl-symbol-macrolet ((c a))
>   (let ((a 1) (b 2))
>     (cl-psetq a b
>               b c)
>     (list a b)))

Dunno how a good fix would look like - but it is easy to follow why this
error happens: `cl-psetf' analyses the expressions (each second
argument) for whether they are "simple" (independent of the variables):

(if (or (not (symbolp (car p))) (cl--expr-depends-p (nth 1 p) vars))
  (setq simple nil))

but the symbol macro is not yet expanded when this is done, and the
expression `c` passes the test - which is wrong.

Maybe that test should just check for symbol macros in addition?

Michael.

bug#56739: 29.0.50; `cl-psetq' and `cl-psetf' fail to recognize symbol macros

[Michael Heerdegen]

Wing Hei Chan <whmunkchan@outlook.com> writes:

> The following form produces (2 2) due to the failure of detecting
> dependencies involving symbol macros.
>
> (cl-symbol-macrolet ((c a))
>   (let ((a 1) (b 2))
>     (cl-psetq a b
>               b c)
>     (list a b)))

There is a second case that also fails: a symbol macro at a PLACE
position:

(cl-symbol-macrolet ((c a))
  (let ((a 1))
    (cl-psetf c 2
              b a)
    (list a b)))

==> (2 2)  ;should be (2 1)

Michael.

bug#56739: 29.0.50; `cl-psetq' and `cl-psetf' fail to recognize symbol macros

[Michael Heerdegen]

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

Hello,

I think we can fix this (both cases mentioned) similarly as in cl-letf:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP-Fix-symbol-macros-used-in-cl-psetf-Bug-56739.patch --]
[-- Type: text/x-diff, Size: 1447 bytes --]

From 29ea21a751ab6e71b2fb34c781131e31fc7b950d Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Wed, 3 Aug 2022 02:06:16 +0200
Subject: [PATCH] WIP: Fix symbol macros used in cl-psetf (Bug#56739)

---
 lisp/emacs-lisp/cl-macs.el | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index 78d19db479..f3051752ba 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -2653,12 +2653,17 @@ cl-psetf

 \(fn PLACE VAL PLACE VAL ...)"
   (declare (debug setf))
-  (let ((p args) (simple t) (vars nil))
+  (let ((p args) (simple t) (vars nil)
+        (smacros (alist-get :cl-symbol-macros macroexpand-all-environment)))
     (while p
-      (if (or (not (symbolp (car p))) (cl--expr-depends-p (nth 1 p) vars))
-	  (setq simple nil))
-      (if (memq (car p) vars)
-	  (error "Destination duplicated in psetf: %s" (car p)))
+      (when (or (not (symbolp (car p)))
+                (assq (car p) smacros)
+                (and (symbolp (nth 1 p))
+                     (assq (nth 1 p) smacros))
+                (cl--expr-depends-p (nth 1 p) vars))
+	(setq simple nil))
+      (when (memq (car p) vars)
+	(error "Destination duplicated in psetf: %s" (car p)))
       (push (pop p) vars)
       (or p (error "Odd number of arguments to cl-psetf"))
       (pop p))
--
2.30.2


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


But I'm not a fan of symbol macros any more: the concept sounds nice
first, but actually you only save one pair of parens when coding while
they introduce a special case that one always has too keep in mind for
macro expansions: any symbol might not just be a symbol.

I guess this is not the only place where they are not handled correctly.

Michael.

bug#56739: 29.0.50; `cl-psetq' and `cl-psetf' fail to recognize symbol macros

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> I think we can fix this (both cases mentioned) similarly as in cl-letf:
>
> From 29ea21a751ab6e71b2fb34c781131e31fc7b950d Mon Sep 17 00:00:00 2001
> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Wed, 3 Aug 2022 02:06:16 +0200
> Subject: [PATCH] WIP: Fix symbol macros used in cl-psetf (Bug#56739)

This was a month ago, but as far as I can tell, this patch was never
pushed.

Perhaps Stefan has some comments here; added to the CCs.

bug#56739: 29.0.50; `cl-psetq' and `cl-psetf' fail to recognize symbol macros

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> This was a month ago, but as far as I can tell, this patch was never
> pushed.

I didn't want to push without at least one person saying "looks good".

Michael.

bug#56739: 29.0.50; `cl-psetq' and `cl-psetf' fail to recognize symbol macros

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

> -  (let ((p args) (simple t) (vars nil))
> +  (let ((p args) (simple t) (vars nil)
> +        (smacros (alist-get :cl-symbol-macros macroexpand-all-environment)))
>      (while p
> -      (if (or (not (symbolp (car p))) (cl--expr-depends-p (nth 1 p) vars))
> -	  (setq simple nil))
> -      (if (memq (car p) vars)
> -	  (error "Destination duplicated in psetf: %s" (car p)))
> +      (when (or (not (symbolp (car p)))
> +                (assq (car p) smacros)

This looks like a safe way to make it work when the place is
a symbol macro.

> +                (and (symbolp (nth 1 p))
> +                     (assq (nth 1 p) smacros))
> +                (cl--expr-depends-p (nth 1 p) vars))

But this doesn't look strong enough; if (nth 1 p) is (+ c c) where `c`
is a symbol macro the same problem will appear.  So the test needs to be
pushed into `cl--expr-depends-p` (probably into `cl--safe-expr-p` where
it will fix other related problems elsewhere).


        Stefan

bug#56739: 29.0.50; `cl-psetq' and `cl-psetf' fail to recognize symbol macros

[Stefan Kangas]

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

>> -  (let ((p args) (simple t) (vars nil))
>> +  (let ((p args) (simple t) (vars nil)
>> +        (smacros (alist-get :cl-symbol-macros macroexpand-all-environment)))
>>      (while p
>> -      (if (or (not (symbolp (car p))) (cl--expr-depends-p (nth 1 p) vars))
>> -	  (setq simple nil))
>> -      (if (memq (car p) vars)
>> -	  (error "Destination duplicated in psetf: %s" (car p)))
>> +      (when (or (not (symbolp (car p)))
>> +                (assq (car p) smacros)
>
> This looks like a safe way to make it work when the place is
> a symbol macro.
>
>> +                (and (symbolp (nth 1 p))
>> +                     (assq (nth 1 p) smacros))
>> +                (cl--expr-depends-p (nth 1 p) vars))
>
> But this doesn't look strong enough; if (nth 1 p) is (+ c c) where `c`
> is a symbol macro the same problem will appear.  So the test needs to be
> pushed into `cl--expr-depends-p` (probably into `cl--safe-expr-p` where
> it will fix other related problems elsewhere).

(That was one year ago.)

Michael, did you have a chance to look into this?  It would be nice to
get this bug fixed.

bug#56739: 29.0.50; `cl-psetq' and `cl-psetf' fail to recognize symbol macros

[Michael Heerdegen]

Stefan Kangas <stefankangas@gmail.com> writes:

> Michael, did you have a chance to look into this?  It would be nice to
> get this bug fixed.

I would rather leave this one to Stefan to be honest because I'm only
partially understanding what is needed.

Michael.

bug#56739: 29.0.50; `cl-psetq' and `cl-psetf' fail to recognize symbol macros

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> > Michael, did you have a chance to look into this?  It would be nice to
> > get this bug fixed.
>
> I would rather leave this one to Stefan to be honest because I'm only
> partially understanding what is needed.

Or maybe Gerd can help?

Don't want to bother you Gerd, but maybe you are even interested in this
one.


Regards,

Michael.

bug#56739: 29.0.50; `cl-psetq' and `cl-psetf' fail to recognize symbol macros

[Gerd Möllmann]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Michael Heerdegen <michael_heerdegen@web.de> writes:
>
>> > Michael, did you have a chance to look into this?  It would be nice to
>> > get this bug fixed.
>>
>> I would rather leave this one to Stefan to be honest because I'm only
>> partially understanding what is needed.
>
> Or maybe Gerd can help?

Not sure I can help.

In CL, all the p?set[qf] handle symbol macros.  I haven't tested this,
but from doc strings I'd say none of these supports symbol macros in
Emacs.

There is also an intricacy here:

Setq, with CL behaviour, would behave like setf for symbol macros.  And
setq is in C.  I don't know if maintainers would accept such a change in
C (performance, dependencies, ...).

So...

bug#56739: 29.0.50; `cl-psetq' and `cl-psetf' fail to recognize symbol macros

[Wing Hei Chan]

Resending this message as I didn't send it to the thread.

 > Afaik, *left-hand* side symbol macros are all handled correctly:
 >
 > - `setq' is handled by `cl-symbol-macrolet' itself;
 > - `setf' expands symbol macros as needed;
 > - `cl-setq' and `cl-setf' expand to `setf'.
 >
 > The original bug is about *right-hand* side symbol macros, namely that
 > `cl-setq' and `cl-setf' can introduce an unsound optimization in such
 > cases.  In fact, I'm not sure this optimization is needed anymore (see
 > Stefan's comment).  The simplest way to fix the bug is to remove the
 > optimization, but I'll leave the decision to more knowledgeable people.

bug#56739: 29.0.50; `cl-psetq' and `cl-psetf' fail to recognize symbol macros

[Gerd Möllmann]

Wing Hei Chan <whmunkchan@outlook.com> writes:

> Resending this message as I didn't send it to the thread.
>
>  > Afaik, *left-hand* side symbol macros are all handled correctly:
>  >
>  > - `setq' is handled by `cl-symbol-macrolet' itself;
>  > - `setf' expands symbol macros as needed;
>  > - `cl-setq' and `cl-setf' expand to `setf'.
>  >
>  > The original bug is about *right-hand* side symbol macros, namely that
>  > `cl-setq' and `cl-setf' can introduce an unsound optimization in such
>  > cases.  In fact, I'm not sure this optimization is needed anymore (see
>  > Stefan's comment).  The simplest way to fix the bug is to remove the
>  > optimization, but I'll leave the decision to more knowledgeable people.

Then I've possibly misunderstood what this is about, sorry.

bug#56739: 29.0.50; `cl-psetq' and `cl-psetf' fail to recognize symbol macros

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

> Setq, with CL behaviour, would behave like setf for symbol macros.  And
> setq is in C.  I don't know if maintainers would accept such a change in
> C (performance, dependencies, ...).

Quoting cl-macs.el:

    [...]
    (defun cl--sm-macroexpand-1 (orig-fun exp &optional env)
    [...]
           ;; Convert setq to setf if required by symbol-macro expansion.
    [...]


-- Stefan

bug#56739: 29.0.50; `cl-psetq' and `cl-psetf' fail to recognize symbol macros

[Gerd Möllmann]

Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

>> Setq, with CL behaviour, would behave like setf for symbol macros.  And
>> setq is in C.  I don't know if maintainers would accept such a change in
>> C (performance, dependencies, ...).
>
> Quoting cl-macs.el:
>
>     [...]
>     (defun cl--sm-macroexpand-1 (orig-fun exp &optional env)
>     [...]
>            ;; Convert setq to setf if required by symbol-macro expansion.
>     [...]

Clever :-)

bug#56739: 29.0.50; `cl-psetq' and `cl-psetf' fail to recognize symbol macros

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

> The following form produces (2 2) due to the failure of detecting
> dependencies involving symbol macros.
>
> (cl-symbol-macrolet ((c a))
>   (let ((a 1) (b 2))
>     (cl-psetq a b
>               b c)
>     (list a b)))

BTW, another way to get similar problems is with `defvaralias`:

    (defvaralias 'my-c 'my-a)
    (defvar my-a 1)

    (let ((b 2))
      (cl-psetq my-a b
                b my-c)
      (list my-a b))

We should probably introduce a `(macroexp|cl)--simple-var-p` test to
catch the various cases where symbols aren't simple variables.

Another thing that might be worth digging into is how important is that
`cl-psetf` optimization for lexical variables (remember: this was
written back in the days of dynamic scoping and Mattias has made some
improvements to the compiler which may render this optimization
redundant inside the macro itself.


        Stefan