bug#61962: 30.0.50; New trouble with symbols with positions

bug#61962: 30.0.50; New trouble with symbols with positions

[Michael Heerdegen]

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


Hello,

since a couple of days, maybe around a week, master builds bug me with
symbols with positions again.

Here is a recipe for emacs -Q (dunno, there may be much simpler and
shorter recipes, didn't try):

Save this into a file:


[-- Attachment #2: swp-test.el --]
[-- Type: application/emacs-lisp, Size: 413 bytes --]

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


Visit and M-x eval-buffer.  M-: (test) yields xxx as expected.

But now compile (C-c C-b) and M-: (test) unexpectedly (I guess?)
yields (#<symbol xxx at 63>), a symbol with position.

TIA,

Michael.


In GNU Emacs 30.0.50 (build 11, x86_64-pc-linux-gnu, cairo version
 1.16.0) of 2023-03-04 built on drachen
Repository revision: 1b726f26986fa54751f613d1e332fd20c705e17f
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=no'

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LCMS2 LIBSELINUX LIBXML2 MODULES NOTIFY INOTIFY OLDXMENU PDUMPER PNG
RSVG SECCOMP SOUND SQLITE3 THREADS TIFF WEBP X11 XDBE XIM XINPUT2 XPM
ZLIB

bug#61962: 30.0.50; New trouble with symbols with positions

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Sat, 04 Mar 2023 17:18:43 +0100
> 
> since a couple of days, maybe around a week, master builds bug me with
> symbols with positions again.
> 
> Here is a recipe for emacs -Q (dunno, there may be much simpler and
> shorter recipes, didn't try):
> 
> Save this into a file:
> 
> ;; -*- lexical-binding: t -*-
> 
> (cl-defstruct teststruct "Doc" xxx)
> 
> (defun test ()
>   (let ((val (make-teststruct :xxx 20)))
>     (pcase val
>       ((and (pred cl-struct-p)
>             (let class-name (type-of val)))
>        (when class-name
>          (ignore-errors
>            (mapcar #'cl--slot-descriptor-name
>                    (cl--class-slots
>                     (cl-find-class class-name)))))))))
> 
> Visit and M-x eval-buffer.  M-: (test) yields xxx as expected.
> 
> But now compile (C-c C-b) and M-: (test) unexpectedly (I guess?)
> yields (#<symbol xxx at 63>), a symbol with position.

Adding Alan and Mattias.

bug#61962: 30.0.50; New trouble with symbols with positions

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Sat, 04 Mar 2023 17:18:43 +0100
> 
> Here is a recipe for emacs -Q

(Doesn't really work from "emacs -Q", since cl-lib needs to be loaded
first.)

bug#61962: 30.0.50; New trouble with symbols with positions

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> > Here is a recipe for emacs -Q
>
> (Doesn't really work from "emacs -Q", since cl-lib needs to be loaded
> first.)

Oh - thanks.  I visited the file from dired, in that variant the file
loaded without loading cl-lib explicitly.

Michael.

bug#61962: 30.0.50; New trouble with symbols with positions

[Mattias Engdegård]

>> since a couple of days, maybe around a week, master builds bug me with
>> symbols with positions again.

That would very likely be fcf2f7aead.

bug#61962: 30.0.50; New trouble with symbols with positions

[Michael Heerdegen]

Mattias Engdegård <mattiase@acm.org> writes:

> >> since a couple of days, maybe around a week, master builds bug me with
> >> symbols with positions again.
>
> That would very likely be fcf2f7aead.

Yes, reverting that commit fixes the issues.

Michael.

bug#61962: 30.0.50; New trouble with symbols with positions

[Michael Heerdegen]

Mattias Engdegård <mattiase@acm.org> writes:

> That would very likely be fcf2f7aead.

When this issue is not easy to fix, maybe consider to revert that commit
for now (inexact compiler warning positions are less problematic than
breaking Elisp code).

Michael.

bug#61962: 30.0.50; New trouble with symbols with positions

[Alan Mackenzie]

Hello, Michael.

On Sun, Mar 05, 2023 at 17:04:12 +0100, Michael Heerdegen wrote:
> Mattias Engdegård <mattiase@acm.org> writes:

> > That would very likely be fcf2f7aead.

> When this issue is not easy to fix, ....

I will look into this in the next day or two.

> .... maybe consider to revert that commit for now (inexact compiler
> warning positions are less problematic than breaking Elisp code).

I disagree with your concept here.  Warning positions are not accurate
or inaccurate (they cannot be +- 2%, for example), they are either
correct or they are wrong.  A lot of effort was put into making them
correct, although it is clear from this bug that that project is as yet
incomplete.

Please don't revert that commit from 2023-02-17.  I will look into this
and try to fix the bug properly.

> Michael.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#61962: 30.0.50; New trouble with symbols with positions

[Michael Heerdegen]

Alan Mackenzie <acm@muc.de> writes:

> I disagree with your concept here.  Warning positions are not accurate
> or inaccurate (they cannot be +- 2%, for example), they are either
> correct or they are wrong.  A lot of effort was put into making them
> correct, although it is clear from this bug that that project is as yet
> incomplete.

This was not intended to sound like it did to you.  I see the
exact compiler warning positions as a big improvement.

> Please don't revert that commit from 2023-02-17.  I will look into
> this and try to fix the bug properly.

I wanted to spare others from seeing these hard to interpret errors
this commit introduces.  Anyway, take your time, a few days won't hurt.

Michael.

bug#61962: 30.0.50; New trouble with symbols with positions

[Alan Mackenzie]

Hello, Michael.

On Sun, Mar 05, 2023 at 20:41:48 +0100, Michael Heerdegen wrote:
> Alan Mackenzie <acm@muc.de> writes:

> > I disagree with your concept here.  Warning positions are not accurate
> > or inaccurate (they cannot be +- 2%, for example), they are either
> > correct or they are wrong.  A lot of effort was put into making them
> > correct, although it is clear from this bug that that project is as yet
> > incomplete.

> This was not intended to sound like it did to you.  I see the
> exact compiler warning positions as a big improvement.

Sorry, I overreacted there.

> > Please don't revert that commit from 2023-02-17.  I will look into
> > this and try to fix the bug properly.

> I wanted to spare others from seeing these hard to interpret errors
> this commit introduces.  Anyway, take your time, a few days won't hurt.

I think I now understand what's going on.  It's all to do with stripping
symbol positions in eval-and-compile forms.  Before the patch of ~two
weeks ago, the positions were stripped in e-and-c.  After the patch,
they weren't stripped.

I think the correct thing to do is to strip the symbol positions in the
`eval' part of eval-and-compile, but leave them alone in the `compile'
part.  This is actually quite tricky, since
byte-run-strip-symbol-positions works destructively.  So I need to copy
the code first, and there is no suitable function to do this.  copy-tree
is close, but can't handle circular lists.  So I will have to write a
safe version of copy tree.

In the mean time, could you try out the following patch which uses
copy-tree as a first approximation.  I think it fixes the problem, apart
from the above.

Thanks!



diff --git a/lisp/emacs-lisp/bytecomp.el b/lisp/emacs-lisp/bytecomp.el
index 6f3d7a70903..30f58eeb731 100644
--- a/lisp/emacs-lisp/bytecomp.el
+++ b/lisp/emacs-lisp/bytecomp.el
@@ -533,7 +533,9 @@ byte-compile-initial-macro-environment
                                       (macroexpand--all-toplevel
                                        form
                                        macroexpand-all-environment)))
-                                (eval expanded lexical-binding)
+                                (eval (byte-run-strip-symbol-positions
+                                       (copy-tree expanded))
+                                      lexical-binding)
                                 expanded)))))
     (with-suppressed-warnings
         . ,(lambda (warnings &rest body)


> Michael.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#61962: 30.0.50; New trouble with symbols with positions

[Michael Heerdegen]

Alan Mackenzie <acm@muc.de> writes:

> I think I now understand what's going on.  It's all to do with stripping
> symbol positions in eval-and-compile forms.  Before the patch of ~two
> weeks ago, the positions were stripped in e-and-c.  After the patch,
> they weren't stripped.
>
> I think the correct thing to do is to strip the symbol positions in the
> `eval' part of eval-and-compile, but leave them alone in the `compile'
> part.  This is actually quite tricky, since
> byte-run-strip-symbol-positions works destructively.  So I need to copy
> the code first, and there is no suitable function to do this.  copy-tree
> is close, but can't handle circular lists.  So I will have to write a
> safe version of copy tree.

Sounds all plausible.  I also don't have a better idea.


> In the mean time, could you try out the following patch which uses
> copy-tree as a first approximation.  I think it fixes the problem,
> apart from the above.

Yes, looks good.

I wonder now if other cases also suffer from the problem.  What happens
when I call `eval' in a macro expander (i.e. while generating the macro
expansion, not in the result of an expansion)?  And how does
`cl-eval-when' behave (this is actually a special case of the first
question) ?


Thanks so far,

Michael.

bug#61962: 30.0.50; New trouble with symbols with positions

[Alan Mackenzie]

Hello, Michael.

On Tue, Mar 07, 2023 at 01:29:21 +0100, Michael Heerdegen wrote:
> Alan Mackenzie <acm@muc.de> writes:

> > I think I now understand what's going on.  It's all to do with stripping
> > symbol positions in eval-and-compile forms.  Before the patch of ~two
> > weeks ago, the positions were stripped in e-and-c.  After the patch,
> > they weren't stripped.

> > I think the correct thing to do is to strip the symbol positions in the
> > `eval' part of eval-and-compile, but leave them alone in the `compile'
> > part.  This is actually quite tricky, since
> > byte-run-strip-symbol-positions works destructively.  So I need to copy
> > the code first, and there is no suitable function to do this.  copy-tree
> > is close, but can't handle circular lists.  So I will have to write a
> > safe version of copy tree.

> Sounds all plausible.  I also don't have a better idea.

I've now written safe-copy-tree, and committed it together with the fix
in bytecomp.el to master.  So I'm closing the bug with this post.

> > In the mean time, could you try out the following patch which uses
> > copy-tree as a first approximation.  I think it fixes the problem,
> > apart from the above.

> Yes, looks good.

Thanks!

> I wonder now if other cases also suffer from the problem.  What happens
> when I call `eval' in a macro expander (i.e. while generating the macro
> expansion, not in the result of an expansion)?  And how does
> `cl-eval-when' behave (this is actually a special case of the first
> question) ?

