bug#18399: 24.4.50; nadvice :filter-args -vs- interactive

bug#18399: 24.4.50; nadvice :filter-args -vs- interactive

[Tom Tromey]

I wrote this function to advise vc-dir, to make it default to starting
at the project root.  It's largely copied from vc-dir, then slightly
hacked:

(defun tromey-vc-dir (dir &optional backend)
  (interactive
   (list
    ;; When you hit C-x v d in a visited VC file,
    ;; the *vc-dir* buffer visits the directory under its truename;
    ;; therefore it makes sense to always do that.
    ;; Otherwise if you do C-x v d -> C-x C-f -> C-c v d
    ;; you may get a new *vc-dir* buffer, different from the original
    (let ((def-dir (or (if (fboundp 'vc-root-dir) (vc-root-dir))
		       default-directory)))
      (file-truename (read-directory-name "VC status for directory: "
					  def-dir def-dir t nil)))
    (if current-prefix-arg
	(intern
	 (completing-read
	  "Use VC backend: "
	  (mapcar (lambda (b) (list (symbol-name b)))
		  vc-handled-backends)
	  nil t nil nil)))))
  (list dir backend))

(require 'vc-dir)
(advice-add #'vc-dir :filter-args #'tromey-vc-dir)


If I now use M-x vc-dir, I get an error:

vc-responsible-backend: Wrong type argument: stringp, ("/home/tromey/Emacs/trunk/" nil)


However, if I invoke M-x tromey-vc-dir directly (and hack in a little
bit of code so I can easily see the result), it works properly.

Using that hack and then invoking vc-dir shows that the result of
tromey-vc-dir in the advised-and-interactive case is:

(("/home/tromey/Emacs/trunk/" nil) nil)

... which is wrong.

So I think there must be a bug with how nadvice handles interactive
specs, either generally or with :filter-args.





In GNU Emacs 24.4.50.4 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.9)
 of 2014-08-29 on bapiya
Repository revision: 117768 dmantipov@yandex.ru-20140829122330-7g7qcbxsy64afin0
Windowing system distributor `Fedora Project', version 11.0.11404000
Configured using:
 `configure --prefix=/home/tromey/Emacs/install'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=none
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  diff-auto-refine-mode: t
  shell-dirtrack-mode: t
  helm-match-plugin-mode: t
  helm-occur-match-plugin-mode: t
  eldoc-mode: t
  which-function-mode: t
  global-auto-revert-mode: t
  erc-services-mode: t
  erc-list-mode: t
  erc-menu-mode: t
  erc-autojoin-mode: t
  erc-ring-mode: t
  erc-networks-mode: t
  erc-pcomplete-mode: t
  erc-track-mode: t
  erc-match-mode: t
  erc-button-mode: t
  erc-fill-mode: t
  erc-stamp-mode: t
  erc-netsplit-mode: t
  erc-irccontrols-mode: t
  erc-noncommands-mode: t
  erc-move-to-prompt-mode: t
  erc-readonly-mode: t
  savehist-mode: t
  tooltip-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  auto-fill-function: do-auto-fill

Recent input:
- f u n c i t o n SPC ' C-u C-b C-b C-t C-e ) C-j C-x 
o C-g C-p C-e C-j C-x o C-g C-p C-e C-j C-f C-f C-] 
C-] C-p C-p C-p C-e C-j C-] C-z n C-x 1 C-s a r i t 
y C-s C-x C-f <M-backspace> <M-backspace> <M-backspace> 
<M-backspace> <M-backspace> <M-backspace> <M-backspace> 
<M-backspace> <M-backspace> t r <tab> l i s <tab> e 
m a c s - l i s <tab> n a d <tab> <return> M-x l g 
r e p <return> a r i t y <return> <return> <return> 
C-x o C-x 1 C-u C-u C-n C-u C-n C-u C-n C-c C-c C-x 
1 M-< C-x k <return> C-u C-p C-p C-p C-p C-c C-c C-x 
1 C-l C-v C-l C-s b y t e - C-w C-w C-r C-r C-r C-a 
C-u C-u C-n C-l C-z o C-p M-d M-d C-e <backspace> C-j 
C-u C-u C-p C-M-f C-M-b M-f C-M-f C-M-f C-M-b C-M-f 
C-M-f C-M-f C-M-f C-M-b C-M-b C-M-b C-M-b C-M-b C-a 
C-s i n t e r a c t i v e C-g C-g C-g C-s d i r C-a 
M-v C-z n C-x b . e m <tab> <return> C-x v v C-u C-n 
C-u C-n M-g d i r <return> d e f - d i r <return> y 
n n n n n y y q C-u C-p C-u C-p C-p C-p TAB C-n TAB 
C-n TAB C-n TAB C-x C-s C-x v v r e n a m e SPC v a 
r i a b l e SPC i n SPC t r o m e y - v - <backspace> 
c - d i r C-c C-c C-z n M-x r e p o r t - e m <tab> 
<return>

