Subject: bug#61244: 30.0.50; [PATCH] Promote called-interactively-p

From: Stefan Kangas <[email-masked]>
Date: Tue, 11 Feb 2025 19:04:07 -0800

Stefan Monnier <[email-masked]> writes:

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

dick, could you please look into and reply to Stefan M's comments above?

Thanks in advance.