I think these are so far unsolved problems with the
symbols-with-position mechanism - sometimes the s-w-p leaks out of macro
contexts.  Are you seeing this problem in real life?

> Thanks so far,

> Michael.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#61962: 30.0.50; New trouble with symbols with positions

[Eli Zaretskii]

> Date: Tue, 7 Mar 2023 10:24:41 +0000
> Cc: Mattias Engdegård <mattiase@acm.org>,
>   Eli Zaretskii <eliz@gnu.org>, 61962-done@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> I've now written safe-copy-tree, and committed it together with the fix
> in bytecomp.el to master.

Next time when you post a patch and ask for comments, please allow
some time for responses, including to those who might be in different
time zones or have less free time on their hands.  13 hours you waited
is definitely not enough.

Btw, what are these "NEW STOUGH" markers you added to bytecomp.el:

+;;;; NEW STOUGH, 2023-03-05
+                    (byte-run-strip-symbol-positions
+;;;; END OF NEW STOUGH
                    (byte-compile-sexp
                      (let ((form (read-positioning-symbols (current-buffer))))
                        (push form byte-compile-form-stack)
                        (eval-sexp-add-defvars
                         form
-                        start-read-position))))
+                        start-read-position)))
+;;;; NEW STOUGH, 2023-03-05
+                    )
+;;;; END OF NEW STOUGH
+                                              )

