bug#73584: 29.3; read-key

bug#73584: 29.3; read-key

[Devon Sean McCullough]

(read-key 0) jails Emacs in a null keymap.

To escape:  Menu Bar > Help > Emacs Psychotherapist

         Peace
             --Devon

P.S.  In a tty Emacs you're out of luck
unless you're running the emacs server.

In GNU Emacs 29.3 (build 1, x86_64-apple-darwin18.7.0, NS appkit-1671.60
  Version 10.14.6 (Build 18G9323)) of 2024-03-24 built on
  builder10-14.lan
Windowing system distributor 'Apple', version 10.3.1671
System Description:  Mac OS X 10.14.6

Configured using:
  'configure --with-ns '--enable-locallisppath=/Library/Application
  Support/Emacs/${version}/site-lisp:/Library/Application
  Support/Emacs/site-lisp' --with-modules 'CFLAGS=-DFD_SETSIZE=10000
  -DDARWIN_UNLIMITED_SELECT' --with-x-toolkit=no'

Configured features:
ACL GLIB GMP GNUTLS JPEG JSON LIBXML2 MODULES NOTIFY KQUEUE NS PDUMPER
PNG RSVG SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS TREE_SITTER ZLIB

Important settings:
   value of $LC_CTYPE: UTF-8
   value of $LANG: en_US.UTF-8
   locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
   tooltip-mode: t
   global-eldoc-mode: t
   eldoc-mode: t
   show-paren-mode: t
   electric-indent-mode: t
   mouse-wheel-mode: t
   tool-bar-mode: t
   menu-bar-mode: t
   file-name-shadow-mode: t
   global-font-lock-mode: t
   font-lock-mode: t
   blink-cursor-mode: t
   line-number-mode: t
   indent-tabs-mode: t
   transient-mark-mode: t
   auto-composition-mode: t
   auto-encryption-mode: t
   auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date subr-x mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
cl-loaddefs cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils rmc iso-transl tooltip cconv eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/ns-win ns-win ucs-normalize mule-util term/common-win tool-bar dnd
fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow
isearch easymenu timer select scroll-bar mouse jit-lock font-lock syntax
font-core term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads kqueue cocoa ns multi-tty
make-network-process emacs)

Memory information:
((conses 16 37423 7895)
  (symbols 48 5048 0)
  (strings 32 12600 1784)
  (string-bytes 1 359254)
  (vectors 16 10402)
  (vector-slots 8 156348 13684)
  (floats 8 37 17)
  (intervals 56 218 0)
  (buffers 984 10))

bug#73584: 29.3; read-key

[Eli Zaretskii]

> Date: Tue, 01 Oct 2024 13:21:11 -0500
> From: Devon Sean McCullough <Emacs-hacker2023@jovi.net>
> 
> (read-key 0) jails Emacs in a null keymap.

