bug#3984: 23.0.96; defadvice of call-interactively defeats interactive-p

bug#3984: 23.0.96; defadvice of call-interactively defeats interactive-p

[Drew Adams]

emacs -Q
 
Load (eval) this:
 
(defun foo ()
  (interactive)
  (if (interactive-p)
      (message "INT")
    (message "NOT")))
 
(global-set-key "\C-l" 'foo)
 
(defadvice call-interactively (after foo-advice disable activate)
  (message "AFTER"))
 
(ad-enable-advice 'call-interactively 'after 'foo-advice)
(ad-activate 'call-interactively)
 
Then hit `C-l'.
 
In *Messages*, you will see this:
 
NOT
AFTER
 
Even though `foo' is called interactively, `interactive-p' returns
nil.  Using `called-interactively-p' in place of `interactive-p' gives
the same thing. Same thing no matter how `foo' is called interactively
(e.g. M-x foo, M-: (call-interactively 'foo)).
 
Seems like a bug, but I realize that advising primitives is iffy, and
perhaps advising `call-interactively' is even more iffy.
 
However, advising `call-interactively' seems to work fine otherwise -
this is the only anomaly I've come across.
 
Can someone please explain why this happens, or how to work around it?
 

In GNU Emacs 23.0.96.1 (i386-mingw-nt5.1.2600)
 of 2009-07-09 on SOFT-MJASON
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (3.4)'
 

bug#3984: 23.0.96; defadvice of call-interactively defeats interactive-p

[Stefan Monnier]

> Can someone please explain why this happens, or how to work around it?
 
Because the implementation of interactive-p (and called-interactively-p)
is brittle: it looks at the latest stack frames to see the name of the
caller, so if you add things between the call to `call-interactively'
and the corresponding function call, it gets confused.

To work around it, don't use `interactive-p' and instead add an optional
argument (call it `interactive') to your function and pass it an
explicit non-nil value from the interactive spec.


        Stefan

bug#3984: 23.0.96; defadvice of call-interactively defeats interactive-p

[Drew Adams]

> > Can someone please explain why this happens, or how to work 
> around it?
>  
> Because the implementation of interactive-p (and 
> called-interactively-p)
> is brittle: it looks at the latest stack frames to see the name of the
> caller, so if you add things between the call to `call-interactively'
> and the corresponding function call, it gets confused.
> 
> To work around it, don't use `interactive-p' and instead add 
> an optional
> argument (call it `interactive') to your function and pass it an
> explicit non-nil value from the interactive spec.

Unfortunately, the code with `interactive-p' is not mine. The defadvice is mine,
but it needs to work for user functions, including those that call
`interactive-p' or `called-interactively-p'.

Is there no way the Emacs implementation could be fixed to handle this better?
Couldn't it take into consideration the `ad-*' stuff that results from advising
`call-interactively'? IOW, couldn't it look for `call-interactively' in its
advised form also?

bug#3984: 23.0.96; defadvice of call-interactively defeats interactive-p

[Stefan Monnier]

> Unfortunately, the code with `interactive-p' is not mine.
> The defadvice is mine, but it needs to work for user functions,
> including those that call `interactive-p' or `called-interactively-p'.

> Is there no way the Emacs implementation could be fixed to handle this
> better?  Couldn't it take into consideration the `ad-*' stuff that
> results from advising `call-interactively'?  IOW, couldn't it look for
> `call-interactively' in its advised form also?

There's probably some way to make it work, of course.  Note that the
same problem is likely to appear with other redefinitions of
call-interactively (e.g. profiling, tracing, ...).
Of course, another way to break these things is also to advise (and/or
profile/trace/...) interactive-p.


        Stefan

bug#3984: 23.0.96; defadvice of call-interactively defeats interactive-p

[Drew Adams]

> > Is there no way the Emacs implementation could be fixed to 
> > handle this better?  Couldn't it take into consideration
> > the `ad-*' stuff that results from advising
> > `call-interactively'?  IOW, couldn't it look for
> > `call-interactively' in its advised form also?
> 
> There's probably some way to make it work, of course.  Note that the
> same problem is likely to appear with other redefinitions of
> call-interactively (e.g. profiling, tracing, ...).

Yes.

> Of course, another way to break these things is also to advise (and/or
> profile/trace/...) interactive-p.

Yes.

Seems like there could be a list of such functions to check wrt advised (and
profiled, etc.) forms. The particular advice wouldn't matter, I believe; I
imagine that all that happens is that the function is currently checked.
Couldn't the check simply be membership in a list that includes the advised
names? Perhaps even have that as the value of a variable, which could be
configured (at least by Emacs dev, if not users).

Realistically, what do you think is the chance of at least the bug as reported
being fixed, that is, at least for advised `call-interactively'? I added a
feature to Icicles that I will likely need to remove if `interactive-p' is not
made to behave normally. Thx.

bug#3984: 23.0.96; defadvice of call-interactively defeats interactive-p

[Kai Tetzlaff]

I'm using icicles and just ran into this bug with emacs compiled from
bzr. The function count-words-region has been changed to use
called-interactively-p to check for interactive use. As reported, the
check fails when icicles is turned on. As there are >300 places under
the emacs lisp/ directory alone which use called-interactively-p, it
would really be great, if it could be made more robust.


In GNU Emacs 24.0.90.6 (x86_64-apple-darwin10.8.0, NS apple-appkit-1038.36)
 of 2011-10-09 on mack.tetzco.de
Windowing system distributor `Apple', version 10.3.1038
configured using `configure  'CC=clang' 'CFLAGS=-g -O0' 'LDFLAGS=-g' '--with-ns' '--with-gnutls''

bug#3984: 23.0.96; defadvice of call-interactively defeats interactive-p

[Drew Adams]

> From: Kai Tetzlaff Sent: Sunday, October 09, 2011 11:00 PM
> 
> I'm using icicles and just ran into this bug with emacs compiled from
> bzr. The function count-words-region has been changed to use
> called-interactively-p to check for interactive use. As reported, the
> check fails when icicles is turned on. As there are >300 places under
> the emacs lisp/ directory alone which use called-interactively-p, it
> would really be great, if it could be made more robust.

For info, this is the defadvice that exposes the `called-interactively' bug
(#3984):

(defadvice call-interactively (after icicle-save-to-history
                               disable activate)
  "Save command to `icicle-interactive-history'."
  ;; If command's input is not a parameterized (e.g. mouse)
  ;; event, record it.
  (let* ((fn   (ad-get-arg 0))
         (int  (interactive-form fn)))
    (when (and (symbolp fn)
               (consp int)
               (or (not (stringp (cadr int)))
                   (string= (cadr int) "")
                   (not (eq ?e (aref (cadr int) 0)))))
      (pushnew (symbol-name fn) icicle-interactive-history))))

This behavior is optional, so Icicles users can turn it off (it is off by
default) via option `icicle-populate-interactive-history-flag'.  And the doc
string of that option refers to Emacs bug #3948.  Still, it would be good to get
this Emacs bug fixed - especially, as Kai remarks, since the Emacs source code
now uses `called-interactively-p' all over the place.

bug#3984: 23.0.96; defadvice of call-interactively defeats interactive-p

[Stefan Monnier]

> string of that option refers to Emacs bug #3948.  Still, it would be good to get
> this Emacs bug fixed - especially, as Kai remarks, since the Emacs source code
> now uses `called-interactively-p' all over the place.

I don't understand the "now" in the above sentence.  AFAIK these uses
are not new (tho they used `interactive-p' in the past, but that should
not make any difference).


        Stefan

bug#3984: 23.0.96; defadvice of call-interactively defeats interactive-p

[Drew Adams]

> > string of that option refers to Emacs bug #3948.  Still, it 
> > would be good to get this Emacs bug fixed - especially, as
> > Kai remarks, since the Emacs source code now uses
> > `called-interactively-p' all over the place.
> 
> I don't understand the "now" in the above sentence.  AFAIK these uses
> are not new (tho they used `interactive-p' in the past, but 
> that should not make any difference).

Whether it matters, for this bug, whether the Emacs code uses
`called-interactively-p' or `interactive-p' I don't know.  I trust your guess
that it should not make any difference.  Feel free to remove "now" from the
sentence.  

The point is to please fix the bug.  Thx.

bug#3984: 23.0.96; defadvice of call-interactively defeats interactive-p

[Christopher Wellons]

Running into this bug with ido-ubiquitous and Emacs 24.3.1:

https://github.com/DarwinAwardWinner/ido-ubiquitous/issues/24

bug#3984: 23.0.96; defadvice of call-interactively defeats interactive-p

[Stefan Monnier]

> Running into this bug with ido-ubiquitous and Emacs 24.3.1:
> https://github.com/DarwinAwardWinner/ido-ubiquitous/issues/24

It's unlikely to get fixed, as you can guess from the fact that it hasn't
been fixed yet.

You might get better results using alternative approaches, based on
pre/post-command-hook or maybe advising some other function
(command-execute, for example).


        Stefan

bug#3984: Fix for #3984

[Ryan]

Dear all,

I "fixed" this in my ido-ubiquitous package by completely reimplementing 
"called-interactively-p" and "interactive-p" in pure elisp: 
https://github.com/DarwinAwardWinner/ido-ubiquitous/commit/f0c42e289a614071e22ad2c08297a7ebd60ba1cc

Apart from simply translating the C code in elisp, I made two key 
adjustments to the logic: first, I filter out all evidence of advice 
from the call stack before checking if the caller is 
"call-interactively". Second, I relax the definition of "caller is 
call-interactively" to include any symbol with the same 
"symbol-function" as call-interactively, or any function that is the 
same as the symbol-function of call-interactively. Combined, these 
adjustments mean that defining advice on call-interactively no longer 
results in erroneous return values from these two interactivity-testing 
functions.

I have implemented this in elisp because that is the only way to 
redefine functions in a running emacs session, but there's no reason 
that the C code couldn't be adapted to use the same logic.

-Ryan

bug#3984: Fix for #3984

[Stefan Monnier]

> I "fixed" this in my ido-ubiquitous package by completely reimplementing
> "called-interactively-p" and "interactive-p" in pure elisp:

FWIW, called-interactively-p is already implemented in Elisp in
Emacs's trunk.


        Stefan

bug#3984: Fix for #3984

[Ryan]

Looking at the code in trunk, I see that there is a special hook for 
functions to decide which stack frames to skip over when looking for 
call-interactively. I still think they should relax the test for 
equality to "equal indirect-functions" instead of exactly the symbol 
call-interactively.

On Fri Sep 13 06:18:50 2013, Stefan Monnier wrote:
>> I "fixed" this in my ido-ubiquitous package by completely reimplementing
>> "called-interactively-p" and "interactive-p" in pure elisp:
>
> FWIW, called-interactively-p is already implemented in Elisp in
> Emacs's trunk.
>
>
>          Stefan

bug#3984: Fix for #3984

[Ryan]

On Fri Sep 13 11:30:37 2013, Ryan wrote:
> Looking at the code in trunk, I see that there is a special hook for
> functions to decide which stack frames to skip over when looking for
> call-interactively. I still think they should relax the test for
> equality to "equal indirect-functions" instead of exactly the symbol
> call-interactively.

Actually, I just noticed that in trunk, nadvice.el adds a function to 
"called-interactively-p-functions" to skip advice-related stack frames, 
but this works only for advice on the interactive function, not advice 
defined on call-interactively itself. Furthermore, from my limited 
testing it appears that the structure of the call stack for advised 
functions has changes significantly in trunk, making my code obsolete. 
The whole thing looks like a work in progress right now.

bug#3984: Fix for #3984

[Stefan Monnier]

>> Looking at the code in trunk, I see that there is a special hook for
>> functions to decide which stack frames to skip over when looking for
>> call-interactively. I still think they should relax the test for
>> equality to "equal indirect-functions" instead of exactly the symbol
>> call-interactively.

The code does check "equal modulo indirect-functions" in some cases, but
indeed not all.  I don't think that replacing the equality check against
`call-interactively' with a check modulo indirect-functions would solve
your problem, tho (that only helps when calling though an alias of
call-interactively, but here the relevant stack frame will be a call to
the #<subr call-interactively> which is not
equal-modulo-indirect-functions to call-interactively since
call-interactively has been redefined to a different functions by the
advice).

You can probably use called-interactively-p-functions to detect the
#<subr call-interactively> and skip the frames between it and the
corresponding call to `call-interactively'.

But if you find a cute patch against the current code which makes it
work for you in a cleanish way, do send it here, to see if it can
be included.

> Actually, I just noticed that in trunk, nadvice.el adds a function to
> "called-interactively-p-functions" to skip advice-related stack frames, but
> this works only for advice on the interactive function, not advice defined
> on call-interactively itself.

Indeed.  It doesn't even work for all advices (more specifically it
doesn't work for :around advices, which means it doesn't work for
advices defined via `defadvice' since these all turn into one
big :around "new advice").

> Furthermore, from my limited testing it appears that the structure of
> the call stack for advised functions has changes significantly in
> trunk, making my code obsolete.

Indeed, the implementation of advices has been completely changed.

> The whole thing looks like a work in progress right now.

There's no planned change to it, so I consider it "ready modulo
bug-reports".  AFAIK it works "at least as well as before" (it works
better than before in the sense that Edebugging a function with calls
to called-interactively-p should now work correctly).

`called-interactively-p' is a big ugly hack and only works sometimes.
It can break in all kinds of cases (e.g. it currently won't work in
byte-compiled lexical-binding code within a `catch', or
a `condition-case', or the unwind part of an `unwind-protect').  Also,
the functions called (non-interactively, obviously) by your
`call-interactively' advice will probably think that they're called
interactively (hopefully your advice doesn't call many functions, and
hopefully none of them cares whether it's called interactively or not).


        Stefan

bug#3984: Fix for #3984

[Ryan]

Ok, I think I figured out how to do it for all advice, but feel free to 
poke holes in my idea before I get to far implementing it.

We have "ad-is-advised" which we can use to find which stack frames 
correspond to advised functions. We have "ad-get-orig-definition" which 
we can use to find the original definition of an advised function. We 
can tell if there is any active advice by testing if the advised 
function's definition is equal to the original definition. If the 
function has active advice, we can search down the stack for the 
original definition of that function and skip over all the stack frames 
in between.

Unfortunately, this requires a search through the call stack frames in 
the wrong order, so the best strategy may be to make a single pass 
through the call stack, finding all pairs of advised/original function 
stack frames, and then cache the result in a dynamically bound variable 
for the duration of the call to "called-interactively-p". This still 
requires a complete pass through the entire call stack for each call to 
"called-interactively-p", which defeats the current "lazy-evaluation" 
strategy of inspecting the frames one by one, although I guess we can 
stop at the first occurrence of "call-interactively" on the stack. But I 
doubt that "called-interactively-p" is a performance hotspot anyway, so 
this may be acceptable.

What do you think?

-Ryan


On 9/13/13 2:02 PM, Stefan Monnier wrote:
>>> Looking at the code in trunk, I see that there is a special hook for
>>> functions to decide which stack frames to skip over when looking for
>>> call-interactively. I still think they should relax the test for
>>> equality to "equal indirect-functions" instead of exactly the symbol
>>> call-interactively.
> The code does check "equal modulo indirect-functions" in some cases, but
> indeed not all.  I don't think that replacing the equality check against
> `call-interactively' with a check modulo indirect-functions would solve
> your problem, tho (that only helps when calling though an alias of
> call-interactively, but here the relevant stack frame will be a call to
> the #<subr call-interactively> which is not
> equal-modulo-indirect-functions to call-interactively since
> call-interactively has been redefined to a different functions by the
> advice).
>
> You can probably use called-interactively-p-functions to detect the
> #<subr call-interactively> and skip the frames between it and the
> corresponding call to `call-interactively'.
>
> But if you find a cute patch against the current code which makes it
> work for you in a cleanish way, do send it here, to see if it can
> be included.
>
>> Actually, I just noticed that in trunk, nadvice.el adds a function to
>> "called-interactively-p-functions" to skip advice-related stack frames, but
>> this works only for advice on the interactive function, not advice defined
>> on call-interactively itself.
> Indeed.  It doesn't even work for all advices (more specifically it
> doesn't work for :around advices, which means it doesn't work for
> advices defined via `defadvice' since these all turn into one
> big :around "new advice").
>
>> Furthermore, from my limited testing it appears that the structure of
>> the call stack for advised functions has changes significantly in
>> trunk, making my code obsolete.
> Indeed, the implementation of advices has been completely changed.
>
>> The whole thing looks like a work in progress right now.
> There's no planned change to it, so I consider it "ready modulo
> bug-reports".  AFAIK it works "at least as well as before" (it works
> better than before in the sense that Edebugging a function with calls
> to called-interactively-p should now work correctly).
>
> `called-interactively-p' is a big ugly hack and only works sometimes.
> It can break in all kinds of cases (e.g. it currently won't work in
> byte-compiled lexical-binding code within a `catch', or
> a `condition-case', or the unwind part of an `unwind-protect').  Also,
> the functions called (non-interactively, obviously) by your
> `call-interactively' advice will probably think that they're called
> interactively (hopefully your advice doesn't call many functions, and
> hopefully none of them cares whether it's called interactively or not).
>
>
>          Stefan

bug#3984: Fix for #3984

[Stefan Monnier]

> We have "ad-is-advised" which we can use to find which stack frames
> correspond to advised functions. We have "ad-get-orig-definition" which we
> can use to find the original definition of an advised function.

These are functions of advice.el, which is on the way out.  We need to
look at nadvice.el.

Thinking about it once more, I realize that your "advised
call-interactively" case might indeed be resolved by the current code,
except that it bumps into the problematic case of "deepest advice is
using :around".

Could you check if it is the case simply be doing a dummy (advice-add
'call-interactively :before #'ignore) before your defadvice?

If that works, then we can probably fix that problem in the following way:
- keep track of every function value (e.g. #<subr call-interactively> in
  our case) that is wrapped (at the deepest level) in an :around advice
  (e.g. in a hash-table).
- when walking up the stack, if we find such a function, look up the
  stack searching for the matching function symbol (just like we already
  do when encountering an internal advice element on the stack).


        Stefan

bug#3984:

[Ryan]

On 9/17/13 6:10 AM, Stefan Monnier wrote:
>> We have "ad-is-advised" which we can use to find which stack frames
>> correspond to advised functions. We have "ad-get-orig-definition" which we
>> can use to find the original definition of an advised function.
> These are functions of advice.el, which is on the way out.  We need to
> look at nadvice.el.
Wait, so defadvice and friends are going away entirely? Is everything in 
advice.el going away, or are some parts of it going to be converted to 
the new advice system? Is all this documented somewhere?
>
> Thinking about it once more, I realize that your "advised
> call-interactively" case might indeed be resolved by the current code,
> except that it bumps into the problematic case of "deepest advice is
> using :around".
>
> Could you check if it is the case simply be doing a dummy (advice-add
> 'call-interactively :before #'ignore) before your defadvice?
>
> If that works, then we can probably fix that problem in the following way:
> - keep track of every function value (e.g. #<subr call-interactively> in
>    our case) that is wrapped (at the deepest level) in an :around advice
>    (e.g. in a hash-table).
> - when walking up the stack, if we find such a function, look up the
>    stack searching for the matching function symbol (just like we already
>    do when encountering an internal advice element on the stack).
>
>
>          Stefan
I tried the before advice, and it doesn't seem to work. With the 
following code in emacs -Q:

(advice-add 'call-interactively :before #'ignore)
(defun myfun2 ()
   (interactive)
   (or (called-interactively-p)
       (error "Must be interactive!")))
(call-interactively 'myfun2)

I get the following stack trace:

Debugger entered--Lisp error: (error "Must be interactive!")
   signal(error ("Must be interactive!"))
   error("Must be interactive!")
   (or (called-interactively-p) (error "Must be interactive!"))
   myfun2()
   #<subr call-interactively>(myfun2)
   apply(#<subr call-interactively> myfun2)
   call-interactively(myfun2)
   eval-region(345 373 t (lambda (ignore) (goto-char 373) (quote 
(call-interactively (quote myfun2)))))  ; Reading at buffer position 345
   apply(eval-region (345 373 t (lambda (ignore) (goto-char 373) (quote 
(call-interactively (quote myfun2))))))
   eval-defun-2()
   eval-defun(nil)
   #<subr call-interactively>(eval-defun nil nil)
   apply(#<subr call-interactively> (eval-defun nil nil))
   call-interactively(eval-defun nil nil)
   command-execute(eval-defun)

Same thing happens when I do M-x myfun2.

bug#3984:

[Stefan Monnier]

>>> We have "ad-is-advised" which we can use to find which stack frames
>>> correspond to advised functions. We have "ad-get-orig-definition" which we
>>> can use to find the original definition of an advised function.
>> These are functions of advice.el, which is on the way out.  We need to
>> look at nadvice.el.
> Wait, so defadvice and friends are going away entirely?

It's very widely used, so it'll be around for a long while.  It's not
even marked obsolete officially yet.  But it is functionally obsoleted
by nadvice.

> Is everything in advice.el going away, or are some parts of it going
> to be converted to the new advice system?

It's already been changed to work on top of nadvice.el.

> Is all this documented somewhere?

No, only the internals were changed, and they're usually not documented,
other than in the form of actual code, and a few comments when you're lucky.

> I tried the before advice, and it doesn't seem to work. With the following
> code in emacs -Q:
> (advice-add 'call-interactively :before #'ignore)
> (defun myfun2 ()
>   (interactive)
>   (or (called-interactively-p)
>       (error "Must be interactive!")))
> (call-interactively 'myfun2)
> I get the following stack trace:

Hmm, indeed, that fails already.  It shouldn't be too hard to make
it work, tho.


        Stefan

bug#3984:

[Ryan]

Ok, I figured out how to walk down the stack, identify which frames are 
calls to advised functions, and for each call to an advised function, 
find the position in the stack of the call to the original function 
definition, using the functions in nadvice.el. First, we define:

(defun unadvised-function (func)
   "Return the original function definition of FUNC before it was advised."
   (let ((func (indirect-function func)))
     (while (advice--p func)
       (setq func (advice--cdr func)))
     func))

Then we scan down the stack starting from the very earliest function 
call, looking for functions that are advised (via "advice--p"). Every 
time we find ad advised function, we use get the original definition via 
unadvised-function and then search down the stack for a call to that 
original definition. Then we know to skip all those frames when 
searching for called-interactively. Specifically, we skip everything but 
the call to the outermost advice, since that call will bear the original 
name of the function.

Again, though, this requires a top-down non-lazy search of the stack, 
which is possible but seems to go against the intentions of the current 
implementation that checks frames one-by-one.

What do you think? I'd be happy to work on an implementation of this in 
the next few days if you think it's worth pursuing. But if so, I could 
use your input on the isue of the top-down search.

-Ryan

bug#3984:

[Ryan]

After reading and finally comprehending the definition of 
"advice--called-interactively-skip", I think I have a possible solution 
that doesn't require a top-down search, but it would require some minor 
rearchitecting of nadvice. Basically, once we know that a particular 
stack frame is a call to the innermost unadvised form of an advised 
function, it is relatively easy to walk up the stack looking for the 
outermost advice. This is what "advice--called-interactively-skip" does. 
(Although reading through it I don't see where the bug is that prevents 
it recognizing the before advice in my example.) The problem, then, is 
knowing when a function is advised, given only the unadvised form and a 
position in the stack. If we always unconditionally wrap an unadvised 
function with a function that we can easily identify, then we can easily 
check whether it has been advised. To that end, I propose the following:

;; Generate a private symbol
(defvar nadvice--indicator-symbol (make-symbol "nadvice--indicator-symbol"))
(defun wrap-function-in-indicator-lambda (func)
   `(lambda (&rest args)
      ,nadvice--indicator-symbol
      (apply ,func args)))
(defun indicator-lambda-p (func)
   (eq nadvice--indicator-symbol
     (nth 2 (wrap-function-in-indicator-lambda (indirect-function func)))))

If all advised functions are wrapped by a call to the above function 
"wrap-function-in-indicator-lambda", then when they are called, the call 
to the "indicator lambda" would always be 2 frames up from the call to 
the original unadvised function. This allows us to easily recognize an 
advised function on the stack by testing the function 2 frames up with 
"indicator-lambda-p". Once we know the function is advised, we can then 
initiate the search for the outermost advice's stack position.

In order to implement this, I think "advice-add" and "advice-remove" 
need to be modified to automatically wrap/unwrap the original function 
in/out of the indicator.
What do you think of this? Obviously my "indicator-lambda" could be 
replaced by e.g. a no-op before/after advice or something similar, which 
would have the same effect of making it easy to recognize the innermost 
call of an advised function based on the contents of specific stack 
frame positions above it.

What do you think of this?

-Ryan

bug#3984:

[Stefan Monnier]

> advice.  This is what "advice--called-interactively-skip" does.  (Although
> reading through it I don't see where the bug is that prevents it recognizing
> the before advice in my example.)

Exactly.  I think we need to fix this problem.  It really should work.

> If all advised functions are wrapped by a call to the above function
> "wrap-function-in-indicator-lambda",

All functions advised with a non-:around advice have such
a "recognizable wrapper"; and that's indeed what
advice--called-interactively-skip checks in

          (and (eq (nth 1 frame2) 'apply)
               (progn
                 (funcall get-next-frame)
                 (advice--p (indirect-function (nth 1 frame2)))))

IOW the nadvice.el machinery is itself the recognizable wrapper.
Not sure why this fails in your test case, tho.

For :around advices, the machinery does not provide a recognizable
wrapper, so we might want to add an ad-hoc wrapper like you suggest for
those cases, tho maybe we can avoid the cost of such a wrapper, by
keeping the so-advised inner-functions in a hash-table (so we can still
recognize them, even tho they're not tagged directly in the backtrace).

So, I think the first thing is to figure out why your test case fails.


        Stefan

bug#3984:

[Ryan]

Ok, I'll work on debugging my test case tomorrow.

On Wed Sep 18 20:38:48 2013, Stefan Monnier wrote:
>> advice.  This is what "advice--called-interactively-skip" does.  (Although
>> reading through it I don't see where the bug is that prevents it recognizing
>> the before advice in my example.)
>
> Exactly.  I think we need to fix this problem.  It really should work.
>
>> If all advised functions are wrapped by a call to the above function
>> "wrap-function-in-indicator-lambda",
>
> All functions advised with a non-:around advice have such
> a "recognizable wrapper"; and that's indeed what
> advice--called-interactively-skip checks in
>
>            (and (eq (nth 1 frame2) 'apply)
>                 (progn
>                   (funcall get-next-frame)
>                   (advice--p (indirect-function (nth 1 frame2)))))
>
> IOW the nadvice.el machinery is itself the recognizable wrapper.
> Not sure why this fails in your test case, tho.
>
> For :around advices, the machinery does not provide a recognizable
> wrapper, so we might want to add an ad-hoc wrapper like you suggest for
> those cases, tho maybe we can avoid the cost of such a wrapper, by
> keeping the so-advised inner-functions in a hash-table (so we can still
> recognize them, even tho they're not tagged directly in the backtrace).
>
> So, I think the first thing is to figure out why your test case fails.
>
>
>          Stefan

bug#3984:

[Ryan]

On a related note, I just noticed that the 
"advice-test-called-interactively-p" test in 
test/automated/advice-tests.el happens to pass, but only because it 
doesn't use "called-interactively-p" inside the original function, but 
rather only inside the advice itself. Also, it doesn't test advising 
"call-interactively" itself. I think I will see about writing a proper 
test first, and then use that to start debugging.

On Thu Sep 19 01:06:03 2013, Ryan wrote:
> Ok, I'll work on debugging my test case tomorrow.
>
> On Wed Sep 18 20:38:48 2013, Stefan Monnier wrote:
>>> advice.  This is what "advice--called-interactively-skip" does.
>>> (Although
>>> reading through it I don't see where the bug is that prevents it
>>> recognizing
>>> the before advice in my example.)
>>
>> Exactly.  I think we need to fix this problem.  It really should work.
>>
>>> If all advised functions are wrapped by a call to the above function
>>> "wrap-function-in-indicator-lambda",
>>
>> All functions advised with a non-:around advice have such
>> a "recognizable wrapper"; and that's indeed what
>> advice--called-interactively-skip checks in
>>
>>            (and (eq (nth 1 frame2) 'apply)
>>                 (progn
>>                   (funcall get-next-frame)
>>                   (advice--p (indirect-function (nth 1 frame2)))))
>>
>> IOW the nadvice.el machinery is itself the recognizable wrapper.
>> Not sure why this fails in your test case, tho.
>>
>> For :around advices, the machinery does not provide a recognizable
>> wrapper, so we might want to add an ad-hoc wrapper like you suggest for
>> those cases, tho maybe we can avoid the cost of such a wrapper, by
>> keeping the so-advised inner-functions in a hash-table (so we can still
>> recognize them, even tho they're not tagged directly in the backtrace).
>>
>> So, I think the first thing is to figure out why your test case fails.
>>
>>
>>          Stefan

bug#3984:

[Stefan Monnier]

> "advice-test-called-interactively-p" test in test/automated/advice-tests.el
> happens to pass, but only because it doesn't use "called-interactively-p"
> inside the original function, but rather only inside the advice
> itself.

Indeed.  And it a very important case: the main definition of a function
can be modified to not use called-interactively-p (add an argument which
is set to a non-nil value in the interactive spec), but that's not
really an option for an advice that requires to know if the function was
called interactively (strictly speaking, it can be done, but it's a lot
more trouble).


        Stefan

bug#3984:

[Ryan]

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

On Thu Sep 19 12:23:58 2013, Ryan wrote:
>
> On a related note, I just noticed that the
> "advice-test-called-interactively-p" test in
> test/automated/advice-tests.el happens to pass, but only because it
> doesn't use "called-interactively-p" inside the original function, but
> rather only inside the advice itself. Also, it doesn't test advising
> "call-interactively" itself. I think I will see about writing a proper
> test first, and then use that to start debugging.
>
Ok, I have written a couple of tests, two of which are currently failing 
(mine are the ones with numbered suffixes 2 through 5). I am attaching a 
patch that adds these tests. ERT output is below:

$ open .
techne:emacs-trunk ryan$ src/emacs -batch -Q -l ert -l 
test/automated/advice-tests.el -f ert-run-tests-batch-and-exit
Running 10 tests (2013-09-19 14:56:00-0700)
    passed   1/10  advice-test-called-interactively-p
    failed   2/10  advice-test-called-interactively-p-2
    passed   3/10  advice-test-called-interactively-p-3
    failed   4/10  advice-test-called-interactively-p-4
    passed   5/10  advice-test-called-interactively-p-5
    passed   6/10  advice-test-interactive
    passed   7/10  advice-test-preactivate
ad-handle-definition: `sm-test2' got redefined
ad-handle-definition: `sm-test4' got redefined
    passed   8/10  advice-tests-advice
ad-handle-definition: `sm-test5' got redefined
    passed   9/10  advice-tests-combination
    passed  10/10  advice-tests-nadvice

Ran 10 tests, 10 results as expected (2013-09-19 14:56:00-0700)
2 expected failures

[-- Attachment #2: more-advice-interactive-tests.diff --]
[-- Type: text/plain, Size: 3609 bytes --]

diff --git a/test/automated/advice-tests.el b/test/automated/advice-tests.el
index 69c15e3..65577ad 100644
--- a/test/automated/advice-tests.el
+++ b/test/automated/advice-tests.el
@@ -23,6 +23,21 @@
 
 (require 'ert)
 
+(defun clear-advice (symbol)
+  "Reset SYMBOL's function to its original unadvised definition."
+  (let ((func (symbol-function symbol)))
+    (while (advice--p func)
+      (setq func (advice--cdr func)))
+    (fset symbol func)))
+
+(defmacro post-restore-func (func &rest body)
+  (let ((fdef (symbol-function func)))
+    `(unwind-protect
+	 (progn ,@body)
+       (fset ',func ,fdef))))
+(put 'post-restore-func 'lisp-indent-function
+     (get 'prog1 'lisp-indent-function))
+
 (ert-deftest advice-tests-nadvice ()
   "Test nadvice code."
   (defun sm-test1 (x) (+ x 4))
@@ -113,6 +128,60 @@
                 (cons (cons 2 (called-interactively-p)) (apply f args))))
   (should (equal (call-interactively 'sm-test7) '((2 . t) (1 . t) 11))))
 
+(ert-deftest advice-test-called-interactively-p-2 ()
+  "Check interaction between around advice and called-interactively-p.
+
+This tests the currently broken case of the innermost advice to a
+function being an around advice."
+  :expected-result :failed
+  (defun sm-test7.2 () (interactive) (cons 1 (called-interactively-p)))
+  (clear-advice 'sm-test7.2)
+  (advice-add 'sm-test7.2 :around
+              (lambda (f &rest args)
+                (list (cons 1 (called-interactively-p)) (apply f args))))
+  (advice-add 'sm-test7.2 :before #'ignore)
+  (advice-add 'sm-test7.2 :after #'ignore)
+  ;(advice-add 'sm-test7.2 :filter-args #'list)
+  ;(advice-add 'sm-test7.2 :filter-return #'identity)
+  (should (equal (sm-test7.2) '((1 . nil) (1 . nil))))
+  (should (equal (call-interactively 'sm-test7.2) '((1 . t) (1 . t)))))
+
+(ert-deftest advice-test-called-interactively-p-3 ()
+  "Check interaction between before advice and called-interactively-p.
+
+This tests the case of the innermost advice being before"
+  (defun sm-test7.3 () (interactive) (cons 1 (called-interactively-p)))
+  (advice-add 'sm-test7.3 :before #'ignore)
+  ;(advice-add 'sm-test7.3 :filter-args #'list)
+  ;(advice-add 'sm-test7.3 :filter-return #'identity)
+  (should (equal (sm-test7.3) '(1 . nil)))
+  (should (equal (call-interactively 'sm-test7.3) '(1 . t))))
+
+(ert-deftest advice-test-called-interactively-p-4 ()
+  "Check interaction between advice on call-interactively and called-interactively-p.
+
+This tests the case where call-interactively itself is advised,
+which is currently broken."
+  :expected-result :failed
+  (defun sm-test7.4 () (interactive) (cons 1 (called-interactively-p)))
+  (post-restore-func call-interactively
+    (advice-add 'call-interactively :before #'ignore)
+    (should (equal (sm-test7.4) '(1 . nil)))
+    (should (equal (call-interactively 'sm-test7.4) '(1 . t)))))
+
+(ert-deftest advice-test-called-interactively-p-5 ()
+  "Check interaction between non-innermost around advice and called-interactively-p.
+
+This tests the case where a function has around advice, but it is
+not the innermost advice."
+  (defun sm-test7.5 () (interactive) (cons 1 (called-interactively-p)))
+  (advice-add 'sm-test7.5 :before #'ignore)
+  (advice-add 'sm-test7.5 :around #'funcall)
+  ;(advice-add 'sm-test7.5 :filter-args #'list)
+  ;(advice-add 'sm-test7.5 :filter-return #'identity)
+  (should (equal (sm-test7.5) '(1 . nil)))
+  (should (equal (call-interactively 'sm-test7.5) '(1 . t))))
+
 (ert-deftest advice-test-interactive ()
   "Check handling of interactive spec."
   (defun sm-test8 (a) (interactive "p") a)

bug#3984:

[Ryan]

After a little more consideration, I think I know what the bug is. The 
goal of "advice--called-interactively-skip" is to skip all the 
advice-related stack frames of the called function, not advice-related 
stack frames for advice on call-interactively.

I also came up with an idea to address this issue: a second function 
added to called-interactively-p-functions that checks the current stack 
frame for equality to the unadvised form of call-interactively and if it 
finds it, skips past all the advice frames to the outermost call. This 
will make it so that advising call-interactively does not affect the 
return of called-interactively-p, but it will not fix the "innermost 
around advice" problem.

I'm trying to code up a solution now. I'll eventually find my way 
through the twisty maze of fencepost errors.

-Ryan

bug#3984: Fix case where call-interactively is advised

[Ryan]

[-- Attachment #1: Type: text/plain, Size: 1105 bytes --]

Here is a patch that applies over my previous patch and fixes 
"advice-test-called-interactively-p-4", which tests whether 
called-interactively-p returns the correct result when 
call-interactively is advised.

On 9/19/13 9:23 PM, Ryan wrote:
> After a little more consideration, I think I know what the bug is. The 
> goal of "advice--called-interactively-skip" is to skip all the 
> advice-related stack frames of the called function, not advice-related 
> stack frames for advice on call-interactively.
>
> I also came up with an idea to address this issue: a second function 
> added to called-interactively-p-functions that checks the current 
> stack frame for equality to the unadvised form of call-interactively 
> and if it finds it, skips past all the advice frames to the outermost 
> call. This will make it so that advising call-interactively does not 
> affect the return of called-interactively-p, but it will not fix the 
> "innermost around advice" problem.
>
> I'm trying to code up a solution now. I'll eventually find my way 
> through the twisty maze of fencepost errors.
>
> -Ryan


[-- Attachment #2: fix-advised-call-interactively.diff --]
[-- Type: text/plain, Size: 2809 bytes --]

diff --git a/lisp/emacs-lisp/nadvice.el b/lisp/emacs-lisp/nadvice.el
index 8b149aa..2dab864 100644
--- a/lisp/emacs-lisp/nadvice.el
+++ b/lisp/emacs-lisp/nadvice.el
@@ -461,6 +461,36 @@ of the piece of advice."
             (funcall get-next-frame))))
       (- i origi 1))))
 
+(defun advice-unadvised-form (func)
+  "Return the definition of FUNC with all advice stripped.
+
+FUNC may be a function definition or a symbol naming a function."
+  (let ((func (indirect-function func)))
+    (while (advice--p func)
+      (setq func (advice--cdr func)))
+    func))
+
+;; When `call-interactively' is advised, called-interactively-p needs
+;; to be taught to skip the advising frames.
+(add-hook 'called-interactively-p-functions
+          #'advice--advised-called-interactively-skip)
+(defun advice--advised-called-interactively-skip (origi frame1 frame2)
+  (when (and frame2
+             (not (eq (nth 1 frame2) 'call-interactively))
+             (eq (advice-unadvised-form 'call-interactively)
+                 (indirect-function (nth 1 frame2))))
+    ;; Skip until frame2 is a call to the symbol call-interactively.
+    (let* ((i origi)
+           (get-next-frame
+            (lambda ()
+              (setq frame1 frame2)
+              (setq frame2 (internal--called-interactively-p--get-frame i))
+              (setq i (1+ i)))))
+      (funcall get-next-frame)
+      (while (and frame2
+                  (not (eq (nth 1 frame2) 'call-interactively)))
+        (funcall get-next-frame))
+      (- i origi 1))))
 
 (provide 'nadvice)
 ;;; nadvice.el ends here
diff --git a/lisp/subr.el b/lisp/subr.el
index 75c6b3a..b5f682a 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -4272,8 +4272,6 @@ command is called from a keyboard macro?"
                  (_ (setq i (+ i skip -1)) (funcall get-next-frame)))))))
       ;; Now `frame' should be "the function from which we were called".
       (pcase (cons frame nextframe)
-        ;; No subr calls `interactive-p', so we can rule that out.
-        (`((,_ ,(pred (lambda (f) (subrp (indirect-function f)))) . ,_) . ,_) nil)
         ;; Somehow, I sometimes got `command-execute' rather than
         ;; `call-interactively' on my stacktrace !?
         ;;(`(,_ . (t command-execute . ,_)) t)
diff --git a/test/automated/advice-tests.el b/test/automated/advice-tests.el
index c8f06e5..01be3ab 100644
--- a/test/automated/advice-tests.el
+++ b/test/automated/advice-tests.el
@@ -163,7 +163,6 @@ This tests the case of the innermost advice being before"
 
 This tests the case where call-interactively itself is advised,
 which is currently broken."
-  :expected-result :failed
   (defun sm-test7.4 () (interactive) (cons 1 (called-interactively-p)))
   (post-restore-func call-interactively
     (advice-add 'call-interactively :before #'ignore)

bug#3984:

[Ryan]

On a related note, what is the proper way to test for the new advice 
system? (featurep 'nadvice)?

Anyway, now that we have a fix for when call-interactively is advised, 
what do you want to do about the case where the called function is 
advised with the innermost advice being around? The only problem we 
really need to solve is being absolutely whether or not a given stack 
frame is the innermost function in an advice stack. The two options that 
I can see are a complete top-down search or modifying around advice so 
that it gives a consistent signature (e.g. by wrapping the advised 
function in a special lambda.

-Ryan

bug#3984:

[Stefan Monnier]

> After a little more consideration, I think I know what the bug is. The goal
> of "advice--called-interactively-skip" is to skip all the advice-related
> stack frames of the called function, not advice-related stack frames for
> advice on call-interactively.

Oh, right.  How 'bout the patch below?


        Stefan


=== modified file 'lisp/subr.el'
--- lisp/subr.el	2013-09-18 03:50:18 +0000
+++ lisp/subr.el	2013-09-20 14:34:03 +0000
@@ -4246,6 +4246,8 @@
 if those frames don't seem special and otherwise, it should return
 the number of frames to skip (minus 1).")
 
+(defconst internal--call-interactively (symbol-function 'call-interactively))
+
 (defun called-interactively-p (&optional kind)
   "Return t if the containing function was called by `call-interactively'.
 If KIND is `interactive', then only return t if the call was made
@@ -4321,7 +4323,9 @@
         ;; Somehow, I sometimes got `command-execute' rather than
         ;; `call-interactively' on my stacktrace !?
         ;;(`(,_ . (t command-execute . ,_)) t)
-        (`(,_ . (t call-interactively . ,_)) t)))))
+        (`(,_ . (t ,(or `call-interactively
+                        (pred (eq internal--call-interactively))) . ,_))
+         t)))))
 
 (defun interactive-p ()
   "Return t if the containing function was run directly by user input.

bug#3984:

[Ryan]

On 9/20/13 7:35 AM, Stefan Monnier wrote:
>> After a little more consideration, I think I know what the bug is. The goal
>> of "advice--called-interactively-skip" is to skip all the advice-related
>> stack frames of the called function, not advice-related stack frames for
>> advice on call-interactively.
> Oh, right.  How 'bout the patch below?
>
>
>          Stefan
>
>
> === modified file 'lisp/subr.el'
> --- lisp/subr.el	2013-09-18 03:50:18 +0000
> +++ lisp/subr.el	2013-09-20 14:34:03 +0000
> @@ -4246,6 +4246,8 @@
>   if those frames don't seem special and otherwise, it should return
>   the number of frames to skip (minus 1).")
>   
> +(defconst internal--call-interactively (symbol-function 'call-interactively))
> +
>   (defun called-interactively-p (&optional kind)
>     "Return t if the containing function was called by `call-interactively'.
>   If KIND is `interactive', then only return t if the call was made
> @@ -4321,7 +4323,9 @@
>           ;; Somehow, I sometimes got `command-execute' rather than
>           ;; `call-interactively' on my stacktrace !?
>           ;;(`(,_ . (t command-execute . ,_)) t)
> -        (`(,_ . (t call-interactively . ,_)) t)))))
> +        (`(,_ . (t ,(or `call-interactively
> +                        (pred (eq internal--call-interactively))) . ,_))
> +         t)))))
>   
>   (defun interactive-p ()
>     "Return t if the containing function was run directly by user input.
>
That would certainly work, assuming that subr.el is always loaded before 
nadvice.el (so call-interactively could not possibly be advised yet), 
which seems likely to be true since subr.el contains so many core functions.

Since this bug report is just about advising call-interactively, which 
we now have a fix for, should I open a separate bug about the around 
advice?

bug#3984:

[Ryan]

I confirm that your simple patch to subr.el passes the test that I 
provided.

bug#3984:

[Stefan Monnier]

> Ok, I have written a couple of tests, two of which are currently failing
> (mine are the ones with numbered suffixes 2 through 5). I am
> attaching a patch that adds these tests. ERT output is below:

Thanks.  Just a few questions, before installing the patch.

> +(defun clear-advice (symbol)
> +  "Reset SYMBOL's function to its original unadvised definition."
> +  (let ((func (symbol-function symbol)))
> +    (while (advice--p func)
> +      (setq func (advice--cdr func)))
> +    (fset symbol func)))

Why do you need that?  AFAICT you only use it after defining sm-test7.2,
and I don't see why that call is needed.

> +(defmacro post-restore-func (func &rest body)
> +  (let ((fdef (symbol-function func)))
> +    `(unwind-protect
> +	 (progn ,@body)
> +       (fset ',func ,fdef))))
> +(put 'post-restore-func 'lisp-indent-function
> +     (get 'prog1 'lisp-indent-function))

Since it's only used once, I don't think it's worth defining a macro for it.


        Stefan

bug#3984:

[Ryan]

>> Ok, I have written a couple of tests, two of which are currently failing
>> (mine are the ones with numbered suffixes 2 through 5). I am
>> attaching a patch that adds these tests. ERT output is below:
> Thanks.  Just a few questions, before installing the patch.
>
>> +(defun clear-advice (symbol)
>> +  "Reset SYMBOL's function to its original unadvised definition."
>> +  (let ((func (symbol-function symbol)))
>> +    (while (advice--p func)
>> +      (setq func (advice--cdr func)))
>> +    (fset symbol func)))
> Why do you need that?  AFAICT you only use it after defining sm-test7.2,
> and I don't see why that call is needed.
Oh, that's actually not necessary. I put it in so I could re-run the 
test without restarting emacs or manually resetting the advice, and I 
forgot to remove it.

>> +(defmacro post-restore-func (func &rest body)
>> +  (let ((fdef (symbol-function func)))
>> +    `(unwind-protect
>> +	 (progn ,@body)
>> +       (fset ',func ,fdef))))
>> +(put 'post-restore-func 'lisp-indent-function
>> +     (get 'prog1 'lisp-indent-function))
> Since it's only used once, I don't think it's worth defining a macro for it.

Probably true. I wasn't sure how much I would end up using it, and it 
seemed like a potentially useful macro to have lying around when testing 
advice that might break core functions of emacs.

Anyway, I wasn't very creative with my test names or function names, and 
I'd be happy to submit a new patch with more informative names (as well 
as the above changes). Is there a naming convention for tests/functions 
that I should adhere to?

bug#3984:

[Stefan Monnier]

> Oh, that's actually not necessary. I put it in so I could re-run the test
> without restarting emacs or manually resetting the advice, and I forgot to
> remove it.

OK, removed.

>> Since it's only used once, I don't think it's worth defining a macro for it.
> Probably true. I wasn't sure how much I would end up using it, and it seemed
> like a potentially useful macro to have lying around when testing advice
> that might break core functions of emacs.

Makes sense as well, but let's postpone putting it in until we do end
up using it repeatedly.

I installed some tests based on your patch, as well as my quick fix in
subr.el.

> That would certainly work, assuming that subr.el is always loaded before
> nadvice.el (so call-interactively could not possibly be advised yet), which
> seems likely to be true since subr.el contains so many core functions.

Since subr.el is both preloaded and loaded very early, that should be
safe, yes.

> Since this bug report is just about advising call-interactively, which we
> now have a fix for, should I open a separate bug about the around advice?

Sure.  Note also that in the tests I installed, I added one that
demonstrates a problem with :filter-args as well (because :filter-args
uses `funcall' rather than `apply', so it doesn't match the pattern
expected by advice--called-interactively-skip).

By the way, I don't see your name in our copyright assignment database.
I installed your tests under "tiny change", but if you intend to
contribute more, we'd need you to sign some paperwork.  Would you be
willing to do that?


        Stefan

bug#3984: bug#123: Potential fix

[Ryan]

Dear all,

I "fixed" this in my ido-ubiquitous package by completely reimplementing 
"called-interactively-p" and "interactive-p" in pure elisp: 
https://github.com/DarwinAwardWinner/ido-ubiquitous/commit/f0c42e289a614071e22ad2c08297a7ebd60ba1cc

Apart from simply translating the C code in elisp, I made two key 
adjustments to the logic: first, I filter out all evidence of advice 
from the call stack before checking if the caller is 
"call-interactively". Second, I relax the definition of "caller is 
call-interactively" to include any symbol with the same 
"symbol-function" as call-interactively, or any function that is the 
same as the symbol-function of call-interactively. Combined, these 
adjustments mean that defining advice on call-interactively no longer 
results in erroneous return values from these two interactivity-testing 
functions.

I have implemented this in elisp because that is the only way to 
redefine functions in a running emacs session, but there's no reason 
that the C code couldn't be adapted to use the same logic.

-Ryan