bug#11747: 24.1.50; called-interactively-p vs. advice

bug#11747: 24.1.50; called-interactively-p vs. advice

[Michael Heerdegen]

Hi,

`called-interactively-p' returns wrong values when called inside
commands that are adviced.

Example:

(defun interactive-test ()
  (interactive)
  (message "Called %sinteractively"
           (if (called-interactively-p 'any) "" "non-")))

M-x interactive-test

  ==>

  Called interactively

M-: (interactive-test)

  ==>

  Called non-interactively


Now eval e.g. this:

(defadvice interactive-test (before foo activate)
  (sleep-for 1))

Then:

M-x interactive-test

  ==>

  Called non-interactively


This is quite surprising, and may cause surprising effects if the user
decides to advice such a command.

I found nothing in the doc about his behavior.  If this is not a bug,
maybe explain this behavior somewhere.


This last paragraph in the docstring of `called-interactively-p' seems
somewhat (but not directly) related:

| This function is meant for implementing advice and other
| function-modifying features.  Instead of using this, it is sometimes
| cleaner to give your function an extra optional argument whose
| `interactive' spec specifies non-nil unconditionally ("p" is a good
| way to do this), or via (not (or executing-kbd-macro noninteractive)). 

It seems a bit confusing - is there a "not" missing in the first
sentence?


Thanks,

Michael.


In GNU Emacs 24.1.50.1 (i486-pc-linux-gnu, GTK+ Version 3.4.2)
 of 2012-06-15 on zelenka, modified by Debian
 (emacs-snapshot package, version 2:20120615-1)
Windowing system distributor `The X.Org Foundation', version 11.0.11201902
Configured using:
 `configure '--build' 'i486-linux-gnu' '--host' 'i486-linux-gnu'
 '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib'
 '--localstatedir=/var' '--infodir=/usr/share/info'
 '--mandir=/usr/share/man' '--with-pop=yes'
 '--enable-locallisppath=/etc/emacs-snapshot:/etc/emacs:/usr/local/share/emacs/24.1.50/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/24.1.50/site-lisp:/usr/share/emacs/site-lisp'
 '--without-compress-info' '--with-crt-dir=/usr/lib/i386-linux-gnu/'
 '--with-x=yes' '--with-x-toolkit=gtk3' '--with-imagemagick=yes'
 'build_alias=i486-linux-gnu' 'host_alias=i486-linux-gnu'
 'CFLAGS=-DDEBIAN -DSITELOAD_PURESIZE_EXTRA=5000 -g -O2' 'LDFLAGS=-g
 -Wl,--as-needed -znocombreloc' 'CPPFLAGS=-D_FORTIFY_SOURCE=2''

Important settings:
  value of $LC_ALL: de_DE.utf8
  value of $LC_TIME: C
  value of $LANG: de_DE.utf8
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

bug#11747: 24.1.50; called-interactively-p vs. advice

[Glenn Morris]

Michael Heerdegen wrote:

> `called-interactively-p' returns wrong values when called inside
> commands that are adviced.

Old "interactive-p" had the same issue, though maybe that mattered less.
It's to do with the way advice is implemented. It actually goes out of
its way to avoid using call-interactively unless it has to (subrs).
See comments in advice.el header.

bug#11747: 24.1.50; called-interactively-p vs. advice

[Michael Heerdegen]

Glenn Morris <rgm@gnu.org> writes:

> > `called-interactively-p' returns wrong values when called inside
> > commands that are adviced.
>
> Old "interactive-p" had the same issue, though maybe that mattered less.
> It's to do with the way advice is implemented. It actually goes out of
> its way to avoid using call-interactively unless it has to (subrs).
> See comments in advice.el header.

So, do you mean this can't be fixed?  Too bad, but ok, that's what I
thought.

But then, we IMHO should improve the docstring of `called-interactively':

1.  This sentence:

   This function is meant for implementing advice and other
   function-modifying features.

Is there a "not" missing?

2.  Explicitly say that `called-interactively' will not work when used
in an adviced function.

This is something completely different than 1.  1. is about implementing
advice via `called-interactively'.  2. says that if you advice any
function that already uses it, it will stop working.  

So, my wish is to improve the docstring of `called-interactively' a bit.
Then, this report can be closed.


Michael.

bug#11747: 24.1.50; called-interactively-p vs. advice

[Glenn Morris]

Michael Heerdegen wrote:

> So, do you mean this can't be fixed? 

No, just that I personally don't see how to fix it. Hopefully someone
else does.

>    This function is meant for implementing advice and other
>    function-modifying features.
>
> Is there a "not" missing?

I'm not sure. I cannot tell what the sentence is supposed to mean under
either reading. I think it could just as well be removed.

> 2.  Explicitly say that `called-interactively' will not work when used
> in an adviced function.

Sure, if no-one fixes it.

bug#11747: 24.1.50; called-interactively-p vs. advice

[Stefan Monnier]

> | This function is meant for implementing advice and other
> | function-modifying features.  Instead of using this, it is sometimes
> | cleaner to give your function an extra optional argument whose
> | `interactive' spec specifies non-nil unconditionally ("p" is a good
> | way to do this), or via (not (or executing-kbd-macro noninteractive)). 
> It seems a bit confusing - is there a "not" missing in the first
> sentence?

No, called-interactively-p is indeed specifically useful when writing
advice (i.e. for use in the advice itself, not in the function that is
advised).
For normal functions, it is usually better (and always more reliable) to
provide an appropriate argument from the `interactive' spec, but doing
that in an advice is a lot more difficult, since it requires modifying
the `interactive' spec, which defadvice does not really let you do (it
lets you override it with another spec, but not modify the existing
spec).


        Stefan