bug#12610: unable to use macro in defadvice

[Le Wang]

Relevant stackoverflow question:
http://stackoverflow.com/questions/12767353/using-macros-in-defadvice

I have this simplified macro.

(defadvice kill-buffer (around show-diff-rephrase-question activate compile)
  "Prompt when a buffer is about to be killed."
  (case (read-char-choice
         "(s/k/q)? "
         (append "sSKkQq" nil))
    ((?s ?S)
     ad-do-it)
    ((?k ?K)
     ad-do-it)
    ((?q ?Q) nil))
  ad-do-it)

If I compile the file and load it, then I get "Invalid function: (115 83)"

It works if I evaluate it.

--
Le

[Glenn Morris-3]

Interesting. Advice does not macroexpand the advice definition.
I guess it should?

*** lisp/emacs-lisp/advice.el 2012-09-14 13:44:31 +0000
--- lisp/emacs-lisp/advice.el 2012-10-12 02:02:39 +0000
***************
*** 3658,3664 ****
  (advice (ad-make-advice
   name (memq 'protect flags)
   (not (memq 'disable flags))
!  `(advice lambda ,arglist ,@body)))
  (preactivation (if (memq 'preactivate flags)
     (ad-preactivate-advice
      function advice class position))))
--- 3658,3664 ----
  (advice (ad-make-advice
   name (memq 'protect flags)
   (not (memq 'disable flags))
!  `(advice lambda ,arglist ,@(macroexpand-all body))))
  (preactivation (if (memq 'preactivate flags)
     (ad-preactivate-advice
      function advice class position))))

[Glenn Morris-3]

Actually I guess it would be better for the byte-compiler to do the
expansion.

[Stefan Monnier]

> -  `(advice lambda ,arglist ,@body)))
> +  `(advice lambda ,arglist ,@(macroexpand-all body))))

I'd rather not do such things manually.  Instead, we should expose the
code as code rather than hide it as data, so that macro-expansion
can take place without calling it explicitly and so the byte-compiler
gets to see the code, optimize it and emit warnings where needed.

I.e. I think the patch should start more along the lines of the one
below (100% guaranteed untested).


        Stefan


=== modified file 'lisp/emacs-lisp/advice.el'
--- lisp/emacs-lisp/advice.el 2012-09-14 13:44:31 +0000
+++ lisp/emacs-lisp/advice.el 2012-10-12 13:25:22 +0000
@@ -3655,13 +3655,13 @@
                      (t (error "defadvice: Invalid or ambiguous flag: %s"
                                flag))))))
    args))
- (advice (ad-make-advice
-  name (memq 'protect flags)
-  (not (memq 'disable flags))
-  `(advice lambda ,arglist ,@body)))
+ (advice `(ad-make-advice
+                   ',name ',(memq 'protect flags)
+                   ',(not (memq 'disable flags))
+                   (cons 'advice (lambda ,arglist ,@body))))
  (preactivation (if (memq 'preactivate flags)
     (ad-preactivate-advice
-     function advice class position))))
+     function (eval advice) class position))))
     ;; Now for the things to be done at evaluation time:
     (if (memq 'freeze flags)
  ;; jwz's idea: Freeze the advised definition into a dumpable
@@ -3669,7 +3669,7 @@
  (ad-make-freeze-definition function advice class position)
         ;; the normal case:
         `(progn
-          (ad-add-advice ',function ',advice ',class ',position)
+          (ad-add-advice ',function ,advice ',class ',position)
           ,@(if preactivation
                 `((ad-set-cache
                    ',function

[Glenn Morris-3]

Stefan Monnier wrote:

> Instead, we should expose the code as code rather than hide it as
> data, so that macro-expansion can take place without calling it
> explicitly and so the byte-compiler gets to see the code, optimize it
> and emit warnings where needed.

Sounds right.

> I.e. I think the patch should start more along the lines of the one
> below (100% guaranteed untested).

It causes warnings like:

In toplevel form:
uniquify.el:479:13:Warning: assignment to free variable `ad-return-value'
uniquify.el:487:41:Warning: reference to free variable `ad-return-value'

In end of data:
uniquify.el:511:1:Warning: the function `ad-get-arg' is not known to be
    defined.


More importantly, it doesn't work...

emacs -Q -l uniquify

-> byte-code: Symbol's function definition is void: ad-make-advice


Compiling the initial example from this report gives:

(byte-code "<STUFF>" [ad-add-advice kill-buffer ad-make-advice
show-diff-rephrase-question nil t advice #[nil "<STUFF>" [#:--cl-var--
ad-do-it read-char-choice "(s/k/q)? " append "sSKkQq" nil memql (115 83)
(107 75) (113 81)] 5 "Prompt when a buffer is about to be killed."]
around ad-activate] 8)

[Stefan Monnier]

>> I.e. I think the patch should start more along the lines of the one
>> below (100% guaranteed untested).
[...]
> More importantly, it doesn't work...

That's what I meant by "start".


        Stefan

[Lars Ingebrigtsen]

Le Wang <[hidden email]> writes:

> I have this simplified macro.
>
> (defadvice kill-buffer (around show-diff-rephrase-question activate compile)
>   "Prompt when a buffer is about to be killed."
>   (case (read-char-choice
>          "(s/k/q)? "
>          (append "sSKkQq" nil))
>     ((?s ?S)
>      ad-do-it)
>     ((?k ?K)
>      ad-do-it)
>     ((?q ?Q) nil))
>   ad-do-it)
>
> If I compile the file and load it, then I get "Invalid function: (115 83)"
>
> It works if I evaluate it.

I've modernised the code slightly:

(require 'cl-lib)
         
(defadvice kill-buffer (around show-diff-rephrase-question activate compile)
  "Prompt when a buffer is about to be killed."
  (cl-case (read-char-choice
            "(s/k/q)? "
            (append "sSKkQq" nil))
    ((?s ?S)
     ad-do-it)
    ((?k ?K)
     ad-do-it)
    ((?q ?Q) nil))
  ad-do-it)

I tried byte-compiling it and loading the .elc file, and it still
doesn't bug out.

So I'm guessing this has been fixed over the years?  I'm closing this
bug report; if you're still seeing this in recent Emacs versions, please
respond to the debbugs address and we'll reopen.

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no