Re: master 0a57dfcff8d 2/2: Ensure that specbind arg is always bare symbol, and drop check

Re: master 0a57dfcff8d 2/2: Ensure that specbind arg is always bare symbol, and drop check

[Stefan Monnier]

>     Ensure that specbind arg is always bare symbol, and drop check
>     
>     * src/eval.c (FletX, Flet, internal_lisp_condition_case)
>     (funcall_lambda): Ensure that the first argument to `specbind` is
>     a bare symbol in the few cases where this isn't statically guaranteed.
>     (specbind): Drop the symbol argument type check on the fast path.

Shouldn't we signal an error if the first arg to `specbind` is a sympos
instead of bending over backward to accommodate incorrect use?
Maybe I'm missing some context, but IIRC we had concluded that if sympos
reach actual evaluation it's a sign of a bug upstream.


        Stefan

> ---
>  src/eval.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/src/eval.c b/src/eval.c
> index 7f7a70b15ae..c5b8a375af4 100644
> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -948,8 +948,9 @@ usage: (let* VARLIST BODY...)  */)
>  	  val = eval_sub (Fcar (XCDR (elt)));
>  	}
>  
> -      if (!NILP (lexenv) && SYMBOLP (var)
> -	  && !XSYMBOL (var)->u.s.declared_special
> +      var = maybe_remove_pos_from_symbol (var);
> +      if (!NILP (lexenv) && BARE_SYMBOL_P (var)
> +	  && !XBARE_SYMBOL (var)->u.s.declared_special
>  	  && NILP (Fmemq (var, Vinternal_interpreter_environment)))
>  	/* Lexically bind VAR by adding it to the interpreter's binding
>  	   alist.  */
> @@ -1016,11 +1017,10 @@ usage: (let VARLIST BODY...)  */)
>    varlist = XCAR (args);
>    for (argnum = 0; argnum < nvars && CONSP (varlist); argnum++)
>      {
> -      Lisp_Object var;
> -
>        elt = XCAR (varlist);
>        varlist = XCDR (varlist);
> -      var = SYMBOLP (elt) ? elt : Fcar (elt);
> +      Lisp_Object var = maybe_remove_pos_from_symbol (SYMBOLP (elt) ? elt
> +						      : Fcar (elt));
>        tem = temps[argnum];
>  
>        if (!NILP (lexenv) && SYMBOLP (var)
> @@ -1416,6 +1416,7 @@ internal_lisp_condition_case (Lisp_Object var, Lisp_Object bodyform,
>    struct handler *oldhandlerlist = handlerlist;
>    ptrdiff_t CACHEABLE clausenb = 0;
>  
> +  var = maybe_remove_pos_from_symbol (var);
>    CHECK_SYMBOL (var);
>  
>    Lisp_Object success_handler = Qnil;
> @@ -3254,7 +3255,7 @@ funcall_lambda (Lisp_Object fun, ptrdiff_t nargs, Lisp_Object *arg_vector)
>  	    lexenv = Fcons (Fcons (next, arg), lexenv);
>  	  else
>  	    /* Dynamically bind NEXT.  */
> -	    specbind (next, arg);
> +	    specbind (maybe_remove_pos_from_symbol (next), arg);
>  	  previous_rest = false;
>  	}
>      }
> @@ -3466,10 +3467,8 @@ do_specbind (struct Lisp_Symbol *sym, union specbinding *bind,
>  void
>  specbind (Lisp_Object symbol, Lisp_Object value)
>  {
> -  struct Lisp_Symbol *sym;
> -
> -  CHECK_SYMBOL (symbol);
> -  sym = XSYMBOL (symbol);
> +  /* The caller must ensure that the SYMBOL argument is a bare symbol.  */
> +  struct Lisp_Symbol *sym = XBARE_SYMBOL (symbol);
>  
>   start:
>    switch (sym->u.s.redirect)

Re: master 0a57dfcff8d 2/2: Ensure that specbind arg is always bare symbol, and drop check

[Mattias Engdegård]

20 apr. 2024 kl. 03.48 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> Maybe I'm missing some context, but IIRC we had concluded that if sympos
> reach actual evaluation it's a sign of a bug upstream.

I certainly wouldn't mind using CHECK_BARE_SYMBOL_P but since the old code tolerated sympos as variable names in interpreted variable bindings, I just kept the semantics unchanged.
This change was just a cycle shave.

Re: master 0a57dfcff8d 2/2: Ensure that specbind arg is always bare symbol, and drop check

[Alan Mackenzie]

Hello, Mattias.

On Sat, Apr 20, 2024 at 09:29:24 +0200, Mattias Engdegård wrote:
> 20 apr. 2024 kl. 03.48 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> > Maybe I'm missing some context, but IIRC we had concluded that if sympos
> > reach actual evaluation it's a sign of a bug upstream.

> I certainly wouldn't mind using CHECK_BARE_SYMBOL_P but since the old
> code tolerated sympos as variable names in interpreted variable
> bindings, I just kept the semantics unchanged.  This change was just a
> cycle shave.

How does that address Stefan's question?  It seems to me it doesn't at
all.

Have you changed _any_ of the semantics, here?

It seems to me that when symbols_with_pos_enabled is non-nil, SWPs are
intended to be used as symbols, so provided the replacement is done only
in that case (which seems to be so), then the code will carry on working
according to its design.

-- 
Alan Mackenzie (Nuremberg, Germany).