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

[Alan Mackenzie <acm@muc.de>]

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