Also, how about adding some tests, to make sure we don't regress in
this area in the future?

bug#61962: 30.0.50; New trouble with symbols with positions

[Robert Pluim]

>>>>> On Tue, 07 Mar 2023 15:13:40 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> Date: Tue, 7 Mar 2023 10:24:41 +0000
    >> Cc: Mattias Engdegård <mattiase@acm.org>,
    >> Eli Zaretskii <eliz@gnu.org>, 61962-done@debbugs.gnu.org
    >> From: Alan Mackenzie <acm@muc.de>
    >> 
    >> I've now written safe-copy-tree, and committed it together with the fix
    >> in bytecomp.el to master.

    Eli> Next time when you post a patch and ask for comments, please allow
    Eli> some time for responses, including to those who might be in different
    Eli> time zones or have less free time on their hands.  13 hours you waited
    Eli> is definitely not enough.

Yes. I was going to ask "why canʼt copy-tree be fixed to support
circular lists instead of making people think about which function to
use?".

Robert
-- 

bug#61962: 30.0.50; New trouble with symbols with positions

[Michael Heerdegen]

Alan Mackenzie <acm@muc.de> writes:

> I've now written safe-copy-tree, and committed it together with the fix
> in bytecomp.el to master.

Thanks.  Works well for me.

One note: the function fails for deeply nested structures because it
hits the recursion limit, e.g. for

