bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Emacs.

The code in question is in internal_equal (src/fns.c) ~37 lines from its
start.  We have:

  if (SYMBOL_WITH_POS_P (o1))
    o1 = SYMBOL_WITH_POS_SYM (o1);
  if (SYMBOL_WITH_POS_P (o2))
    o2 = SYMBOL_WITH_POS_SYM (o2);

at the top level of the function.  Thus

    (equal 'foo #<symbol foo at 42>)

will return non-nil, regardless of the setting of
symbols-with-pos-enabled.  It should return non-nil only when that flag
variable is non-nil.  This is a bug.

#########################################################################

A simple fix is:



diff --git a/src/fns.c b/src/fns.c
index bfd19e8c8f2..b1f0c4ecdd6 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2773,10 +2773,13 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
 
   /* A symbol with position compares the contained symbol, and is
      `equal' to the corresponding ordinary symbol.  */
-  if (SYMBOL_WITH_POS_P (o1))
-    o1 = SYMBOL_WITH_POS_SYM (o1);
-  if (SYMBOL_WITH_POS_P (o2))
-    o2 = SYMBOL_WITH_POS_SYM (o2);
+  if (symbols_with_pos_enabled)
+    {
+      if (SYMBOL_WITH_POS_P (o1))
+	o1 = SYMBOL_WITH_POS_SYM (o1);
+      if (SYMBOL_WITH_POS_P (o2))
+	o2 = SYMBOL_WITH_POS_SYM (o2);
+    }
 
   if (BASE_EQ (o1, o2))
     return true;


-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Eli Zaretskii]

