GNU bug report logs - #59820
[PATCH] * nadvice/nadvice.el: support non-symbol (closure/lambda) advices (old Emacs)

Previous Next

To reply to this bug, email your comments to 59820 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: daanturo <daanturo <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] * nadvice/nadvice.el: support non-symbol (closure/lambda)
 advices (old Emacs)
Date: Mon, 5 Dec 2022 00:14:49 +0700

[Message part 1 (text/plain, inline)]

This patch provides the ELPA version (for Emacs < 24.4) of nadvice.el's
advice-add the ability to handle anonymous advices.

I have tested with a simple example:


```emacs-lisp

(let* ((sym (make-symbol "nadvice λ")))
  (defalias sym (lambda (&rest args) '(1)))
  (advice-add sym :around (lambda (func &rest args)
                            (append (apply func args) '(2))))
  (vector
   ;; advised returned value
   (funcall sym)
   (progn
     (advice-remove sym (lambda (func &rest args)
                          (append (apply func args) '(2))))
     ;; unadvised returned value
     (funcall sym))))

;; => [(1 2) (1)]

```


In GNU Emacs 24.3.1 (x86_64-redhat-linux-gnu, GTK+ Version 3.22.30)
 of 2020-04-04 on x86-01.bsys.centos.org
Windowing system distributor `The X.Org Foundation', version
 11.0.12201005
 
Configured using:
 `configure '--build=x86_64-redhat-linux-gnu'
 '--host=x86_64-redhat-linux-gnu' '--program-prefix='
 '--disable-dependency-tracking' '--prefix=/usr' '--exec-prefix=/usr'
 '--bindir=/usr/bin' '--sbindir=/usr/sbin' '--sysconfdir=/etc'
 '--datadir=/usr/share' '--includedir=/usr/include'
 '--libdir=/usr/lib64' '--libexecdir=/usr/libexec'
 '--localstatedir=/var' '--sharedstatedir=/var/lib'
 '--mandir=/usr/share/man' '--infodir=/usr/share/info' '--with-dbus'
 '--with-gif' '--with-jpeg' '--with-png' '--with-rsvg' '--with-tiff'
 '--with-xft' '--with-xpm' '--with-x-toolkit=gtk3' '--with-gpm=no'
 'build_alias=x86_64-redhat-linux-gnu'
 'host_alias=x86_64-redhat-linux-gnu' 'CFLAGS=-DMAIL_USE_LOCKF -O2 -g
 -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
 -fstack-protector-strong --param=ssp-buffer-size=4
 -grecord-gcc-switches -m64 -mtune=generic' 'LDFLAGS=-Wl,-z,relro ''


-- 
Daanturo.

[0001-nadvice-nadvice.el-support-non-symbol-advices.patch (text/x-patch, attachment)]

Message #8 received at 59820 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: daanturo <daanturo <at> gmail.com>
Cc: 59820 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#59820: [PATCH] * nadvice/nadvice.el: support non-symbol
 (closure/lambda) advices (old Emacs)
Date: Mon, 12 Dec 2022 17:04:01 -0800

daanturo <daanturo <at> gmail.com> writes:

> This patch provides the ELPA version (for Emacs < 24.4) of nadvice.el's
> advice-add the ability to handle anonymous advices.
>
> I have tested with a simple example:
>
> ```emacs-lisp
>
> (let* ((sym (make-symbol "nadvice λ")))
>   (defalias sym (lambda (&rest args) '(1)))
>   (advice-add sym :around (lambda (func &rest args)
>                             (append (apply func args) '(2))))
>   (vector
>    ;; advised returned value
>    (funcall sym)
>    (progn
>      (advice-remove sym (lambda (func &rest args)
>                           (append (apply func args) '(2))))
>      ;; unadvised returned value
>      (funcall sym))))
>
> ;; => [(1 2) (1)]
>
> ```

Stefan, any comments here?

Message #13 received at 59820 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: daanturo <daanturo <at> gmail.com>
Cc: 59820 <at> debbugs.gnu.org
Subject: Re: bug#59820: [PATCH] * nadvice/nadvice.el: support non-symbol
 (closure/lambda) advices (old Emacs)
Date: Tue, 13 Dec 2022 08:50:27 -0500

