bug#35231: 26.1; Redefine `read-command' behavior for empty input and DEFAULT not a command name

bug#35231: 26.1; Redefine `read-command' behavior for empty input and DEFAULT not a command name

[Drew Adams]

This is a followup to bug #35222 (and thanks for fixing that doc bug).

As discussed in #35222, the current C-code definition of `read-command'
seems not so desirable, when it comes to DEFAULT = nil or "".

The behavior in that case is to return an uninterned symbol whose name
is empty, which has the read/print syntax `##'.

This is _not_ a symbol whose function definition is `commandp'.  There
is nothing about this symbol that qualifies for consideration as a
command.  It seems wrong (a bug) for `read-command' to ever return a
symbol whose function definition is `commandp'.

Function `read-command' is old, and it has always had this odd behavior.
One question is whether there is code out there that depends on this odd
null DEFAULT behavior.

Another question is whether this behavior is good.  I don't think so
(see above), but perhaps there is an argument supporting it?

Supposing that there is no sufficient argument supporting it, can we fix
it, adjust any deliverable calling code as needed, and record in NEWS
the change in this corner-case behavior?  If so, what's the right fix?

Here's one suggestion:

1. Define it in Lisp, not C (why should it ever have been in C?).

2. Use `completing-read' with REQUIRE-MATCH=t and PREDICATE=`commandp'.

