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

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

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

Package: Emacs
Version: 29.0.50


`nadvice.el` needs to build commands whose interactive spec is computed.
This currently can't be done with `lambda` (see also bug#51695 for
a related problem) but `nadvice.el` is unaffected because it assembles
its byte-code functions all by hand.  In order for `nadvice.el` to be
able to use OClosures, we need to address this limitation.

The patch below does it by making `interactive-form` a generic function,
so OClosures can compute their interactive specs from their slots.

Maybe it should be `call-interactively` that's turned into a generic
function (which would also open up the possibility to do more than just
compute the args to pass to the function, such as also printing the
return value or things like that), but that would be a more significant
change.

While the performance of `call-interactively` and `interactive-form` are
not critical, `commandp` is a function that is occasionally used in
tight loops (typically when filtering completions from `obarray`) so
I refrained from making it into a generic function, and instead I make
it defer to `interactive-form` when we counter what looks like an OClosure.

That keeps the common code as fast as before, tho it makes `commandp`
slow(ish) when applied to interactive OClosures.

Making `commandp` into a generic function would apparently slow down
a loop like

    (mapatoms (lambda (s) (if (commandp s) (cl-incf count))))

by a factor around 2x or 3x, which is not the end of the world but
doesn't seem justified.

The patch below also includes a use of this new generic function by
moving the interactive spec of kmacros from the kmacro objects
themselves to the generic function.  The gain is that each `kmacro` is
now 1 word smaller (negligible, in the grand scheme of things, but
I included it for illustration and testing purposes).

Any commment?  Objection?


        Stefan


diff --git a/doc/lispref/commands.texi b/doc/lispref/commands.texi
index 74bf0f48692..eb29fd0044c 100644
--- a/doc/lispref/commands.texi
+++ b/doc/lispref/commands.texi
@@ -312,6 +312,8 @@ 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.
+This is a generic function, so additional methods can be used
+for specific function types, e.g. for advice and keyboard macros.
 @end defun
 
 @node Interactive Codes
diff --git a/etc/NEWS b/etc/NEWS
index 1043873f2d7..3728f9a9231 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1341,6 +1341,11 @@ Can dynamically generate a raw docstring depending on the type of
 a function.
 Used mainly for docstrings of OClosures.
 
++++
+** 'interactive-form' is now a generic function.
+This allows specific OClosure types to compute their interactive specs
+on demand rather than precompute them when created.
+
 +++
 ** Base64 encoding no longer tolerates latin-1 input.
 The functions 'base64-encode-string', 'base64url-encode-string',
