bug#13023: 24.3.50; lexical binding does not work within defadvice

bug#13023: 24.3.50; lexical binding does not work within defadvice

[Christopher Schmidt]

    ;; -*- lexical-binding: t -*-

    (funcall (let ((rms "works"))
               (lambda ()
                 (message "lex-bind %s" rms))))

    (defun asdf (b) (funcall b))

    (defadvice asdf (before rms (b) activate)
      (setf b (let ((abc 1) (b b)) (lambda () (print abc) (funcall b)))))

    (asdf 'ding)

I think this code should work fine.  It doesn't work with trunk and
emacs-24, though:

    Load test.elc? (y or n)  y
    Loading test.elc...
    lex-bind works
    Load error for test.elc:
    (void-variable abc)

        Christopher

bug#13023: 24.3.50; lexical binding does not work within defadvice

[Stefan Monnier]

>     ;; -*- lexical-binding: t -*-
>     (funcall (let ((rms "works"))
>                (lambda ()
>                  (message "lex-bind %s" rms))))

>     (defun asdf (b) (funcall b))

>     (defadvice asdf (before rms (b) activate)
>       (setf b (let ((abc 1) (b b)) (lambda () (print abc) (funcall b)))))

>     (asdf 'ding)

> I think this code should work fine.  It doesn't work with trunk and
> emacs-24, though:

Indeed, it doesn't work.  And because of the way advice.el works
(building a new function by combining the code chunks from all the
pieces of advice applied to that function) it's not easy to fix.

This is partly related to the issue mentioned recently that macro calls
in pieces of advice are expanded late (typically when the advised
function is defined).

BTW, this does not affect the new `advice-add' feature in Emacs trunk.


        Stefan

bug#13023: 24.3.50; lexical binding does not work within defadvice

[Richard Stallman]

    Indeed, it doesn't work.  And because of the way advice.el works
    (building a new function by combining the code chunks from all the
    pieces of advice applied to that function) it's not easy to fix.

Would it be possible to fix this by defining a new primitive construct
for use in the constructed function that runs the advince?

-- 
Dr Richard Stallman
President, Free Software Foundation
51 Franklin St
Boston MA 02110
USA
www.fsf.org  www.gnu.org
Skype: No way! That's nonfree (freedom-denying) software.
  Use Ekiga or an ordinary phone call

bug#13023: 24.3.50; lexical binding does not work within defadvice

[Stefan Monnier]

>     Indeed, it doesn't work.  And because of the way advice.el works
>     (building a new function by combining the code chunks from all the
>     pieces of advice applied to that function) it's not easy to fix.
> Would it be possible to fix this by defining a new primitive construct
> for use in the constructed function that runs the advice?

I don't think there's a need to add any new primitive.  If someone wants
to dig into advice.el to try and fix it, it can be fixed there (as
evidenced by advice-add).
I personally would rather tell people to use advice-add instead.


        Stefan

bug#13023: 24.3.50; lexical binding does not work within defadvice

[Richard Stallman]

    I don't think there's a need to add any new primitive.  If someone wants
    to dig into advice.el to try and fix it, it can be fixed there (as
    evidenced by advice-add).
    I personally would rather tell people to use advice-add instead.

That would, in effect, be an incompatible change in interface.
Why make the change?  Is there something wrong with defadvice?

-- 
Dr Richard Stallman
President, Free Software Foundation
51 Franklin St
Boston MA 02110
USA
www.fsf.org  www.gnu.org
Skype: No way! That's nonfree (freedom-denying) software.
  Use Ekiga or an ordinary phone call

bug#13023: 24.3.50; lexical binding does not work within defadvice

[Stefan Monnier]

> Is there something wrong with defadvice?

Yes: complexity.  Compare nadvice.el and advice.el.  Even after you
remove the long comment at the beginning, the difference in code size
speaks for itself.


        Stefan

bug#13023: 24.3.50; lexical binding does not work within defadvice

[Juanma Barranquero]

> Even after you
> remove the long comment at the beginning, the difference in code size
> speaks for itself.

Exactly 1,000 lines longer, after removing comments and blank lines:
1,305 vs 305.

(Bored, moi?)

    Juanma

bug#13023: 24.3.50; lexical binding does not work within defadvice

[Richard Stallman]

    Yes: complexity.  Compare nadvice.el and advice.el.

The main difference seems to me that part of the arguments of
advice.el get pushed into the function that you need to pass
to advice-add.  So the complexity of using it is no more.
Also, you lose the ability to modify the argument values before
the advised function's body.

The doc string of advice-add is incomplete, so I cannot tell
what all its args mean or what it can really do.

-- 
Dr Richard Stallman
President, Free Software Foundation
51 Franklin St
Boston MA 02110
USA
www.fsf.org  www.gnu.org
Skype: No way! That's nonfree (freedom-denying) software.
  Use Ekiga or an ordinary phone call

bug#13023: 24.3.50; lexical binding does not work within defadvice

[Richard Stallman]

Correction:

    Yes: complexity.  Compare nadvice.el and advice.el.

The main difference seems to me that part of the arguments of
advice.el get pushed into the function that you need to pass
to advice-add.  So the complexity of using advice-add is no less.
Also, you lose the ability to modify the argument values before
the advised function's body.

The doc string of advice-add is incomplete, so I cannot tell
what all its args mean or what it can really do.

-- 
Dr Richard Stallman
President, Free Software Foundation
51 Franklin St
Boston MA 02110
USA
www.fsf.org  www.gnu.org
Skype: No way! That's nonfree (freedom-denying) software.
  Use Ekiga or an ordinary phone call

bug#13023: 24.3.50; lexical binding does not work within defadvice

[Marcin Borkowski]

On 2012-11-28, at 23:32, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

>>     ;; -*- lexical-binding: t -*-
>>     (funcall (let ((rms "works"))
>>                (lambda ()
>>                  (message "lex-bind %s" rms))))
>
>>     (defun asdf (b) (funcall b))
>
>>     (defadvice asdf (before rms (b) activate)
>>       (setf b (let ((abc 1) (b b)) (lambda () (print abc) (funcall b)))))
>
>>     (asdf 'ding)
>
>> I think this code should work fine.  It doesn't work with trunk and
>> emacs-24, though:
>
> Indeed, it doesn't work.  And because of the way advice.el works
> (building a new function by combining the code chunks from all the
> pieces of advice applied to that function) it's not easy to fix.
>
> This is partly related to the issue mentioned recently that macro calls
> in pieces of advice are expanded late (typically when the advised
> function is defined).
>
> BTW, this does not affect the new `advice-add' feature in Emacs trunk.

Hi Stefan,
hi all,

does the above mean that this bug should be closed?  AFAIU, new code
should (at least usually) use advice-add, and while the "old" advice
system is not going to be deleted, it is kind of deprecated.

Best,

-- 
Marcin Borkowski
http://mbork.pl/en

bug#13023: 24.3.50; lexical binding does not work within defadvice

[Stefan Monnier]

> does the above mean that this bug should be closed?

To the extent that I don't know of anyone who intends to fix this
problem, I guess we could close it as "won't fix".


        Stefan