From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#61244: 30.0.50; [PATCH] Promote called-interactively-p Date: Fri, 03 Feb 2023 10:52:13 -0500 Message-ID: References: <87sffnzrbj.fsf@dick> Reply-To: Stefan Monnier Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="40523"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: 61244@debbugs.gnu.org To: dick.r.chiang@gmail.com Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri Feb 03 16:53:55 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pNyNs-000AFP-9i for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 03 Feb 2023 16:53:52 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pNyN8-00068c-3Y; Fri, 03 Feb 2023 10:53:06 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pNyN4-00067w-9e for bug-gnu-emacs@gnu.org; Fri, 03 Feb 2023 10:53:04 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1pNyN4-0005nJ-1O for bug-gnu-emacs@gnu.org; Fri, 03 Feb 2023 10:53:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1pNyN3-0003pT-UG for bug-gnu-emacs@gnu.org; Fri, 03 Feb 2023 10:53:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 03 Feb 2023 15:53:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 61244 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 61244-submit@debbugs.gnu.org id=B61244.167543954614675 (code B ref 61244); Fri, 03 Feb 2023 15:53:01 +0000 Original-Received: (at 61244) by debbugs.gnu.org; 3 Feb 2023 15:52:26 +0000 Original-Received: from localhost ([127.0.0.1]:39831 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pNyMT-0003od-K7 for submit@debbugs.gnu.org; Fri, 03 Feb 2023 10:52:26 -0500 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:20041) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pNyMR-0003oN-6m for 61244@debbugs.gnu.org; Fri, 03 Feb 2023 10:52:24 -0500 Original-Received: from pmg1.iro.umontreal.ca (localhost.localdomain [127.0.0.1]) by pmg1.iro.umontreal.ca (Proxmox) with ESMTP id 4DA461000D2; Fri, 3 Feb 2023 10:52:17 -0500 (EST) Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg1.iro.umontreal.ca (Proxmox) with ESMTP id A05BE1000C7; Fri, 3 Feb 2023 10:52:14 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1675439534; bh=gQ4psLzOHLnwK31oG+jbP0CsPv4F4cfZ+KkfGzQybaE=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=gAS5ooFVdHlJbGGShAI2gYfQngiRT2AXdInyvw/O8TIgcvS+DIwgz/aZ6c+XflPrY o8sN5eODGmXBqOVoWtWT/esocB1grWhLJ+bQ0lBzMVzEoL7IpwGATO/jEiWXg0W9Nv El3+8zqI0lfSGCCjU03Dx/6ev+fOy3Fb9R6rF943HVrnh8NJC/To9bksnlFwZzEbX5 L2fSTKw1S35yfmlxPScyc+KROezxU4lVW1d5i4q+ImrYBlCPp7Qll5Hqet6ByylL4Y sCwUA+MZrQLyFS8obWm6s/hB5tvs4yIDiZvOzHLBy4olscGjtCDys4pSYZxGPsRjXx NDkIFxvYiQD5w== Original-Received: from pastel (76-10-137-88.dsl.teksavvy.com [76.10.137.88]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 7A856120488; Fri, 3 Feb 2023 10:52:14 -0500 (EST) In-Reply-To: <87sffnzrbj.fsf@dick> (dick r. chiang's message of "Thu, 02 Feb 2023 16:32:32 -0500") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:254735 Archived-At: > FUD surrounding `called-interactively-p` continues to saddle functions > with an incongruous "interactive-p" optional argument. If the arg is called `interactive-p` it's usually because the programmer was too lazy to think about a good name for that arg (one which actually describes the effect it has). Maybe in our recommendation for the use of an extra argument, we should make an effort to mention that (i.e. that we recommend that the arg's name reflect the effect, and that the docstring document it accordingly). > `called-interactively-p` is safe enough, I agree that `called-interactively-p` is safe: nothing bad will result if you call it. But its return value is unreliable (when it returns non-nil, I think it *is* reliable, but in many cases it will return nil even tho it "should" return non-nil). > +runtime links one to the other. When @code{called-interactively-p} is > +invoked, tracing the stack frame of the subject function is fraught > +given how many intervening function calls could result from arbitrary > +macro expansions, special forms, and advices including those for > +debugging instrumentation. The heuristics applied cannot guarantee a > +correct result under all conceivable conditions. Clearly, we agree on the technical aspect. We just disagree about what conclusion to draw from it. I personally can't recommend the use of a function for which we "cannot guarantee a correct result under all conceivable conditions" when there is a good alternative which does. > + "Return t if the containing function was called interactively. > +Be warned the function may yield an incorrect result when the > +containing function is advised or instrumented for debugging, or > +when the call to `called-interactively-p' is enclosed in > +macros or special forms which wrap it in a lambda closure. > + > +If knowing the calling context is critical, one must modify the > +containing function's lexical environment as described in Info > +node `(elisp)Distinguish Interactive'. > + > +If KIND is \\='interactive, the function returns nil if either > +`executing-kbd-macro' or `noninteractive' is true. The KIND > +argument is deprecated in favor of checking those conditions > +outside this function." > + (let ((kind-exception (and (eq kind 'interactive) > + (or noninteractive executing-kbd-macro)))) > + (unless kind-exception > + ;; Call stack grows down with decreasing I. > + ;; Walk up stack until containing function's frame reached. > + (let* ((i 0) > + (child (backtrace-frame i 'called-interactively-p)) > + (parent (backtrace-frame (1+ i) 'called-interactively-p)) > + (walk-up-stack > + (lambda () > + (setq i (1+ i) > + child parent > + parent (backtrace-frame (1+ i) 'called-interactively-p))))) > + (while (progn (funcall walk-up-stack) > + (or > + ;; Skip special forms from non-compiled code. > + (and child (null (car child))) > + ;; Skip package-specific stack-frames. > + (let ((skip (run-hook-with-args-until-success > + 'called-interactively-p-functions > + (+ i 2) child parent))) > + (pcase skip > + ('nil nil) > + (0 t) > + (_ (setq i (1- (+ i skip))) > + (funcall walk-up-stack))))))) > + ;; CHILD should now be containing function. > + (pcase (cons child parent) > + ;; checks if CHILD is built-in primitive (never interactive). > + (`((,_ ,(pred (lambda (f) (subr-primitive-p (indirect-function f)))) . ,_) . ,_) > + nil) > + ;; checks if PARENT is `funcall_interactively'. > + (`(,_ . (t ,(pred (lambda (f) > + (eq internal--funcall-interactively > + (indirect-function f)))) > + . ,_)) > + t)))))) You describe this change as "Clarify". I don't immediately see whether the code does the same as before nor in which sense it's more clear. Can you describe a bit more precisely what changes you've made here? I see you renamed the frames to `child` and `parent`, which doesn't sound too bad. [ One cosmetic thing I notice is that I was careful to have a single copy of (backtrace-frame i 'called-interactively-p) whereas you now have 3 of them. ] > +(define-obsolete-function-alias 'interactive-p > + #'called-interactively-p "23.2" > + "Keep alias (https://lists.gnu.org/r/emacs-devel/2020-08/msg00564.html)") I don't have an opinion on that. > -/* BEWARE: Calling this directly from C would defeat the purpose! */ > DEFUN ("funcall-interactively", Ffuncall_interactively, Sfuncall_interactively, > - 1, MANY, 0, doc: /* Like `funcall' but marks the call as interactive. > -I.e. arrange that within the called function `called-interactively-p' will > -return non-nil. > + 1, MANY, 0, doc: /* Differentiate from `funcall' to indicate interactive call. > +The function `called-interactively-p' looks for this very function token. > +This primitive should not be called from C since its very purpose > +is to appear as a literal token in the lisp call stack. > usage: (funcall-interactively FUNCTION &rest ARGUMENTS) */) > (ptrdiff_t nargs, Lisp_Object *args) > { > specpdl_ref speccount = SPECPDL_INDEX (); > temporarily_switch_to_single_kboard (NULL); > - > - /* Nothing special to do here, all the work is inside > - `called-interactively-p'. Which will look for us as a marker in the > - backtrace. */ > return unbind_to (speccount, Ffuncall (nargs, args)); > } No clear opinion on this either. I like when comments are replaced by actual documentation, but we usually don't put into docstrings info about whether a function can be called from C (docstrings are meant for ELisp callers only). > diff --git a/test/lisp/emacs-lisp/edebug-resources/edebug-test-code.el b/test/lisp/emacs-lisp/edebug-resources/edebug-test-code.el > index b0211c915e6..b033fdddcd8 100644 > --- a/test/lisp/emacs-lisp/edebug-resources/edebug-test-code.el > +++ b/test/lisp/emacs-lisp/edebug-resources/edebug-test-code.el > @@ -33,6 +33,10 @@ edebug-test-code-fac > (* n (edebug-test-code-fac (1- n)))!mult! > 1)) > > +(defun edebug-test-code-called-interactively-p () > + (interactive) > + !start!(called-interactively-p)) > + > (defun edebug-test-code-concat (a b flag) > !start!(if flag!flag! > !then-start!(concat a!then-a! b!then-b!)!then-concat! > diff --git a/test/lisp/emacs-lisp/edebug-tests.el b/test/lisp/emacs-lisp/edebug-tests.el > index de2fff5ef19..72ea5874cae 100644 > --- a/test/lisp/emacs-lisp/edebug-tests.el > +++ b/test/lisp/emacs-lisp/edebug-tests.el > @@ -56,6 +56,7 @@ edebug-tests-failure-in-post-command > (defvar-keymap edebug-tests-keymap > :doc "Keys used by the keyboard macros in Edebug's tests." > "@" 'edebug-tests-call-instrumented-func > + "#" 'edebug-tests-call-interactively-instrumented-func > "C-u" 'universal-argument > "C-p" 'previous-line > "C-n" 'next-line > @@ -268,6 +269,13 @@ edebug-tests-setup-@ > edebug-tests-args args) > (setq edebug-tests-@-result 'no-result))) > > +(defun edebug-tests-call-interactively-instrumented-func () > + "Call interactively `edebug-tests-func' and save results." > + (interactive) > + (let ((result (call-interactively edebug-tests-func))) > + (should (eq edebug-tests-@-result 'no-result)) > + (setq edebug-tests-@-result result))) > + > (defun edebug-tests-call-instrumented-func () > "Call `edebug-tests-func' with `edebug-tests-args' and save the results." > (interactive) > @@ -440,6 +448,14 @@ edebug-tests-stop-point-at-start-of-first-instrumented-function > "SPC" (edebug-tests-should-be-at "fac" "step") > "g" (should (equal edebug-tests-@-result 1))))) > > +(ert-deftest edebug-tests-called-interactively-p () > + "`called-interactively-p' still works under edebug." > + (edebug-tests-with-normal-env > + (edebug-tests-setup-@ "called-interactively-p" '() t) > + (edebug-tests-run-kbd-macro > + "#" (edebug-tests-should-be-at "called-interactively-p" "start") > + "g" (should (equal edebug-tests-@-result t))))) > + > (ert-deftest edebug-tests-step-showing-evaluation-results () > "Edebug prints expression evaluation results to the echo area." > (edebug-tests-with-normal-env Are these new tests things that are fixed by your patch, or they are just new tests "for the sake of it"? [ In either case, I welcome new tests. ] > diff --git a/test/lisp/emacs-lisp/nadvice-tests.el b/test/lisp/emacs-lisp/nadvice-tests.el > index 748d42f2120..77df743a3e2 100644 > --- a/test/lisp/emacs-lisp/nadvice-tests.el > +++ b/test/lisp/emacs-lisp/nadvice-tests.el > @@ -145,9 +145,8 @@ advice-test-called-interactively-p-around > > (ert-deftest advice-test-called-interactively-p-filter-args () > "Check interaction between filter-args advice and called-interactively-p." > - :expected-result :failed > (defun sm-test7.3 () (interactive) (cons 1 (called-interactively-p))) > - (advice-add 'sm-test7.3 :filter-args #'list) > + (advice-add 'sm-test7.3 :filter-args #'identity) > (should (equal (sm-test7.3) '(1 . nil))) > (should (equal (call-interactively 'sm-test7.3) '(1 . t)))) Duh! Thanks. > @@ -163,6 +162,18 @@ advice-test-call-interactively > (advice-remove 'call-interactively #'ignore) > (should (eq (symbol-function 'call-interactively) old))))) > > +(ert-deftest advice-test-called-interactively-p-around-careful () > + "Like sm-test7.2 but defensively preserve interactive context." > + (defun sm-test7.5 () (interactive) (cons 1 (called-interactively-p))) > + (advice-add 'sm-test7.5 :around > + (lambda (f &rest args) > + (list (cons 1 (called-interactively-p)) > + (if (called-interactively-p) > + (call-interactively f) > + (apply f args))))) > + (should (equal (sm-test7.5) '((1 . nil) (1 . nil)))) > + (should (equal (call-interactively 'sm-test7.5) '((1 . t) (1 . t))))) > + > (ert-deftest advice-test-interactive () > "Check handling of interactive spec." > (defun sm-test8 (a) (interactive "p") a) I'd use `funcall-interactively` rather than `call-interactively` so as to correctly preserve `args` rather than recreate them. Stefan