You should be able to escape the jail with ESC or C-[.

I don't see a bug here.

bug#73584: 29.3; read-key

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Tue, 01 Oct 2024 13:21:11 -0500
>> From: Devon Sean McCullough <Emacs-hacker2023@jovi.net>
>>
>> (read-key 0) jails Emacs in a null keymap.
>
> You should be able to escape the jail with ESC or C-[.
>
> I don't see a bug here.

It would arguably be a bit nicer to signal the error early to avoid
having to mash C-[:

diff --git a/lisp/subr.el b/lisp/subr.el
index 2eaed682406..b3872513005 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3307,6 +3307,8 @@ read-key
 what you want as `read-key' temporarily removes all bindings
 while calling `read-key-sequence'.  If nil or unspecified, the
 only unbound fallback disabled is downcasing of the last event."
+  (or (stringp prompt)
+      (signal 'wrong-type-argument (list 'stringp prompt)))
   ;; This overriding-terminal-local-map binding also happens to
   ;; disable quail's input methods, so although read-key-sequence
   ;; always inherits the input method, in practice read-key does not

BTW, do we have something like `cl-check-type' outside of cl-lib?

bug#73584: 29.3; read-key

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Wed, 2 Oct 2024 10:02:44 +0000
> Cc: 73584@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Date: Tue, 01 Oct 2024 13:21:11 -0500
> >> From: Devon Sean McCullough <Emacs-hacker2023@jovi.net>
> >>
> >> (read-key 0) jails Emacs in a null keymap.
> >
> > You should be able to escape the jail with ESC or C-[.
> >
> > I don't see a bug here.
> 
> It would arguably be a bit nicer to signal the error early to avoid
> having to mash C-[:

Maybe.  But what exactly is wrong with signaling the error from
read-key-sequence-vector, which read-key calls?

> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -3307,6 +3307,8 @@ read-key
>  what you want as `read-key' temporarily removes all bindings
>  while calling `read-key-sequence'.  If nil or unspecified, the
>  only unbound fallback disabled is downcasing of the last event."
> +  (or (stringp prompt)
> +      (signal 'wrong-type-argument (list 'stringp prompt)))

This will signal an error if PROMPT is nil or omitted, which we
definitely must support.

>    ;; This overriding-terminal-local-map binding also happens to
>    ;; disable quail's input methods, so although read-key-sequence
>    ;; always inherits the input method, in practice read-key does not
> 
> BTW, do we have something like `cl-check-type' outside of cl-lib?

We shouldn't use any cl stuff in subr.el, surely?

bug#73584: 29.3; read-key

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Kangas <stefankangas@gmail.com>
>> Date: Wed, 2 Oct 2024 10:02:44 +0000
>> Cc: 73584@debbugs.gnu.org
>>
>> It would arguably be a bit nicer to signal the error early to avoid
>> having to mash C-[:
>
> Maybe.  But what exactly is wrong with signaling the error from
> read-key-sequence-vector, which read-key calls?

I just observe that I have to mash C-[ to get back control of Emacs, and
that it would probably have been better if I didn't have to.  I imagine
situations when this happens because of a bug in some library, and users
won't know what to do in that situation.  It clearly confused OP.

>> --- a/lisp/subr.el
>> +++ b/lisp/subr.el
>> @@ -3307,6 +3307,8 @@ read-key
>>  what you want as `read-key' temporarily removes all bindings
>>  while calling `read-key-sequence'.  If nil or unspecified, the
>>  only unbound fallback disabled is downcasing of the last event."
>> +  (or (stringp prompt)
>> +      (signal 'wrong-type-argument (list 'stringp prompt)))
>
> This will signal an error if PROMPT is nil or omitted, which we
> definitely must support.

Thanks, yes.  That would have to be amended.

>>    ;; This overriding-terminal-local-map binding also happens to
>>    ;; disable quail's input methods, so although read-key-sequence
>>    ;; always inherits the input method, in practice read-key does not
>>
>> BTW, do we have something like `cl-check-type' outside of cl-lib?
>
> We shouldn't use any cl stuff in subr.el, surely?

I meant to ask if we have something similar to `cl-check-type' that does
not come from cl-lib.el.  I'm asking that precisely because we can't use
cl-lib in subr.el.

It would have been convenient if we had something like:

    (defmacro check-type (form predicate)
       `(unless (,predicate form)
           (signal 'wrong-type-argument (list ,predicate form))))

We have this pattern in quite a few places in our code, so maybe we
should consider adding it if we don't have it already.

bug#73584: 29.3; read-key

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Wed, 2 Oct 2024 21:47:19 +0000
> Cc: Emacs-hacker2023@jovi.net, 73584@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Stefan Kangas <stefankangas@gmail.com>
> >> Date: Wed, 2 Oct 2024 10:02:44 +0000
> >> Cc: 73584@debbugs.gnu.org
> >>
> >> It would arguably be a bit nicer to signal the error early to avoid
> >> having to mash C-[:
> >
> > Maybe.  But what exactly is wrong with signaling the error from
> > read-key-sequence-vector, which read-key calls?
> 
> I just observe that I have to mash C-[ to get back control of Emacs, and
> that it would probably have been better if I didn't have to.  I imagine
> situations when this happens because of a bug in some library, and users
> won't know what to do in that situation.  It clearly confused OP.

The OP shot himself in the foot by invoking by hand a low-level
function with a wrong argument.  Interactive functions and high-level
APIs must check their arguments right away, but low-level functions
like this one do not.

What I could understand is if we'd arranged for the temporary keymap
to stop being in effect even inside the debugger.  Because the
scenario of this bug reveals some more general issue: when we signal
an error from a function that sets up some limited keymap, the
debugger is invoked with that keymap still in effect, or so it seems.

Stefan Monnier, do we have a good way of doing that in cases like
this?  Perhaps the debugger should define some minimal key bindings
when it starts or something?

> I meant to ask if we have something similar to `cl-check-type' that does
> not come from cl-lib.el.  I'm asking that precisely because we can't use
> cl-lib in subr.el.

Sorry, my misunderstanding.

> It would have been convenient if we had something like:
> 
>     (defmacro check-type (form predicate)
>        `(unless (,predicate form)
>            (signal 'wrong-type-argument (list ,predicate form))))
> 
> We have this pattern in quite a few places in our code, so maybe we
> should consider adding it if we don't have it already.

If we want something like this, I think we should support a list of
allowed types, not just one type.

bug#73584: 29.3; read-key

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

> The OP shot himself in the foot by invoking by hand a low-level
> function with a wrong argument.  Interactive functions and high-level
> APIs must check their arguments right away, but low-level functions
> like this one do not.

I think the "bug" is indeed not in the fact that we check too late for
a string.  I guess we could say that any code which temporarily rebinds
the global map should be super-extra careful never to signal an error,
and prevent itself from being Edebugged etc... but it would be
preferable for the debuggers to "handle it".

> Stefan Monnier, do we have a good way of doing that in cases like
> this?  Perhaps the debugger should define some minimal key bindings
> when it starts or something?

The debuggers already do a fair bit of "setup" to try and isolate the
state of the debugger from the state of the debugged code.

If we got rid of `current-global-map` and `use-global-map` (and instead
just tied the ELisp `global-map` to C's `current_global_map`, so we
could (re) define those two functions in ELisp), then `read-key` could
let-bind `global-map` and the debuggers could also let-bind it (back) to
the value in `default-toplevel-value`.

Short of doing that we could have a new var `normal-global-map` which
would be initialized to the value of `global-map` and then make the
debuggers install that map with `set-global-map` while they're active.

More generally, the debuggers should (behave as if they're) run
in their own thread so they're not affected by the dynamic-bindings of
the code they're debugging.

>> I meant to ask if we have something similar to `cl-check-type' that does
>> not come from cl-lib.el.  I'm asking that precisely because we can't use
>> cl-lib in subr.el.
> Sorry, my misunderstanding.

A think a better course of action for this specific sub-problem is to
arrange for `read-keys` to be able to use `cl-check-type` (instead of
re-inventing it).

In general, I think it would be beneficial to split `subr.el` into two
files: one that is still loaded just as early and another that's loaded
a bit later so it can use `cl-lib` macros.
In the mean time, we can often get a similar result by moving the code
to `simple.el`.

There can be other ways to solve this bootstrap problem.  Such as
arranging for `subr.el` to load `cl-lib` in a slightly more careful way
than `eval-when-compile`: it would load it when the file is being
compiled (just like `eval-when-compile`) or it would schedule `cl-lib`
for "near future" loading (as soon as enough other files have been
loaded that `cl-lib` can be loaded).  As long as `cl-lib` is loaded
before `read-keys` is called, that would be sufficient.


        Stefan

bug#73584: 29.3; read-key

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Stefan Kangas <stefankangas@gmail.com>,  Emacs-hacker2023@jovi.net,
>   73584@debbugs.gnu.org
> Date: Thu, 03 Oct 2024 11:43:25 -0400
> 
> > The OP shot himself in the foot by invoking by hand a low-level
> > function with a wrong argument.  Interactive functions and high-level
> > APIs must check their arguments right away, but low-level functions
> > like this one do not.
> 
> I think the "bug" is indeed not in the fact that we check too late for
> a string.  I guess we could say that any code which temporarily rebinds
> the global map should be super-extra careful never to signal an error,
> and prevent itself from being Edebugged etc... but it would be
> preferable for the debuggers to "handle it".

Is the patch below acceptable as the solution to this?

diff --git a/lisp/emacs-lisp/backtrace.el b/lisp/emacs-lisp/backtrace.el
index 120972d..e353970 100644
--- a/lisp/emacs-lisp/backtrace.el
+++ b/lisp/emacs-lisp/backtrace.el
@@ -202,6 +202,7 @@ backtrace-mode-map
   "+"   #'backtrace-multi-line
   "-"   #'backtrace-single-line
   "."   #'backtrace-expand-ellipses
+  "C-]"    #'abort-recursive-edit
   "<follow-link>" 'mouse-face
   "<mouse-2>"     #'mouse-select-window