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

[Stefan Monnier]

> 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