3. Because even with REQUIRE-MATCH=t you can exit `completing-read'
   with empty input that does not satisfy PREDICATE, loop until the
   input is nonempty and satisfies PREDICATE.

Something like this, for example:

(defun read-command (prompt &optional default)
  "Read the name of a command and return a symbol with that name.
\(A command is anything that satisfies predicate `commandp'.)
Prompt with PROMPT.
By default, return the command named DEFAULT (or its first element
if DEFAULT is a list).  (If DEFAULT does not name a command then it
is ignored.)"
  (let ((name  (completing-read prompt obarray #'commandp t nil
                                'extended-command-history default)))
    (while (string= "" name)
      (setq name  (completing-read prompt obarray #'commandp t nil
                                   'extended-command-history default)))
    (intern name)))

In GNU Emacs 26.1 (build 1, x86_64-w64-mingw32)
 of 2018-05-30
Repository revision: 07f8f9bc5a51f5aa94eb099f3e15fbe0c20ea1ea
Windowing system distributor `Microsoft Corp.', version 10.0.17134
Configured using:
 `configure --without-dbus --host=x86_64-w64-mingw32
 --without-compress-install 'CFLAGS=-O2 -static -g3''

bug#35231: 26.1; Redefine `read-command' behavior for empty input and DEFAULT not a command name

[Lars Ingebrigtsen]

Drew Adams <drew.adams@oracle.com> writes:

> This is a followup to bug #35222 (and thanks for fixing that doc bug).
>
> As discussed in #35222, the current C-code definition of `read-command'
> seems not so desirable, when it comes to DEFAULT = nil or "".
>
> The behavior in that case is to return an uninterned symbol whose name
> is empty, which has the read/print syntax `##'.
>
> This is _not_ a symbol whose function definition is `commandp'.  There
> is nothing about this symbol that qualifies for consideration as a
> command.  It seems wrong (a bug) for `read-command' to ever return a
> symbol whose function definition is `commandp'.

That is, indeed, pretty odd.  The function isn't used a lot in Emacs:

./lisp/subr.el1088:           (read-command (format "Set key %s to command: "
./lisp/edmacro.el118:	     (setq cmd (read-command "Name of keyboard macro to edit: "))
./lisp/emulation/edt.el1130:	(setq edt-function (read-command "Enter command name: "))
./lisp/info.el4526:	    (read-command "Find documentation for command: ")))
./lisp/strokes.el448:    (read-command "Command to map stroke to: ")))

But some of them expect this behaviour:

	     (setq cmd (read-command "Name of keyboard macro to edit: "))
	     (if (string-equal cmd "")
		 (error "No command name given"))

	(setq edt-function (read-command "Enter command name: "))
	(if (string-equal "" edt-function)
	    (message "Key not defined")

So changing the behaviour is probably not a good idea -- there may be
out-of-tree usages.  So I think we'll have to live with it, and I'm
closing this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#35231: 26.1; Redefine `read-command' behavior for empty input and DEFAULT not a command name

[Drew Adams]

> > This is a followup to bug #35222 (and thanks for fixing that doc bug).
> >
> > As discussed in #35222, the current C-code definition of `read-command'
> > seems not so desirable, when it comes to DEFAULT = nil or "".
> >
> > The behavior in that case is to return an uninterned symbol whose name
> > is empty, which has the read/print syntax `##'.
> >
> > This is _not_ a symbol whose function definition is `commandp'.  There
> > is nothing about this symbol that qualifies for consideration as a
> > command.  It seems wrong (a bug) for `read-command' to ever return a
> > symbol whose function definition is `commandp'.
> 
> That is, indeed, pretty odd.  The function isn't used a lot in Emacs:
> 
> ./lisp/subr.el1088:           (read-command (format "Set key %s to command:
> "
> ./lisp/edmacro.el118:	     (setq cmd (read-command "Name of keyboard
> macro to edit: "))
> ./lisp/emulation/edt.el1130:	(setq edt-function (read-command "Enter
> command name: "))
> ./lisp/info.el4526:	    (read-command "Find documentation for command:
> ")))
> ./lisp/strokes.el448:    (read-command "Command to map stroke to: ")))
> 
> But some of them expect this behaviour:
> 
> 	     (setq cmd (read-command "Name of keyboard macro to edit: "))
> 	     (if (string-equal cmd "")
> 		 (error "No command name given"))
> 
> 	(setq edt-function (read-command "Enter command name: "))
> 	(if (string-equal "" edt-function)
> 	    (message "Key not defined")
> 
> So changing the behaviour is probably not a good idea -- there may be
> out-of-tree usages.  So I think we'll have to live with it, and I'm
> closing this bug report.

Just for the record:

I don't think this is the best way to handle this.
It seems clear from those rare examples you show
that they do what they do only to _work around_
the odd (bugged) behavior.  It would be easy to
fix them.  Likewise, any 3rd-party code.

And in fact, with a fixed `read-command' no change
would really be needed to any such caller.  The
test for equality with "" would simply never
succeed.  The test would be superfluous, but if
it were not removed there would be no loss.

I made good suggestions for how this function can
be better defined, and how to handle the (minor)
incompatible change (fix) for returning a non
command name.

The function can be trivially defined in Lisp, in
such a way that users can benefit from completion
and no caller will need to test for a non-command
name (whether odd, such as "", or not).

That's the way to improve Emacs, here.  Closing
the bug because there may be callers that "depend"
on the bugged behavior in the way you indicated
(i.e., as a bug workaround, for the odd possibility
that a non-command name would be read) doesn't
make sense to me.

This bug is simple to fix properly, and I see no
downside to that being done.

It's also possible (but it shouldn't be necessary)
to define a new function, with a different name,
and use that.  And declare the old one, with the
bug, defined in C, as obsolete.

FWIW, I use this (in Dired+):

(defun diredp-read-command (&optional prompt default)
  "Read the name of a command and return a symbol with that name.
\(A command is anything that satisfies predicate `commandp'.)
Prompt with PROMPT, which defaults to \"Command: \".
By default, return the command named DEFAULT (or, with Emacs 23+, its
first element if DEFAULT is a list).  (If DEFAULT does not name a
command then it is ignored.)"
  (setq prompt  (or prompt  "Command: "))
  (let ((name  (completing-read prompt obarray #'commandp t nil
                                'extended-command-history default)))
    (while (string= "" name)
      (setq name  (completing-read prompt obarray #'commandp t nil
                                   'extended-command-history default)))
    (intern name)))

Any good reason why such a definition (or
similar) should not be used as a replacement
for the current, bugged, `read-command'?