Package: emacs;
Reported by: Stefan Monnier <monnier <at> iro.umontreal.ca>
Date: Fri, 8 Apr 2022 20:35:01 UTC
Severity: normal
Found in version 29.0.50
Done: Stefan Monnier <monnier <at> iro.umontreal.ca>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 54802 in the body.
You can then email your comments to 54802 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
bug-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Fri, 08 Apr 2022 20:35:01 GMT) Full text and rfc822 format available.Stefan Monnier <monnier <at> iro.umontreal.ca>
:bug-gnu-emacs <at> gnu.org
.
(Fri, 08 Apr 2022 20:35:01 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: bug-gnu-emacs <at> gnu.org Subject: OClosure: Make `interactive-form` a generic function Date: Fri, 08 Apr 2022 16:33:51 -0400
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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Sat, 09 Apr 2022 05:59:02 GMT) Full text and rfc822 format available.Message #8 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Sat, 09 Apr 2022 08:58:09 +0300
> 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 <at> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Sat, 09 Apr 2022 13:51:01 GMT) Full text and rfc822 format available.Message #11 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Sat, 09 Apr 2022 09:50:01 -0400
> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Sun, 10 Apr 2022 12:46:02 GMT) Full text and rfc822 format available.Message #14 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Lars Ingebrigtsen <larsi <at> gnus.org> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Sun, 10 Apr 2022 14:45:41 +0200
Stefan Monnier <monnier <at> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Wed, 13 Apr 2022 07:55:02 GMT) Full text and rfc822 format available.Message #17 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Po Lu <luangruo <at> yahoo.com> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Wed, 13 Apr 2022 15:53:58 +0800
Stefan Monnier <monnier <at> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Thu, 14 Apr 2022 18:35:02 GMT) Full text and rfc822 format available.Message #20 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Po Lu <luangruo <at> yahoo.com> Cc: 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Thu, 14 Apr 2022 14:34:24 -0400
> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Fri, 15 Apr 2022 00:48:01 GMT) Full text and rfc822 format available.Message #23 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Po Lu <luangruo <at> yahoo.com> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Fri, 15 Apr 2022 08:46:55 +0800
Stefan Monnier <monnier <at> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Fri, 15 Apr 2022 01:20:02 GMT) Full text and rfc822 format available.Message #26 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Po Lu <luangruo <at> yahoo.com> Cc: 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Thu, 14 Apr 2022 21:18:58 -0400
>> - 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Fri, 15 Apr 2022 01:39:02 GMT) Full text and rfc822 format available.Message #29 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Po Lu <luangruo <at> yahoo.com> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Fri, 15 Apr 2022 09:37:43 +0800
Stefan Monnier <monnier <at> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Fri, 15 Apr 2022 03:25:01 GMT) Full text and rfc822 format available.Message #32 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Po Lu <luangruo <at> yahoo.com> Cc: 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Thu, 14 Apr 2022 23:24:23 -0400
>> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Fri, 15 Apr 2022 04:28:01 GMT) Full text and rfc822 format available.Message #35 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Po Lu <luangruo <at> yahoo.com> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Fri, 15 Apr 2022 12:27:02 +0800
Stefan Monnier <monnier <at> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Fri, 15 Apr 2022 16:09:01 GMT) Full text and rfc822 format available.Message #38 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Po Lu <luangruo <at> yahoo.com> Cc: 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Fri, 15 Apr 2022 12:08:04 -0400
Po Lu [2022-04-15 12:27:02] wrote: > Stefan Monnier <monnier <at> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Fri, 15 Apr 2022 16:15:02 GMT) Full text and rfc822 format available.Message #41 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: luangruo <at> yahoo.com, 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Fri, 15 Apr 2022 19:14:11 +0300
> Cc: 54802 <at> 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 <at> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Mon, 18 Apr 2022 23:00:02 GMT) Full text and rfc822 format available.Message #44 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Eli Zaretskii <eliz <at> gnu.org> Cc: luangruo <at> yahoo.com, 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Mon, 18 Apr 2022 18:59:28 -0400
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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Tue, 19 Apr 2022 05:41:02 GMT) Full text and rfc822 format available.Message #47 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: luangruo <at> yahoo.com, 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Tue, 19 Apr 2022 08:40:37 +0300
> From: Stefan Monnier <monnier <at> iro.umontreal.ca> > Cc: luangruo <at> yahoo.com, 54802 <at> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Tue, 19 Apr 2022 12:40:02 GMT) Full text and rfc822 format available.Message #50 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Eli Zaretskii <eliz <at> gnu.org> Cc: luangruo <at> yahoo.com, 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function 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. 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Tue, 19 Apr 2022 12:45:01 GMT) Full text and rfc822 format available.Message #53 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Lars Ingebrigtsen <larsi <at> gnus.org> To: Eli Zaretskii <eliz <at> gnu.org> Cc: luangruo <at> yahoo.com, 54802 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca> Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Tue, 19 Apr 2022 14:43:48 +0200
Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs <at> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Tue, 19 Apr 2022 13:02:02 GMT) Full text and rfc822 format available.Message #56 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: luangruo <at> yahoo.com, 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Tue, 19 Apr 2022 16:00:58 +0300
> From: Stefan Monnier <monnier <at> iro.umontreal.ca> > Cc: luangruo <at> yahoo.com, 54802 <at> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Tue, 19 Apr 2022 13:35:02 GMT) Full text and rfc822 format available.Message #59 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Eli Zaretskii <eliz <at> gnu.org> Cc: luangruo <at> yahoo.com, 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function 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. 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Tue, 19 Apr 2022 13:54:02 GMT) Full text and rfc822 format available.Message #62 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: luangruo <at> yahoo.com, 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Tue, 19 Apr 2022 16:53:37 +0300
> From: Stefan Monnier <monnier <at> iro.umontreal.ca> > Cc: luangruo <at> yahoo.com, 54802 <at> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Tue, 19 Apr 2022 14:54:02 GMT) Full text and rfc822 format available.Message #65 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Tue, 19 Apr 2022 10:53:06 -0400
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 <at> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Tue, 19 Apr 2022 16:36:01 GMT) Full text and rfc822 format available.Message #68 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Tue, 19 Apr 2022 19:35:34 +0300
> 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 <at> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Tue, 19 Apr 2022 17:53:02 GMT) Full text and rfc822 format available.Message #71 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Tue, 19 Apr 2022 13:52:10 -0400
> 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))
Stefan Monnier <monnier <at> iro.umontreal.ca>
:Stefan Monnier <monnier <at> iro.umontreal.ca>
:Message #76 received at 54802-done <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 54802-done <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Tue, 26 Apr 2022 10:38:11 -0400
> See updated patch after my sig. Pushed with a few more tweaks, Stefan
bug-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Wed, 27 Apr 2022 16:06:02 GMT) Full text and rfc822 format available.Message #79 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: "Basil L. Contovounesios" <contovob <at> tcd.ie> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: Eli Zaretskii <eliz <at> gnu.org>, 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Wed, 27 Apr 2022 19:05:03 +0300
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
Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org
.
(Thu, 26 May 2022 11:24:04 GMT) Full text and rfc822 format available."Basil L. Contovounesios" <contovob <at> tcd.ie>
to control <at> debbugs.gnu.org
.
(Sat, 11 Jun 2022 21:24:02 GMT) Full text and rfc822 format available.bug-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Sat, 11 Jun 2022 21:25:02 GMT) Full text and rfc822 format available.Message #86 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: "Basil L. Contovounesios" <contovob <at> tcd.ie> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: Eli Zaretskii <eliz <at> gnu.org>, 54802 <at> debbugs.gnu.org Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Sun, 12 Jun 2022 00:24:48 +0300
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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Sun, 12 Jun 2022 05:35:02 GMT) Full text and rfc822 format available.Message #89 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: "Basil L. Contovounesios" <contovob <at> tcd.ie> Cc: 54802 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Sun, 12 Jun 2022 08:34:32 +0300
> From: "Basil L. Contovounesios" <contovob <at> tcd.ie> > Cc: Eli Zaretskii <eliz <at> gnu.org>, 54802 <at> 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-gnu-emacs <at> gnu.org
:bug#54802
; Package emacs
.
(Sun, 12 Jun 2022 20:57:01 GMT) Full text and rfc822 format available.Message #92 received at 54802 <at> debbugs.gnu.org (full text, mbox):
From: "Basil L. Contovounesios" <contovob <at> tcd.ie> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 54802 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca Subject: Re: bug#54802: OClosure: Make `interactive-form` a generic function Date: Sun, 12 Jun 2022 23:56:18 +0300
Eli Zaretskii [2022-06-12 01:34 -0400] wrote: >> From: "Basil L. Contovounesios" <contovob <at> tcd.ie> >> Cc: Eli Zaretskii <eliz <at> gnu.org>, 54802 <at> 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
Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org
.
(Mon, 11 Jul 2022 11:24:04 GMT) Full text and rfc822 format available.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.