Recent messages:
Mark saved where search started
Checking out /home/tromey/Emacs/COPY/.emacs...done
Mark set
Replaced 3 occurrences
Saving file /home/tromey/Emacs/COPY/.emacs...
Wrote /home/tromey/Emacs/COPY/.emacs
Mark set
Press C-c C-c when you are done editing.
Enter a change comment.  Type C-c C-c when done
Checking in /home/tromey/Emacs/COPY/.emacs...done

Load-path shadows:
/home/tromey/.emacs.d/elpa/css-mode-1.0/css-mode hides /home/tromey/Emacs/install/share/emacs/24.4.50/lisp/textmodes/css-mode
/home/tromey/.emacs.d/elpa/bubbles-0.5/bubbles hides /home/tromey/Emacs/install/share/emacs/24.4.50/lisp/play/bubbles

Features:
(emacsbug log-edit pcvs-util smerge-mode bug-reference goto-addr
image-file dabbrev bbdb-sc supercite regi gnus-draft xscheme unsafep
trace testcover shadow scheme re-builder profiler pcase inf-lisp ielm
ert debug elp edebug cl-indent checkdoc cus-edit copyright gnus-fun
mailalias mail-hist nnir shr-color color diff-mode easy-mmode idutils
derived cc-langs cc-mode cc-fonts cc-guess cc-menus cc-cmds ido
helm-mode helm-files rx image-dired tramp tramp-compat tramp-loaddefs
trampver shell dired-x dired-aux ffap helm-buffers helm-elscreen
helm-tags helm-bookmark helm-adaptive helm-info helm-net helm-plugin
bookmark helm-locate helm-help helm-match-plugin helm-grep helm-regexp
helm-external helm-utils helm cl-macs eieio-opt speedbar sb-image
ezimage dframe strokes help-mode shr flow-fill mm-archive gnus-html
browse-url xml url-cache mm-url url url-proxy url-privacy url-expand
url-methods url-history url-cookie url-domsuf url-util url-parse
url-vars bbdb-gui bbdb-hooks mule-util sort smiley gnus-cite gnus-async
gnus-bcklg qp gnus-ml disp-table gnus-topic nndraft nnmh nnfolder utf-7
bbdb-gnus bbdb-snarf mail-extr bbdb-com warnings cl gv gnutls
network-stream starttls gnus-agent gnus-srvr gnus-score score-mode
nnvirtual gnus-msg nntp gnus-cache gnus-registry registry eieio-base
gnus-art mm-uu mml2015 epg-config mm-view mml-smime smime dig mailcap
gnus-sum gnus-group gnus-undo smtpmail sendmail gnus-start gnus-cloud
nnimap nnmail mail-source tls utf7 netrc nnoo parse-time gnus-spec
gnus-int gnus-range message dired rfc822 mml mml-sec mm-decode mm-bodies
mm-encode mail-parse rfc2231 rfc2047 rfc2045 ietf-drums mailabbrev
gmm-utils mailheader gnus-win gnus gnus-ems nnheader mail-utils misearch
multi-isearch jka-compr add-log vc-arch vc-mtn vc-hg vc-git vc-bzr
vc-sccs vc-svn vc-cvs vc-rcs tcl flyspell ispell eldoc diminish
projectile edmacro kmacro pkg-info find-func lisp-mnt epl grep compile
dash s appt diary-lib diary-loaddefs cal-menu calendar cal-loaddefs
which-func imenu minimap autorevert filenotify cus-start cus-load status
erc-services erc-list erc-menu erc-join erc-ring erc-networks
erc-pcomplete pcomplete erc-track erc-match erc-button wid-edit
cl-loaddefs cl-lib erc-fill erc-stamp erc-netsplit erc-goodies erc
erc-backend erc-compat format-spec auth-source eieio byte-opt bytecomp
byte-compile cconv eieio-core gnus-util mm-util mail-prsvr
password-cache thingatpt pp advice help-fns vc-dir ewoc vc vc-dispatcher
cc-styles cc-align cc-engine cc-vars cc-defs bbdb timezone ange-ftp
comint ansi-color ring server savehist dwarf-mode-autoloads
gdb-shell-autoloads jabber-autoloads lisppaste-autoloads
pydoc-info-autoloads info-look info easymenu weblogger-autoloads package
bbdb-autoloads time-date tooltip electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel x-win x-dnd tool-bar dnd fontset image regexp-opt
fringe tabulated-list newcomment lisp-mode prog-mode register page
menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core frame cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
case-table epa-hook jka-cmpr-hook help simple abbrev minibuffer nadvice
loaddefs button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process dbusbind
gfilenotify dynamic-setting system-font-setting font-render-setting
move-toolbar gtk x-toolkit x multi-tty emacs)