> This patch provides the ELPA version (for Emacs < 24.4) of nadvice.el's
> advice-add the ability to handle anonymous advices.
[...]
> +(defun advice--ensure-symbol (func)
> +  (if (symbolp func)
> +      func
> +    (let* ((sym (intern (format "%S" func))))
> +      (unless (fboundp sym)
> +        (defalias sym func))
> +      sym)))

I'm not a big fan of this approach, and I usually recommend to use named
functions for advice anyway (avoids all kinds of problems like the
`advice-remove` failing to remove, or the equality test taking too much
time, ...).

IOW I'd rather align the "real nadvice.el" with the one in GNU ELPA than
the other way around in this respect.


        Stefan

Message #16 received at 59820 <at> debbugs.gnu.org (full text, mbox):

From: daanturo <daanturo <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 59820 <at> debbugs.gnu.org
Subject: Re: bug#59820: [PATCH] * nadvice/nadvice.el: support non-symbol
 (closure/lambda) advices (old Emacs)
Date: Tue, 13 Dec 2022 21:59:23 +0700

[Message part 1 (text/plain, inline)]

> I usually recommend to use named
> functions for advice anyway

How about we still allow but warn against such problematic usage?


On 13/12/2022 20:50, Stefan Monnier wrote:
>> This patch provides the ELPA version (for Emacs < 24.4) of nadvice.el's
>> advice-add the ability to handle anonymous advices.
> [...]
>> +(defun advice--ensure-symbol (func)
>> +  (if (symbolp func)
>> +      func
>> +    (let* ((sym (intern (format "%S" func))))
>> +      (unless (fboundp sym)
>> +        (defalias sym func))
>> +      sym)))
> I'm not a big fan of this approach, and I usually recommend to use named
> functions for advice anyway (avoids all kinds of problems like the
> `advice-remove` failing to remove, or the equality test taking too much
> time, ...).
>
> IOW I'd rather align the "real nadvice.el" with the one in GNU ELPA than
> the other way around in this respect.
>
>
>         Stefan
>
-- 
Daanturo.

[0002-nadvice-nadvice.el-warn-against-non-symbol-FUNCTIONs.patch (text/x-patch, attachment)]

Message #19 received at 59820 <at> debbugs.gnu.org (full text, mbox):

From: Daan Ro <daanturo <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 59820 <at> debbugs.gnu.org, Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: bug#59820: [PATCH] * nadvice/nadvice.el: support non-symbol
 (closure/lambda) advices (old Emacs)
Date: Tue, 13 Dec 2022 22:02:51 +0700

[Message part 1 (text/plain, inline)]

Use `warn` instead of `message`.

On Tue, Dec 13, 2022 at 9:59 PM daanturo <daanturo <at> gmail.com> wrote:

> > I usually recommend to use named
> > functions for advice anyway
>
> How about we still allow but warn against such problematic usage?
>
>
> On 13/12/2022 20:50, Stefan Monnier wrote:
> >> This patch provides the ELPA version (for Emacs < 24.4) of nadvice.el's
> >> advice-add the ability to handle anonymous advices.
> > [...]
> >> +(defun advice--ensure-symbol (func)
> >> +  (if (symbolp func)
> >> +      func
> >> +    (let* ((sym (intern (format "%S" func))))
> >> +      (unless (fboundp sym)
> >> +        (defalias sym func))
> >> +      sym)))
> > I'm not a big fan of this approach, and I usually recommend to use named
> > functions for advice anyway (avoids all kinds of problems like the
> > `advice-remove` failing to remove, or the equality test taking too much
> > time, ...).
> >
> > IOW I'd rather align the "real nadvice.el" with the one in GNU ELPA than
> > the other way around in this respect.
> >
> >
> >         Stefan
> >
> --
> Daanturo.
>


-- 
Daanturo.

[Message part 2 (text/html, inline)]

[0002-nadvice-nadvice.el-warn-against-non-symbol-FUNCTIONs.patch (text/x-patch, attachment)]

Message #22 received at 59820 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Daan Ro <daanturo <at> gmail.com>, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 59820 <at> debbugs.gnu.org
Subject: Re: bug#59820: [PATCH] * nadvice/nadvice.el: support non-symbol
 (closure/lambda) advices (old Emacs)
