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

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

[daanturo]

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

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.

[-- Attachment #2: 0001-nadvice-nadvice.el-support-non-symbol-advices.patch --]
[-- Type: text/x-patch, Size: 3446 bytes --]

From b07fd697e097ed0ca6040781830ad42be2a9ac86 Mon Sep 17 00:00:00 2001
From: Daanturo <daanturo@gmail.com>
Date: Sun, 4 Dec 2022 21:34:52 +0700
Subject: [PATCH] * nadvice/nadvice.el: support non-symbol advices

(advice-add): by aliasing the function to a new symbol
---
 nadvice.el | 53 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/nadvice.el b/nadvice.el
index 58523f6..443a5d0 100644
--- a/nadvice.el
+++ b/nadvice.el
@@ -52,30 +52,38 @@
 (defun advice-member-p (advice symbol)
   (ad-find-advice symbol 'around advice))
 
+
+(defun advice--ensure-symbol (func)
+  (if (symbolp func)
+      func
+    (let* ((sym (intern (format "%S" func))))
+      (unless (fboundp sym)
+        (defalias sym func))
+      sym)))
+
 ;;;###autoload
 (defun advice-add (symbol where function &optional props)
   (when props
     (error "This version of nadvice.el does not support PROPS"))
-  (unless (symbolp function)
-    (error "This version of nadvice.el requires FUNCTION to be a symbol"))
-  (let ((body (cond
-               ((eq where :before)
-                `(progn (apply #',function (ad-get-args 0)) ad-do-it))
-               ((eq where :after)
-                `(progn ad-do-it (apply #',function (ad-get-args 0))))
-               ((eq where :override)
-                `(setq ad-return-value (apply #',function (ad-get-args 0))))
-               ((eq where :around)
-                `(setq ad-return-value
-                       (apply #',function
-                              (lambda (&rest nadvice--rest-arg)
-                                (ad-set-args 0 nadvice--rest-arg)
-                                ad-do-it)
-                              (ad-get-args 0))))
-               (t (error "This version of nadvice.el does not handle %S"
-                         where)))))
+  (let* ((advice-fn (advice--ensure-symbol function))
+         (body (cond
+                ((eq where :before)
+                 `(progn (apply #',advice-fn (ad-get-args 0)) ad-do-it))
+                ((eq where :after)
+                 `(progn ad-do-it (apply #',advice-fn (ad-get-args 0))))
+                ((eq where :override)
+                 `(setq ad-return-value (apply #',advice-fn (ad-get-args 0))))
+                ((eq where :around)
+                 `(setq ad-return-value
+                        (apply #',advice-fn
+                               (lambda (&rest nadvice--rest-arg)
+                                 (ad-set-args 0 nadvice--rest-arg)
+                                 ad-do-it)
+                               (ad-get-args 0))))
+                (t (error "This version of nadvice.el does not handle %S"
+                          where)))))
     (ad-add-advice symbol
-                   `(,function nil t (advice lambda () ,body))
+                   `(,advice-fn nil t (advice lambda () ,body))
                    'around
                    nil)
     (ad-activate symbol)))
@@ -84,9 +92,10 @@
 (defun advice-remove (symbol function)
   ;; Just return nil if there is no advice, rather than signaling an
   ;; error.
-  (when (advice-member-p function symbol)
-    (ad-remove-advice symbol 'around function)
-    (ad-activate symbol)))
+  (let* ((advice-fn (advice--ensure-symbol function)))
+    (when (advice-member-p advice-fn symbol)
+      (ad-remove-advice symbol 'around advice-fn)
+      (ad-activate symbol))))
 
 )
 
-- 
2.38.1

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

[Stefan Kangas]

daanturo <daanturo@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?

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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

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

[daanturo]

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

> 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.

[-- Attachment #2: 0002-nadvice-nadvice.el-warn-against-non-symbol-FUNCTIONs.patch --]
[-- Type: text/x-patch, Size: 671 bytes --]

From bf10a19145e3f08acb7d65fa26316fbdbeb6fd17 Mon Sep 17 00:00:00 2001
From: Daanturo <daanturo@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..e44bfe1 100644
--- a/nadvice.el
+++ b/nadvice.el
@@ -57,6 +57,8 @@
   (if (symbolp func)
       func
     (let* ((sym (intern (format "%S" func))))
+      (message "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

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

[Daan Ro]

[-- Attachment #1.1: Type: text/plain, Size: 1091 bytes --]

Use `warn` instead of `message`.

On Tue, Dec 13, 2022 at 9:59 PM daanturo <daanturo@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.

[-- Attachment #1.2: Type: text/html, Size: 1707 bytes --]

[-- Attachment #2: 0002-nadvice-nadvice.el-warn-against-non-symbol-FUNCTIONs.patch --]
[-- Type: text/x-patch, Size: 668 bytes --]

From f30a1f6d815a038dde51ea09089e9aa2155c8c62 Mon Sep 17 00:00:00 2001
From: Daanturo <daanturo@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

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

[Stefan Kangas]

tags 59820 + patch
thanks

Daan Ro <daanturo@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@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

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> 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

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

[Stefan Kangas]

tags 59820 + moreinfo
thanks

Stefan Monnier <monnier@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.

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

[Daan Ro]

[-- Attachment #1.1: Type: text/plain, Size: 1464 bytes --]

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.


[-- Attachment #1.2: Type: text/html, Size: 3482 bytes --]

[-- Attachment #2: 0001-nadvice-nadvice.el-support-non-symbol-advices.patch --]
[-- Type: application/octet-stream, Size: 3593 bytes --]

From 995bc962194853c40d90f582de793a6090382f87 Mon Sep 17 00:00:00 2001
From: Daanturo <daanturo@gmail.com>
Date: Sun, 4 Dec 2022 21:34:52 +0700
Subject: [PATCH] * nadvice/nadvice.el: support non-symbol advices

(advice-add): by aliasing the function to a new symbol
---
 nadvice.el | 57 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 22 deletions(-)

diff --git a/nadvice.el b/nadvice.el
index 58523f6..78ecb3f 100644
--- a/nadvice.el
+++ b/nadvice.el
@@ -52,30 +52,41 @@
 (defun advice-member-p (advice symbol)
   (ad-find-advice symbol 'around advice))
 
+
+(defun advice--ensure-symbol (func)
+  (if (symbolp func)
+      func
+    (let* ((sym (intern (format "nadvice--%S" func))))
+      (display-warning
+       'nadvice
+       (format "FUNCTION: %S should be a named symbol instead." func))
+      (unless (fboundp sym)
+        (defalias sym func))
+      sym)))
+
 ;;;###autoload
 (defun advice-add (symbol where function &optional props)
   (when props
     (error "This version of nadvice.el does not support PROPS"))
-  (unless (symbolp function)
-    (error "This version of nadvice.el requires FUNCTION to be a symbol"))
-  (let ((body (cond
-               ((eq where :before)
-                `(progn (apply #',function (ad-get-args 0)) ad-do-it))
-               ((eq where :after)
-                `(progn ad-do-it (apply #',function (ad-get-args 0))))
-               ((eq where :override)
-                `(setq ad-return-value (apply #',function (ad-get-args 0))))
-               ((eq where :around)
-                `(setq ad-return-value
-                       (apply #',function
-                              (lambda (&rest nadvice--rest-arg)
-                                (ad-set-args 0 nadvice--rest-arg)
-                                ad-do-it)
-                              (ad-get-args 0))))
-               (t (error "This version of nadvice.el does not handle %S"
-                         where)))))
+  (let* ((advice-fn (advice--ensure-symbol function))
+         (body (cond
+                ((eq where :before)
+                 `(progn (apply #',advice-fn (ad-get-args 0)) ad-do-it))
+                ((eq where :after)
+                 `(progn ad-do-it (apply #',advice-fn (ad-get-args 0))))
+                ((eq where :override)
+                 `(setq ad-return-value (apply #',advice-fn (ad-get-args 0))))
+                ((eq where :around)
+                 `(setq ad-return-value
+                        (apply #',advice-fn
+                               (lambda (&rest nadvice--rest-arg)
+                                 (ad-set-args 0 nadvice--rest-arg)
+                                 ad-do-it)
+                               (ad-get-args 0))))
+                (t (error "This version of nadvice.el does not handle %S"
+                          where)))))
     (ad-add-advice symbol
-                   `(,function nil t (advice lambda () ,body))
+                   `(,advice-fn nil t (advice lambda () ,body))
                    'around
                    nil)
     (ad-activate symbol)))
@@ -84,10 +95,12 @@
 (defun advice-remove (symbol function)
   ;; Just return nil if there is no advice, rather than signaling an
   ;; error.
-  (when (advice-member-p function symbol)
-    (ad-remove-advice symbol 'around function)
-    (ad-activate symbol)))
+  (let* ((advice-fn (advice--ensure-symbol function)))
+    (when (advice-member-p advice-fn symbol)
+      (ad-remove-advice symbol 'around advice-fn)
+      (ad-activate symbol))))
 
+;
 )
 
 (provide 'nadvice)
-- 
2.42.0

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> 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