bug#54802: OClosure: Make `interactive-form` a generic function

[Eli Zaretskii]

> Date: Tue, 19 Apr 2022 10:53:06 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Here is an alternative patch which does not make `interactive-form`
> a generic function, but instead does what we discussed with Po,
> i.e. introduce a new generic function to which `interactive-form`
> delegates the work when it encounters an OClosure.
> 
> This way, we avoid slowdowns both for `commandp` and for
> `interactive-form` and it minimizes the changes to `interactive-form`.

Thanks.  A few minor comments below:

> diff --git a/doc/lispref/commands.texi b/doc/lispref/commands.texi
> index ace0c025512..16712fd7cb7 100644
> --- a/doc/lispref/commands.texi
> +++ b/doc/lispref/commands.texi
> @@ -312,6 +312,9 @@ Using Interactive
>  specifies how to compute its arguments.  Otherwise, the value is
>  @code{nil}.  If @var{function} is a symbol, its function definition is
>  used.
> +When called on an OClosure, the work is delegated to the generic
> +function @code{oclosure-interactive-form}, where additional methods
> +can be used for specific OClosure types, e.g. for advice and keyboard macros.
>  @end defun

I think oclosure-interactive-form should be documented in more detail,
since we will probably see it used more and more in the future.  E.g.,
we should say something about all those "additional methods" that are
only hinted above.

> diff --git a/lisp/kmacro.el b/lisp/kmacro.el
> index 8a9d89929eb..5476c2395ca 100644
> --- a/lisp/kmacro.el
> +++ b/lisp/kmacro.el
> @@ -820,13 +820,14 @@ kmacro
>                             (counter (or counter 0))
>                             (format (or format "%d")))
>        (&optional arg)
> -    (interactive "p")
>      ;; Use counter and format specific to the macro on the ring!
>      (let ((kmacro-counter counter)
>         (kmacro-counter-format-start format))
>        (execute-kbd-macro keys arg #'kmacro-loop-setup-function)
>        (setq counter kmacro-counter))))
>  
> +(cl-defmethod oclosure-interactive-form ((_ kmacro)) '(interactive "p"))

So suppose we'd like later to modify the interactive form of kmacro to
use Lisp code instead of just the "p" thing -- how should we go about
that?  Does oclosure-interactive-form accept everything that
'interactive' accepts?  Does it use the same syntax, or will we need
to use some special quoting there?

I also wonder whether this will make commands harder to spot just by
looking at their code than it is now.

> +      else if (PVSIZE (fun) > COMPILED_DOC_STRING)
> +        {
> +          Lisp_Object doc = AREF (fun, COMPILED_DOC_STRING);
> +          if (!(NILP (doc) || VALID_DOCSTRING_P (doc)))
> +            genfun = true;
> +        }

There should be a comment there explaining the significance of
comparison with COMPILED_DOC_STRING and why this turns on the genfun
flag.

> +  bool genfun = false; /* If true, we should consult `interactive-form`.  */

Please don't use Markdown-style quoting in code comments.

>    /* Bytecode objects are interactive if they are long enough to
>       have an element whose index is COMPILED_INTERACTIVE, which is
>       where the interactive spec is stored.  */
>    else if (COMPILEDP (fun))
> -    return (PVSIZE (fun) > COMPILED_INTERACTIVE ? Qt : if_prop);
> +    {
> +      if (PVSIZE (fun) > COMPILED_INTERACTIVE)
> +        return Qt;
> +      else if (PVSIZE (fun) > COMPILED_DOC_STRING)
> +        {
> +          Lisp_Object doc = AREF (fun, COMPILED_DOC_STRING);
> +          genfun = !(NILP (doc) || VALID_DOCSTRING_P (doc));
> +        }
> +    }

Here, too, the significance of comparison with COMPILED_DOC_STRING
should be explained in a comment.

>    /* Strings and vectors are keyboard macros.  */
> -  if (STRINGP (fun) || VECTORP (fun))
> +  else if (STRINGP (fun) || VECTORP (fun))
>      return (NILP (for_call_interactively) ? Qt : Qnil);
>  
>    /* Lists may represent commands.  */
> -  if (!CONSP (fun))
> +  else if (!CONSP (fun))
>      return Qnil;

I don't understand why you replace 'if' with 'else if' here: are they
just stylistic preferences?  If so, I'd prefer to leave the original
code intact where it doesn't have to be changed.

> +  while (SYMBOLP (fun))
> +    {
> +      Lisp_Object tmp = Fget (fun, Qinteractive_form);
> +      if (!NILP (tmp))
> +     error ("Found an `interactive-form` property!");

Please quote `like this' in error messages, as text-quoting-style
expects that.