> Date: Fri, 4 Aug 2023 14:00:29 +0000
> From: Alan Mackenzie <acm@muc.de>
> 
> Hello, Emacs.
> 
> The code in question is in internal_equal (src/fns.c) ~37 lines from its
> start.  We have:
> 
>   if (SYMBOL_WITH_POS_P (o1))
>     o1 = SYMBOL_WITH_POS_SYM (o1);
>   if (SYMBOL_WITH_POS_P (o2))
>     o2 = SYMBOL_WITH_POS_SYM (o2);
> 
> at the top level of the function.  Thus
> 
>     (equal 'foo #<symbol foo at 42>)
> 
> will return non-nil, regardless of the setting of
> symbols-with-pos-enabled.  It should return non-nil only when that flag
> variable is non-nil.  This is a bug.
> 
> #########################################################################
> 
> A simple fix is:

What will happen to the comparison in internal_equal when
symbols_with_pos_enabled is zero and the two objects have different
positions, or one has a position, the other doesn't?

And which branch are you proposing this change for?

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Eli.

On Fri, Aug 04, 2023 at 17:32:35 +0300, Eli Zaretskii wrote:
> > Date: Fri, 4 Aug 2023 14:00:29 +0000
> > From: Alan Mackenzie <acm@muc.de>

> > Hello, Emacs.

> > The code in question is in internal_equal (src/fns.c) ~37 lines from its
> > start.  We have:

> >   if (SYMBOL_WITH_POS_P (o1))
> >     o1 = SYMBOL_WITH_POS_SYM (o1);
> >   if (SYMBOL_WITH_POS_P (o2))
> >     o2 = SYMBOL_WITH_POS_SYM (o2);

> > at the top level of the function.  Thus

> >     (equal 'foo #<symbol foo at 42>)

> > will return non-nil, regardless of the setting of
> > symbols-with-pos-enabled.  It should return non-nil only when that flag
> > variable is non-nil.  This is a bug.

> > #########################################################################

> > A simple fix is:

> What will happen to the comparison in internal_equal when
> symbols_with_pos_enabled is zero and the two objects have different
> positions, or one has a position, the other doesn't?

In these cases, equal will return nil.  This is correct.

In the other case, when two symbols with position have the same base
symbol and the same position, yet aren't identical, this will also return
nil, which is incorrect.  Do you think this case is worth bothering
about?  It could easily be amended.

> And which branch are you proposing this change for?

master.  It doesn't seem important enough for the release branch.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Eli Zaretskii]

> Date: Fri, 4 Aug 2023 14:59:58 +0000
> Cc: 65051@debbugs.gnu.org, acm@muc.de
> From: Alan Mackenzie <acm@muc.de>
> 
> > What will happen to the comparison in internal_equal when
> > symbols_with_pos_enabled is zero and the two objects have different
> > positions, or one has a position, the other doesn't?
> 
> In these cases, equal will return nil.  This is correct.

It is?  I thought when symbols with position are disabled, symbols
that are 'eq', but have different positions, should compare equal?
Why not?

> In the other case, when two symbols with position have the same base
> symbol and the same position, yet aren't identical, this will also return
> nil, which is incorrect.

How can they be not identical if the symbols and the positions are the
same?  Or maybe I don't understand what you mean by "base symbol"?

> > And which branch are you proposing this change for?
> 
> master.  It doesn't seem important enough for the release branch.

OK.

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Eli.

On Fri, Aug 04, 2023 at 18:27:55 +0300, Eli Zaretskii wrote:
> > Date: Fri, 4 Aug 2023 14:59:58 +0000
> > Cc: 65051@debbugs.gnu.org, acm@muc.de
> > From: Alan Mackenzie <acm@muc.de>

> > > What will happen to the comparison in internal_equal when
> > > symbols_with_pos_enabled is zero and the two objects have different
> > > positions, or one has a position, the other doesn't?

> > In these cases, equal will return nil.  This is correct.

> It is?  I thought when symbols with position are disabled, symbols
> that are 'eq', but have different positions, should compare equal?
> Why not?

With symbols-with-pos-enabled nil, #<symbol foo at 42> is not EQ to
#<symbol foo at 666>.  Neither are these two objects `equal'.  This is
because the special, time consuming processing which makes them EQ or
`equal' is enabled by that variable being bound to non-nil.

That's the theory.  In practice, the handling in internal_equal forgot to
check for symbols-with-pos-enabled.  That's what I want to fix, now.

> > In the other case, when two symbols with position have the same base
> > symbol and the same position, yet aren't identical, this will also return
> > nil, which is incorrect.

> How can they be not identical if the symbols and the positions are the
> same?  Or maybe I don't understand what you mean by "base symbol"?

By "base symbol" I mean 'foo in #<symbol foo at 42>.  By "identical" I
meant that the two Lisp_Objects would have the same hex value (i.e. be
EQ without symbols-with-pos-enabled), as contrasted to two distinct
Lisp_Objects with the same base symbol, and the same position, i.e.
should be `equal'.

The way internal_equal is coded, only certain subtypes of Lisp_vectorlike
have their innards compared.  The other subtypes are simply assumed not
`equal'.  PVEC_SYMBOL_WITH_POS currently isn't among these subtypes,
though this could be changed easily enough.

> > > And which branch are you proposing this change for?

> > master.  It doesn't seem important enough for the release branch.

> OK.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Eli Zaretskii]

> Date: Fri, 4 Aug 2023 17:06:10 +0000
> Cc: 65051@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> On Fri, Aug 04, 2023 at 18:27:55 +0300, Eli Zaretskii wrote:
> > > Date: Fri, 4 Aug 2023 14:59:58 +0000
> > > Cc: 65051@debbugs.gnu.org, acm@muc.de
> > > From: Alan Mackenzie <acm@muc.de>
> 
> > > > What will happen to the comparison in internal_equal when
> > > > symbols_with_pos_enabled is zero and the two objects have different
> > > > positions, or one has a position, the other doesn't?
> 
> > > In these cases, equal will return nil.  This is correct.
> 
> > It is?  I thought when symbols with position are disabled, symbols
> > that are 'eq', but have different positions, should compare equal?
> > Why not?
> 
> With symbols-with-pos-enabled nil, #<symbol foo at 42> is not EQ to
> #<symbol foo at 666>.  Neither are these two objects `equal'.  This is
> because the special, time consuming processing which makes them EQ or
> `equal' is enabled by that variable being bound to non-nil.

But I thought that with symbols-with-pos-enabled OFF, we just ignore
the positions?  Truth is, neither the ELisp manual nor the doc string
tell us what happens when this variable is nil, they only tell what
happens when it's non-nil.  So how about documenting that somewhere?

> That's the theory.  In practice, the handling in internal_equal forgot to
> check for symbols-with-pos-enabled.  That's what I want to fix, now.

I understand, but I question the correctness of your proposed fix.
And for now, I'm utterly confused regarding the expected semantics of
these comparisons when symbols-with-pos-enabled is nil.

> > > In the other case, when two symbols with position have the same base
> > > symbol and the same position, yet aren't identical, this will also return
> > > nil, which is incorrect.
> 
> > How can they be not identical if the symbols and the positions are the
> > same?  Or maybe I don't understand what you mean by "base symbol"?
> 
> By "base symbol" I mean 'foo in #<symbol foo at 42>.  By "identical" I
> meant that the two Lisp_Objects would have the same hex value (i.e. be
> EQ without symbols-with-pos-enabled), as contrasted to two distinct
> Lisp_Objects with the same base symbol, and the same position, i.e.
> should be `equal'.

So we can have two different copies of #<symbol foo at 42>, such that
their hex values are different?  Isn't that a bug? why don't we
conflate such identical symbols?

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Eli.

On Fri, Aug 04, 2023 at 21:01:30 +0300, Eli Zaretskii wrote:
> > Date: Fri, 4 Aug 2023 17:06:10 +0000
> > Cc: 65051@debbugs.gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > On Fri, Aug 04, 2023 at 18:27:55 +0300, Eli Zaretskii wrote:
> > > > Date: Fri, 4 Aug 2023 14:59:58 +0000
> > > > Cc: 65051@debbugs.gnu.org, acm@muc.de
> > > > From: Alan Mackenzie <acm@muc.de>

> > > > > What will happen to the comparison in internal_equal when
> > > > > symbols_with_pos_enabled is zero and the two objects have different
> > > > > positions, or one has a position, the other doesn't?

> > > > In these cases, equal will return nil.  This is correct.

> > > It is?  I thought when symbols with position are disabled, symbols
> > > that are 'eq', but have different positions, should compare equal?
> > > Why not?

> > With symbols-with-pos-enabled nil, #<symbol foo at 42> is not EQ to
> > #<symbol foo at 666>.  Neither are these two objects `equal'.  This is
> > because the special, time consuming processing which makes them EQ or
> > `equal' is enabled by that variable being bound to non-nil.

> But I thought that with symbols-with-pos-enabled OFF, we just ignore
> the positions?  Truth is, neither the ELisp manual nor the doc string
> tell us what happens when this variable is nil, they only tell what
> happens when it's non-nil.  So how about documenting that somewhere?

OK, please see the patch below.

> > That's the theory.  In practice, the handling in internal_equal forgot to
> > check for symbols-with-pos-enabled.  That's what I want to fix, now.

> I understand, but I question the correctness of your proposed fix.
> And for now, I'm utterly confused regarding the expected semantics of
> these comparisons when symbols-with-pos-enabled is nil.

> > > > In the other case, when two symbols with position have the same base
> > > > symbol and the same position, yet aren't identical, this will also return
> > > > nil, which is incorrect.

> > > How can they be not identical if the symbols and the positions are the
> > > same?  Or maybe I don't understand what you mean by "base symbol"?

> > By "base symbol" I mean 'foo in #<symbol foo at 42>.  By "identical" I
> > meant that the two Lisp_Objects would have the same hex value (i.e. be
> > EQ without symbols-with-pos-enabled), as contrasted to two distinct
> > Lisp_Objects with the same base symbol, and the same position, i.e.
> > should be `equal'.

> So we can have two different copies of #<symbol foo at 42>, such that
> their hex values are different?  Isn't that a bug? why don't we
> conflate such identical symbols?

No, it's not a bug, anymore than having two `equal' copies of '(a b c)
would be a bug.  It's vanishingly unlikely to happen in practice.  I
think it could only happen if a user creates a SWP with something like

    (setq bar (position-symbol 'foo 42))

, when there is already such a SWP created by read-positioning-symbols.
Or, perhaps, if reading one function produced a certain SWP which hangs
around in a variable such as cl--labels-convert-cache, and reading the
next function produces another SWP which happens to have the same bare
symbol and position.

Anyway, I've added code (in that patch below) to check two SWPs properly
in the event of symbols-with-pos-enabled being nil.

As already discussed, this is intended for master, not the release
branch.



diff --git a/doc/lispref/symbols.texi b/doc/lispref/symbols.texi
index 34db0caf3a8..a6ecfe896ad 100644
--- a/doc/lispref/symbols.texi
+++ b/doc/lispref/symbols.texi
@@ -784,9 +784,11 @@ Symbols with Position
 @cindex bare symbol
 A @dfn{symbol with position} is a symbol, the @dfn{bare symbol},
 together with an unsigned integer called the @dfn{position}.  These
-objects are intended for use by the byte compiler, which records in
+objects are for the use of the byte compiler, which records in
 them the position of each symbol occurrence and uses those positions
-in warning and error messages.
+in warning and error messages.  They shouldn't normally be used
+otherwise.  Doing so can cause unexpected results with basic Emacs
+functions such as @code{eq} and @code{equal}.
 
 The printed representation of a symbol with position uses the hash
 notation outlined in @ref{Printed Representation}.  It looks like
@@ -798,11 +800,20 @@ Symbols with Position
 
 For most purposes, when the flag variable
 @code{symbols-with-pos-enabled} is non-@code{nil}, symbols with
-positions behave just as bare symbols do.  For example, @samp{(eq
-#<symbol foo at 12345> foo)} has a value @code{t} when that variable
-is set (but @code{nil} when it isn't set).  Most of the time in Emacs this
-variable is @code{nil}, but the byte compiler binds it to @code{t}
-when it runs.
+positions behave just as their bare symbols do.  For example,
+@samp{(eq #<symbol foo at 12345> foo)} has a value @code{t} when the
+variable is set; likewise, @code{equal} will treat a symbol with
+position argument as its bare symbol.
+
+When @code{symbols-with-pos-enabled} is @code{nil}, any symbols with
+position continue to exist, but do not behave as symbols, or have the
+other useful properties outlined in the previous paragraph.  @code{eq}
+returns @code{t} when given identical arguments, and @code{equal}
+returns @code{t} when given arguments with @code{equal} components.
+
+Most of the time in Emacs @code{symbols-with-pos-enabled} is
+@code{nil}, but the byte compiler and the native compiler bind it to
+@code{t} when they run.
 
 Typically, symbols with position are created by the byte compiler
 calling the reader function @code{read-positioning-symbols}
@@ -820,7 +831,7 @@ Symbols with Position
 a symbol with position, ignoring the position.
 @end defvar
 
-@defun symbol-with-pos-p symbol.
+@defun symbol-with-pos-p symbol
 This function returns @code{t} if @var{symbol} is a symbol with
 position, @code{nil} otherwise.
 @end defun
diff --git a/src/fns.c b/src/fns.c
index bfd19e8c8f2..d47098c8791 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2773,10 +2773,13 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
 
   /* A symbol with position compares the contained symbol, and is
      `equal' to the corresponding ordinary symbol.  */
-  if (SYMBOL_WITH_POS_P (o1))
-    o1 = SYMBOL_WITH_POS_SYM (o1);
-  if (SYMBOL_WITH_POS_P (o2))
-    o2 = SYMBOL_WITH_POS_SYM (o2);
+  if (symbols_with_pos_enabled)
+    {
+      if (SYMBOL_WITH_POS_P (o1))
+	o1 = SYMBOL_WITH_POS_SYM (o1);
+      if (SYMBOL_WITH_POS_P (o2))
+	o2 = SYMBOL_WITH_POS_SYM (o2);
+    }
 
   if (BASE_EQ (o1, o2))
     return true;
@@ -2824,8 +2827,8 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
 	if (ASIZE (o2) != size)
 	  return false;
 
-	/* Compare bignums, overlays, markers, and boolvectors
-	   specially, by comparing their values.  */
+	/* Compare bignums, overlays, markers, boolvectors, and
+	   symbols with position specially, by comparing their values.  */
 	if (BIGNUMP (o1))
 	  return mpz_cmp (*xbignum_val (o1), *xbignum_val (o2)) == 0;
 	if (OVERLAYP (o1))
@@ -2857,6 +2860,13 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
 	if (TS_NODEP (o1))
 	  return treesit_node_eq (o1, o2);
 #endif
+	if (SYMBOL_WITH_POS_P(o1)) /* symbols_with_pos_enabled is false.  */
+	  return (internal_equal (XSYMBOL_WITH_POS (o1)->sym,
+				  XSYMBOL_WITH_POS (o2)->sym,
+				  equal_kind, depth + 1, ht)
+		  && internal_equal (XSYMBOL_WITH_POS (o1)->pos,
+				     XSYMBOL_WITH_POS (o2)->pos,
+				     equal_kind, depth + 1, ht));
 
 	/* Aside from them, only true vectors, char-tables, compiled
 	   functions, and fonts (font-spec, font-entity, font-object)


-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Eli Zaretskii]

> Date: Sat, 5 Aug 2023 10:45:37 +0000
> Cc: 65051@debbugs.gnu.org, acm@muc.de
> From: Alan Mackenzie <acm@muc.de>
> 
> > > With symbols-with-pos-enabled nil, #<symbol foo at 42> is not EQ to
> > > #<symbol foo at 666>.  Neither are these two objects `equal'.  This is
> > > because the special, time consuming processing which makes them EQ or
> > > `equal' is enabled by that variable being bound to non-nil.
> 
> > But I thought that with symbols-with-pos-enabled OFF, we just ignore
> > the positions?  Truth is, neither the ELisp manual nor the doc string
> > tell us what happens when this variable is nil, they only tell what
> > happens when it's non-nil.  So how about documenting that somewhere?
> 
> OK, please see the patch below.

Thanks.

> > > By "base symbol" I mean 'foo in #<symbol foo at 42>.  By "identical" I
> > > meant that the two Lisp_Objects would have the same hex value (i.e. be
> > > EQ without symbols-with-pos-enabled), as contrasted to two distinct
> > > Lisp_Objects with the same base symbol, and the same position, i.e.
> > > should be `equal'.
> 
> > So we can have two different copies of #<symbol foo at 42>, such that
> > their hex values are different?  Isn't that a bug? why don't we
> > conflate such identical symbols?
> 
> No, it's not a bug, anymore than having two `equal' copies of '(a b c)
> would be a bug.

But with symbols we store them in obarray, and obarray ought to find
the existing slot for #<symbol foo at 42> and reuse it, rather than
create a new slot?

> Anyway, I've added code (in that patch below) to check two SWPs properly
> in the event of symbols-with-pos-enabled being nil.
> 
> As already discussed, this is intended for master, not the release
> branch.

Thanks.  I added Stefan to the discussion, since I'd like another pair
of eyes (with more knowledge about this than mine) to eyeball the
changes before we install them.

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Eli.

On Sat, Aug 05, 2023 at 13:57:50 +0300, Eli Zaretskii wrote:
> > Date: Sat, 5 Aug 2023 10:45:37 +0000
> > Cc: 65051@debbugs.gnu.org, acm@muc.de
> > From: Alan Mackenzie <acm@muc.de>

[ .... ]

> > > So we can have two different copies of #<symbol foo at 42>, such that
> > > their hex values are different?  Isn't that a bug? why don't we
> > > conflate such identical symbols?

> > No, it's not a bug, anymore than having two `equal' copies of '(a b c)
> > would be a bug.

> But with symbols we store them in obarray, and obarray ought to find
> the existing slot for #<symbol foo at 42> and reuse it, rather than
> create a new slot?

No, only the bare symbol is in the obarray.  The symbol with position
itself is a pseudovector, with contents (i) a bare symbol (a Lisp_Object
"pointing at" the obarray) and (ii) the unsigned integer position.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Eli Zaretskii]

> Date: Sat, 5 Aug 2023 11:52:52 +0000
> Cc: 65051@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>,
>   acm@muc.de
> From: Alan Mackenzie <acm@muc.de>
> 
> Hello, Eli.
> 
> On Sat, Aug 05, 2023 at 13:57:50 +0300, Eli Zaretskii wrote:
> > > Date: Sat, 5 Aug 2023 10:45:37 +0000
> > > Cc: 65051@debbugs.gnu.org, acm@muc.de
> > > From: Alan Mackenzie <acm@muc.de>
> 
> [ .... ]
> 
> > > > So we can have two different copies of #<symbol foo at 42>, such that
> > > > their hex values are different?  Isn't that a bug? why don't we
> > > > conflate such identical symbols?
> 
> > > No, it's not a bug, anymore than having two `equal' copies of '(a b c)
> > > would be a bug.
> 
> > But with symbols we store them in obarray, and obarray ought to find
> > the existing slot for #<symbol foo at 42> and reuse it, rather than
> > create a new slot?
> 
> No, only the bare symbol is in the obarray.  The symbol with position
> itself is a pseudovector, with contents (i) a bare symbol (a Lisp_Object
> "pointing at" the obarray) and (ii) the unsigned integer position.

Please document this factoid in the ELisp manual, I think it's very
important, and having it undocumented is a Bad Thing.

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Eli.

On Sat, Aug 05, 2023 at 15:13:22 +0300, Eli Zaretskii wrote:
> > Date: Sat, 5 Aug 2023 11:52:52 +0000
> > Cc: 65051@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>,
> >   acm@muc.de
> > From: Alan Mackenzie <acm@muc.de>

> > Hello, Eli.

> > On Sat, Aug 05, 2023 at 13:57:50 +0300, Eli Zaretskii wrote:
> > > > Date: Sat, 5 Aug 2023 10:45:37 +0000
> > > > Cc: 65051@debbugs.gnu.org, acm@muc.de
> > > > From: Alan Mackenzie <acm@muc.de>

> > [ .... ]

> > > > > So we can have two different copies of #<symbol foo at 42>, such that
> > > > > their hex values are different?  Isn't that a bug? why don't we
> > > > > conflate such identical symbols?

> > > > No, it's not a bug, anymore than having two `equal' copies of '(a b c)
> > > > would be a bug.

> > > But with symbols we store them in obarray, and obarray ought to find
> > > the existing slot for #<symbol foo at 42> and reuse it, rather than
> > > create a new slot?

> > No, only the bare symbol is in the obarray.  The symbol with position
> > itself is a pseudovector, with contents (i) a bare symbol (a Lisp_Object
> > "pointing at" the obarray) and (ii) the unsigned integer position.

> Please document this factoid in the ELisp manual, I think it's very
> important, and having it undocumented is a Bad Thing.

DONE.  The change is near the top of the following patch, amended from
the previous version.



diff --git a/doc/lispref/symbols.texi b/doc/lispref/symbols.texi
index 34db0caf3a8..a828d303c04 100644
--- a/doc/lispref/symbols.texi
+++ b/doc/lispref/symbols.texi
@@ -784,9 +784,15 @@ Symbols with Position
 @cindex bare symbol
 A @dfn{symbol with position} is a symbol, the @dfn{bare symbol},
 together with an unsigned integer called the @dfn{position}.  These
-objects are intended for use by the byte compiler, which records in
-them the position of each symbol occurrence and uses those positions
-in warning and error messages.
+objects are stored internally much like vectors, and don't themselves
+have entries in the obarray (though their bare symbols do;
+@pxref{Creating Symbols}).
+
+Symbols with position are for the use of the byte compiler, which
+records in them the position of each symbol occurrence and uses those
+positions in warning and error messages.  They shouldn't normally be
+used otherwise.  Doing so can cause unexpected results with basic
+Emacs functions such as @code{eq} and @code{equal}.
 
 The printed representation of a symbol with position uses the hash
 notation outlined in @ref{Printed Representation}.  It looks like
@@ -798,11 +804,20 @@ Symbols with Position
 
 For most purposes, when the flag variable
 @code{symbols-with-pos-enabled} is non-@code{nil}, symbols with
-positions behave just as bare symbols do.  For example, @samp{(eq
-#<symbol foo at 12345> foo)} has a value @code{t} when that variable
-is set (but @code{nil} when it isn't set).  Most of the time in Emacs this
-variable is @code{nil}, but the byte compiler binds it to @code{t}
-when it runs.
+positions behave just as their bare symbols would.  For example,
+@samp{(eq #<symbol foo at 12345> foo)} has a value @code{t} when the
+variable is set; likewise, @code{equal} will treat a symbol with
+position argument as its bare symbol.
+
+When @code{symbols-with-pos-enabled} is @code{nil}, any symbols with
+position continue to exist, but do not behave as symbols, or have the
+other useful properties outlined in the previous paragraph.  @code{eq}
+returns @code{t} when given identical arguments, and @code{equal}
+returns @code{t} when given arguments with @code{equal} components.
+
+Most of the time in Emacs @code{symbols-with-pos-enabled} is
+@code{nil}, but the byte compiler and the native compiler bind it to
+@code{t} when they run.
 
 Typically, symbols with position are created by the byte compiler
 calling the reader function @code{read-positioning-symbols}
@@ -820,7 +835,7 @@ Symbols with Position
 a symbol with position, ignoring the position.
 @end defvar
 
-@defun symbol-with-pos-p symbol.
+@defun symbol-with-pos-p symbol
 This function returns @code{t} if @var{symbol} is a symbol with
 position, @code{nil} otherwise.
 @end defun


-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Eli Zaretskii]

> Date: Sat, 5 Aug 2023 13:04:07 +0000
> Cc: 65051@debbugs.gnu.org, monnier@iro.umontreal.ca, acm@muc.de
> From: Alan Mackenzie <acm@muc.de>
> 
> > > No, only the bare symbol is in the obarray.  The symbol with position
> > > itself is a pseudovector, with contents (i) a bare symbol (a Lisp_Object
> > > "pointing at" the obarray) and (ii) the unsigned integer position.
> 
> > Please document this factoid in the ELisp manual, I think it's very
> > important, and having it undocumented is a Bad Thing.
> 
> DONE.  The change is near the top of the following patch, amended from
> the previous version.

LGTM, thanks.

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

On Sat, Aug 05, 2023 at 16:13:13 +0300, Eli Zaretskii wrote:
> > Date: Sat, 5 Aug 2023 13:04:07 +0000
> > Cc: 65051@debbugs.gnu.org, monnier@iro.umontreal.ca, acm@muc.de
> > From: Alan Mackenzie <acm@muc.de>

> > > > No, only the bare symbol is in the obarray.  The symbol with position
> > > > itself is a pseudovector, with contents (i) a bare symbol (a Lisp_Object
> > > > "pointing at" the obarray) and (ii) the unsigned integer position.

> > > Please document this factoid in the ELisp manual, I think it's very
> > > important, and having it undocumented is a Bad Thing.

> > DONE.  The change is near the top of the following patch, amended from
> > the previous version.

> LGTM, thanks.

Here's the latest, probably final, version of the patch.  It includes
new tests in fns-tests.el, adds paragraphs to the descriptions of `eq'
and `equal' in objects.texi, and removes the bit about symbols with
position being "stored somewhat like vectors" (at the suggestion of
Mattias E.).



diff --git a/doc/lispref/objects.texi b/doc/lispref/objects.texi
index ad079e0d63a..784d59720ed 100644
--- a/doc/lispref/objects.texi
+++ b/doc/lispref/objects.texi
@@ -2206,6 +2206,10 @@ Equality Predicates
 the same object, and @code{eq} returns @code{t} or @code{nil}
 depending on whether the Lisp interpreter created one object or two.
 
+If @var{object1} or @var{object2} is a symbol with position, @code{eq}
+regards it as its bare symbol when @code{symbols-with-pos-enabled} is
+non-@code{nil} (@pxref{Symbols with Position}).
+
 @example
 @group
 (eq 'foo 'foo)
@@ -2363,6 +2367,13 @@ Equality Predicates
 The @code{equal} function recursively compares the contents of objects
 if they are integers, strings, markers, vectors, bool-vectors,
 byte-code function objects, char-tables, records, or font objects.
+
+If @var{object1} or @var{object2} is a symbol with position,
+@code{equal} regards it as its bare symbol when
+@code{symbols-with-pos-enabled} is non-@code{nil}.  Otherwise
+@code{equal} compares two symbols with position by recursively
+comparing their components.  @xref{Symbols with Position}.
+
 Other objects are considered @code{equal} only if they are @code{eq}.
 For example, two distinct buffers are never considered @code{equal},
 even if their textual contents are the same.
diff --git a/doc/lispref/symbols.texi b/doc/lispref/symbols.texi
index 34db0caf3a8..1f3b677d7fb 100644
--- a/doc/lispref/symbols.texi
+++ b/doc/lispref/symbols.texi
@@ -783,10 +783,15 @@ Symbols with Position
 
 @cindex bare symbol
 A @dfn{symbol with position} is a symbol, the @dfn{bare symbol},
-together with an unsigned integer called the @dfn{position}.  These
-objects are intended for use by the byte compiler, which records in
-them the position of each symbol occurrence and uses those positions
-in warning and error messages.
+together with an unsigned integer called the @dfn{position}.  Symbols
+with position don't themselves have entries in the obarray (though
+their bare symbols do; @pxref{Creating Symbols}).
+
+Symbols with position are for the use of the byte compiler, which
+records in them the position of each symbol occurrence and uses those
+positions in warning and error messages.  They shouldn't normally be
+used otherwise.  Doing so can cause unexpected results with basic
+Emacs functions such as @code{eq} and @code{equal}.
 
 The printed representation of a symbol with position uses the hash
 notation outlined in @ref{Printed Representation}.  It looks like
@@ -798,11 +803,20 @@ Symbols with Position
 
 For most purposes, when the flag variable
 @code{symbols-with-pos-enabled} is non-@code{nil}, symbols with
-positions behave just as bare symbols do.  For example, @samp{(eq
-#<symbol foo at 12345> foo)} has a value @code{t} when that variable
-is set (but @code{nil} when it isn't set).  Most of the time in Emacs this
-variable is @code{nil}, but the byte compiler binds it to @code{t}
-when it runs.
+positions behave just as their bare symbols would.  For example,
+@samp{(eq #<symbol foo at 12345> foo)} has a value @code{t} when the
+variable is set; likewise, @code{equal} will treat a symbol with
+position argument as its bare symbol.
+
+When @code{symbols-with-pos-enabled} is @code{nil}, any symbols with
+position continue to exist, but do not behave as symbols, or have the
+other useful properties outlined in the previous paragraph.  @code{eq}
+returns @code{t} when given identical arguments, and @code{equal}
+returns @code{t} when given arguments with @code{equal} components.
+
+Most of the time in Emacs @code{symbols-with-pos-enabled} is
+@code{nil}, but the byte compiler and the native compiler bind it to
+@code{t} when they run.
 
 Typically, symbols with position are created by the byte compiler
 calling the reader function @code{read-positioning-symbols}
@@ -810,17 +824,17 @@ Symbols with Position
 @code{position-symbol}.
 
 @defvar symbols-with-pos-enabled
-When this variable is non-@code{nil}, symbols with position behave
+When this variable is non-@code{nil}, a symbol with position behaves
 like the contained bare symbol.  Emacs runs a little more slowly in
 this case.
 @end defvar
 
 @defvar print-symbols-bare
-When bound to non-@code{nil}, the Lisp printer prints only the bare symbol of
-a symbol with position, ignoring the position.
+When bound to non-@code{nil}, the Lisp printer prints only the bare
+symbol of a symbol with position, ignoring the position.
 @end defvar
 
-@defun symbol-with-pos-p symbol.
+@defun symbol-with-pos-p symbol
 This function returns @code{t} if @var{symbol} is a symbol with
 position, @code{nil} otherwise.
 @end defun
diff --git a/src/fns.c b/src/fns.c
index d7b2e7908b6..9b300d503ff 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2773,10 +2773,13 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
 
   /* A symbol with position compares the contained symbol, and is
      `equal' to the corresponding ordinary symbol.  */
-  if (SYMBOL_WITH_POS_P (o1))
-    o1 = SYMBOL_WITH_POS_SYM (o1);
-  if (SYMBOL_WITH_POS_P (o2))
-    o2 = SYMBOL_WITH_POS_SYM (o2);
+  if (symbols_with_pos_enabled)
+    {
+      if (SYMBOL_WITH_POS_P (o1))
+	o1 = SYMBOL_WITH_POS_SYM (o1);
+      if (SYMBOL_WITH_POS_P (o2))
+	o2 = SYMBOL_WITH_POS_SYM (o2);
+    }
 
   if (BASE_EQ (o1, o2))
     return true;
@@ -2824,8 +2827,8 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
 	if (ASIZE (o2) != size)
 	  return false;
 
-	/* Compare bignums, overlays, markers, and boolvectors
-	   specially, by comparing their values.  */
+	/* Compare bignums, overlays, markers, boolvectors, and
+	   symbols with position specially, by comparing their values.  */
 	if (BIGNUMP (o1))
 	  return mpz_cmp (*xbignum_val (o1), *xbignum_val (o2)) == 0;
 	if (OVERLAYP (o1))
@@ -2857,6 +2860,11 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
 	if (TS_NODEP (o1))
 	  return treesit_node_eq (o1, o2);
 #endif
+	if (SYMBOL_WITH_POS_P(o1)) /* symbols_with_pos_enabled is false.  */
+	  return (BASE_EQ (XSYMBOL_WITH_POS (o1)->sym,
+			   XSYMBOL_WITH_POS (o2)->sym)
+		  && BASE_EQ (XSYMBOL_WITH_POS (o1)->pos,
+			      XSYMBOL_WITH_POS (o2)->pos));
 
 	/* Aside from them, only true vectors, char-tables, compiled
 	   functions, and fonts (font-spec, font-entity, font-object)
diff --git a/test/src/fns-tests.el b/test/src/fns-tests.el
index 79ae4393f40..9c09e4f0c33 100644
--- a/test/src/fns-tests.el
+++ b/test/src/fns-tests.el
@@ -98,6 +98,26 @@
   (should-not (equal-including-properties #("a" 0 1 (k "v"))
                                           #("b" 0 1 (k "v")))))
 
+(ert-deftest fns-tests-equal-symbols-with-position ()
+  "Test `eq' and `equal' on symbols with position."
+  (let ((foo1 (position-symbol 'foo 42))
+        (foo2 (position-symbol 'foo 666))
+        (foo3 (position-symbol 'foo 42)))
+    (let (symbols-with-pos-enabled)
+      (should (eq foo1 foo1))
+      (should (equal foo1 foo1))
+      (should-not (eq foo1 foo2))
+      (should-not (equal foo1 foo2))
+      (should-not (eq foo1 foo3))
+      (should (equal foo1 foo3)))
+    (let ((symbols-with-pos-enabled t))
+      (should (eq foo1 foo1))
+      (should (equal foo1 foo1))
+      (should (eq foo1 foo2))
+      (should (equal foo1 foo2))
+      (should (eq foo1 foo3))
+      (should (equal foo1 foo3)))))
+
 (ert-deftest fns-tests-reverse ()
   (should-error (reverse))
   (should-error (reverse 1))


-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Mattias Engdegård]

Alan, could you produce a combined patch that includes your proposal so far (and all tests)?

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Mattias.

On Sat, Aug 05, 2023 at 16:40:23 +0200, Mattias Engdegård wrote:
> Alan, could you produce a combined patch that includes your proposal
> so far (and all tests)?

I've already published what I have so far on the bug list.  I'll amend
fns-tests.el hopefully this evening, maybe tomorrow.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Mattias Engdegård]

5 aug. 2023 kl. 18.59 skrev Alan Mackenzie <acm@muc.de>:

> I've already published what I have so far on the bug list.  I'll amend
> fns-tests.el hopefully this evening, maybe tomorrow.

Thank you. Take your time, I'm in no hurry.

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Mattias.

On Sat, Aug 05, 2023 at 16:40:23 +0200, Mattias Engdegård wrote:
> Alan, could you produce a combined patch that includes your proposal so far (and all tests)?

Here it is:



diff --git a/doc/lispref/symbols.texi b/doc/lispref/symbols.texi
index 34db0caf3a8..a828d303c04 100644
--- a/doc/lispref/symbols.texi
+++ b/doc/lispref/symbols.texi
@@ -784,9 +784,15 @@ Symbols with Position
 @cindex bare symbol
 A @dfn{symbol with position} is a symbol, the @dfn{bare symbol},
 together with an unsigned integer called the @dfn{position}.  These
-objects are intended for use by the byte compiler, which records in
-them the position of each symbol occurrence and uses those positions
-in warning and error messages.
+objects are stored internally much like vectors, and don't themselves
+have entries in the obarray (though their bare symbols do;
+@pxref{Creating Symbols}).
+
+Symbols with position are for the use of the byte compiler, which
+records in them the position of each symbol occurrence and uses those
+positions in warning and error messages.  They shouldn't normally be
+used otherwise.  Doing so can cause unexpected results with basic
+Emacs functions such as @code{eq} and @code{equal}.
 
 The printed representation of a symbol with position uses the hash
 notation outlined in @ref{Printed Representation}.  It looks like
@@ -798,11 +804,20 @@ Symbols with Position
 
 For most purposes, when the flag variable
 @code{symbols-with-pos-enabled} is non-@code{nil}, symbols with
-positions behave just as bare symbols do.  For example, @samp{(eq
-#<symbol foo at 12345> foo)} has a value @code{t} when that variable
-is set (but @code{nil} when it isn't set).  Most of the time in Emacs this
-variable is @code{nil}, but the byte compiler binds it to @code{t}
-when it runs.
+positions behave just as their bare symbols would.  For example,
+@samp{(eq #<symbol foo at 12345> foo)} has a value @code{t} when the
+variable is set; likewise, @code{equal} will treat a symbol with
+position argument as its bare symbol.
+
+When @code{symbols-with-pos-enabled} is @code{nil}, any symbols with
+position continue to exist, but do not behave as symbols, or have the
+other useful properties outlined in the previous paragraph.  @code{eq}
+returns @code{t} when given identical arguments, and @code{equal}
+returns @code{t} when given arguments with @code{equal} components.
+
+Most of the time in Emacs @code{symbols-with-pos-enabled} is
+@code{nil}, but the byte compiler and the native compiler bind it to
+@code{t} when they run.
 
 Typically, symbols with position are created by the byte compiler
 calling the reader function @code{read-positioning-symbols}
@@ -820,7 +835,7 @@ Symbols with Position
 a symbol with position, ignoring the position.
 @end defvar
 
-@defun symbol-with-pos-p symbol.
+@defun symbol-with-pos-p symbol
 This function returns @code{t} if @var{symbol} is a symbol with
 position, @code{nil} otherwise.
 @end defun
diff --git a/src/fns.c b/src/fns.c
index bfd19e8c8f2..d47098c8791 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -2773,10 +2773,13 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
 
   /* A symbol with position compares the contained symbol, and is
      `equal' to the corresponding ordinary symbol.  */
-  if (SYMBOL_WITH_POS_P (o1))
-    o1 = SYMBOL_WITH_POS_SYM (o1);
-  if (SYMBOL_WITH_POS_P (o2))
-    o2 = SYMBOL_WITH_POS_SYM (o2);
+  if (symbols_with_pos_enabled)
+    {
+      if (SYMBOL_WITH_POS_P (o1))
+	o1 = SYMBOL_WITH_POS_SYM (o1);
+      if (SYMBOL_WITH_POS_P (o2))
+	o2 = SYMBOL_WITH_POS_SYM (o2);
+    }
 
   if (BASE_EQ (o1, o2))
     return true;
@@ -2824,8 +2827,8 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
 	if (ASIZE (o2) != size)
 	  return false;
 
-	/* Compare bignums, overlays, markers, and boolvectors
-	   specially, by comparing their values.  */
+	/* Compare bignums, overlays, markers, boolvectors, and
+	   symbols with position specially, by comparing their values.  */
 	if (BIGNUMP (o1))
 	  return mpz_cmp (*xbignum_val (o1), *xbignum_val (o2)) == 0;
 	if (OVERLAYP (o1))
@@ -2857,6 +2860,13 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
 	if (TS_NODEP (o1))
 	  return treesit_node_eq (o1, o2);
 #endif
+	if (SYMBOL_WITH_POS_P(o1)) /* symbols_with_pos_enabled is false.  */
+	  return (internal_equal (XSYMBOL_WITH_POS (o1)->sym,
+				  XSYMBOL_WITH_POS (o2)->sym,
+				  equal_kind, depth + 1, ht)
+		  && internal_equal (XSYMBOL_WITH_POS (o1)->pos,
+				     XSYMBOL_WITH_POS (o2)->pos,
+				     equal_kind, depth + 1, ht));
 
 	/* Aside from them, only true vectors, char-tables, compiled
 	   functions, and fonts (font-spec, font-entity, font-object)
diff --git a/test/src/fns-tests.el b/test/src/fns-tests.el
index 79ae4393f40..9c09e4f0c33 100644
--- a/test/src/fns-tests.el
+++ b/test/src/fns-tests.el
@@ -98,6 +98,26 @@
   (should-not (equal-including-properties #("a" 0 1 (k "v"))
                                           #("b" 0 1 (k "v")))))
 
+(ert-deftest fns-tests-equal-symbols-with-position ()
+  "Test `eq' and `equal' on symbols with position."
+  (let ((foo1 (position-symbol 'foo 42))
+        (foo2 (position-symbol 'foo 666))
+        (foo3 (position-symbol 'foo 42)))
+    (let (symbols-with-pos-enabled)
+      (should (eq foo1 foo1))
+      (should (equal foo1 foo1))
+      (should-not (eq foo1 foo2))
+      (should-not (equal foo1 foo2))
+      (should-not (eq foo1 foo3))
+      (should (equal foo1 foo3)))
+    (let ((symbols-with-pos-enabled t))
+      (should (eq foo1 foo1))
+      (should (equal foo1 foo1))
+      (should (eq foo1 foo2))
+      (should (equal foo1 foo2))
+      (should (eq foo1 foo3))
+      (should (equal foo1 foo3)))))
+
 (ert-deftest fns-tests-reverse ()
   (should-error (reverse))
   (should-error (reverse 1))


-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Mattias Engdegård]

5 aug. 2023 kl. 23.07 skrev Alan Mackenzie <acm@muc.de>:

> diff --git a/doc/lispref/symbols.texi b/doc/lispref/symbols.texi
> index 34db0caf3a8..a828d303c04 100644
> --- a/doc/lispref/symbols.texi
> +++ b/doc/lispref/symbols.texi
> @@ -784,9 +784,15 @@ Symbols with Position
> @cindex bare symbol
> A @dfn{symbol with position} is a symbol, the @dfn{bare symbol},
> together with an unsigned integer called the @dfn{position}.  These
> -objects are intended for use by the byte compiler, which records in
> -them the position of each symbol occurrence and uses those positions
> -in warning and error messages.
> +objects are stored internally much like vectors

Not sure why we want to say how they are stored here. They can be stored in bubble memory for all the user cares.

> , and don't themselves
> +have entries in the obarray (though their bare symbols do;
> +@pxref{Creating Symbols}).
> +
> +Symbols with position are for the use of the byte compiler, which
> +records in them the position of each symbol occurrence and uses those
> +positions in warning and error messages.  They shouldn't normally be
> +used otherwise.  Doing so can cause unexpected results with basic
> +Emacs functions such as @code{eq} and @code{equal}.
> 
> The printed representation of a symbol with position uses the hash
> notation outlined in @ref{Printed Representation}.  It looks like
> @@ -798,11 +804,20 @@ Symbols with Position
> 
> For most purposes, when the flag variable
> @code{symbols-with-pos-enabled} is non-@code{nil}, symbols with
> -positions behave just as bare symbols do.  For example, @samp{(eq
> -#<symbol foo at 12345> foo)} has a value @code{t} when that variable
> -is set (but @code{nil} when it isn't set).  Most of the time in Emacs this
> -variable is @code{nil}, but the byte compiler binds it to @code{t}
> -when it runs.
> +positions behave just as their bare symbols would.  For example,
> +@samp{(eq #<symbol foo at 12345> foo)} has a value @code{t} when the
> +variable is set; likewise, @code{equal} will treat a symbol with
> +position argument as its bare symbol.
> +
> +When @code{symbols-with-pos-enabled} is @code{nil}, any symbols with
> +position continue to exist, but do not behave as symbols, or have the
> +other useful properties outlined in the previous paragraph.  @code{eq}
> +returns @code{t} when given identical arguments, and @code{equal}
> +returns @code{t} when given arguments with @code{equal} components.

Since the components are bare symbols and fixnums, equality and identity for them are equivalent, right?

> +
> +Most of the time in Emacs @code{symbols-with-pos-enabled} is
> +@code{nil}, but the byte compiler and the native compiler bind it to
> +@code{t} when they run.
> 
> Typically, symbols with position are created by the byte compiler
> calling the reader function @code{read-positioning-symbols}
> @@ -820,7 +835,7 @@ Symbols with Position
> a symbol with position, ignoring the position.
> @end defvar
> 
> -@defun symbol-with-pos-p symbol.
> +@defun symbol-with-pos-p symbol
> This function returns @code{t} if @var{symbol} is a symbol with
> position, @code{nil} otherwise.
> @end defun
> diff --git a/src/fns.c b/src/fns.c
> index bfd19e8c8f2..d47098c8791 100644
> --- a/src/fns.c
> +++ b/src/fns.c
> @@ -2773,10 +2773,13 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
> 
>   /* A symbol with position compares the contained symbol, and is
>      `equal' to the corresponding ordinary symbol.  */
> -  if (SYMBOL_WITH_POS_P (o1))
> -    o1 = SYMBOL_WITH_POS_SYM (o1);
> -  if (SYMBOL_WITH_POS_P (o2))
> -    o2 = SYMBOL_WITH_POS_SYM (o2);
> +  if (symbols_with_pos_enabled)
> +    {
> +      if (SYMBOL_WITH_POS_P (o1))
> +	o1 = SYMBOL_WITH_POS_SYM (o1);
> +      if (SYMBOL_WITH_POS_P (o2))
> +	o2 = SYMBOL_WITH_POS_SYM (o2);
> +    }

OK. This reduces the number of branches in the hot path for ordinary (non-sympos) code by one while adding one to sym-pos code, and that should be a fair trade-off. The new branch should be well-predicted but is still consuming resources.

>   if (BASE_EQ (o1, o2))
>     return true;
> @@ -2824,8 +2827,8 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
> 	if (ASIZE (o2) != size)
> 	  return false;
> 
> -	/* Compare bignums, overlays, markers, and boolvectors
> -	   specially, by comparing their values.  */
> +	/* Compare bignums, overlays, markers, boolvectors, and
> +	   symbols with position specially, by comparing their values.  */
> 	if (BIGNUMP (o1))
> 	  return mpz_cmp (*xbignum_val (o1), *xbignum_val (o2)) == 0;
> 	if (OVERLAYP (o1))
> @@ -2857,6 +2860,13 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
> 	if (TS_NODEP (o1))
> 	  return treesit_node_eq (o1, o2);
> #endif
> +	if (SYMBOL_WITH_POS_P(o1)) /* symbols_with_pos_enabled is false.  */
> +	  return (internal_equal (XSYMBOL_WITH_POS (o1)->sym,
> +				  XSYMBOL_WITH_POS (o2)->sym,
> +				  equal_kind, depth + 1, ht)
> +		  && internal_equal (XSYMBOL_WITH_POS (o1)->pos,
> +				     XSYMBOL_WITH_POS (o2)->pos,
> +				     equal_kind, depth + 1, ht));

Why recurse here if the components are a bare symbol and a fixnum, respectively?

> 	/* Aside from them, only true vectors, char-tables, compiled
> 	   functions, and fonts (font-spec, font-entity, font-object)
> diff --git a/test/src/fns-tests.el b/test/src/fns-tests.el
> index 79ae4393f40..9c09e4f0c33 100644
> --- a/test/src/fns-tests.el
> +++ b/test/src/fns-tests.el
> @@ -98,6 +98,26 @@
>   (should-not (equal-including-properties #("a" 0 1 (k "v"))
>                                           #("b" 0 1 (k "v")))))
> 
> +(ert-deftest fns-tests-equal-symbols-with-position ()
> +  "Test `eq' and `equal' on symbols with position."
> +  (let ((foo1 (position-symbol 'foo 42))
> +        (foo2 (position-symbol 'foo 666))
> +        (foo3 (position-symbol 'foo 42)))
> +    (let (symbols-with-pos-enabled)
> +      (should (eq foo1 foo1))

Thank you! There is nothing wrong with the coverage of these tests with respect to your changes.

However we should make an effort to prevent the compiler from optimising (eq X X) -> t etc, which it is completely entitled to doing, and also test both the interpreted and compiled version of `eq` and `equal`.

The test bytecomp--eq-symbols-with-pos-enabled already does most of this for a different reason. Perhaps it can be extended to cover `equal` as well?

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Mattias.

On Sun, Aug 06, 2023 at 15:37:24 +0200, Mattias Engdegård wrote:
> 5 aug. 2023 kl. 23.07 skrev Alan Mackenzie <acm@muc.de>:

> > diff --git a/doc/lispref/symbols.texi b/doc/lispref/symbols.texi
> > index 34db0caf3a8..a828d303c04 100644
> > --- a/doc/lispref/symbols.texi
> > +++ b/doc/lispref/symbols.texi
> > @@ -784,9 +784,15 @@ Symbols with Position
> > @cindex bare symbol
> > A @dfn{symbol with position} is a symbol, the @dfn{bare symbol},
> > together with an unsigned integer called the @dfn{position}.  These
> > -objects are intended for use by the byte compiler, which records in
> > -them the position of each symbol occurrence and uses those positions
> > -in warning and error messages.
> > +objects are stored internally much like vectors

> Not sure why we want to say how they are stored here. They can be
> stored in bubble memory for all the user cares.

The point is, they are _not_ stored in the obarray.  Eli specifically
asked me to clarify this point, yesterday.

> > , and don't themselves
> > +have entries in the obarray (though their bare symbols do;
> > +@pxref{Creating Symbols}).
> > +
> > +Symbols with position are for the use of the byte compiler, which
> > +records in them the position of each symbol occurrence and uses those
> > +positions in warning and error messages.  They shouldn't normally be
> > +used otherwise.  Doing so can cause unexpected results with basic
> > +Emacs functions such as @code{eq} and @code{equal}.

> > The printed representation of a symbol with position uses the hash
> > notation outlined in @ref{Printed Representation}.  It looks like
> > @@ -798,11 +804,20 @@ Symbols with Position

> > For most purposes, when the flag variable
> > @code{symbols-with-pos-enabled} is non-@code{nil}, symbols with
> > -positions behave just as bare symbols do.  For example, @samp{(eq
> > -#<symbol foo at 12345> foo)} has a value @code{t} when that variable
> > -is set (but @code{nil} when it isn't set).  Most of the time in Emacs this
> > -variable is @code{nil}, but the byte compiler binds it to @code{t}
> > -when it runs.
> > +positions behave just as their bare symbols would.  For example,
> > +@samp{(eq #<symbol foo at 12345> foo)} has a value @code{t} when the
> > +variable is set; likewise, @code{equal} will treat a symbol with
> > +position argument as its bare symbol.
> > +
> > +When @code{symbols-with-pos-enabled} is @code{nil}, any symbols with
> > +position continue to exist, but do not behave as symbols, or have the
> > +other useful properties outlined in the previous paragraph.  @code{eq}
> > +returns @code{t} when given identical arguments, and @code{equal}
> > +returns @code{t} when given arguments with @code{equal} components.

> Since the components are bare symbols and fixnums, equality and
> identity for them are equivalent, right?

No.  If there are two distinct SWPs with the same bare symbol and the
same position, they should be equal, but not eq.  But the real point is
to contrast how equal and eq work when symbols-with-pos-enabled is nil
with when it is non-nil.

> > +
> > +Most of the time in Emacs @code{symbols-with-pos-enabled} is
> > +@code{nil}, but the byte compiler and the native compiler bind it to
> > +@code{t} when they run.

> > Typically, symbols with position are created by the byte compiler
> > calling the reader function @code{read-positioning-symbols}
> > @@ -820,7 +835,7 @@ Symbols with Position
> > a symbol with position, ignoring the position.
> > @end defvar

> > -@defun symbol-with-pos-p symbol.
> > +@defun symbol-with-pos-p symbol
> > This function returns @code{t} if @var{symbol} is a symbol with
> > position, @code{nil} otherwise.
> > @end defun
> > diff --git a/src/fns.c b/src/fns.c
> > index bfd19e8c8f2..d47098c8791 100644
> > --- a/src/fns.c
> > +++ b/src/fns.c
> > @@ -2773,10 +2773,13 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,

> >   /* A symbol with position compares the contained symbol, and is
> >      `equal' to the corresponding ordinary symbol.  */
> > -  if (SYMBOL_WITH_POS_P (o1))
> > -    o1 = SYMBOL_WITH_POS_SYM (o1);
> > -  if (SYMBOL_WITH_POS_P (o2))
> > -    o2 = SYMBOL_WITH_POS_SYM (o2);
> > +  if (symbols_with_pos_enabled)
> > +    {
> > +      if (SYMBOL_WITH_POS_P (o1))
> > +	o1 = SYMBOL_WITH_POS_SYM (o1);
> > +      if (SYMBOL_WITH_POS_P (o2))
> > +	o2 = SYMBOL_WITH_POS_SYM (o2);
> > +    }

> OK. This reduces the number of branches in the hot path for ordinary
> (non-sympos) code by one while adding one to sym-pos code, and that
> should be a fair trade-off. The new branch should be well-predicted but
> is still consuming resources.

I did some simple timings on the old and new code, and the new code is
not slower.  See my post to Eli from yesterday evening [European time] on
the bug #65017 thread.

> >   if (BASE_EQ (o1, o2))
> >     return true;
> > @@ -2824,8 +2827,8 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
> > 	if (ASIZE (o2) != size)
> > 	  return false;

> > -	/* Compare bignums, overlays, markers, and boolvectors
> > -	   specially, by comparing their values.  */
> > +	/* Compare bignums, overlays, markers, boolvectors, and
> > +	   symbols with position specially, by comparing their values.  */
> > 	if (BIGNUMP (o1))
> > 	  return mpz_cmp (*xbignum_val (o1), *xbignum_val (o2)) == 0;
> > 	if (OVERLAYP (o1))
> > @@ -2857,6 +2860,13 @@ internal_equal (Lisp_Object o1, Lisp_Object o2, enum equal_kind equal_kind,
> > 	if (TS_NODEP (o1))
> > 	  return treesit_node_eq (o1, o2);
> > #endif
> > +	if (SYMBOL_WITH_POS_P(o1)) /* symbols_with_pos_enabled is false.  */
> > +	  return (internal_equal (XSYMBOL_WITH_POS (o1)->sym,
> > +				  XSYMBOL_WITH_POS (o2)->sym,
> > +				  equal_kind, depth + 1, ht)
> > +		  && internal_equal (XSYMBOL_WITH_POS (o1)->pos,
> > +				     XSYMBOL_WITH_POS (o2)->pos,
> > +				     equal_kind, depth + 1, ht));

> Why recurse here if the components are a bare symbol and a fixnum,
> respectively?

Maybe in case they might somehow be something else?  The code in the
patch prevents an error being thrown in such a case.  The code should be
run vanishingly seldomly anyhow, so it shouldn't matter much.

> > 	/* Aside from them, only true vectors, char-tables, compiled
> > 	   functions, and fonts (font-spec, font-entity, font-object)
> > diff --git a/test/src/fns-tests.el b/test/src/fns-tests.el
> > index 79ae4393f40..9c09e4f0c33 100644
> > --- a/test/src/fns-tests.el
> > +++ b/test/src/fns-tests.el
> > @@ -98,6 +98,26 @@
> >   (should-not (equal-including-properties #("a" 0 1 (k "v"))
> >                                           #("b" 0 1 (k "v")))))

> > +(ert-deftest fns-tests-equal-symbols-with-position ()
> > +  "Test `eq' and `equal' on symbols with position."
> > +  (let ((foo1 (position-symbol 'foo 42))
> > +        (foo2 (position-symbol 'foo 666))
> > +        (foo3 (position-symbol 'foo 42)))
> > +    (let (symbols-with-pos-enabled)
> > +      (should (eq foo1 foo1))

> Thank you! There is nothing wrong with the coverage of these tests with
> respect to your changes.

> However we should make an effort to prevent the compiler from
> optimising (eq X X) -> t etc, which it is completely entitled to doing,
> ....

Why?  (eq X X) is t in all circumstances, whether X is a symbol, a cons
structure, or anything else.  What am I missing, here?

> .... and also test both the interpreted and compiled version of `eq`
> and `equal`.

They're the same code in both cases.  I'm missing something here, too, I
think.

> The test bytecomp--eq-symbols-with-pos-enabled already does most of
> this for a different reason. Perhaps it can be extended to cover
> `equal` as well?

I don't have such a test in my repository anywhere.  Are you sure you
wrote it right?

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Mattias Engdegård]

6 aug. 2023 kl. 17.02 skrev Alan Mackenzie <acm@muc.de>:

>>> together with an unsigned integer called the @dfn{position}.  These
>>> -objects are intended for use by the byte compiler, which records in
>>> -them the position of each symbol occurrence and uses those positions
>>> -in warning and error messages.
>>> +objects are stored internally much like vectors
> 
>> Not sure why we want to say how they are stored here. They can be
>> stored in bubble memory for all the user cares.
> 
> The point is, they are _not_ stored in the obarray.  Eli specifically
> asked me to clarify this point, yesterday.

Oh that part is perfectly fine (thank you), we just don't need to say that the sympos objects are stored "like vectors" -- that just confuses the reader.

>>> +When @code{symbols-with-pos-enabled} is @code{nil}, any symbols with
>>> +position continue to exist, but do not behave as symbols, or have the
>>> +other useful properties outlined in the previous paragraph.  @code{eq}
>>> +returns @code{t} when given identical arguments, and @code{equal}
>>> +returns @code{t} when given arguments with @code{equal} components.
> 
>> Since the components are bare symbols and fixnums, equality and
>> identity for them are equivalent, right?
> 
> No.  If there are two distinct SWPs with the same bare symbol and the
> same position, they should be equal, but not eq.  But the real point is
> to contrast how equal and eq work when symbols-with-pos-enabled is nil
> with when it is non-nil.

I meant that the components of equal sympos objects aren't merely equal but identical. (This is a very minor quibble; you can keep the text if you like.)

>> OK. This reduces the number of branches in the hot path for ordinary
>> (non-sympos) code by one while adding one to sym-pos code, and that
>> should be a fair trade-off. The new branch should be well-predicted but
>> is still consuming resources.
> 
> I did some simple timings on the old and new code, and the new code is
> not slower.

This is not easy to measure and details matter, but as I said -- there is no reason to believe that your changes should be a regression in the important measure, rather the opposite.

>>> +	if (SYMBOL_WITH_POS_P(o1)) /* symbols_with_pos_enabled is false. */
>>> +	  return (internal_equal (XSYMBOL_WITH_POS (o1)->sym,
>>> +				  XSYMBOL_WITH_POS (o2)->sym,
>>> +				  equal_kind, depth + 1, ht)
>>> +		  && internal_equal (XSYMBOL_WITH_POS (o1)->pos,
>>> +				     XSYMBOL_WITH_POS (o2)->pos,
>>> +				     equal_kind, depth + 1, ht));
> 
>> Why recurse here if the components are a bare symbol and a fixnum,
>> respectively?
> 
> Maybe in case they might somehow be something else?

No, we must be able to assume that internal invariants hold when we offer no way for them to be violated. Let's just change the calls to BASE_EQ and be done with it.

>> However we should make an effort to prevent the compiler from
>> optimising (eq X X) -> t etc, which it is completely entitled to doing,
>> ....
> 
> Why?  (eq X X) is t in all circumstances, whether X is a symbol, a cons
> structure, or anything else.  What am I missing, here?

If the compiler transforms (eq foo1 foo1) into t then the test won't actually exercise the implementation of `eq`.

>> .... and also test both the interpreted and compiled version of `eq`
>> and `equal`.
> 
> They're the same code in both cases.  I'm missing something here, too, I
> think.

Byte-code doesn't call Feq, it uses its own implementation. They should work identically but as we are checking edge cases here we'd better be sure about that.

>> The test bytecomp--eq-symbols-with-pos-enabled already does most of
>> this for a different reason. Perhaps it can be extended to cover
>> `equal` as well?
> 
> I don't have such a test in my repository anywhere.  Are you sure you
> wrote it right?

It was added in 44d7fd3805.

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Mattias.

On Mon, Aug 07, 2023 at 10:58:41 +0200, Mattias Engdegård wrote:
> 6 aug. 2023 kl. 17.02 skrev Alan Mackenzie <acm@muc.de>:

> >>> together with an unsigned integer called the @dfn{position}.  These
> >>> -objects are intended for use by the byte compiler, which records in
> >>> -them the position of each symbol occurrence and uses those positions
> >>> -in warning and error messages.
> >>> +objects are stored internally much like vectors

> >> Not sure why we want to say how they are stored here. They can be
> >> stored in bubble memory for all the user cares.

> > The point is, they are _not_ stored in the obarray.  Eli specifically
> > asked me to clarify this point, yesterday.

> Oh that part is perfectly fine (thank you), we just don't need to say
> that the sympos objects are stored "like vectors" -- that just confuses
> the reader.

Why not?  It's true, and I doubt it will cause confusion.  I think we
need to say something positive in that place (since we're following it
with a negative).  Perhaps you could suggest an alternative.

> >>> +When @code{symbols-with-pos-enabled} is @code{nil}, any symbols with
> >>> +position continue to exist, but do not behave as symbols, or have the
> >>> +other useful properties outlined in the previous paragraph.  @code{eq}
> >>> +returns @code{t} when given identical arguments, and @code{equal}
> >>> +returns @code{t} when given arguments with @code{equal} components.

> >> Since the components are bare symbols and fixnums, equality and
> >> identity for them are equivalent, right?

> > No.  If there are two distinct SWPs with the same bare symbol and the
> > same position, they should be equal, but not eq.  But the real point is
> > to contrast how equal and eq work when symbols-with-pos-enabled is nil
> > with when it is non-nil.

> I meant that the components of equal sympos objects aren't merely equal
> but identical. (This is a very minor quibble; you can keep the text if
> you like.)

The current proposed text has a more subtle intention.  It says that eq
and equal behave just like they always have done for everything when
symbols-with-pos-enabled is nil.

> >> OK. This reduces the number of branches in the hot path for ordinary
> >> (non-sympos) code by one while adding one to sym-pos code, and that
> >> should be a fair trade-off. The new branch should be well-predicted but
> >> is still consuming resources.

> > I did some simple timings on the old and new code, and the new code is
> > not slower.

> This is not easy to measure and details matter, but as I said -- there
> is no reason to believe that your changes should be a regression in the
> important measure, rather the opposite.

Agreed.

> >>> +	if (SYMBOL_WITH_POS_P(o1)) /* symbols_with_pos_enabled is false. */
> >>> +	  return (internal_equal (XSYMBOL_WITH_POS (o1)->sym,
> >>> +				  XSYMBOL_WITH_POS (o2)->sym,
> >>> +				  equal_kind, depth + 1, ht)
> >>> +		  && internal_equal (XSYMBOL_WITH_POS (o1)->pos,
> >>> +				     XSYMBOL_WITH_POS (o2)->pos,
> >>> +				     equal_kind, depth + 1, ht));

> >> Why recurse here if the components are a bare symbol and a fixnum,
> >> respectively?

> > Maybe in case they might somehow be something else?

> No, we must be able to assume that internal invariants hold when we
> offer no way for them to be violated. Let's just change the calls to
> BASE_EQ and be done with it.

OK, I think you're right, here, I'll change that.

> >> However we should make an effort to prevent the compiler from
> >> optimising (eq X X) -> t etc, which it is completely entitled to doing,
> >> ....

> > Why?  (eq X X) is t in all circumstances, whether X is a symbol, a cons
> > structure, or anything else.  What am I missing, here?

> If the compiler transforms (eq foo1 foo1) into t then the test won't
> actually exercise the implementation of `eq`.

Ah!  You're talking about the tests.  OK.  In my tests, I timed (equal a
b) where a and b were variables which were either equal or not.

> >> .... and also test both the interpreted and compiled version of `eq`
> >> and `equal`.

> > They're the same code in both cases.  I'm missing something here, too, I
> > think.

> Byte-code doesn't call Feq, it uses its own implementation. They should
> work identically but as we are checking edge cases here we'd better be
> sure about that.

> >> The test bytecomp--eq-symbols-with-pos-enabled already does most of
> >> this for a different reason. Perhaps it can be extended to cover
> >> `equal` as well?

> > I don't have such a test in my repository anywhere.  Are you sure you
> > wrote it right?

> It was added in 44d7fd3805.

OK.  That commit is recent, then?  If so, I'll see it soon.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Mattias Engdegård]

7 aug. 2023 kl. 11.44 skrev Alan Mackenzie <acm@muc.de>:

>> Oh that part is perfectly fine (thank you), we just don't need to say
>> that the sympos objects are stored "like vectors" -- that just confuses
>> the reader.
> 
> Why not?  It's true, and I doubt it will cause confusion.  I think we
> need to say something positive in that place (since we're following it
> with a negative).  Perhaps you could suggest an alternative.

What I meant is that sympos objects are not at all like vectors in that they aren't mutable, indexable, readable, they can't be used with aref/aset, can't hold arbitrary values, don't form sequences, and so on, so it makes sense that we try finding another description. What about some variation on:

  A @dfn{symbol with position} is an immutable object consisting of a
  @dfn{bare symbol} and a position, a natural number.

(You can probably do better.)

>> I meant that the components of equal sympos objects aren't merely equal
>> but identical. (This is a very minor quibble; you can keep the text if
>> you like.)
> 
> The current proposed text has a more subtle intention.  It says that eq
> and equal behave just like they always have done for everything when
> symbols-with-pos-enabled is nil.

No, I just meant that the components themselves in equal sympos objects are eq, not just equal. But this isn't important.

>> If the compiler transforms (eq foo1 foo1) into t then the test won't
>> actually exercise the implementation of `eq`.
> 
> Ah!  You're talking about the tests.  OK.  In my tests, I timed (equal a
> b) where a and b were variables which were either equal or not.

As a matter of fact I went ahead and added the aforementioned optimisation. It is something I had wanted to do for quite some time because code like (eq VAR VAR) does occur from time to time from macro-expansion etc.

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

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

> at the top level of the function.  Thus
>
>     (equal 'foo #<symbol foo at 42>)
>
> will return non-nil, regardless of the setting of
> `symbols-with-pos-enabled`.  It should return non-nil only when that
> flag variable is non-nil.  This is a bug.

Could you explain why you think it's a bug?


        Stefan

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Stefan.

On Sun, Aug 06, 2023 at 23:30:15 -0400, Stefan Monnier wrote:
> > at the top level of the function.  Thus

> >     (equal 'foo #<symbol foo at 42>)

> > will return non-nil, regardless of the setting of
> > `symbols-with-pos-enabled`.  It should return non-nil only when that
> > flag variable is non-nil.  This is a bug.

> Could you explain why you think it's a bug?

When symbols-with-pos-enabled is non-nil, the two arguments to that
equal call are equal.  That is the point of s-w-p-e.

When s-w-p-e is nil, and the "magic" is thus switched off, the two lisp
objects have different type (the first is a symbol, the second is a
pseudovector), thus cannot be equal.

I think the amendments I've proposed for the elisp manual page "Symbols
with Position" are now clearer about this sort of thing.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

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

>> Could you explain why you think it's a bug?
> When symbols-with-pos-enabled is non-nil, the two arguments to that
> equal call are equal.  That is the point of s-w-p-e.

AFAIK the point of the `symbols-with-pos-enabled` is to try and keep the
performance impact of sympos under control, and that matters only for
`eq`, so I don't think there's a strong reason here for `equal` to pay
attention to it.

> When s-w-p-e is nil, and the "magic" is thus switched off, the two lisp
> objects have different type (the first is a symbol, the second is a
> pseudovector), thus cannot be equal.

"cannot" is obviously not the correct word here, since currently they
are considered `equal`, so clearly they *can* be considered `equal`:
it's easy to implement, and experience shows that "it works".

So I'm still wondering why you think it's a bug.

AFAICT whether sympos should be `equal` to others and/or to bare symbols
is something we pretty much get to choose freely based on convenience:
either the current behavior or the one you now advocate are perfectly
acceptable and not bugs.

As I said elsewhere, I'm not sure which choice is best, but at least we
have some experience with the current choice and I haven't seen any
clear problem with it yet, so I'd tend to lean towards keeping the
current behavior.

What would be the concrete advantages of the new behavior compared to
the current one?


        Stefan

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Stefan.

On Mon, Aug 07, 2023 at 22:56:31 -0400, Stefan Monnier wrote:
> >> Could you explain why you think it's a bug?
> > When symbols-with-pos-enabled is non-nil, the two arguments to that
> > equal call are equal.  That is the point of s-w-p-e.

> AFAIK the point of the `symbols-with-pos-enabled` is to try and keep the
> performance impact of sympos under control, and that matters only for
> `eq`, so I don't think there's a strong reason here for `equal` to pay
> attention to it.

Which is like saying you're happy for it to be undefined.  In the code
at the moment, the result of `equal' on symbols with position is
undefined, i.e. it returns a random value.

> > When s-w-p-e is nil, and the "magic" is thus switched off, the two lisp
> > objects have different type (the first is a symbol, the second is a
> > pseudovector), thus cannot be equal.

> "cannot" is obviously not the correct word here, since currently they
> are considered `equal`, so clearly they *can* be considered `equal`:
> it's easy to implement, and experience shows that "it works".

> So I'm still wondering why you think it's a bug.

Because it violates the definition and basic understanding of equal.
It's a special case when no special case is needed.

> AFAICT whether sympos should be `equal` to others and/or to bare symbols
> is something we pretty much get to choose freely based on convenience:

No we don't.  They have to be chosen to be as consistent as possible
with the rest of Emacs.

> either the current behavior or the one you now advocate are perfectly
> acceptable and not bugs.

The current behaviour is a bug.  It was me that coded up that amendment
to equal, and I can remember simply not taking into account the scenario
we're talking about.

> As I said elsewhere, I'm not sure which choice is best, but at least we
> have some experience with the current choice ....

I rather doubt that.  When have SWPs, when symbols-with-pos-enabled is
nil, been tested by equal, apart from in tests, maybe?

> .... and I haven't seen any clear problem with it yet, so I'd tend to
> lean towards keeping the current behavior.

I'm wondering why you're making such a big thing out of it.  It's a
small change which will increase consistency and predictability in
Emacs in a small way, without any negative effects.

> What would be the concrete advantages of the new behavior compared to
> the current one?

There are no "concrete" advantages, aside from an insignificant increase
in speed for Emacs when not byte compiling.  The code and the
documentation currently don't match.  Fixing the code, by removing a
special case, is easier and more satisfying than documenting that
special case in the Elisp manual.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

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

>> >> Could you explain why you think it's a bug?
>> > When symbols-with-pos-enabled is non-nil, the two arguments to that
>> > equal call are equal.  That is the point of s-w-p-e.
>> AFAIK the point of the `symbols-with-pos-enabled` is to try and keep the
>> performance impact of sympos under control, and that matters only for
>> `eq`, so I don't think there's a strong reason here for `equal` to pay
>> attention to it.
> Which is like saying you're happy for it to be undefined.

Not quite.  I'm saying that as far as technical reasons go, I can't see
any reason why `equal` needs to pay attention to
`symbols-with-pos-enabled`.  IOW affecting the behavior of `equal` is
*not* part of "the point of s-w-p-e".

> In the code at the moment, the result of `equal' on symbols with
> position is undefined, i.e. it returns a random value.

In which sense?
AFAICT it returns non-nil iff the underlying bare symbols are `eq`.
That does not sound "random" at all to me.
What am I missing?

>> So I'm still wondering why you think it's a bug.
> Because it violates the definition and basic understanding of equal.

Could you expand on that, e.g. explaining which part of your
understanding of "the definition and basic understanding of equal"
it violates?

> It's a special case when no special case is needed.

Making `equal` depend on a global variable is also introducing
a special case.  IOW, all choices suck in one way or another.
I think we need more practical and concrete reasons to prefer one over
another.  Philosophical arguments seem rather weak here.

>> AFAICT whether sympos should be `equal` to others and/or to bare symbols
>> is something we pretty much get to choose freely based on convenience:
> No we don't.  They have to be chosen to be as consistent as possible
> with the rest of Emacs.

`equal` is not self-consistent.  It compares hash-tables like `eq` but
looks inside vectors.  It ignores strings' properties.  The list goes on
and on.

>> either the current behavior or the one you now advocate are perfectly
>> acceptable and not bugs.
> The current behaviour is a bug.

Hmm... This subthread is supposed to answer my question about why you
think it's a bug.  So just re-stating it is not very helpful.
Please try and articulate more precisely *why* you think it's the case.
Is it a gut-feeling?

> It was me that coded up that amendment to equal, and I can remember
> simply not taking into account the scenario we're talking about.

Which scenario?

>> As I said elsewhere, I'm not sure which choice is best, but at least we
>> have some experience with the current choice ....
> I rather doubt that.  When have SWPs, when symbols-with-pos-enabled is
> nil, been tested by equal, apart from in tests, maybe?

We don't know, admittedly, but we do know that if/when it has happened,
it hasn't caused any problem so far.

>> .... and I haven't seen any clear problem with it yet, so I'd tend to
>> lean towards keeping the current behavior.
> I'm wondering why you're making such a big thing out of it.  It's a
> small change which will increase consistency and predictability in
> Emacs in a small way, without any negative effects.

I don't see either of the two options as being more consistent or
more predictable.  You can see symbols' positions as being similar to
strings' properties, which `equal` gleefully ignores.

I think the main reasons I'm rather opposed are:

- I don't like making `equal` depend on a global variable.  It makes it
  impure, and will invalidate existing optimizations, exactly like we've
  just witnessed for `eq`.
- I consider `symbols-with-pos-enabled` to be a wart, so I'd rather try
  and minimize its use as much as possible.

>> What would be the concrete advantages of the new behavior compared to
>> the current one?
> There are no "concrete" advantages, aside from an insignificant increase
> in speed for Emacs when not byte compiling.  The code and the
> documentation currently don't match.  Fixing the code, by removing a
> special case, is easier and more satisfying than documenting that
> special case in the Elisp manual.

Then, I'd vote to fix the doc rather than the code.


        Stefan

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Stefan.

On Wed, Aug 09, 2023 at 23:28:53 -0400, Stefan Monnier wrote:
> >> >> Could you explain why you think it's a bug?
> >> > When symbols-with-pos-enabled is non-nil, the two arguments to that
> >> > equal call are equal.  That is the point of s-w-p-e.
> >> AFAIK the point of the `symbols-with-pos-enabled` is to try and keep the
> >> performance impact of sympos under control, and that matters only for
> >> `eq`, so I don't think there's a strong reason here for `equal` to pay
> >> attention to it.
> > Which is like saying you're happy for it to be undefined.

> Not quite.  I'm saying that as far as technical reasons go, I can't see
> any reason why `equal` needs to pay attention to
> `symbols-with-pos-enabled`.

I can.

> IOW affecting the behavior of `equal` is *not* part of "the point of
> s-w-p-e".

Which is precisely why I want to correct the behaviour of equal with
SWPs.

> > In the code at the moment, the result of `equal' on symbols with
> > position is undefined, i.e. it returns a random value.

> In which sense?

In the sense it wasn't deliberately coded.  It's just a random value
resulting from the code for other scenarios.

> AFAICT it returns non-nil iff the underlying bare symbols are `eq`.
> That does not sound "random" at all to me.
> What am I missing?

That equal is different from eq.  The definition of eq (more or less) is
"identical objects".  The definition of equal (more or less) is "same
structure with same components".

> >> So I'm still wondering why you think it's a bug.
> > Because it violates the definition and basic understanding of equal.

> Could you expand on that, e.g. explaining which part of your
> understanding of "the definition and basic understanding of equal"
> it violates?

See my previous paragraph of this post.  You're proposing that the
position elements of SWPs should be ignored in equal.  I don't see any
good reason for this.

> > It's a special case when no special case is needed.

> Making `equal` depend on a global variable is also introducing
> a special case.

I know you don't like symbols-with-pos-enabled, but it's there.  It
implements, by its very nature, special cases when it's non-nil.  You
want to extend those special cases to the behaviour when it's nil.

> IOW, all choices suck in one way or another.

Not really.

> I think we need more practical and concrete reasons to prefer one over
> another.  Philosophical arguments seem rather weak here.

The consistency of Emacs's basic functions seems very important to me,
and it's likely very important to other people, too.  You seem to be
dismissing it as unimportant.

> >> AFAICT whether sympos should be `equal` to others and/or to bare symbols
> >> is something we pretty much get to choose freely based on convenience:
> > No we don't.  They have to be chosen to be as consistent as possible
> > with the rest of Emacs.

> `equal` is not self-consistent.  It compares hash-tables like `eq` but
> looks inside vectors.  It ignores strings' properties.  The list goes on
> and on.

I said "as consistent as possible", not "absolutely consistent".  There
is no need to make that list of inconsistencies any bigger.

> >> either the current behavior or the one you now advocate are perfectly
> >> acceptable and not bugs.
> > The current behaviour is a bug.

> Hmm... This subthread is supposed to answer my question about why you
> think it's a bug.  So just re-stating it is not very helpful.
> Please try and articulate more precisely *why* you think it's the case.
> Is it a gut-feeling?

I've outlined several times why it's a bug.  Please re-read my posts in
this thread.

> > It was me that coded up that amendment to equal, and I can remember
> > simply not taking into account the scenario we're talking about.

> Which scenario?

<sigh> Comparing two arguments using equal, at least one of which is a
symbol with position, when symbols-with-pos-enabled is nil.

> >> As I said elsewhere, I'm not sure which choice is best, but at least we
> >> have some experience with the current choice ....
> > I rather doubt that.  When have SWPs, when symbols-with-pos-enabled is
> > nil, been tested by equal, apart from in tests, maybe?

> We don't know, admittedly, but we do know that if/when it has happened,
> it hasn't caused any problem so far.

Just like binding symbols-with-position-enabled in
internal-macroexpand-for-load didn't cause any problems, until it did
(bug #65017).

> >> .... and I haven't seen any clear problem with it yet, so I'd tend to
> >> lean towards keeping the current behavior.
> > I'm wondering why you're making such a big thing out of it.  It's a
> > small change which will increase consistency and predictability in
> > Emacs in a small way, without any negative effects.

> I don't see either of the two options as being more consistent or
> more predictable.

So why are you making such a big thing out of it?  I see quite clearly
which of these options is correct.  Why won't you respect my superior
insight into the matter?

> You can see symbols' positions as being similar to strings'
> properties, which `equal` gleefully ignores.

> I think the main reasons I'm rather opposed are:

> - I don't like making `equal` depend on a global variable.

You prefer to make the effect of equal inconsistent.

>   It makes it impure, and will invalidate existing optimizations,
>   exactly like we've just witnessed for `eq`.

Which optimisations are you talking about here?  Just how is equal
optimised?  The function internal_equal will be called in all cases,
apart from, perhaps, when called with identical arguments.

> - I consider `symbols-with-pos-enabled` to be a wart, so I'd rather try
>   and minimize its use as much as possible.

Why do you want to minimise its use, wart or not?  Do you not care about
its consistency?

> >> What would be the concrete advantages of the new behavior compared to
> >> the current one?
> > There are no "concrete" advantages, aside from an insignificant increase
> > in speed for Emacs when not byte compiling.  The code and the
> > documentation currently don't match.  Fixing the code, by removing a
> > special case, is easier and more satisfying than documenting that
> > special case in the Elisp manual.

> Then, I'd vote to fix the doc rather than the code.

Write a proposed patch to the doc, then, and post it in this thread.

At this stage, I feel I must point out that arguing with you on this
mailing list about this point has taken up more time that fixing the
code and documentation did.  That is not a good thing.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

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

>> IOW affecting the behavior of `equal` is *not* part of "the point of
>> s-w-p-e".
> Which is precisely why I want to correct the behaviour of equal with
> SWPs.

I can't see the connection between the two.

>> > In the code at the moment, the result of `equal' on symbols with
>> > position is undefined, i.e. it returns a random value.
>> In which sense?
> In the sense it wasn't deliberately coded.  It's just a random value
> resulting from the code for other scenarios.

Then it's not "random" nor "undefined".
I'd describe it as "arbitrary".

>> AFAICT it returns non-nil iff the underlying bare symbols are `eq`.
>> That does not sound "random" at all to me.
>> What am I missing?
> That equal is different from eq.

Then let me rephrase the above:

    AFAICT it returns non-nil iff the underlying bare symbols are `equal`.

See?  No` eq` any more :-)

> The definition of eq (more or less) is "identical objects".
> The definition of equal (more or less) is "same structure with same
> components".

Yes, but the "more or less" is very applicable to SWP.

E.g. one could argue that if two objects are sometimes `eq`, then
I think it's a good enough justification to treat them as always
`equal`.

> See my previous paragraph of this post.  You're proposing that the
> position elements of SWPs should be ignored in equal.  I don't see any
> good reason for this.

One reason is that it's the semantics we use 99% of the time (where
`symbols-with-pos-enabled` is also non-nil).

But as I said at the beginning, my main point is that the current
behavior is not a *bug*.  It's just an arbitrary semantics, and you're
proposing to use another arbitrary semantics.
And the new arbitrary semantics does not seem clearly superior.
IOW, bikeshedding material.

>> > It's a special case when no special case is needed.
>> Making `equal` depend on a global variable is also introducing
>> a special case.
> I know you don't like symbols-with-pos-enabled, but it's there.
> It implements, by its very nature, special cases when it's non-nil.
> You want to extend those special cases to the behaviour when it's nil.

I'd say it's a biased way to look at it.

For `eq` the semantics provided by `symbols-with-pos-enabled` is
definitely very special because it is fundamentally incompatible with
the usual promise of `eq` which is that when two objects are `eq` you
can't distinguish them at all.

But the behavior for `equal` is not "special" IMO.
It fits within the general behavior of `equal`.

> The consistency of Emacs's basic functions seems very important to me,
> and it's likely very important to other people, too.  You seem to be
> dismissing it as unimportant.

No, I don't dismiss the importance of consistency in general.  I just
think here both behaviors are about equally consistent with the general
behavior of `equal`.  So consistency is not a good guideline because
it's based on nothing more than opinions.

> I've outlined several times why it's a bug.

That has not come through, I'm afraid.  All I've seen so far are
repetitions that you think the current behavior is
inconsistent/undefined/random.

None of it is concrete, and I disagree with them, so it's just my opinion
against your opinion.  We're not going to have much success with that.

Hence the need for more concrete practical arguments.

>> > It was me that coded up that amendment to equal, and I can remember
>> > simply not taking into account the scenario we're talking about.
>> Which scenario?
> <sigh> Comparing two arguments using equal, at least one of which is a
> symbol with position, when symbols-with-pos-enabled is nil.

Ah, sorry, I thought you were referring to a more concrete use-case
where such a `equal` test would occur.  Having such use-cases would help
the current discussion significantly, since currently we're basically
arguing about what Emacs should do in cases that never occur.

>> We don't know, admittedly, but we do know that if/when it has happened,
>> it hasn't caused any problem so far.
> Just like binding symbols-with-position-enabled in
> internal-macroexpand-for-load didn't cause any problems, until it did
> (bug #65017).

We don't know that.  Maybe the new behavior would be the one that
introduces such bugs.  Or maybe both.  Or neither.  We just have no idea.

> So why are you making such a big thing out of it?

[ Hmm... I have a feeling of déjà-vu.  ]

> I see quite clearly which of these options is correct.
> Why won't you respect my superior insight into the matter?

[ Hmm... this sounds a bit arrogant, so I'll just skip it.  ]

>>   It makes it impure, and will invalidate existing optimizations,
>>   exactly like we've just witnessed for `eq`.
> Which optimisations are you talking about here?  Just how is equal
> optimised?

The same one that causes my bug-fix to fail:

    (let ((symbols-with-position-enabled V)) (equal E1 E2))

is optimized to

    (equal E1 E2)


-- Stefan

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Stefan.

On Thu, Aug 10, 2023 at 10:28:55 -0400, Stefan Monnier wrote:

[ .... ]

> >> > In the code at the moment, the result of `equal' on symbols with
> >> > position is undefined, i.e. it returns a random value.
> >> In which sense?
> > In the sense it wasn't deliberately coded.  It's just a random value
> > resulting from the code for other scenarios.

> Then it's not "random" nor "undefined".
> I'd describe it as "arbitrary".

Then we're reduced to arguing about words rather than Emacs.

> >> AFAICT it returns non-nil iff the underlying bare symbols are `eq`.
> >> That does not sound "random" at all to me.
> >> What am I missing?
> > That equal is different from eq.

> Then let me rephrase the above:

>     AFAICT it returns non-nil iff the underlying bare symbols are `equal`.

> See?  No` eq` any more :-)

Yes I see, and it is wrong.

Let me try another tack.  Look closely at the definition of
symbols-with-pos-enabled:

(i) When nil: Emacs works without any special handling for SWPs.

(ii) When non-nil: Emacs handles SWPs as though they were their bare
symbols.

> > The definition of eq (more or less) is "identical objects".
> > The definition of equal (more or less) is "same structure with same
> > components".

> Yes, but the "more or less" is very applicable to SWP.

> E.g. one could argue that if two objects are sometimes `eq`, then
> I think it's a good enough justification to treat them as always
> `equal`.

A SWP is sometimes EQ its bare symbol, in particular when
symbols-with-pos-enabled is non-nil.  That is no justification for
regarding the two as _always_ EQ, in particular when s-w-p-enabled is
nil.  If you were to make that argument, you would be arguing for a
significant slowdown in Emacs.

> > See my previous paragraph of this post.  You're proposing that the
> > position elements of SWPs should be ignored in equal.  I don't see any
> > good reason for this.

> One reason is that it's the semantics we use 99% of the time (where
> `symbols-with-pos-enabled` is also non-nil).

It's not the semantics we should use when s-w-p-enabled is nil.  That
would violate the definition of symbols-with-pos-enabled.  See above.

> But as I said at the beginning, my main point is that the current
> behavior is not a *bug*.  It's just an arbitrary semantics, and you're
> proposing to use another arbitrary semantics.

I'm proposing correctness, according to a coherent definition of
symbols-with-pos-enabled.  I was surprised indeed, and continue to be
surprised, that you do not see this correction as a correction.  To me,
it's obvious.

> And the new arbitrary semantics does not seem clearly superior.
> IOW, bikeshedding material.

The correctness of symbols-with-pos-enabled is not arbitrary.

If you think the semantics are arbitrary, why are you so fixed on
implementing a particular one of them?

[ .... ]

> But the behavior for `equal` is not "special" IMO.  It fits within the
> general behavior of `equal`.

Yet you're insisting on special behaviour for SWP's when
symbols-with-pos-enabled is nil.

[ .... ]

> None of it is concrete, and I disagree with them, so it's just my opinion
> against your opinion.  We're not going to have much success with that.

> Hence the need for more concrete practical arguments.

Well, one argument, not very strong but stronger than any other you'll
accept is that the code and documentation amendments have already been
made.  There are so far no signs of the needed documentation changes for
leaving the handling of SWPs by equal contrasting with that of other
types.

> >> > It was me that coded up that amendment to equal, and I can remember
> >> > simply not taking into account the scenario we're talking about.
> >> Which scenario?
> > <sigh> Comparing two arguments using equal, at least one of which is a
> > symbol with position, when symbols-with-pos-enabled is nil.

> Ah, sorry, I thought you were referring to a more concrete use-case
> where such a `equal` test would occur.  Having such use-cases would help
> the current discussion significantly, since currently we're basically
> arguing about what Emacs should do in cases that never occur.

That is the case, yes.  There are no current use-cases for SWPs with
s-w-p-enabled nil.

[ .... ]

> > So why are you making such a big thing out of it?

> [ Hmm... I have a feeling of déjà-vu.  ]

> > I see quite clearly which of these options is correct.
> > Why won't you respect my superior insight into the matter?

> [ Hmm... this sounds a bit arrogant, so I'll just skip it.  ]

No, you won't.  There is a basic contradiction in your stance, namely
you say on the one hand that the correct strategy here is arbitrary, yet
on the other hand that the one you prefer is overwhelmingly better.  If
you were being honest about this arbitrariness, you wouldn't be making
such a song and dance about it.

> >>   It makes it impure, and will invalidate existing optimizations,
> >>   exactly like we've just witnessed for `eq`.
> > Which optimisations are you talking about here?  Just how is equal
> > optimised?

> The same one that causes my bug-fix to fail:

>     (let ((symbols-with-position-enabled V)) (equal E1 E2))

> is optimized to

>     (equal E1 E2)

What can I say?  If you eliminate semantically essential code what you
end up with is not what you started with.  Is there actually an instance
of the above code in the Emacs sources?  In particular, one where
symbols-with-position-enabled gets bound by a let, and the body of the
let contains an equal (or member, or ...), but no uses of EQ?

> -- Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

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

> I'm proposing correctness, according to a coherent definition of
> symbols-with-pos-enabled.  I was surprised indeed, and continue to be
> surprised, that you do not see this correction as a correction.  To me,
> it's obvious.

I guess it's because you see it as a feature that (symbol-function
(position-symbol 'a 3)) signals an error when `symbols-with-pos-enabled`
is nil, whereas I see it as a misfeature we should try and fix.

> That is the case, yes.  There are no current use-cases for SWPs with
> s-w-p-enabled nil.

Right.  So all the code which behaves differently when encountering an
SWP depending on the value of `s-w-p-enabled` has only one of the two
branches tested.  My preference for making the behavior oblivious to
`s-w-p-enabled` (except for those rare cases where it's needed for
performance reasons) removes these untested code paths.

In any case, here's my "counter offer" (BTW, why we do handle SWP
specially in `time-convert`?).


        Stefan


diff --git a/doc/lispref/symbols.texi b/doc/lispref/symbols.texi
index 34db0caf3a8..0eb3e8f211d 100644
--- a/doc/lispref/symbols.texi
+++ b/doc/lispref/symbols.texi
@@ -782,11 +782,16 @@ Symbols with Position
 @cindex symbol with position
 
 @cindex bare symbol
-A @dfn{symbol with position} is a symbol, the @dfn{bare symbol},
-together with an unsigned integer called the @dfn{position}.  These
-objects are intended for use by the byte compiler, which records in
-them the position of each symbol occurrence and uses those positions
-in warning and error messages.
+A @dfn{symbol with position} is a pair of a symbol, the @dfn{bare symbol},
+together with an unsigned integer called the @dfn{position}.
+They cannot themselves have entries in obarrays (contrary to their
+bare symbols; @pxref{Creating Symbols}).
+
+Symbols with position are for the use of the byte compiler, which
+records in them the position of each symbol occurrence and uses those
+positions in warning and error messages.  They shouldn't normally be
+used otherwise.  Doing so can cause unexpected results with basic
+Emacs functions such as @code{eq} and @code{equal}.
 
 The printed representation of a symbol with position uses the hash
 notation outlined in @ref{Printed Representation}.  It looks like
@@ -798,11 +803,20 @@ Symbols with Position
 
 For most purposes, when the flag variable
 @code{symbols-with-pos-enabled} is non-@code{nil}, symbols with
-positions behave just as bare symbols do.  For example, @samp{(eq
-#<symbol foo at 12345> foo)} has a value @code{t} when that variable
-is set (but @code{nil} when it isn't set).  Most of the time in Emacs this
-variable is @code{nil}, but the byte compiler binds it to @code{t}
-when it runs.
+positions behave just as their bare symbols would.  For example,
+@samp{(eq #<symbol foo at 12345> foo)} has a value @code{t} when the
+variable is set; likewise, @code{equal} will treat a symbol with
+position argument as its bare symbol.
+
+When @code{symbols-with-pos-enabled} is @code{nil}, any symbols with
+position continue to exist, but do not always behave as symbols.
+Most importantly @code{eq} only returns @code{t} when given truly
+identical arguments, for performance reasons.  @code{equal} on the
+other hand is not affected,
+
+Most of the time in Emacs @code{symbols-with-pos-enabled} is
+@code{nil}, but the byte compiler and the native compiler bind it to
+@code{t} when they run.
 
 Typically, symbols with position are created by the byte compiler
 calling the reader function @code{read-positioning-symbols}
diff --git a/src/fns.c b/src/fns.c
index d7b2e7908b6..5239eb73026 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -5166,7 +5166,7 @@ sxhash_obj (Lisp_Object obj, int depth)
 	    hash = sxhash_combine (hash, sxhash_obj (XOVERLAY (obj)->plist, depth));
 	    return SXHASH_REDUCE (hash);
 	  }
-	else if (symbols_with_pos_enabled && pvec_type == PVEC_SYMBOL_WITH_POS)
+	else if (pvec_type == PVEC_SYMBOL_WITH_POS)
 	  return sxhash_obj (XSYMBOL_WITH_POS (obj)->sym, depth + 1);
 	else
 	  /* Others are 'equal' if they are 'eq', so take their
diff --git a/src/timefns.c b/src/timefns.c
index 151f5b482a3..7e030da3fcd 100644
--- a/src/timefns.c
+++ b/src/timefns.c
@@ -1767,8 +1767,6 @@ DEFUN ("time-convert", Ftime_convert, Stime_convert, 1, 2, 0,
   enum timeform input_form = decode_lisp_time (time, false, &t, 0);
   if (NILP (form))
     form = current_time_list ? Qlist : Qt;
-  if (symbols_with_pos_enabled && SYMBOL_WITH_POS_P (form))
-    form = SYMBOL_WITH_POS_SYM (form);
   if (BASE_EQ (form, Qlist))
     return ticks_hz_list4 (t.ticks, t.hz);
   if (BASE_EQ (form, Qinteger))

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Eli Zaretskii]

> Cc: 65051@debbugs.gnu.org
> Date: Sat, 12 Aug 2023 01:36:08 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> +A @dfn{symbol with position} is a pair of a symbol, the @dfn{bare symbol},
> +together with an unsigned integer called the @dfn{position}.
> +They cannot themselves have entries in obarrays (contrary to their
> +bare symbols; @pxref{Creating Symbols}).

That "They" which begins the last sentence doesn't make clear who are
"they".  You want to say "Symbols with position" instead -- this is
repetition, but in this case it is actually needed.

>  For most purposes, when the flag variable
>  @code{symbols-with-pos-enabled} is non-@code{nil}, symbols with
> -positions behave just as bare symbols do.  For example, @samp{(eq
> -#<symbol foo at 12345> foo)} has a value @code{t} when that variable
> -is set (but @code{nil} when it isn't set).  Most of the time in Emacs this
> -variable is @code{nil}, but the byte compiler binds it to @code{t}
> -when it runs.
> +positions behave just as their bare symbols would.  For example,
   ^^^^^^^^^
"position", singular.

Thanks.

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

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

Thanks,


        Stefan


diff --git a/doc/lispref/symbols.texi b/doc/lispref/symbols.texi
index 34db0caf3a8..d2e397faf80 100644
--- a/doc/lispref/symbols.texi
+++ b/doc/lispref/symbols.texi
@@ -782,11 +782,16 @@ Symbols with Position
 @cindex symbol with position
 
 @cindex bare symbol
-A @dfn{symbol with position} is a symbol, the @dfn{bare symbol},
-together with an unsigned integer called the @dfn{position}.  These
-objects are intended for use by the byte compiler, which records in
-them the position of each symbol occurrence and uses those positions
-in warning and error messages.
+A @dfn{symbol with position} is a pair of a symbol, the @dfn{bare
+symbol}, together with an unsigned integer called the @dfn{position}.
+Symbol with position cannot themselves have entries in obarrays
+(contrary to their bare symbols; @pxref{Creating Symbols}).
+
+Symbols with position are for the use of the byte compiler, which
+records in them the position of each symbol occurrence and uses those
+positions in warning and error messages.  They shouldn't normally be
+used otherwise.  Doing so can cause unexpected results with basic
+Emacs functions such as @code{eq} and @code{equal}.
 
 The printed representation of a symbol with position uses the hash
 notation outlined in @ref{Printed Representation}.  It looks like
@@ -798,11 +803,21 @@ Symbols with Position
 
 For most purposes, when the flag variable
 @code{symbols-with-pos-enabled} is non-@code{nil}, symbols with
-positions behave just as bare symbols do.  For example, @samp{(eq
-#<symbol foo at 12345> foo)} has a value @code{t} when that variable
-is set (but @code{nil} when it isn't set).  Most of the time in Emacs this
-variable is @code{nil}, but the byte compiler binds it to @code{t}
-when it runs.
+position behave just as their bare symbols would.  For example,
+@samp{(eq #<symbol foo at 12345> foo)} has a value @code{t} when the
+variable is set; likewise, @code{equal} will treat a symbol with
+position argument as its bare symbol.
+
+When @code{symbols-with-pos-enabled} is @code{nil}, any symbols with
+position continue to exist, but do not always behave as symbols.
+Most importantly @code{eq} only returns @code{t} when given truly
+identical arguments, for performance reasons.  @code{equal} on the
+other hand continues to treat a symbol with
+position argument as its bare symbol.
+
+Most of the time in Emacs @code{symbols-with-pos-enabled} is
+@code{nil}, but the byte compiler and the native compiler bind it to
+@code{t} when they run.
 
 Typically, symbols with position are created by the byte compiler
 calling the reader function @code{read-positioning-symbols}
diff --git a/src/fns.c b/src/fns.c
index ac30670b3ac..fde4ef6b08b 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -5166,7 +5166,7 @@ sxhash_obj (Lisp_Object obj, int depth)
 	    hash = sxhash_combine (hash, sxhash_obj (XOVERLAY (obj)->plist, depth));
 	    return SXHASH_REDUCE (hash);
 	  }
-	else if (symbols_with_pos_enabled && pvec_type == PVEC_SYMBOL_WITH_POS)
+	else if (pvec_type == PVEC_SYMBOL_WITH_POS)
 	  return sxhash_obj (XSYMBOL_WITH_POS (obj)->sym, depth + 1);
 	else
 	  /* Others are 'equal' if they are 'eq', so take their

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: acm@muc.de,  65051@debbugs.gnu.org
> Date: Sat, 12 Aug 2023 14:46:24 -0400
> 
> +A @dfn{symbol with position} is a pair of a symbol, the @dfn{bare
> +symbol}, together with an unsigned integer called the @dfn{position}.
> +Symbol with position cannot themselves have entries in obarrays
   ^^^^^^
"Symbols"

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Stefan.

I haven't changed my view that the current handling of SWPs in equal is
a bug, and that the bug should be fixed.  Your patch isn't this fix.

I continue to be uneasy about the contradictions in your attidude where,
on the one hand, you say "anything we do here sucks equally" and
describe this conversation as "bikeshedding", and on the other hand
you're steamrolling over my clear vision for fixing the bug.

Nevertheless, here are my comments on your proposed patch, as promised.


On Sat, Aug 12, 2023 at 14:46:24 -0400, Stefan Monnier wrote:
> Thanks,


>         Stefan


> diff --git a/doc/lispref/symbols.texi b/doc/lispref/symbols.texi
> index 34db0caf3a8..d2e397faf80 100644
> --- a/doc/lispref/symbols.texi
> +++ b/doc/lispref/symbols.texi
> @@ -782,11 +782,16 @@ Symbols with Position
>  @cindex symbol with position
 
>  @cindex bare symbol
> -A @dfn{symbol with position} is a symbol, the @dfn{bare symbol},
> -together with an unsigned integer called the @dfn{position}.  These
> -objects are intended for use by the byte compiler, which records in
> -them the position of each symbol occurrence and uses those positions
> -in warning and error messages.
> +A @dfn{symbol with position} is a pair of a symbol, the @dfn{bare
> +symbol}, together with an unsigned integer called the @dfn{position}.
> +Symbol with position cannot themselves have entries in obarrays
> +(contrary to their bare symbols; @pxref{Creating Symbols}).

"Cannot" is inappropriate here, since there is nothing regrettable about
SWPs not being stored in obarrays.  "Contrary" is also inappropriate
since there is no disagreement, opposition, or conflict between the two
things.  "As contrasted with" would be better.  On the other hand, why
didn't you just leave it as I had left it?

> +
> +Symbols with position are for the use of the byte compiler, which
> +records in them the position of each symbol occurrence and uses those
> +positions in warning and error messages.  They shouldn't normally be
> +used otherwise.  Doing so can cause unexpected results with basic
> +Emacs functions such as @code{eq} and @code{equal}.
 
>  The printed representation of a symbol with position uses the hash
>  notation outlined in @ref{Printed Representation}.  It looks like
> @@ -798,11 +803,21 @@ Symbols with Position
 
>  For most purposes, when the flag variable
>  @code{symbols-with-pos-enabled} is non-@code{nil}, symbols with
> -positions behave just as bare symbols do.  For example, @samp{(eq
> -#<symbol foo at 12345> foo)} has a value @code{t} when that variable
> -is set (but @code{nil} when it isn't set).  Most of the time in Emacs this
> -variable is @code{nil}, but the byte compiler binds it to @code{t}
> -when it runs.
> +position behave just as their bare symbols would.  For example,
> +@samp{(eq #<symbol foo at 12345> foo)} has a value @code{t} when the
> +variable is set; likewise, @code{equal} will treat a symbol with
> +position argument as its bare symbol.

This is suboptimal for your version.  The paragraph is about Emacs's
behaviour when symbols-with-pos-enabled is non-nil.  You're including
behaviour in this paragraph which you want to be independent of
s-w-p-enabled.  It really needs its own paragraph.

> +
> +When @code{symbols-with-pos-enabled} is @code{nil}, any symbols with
> +position continue to exist, but do not always behave as symbols.

The "do not always" is vaguer than it should be.  The doc should be
explicit about when SWPs behave as symbols, and when not.

> +Most importantly @code{eq} only returns @code{t} when given truly
> +identical arguments, for performance reasons.  ....

There's more to it than "for performance reasons".  I think you could
omit ", for performance reasons" since it is more likely to cause
confusion than anything else.

>                                            .... @code{equal} on the
> +other hand continues to treat a symbol with
> +position argument as its bare symbol.

"Continues to" is inappropriate here, since there is nothing continuous
happening, nor a continuous "treating" of anything.  A useful phrase
might be "regardless of the value of @code{symbols-with-pos-enabled}".
But as noted above for s-w-p-e's non-nil case, this doesn't belong in
the paragraph about the behaviour when s-w-p-enabled is nil.

> +
> +Most of the time in Emacs @code{symbols-with-pos-enabled} is
> +@code{nil}, but the byte compiler and the native compiler bind it to
> +@code{t} when they run.
 
>  Typically, symbols with position are created by the byte compiler
>  calling the reader function @code{read-positioning-symbols}

What's missing:  (i) You'll need to amend the definition of
symbols-with-pos-enabled in its @defvar, since you're changing it's
meaning.  (ii) You should document the behaviour of SWP's in the
sections on `eq' and `equal'.  (iii) See below.

> diff --git a/src/fns.c b/src/fns.c
> index ac30670b3ac..fde4ef6b08b 100644
> --- a/src/fns.c
> +++ b/src/fns.c
> @@ -5166,7 +5166,7 @@ sxhash_obj (Lisp_Object obj, int depth)
>  	    hash = sxhash_combine (hash, sxhash_obj (XOVERLAY (obj)->plist, depth));
>  	    return SXHASH_REDUCE (hash);
>  	  }
> -	else if (symbols_with_pos_enabled && pvec_type == PVEC_SYMBOL_WITH_POS)
> +	else if (pvec_type == PVEC_SYMBOL_WITH_POS)
>  	  return sxhash_obj (XSYMBOL_WITH_POS (obj)->sym, depth + 1);
>  	else
>  	  /* Others are 'equal' if they are 'eq', so take their
  
Have you tested this thoroughly?  Hash tables were one of the more
troublesome things to get right when I was developing this stuff.  It
also needs documenting in the hash table chapter of the elisp manual.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Stefan.

On Sat, Aug 12, 2023 at 01:36:08 -0400, Stefan Monnier wrote:

[ .... ]

> ..... (BTW, why we do handle SWP specially in `time-convert`?).

That was this patch:

commit 343482d641511b54aa0444791770b4ea70d27cc7
Author: Paul Eggert <eggert@cs.ucla.edu>
Date:   Wed Jun 15 23:08:03 2022 -0500

    Streamline time decoding and conversion

    * src/lisp.h (lisp_h_BASE2_EQ, BASE2_EQ): New macros and functions.
    * src/timefns.c (tzlookup, Fdecode_time): Use them.
    (Ftime_convert): Convert to symbol once, instead of many times.  <=====

..  form, if a SWP, gets replaced by its bare symbol, so that the
following comparisons can be done by BASE_EQ rather than EQ.  It's an
optimisation, though I can't really see why it's worthwhile.

If you remove that SWP handling, as you are proposing, you'll need to
set these BASE_EQ's back to EQ's, too.

>         Stefan

[ .... ]

> diff --git a/src/timefns.c b/src/timefns.c
> index 151f5b482a3..7e030da3fcd 100644
> --- a/src/timefns.c
> +++ b/src/timefns.c
> @@ -1767,8 +1767,6 @@ DEFUN ("time-convert", Ftime_convert, Stime_convert, 1, 2, 0,
>    enum timeform input_form = decode_lisp_time (time, false, &t, 0);
>    if (NILP (form))
>      form = current_time_list ? Qlist : Qt;
> -  if (symbols_with_pos_enabled && SYMBOL_WITH_POS_P (form))
> -    form = SYMBOL_WITH_POS_SYM (form);
>    if (BASE_EQ (form, Qlist))
>      return ticks_hz_list4 (t.ticks, t.hz);
>    if (BASE_EQ (form, Qinteger))

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

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

>     Streamline time decoding and conversion
>
>     * src/lisp.h (lisp_h_BASE2_EQ, BASE2_EQ): New macros and functions.
>     * src/timefns.c (tzlookup, Fdecode_time): Use them.
>     (Ftime_convert): Convert to symbol once, instead of many times.  <=====
>
> ..  form, if a SWP, gets replaced by its bare symbol, so that the
> following comparisons can be done by BASE_EQ rather than EQ.  It's an
> optimisation, though I can't really see why it's worthwhile.
>
> If you remove that SWP handling, as you are proposing, you'll need to
> set these BASE_EQ's back to EQ's, too.

I see, thanks.

It's kind of odd, tho: it's the only function that does that, AFAICT.
I also can't see why `time-convert` should accept SWPs in addition to
bare symbols.  In my book, it seems like passing it an SWP would be
a mistake.


        Stefan

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Stefan.

On Sat, Aug 12, 2023 at 14:07:39 -0400, Stefan Monnier wrote:
> >     Streamline time decoding and conversion

> >     * src/lisp.h (lisp_h_BASE2_EQ, BASE2_EQ): New macros and functions.
> >     * src/timefns.c (tzlookup, Fdecode_time): Use them.
> >     (Ftime_convert): Convert to symbol once, instead of many times.  <=====

> > ..  form, if a SWP, gets replaced by its bare symbol, so that the
> > following comparisons can be done by BASE_EQ rather than EQ.  It's an
> > optimisation, though I can't really see why it's worthwhile.

> > If you remove that SWP handling, as you are proposing, you'll need to
> > set these BASE_EQ's back to EQ's, too.

> I see, thanks.

> It's kind of odd, tho: it's the only function that does that, AFAICT.
> I also can't see why `time-convert` should accept SWPs in addition to
> bare symbols.  In my book, it seems like passing it an SWP would be
> a mistake.

Anything which can accept symbols should accept SWPs when
symbols-with-pos-enabled is non-nil.  It's just basic consistency.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Stefan.

On Sat, Aug 12, 2023 at 01:36:08 -0400, Stefan Monnier wrote:
> > I'm proposing correctness, according to a coherent definition of
> > symbols-with-pos-enabled.  I was surprised indeed, and continue to be
> > surprised, that you do not see this correction as a correction.  To me,
> > it's obvious.

> I guess it's because you see it as a feature that (symbol-function
> (position-symbol 'a 3)) signals an error when `symbols-with-pos-enabled`
> is nil, ....

No, not for that reason, but for the reasons I've outlined already at
great length.

> .... whereas I see it as a misfeature we should try and fix.

What, you want to get a symbol-function from something which isn't a
symbol?  That's a misfeature?  What's stopping you binding s-w-p-enabled
to t for the operation you have in mind?

In the current design, a SWP acts as its bare symbol when and only when
symbols-with-pos-enabled is non-nil.  Simple, consequent, and elegant.
Why do you want to mess up that design?

> > That is the case, yes.  There are no current use-cases for SWPs with
> > s-w-p-enabled nil.

> Right.  So all the code which behaves differently when encountering an
> SWP depending on the value of `s-w-p-enabled` has only one of the two
> branches tested.

Not true.  I have written tests into fns-tests.el (not yet committed) to
test them.

> My preference for making the behavior ....

What behaviour, exactly?

> .... oblivious to `s-w-p-enabled` (except for those rare cases where
> it's needed for performance reasons) removes these untested code
> paths.

Rubbish!  Those "untested code paths" will remain untested in your
version until you test them.  In my version, where I fix the bug, they
have already been tested.

And what exactly do you mean by "those rare cases where <something?>'s
needed for performance reasons"?  What's the <something>, here?  What
exactly are you referring to?

> In any case, here's my "counter offer".  [ .... ]

Thanks!  It's incomplete, and there are several English usage errors in
it.  I'll get back to you tomorrow.

But I still say we should fix the bug in the code.  Anything you want to
do with SWPs when you want to regard them as their bare symbols, you can
do by binding symbols-with-pos-enabled to t.  With your version of (not)
fixing the bug, there is no convenient way to do a (full) equal
operation on two SWPs when s-w-p-enabled is nil.  Who knows what we
might want to do with them in the future?

>         Stefan


> diff --git a/doc/lispref/symbols.texi b/doc/lispref/symbols.texi
> index 34db0caf3a8..0eb3e8f211d 100644
> --- a/doc/lispref/symbols.texi
> +++ b/doc/lispref/symbols.texi
> @@ -782,11 +782,16 @@ Symbols with Position
>  @cindex symbol with position
 
>  @cindex bare symbol
> -A @dfn{symbol with position} is a symbol, the @dfn{bare symbol},
> -together with an unsigned integer called the @dfn{position}.  These
> -objects are intended for use by the byte compiler, which records in
> -them the position of each symbol occurrence and uses those positions
> -in warning and error messages.
> +A @dfn{symbol with position} is a pair of a symbol, the @dfn{bare symbol},
> +together with an unsigned integer called the @dfn{position}.
> +They cannot themselves have entries in obarrays (contrary to their
> +bare symbols; @pxref{Creating Symbols}).
> +
> +Symbols with position are for the use of the byte compiler, which
> +records in them the position of each symbol occurrence and uses those
> +positions in warning and error messages.  They shouldn't normally be
> +used otherwise.  Doing so can cause unexpected results with basic
> +Emacs functions such as @code{eq} and @code{equal}.
 
>  The printed representation of a symbol with position uses the hash
>  notation outlined in @ref{Printed Representation}.  It looks like
> @@ -798,11 +803,20 @@ Symbols with Position
 
>  For most purposes, when the flag variable
>  @code{symbols-with-pos-enabled} is non-@code{nil}, symbols with
> -positions behave just as bare symbols do.  For example, @samp{(eq
> -#<symbol foo at 12345> foo)} has a value @code{t} when that variable
> -is set (but @code{nil} when it isn't set).  Most of the time in Emacs this
> -variable is @code{nil}, but the byte compiler binds it to @code{t}
> -when it runs.
> +positions behave just as their bare symbols would.  For example,
> +@samp{(eq #<symbol foo at 12345> foo)} has a value @code{t} when the
> +variable is set; likewise, @code{equal} will treat a symbol with
> +position argument as its bare symbol.
> +
> +When @code{symbols-with-pos-enabled} is @code{nil}, any symbols with
> +position continue to exist, but do not always behave as symbols.
> +Most importantly @code{eq} only returns @code{t} when given truly
> +identical arguments, for performance reasons.  @code{equal} on the
> +other hand is not affected,
> +
> +Most of the time in Emacs @code{symbols-with-pos-enabled} is
> +@code{nil}, but the byte compiler and the native compiler bind it to
> +@code{t} when they run.
 
>  Typically, symbols with position are created by the byte compiler
>  calling the reader function @code{read-positioning-symbols}
> diff --git a/src/fns.c b/src/fns.c
> index d7b2e7908b6..5239eb73026 100644
> --- a/src/fns.c
> +++ b/src/fns.c
> @@ -5166,7 +5166,7 @@ sxhash_obj (Lisp_Object obj, int depth)
>  	    hash = sxhash_combine (hash, sxhash_obj (XOVERLAY (obj)->plist, depth));
>  	    return SXHASH_REDUCE (hash);
>  	  }
> -	else if (symbols_with_pos_enabled && pvec_type == PVEC_SYMBOL_WITH_POS)
> +	else if (pvec_type == PVEC_SYMBOL_WITH_POS)
>  	  return sxhash_obj (XSYMBOL_WITH_POS (obj)->sym, depth + 1);
>  	else
>  	  /* Others are 'equal' if they are 'eq', so take their
> diff --git a/src/timefns.c b/src/timefns.c
> index 151f5b482a3..7e030da3fcd 100644
> --- a/src/timefns.c
> +++ b/src/timefns.c
> @@ -1767,8 +1767,6 @@ DEFUN ("time-convert", Ftime_convert, Stime_convert, 1, 2, 0,
>    enum timeform input_form = decode_lisp_time (time, false, &t, 0);
>    if (NILP (form))
>      form = current_time_list ? Qlist : Qt;
> -  if (symbols_with_pos_enabled && SYMBOL_WITH_POS_P (form))
> -    form = SYMBOL_WITH_POS_SYM (form);
>    if (BASE_EQ (form, Qlist))
>      return ticks_hz_list4 (t.ticks, t.hz);
>    if (BASE_EQ (form, Qinteger))


-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Dmitry Gutov]

On 08/08/2023 18:33, Alan Mackenzie wrote:
>> So I'm still wondering why you think it's a bug.
> Because it violates the definition and basic understanding of equal.
> It's a special case when no special case is needed.
> 

Does 'equal'-ity of strings with and without text properties (or with 
different text properties) violate these as well?

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Dmitry.

On Fri, Aug 11, 2023 at 03:51:09 +0300, Dmitry Gutov wrote:
> On 08/08/2023 18:33, Alan Mackenzie wrote:
> >> So I'm still wondering why you think it's a bug.
> > Because it violates the definition and basic understanding of equal.
> > It's a special case when no special case is needed.

> Does 'equal'-ity of strings with and without text properties (or with 
> different text properties) violate these as well?

Maybe, maybe not.  It depends on whether you consider the text
properties on a string (or buffer portion) an essential part of the
string or not.  I think it was possibly a design error to have text
properties conceptually as a part of a string/buffer rather than
something associated with it, like an overlay.  The fact that equal
ignores these properties supports this view.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Dmitry Gutov]

On 11/08/2023 13:42, Alan Mackenzie wrote:
> Hello, Dmitry.
> 
> On Fri, Aug 11, 2023 at 03:51:09 +0300, Dmitry Gutov wrote:
>> On 08/08/2023 18:33, Alan Mackenzie wrote:
>>>> So I'm still wondering why you think it's a bug.
>>> Because it violates the definition and basic understanding of equal.
>>> It's a special case when no special case is needed.
> 
>> Does 'equal'-ity of strings with and without text properties (or with
>> different text properties) violate these as well?
> 
> Maybe, maybe not.  It depends on whether you consider the text
> properties on a string (or buffer portion) an essential part of the
> string or not.

...or, like, a metadata attached to a value. Which is an approach to 
have its benefits as well, seeing how we've been living with it for many 
years.

> I think it was possibly a design error to have text
> properties conceptually as a part of a string/buffer rather than
> something associated with it, like an overlay.  The fact that equal
> ignores these properties supports this view.

We needed a reference to access the properties from. Overlays are 
different because they attach to a buffer. There is nothing else to 
attach to when you have a string value.

Which seems very similar to the situation with symbols, I think.

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Dmitry.

On Fri, Aug 11, 2023 at 14:18:15 +0300, Dmitry Gutov wrote:
> On 11/08/2023 13:42, Alan Mackenzie wrote:
> > On Fri, Aug 11, 2023 at 03:51:09 +0300, Dmitry Gutov wrote:
> >> On 08/08/2023 18:33, Alan Mackenzie wrote:
> >>>> So I'm still wondering why you think it's a bug.
> >>> Because it violates the definition and basic understanding of equal.
> >>> It's a special case when no special case is needed.

> >> Does 'equal'-ity of strings with and without text properties (or with
> >> different text properties) violate these as well?

> > Maybe, maybe not.  It depends on whether you consider the text
> > properties on a string (or buffer portion) an essential part of the
> > string or not.

> ...or, like, a metadata attached to a value. Which is an approach to 
> have its benefits as well, seeing how we've been living with it for many 
> years.

> > I think it was possibly a design error to have text
> > properties conceptually as a part of a string/buffer rather than
> > something associated with it, like an overlay.  The fact that equal
> > ignores these properties supports this view.

> We needed a reference to access the properties from. Overlays are 
> different because they attach to a buffer. There is nothing else to 
> attach to when you have a string value.

This is arbitrary; overlays _could_ have been made attachable to
strings, in which case text properties need not have been.  That would
have prevented all the heart searching when considering equal with
strings.

> Which seems very similar to the situation with symbols, I think.

There are practical differences.  Having symbols with position simply
handled as their bare symbols would slow down Emacs quite a lot.  That's
why we have symbols-with-pos-enabled.  But you know that.  Currently,
the working of s-w-p-enabled is inconsistent, and should be fixed, which
is what this bug is about.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Dmitry Gutov]

Hi again, Alan,

On 11/08/2023 15:05, Alan Mackenzie wrote:

>>> I think it was possibly a design error to have text
>>> properties conceptually as a part of a string/buffer rather than
>>> something associated with it, like an overlay.  The fact that equal
>>> ignores these properties supports this view.
> 
>> We needed a reference to access the properties from. Overlays are
>> different because they attach to a buffer. There is nothing else to
>> attach to when you have a string value.
> 
> This is arbitrary; overlays _could_ have been made attachable to
> strings, in which case text properties need not have been.  That would
> have prevented all the heart searching when considering equal with
> strings.

Then we would have some "metadata" that's part of the value, and some 
that is not part of the value. How would we look those up, though? 
Through a global registry?

equal-including-properties is useful enough, by the way. In the tests, 
at least.

>> Which seems very similar to the situation with symbols, I think.
> 
> There are practical differences.  Having symbols with position simply
> handled as their bare symbols would slow down Emacs quite a lot.  That's
> why we have symbols-with-pos-enabled.  But you know that.

Does the current impl of 'equal' create worse performance as well? That 
would be a good argument to change it.

 >  Currently,
 > the working of s-w-p-enabled is inconsistent, and should be fixed, which
 > is what this bug is about.

Inconsistent with what? If we're talking about the relation between 
EQUAL and EQ, objects that are EQ have to be EQUAL, but those that are 
EQUAL don't have to be EQ.

Anyway, I'd like to offer a question from a different perspective: 
should two symbols-with-positions where the positions are different but 
the symbol is the same, be equal between each other? If yes (which is my 
reading of fns.c:2755), then it makes sense for them to be equal-able to 
symbols without positions as well.

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Alan Mackenzie]

Hello, Dmitry.

On Fri, Aug 11, 2023 at 16:19:47 +0300, Dmitry Gutov wrote:
> Hi again, Alan,

> On 11/08/2023 15:05, Alan Mackenzie wrote:

> >>> I think it was possibly a design error to have text
> >>> properties conceptually as a part of a string/buffer rather than
> >>> something associated with it, like an overlay.  The fact that equal
> >>> ignores these properties supports this view.

> >> We needed a reference to access the properties from. Overlays are
> >> different because they attach to a buffer. There is nothing else to
> >> attach to when you have a string value.

> > This is arbitrary; overlays _could_ have been made attachable to
> > strings, in which case text properties need not have been.  That would
> > have prevented all the heart searching when considering equal with
> > strings.

> Then we would have some "metadata" that's part of the value, and some 
> that is not part of the value. How would we look those up, though? 
> Through a global registry?

> equal-including-properties is useful enough, by the way. In the tests, 
> at least.

Yes.

> >> Which seems very similar to the situation with symbols, I think.

> > There are practical differences.  Having symbols with position simply
> > handled as their bare symbols would slow down Emacs quite a lot.  That's
> > why we have symbols-with-pos-enabled.  But you know that.

> Does the current impl of 'equal' create worse performance as well? That 
> would be a good argument to change it.

Yes, but unmeasurably so.  The current implementation has two
comparisons, quite complicated, where only one simple one is needed, for
the typical use-case.

>  >  Currently, the working of s-w-p-enabled is inconsistent, and
>  >  should be fixed, which is what this bug is about.

> Inconsistent with what?

With its definition: when s-w-p-enabled is non-nil, SWPs are handled
specially.  When it's nil, they're not (or, at least, shouldn't be).

> If we're talking about the relation between EQUAL and EQ, objects that
> are EQ have to be EQUAL, but those that are EQUAL don't have to be EQ.

I wasn't talking about that relationship, no, but there is no danger to
it in fixing the current bug (or, indeed, in leaving it unfixed).

> Anyway, I'd like to offer a question from a different perspective: 
> should two symbols-with-positions where the positions are different but 
> the symbol is the same, be equal between each other?

Yes, when and only when symbols-with-pos-enabled is non-nil.

> If yes (which is my reading of fns.c:2755), then it makes sense for
> them to be equal-able to symbols without positions as well.

Again, this should be the case when s-w-p-enabled is non-nil and only
then.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#65051: internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.

[Dmitry Gutov]

Hi Alan,

On 11/08/2023 17:04, Alan Mackenzie wrote:

>>   >  Currently, the working of s-w-p-enabled is inconsistent, and
>>   >  should be fixed, which is what this bug is about.
> 
>> Inconsistent with what?
> 
> With its definition: when s-w-p-enabled is non-nil, SWPs are handled
> specially.  When it's nil, they're not (or, at least, shouldn't be).

Point taken. So either the behavior needs to be changed, or the 
docstring updated.

>> If we're talking about the relation between EQUAL and EQ, objects that
>> are EQ have to be EQUAL, but those that are EQUAL don't have to be EQ.
> 
> I wasn't talking about that relationship, no, but there is no danger to
> it in fixing the current bug (or, indeed, in leaving it unfixed).

Yep.

>> Anyway, I'd like to offer a question from a different perspective:
>> should two symbols-with-positions where the positions are different but
>> the symbol is the same, be equal between each other?
> 
> Yes, when and only when symbols-with-pos-enabled is non-nil.
> 
>> If yes (which is my reading of fns.c:2755), then it makes sense for
>> them to be equal-able to symbols without positions as well.
> 
> Again, this should be the case when s-w-p-enabled is non-nil and only
> then.

All right, that also makes sense.

And I can see some theoretical benefit to not having these kinds of 
objects be 'equal' in contexts where that is not anticipated in advance 
(and so the variable is not bound). Especially in a stronger-typed 
language where such comparison or pattern matching could result in an 
error (e.g. comparing incompatible types). In our case, since we're just 
talking about 'equal', the comparison could result in execution just 
falling though and e.g. some bytecomp optimization not being applied, 
silently (hence the talk of "if not broken don't fix it").

It would be nice to see a piece of code that would benefit from the 
distinction. The reverse example I can imagine myself (some pcase form 
outside of any such binding, whether by accident or not).

bug#65051: Acknowledgement (internal_equal manipulates symbols with position without checking symbols-with-pos-enabled.)

[Alan Mackenzie]

Bug fixed in the master branch, 2023-09-04.

-- 
Alan Mackenzie (Nuremberg, Germany).