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

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