#+begin_src emacs-lisp
(let ((my-list (list 1)))
  (dotimes (i 10000)
    (setq my-list (list my-list)))
  (safe-copy-tree my-list))
#+end_src

> > I wonder now if other cases also suffer from the problem.  What happens
> > when I call `eval' in a macro expander (i.e. while generating the macro
> > expansion, not in the result of an expansion)?  And how does
> > `cl-eval-when' behave (this is actually a special case of the first
> > question) ?
>
> I think these are so far unsolved problems with the
> symbols-with-position mechanism - sometimes the s-w-p leaks out of macro
> contexts.  Are you seeing this problem in real life?

So far, not that I knew, no.  I'll keep my eyes open.

Thanks,

Michael.

bug#61962: 30.0.50; New trouble with symbols with positions

[Alan Mackenzie]

Hello, Eli.

On Tue, Mar 07, 2023 at 15:13:40 +0200, Eli Zaretskii wrote:
> > Date: Tue, 7 Mar 2023 10:24:41 +0000
> > Cc: Mattias Engdegård <mattiase@acm.org>,
> >   Eli Zaretskii <eliz@gnu.org>, 61962-done@debbugs.gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > I've now written safe-copy-tree, and committed it together with the fix
> > in bytecomp.el to master.

> Next time when you post a patch and ask for comments, please allow
> some time for responses, including to those who might be in different
> time zones or have less free time on their hands.  13 hours you waited
> is definitely not enough.

Yes.  For some reason I was in a bit of a hurry to close the bug.

> Btw, what are these "NEW STOUGH" markers you added to bytecomp.el:

> +;;;; NEW STOUGH, 2023-03-05
> +                    (byte-run-strip-symbol-positions
> +;;;; END OF NEW STOUGH
>                     (byte-compile-sexp
>                       (let ((form (read-positioning-symbols (current-buffer))))
>                         (push form byte-compile-form-stack)
>                         (eval-sexp-add-defvars
>                          form
> -                        start-read-position))))
> +                        start-read-position)))
> +;;;; NEW STOUGH, 2023-03-05
> +                    )
> +;;;; END OF NEW STOUGH
> +                                              )

A change I didn't intend to commit, now tidied up and removed.  I've
also tidied up the documentation, and now delete the hash table at the
end of the function, as you suggested in another post.

> Also, how about adding some tests, to make sure we don't regress in
> this area in the future?

Good idea!  I'll see what I can manage.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#61962: 30.0.50; New trouble with symbols with positions

[Alan Mackenzie]

Hello, Robert.

On Tue, Mar 07, 2023 at 14:51:38 +0100, Robert Pluim wrote:
> >>>>> On Tue, 07 Mar 2023 15:13:40 +0200, Eli Zaretskii <eliz@gnu.org> said:

>     >> Date: Tue, 7 Mar 2023 10:24:41 +0000
>     >> Cc: Mattias Engdegård <mattiase@acm.org>,
>     >> Eli Zaretskii <eliz@gnu.org>, 61962-done@debbugs.gnu.org
>     >> From: Alan Mackenzie <acm@muc.de>

>     >> I've now written safe-copy-tree, and committed it together with the fix
>     >> in bytecomp.el to master.

>     Eli> Next time when you post a patch and ask for comments, please allow
>     Eli> some time for responses, including to those who might be in different
>     Eli> time zones or have less free time on their hands.  13 hours you waited
>     Eli> is definitely not enough.

> Yes. I was going to ask "why canʼt copy-tree be fixed to support
> circular lists instead of making people think about which function to
> use?".

safe-copy-tree is slower than copy-tree, probably _much_ slower, though
I haven't measured it.  If there's no possibility of circular lists,
copy-tree will be far the better function to use.