Memory information:
((conses 16 765705 79812)
 (symbols 48 98987 0)
 (miscs 40 29965 3141)
 (strings 32 306257 26993)
 (string-bytes 1 6729704)
 (vectors 16 94726)
 (vector-slots 8 2050490 133815)
 (floats 8 591 1493)
 (intervals 56 28928 314)
 (buffers 976 110)
 (heap 1024 200585 13852))

Tom

bug#18399: 24.4.50; nadvice :filter-args -vs- interactive

[Michael Heerdegen]

Tom Tromey <tom@tromey.com> writes:

> So I think there must be a bug with how nadvice handles interactive
> specs, either generally or with :filter-args.

According to the doc (of `add-function'), an filter-args advice function
has to accept exactly one argument (which is bound to the list of given
arguments).  So I think what you see is expected.

I have stumbled over that behavior several times myself.

Michael.

bug#18399: 24.4.50; nadvice :filter-args -vs- interactive

[Tom Tromey]

Michael> According to the doc (of `add-function'), an filter-args advice
Michael> function has to accept exactly one argument (which is bound to
Michael> the list of given arguments).  So I think what you see is
Michael> expected.

Michael> I have stumbled over that behavior several times myself.

I looked at the docs again and I agree.  Sorry about the noise.  Perhaps
a note and/or a small example here would be nice for future users.  If
we were both fooled by this then perhaps others will be as well.

Tom

bug#18399: 24.4.50; nadvice :filter-args -vs- interactive

[Stefan Monnier]

Michael> According to the doc (of `add-function'), an filter-args advice
Michael> function has to accept exactly one argument (which is bound to
Michael> the list of given arguments).  So I think what you see is
Michael> expected.
Michael> I have stumbled over that behavior several times myself.
> I looked at the docs again and I agree.  Sorry about the noise.  Perhaps
> a note and/or a small example here would be nice for future users.  If
> we were both fooled by this then perhaps others will be as well.

FWIW, the use of a single formal arg receiving the actual arg-list
in :filter-args is based on performance reasons (we have the list
anyway, so it's more efficient to pass it to `funcall' than to `apply').


        Stefan

bug#18399: 24.4.50; nadvice :filter-args -vs- interactive

[Stefan Monnier]

> FWIW, the use of a single formal arg receiving the actual arg-list
> in :filter-args is based on performance reasons (we have the list
> anyway, so it's more efficient to pass it to `funcall' than to `apply').

Oh, and also, this way the function can return the list as-is when
the args don't need to be modified.


        Stefan

bug#18399: 24.4.50; nadvice :filter-args -vs- interactive

[Stefan Monnier]

> I looked at the docs again and I agree.  Sorry about the noise.  Perhaps
> a note and/or a small example here would be nice for future users.  If
> we were both fooled by this then perhaps others will be as well.

I installed the patch below, which I hope will help clear up such
confusion.


        Stefan


=== modified file 'doc/lispref/ChangeLog'
--- doc/lispref/ChangeLog	2014-08-19 18:56:29 +0000
+++ doc/lispref/ChangeLog	2014-09-04 15:42:28 +0000
@@ -1,3 +1,8 @@
+2014-09-04  Stefan Monnier  <monnier@iro.umontreal.ca>
+
+	* functions.texi (Core Advising Primitives): Add a note about the
+	confusing treatment of `interactive' for :filter-args (bug#18399).
+
 2014-08-19  Eli Zaretskii  <eliz@gnu.org>
 
 	* display.texi (Bidirectional Display): Update the Emacs's class

=== modified file 'doc/lispref/functions.texi'
--- doc/lispref/functions.texi	2014-05-27 01:09:45 +0000
+++ doc/lispref/functions.texi	2014-09-04 15:40:13 +0000
@@ -1220,15 +1220,6 @@
 This macro is the handy way to add the advice @var{function} to the function
 stored in @var{place} (@pxref{Generalized Variables}).
 
-If @var{function} is not interactive, then the combined function will inherit
-the interactive spec, if any, of the original function.  Else, the combined
-function will be interactive and will use the interactive spec of
-@var{function}.  One exception: if the interactive spec of @var{function}
-is a function (rather than an expression or a string), then the interactive
-spec of the combined function will be a call to that function with as sole
-argument the interactive spec of the original function.  To interpret the spec
-received as argument, use @code{advice-eval-interactive-spec}.
-
 @var{where} determines how @var{function} is composed with the
 existing function, e.g. whether @var{function} should be called before, or
 after the original function.  @xref{Advice combinators}, for the list of
@@ -1271,6 +1262,21 @@
 @code{:override} advice will override not only the original function but all
 other advices applied to it as well.
 @end table
+
+If @var{function} is not interactive, then the combined function will inherit
+the interactive spec, if any, of the original function.  Else, the combined
+function will be interactive and will use the interactive spec of
+@var{function}.  One exception: if the interactive spec of @var{function}
+is a function (rather than an expression or a string), then the interactive
+spec of the combined function will be a call to that function with as sole
+argument the interactive spec of the original function.  To interpret the spec
+received as argument, use @code{advice-eval-interactive-spec}.
+
+Note: The interactive spec of @var{function} will apply to the combined
+function and should hence obey the calling convention of the combined function
+rather than that of @var{function}.  In many cases, it makes no difference
+since they are identical, but it does matter for @code{:around},
+@code{:filter-args}, and @code{filter-return}, where @var{function}.
 @end defmac
 
 @defmac remove-function place function

bug#18399: 24.4.50; nadvice :filter-args -vs- interactive

[Michael Heerdegen]

Hi Stefan

> I installed the patch below, which I hope will help clear up such
> confusion.

Thanks, a good clarification.

But I'm not sure if that covers what Tom meant.

I myself was confused by the fact that :filter-args is the only case of
all advice types where the advice fun receives the arguments as a list.
It's a bit surprising, although the doc is clear and there are good
reasons for that "exception".  Maybe we could add a sentence to the
‘:filter-args’ paragraph of (info "(elisp) Advice combinators") like
 
  "Note that FUNCTION is called with only one argument, the list of
  arguments, for this advice type".

Although it is redundant, it may be good to emphasize it.

Michael.

bug#18399: 24.4.50; nadvice :filter-args -vs- interactive

[Stefan Monnier]

> But I'm not sure if that covers what Tom meant.

The fact that we mention the unusual calling convention there does
partly cover it.

> Although it is redundant, it may be good to emphasize it.

As you say, it's a bit redundant there.


        Stefan

bug#18399: 24.4.50; nadvice :filter-args -vs- interactive

[Michael Heerdegen]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> > But I'm not sure if that covers what Tom meant.
>
> The fact that we mention the unusual calling convention there does
> partly cover it.

Ok, I agree, let's keep the patch as is, thanks.

Michael.

bug#18399: 24.4.50; nadvice :filter-args -vs- interactive

[Tom Tromey]

Michael> I myself was confused by the fact that :filter-args is the only case of
Michael> all advice types where the advice fun receives the arguments as a list.
Michael> It's a bit surprising, although the doc is clear and there are good
Michael> reasons for that "exception".  Maybe we could add a sentence to the
Michael> ‘:filter-args’ paragraph of (info "(elisp) Advice combinators") like
 
Michael>   "Note that FUNCTION is called with only one argument, the list of
Michael>   arguments, for this advice type".

Yeah, I had the same thought and had written the appended patch.
My reason was simply that I had been (mis-)reading the :filter-args
text, not the stuff about the (interactive) spec.

Tom

=== modified file 'doc/lispref/functions.texi'
*** doc/lispref/functions.texi	2014-06-02 00:18:22 +0000
--- doc/lispref/functions.texi	2014-09-06 05:40:02 +0000
***************
*** 1480,1485 ****
--- 1480,1488 ----
  @example
  (lambda (&rest r) (apply @var{oldfun} (funcall @var{function} r)))
  @end example
+ Note carefully that, unlike with other combinators, in the
+ @code{:filter-args} case, the original arguments are passed as a
+ single argument to the advising function.
  
  @item :filter-return
  Call the old function first and pass the result to @var{function}.