Date: Mon, 9 Oct 2023 09:45:20 +0000

tags 59820 + patch
thanks

Daan Ro <daanturo <at> gmail.com> writes:

> Use `warn` instead of `message`.

Stefan, what do you think?  Should the below patch be installed?

> From f30a1f6d815a038dde51ea09089e9aa2155c8c62 Mon Sep 17 00:00:00 2001
> From: Daanturo <daanturo <at> gmail.com>
> Date: Tue, 13 Dec 2022 21:28:03 +0700
> Subject: [PATCH] * nadvice/nadvice.el: warn against non-symbol FUNCTIONs
>
> ---
>  nadvice.el | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/nadvice.el b/nadvice.el
> index 443a5d0..c6666d0 100644
> --- a/nadvice.el
> +++ b/nadvice.el
> @@ -57,6 +57,8 @@
>    (if (symbolp func)
>        func
>      (let* ((sym (intern (format "%S" func))))
> +      (warn "This version of nadvice.el recommends that \
> +FUNCTION: %S is a named symbol instead." func)
>        (unless (fboundp sym)
>          (defalias sym func))
>        sym)))
> --
> 2.39.0

Message #25 received at 59820 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 59820 <at> debbugs.gnu.org, Daan Ro <daanturo <at> gmail.com>
Subject: Re: bug#59820: [PATCH] * nadvice/nadvice.el: support non-symbol
 (closure/lambda) advices (old Emacs)
Date: Mon, 09 Oct 2023 18:06:40 -0400

>> Use `warn` instead of `message`.
> Stefan, what do you think?  Should the below patch be installed?

Still not a fan for the same reasons, plus:

- The real `nadvice.el` was added to Emacs-24.4, released 9 years ago,
  and this forward compatibility was first released 5 years ago, so the
  need for the library is becoming rare and its functionality proved
  adequate for 5 years already.
- (intern (format "%S" func)) can collide with another library
  doing something similar, so better use something like
  (intern (format "nadvice--%S" func)) just to be on the safe side.

Daan, is there a specific use-case that motivates you to want to pass an
anonymous lambda to this compatibility library?

Maybe I could be convinced by a good use-case.


        Stefan

Message #28 received at 59820 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 59820 <at> debbugs.gnu.org, Daan Ro <daanturo <at> gmail.com>
Subject: Re: bug#59820: [PATCH] * nadvice/nadvice.el: support non-symbol
 (closure/lambda) advices (old Emacs)
Date: Tue, 10 Oct 2023 10:44:18 +0000

tags 59820 + moreinfo
thanks

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>>> Use `warn` instead of `message`.
>> Stefan, what do you think?  Should the below patch be installed?
>
> Still not a fan for the same reasons, plus:
>
> - The real `nadvice.el` was added to Emacs-24.4, released 9 years ago,
>   and this forward compatibility was first released 5 years ago, so the
>   need for the library is becoming rare and its functionality proved
>   adequate for 5 years already.
> - (intern (format "%S" func)) can collide with another library
>   doing something similar, so better use something like
>   (intern (format "nadvice--%S" func)) just to be on the safe side.
>
> Daan, is there a specific use-case that motivates you to want to pass an
> anonymous lambda to this compatibility library?
>
> Maybe I could be convinced by a good use-case.

Makes sense.  I've tagged the bug as moreinfo for now, but barring a
compelling use case I'm also leaning towards closing this as wontfix.

Message #33 received at 59820 <at> debbugs.gnu.org (full text, mbox):

From: Daan Ro <daanturo <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: "59820 <at> debbugs.gnu.org" <59820 <at> debbugs.gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#59820: [PATCH] * nadvice/nadvice.el: support non-symbol
 (closure/lambda) advices (old Emacs)
Date: Wed, 11 Oct 2023 13:04:51 +0700

[Message part 1 (text/plain, inline)]

As per S. Monnier's suggestion, I prefixed "nadvice--" to the interning symbol's
name (squashed patch attached).

> Daan, is there a specific use-case that motivates you to want to pass an
> anonymous lambda to this compatibility library?

I think lambdas are useful for temporary advices that doesn't need to be
attached to their symbols forever.

For example, when pressing "C-<backspace>", or some other editing operations, I
don't want it to modify the kill ring and the desktop's clipboard.

