Subject: bug#75784: Typo "unreadeable" in print.c; was exposed to Lisp

From: Stefan Kangas <[email-masked]>
Date: Fri, 24 Jan 2025 10:52:10 -0600

Pip Cet <[email-masked]> writes:

> "Stefan Kangas" <[email-masked]> writes:
>
>> Do we have a way to add an obsoletion warning to both names?
>
> I'm sorry: why both names?  The new name should be permanent, unless we
> decide to move to a different name entirely to get rid of the typo (this
> is what happened with kdb-macro-redisplay, IIRC).

Sorry, that was a typo.  I meant the old name only.

> While we could check for an obsoletion property on the symbol in
> question in print_bind_overrides, it's very likely that if anyone
> actually uses this code, they use it in an automated loop, so the
> warning would need rate-limiting as well.  My suggestion is to print the
> warning just once, increasing the risk it'll be missed but saving us
> having to write a rate-limiting facility for Emacs which, if I
> understand the context directly, cannot use Lisp.

Printing it once is what we usually do in similar circumstances, sounds
good.

Pip Cet <[email-masked]> writes:

> Here's a better patch.  While this goes overboard and fixes the case
> that Vprint_variable_mapping has become invalid, I'm not sure this can
> happen: Fassq can quit, but the Lisp backtrace won't expose its
> arguments to the debugger, it seems.  I'd like to err on the side of
> caution and double-check that this mapping is still valid in case
> someone somehow exposes it to Lisp.
>
> Note that specbind currently insists on a BARE_SYMBOL.  We'll have to
> check all callers to make sure they never pass a symbol-with-position!

I guess we could also patch specbind.  Not very pretty, overall.

> Tested only very briefly.
>
> From 879714122b1de7852be898a7940ed85aefde6e2e Mon Sep 17 00:00:00 2001
> From: Pip Cet <[email-masked]>
> Subject: [PATCH] Fix typo (and bugs) in prin1's overrides code (bug#75754)
>
> Vprint_variable_mapping may have been exposed to Lisp via the
> backtrace if there was a previous quit in the Fassq call, requiring us
> to check it still has the intended format.
>
> * src/print.c (print_create_variable_mapping): Add correct spelling
> "unreadable-function" as an alternative to "unreadeable-function".
> (print_bind_overrides): Preserve original argument for error case. Use
> FOR_EACH_TAIL, fixing infloop.  Warn about obsolete symbol.  Guard
> against invalid Vprint_variable_mapping.
> ---
>  src/print.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/src/print.c b/src/print.c
> index 43698f309b1..0da2baa6e09 100644
> --- a/src/print.c
> +++ b/src/print.c
> @@ -676,6 +676,8 @@ print_create_variable_mapping (void)
>  	   intern ("print-escape-multibyte"), Qnil),
>      list3 (intern ("charset-text-property"),
>  	   intern ("print-charset-text-property"), Qnil),
> +    list3 (intern ("unreadable-function"),
> +	   intern ("print-unreadable-function"), Qnil),
>      list3 (intern ("unreadeable-function"),
>  	   intern ("print-unreadable-function"), Qnil),

Please add a comment saying why we are keeping the typo, and that
it should be deleted at some point.

>      list3 (intern ("gensym"), intern ("print-gensym"), Qnil),
> @@ -698,11 +700,10 @@ print_bind_overrides (Lisp_Object overrides)
>
>    if (EQ (overrides, Qt))
>      print_bind_all_defaults ();
> -  else if (!CONSP (overrides))
> -    xsignal (Qwrong_type_argument, Qconsp);

FOR_EACH_TAIL won't raise a signal, AFAICT.  Do we really want to stop
signaling for the wrong argument?  And similarly below.

>    else
>      {
> -      while (!NILP (overrides))
> +      Lisp_Object original_overrides = overrides;
> +      FOR_EACH_TAIL (overrides)
>  	{
>  	  Lisp_Object setting = XCAR (overrides);
>  	  if (EQ (setting, Qt))
> @@ -714,15 +715,23 @@ print_bind_overrides (Lisp_Object overrides)
>  	      Lisp_Object key = XCAR (setting),
>  		value = XCDR (setting);
>  	      Lisp_Object map = Fassq (key, Vprint_variable_mapping);
> +	      static bool warned;
> +	      if (!warned && SYMBOLP (key) &&
> +		  strcmp (SSDATA (SYMBOL_NAME (key)),
> +			  "unreadeable-function") == 0)
> +		{
> +		  warned = true;
> +		  add_to_log ("Obsolete symbol `unreadeable-function' used.  Use `unreadable-function' instead.");
> +		}

Nice. I don't think we want to end the message with a period though,
according to our conventions, do we?

I'd suggest:

		  add_to_log ("\
Obsolete symbol `unreadeable-function'; use `unreadable-function' instead");

Using "\ trades one kind of ugly for another, so it's up to you which
you find less grating.  I have no real opinion.

>  	      if (NILP (map))
>  		xsignal2 (Qwrong_type_argument, Qsymbolp, map);
> -	      specbind (XCAR (XCDR (map)), value);
> +	      Lisp_Object bare_symbol =
> +		maybe_remove_pos_from_symbol (Fcar (XCDR (map)));
> +	      CHECK_SYMBOL (bare_symbol);
> +	      specbind (bare_symbol, value);
>  	    }
> -
> -	  if (!NILP (XCDR (overrides)) && !CONSP (XCDR (overrides)))
> -	    xsignal (Qwrong_type_argument, Qconsp);
> -	  overrides = XCDR (overrides);
>  	}
> +      CHECK_LIST_END (overrides, original_overrides);
>      }
>  }
>
> --
> 2.47.1