> Robert
> -- 

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#61962: 30.0.50; New trouble with symbols with positions

[Mattias Engdegård]

As promised earlier, I gave the safe-copy-tree code a good working-through. Testing revealed bugs but the new implementation shouldn't have them. It's internal for now as there seems to be no need for it elsewhere, which also permitted some gold-plating to be removed. The new code is also quite a bit faster.

bug#61962: 30.0.50; New trouble with symbols with positions

[Alan Mackenzie]

Hello, Mattias.

On Sun, Mar 12, 2023 at 18:30:51 +0100, Mattias Engdegård wrote:
> As promised earlier, I gave the safe-copy-tree code a good
> working-through. Testing revealed bugs but the new implementation
> shouldn't have them. It's internal for now as there seems to be no need
> for it elsewhere, which also permitted some gold-plating to be removed.
> The new code is also quite a bit faster.

I'm not at all happy with the changes you've made.  You've transformed a
general purpose utility into a special purpose restricted one.  It was me
that put the work in in the first place, and I wonder why.  It was a
substantial amount of work, and it would appear to have been for nothing.

Why did you not talk to me about the changes you were intending to make?
You've simply overridden my judgment with your own in cutting the scope
of the new function down.  Why?  I thoroughly disagree with you that no
general purpose copy-tree is needed (I've lamented its lack before now),
and I thoroughly disagree with you that vectors and records need never be
copied.

You say there were bugs with my version.  OK, thanks for correcting them,
but would you please identify exactly what the bugs were.  How else am I
supposed to learn?

You say your new version is "quite a bit" faster.  What does that mean?
A factor of 10?  A factor of 2?  20%?  10%?  How did you measure this
speed up, and what particular code change was responsible for it?
Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#61962: 30.0.50; New trouble with symbols with positions

[Eli Zaretskii]

> Date: Sun, 12 Mar 2023 20:42:25 +0000
> Cc: Robert Pluim <rpluim@gmail.com>, Eli Zaretskii <eliz@gnu.org>,
>   Michael Heerdegen <michael_heerdegen@web.de>, 61962@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> On Sun, Mar 12, 2023 at 18:30:51 +0100, Mattias Engdegård wrote:
> > As promised earlier, I gave the safe-copy-tree code a good
> > working-through. Testing revealed bugs but the new implementation
> > shouldn't have them. It's internal for now as there seems to be no need
> > for it elsewhere, which also permitted some gold-plating to be removed.
> > The new code is also quite a bit faster.
> 
> I'm not at all happy with the changes you've made.

I'm not happy at all.

Mattias, this kind of modus operandi is only acceptable if the
original author installed something completely unreasonable or clearly
broken.  In all other cases, please present the proposed changes and
their motivation, and wait for the discussion to converge before
installing.

Please don't do this again.  Bonus points for reverting your changes
and starting the discussion now.

bug#61962: 30.0.50; New trouble with symbols with positions

[Mattias Engdegård]

12 mars 2023 kl. 21.42 skrev Alan Mackenzie <acm@muc.de>:

> I'm not at all happy with the changes you've made.

Don't worry, Alan, we're writing software, not carving granite. Anything can be changed, and I'm not out to get you!
There seems to be some misunderstanding on both sides so let's clear that up.

It was far from obvious that your making `safe-copy-tree` a user-visible utility arose from a particular need rather than just being easy to do -- it wouldn't be the first time.

If you really have a good reason to add safe-copy-tree then I'm certainly not against it, but you haven't made much of a case for it. It seems to have made its way in on the slip-stream of a bug fix. I found no justification for it, so please forgive me for assuming there wasn't any.

The status quo is something everybody can agree is an improvement: a bug has been fixed and that's it. Now if you want Emacs to come with a standard circular tree copy function then it needs to go in on its own merits: please argue why and how. Concrete examples would be useful.

If added, your `safe-copy-tree` (wouldn't `copy-graph` be a more accurate name?) should not be forced to share the implementation of what is now `bytecomp--copy-tree`; that would just introduce unnecessary compromises for either use.

> would you please identify exactly what the bugs were.

I only know that some of my (probably insufficient) tests failed for the old implementation, for example ((a . #1=(b)) #1#) -- I didn't dig deeper.