```elisp
(defun my-delete-instead-of-kill-when-interactive-a (func &rest args)
(if (called-interactively-p 'any)
(let* ((func (lambda (beg end &rest _) (delete-region beg end))))
(advice-add #'kill-region :override func)
(unwind-protect
(apply func args)
(advice-remove #'kill-region func)))
(apply func args)))

(advice-add #'backward-kill-word :around #'my-delete-instead-of-kill-when-interactive-a)
(advice-add #'subword-backward-kill :around #'my-delete-instead-of-kill-when-interactive-a)

(advice-add #'kill-line :around #'my-delete-instead-of-kill-when-interactive-a)
;; and other (potentially in the future) variants of `backward-kill-word' such
;; as `puni-backward-kill-word', `sp-backward-kill-word',
;; `sp-backward-kill-symbol', etc. that are bound to some key bindings

```
There are many short-lived advices like the anonymous function above that making
them a dedicated function isn't worthy, IMO. Especially closures that capture
lexical variables.

[Message part 2 (text/html, inline)]

[0001-nadvice-nadvice.el-support-non-symbol-advices.patch (application/octet-stream, attachment)]

Message #36 received at 59820 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Daan Ro <daanturo <at> gmail.com>
Cc: "59820 <at> debbugs.gnu.org" <59820 <at> debbugs.gnu.org>,
 Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: bug#59820: [PATCH] * nadvice/nadvice.el: support non-symbol
 (closure/lambda) advices (old Emacs)
Date: Wed, 11 Oct 2023 02:50:39 -0400

>> Daan, is there a specific use-case that motivates you to want to pass an
>> anonymous lambda to this compatibility library?
>
> I think lambdas are useful for temporary advices that doesn't need to be
> attached to their symbols forever.

That doesn't explain why you need to use them with the forward compatibility
library.

> For example, when pressing "C-<backspace>", or some other editing operations, I
> don't want it to modify the kill ring and the desktop's clipboard.
>
> ```elisp
> (defun my-delete-instead-of-kill-when-interactive-a (func &rest args)
> (if (called-interactively-p 'any)
> (let* ((func (lambda (beg end &rest _) (delete-region beg end))))
> (advice-add #'kill-region :override func)
> (unwind-protect
> (apply func args)
> (advice-remove #'kill-region func)))
> (apply func args)))
> (advice-add #'backward-kill-word :around #'my-delete-instead-of-kill-when-interactive-a)
> (advice-add #'subword-backward-kill :around #'my-delete-instead-of-kill-when-interactive-a)

FWIW, as a general rule I prefer to avoid such transient advice and
prefer to use a permanent advice together with an "enabling" variable:

    (defvar my-dont-kill nil)
    
    (defun my-obey-dont-kill (func beg end &rest args)
      "Obey `my-dont-kill."
      (if my-dont-kill
          (delete-region beg end)
        (apply func beg end args)))
    (advice-add 'kill-region :around #'my-obey-dont-kill)
    
    (defun my-delete-instead-of-kill-when-interactive-a (func &rest args)
      (if (called-interactively-p 'any)
          (let ((my-dont-kill t))
            (apply func args))
        (apply func args)))
    
    (advice-add #'backward-kill-word :around #'my-delete-instead-of-kill-when-interactive-a)
    (advice-add #'subword-backward-kill :around #'my-delete-instead-of-kill-when-interactive-a)

The main advantage, for me, is that `C-h o kill-region` will tell me
that the function has an advice (and I can click on it to jump to its
definition, ...) so I'll be less puzzled when it doesn't behave in "the
normal way".  Sometimes it also comes with the advantage that I can make
the variable buffer-local, contrary to the advice itself.

Of course, in this specific instance, there's another way to get "the
same" result:

    (defun my-delete-instead-of-kill-when-interactive-a (func &rest args)
      (if (called-interactively-p 'any)
          (let ((kill-ring nil)
                (kill-ring-yank-pointer nil)
                (interprogram-cut-function nil))
            (apply func args))
        (apply func args)))
    
    (advice-add #'backward-kill-word :around #'my-delete-instead-of-kill-when-interactive-a)
    (advice-add #'subword-backward-kill :around #'my-delete-instead-of-kill-when-interactive-a)

:-)


        Stefan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.