diff --git a/lisp/kmacro.el b/lisp/kmacro.el
index 8a9d89929eb..9aaf90e0f5a 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 interactive-form ((_ kmacro) &optional _) '(interactive "p"))
+
 ;;;###autoload
 (defun kmacro-lambda-form (mac &optional counter format)
   ;; Apparently, there are two different ways this is called:
diff --git a/lisp/simple.el b/lisp/simple.el
index eb657018039..f14da3d6d74 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2389,6 +2389,39 @@ function-documentation
 (cl-defmethod function-documentation ((function accessor))
   (oclosure--accessor-docstring function)) ;; FIXME: η-reduce!
 
+(cl-defgeneric interactive-form (cmd &optional original-name)
+  "Return the interactive form of CMD or nil if none.
+If CMD is not a command, the return value is nil.
+Value, if non-nil, is a list (interactive SPEC).
+ORIGINAL-NAME is used internally only."
+  (pcase cmd
+    ((pred symbolp)
+     (let ((fun (indirect-function cmd)))  ;Check cycles.
+       (when fun
+          (or (get cmd 'interactive-form)
+              (interactive-form (symbol-function cmd)
+                                (or original-name cmd))))))
+    ((pred byte-code-function-p)
+     (when (> (length cmd) 5)
+       (let ((form (aref cmd 5)))
+         (list 'interactive
+	       (if (vectorp form)
+	           ;; The vector form is the new form, where the first
+	           ;; element is the interactive spec, and the second
+	           ;; is the "command modes" info.
+	           (aref form 0)
+	         form)))))
+    ((pred autoloadp)
+     (interactive-form (autoload-do-load cmd original-name)))
+    ((or `(lambda ,_args . ,body)
+         `(closure ,_env ,_args . ,body))
+     (let ((spec (assq 'interactive body)))
+       (if (cddr spec)
+           ;; Drop the "command modes" info.
+           (list 'interactive (cadr spec))
+         spec)))
+    (_ (internal--interactive-form cmd))))
+
 (defun command-execute (cmd &optional record-flag keys special)
   ;; BEWARE: Called directly from the C code.
   "Execute CMD as an editor command.
diff --git a/src/callint.c b/src/callint.c
index 31919d6bb81..92bfaf8d397 100644
--- a/src/callint.c
+++ b/src/callint.c
@@ -315,7 +315,7 @@ DEFUN ("call-interactively", Fcall_interactively, Scall_interactively, 1, 3, 0,
   Lisp_Object up_event = Qnil;
 
   /* Set SPECS to the interactive form, or barf if not interactive.  */
-  Lisp_Object form = Finteractive_form (function);
+  Lisp_Object form = call1 (Qinteractive_form, function);
   if (! CONSP (form))
     wrong_type_argument (Qcommandp, function);
   Lisp_Object specs = Fcar (XCDR (form));
diff --git a/src/data.c b/src/data.c
index f06b561dcc6..888e3f66b27 100644
--- a/src/data.c
+++ b/src/data.c
@@ -1065,29 +1065,12 @@ DEFUN ("native-comp-unit-set-file", Fnative_comp_unit_set_file,
 
 #endif
 
-DEFUN ("interactive-form", Finteractive_form, Sinteractive_form, 1, 1, 0,
-       doc: /* Return the interactive form of CMD or nil if none.
+DEFUN ("internal--interactive-form", Finternal__interactive_form, Sinternal__interactive_form, 1, 1, 0,
+       doc: /* Return the interactive form of FUN or nil if none.
 If CMD is not a command, the return value is nil.
 Value, if non-nil, is a list (interactive SPEC).  */)
-  (Lisp_Object cmd)
+  (Lisp_Object fun)
 {
-  Lisp_Object fun = indirect_function (cmd); /* Check cycles.  */
-
-  if (NILP (fun))
-    return Qnil;
-
-  /* Use an `interactive-form' property if present, analogous to the
-     function-documentation property.  */
-  fun = cmd;
-  while (SYMBOLP (fun))
-    {
-      Lisp_Object tmp = Fget (fun, Qinteractive_form);
-      if (!NILP (tmp))
-	return tmp;
-      else
-	fun = Fsymbol_function (fun);
-    }
-
   if (SUBRP (fun))
     {
       if (SUBR_NATIVE_COMPILEDP (fun) && !NILP (XSUBR (fun)->native_intspec))
@@ -1099,21 +1082,6 @@ DEFUN ("interactive-form", Finteractive_form, Sinteractive_form, 1, 1, 0,
 		      (*spec != '(') ? build_string (spec) :
 		      Fcar (Fread_from_string (build_string (spec), Qnil, Qnil)));
     }
-  else if (COMPILEDP (fun))
-    {
-      if (PVSIZE (fun) > COMPILED_INTERACTIVE)
-	{
-	  Lisp_Object form = AREF (fun, COMPILED_INTERACTIVE);
-	  if (VECTORP (form))
-	    /* The vector form is the new form, where the first
-	       element is the interactive spec, and the second is the
-	       command modes. */
-	    return list2 (Qinteractive, AREF (form, 0));
-	  else
-	    /* Old form -- just the interactive spec. */
-	    return list2 (Qinteractive, form);
-	}
-    }
 #ifdef HAVE_MODULES
   else if (MODULE_FUNCTIONP (fun))
     {
@@ -1123,24 +1091,6 @@ DEFUN ("interactive-form", Finteractive_form, Sinteractive_form, 1, 1, 0,
         return form;
     }
 #endif
-  else if (AUTOLOADP (fun))
-    return Finteractive_form (Fautoload_do_load (fun, cmd, Qnil));
-  else if (CONSP (fun))
-    {
-      Lisp_Object funcar = XCAR (fun);
-      if (EQ (funcar, Qclosure)
-	  || EQ (funcar, Qlambda))
-	{
-	  Lisp_Object form = Fcdr (XCDR (fun));
-	  if (EQ (funcar, Qclosure))
-	    form = Fcdr (form);
-	  Lisp_Object spec = Fassq (Qinteractive, form);
-	  if (NILP (Fcdr (Fcdr (spec))))
-	    return spec;
-	  else
-	    return list2 (Qinteractive, Fcar (Fcdr (spec)));
-	}
-    }
   return Qnil;
 }
 
@@ -4255,7 +4205,7 @@ #define PUT_ERROR(sym, tail, msg)			\
   DEFSYM (Qbyte_code_function_p, "byte-code-function-p");
 
   defsubr (&Sindirect_variable);
-  defsubr (&Sinteractive_form);
+  defsubr (&Sinternal__interactive_form);
   defsubr (&Scommand_modes);
   defsubr (&Seq);
   defsubr (&Snull);
diff --git a/src/eval.c b/src/eval.c
index a1cebcd0257..35a9f70d7b5 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -2032,8 +2032,7 @@ DEFUN ("commandp", Fcommandp, Scommandp, 1, 2, 0,
   (Lisp_Object function, Lisp_Object for_call_interactively)
 {
   register Lisp_Object fun;
-  register Lisp_Object funcar;
-  Lisp_Object if_prop = Qnil;
+  bool genfun = false; /* If true, we should consult `interactive-form`.  */
 
   fun = function;
 
@@ -2041,52 +2040,92 @@ DEFUN ("commandp", Fcommandp, Scommandp, 1, 2, 0,
   if (NILP (fun))
     return Qnil;
 
-  /* Check an `interactive-form' property if present, analogous to the
-     function-documentation property.  */
-  fun = function;
-  while (SYMBOLP (fun))
-    {
-      Lisp_Object tmp = Fget (fun, Qinteractive_form);
-      if (!NILP (tmp))
-	if_prop = Qt;
-      fun = Fsymbol_function (fun);
-    }
-
   /* Emacs primitives are interactive if their DEFUN specifies an
      interactive spec.  */
   if (SUBRP (fun))
-    return XSUBR (fun)->intspec ? Qt : if_prop;
-
+    {
+      if (XSUBR (fun)->intspec)
+        return Qt;
+    }
   /* 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)
+        genfun = true;
+    }
 
 #ifdef HAVE_MODULES
   /* Module functions are interactive if their `interactive_form'
      field is non-nil. */
   else if (MODULE_FUNCTIONP (fun))
-    return NILP (module_function_interactive_form (XMODULE_FUNCTION (fun)))
-             ? if_prop
-             : Qt;
+    {
+      if (!NILP (module_function_interactive_form (XMODULE_FUNCTION (fun))))
+        return Qt;
+    }
 #endif
 
   /* 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;
-  funcar = XCAR (fun);
-  if (EQ (funcar, Qclosure))
-    return (!NILP (Fassq (Qinteractive, Fcdr (Fcdr (XCDR (fun)))))
-	    ? Qt : if_prop);
-  else if (EQ (funcar, Qlambda))
-    return !NILP (Fassq (Qinteractive, Fcdr (XCDR (fun)))) ? Qt : if_prop;
-  else if (EQ (funcar, Qautoload))
-    return !NILP (Fcar (Fcdr (Fcdr (XCDR (fun))))) ? Qt : if_prop;
+  else
+    {
+      Lisp_Object funcar = XCAR (fun);
+      if (EQ (funcar, Qautoload))
+        {
+          if (!NILP (Fcar (Fcdr (Fcdr (XCDR (fun))))))
+            return Qt;
+        }
+      else
+        {
+          Lisp_Object body = CDR_SAFE (XCDR (fun));
+          if (EQ (funcar, Qclosure))
+            body = CDR_SAFE (body);
+          else if (!EQ (funcar, Qlambda))
+	    return Qnil;
+	  if (!NILP (Fassq (Qinteractive, body)))
+	    return Qt;
+	  else
+	    {
+	      Lisp_Object first = CAR_SAFE (body);
+	      if (!NILP (CDR_SAFE (body))
+	          && (STRINGP (first) || FIXNUMP (first) ||
+	              FIXNUMP (CDR_SAFE (first))))
+	        genfun = true;
+	    }
+	}
+    }
+
+  /* By now, if it's not a function we already returned nil.  */
+
+  /* Check an `interactive-form' property if present, analogous to the
+     function-documentation property.  */
+  fun = function;
+  while (SYMBOLP (fun))
+    {
+      Lisp_Object tmp = Fget (fun, Qinteractive_form);
+      if (!NILP (tmp))
+	error ("Found an `interactive-form` property!");
+      fun = Fsymbol_function (fun);
+    }
+
+  /* If there's no immediate interactive form but there's a docstring,
+     then delegate to the generic-function in case it's an FCR with
+     a type-specific interactive-form.  */
+  if (genfun
+      /* Avoid burping during bootstrap.  */
+      && !NILP (Fsymbol_function (Qinteractive_form)))
+    {
+      Lisp_Object iform = call1 (Qinteractive_form, fun);
+      return NILP (iform) ? Qnil : Qt;
+    }
   else
     return Qnil;
 }

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

[Eli Zaretskii]

> Date: Fri, 08 Apr 2022 16:33:51 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> `nadvice.el` needs to build commands whose interactive spec is computed.
> This currently can't be done with `lambda` (see also bug#51695 for
> a related problem) but `nadvice.el` is unaffected because it assembles
> its byte-code functions all by hand.  In order for `nadvice.el` to be
> able to use OClosures, we need to address this limitation.
> 
> The patch below does it by making `interactive-form` a generic function,
> so OClosures can compute their interactive specs from their slots.
> 
> Maybe it should be `call-interactively` that's turned into a generic
> function (which would also open up the possibility to do more than just
> compute the args to pass to the function, such as also printing the
> return value or things like that), but that would be a more significant
> change.
> 
> While the performance of `call-interactively` and `interactive-form` are
> not critical, `commandp` is a function that is occasionally used in
> tight loops (typically when filtering completions from `obarray`) so
> I refrained from making it into a generic function, and instead I make
> it defer to `interactive-form` when we counter what looks like an OClosure.
> 
> That keeps the common code as fast as before, tho it makes `commandp`
> slow(ish) when applied to interactive OClosures.
> 
> Making `commandp` into a generic function would apparently slow down
> a loop like
> 
>     (mapatoms (lambda (s) (if (commandp s) (cl-incf count))))
> 
> by a factor around 2x or 3x, which is not the end of the world but
> doesn't seem justified.
> 
> The patch below also includes a use of this new generic function by
> moving the interactive spec of kmacros from the kmacro objects
> themselves to the generic function.  The gain is that each `kmacro` is
> now 1 word smaller (negligible, in the grand scheme of things, but
> I included it for illustration and testing purposes).
> 
> Any commment?  Objection?

My comment is that when you introduced OClosures, you never said that
the plan was to go over every place in Emacs where they can be used
and reimplement those places based on OClosures.  It sounds like this
is what's happening, and next we will see another round of churn of
the code we old-timers are familiar with to make it utterly unfamiliar
and dependent on stuff that needs to be carefully studied before it
can even be understood?  Including basic internals such as
interactive-form and its ilk?  All that to solve some minor issues
with nadvice, which itself is a minor feature as Emacs features go?
Doesn't that sound excessive?

I'm okay with having OClosures, yet another new feature of Emacs Lisp,
but I don't think I like seeing stuff like interactive-form rewritten
to be generics or having OClosures in general permeate our internals.
And speed is only one (important) aspect of that: I'd also like to
keep those internals easier understandable by people like myself, who
aren't and will never be CS professionals with special interest in
functional languages.  If that means bug#51695 must be solved some
other way, or even remain unsolved, I think I'd prefer that if this is
the price.

I'm sorry to sound like keeping progress from happening, but it lately
becomes more and more hard to do Emacs maintenance due to excessive
use of new features whose tricky and hard-to-debug code makes finding
the reasons for problems harder and harder.  We already have areas
where no one can suggest safe solutions for problems.  Do we really
want those areas to grow and multiply?

IMNSHO, Emacs is not a platform for programming language development,
it is mainly a platform for writing useful applications.  Let's not go
overboard with new Lisp features in a way that makes our main task
harder than it has to be.

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

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

> My comment is that when you introduced OClosures, you never said that
> the plan was to go over every place in Emacs where they can be used
> and reimplement those places based on OClosures.

I don't understand the above because this patch doesn't make use
of OClosures.  Instead it adds a bit of flexibility to OClosures (the
ability to compute their `interactive-form`).

OT1H if I was given a chance to go back in time I probably would
define `lambda` as a macro that expands to a (trivial)
`oclosure-lambda` and then define `function-documentation` as a function
that simply extracts the `documentation` slot of its OClosure argument
(and same for `interactive-form`).

But given where we are I don't think this will happen and I don't think
my patches "go over every place in Emacs where [OClosures] can be used".

Instead, I see OClosures only used at a few places where they are
beneficial.  So far these places are: `kmacro.el`, `nadvice.el`,
and `cl-generic.el`.  The original motivation behind OClosures was
`nadvice.el`.

Currently on `master` we use OClosures in `cl-generic.el` and in
`kmacro.el`.  For `nadvice.el` there is one remaining hurdle which is
the handling of the interactive specs, i.e. the subject of this
bug report.

> All that to solve some minor issues with nadvice, which itself is
> a minor feature as Emacs features go?

That's the core question, yes (tho I think the improvements in
`kmacro.el` and the slight speed up of `cl-generic.el` are a nice side
effect for end users).


        Stefan

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

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> While the performance of `call-interactively` and `interactive-form` are
> not critical, `commandp` is a function that is occasionally used in
> tight loops (typically when filtering completions from `obarray`) so
> I refrained from making it into a generic function, and instead I make
> it defer to `interactive-form` when we counter what looks like an OClosure.

Makes sense to me.

> That keeps the common code as fast as before, tho it makes `commandp`
> slow(ish) when applied to interactive OClosures.
>
> Making `commandp` into a generic function would apparently slow down
> a loop like
>
>     (mapatoms (lambda (s) (if (commandp s) (cl-incf count))))
>
> by a factor around 2x or 3x, which is not the end of the world but
> doesn't seem justified.

Yeah, we loop on commandp in a lot of contexts, so keeping it fast is
good.

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

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

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> `nadvice.el` needs to build commands whose interactive spec is computed.
> This currently can't be done with `lambda` (see also bug#51695 for
> a related problem) but `nadvice.el` is unaffected because it assembles
> its byte-code functions all by hand.  In order for `nadvice.el` to be
> able to use OClosures, we need to address this limitation.
>
> The patch below does it by making `interactive-form` a generic function,
> so OClosures can compute their interactive specs from their slots.
>
> Maybe it should be `call-interactively` that's turned into a generic
> function (which would also open up the possibility to do more than just
> compute the args to pass to the function, such as also printing the
> return value or things like that), but that would be a more significant
> change.
>
> While the performance of `call-interactively` and `interactive-form` are
> not critical, `commandp` is a function that is occasionally used in
> tight loops (typically when filtering completions from `obarray`) so
> I refrained from making it into a generic function, and instead I make
> it defer to `interactive-form` when we counter what looks like an OClosure.
>
> That keeps the common code as fast as before, tho it makes `commandp`
> slow(ish) when applied to interactive OClosures.
>
> Making `commandp` into a generic function would apparently slow down
> a loop like
>
>     (mapatoms (lambda (s) (if (commandp s) (cl-incf count))))
>
> by a factor around 2x or 3x, which is not the end of the world but
> doesn't seem justified.
>
> The patch below also includes a use of this new generic function by
> moving the interactive spec of kmacros from the kmacro objects
> themselves to the generic function.  The gain is that each `kmacro` is
> now 1 word smaller (negligible, in the grand scheme of things, but
> I included it for illustration and testing purposes).
>
> Any commment?  Objection?

Calling `interactive-form' in a loop is also fairly common.  For
example, I wrote some code a while back to list commands which operate
on the region, which involves running this on each interned atom:

  (defun region-command-p (command)
    "Test if COMMAND, a symbol, is a command that accepts a region."
    (and (commandp command)
         (equal (cadr (interactive-form command)) "r")))

I'm sure a 3x slowdown would be noticeable, so why does this have to be
a generic function?  Why can't we have `interactive-form' return some
field of a given OClosure object instead?

Thanks.

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

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

> Calling `interactive-form' in a loop is also fairly common.

Do you have actual examples in some existing package?

> For example, I wrote some code a while back to list commands which
> operate on the region, which involves running this on each interned
> atom:
>
>   (defun region-command-p (command)
>     "Test if COMMAND, a symbol, is a command that accepts a region."
>     (and (commandp command)
>          (equal (cadr (interactive-form command)) "r")))

Interesting.  I just tested this in an `all-completions` loop and my
patch makes it run a factor of 2 slower (it went from ~30ms to ~60ms to
enumerate the 141 commands that matched).

There are some mitigating circumstances, tho:
- The first call to this code takes a lot of time (~6s) because
  it loads a crapload of packages (every package with a registered
  autoloaded command).
- It's very brittle since it won't find those commands that have
  interactive specs like "r\np" or where it's not a string (like
  `kill-region` and many others, actually there are regularly more, e.g.
  to make them obey `use-region-p`).
- That loop signaled an error because of an erroneous autoload in
  `gnus.el` (it's now fixed in `master`), so your code probably did
  not do that.
  [ Amusingly, I also tried the loop after removing the `commandp` call
    (which is arguably unnecessary and could even slow down the loop)
    but this bumped into even more errors because we then try to load
    even the non-interactive functions.  ]

> I'm sure a 3x slowdown would be noticeable, so why does this have to be
> a generic function?

Making a generic function is the "natural" choice in terms of the
semantics of the function (which is implemented as a sequence of tests
to dispatch to some implementation-specific alternative).

I could change `interactive-form` along the same lines as what I've done
with `function-documentation`, i.e. only delegate to a generic function
when it looks like an OClosure.  That would significantly reduce the
performance impact, probably to negligible levels, but semantically the
only difference between `interactive-form` and that new
`generic-interactive-form` is that one is generic and the other isn't,
so it's rather ugly.

I've never seen the kind of tight loop you suggest, which is why I have
the impression that we can afford to just make `interactive-form` into
a generic function, which is a simpler and cleaner API.

> Why can't we have `interactive-form' return some
> field of a given OClosure object instead?

One reason is that for the case of advice, I'd much rather compute the
interactive spec lazily (when the command is called) rather than when
the advice is built.

Another reason is that there is no dedicated "oclosure slot" for an
interactive-spec.  In theory we could use the byte-code object's slot
for that, but making it computable (as needed for bug#51695 and for
advice) would require significant changes to cconv.el and bytecomp.el
(and to make it not too inconvenient to use in `advice.el` it'd
additionally require extending the syntax of `interactive`).

We could add a dedicated "oclosure slot" for the interactive-spec, but
it'd likely be rather ugly, since that would need to be accessed from
the C in `cmds.c` but would require testing the type of the OClosure
first and that would have to be written in ELisp since it depends on how
OClosure types are represented which itself depends on `cl-defstruct`,
etc...

So if we want to go in this direction it'd be simpler and cleaner to
keep the C implementation of `interactive-form` and have it delegate to
a new `generic-interactive-form` when it finds an OClosure.


        Stefan

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

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Do you have actual examples in some existing package?

Just the one I wrote for myself.

> Interesting.  I just tested this in an `all-completions` loop and my
> patch makes it run a factor of 2 slower (it went from ~30ms to ~60ms to
> enumerate the 141 commands that matched).
>
> There are some mitigating circumstances, tho:
> - The first call to this code takes a lot of time (~6s) because
>   it loads a crapload of packages (every package with a registered
>   autoloaded command).

Indeed, that's a problem I know of, but it solves itself after the first
call.

> - It's very brittle since it won't find those commands that have
>   interactive specs like "r\np" or where it's not a string (like
>   `kill-region` and many others, actually there are regularly more, e.g.
>   to make them obey `use-region-p`).

Hmm, I'll try to adapt that code to those commands.

> - That loop signaled an error because of an erroneous autoload in
>   `gnus.el` (it's now fixed in `master`), so your code probably did
>   not do that.

Somehow I never ran into that.

> One reason is that for the case of advice, I'd much rather compute the
> interactive spec lazily (when the command is called) rather than when
> the advice is built.
>
> Another reason is that there is no dedicated "oclosure slot" for an
> interactive-spec.  In theory we could use the byte-code object's slot
> for that, but making it computable (as needed for bug#51695 and for
> advice) would require significant changes to cconv.el and bytecomp.el
> (and to make it not too inconvenient to use in `advice.el` it'd
> additionally require extending the syntax of `interactive`).
>
> We could add a dedicated "oclosure slot" for the interactive-spec, but
> it'd likely be rather ugly, since that would need to be accessed from
> the C in `cmds.c` but would require testing the type of the OClosure
> first and that would have to be written in ELisp since it depends on how
> OClosure types are represented which itself depends on `cl-defstruct`,
> etc...

OClosures are records, right?  There's no other record type that can be
a function with an interactive form, so we could just use `recordp'
instead of calling into Lisp for that.

Alternatively, we could try to speed up generic dispatch, but I don't
know that code and as such am devoid of ideas in that direction.

Thanks.

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

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

>> - It's very brittle since it won't find those commands that have
>>   interactive specs like "r\np" or where it's not a string (like
>>   `kill-region` and many others, actually there are regularly more, e.g.
>>   to make them obey `use-region-p`).
> Hmm, I'll try to adapt that code to those commands.

The "r\np" case is easy to solve: just use (string-match "^r").
But the other case is a lot harder since we're talking about analyzing
arbitrary code, potentially byte-compiled.

>> - That loop signaled an error because of an erroneous autoload in
>>   `gnus.el` (it's now fixed in `master`), so your code probably did
>>   not do that.
> Somehow I never ran into that.

Hmm... odd.  I know the problem doesn't show up if you have
`gnus-score.el` loaded beforehand, but otherwise I wonder how you dodged
that "bullet" (not that it matters, it's fixed now anyway; just curious).

Talking about curious, I wonder what you use that loop for.

>> One reason is that for the case of advice, I'd much rather compute the
>> interactive spec lazily (when the command is called) rather than when
>> the advice is built.
>>
>> Another reason is that there is no dedicated "oclosure slot" for an
>> interactive-spec.  In theory we could use the byte-code object's slot
>> for that, but making it computable (as needed for bug#51695 and for
>> advice) would require significant changes to cconv.el and bytecomp.el
>> (and to make it not too inconvenient to use in `advice.el` it'd
>> additionally require extending the syntax of `interactive`).
>>
>> We could add a dedicated "oclosure slot" for the interactive-spec, but
>> it'd likely be rather ugly, since that would need to be accessed from
>> the C in `cmds.c` but would require testing the type of the OClosure
>> first and that would have to be written in ELisp since it depends on how
>> OClosure types are represented which itself depends on `cl-defstruct`,
>> etc...
> OClosures are records, right?  There's no other record type that can be
> a function with an interactive form, so we could just use `recordp'
> instead of calling into Lisp for that.

They're somewhat like records but they're not `recordp`, they're `functionp`.

> Alternatively, we could try to speed up generic dispatch, but I don't
> know that code and as such am devoid of ideas in that direction.

Part of the 2x slowdown is due to generic dispatch but part is due to
the fact the code is in ELisp rather than in C, so even a "perfectly
fast" generic dispatch wouldn't get us back the factor 2x.
And speeding up generic dispatch is not super easy.


        Stefan

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

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Hmm... odd.  I know the problem doesn't show up if you have
> `gnus-score.el` loaded beforehand, but otherwise I wonder how you dodged
> that "bullet" (not that it matters, it's fixed now anyway; just curious).

Probably because every autoload that could cause problems was already
loaded.

> Talking about curious, I wonder what you use that loop for.

It was originally supposed to demonstrate the feasibility of a
"context-sensitive" menu in Emacs, i.e. one that displays commands
pertinent to the region when it is active, etc.

But in the end I made a completing-read wrapper around it, which does
make it easier to find some odd commands I can't remember.

>> OClosures are records, right?  There's no other record type that can be
>> a function with an interactive form, so we could just use `recordp'
>> instead of calling into Lisp for that.
>
> They're somewhat like records but they're not `recordp`, they're `functionp`.

[...]

> Part of the 2x slowdown is due to generic dispatch but part is due to
> the fact the code is in ELisp rather than in C, so even a "perfectly
> fast" generic dispatch wouldn't get us back the factor 2x.
> And speeding up generic dispatch is not super easy.

Hmm, so what about having a special "OClosure" pseudovector type on the
C level which would otherwise behave like byte-code functions, but
behave specially in `interactive-form'?

Thanks.

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

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

>> Part of the 2x slowdown is due to generic dispatch but part is due to
>> the fact the code is in ELisp rather than in C, so even a "perfectly
>> fast" generic dispatch wouldn't get us back the factor 2x.
>> And speeding up generic dispatch is not super easy.
>
> Hmm, so what about having a special "OClosure" pseudovector type on the
> C level which would otherwise behave like byte-code functions, but
> behave specially in `interactive-form'?

I can do a sort of `oclosurep` from C already.  But that doesn't
guarantee that the OClosure has an interactive form, so in the case we
match `oclosurep` I'd still need to call another function from the ELisp
world (that's the one I called `generic-interactive-form` since it would
most naturally be a generic function) because it'd be inconvenient to
figure out from C if that OClosure has an interactive-form (and where it
is).


        Stefan

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

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I can do a sort of `oclosurep` from C already.  But that doesn't
> guarantee that the OClosure has an interactive form, so in the case we
> match `oclosurep` I'd still need to call another function from the ELisp
> world (that's the one I called `generic-interactive-form` since it would
> most naturally be a generic function)

Well, as long as its possible, I think we should do that.

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

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

Po Lu [2022-04-15 12:27:02] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> I can do a sort of `oclosurep` from C already.  But that doesn't
>> guarantee that the OClosure has an interactive form, so in the case we
>> match `oclosurep` I'd still need to call another function from the ELisp
>> world (that's the one I called `generic-interactive-form` since it would
>> most naturally be a generic function)
> Well, as long as its possible, I think we should do that.

It's definitely possible.  The reason I resist doing that is that the
resulting API is ugly (you basically have two "identical" functions with
the only justification for the difference is that one is implemented in
C and the other is a generic function) and the only existing reason for
this ugliness is this one loop of yours whose behavior is brittle
anyway, whose first execution is 100x times slower (so the remaining 2x
slowdown when the loop is fast can't be that important), and whose
subsequent fast executions could trivially be made faster by caching the
result of the first call.

So, I'm waiting to hear what others have to say about this choice.

Eli and Lars, do you prefer the faster-but-uglier API or the
cleaner-but-slower API for `interactive-form`?


        Stefan

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

[Eli Zaretskii]

> Cc: 54802@debbugs.gnu.org
> Date: Fri, 15 Apr 2022 12:08:04 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Eli and Lars, do you prefer the faster-but-uglier API or the
> cleaner-but-slower API for `interactive-form`?

As I said up-thread, I don't understand why we need to touch
interactive-form at all.

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

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

Eli Zaretskii [2022-04-15 19:14:11] wrote:
> As I said up-thread, I don't understand why we need to touch
> interactive-form at all.

In my experience, OClosures fall into two camps:
- Those, like `advice`, where their interactive form depends on some of
  the data carried by the OClosure.
- Those, like `kmacro`, where all the OClosures of
  that type have the same, constant, interactive form.

For the second group, we can use the current `interactive-form`, with
the only downside that every OClosure of that type will have to carry
its own reference to that same interactive form.

For the first, it's much more problematic: see for example the function
`advice--make-1`.  There we build a new byte-code function with is
a composition of a base function with an advice function.  We currently
do it by hand out of its actual bytecode string, constant vector, ...
The interactive form of the composed function should be a combination of
the interactive forms of the two functions (so that an advice can
advise also the interactive form of a function), which is computed by
`advice--make-interactive-form`.

When working at that level, the interactive form can be computed
dynamically fairly easily, but there are some problems:
- This is messy because we have to dig inside the representation of
  byte code objects.  The use of OClosures would be able to save us
  from that.
- `oclosure.el` can't use the same trick we currently use in
  `advice--make-1` because it lets the byte compiler build the byte-code
  objects for us and the byte-compiler's code doesn't know how to build
  a bytecode object whose interactive-spec is not just an
  immediate constant.
- Doing it like we do in `advice--make-1` computes the interactive form
  too early: if the base function (or the advice function, but that's
  less likely) is an autoloaded function, it will eagerly load the
  file when the advice is applied.  Also if that base function is an
  alias, it will use the interactive form of the current definition of
  the alias and won't be updated if the alias is later redefined.

There are various ways to work around those problems, but the cleanest
fix is to delay the computation of the interactive form to the moment
`interactive-form` is called.


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: luangruo@yahoo.com,  54802@debbugs.gnu.org
> Date: Mon, 18 Apr 2022 18:59:28 -0400
> 
> There are various ways to work around those problems, but the cleanest
> fix is to delay the computation of the interactive form to the moment
> `interactive-form` is called.

I'm sorry, but I think the cleanest fix is too much to pay for a minor
feature such as this one.  Can't you find a less intrusive way of
fixing these issues, one that doesn't affect all of Emacs for the
benefit of one or two packages of minor importance?

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

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

> I'm sorry, but I think the cleanest fix is too much to pay for a minor
> feature such as this one.

I don't see much price to pay here.

Am I the only here who finds that defining `interactive-form` as
an ELisp generic function is, in itself, a good idea (not good enough
to do it without a good reason, but something I put on the side of
"advantages" rather than "defects" when assessing my patch)?

> Can't you find a less intrusive way of fixing these issues, one that
> doesn't affect all of Emacs for the benefit of one or two packages of
> minor importance?

Not sure what you mean by "affect all of Emacs".  It affects
a well-delimited (and small) part of the code.  `interactive-form` is
a rather simple function with a well understood semantics.

Are you worried about introducing bugs, about the performance impact,
or something else?


        Stefan

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

[Lars Ingebrigtsen]

Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> Am I the only here who finds that defining `interactive-form` as
> an ELisp generic function is, in itself, a good idea (not good enough
> to do it without a good reason, but something I put on the side of
> "advantages" rather than "defects" when assessing my patch)?

Sounds like a good idea to me.

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

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: luangruo@yahoo.com,  54802@debbugs.gnu.org
> Date: Tue, 19 Apr 2022 08:38:47 -0400
> 
> > I'm sorry, but I think the cleanest fix is too much to pay for a minor
> > feature such as this one.
> 
> I don't see much price to pay here.

See below.

> Am I the only here who finds that defining `interactive-form` as
> an ELisp generic function is, in itself, a good idea (not good enough
> to do it without a good reason, but something I put on the side of
> "advantages" rather than "defects" when assessing my patch)?

Maybe you aren't the only one, but I don't share that opinion.  And in
this particular case, I don't even consider the reason to be anywhere
near "good enough".

> > Can't you find a less intrusive way of fixing these issues, one that
> > doesn't affect all of Emacs for the benefit of one or two packages of
> > minor importance?
> 
> Not sure what you mean by "affect all of Emacs".  It affects
> a well-delimited (and small) part of the code.

It is called outside of the advice functions.

> Are you worried about introducing bugs, about the performance impact,
> or something else?

All of them.  And again, the reason doesn't seem to justify the risks,
not IMO.

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

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

>> Not sure what you mean by "affect all of Emacs".  It affects
>> a well-delimited (and small) part of the code.
> It is called outside of the advice functions.

The patch under discussion moves a well-understood and simple piece of
code from C to ELisp, and while doing so marks the resulting function as
generic so that it can be overridden on a case-by-case basis
by packages.

The move from C to ELisp should make no difference other than
performance.  As discussed with Po Lu, if needed, we can reduce this
impact at the cost of a less clean API, and I think it would be for the
worse (the result would be a more complex system with two equivalent
functions where the coders need to understand when to use which), but
I could live with that.

The fact of marking it as generic does not directly have any impact at
all (literally: it just adds a `cl--generic` property to the
`interactive-form` symbol but the content of the `symbol-function` is
the same as it would be for a normal function :-).

The act of overriding it for specific cases by added methods will on the
other hand make a difference and can introduce bugs, just like advising
`interactive-form` can introduce bugs.  It'll be the responsability of
those writing those methods.  I don't foresee this to become a problem at
all since it'll be rather uncommon to define such methods and it will
most likely be limited to OClosures (doing it for other values would be
of limited interest).

>> Are you worried about introducing bugs, about the performance impact,
>> or something else?
> All of them.

What makes you think this can introduce bugs?  What kinds of bugs?

Can you point to code where you'd expect `interactive-form` to be
a potential performance problem?

Care to be a bit more specific about the "something else"?


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: luangruo@yahoo.com,  54802@debbugs.gnu.org
> Date: Tue, 19 Apr 2022 09:34:47 -0400
> 
> >> Not sure what you mean by "affect all of Emacs".  It affects
> >> a well-delimited (and small) part of the code.
> > It is called outside of the advice functions.
> 
> The patch under discussion moves a well-understood and simple piece of
> code from C to ELisp, and while doing so marks the resulting function as
> generic so that it can be overridden on a case-by-case basis
> by packages.
> 
> The move from C to ELisp should make no difference other than
> performance.

We've seen this proven wrong quite a few times already.  This isn't
the first time we move some "well-understood and simple" code from C
to Lisp.  And every time we did that it had unintended consequences:
subtle bugs, features that were lost, subtly changed behavior, etc.
How many times we need to experience this before we all understand
that there are no "well-understood and simple" enough code in Emacs
internals that can be reimplemented without consequences?

I could understand an argument about advantages that outweigh these
costs, but denying these costs exist? that I cannot understand, given
our common experience.

> The fact of marking it as generic does not directly have any impact at
> all (literally: it just adds a `cl--generic` property to the
> `interactive-form` symbol but the content of the `symbol-function` is
> the same as it would be for a normal function :-).

And makes debugging harder while at that.  Right?

> >> Are you worried about introducing bugs, about the performance impact,
> >> or something else?
> > All of them.
> 
> What makes you think this can introduce bugs?

Bitter experience.  You were there, so I'm surprised you don't have
the same experience.

> What kinds of bugs?

How should I know?  We will know when we discover them, months or
maybe years from now.  But discover them we will, of that I'm certain.

> Care to be a bit more specific about the "something else"?

See above.

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

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

Stefan Monnier [2022-04-08 16:33:51] wrote:
> The patch below does it by making `interactive-form` a generic function,
> so OClosures can compute their interactive specs from their slots.

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`.


        Stefan


2022-04-19  Stefan Monnier  <monnier@iro.umontreal.ca>

    New generic function `oclosure-interactive-form`.
    It's used by `interactive-form` when it encounters an OClosure.
    This lets one compute the `interactive-form` of OClosures
    dynamically by adding appropriate methods.

    * lisp/simple.el (oclosure-interactive-form): New generic function.

    * src/data.c (Finteractive_form): Delegate to
    `oclosure-interactive-form` if the arg is an OClosure.
    (syms_of_data): New symbol `Qoclosure_interactive_form`.
    * src/eval.c (Fcommandp): Delegate to `interactive-form` if the arg is
    an OClosure.

    * src/lisp.h (VALID_DOCSTRING_P): New function, extracted from
    `store_function_docstring`.
    * src/doc.c (store_function_docstring): Use it.

    * lisp/kmacro.el (kmacro): Don't carry any interactive form.
    (oclosure-interactive-form) <kmacro>: New method, instead.

    * test/lisp/emacs-lisp/oclosure-tests.el (oclosure-interactive-form)
    <oclosure-test>: New method.
    (oclosure-test-interactive-form): New test.



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
 
 @node Interactive Codes
diff --git a/etc/NEWS b/etc/NEWS
index 3e7788277d3..284e6ab50dc 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1292,6 +1292,11 @@ remote host are shown.  Alternatively, the user option
 Allows the creation of "functions with slots" or "function objects"
 via the macros 'oclosure-define' and 'oclosure-lambda'.
 
+*** New generic function 'oclosure-interactive-form'.
+Used by `interactive-form` when called on an OClosure.
+This allows specific OClosure types to compute their interactive specs
+on demand rather than precompute them when created.
+
 ---
 ** New theme 'leuven-dark'.
 This is a dark version of the 'leuven' theme.
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"))
+
 ;;;###autoload
 (defun kmacro-lambda-form (mac &optional counter format)
   ;; Apparently, there are two different ways this is called:
diff --git a/lisp/simple.el b/lisp/simple.el
index 7e964c9d1d5..ead973d45e0 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2389,6 +2389,15 @@ function-documentation
 (cl-defmethod function-documentation ((function accessor))
   (oclosure--accessor-docstring function)) ;; FIXME: η-reduce!
 
+;; This should be in `oclosure.el' but that file is loaded before `cl-generic'.
+(cl-defgeneric oclosure-interactive-form (_function)
+  "Return the interactive form of FUNCTION or nil if none.
+This is called by `interactive-form' when invoked on OClosures.
+Add your methods to this generic function, but always call `interactive-form'
+instead."
+  ;; (interactive-form function)
+  nil)
+
 (defun command-execute (cmd &optional record-flag keys special)
   ;; BEWARE: Called directly from the C code.
   "Execute CMD as an editor command.
diff --git a/src/callint.c b/src/callint.c
index 31919d6bb81..92bfaf8d397 100644
--- a/src/callint.c
+++ b/src/callint.c
@@ -315,7 +315,7 @@ DEFUN ("call-interactively", Fcall_interactively, Scall_interactively, 1, 3, 0,
   Lisp_Object up_event = Qnil;
 
   /* Set SPECS to the interactive form, or barf if not interactive.  */
-  Lisp_Object form = Finteractive_form (function);
+  Lisp_Object form = call1 (Qinteractive_form, function);
   if (! CONSP (form))
     wrong_type_argument (Qcommandp, function);
   Lisp_Object specs = Fcar (XCDR (form));
diff --git a/src/data.c b/src/data.c
index 72af8a6648e..543590dfa31 100644
--- a/src/data.c
+++ b/src/data.c
@@ -1072,6 +1072,7 @@ DEFUN ("interactive-form", Finteractive_form, Sinteractive_form, 1, 1, 0,
   (Lisp_Object cmd)
 {
   Lisp_Object fun = indirect_function (cmd); /* Check cycles.  */
+  bool genfun = false;
 
   if (NILP (fun))
     return Qnil;
@@ -1113,6 +1114,12 @@ DEFUN ("interactive-form", Finteractive_form, Sinteractive_form, 1, 1, 0,
 	    /* Old form -- just the interactive spec. */
 	    return list2 (Qinteractive, form);
 	}
+      else if (PVSIZE (fun) > COMPILED_DOC_STRING)
+        {
+          Lisp_Object doc = AREF (fun, COMPILED_DOC_STRING);
+          if (!(NILP (doc) || VALID_DOCSTRING_P (doc)))
+            genfun = true;
+        }
     }
 #ifdef HAVE_MODULES
   else if (MODULE_FUNCTIONP (fun))
@@ -1135,13 +1142,20 @@ DEFUN ("interactive-form", Finteractive_form, Sinteractive_form, 1, 1, 0,
 	  if (EQ (funcar, Qclosure))
 	    form = Fcdr (form);
 	  Lisp_Object spec = Fassq (Qinteractive, form);
-	  if (NILP (Fcdr (Fcdr (spec))))
+	  if (NILP (spec) && VALID_DOCSTRING_P (CAR_SAFE (form)))
+	    genfun = true;
+	  else if (NILP (Fcdr (Fcdr (spec))))
 	    return spec;
 	  else
 	    return list2 (Qinteractive, Fcar (Fcdr (spec)));
 	}
     }
-  return Qnil;
+  if (genfun
+      /* Avoid burping during bootstrap.  */
+      && !NILP (Fsymbol_function (Qoclosure_interactive_form)))
+    return call1 (Qoclosure_interactive_form, fun);
+  else
+    return Qnil;
 }
 
 DEFUN ("command-modes", Fcommand_modes, Scommand_modes, 1, 1, 0,
@@ -4123,6 +4137,7 @@ syms_of_data (void)
   DEFSYM (Qchar_table_p, "char-table-p");
   DEFSYM (Qvector_or_char_table_p, "vector-or-char-table-p");
   DEFSYM (Qfixnum_or_symbol_with_pos_p, "fixnum-or-symbol-with-pos-p");
+  DEFSYM (Qoclosure_interactive_form, "oclosure-interactive-form");
 
   DEFSYM (Qsubrp, "subrp");
   DEFSYM (Qunevalled, "unevalled");
diff --git a/src/doc.c b/src/doc.c
index 5326195c6a0..71e66853b08 100644
--- a/src/doc.c
+++ b/src/doc.c
@@ -469,9 +469,7 @@ store_function_docstring (Lisp_Object obj, EMACS_INT offset)
       if (PVSIZE (fun) > COMPILED_DOC_STRING
 	  /* Don't overwrite a non-docstring value placed there,
            * such as the symbols used for Oclosures.  */
-	  && (FIXNUMP (AREF (fun, COMPILED_DOC_STRING))
-	      || STRINGP (AREF (fun, COMPILED_DOC_STRING))
-	      || CONSP (AREF (fun, COMPILED_DOC_STRING))))
+	  && VALID_DOCSTRING_P (AREF (fun, COMPILED_DOC_STRING)))
 	ASET (fun, COMPILED_DOC_STRING, make_fixnum (offset));
       else
 	{
diff --git a/src/eval.c b/src/eval.c
index 37bc03465cc..15e34790a1c 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -2032,8 +2032,7 @@ DEFUN ("commandp", Fcommandp, Scommandp, 1, 2, 0,
   (Lisp_Object function, Lisp_Object for_call_interactively)
 {
   register Lisp_Object fun;
-  register Lisp_Object funcar;
-  Lisp_Object if_prop = Qnil;
+  bool genfun = false; /* If true, we should consult `interactive-form`.  */
 
   fun = function;
 
@@ -2041,52 +2040,87 @@ DEFUN ("commandp", Fcommandp, Scommandp, 1, 2, 0,
   if (NILP (fun))
     return Qnil;
 
-  /* Check an `interactive-form' property if present, analogous to the
-     function-documentation property.  */
-  fun = function;
-  while (SYMBOLP (fun))
-    {
-      Lisp_Object tmp = Fget (fun, Qinteractive_form);
-      if (!NILP (tmp))
-	if_prop = Qt;
-      fun = Fsymbol_function (fun);
-    }
-
   /* Emacs primitives are interactive if their DEFUN specifies an
      interactive spec.  */
   if (SUBRP (fun))
-    return XSUBR (fun)->intspec.string ? Qt : if_prop;
-
+    {
+      if (XSUBR (fun)->intspec.string)
+        return Qt;
+    }
   /* 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));
+        }
+    }
 
 #ifdef HAVE_MODULES
   /* Module functions are interactive if their `interactive_form'
      field is non-nil. */
   else if (MODULE_FUNCTIONP (fun))
-    return NILP (module_function_interactive_form (XMODULE_FUNCTION (fun)))
-             ? if_prop
-             : Qt;
+    {
+      if (!NILP (module_function_interactive_form (XMODULE_FUNCTION (fun))))
+        return Qt;
+    }
 #endif
 
   /* 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;
-  funcar = XCAR (fun);
-  if (EQ (funcar, Qclosure))
-    return (!NILP (Fassq (Qinteractive, Fcdr (Fcdr (XCDR (fun)))))
-	    ? Qt : if_prop);
-  else if (EQ (funcar, Qlambda))
-    return !NILP (Fassq (Qinteractive, Fcdr (XCDR (fun)))) ? Qt : if_prop;
-  else if (EQ (funcar, Qautoload))
-    return !NILP (Fcar (Fcdr (Fcdr (XCDR (fun))))) ? Qt : if_prop;
+  else
+    {
+      Lisp_Object funcar = XCAR (fun);
+      if (EQ (funcar, Qautoload))
+        {
+          if (!NILP (Fcar (Fcdr (Fcdr (XCDR (fun))))))
+            return Qt;
+        }
+      else
+        {
+          Lisp_Object body = CDR_SAFE (XCDR (fun));
+          if (EQ (funcar, Qclosure))
+            body = CDR_SAFE (body);
+          else if (!EQ (funcar, Qlambda))
+	    return Qnil;
+	  if (!NILP (Fassq (Qinteractive, body)))
+	    return Qt;
+	  else if (VALID_DOCSTRING_P (CAR_SAFE (body)))
+	    genfun = true;
+	}
+    }
+
+  /* By now, if it's not a function we already returned nil.  */
+
+  /* Check an `interactive-form' property if present, analogous to the
+     function-documentation property.  */
+  fun = function;
+  while (SYMBOLP (fun))
+    {
+      Lisp_Object tmp = Fget (fun, Qinteractive_form);
+      if (!NILP (tmp))
+	error ("Found an `interactive-form` property!");
+      fun = Fsymbol_function (fun);
+    }
+
+  /* If there's no immediate interactive form but it's an OClosure,
+     then delegate to the generic-function in case it has
+     a type-specific interactive-form.  */
+  if (genfun)
+    {
+      Lisp_Object iform = call1 (Qinteractive_form, fun);
+      return NILP (iform) ? Qnil : Qt;
+    }
   else
     return Qnil;
 }
diff --git a/src/lisp.h b/src/lisp.h
index 75f369f5245..1ad89fc4689 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -2185,6 +2185,16 @@ XSUBR (Lisp_Object a)
   return &XUNTAG (a, Lisp_Vectorlike, union Aligned_Lisp_Subr)->s;
 }
 
+/* Return whether a value might be a valid docstring.
+   Used to distinguish the presence of non-docstring in the docstring slot,
+   as in the case of OClosures.  */
+INLINE bool
+VALID_DOCSTRING_P (Lisp_Object doc)
+{
+  return FIXNUMP (doc) || STRINGP (doc)
+         || (CONSP (doc) && STRINGP (XCAR (doc)) && FIXNUMP (XCDR (doc)));
+}
+
 enum char_table_specials
   {
     /* This is the number of slots that every char table must have.  This
diff --git a/test/lisp/emacs-lisp/oclosure-tests.el b/test/lisp/emacs-lisp/oclosure-tests.el
index b6bdebc0a2b..1af40bcdab4 100644
--- a/test/lisp/emacs-lisp/oclosure-tests.el
+++ b/test/lisp/emacs-lisp/oclosure-tests.el
@@ -106,6 +106,27 @@ oclosure-test-limits
       (and (eq 'error (car err))
            (string-match "Duplicate slot: fst$" (cadr err)))))))
 
+(cl-defmethod oclosure-interactive-form ((ot oclosure-test))
+  (let ((snd (oclosure-test--snd ot)))
+    (if (stringp snd) (list 'interactive snd))))
+
+(ert-deftest oclosure-test-interactive-form ()
+  (should (equal (interactive-form
+                  (oclosure-lambda (oclosure-test (fst 1) (snd 2))
+                      () fst))
+                 nil))
+  (should (equal (interactive-form
+                  (oclosure-lambda (oclosure-test (fst 1) (snd 2))
+                      ()
+                    (interactive "r")
+                    fst))
+                 '(interactive "r")))
+  (should (equal (interactive-form
+                  (oclosure-lambda (oclosure-test (fst 1) (snd "P"))
+                      ()
+                    fst))
+                 '(interactive "P"))))
+
 (oclosure-define (oclosure-test-mut
                   (:parent oclosure-test)
                   (:copier oclosure-test-mut-copy))

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.

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

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

> Thanks.  A few minor comments below:

See updated patch after my sig.

> 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.

I tried to do that.  Let me know if that fits your expectations.

> 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?

Currently, it is fundamentally defined not by the syntax of the
`interactive` thingy in source code but by what `call-interactively`
expects as return value of `interactive-form`.

So yes, it can return `(interactive (list <foo> <bar>))` just fine.

OTOH it currently doesn't offer any way to have an OClosure with
a non-nil `command-modes`.
I.e. if you return (interactive (list <foo> gomoku-mode)) the
`gomoku-mode` part will not be understood as a `commands-mode` spec and
may even cause trouble since `interactive-form` is not expected to
return something of this form (tho most callers just extract the
form with `cadr` and just ignore any extra elements).

Maybe you're right that we should define the return value as "whatever is
accepted in the `interactive` source thingy", and then arrange for
`command-modes` to delegate to `oclosure-interactive-mode`?

> Does it use the same syntax, or will we need
> to use some special quoting there?

No special quoting, no.

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

Indeed, it is better not to abuse it.

>> +      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.

Added.

>> +  bool genfun = false; /* If true, we should consult `interactive-form`.  */
> Please don't use Markdown-style quoting in code comments.

Duh, sorry, they were "everywhere".

>>    /* 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.

That was a left over from an earlier code reorg.


        Stefan


diff --git a/doc/lispref/commands.texi b/doc/lispref/commands.texi
index ace0c025512..6c60216796c 100644
--- a/doc/lispref/commands.texi
+++ b/doc/lispref/commands.texi
@@ -312,6 +312,25 @@ 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}.
+@end defun
+
+@defun oclosure-interactive-form function
+Just like @code{interactive-form}, this function takes a command and
+returns its interactive form.  The difference is that it is a generic
+function and it is only called when @var{function} is an OClosure.
+The purpose is to make it possible for some OClosure types to compute
+their interactive forms dynamically instead of carrying it in one of
+their slots.
+
+This is used for example for @code{kmacro} functions in order to
+reduce their memory size, since they all share the same interactive
+form.  It is also used for @code{advice} functions, where the
+interactive form is computed from the interactive forms of its
+components, so as to make this computation more lazily and to
+correctly adjust the interactive form when one of its component's
+is redefined.
 @end defun
 
 @node Interactive Codes
diff --git a/etc/NEWS b/etc/NEWS
index 3442ebd81b3..62b7128fea5 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1292,6 +1292,11 @@ remote host are shown.  Alternatively, the user option
 Allows the creation of "functions with slots" or "function objects"
 via the macros 'oclosure-define' and 'oclosure-lambda'.
 
+*** New generic function 'oclosure-interactive-form'.
+Used by 'interactive-form' when called on an OClosure.
+This allows specific OClosure types to compute their interactive specs
+on demand rather than precompute them when created.
+
 ---
 ** New theme 'leuven-dark'.
 This is a dark version of the 'leuven' theme.
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"))
+
 ;;;###autoload
 (defun kmacro-lambda-form (mac &optional counter format)
   ;; Apparently, there are two different ways this is called:
diff --git a/lisp/simple.el b/lisp/simple.el
index 7e964c9d1d5..ead973d45e0 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2389,6 +2389,15 @@ function-documentation
 (cl-defmethod function-documentation ((function accessor))
   (oclosure--accessor-docstring function)) ;; FIXME: η-reduce!
 
+;; This should be in `oclosure.el' but that file is loaded before `cl-generic'.
+(cl-defgeneric oclosure-interactive-form (_function)
+  "Return the interactive form of FUNCTION or nil if none.
+This is called by `interactive-form' when invoked on OClosures.
+Add your methods to this generic function, but always call `interactive-form'
+instead."
+  ;; (interactive-form function)
+  nil)
+
 (defun command-execute (cmd &optional record-flag keys special)
   ;; BEWARE: Called directly from the C code.
   "Execute CMD as an editor command.
diff --git a/src/callint.c b/src/callint.c
index 31919d6bb81..92bfaf8d397 100644
--- a/src/callint.c
+++ b/src/callint.c
@@ -315,7 +315,7 @@ DEFUN ("call-interactively", Fcall_interactively, Scall_interactively, 1, 3, 0,
   Lisp_Object up_event = Qnil;
 
   /* Set SPECS to the interactive form, or barf if not interactive.  */
-  Lisp_Object form = Finteractive_form (function);
+  Lisp_Object form = call1 (Qinteractive_form, function);
   if (! CONSP (form))
     wrong_type_argument (Qcommandp, function);
   Lisp_Object specs = Fcar (XCDR (form));
diff --git a/src/data.c b/src/data.c
index 72af8a6648e..e9aad75f59b 100644
--- a/src/data.c
+++ b/src/data.c
@@ -1072,6 +1072,7 @@ DEFUN ("interactive-form", Finteractive_form, Sinteractive_form, 1, 1, 0,
   (Lisp_Object cmd)
 {
   Lisp_Object fun = indirect_function (cmd); /* Check cycles.  */
+  bool genfun = false;
 
   if (NILP (fun))
     return Qnil;
@@ -1113,6 +1114,12 @@ DEFUN ("interactive-form", Finteractive_form, Sinteractive_form, 1, 1, 0,
 	    /* Old form -- just the interactive spec. */
 	    return list2 (Qinteractive, form);
 	}
+      else if (PVSIZE (fun) > COMPILED_DOC_STRING)
+        {
+          Lisp_Object doc = AREF (fun, COMPILED_DOC_STRING);
+          /* An invalid "docstring" is a sign that we have an OClosure.  */
+          genfun = !(NILP (doc) || VALID_DOCSTRING_P (doc));
+        }
     }
 #ifdef HAVE_MODULES
   else if (MODULE_FUNCTIONP (fun))
@@ -1135,13 +1142,21 @@ DEFUN ("interactive-form", Finteractive_form, Sinteractive_form, 1, 1, 0,
 	  if (EQ (funcar, Qclosure))
 	    form = Fcdr (form);
 	  Lisp_Object spec = Fassq (Qinteractive, form);
-	  if (NILP (Fcdr (Fcdr (spec))))
+	  if (NILP (spec) && VALID_DOCSTRING_P (CAR_SAFE (form)))
+            /* A "docstring" is a sign that we may have an OClosure.  */
+	    genfun = true;
+	  else if (NILP (Fcdr (Fcdr (spec))))
 	    return spec;
 	  else
 	    return list2 (Qinteractive, Fcar (Fcdr (spec)));
 	}
     }
-  return Qnil;
+  if (genfun
+      /* Avoid burping during bootstrap.  */
+      && !NILP (Fsymbol_function (Qoclosure_interactive_form)))
+    return call1 (Qoclosure_interactive_form, fun);
+  else
+    return Qnil;
 }
 
 DEFUN ("command-modes", Fcommand_modes, Scommand_modes, 1, 1, 0,
@@ -4123,6 +4138,7 @@ syms_of_data (void)
   DEFSYM (Qchar_table_p, "char-table-p");
   DEFSYM (Qvector_or_char_table_p, "vector-or-char-table-p");
   DEFSYM (Qfixnum_or_symbol_with_pos_p, "fixnum-or-symbol-with-pos-p");
+  DEFSYM (Qoclosure_interactive_form, "oclosure-interactive-form");
 
   DEFSYM (Qsubrp, "subrp");
   DEFSYM (Qunevalled, "unevalled");
diff --git a/src/doc.c b/src/doc.c
index 5326195c6a0..71e66853b08 100644
--- a/src/doc.c
+++ b/src/doc.c
@@ -469,9 +469,7 @@ store_function_docstring (Lisp_Object obj, EMACS_INT offset)
       if (PVSIZE (fun) > COMPILED_DOC_STRING
 	  /* Don't overwrite a non-docstring value placed there,
            * such as the symbols used for Oclosures.  */
-	  && (FIXNUMP (AREF (fun, COMPILED_DOC_STRING))
-	      || STRINGP (AREF (fun, COMPILED_DOC_STRING))
-	      || CONSP (AREF (fun, COMPILED_DOC_STRING))))
+	  && VALID_DOCSTRING_P (AREF (fun, COMPILED_DOC_STRING)))
 	ASET (fun, COMPILED_DOC_STRING, make_fixnum (offset));
       else
 	{
diff --git a/src/eval.c b/src/eval.c
index 37bc03465cc..1de59518381 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -2032,8 +2032,7 @@ DEFUN ("commandp", Fcommandp, Scommandp, 1, 2, 0,
   (Lisp_Object function, Lisp_Object for_call_interactively)
 {
   register Lisp_Object fun;
-  register Lisp_Object funcar;
-  Lisp_Object if_prop = Qnil;
+  bool genfun = false; /* If true, we should consult `interactive-form'.  */
 
   fun = function;
 
@@ -2041,52 +2040,89 @@ DEFUN ("commandp", Fcommandp, Scommandp, 1, 2, 0,
   if (NILP (fun))
     return Qnil;
 
-  /* Check an `interactive-form' property if present, analogous to the
-     function-documentation property.  */
-  fun = function;
-  while (SYMBOLP (fun))
-    {
-      Lisp_Object tmp = Fget (fun, Qinteractive_form);
-      if (!NILP (tmp))
-	if_prop = Qt;
-      fun = Fsymbol_function (fun);
-    }
-
   /* Emacs primitives are interactive if their DEFUN specifies an
      interactive spec.  */
   if (SUBRP (fun))
-    return XSUBR (fun)->intspec.string ? Qt : if_prop;
-
+    {
+      if (XSUBR (fun)->intspec.string)
+        return Qt;
+    }
   /* 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);
+          /* An invalid "docstring" is a sign that we have an OClosure.  */
+          genfun = !(NILP (doc) || VALID_DOCSTRING_P (doc));
+        }
+    }
 
 #ifdef HAVE_MODULES
   /* Module functions are interactive if their `interactive_form'
      field is non-nil. */
   else if (MODULE_FUNCTIONP (fun))
-    return NILP (module_function_interactive_form (XMODULE_FUNCTION (fun)))
-             ? if_prop
-             : Qt;
+    {
+      if (!NILP (module_function_interactive_form (XMODULE_FUNCTION (fun))))
+        return Qt;
+    }
 #endif
 
   /* 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))
     return Qnil;
-  funcar = XCAR (fun);
-  if (EQ (funcar, Qclosure))
-    return (!NILP (Fassq (Qinteractive, Fcdr (Fcdr (XCDR (fun)))))
-	    ? Qt : if_prop);
-  else if (EQ (funcar, Qlambda))
-    return !NILP (Fassq (Qinteractive, Fcdr (XCDR (fun)))) ? Qt : if_prop;
-  else if (EQ (funcar, Qautoload))
-    return !NILP (Fcar (Fcdr (Fcdr (XCDR (fun))))) ? Qt : if_prop;
+  else
+    {
+      Lisp_Object funcar = XCAR (fun);
+      if (EQ (funcar, Qautoload))
+        {
+          if (!NILP (Fcar (Fcdr (Fcdr (XCDR (fun))))))
+            return Qt;
+        }
+      else
+        {
+          Lisp_Object body = CDR_SAFE (XCDR (fun));
+          if (EQ (funcar, Qclosure))
+            body = CDR_SAFE (body);
+          else if (!EQ (funcar, Qlambda))
+	    return Qnil;
+	  if (!NILP (Fassq (Qinteractive, body)))
+	    return Qt;
+	  else if (VALID_DOCSTRING_P (CAR_SAFE (body)))
+            /* A "docstring" is a sign that we may have an OClosure.  */
+	    genfun = true;
+	}
+    }
+
+  /* By now, if it's not a function we already returned nil.  */
+
+  /* Check an `interactive-form' property if present, analogous to the
+     function-documentation property.  */
+  fun = function;
+  while (SYMBOLP (fun))
+    {
+      Lisp_Object tmp = Fget (fun, Qinteractive_form);
+      if (!NILP (tmp))
+	error ("Found an 'interactive-form' property!");
+      fun = Fsymbol_function (fun);
+    }
+
+  /* If there's no immediate interactive form but it's an OClosure,
+     then delegate to the generic-function in case it has
+     a type-specific interactive-form.  */
+  if (genfun)
+    {
+      Lisp_Object iform = call1 (Qinteractive_form, fun);
+      return NILP (iform) ? Qnil : Qt;
+    }
   else
     return Qnil;
 }
diff --git a/src/lisp.h b/src/lisp.h
index 75f369f5245..1ad89fc4689 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -2185,6 +2185,16 @@ XSUBR (Lisp_Object a)
   return &XUNTAG (a, Lisp_Vectorlike, union Aligned_Lisp_Subr)->s;
 }
 
+/* Return whether a value might be a valid docstring.
+   Used to distinguish the presence of non-docstring in the docstring slot,
+   as in the case of OClosures.  */
+INLINE bool
+VALID_DOCSTRING_P (Lisp_Object doc)
+{
+  return FIXNUMP (doc) || STRINGP (doc)
+         || (CONSP (doc) && STRINGP (XCAR (doc)) && FIXNUMP (XCDR (doc)));
+}
+
 enum char_table_specials
   {
     /* This is the number of slots that every char table must have.  This
diff --git a/test/lisp/emacs-lisp/oclosure-tests.el b/test/lisp/emacs-lisp/oclosure-tests.el
index b6bdebc0a2b..1af40bcdab4 100644
--- a/test/lisp/emacs-lisp/oclosure-tests.el
+++ b/test/lisp/emacs-lisp/oclosure-tests.el
@@ -106,6 +106,27 @@ oclosure-test-limits
       (and (eq 'error (car err))
            (string-match "Duplicate slot: fst$" (cadr err)))))))
 
+(cl-defmethod oclosure-interactive-form ((ot oclosure-test))
+  (let ((snd (oclosure-test--snd ot)))
+    (if (stringp snd) (list 'interactive snd))))
+
+(ert-deftest oclosure-test-interactive-form ()
+  (should (equal (interactive-form
+                  (oclosure-lambda (oclosure-test (fst 1) (snd 2))
+                      () fst))
+                 nil))
+  (should (equal (interactive-form
+                  (oclosure-lambda (oclosure-test (fst 1) (snd 2))
+                      ()
+                    (interactive "r")
+                    fst))
+                 '(interactive "r")))
+  (should (equal (interactive-form
+                  (oclosure-lambda (oclosure-test (fst 1) (snd "P"))
+                      ()
+                    fst))
+                 '(interactive "P"))))
+
 (oclosure-define (oclosure-test-mut
                   (:parent oclosure-test)
                   (:copier oclosure-test-mut-copy))

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

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

> See updated patch after my sig.

Pushed with a few more tweaks,


        Stefan

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

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" [2022-04-19 13:52 -0400] wrote:

> @@ -2041,52 +2040,89 @@ DEFUN ("commandp", Fcommandp, Scommandp, 1, 2, 0,

[...]

> +  /* By now, if it's not a function we already returned nil.  */
> +
> +  /* Check an `interactive-form' property if present, analogous to the
> +     function-documentation property.  */
> +  fun = function;
> +  while (SYMBOLP (fun))
> +    {
> +      Lisp_Object tmp = Fget (fun, Qinteractive_form);
> +      if (!NILP (tmp))
> +	error ("Found an 'interactive-form' property!");
> +      fun = Fsymbol_function (fun);
> +    }

error ("Success!");

Why is it now an error for functions to have an interactive-form
property?  The Elisp manual is careful to describe this practice as
unusual, but nevertheless supported, e.g. in cases such as:

0. emacs -Q -f toggle-debug-on-error
1. (progn
     (defun my-foo (&rest _))
     (function-put 'my-foo 'interactive-form
                   (interactive-form 'ignore)))
2. C-x C-e
3. M-x C-i

Debugger entered--Lisp error: (error "Found an ’interactive-form’ property!")
  commandp(my-foo)
  [...]

Thanks,

-- 
Basil

In GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo version 1.16.0, Xaw3d scroll bars)
 of 2022-04-27 built on tia
Repository revision: 0beb8fd663663dcaa1bda4df5995d10f1ef615fb
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101003
System Description: Debian GNU/Linux bookworm/sid

Configured using:
 'configure 'CFLAGS=-Og -ggdb3' --config-cache --prefix /home/blc/.local
 --enable-checking=structs --with-x-toolkit=lucid
 --with-file-notification=yes --with-xinput2 --with-x'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS WEBP X11 XAW3D XDBE XIM XINPUT2 XPM LUCID ZLIB

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

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Basil L. Contovounesios [2022-04-27 19:05 +0300] wrote:

> Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" [2022-04-19 13:52 -0400] wrote:
>
>> @@ -2041,52 +2040,89 @@ DEFUN ("commandp", Fcommandp, Scommandp, 1, 2, 0,
>
> [...]
>
>> +  /* By now, if it's not a function we already returned nil.  */
>> +
>> +  /* Check an `interactive-form' property if present, analogous to the
>> +     function-documentation property.  */
>> +  fun = function;
>> +  while (SYMBOLP (fun))
>> +    {
>> +      Lisp_Object tmp = Fget (fun, Qinteractive_form);
>> +      if (!NILP (tmp))
>> +	error ("Found an 'interactive-form' property!");
>> +      fun = Fsymbol_function (fun);
>> +    }
>
> error ("Success!");
>
> Why is it now an error for functions to have an interactive-form
> property?  The Elisp manual is careful to describe this practice as
> unusual, but nevertheless supported, e.g. in cases such as:
>
> 0. emacs -Q -f toggle-debug-on-error
> 1. (progn
>      (defun my-foo (&rest _))
>      (function-put 'my-foo 'interactive-form
>                    (interactive-form 'ignore)))
> 2. C-x C-e
> 3. M-x C-i
>
> Debugger entered--Lisp error: (error "Found an ’interactive-form’ property!")
>   commandp(my-foo)
>   [...]

In the meantime should I reopen this bug or report a new one, so this
isn't forgotten about?

Thanks,

-- 
Basil

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

[Eli Zaretskii]

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Cc: Eli Zaretskii <eliz@gnu.org>,  54802@debbugs.gnu.org
> Date: Sun, 12 Jun 2022 00:24:48 +0300
> 
> > Why is it now an error for functions to have an interactive-form
> > property?  The Elisp manual is careful to describe this practice as
> > unusual, but nevertheless supported, e.g. in cases such as:
> >
> > 0. emacs -Q -f toggle-debug-on-error
> > 1. (progn
> >      (defun my-foo (&rest _))
> >      (function-put 'my-foo 'interactive-form
> >                    (interactive-form 'ignore)))
> > 2. C-x C-e
> > 3. M-x C-i
> >
> > Debugger entered--Lisp error: (error "Found an ’interactive-form’ property!")
> >   commandp(my-foo)
> >   [...]
> 
> In the meantime should I reopen this bug or report a new one, so this
> isn't forgotten about?

Yes, please.  A new bug is probably better.

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

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii [2022-06-12 01:34 -0400] wrote:

>> From: "Basil L. Contovounesios" <contovob@tcd.ie>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  54802@debbugs.gnu.org
>> Date: Sun, 12 Jun 2022 00:24:48 +0300
>> 
>> In the meantime should I reopen this bug or report a new one, so this
>> isn't forgotten about?
>
> Yes, please.  A new bug is probably better.

Here it is, for posterity: https://bugs.gnu.org/55925

-- 
Basil