Package: emacs;
Reported by: Pip Cet <pipcet <at> protonmail.com>
Date: Thu, 23 Jan 2025 12:22:01 UTC
Severity: minor
Message #20 received at 75784 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Kangas <stefankangas <at> gmail.com> To: Pip Cet <pipcet <at> protonmail.com> Cc: 75784 <at> debbugs.gnu.org Subject: Re: bug#75784: Typo "unreadeable" in print.c; was exposed to Lisp Date: Fri, 24 Jan 2025 10:52:10 -0600
Pip Cet <pipcet <at> protonmail.com> writes:
> "Stefan Kangas" <stefankangas <at> gmail.com> 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 <pipcet <at> protonmail.com> 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 <pipcet <at> protonmail.com>
> 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.