all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* [PATCH] Improve detection of local function calls in methods
@ 2021-08-27 21:41 akater
  2021-08-27 22:59 ` Stefan Monnier
  0 siblings, 1 reply; 6+ messages in thread
From: akater @ 2021-08-27 21:41 UTC (permalink / raw)
  To: emacs-devel


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


Instead of using unreliale and expensive macroexp--fgrep, we record the
relevant calls in the macroexpansion, as suggested in the FIXME entry.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 865 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: cl--generic-lambda fix --]
[-- Type: text/x-diff, Size: 4091 bytes --]

From 099c63eb1c107531252fde859dee7466de05f210 Mon Sep 17 00:00:00 2001
From: akater <nuclearspace@gmail.com>
Date: Thu, 26 Aug 2021 06:09:07 +0000
Subject: [PATCH] Improve detection of local function calls in methods

* lisp/emacs-lisp/cl-generic.el (cl--generic-lambda):
Rather than `grep' after the fact,
the macroexpansion records directly
when cl-call-next-method or cl-next-method-p are used.
---
 lisp/emacs-lisp/cl-generic.el | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/lisp/emacs-lisp/cl-generic.el b/lisp/emacs-lisp/cl-generic.el
index 4a69df15bc..d5d77fe553 100644
--- a/lisp/emacs-lisp/cl-generic.el
+++ b/lisp/emacs-lisp/cl-generic.el
@@ -361,6 +361,14 @@ defun cl--generic-split-args (args)
       (cons (nreverse specializers)
             (nreverse (delq nil plain-args)))))
 
+  (defvar cl-generic--uses-cnm nil
+    ;; It would be better to declare the variable special
+    ;; locally where it's used
+    ;; but there is no support for local special declarations in Elisp.
+    "In a runtime environment, keeps a list of flags that indicate
+the presence of `cl-call-next-method' or `cl-next-method-p'
+in a method body.")
+
   (defun cl--generic-lambda (args body)
     "Make the lambda expression for a method with ARGS and BODY."
     (pcase-let* ((`(,spec-args . ,plain-args)
@@ -369,7 +377,7 @@ defun cl--generic-lambda (args body)
                  (macroenv (cons `(cl-generic-current-method-specializers
                                    . ,(lambda () spec-args))
                                  macroexpand-all-environment)))
-      (require 'cl-lib)        ;Needed to expand `cl-flet' and `cl-function'.
+      (require 'cl-lib)        ;Needed to expand `cl-function', `cl-macrolet'.
       (when (interactive-form (cadr fun))
         (message "Interactive forms unsupported in generic functions: %S"
                  (interactive-form (cadr fun))))
@@ -380,21 +388,29 @@ defun cl--generic-lambda (args body)
          (let* ((parsed-body (macroexp-parse-body body))
                 (cnm (make-symbol "cl--cnm"))
                 (nmp (make-symbol "cl--nmp"))
+                (cl-generic--uses-cnm)
                 (nbody (macroexpand-all
-                        `(cl-flet ((cl-call-next-method ,cnm)
-                                   (cl-next-method-p ,nmp))
+                        `(cl-macrolet ((cl-call-next-method
+                                        (&rest args)
+                                        (prog1 `(funcall ,',cnm ,@args)
+                                          (cl-pushnew
+                                           ',cnm cl-generic--uses-cnm
+                                           :test #'eq)))
+                                       (cl-next-method-p
+                                        ()
+                                        (prog1 `(funcall ,',nmp)
+                                          (cl-pushnew
+                                           ',nmp cl-generic--uses-cnm
+                                           :test #'eq))))
                            ,@(cdr parsed-body))
                         macroenv))
-                ;; FIXME: Rather than `grep' after the fact, the
-                ;; macroexpansion should directly set some flag when cnm
-                ;; is used.
-                ;; FIXME: Also, optimize the case where call-next-method is
+                ;; FIXME: Optimize the case where call-next-method is
                 ;; only called with explicit arguments.
-                (uses-cnm (macroexp--fgrep `((,cnm) (,nmp)) nbody)))
+                (uses-cnm cl-generic--uses-cnm))
            (cons (not (not uses-cnm))
                  `#'(lambda (,@(if uses-cnm (list cnm)) ,@args)
                       ,@(car parsed-body)
-                      ,(if (not (assq nmp uses-cnm))
+                      ,(if (not (memq nmp uses-cnm))
                            nbody
                          `(let ((,nmp (lambda ()
                                         (cl--generic-isnot-nnm-p ,cnm))))
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] Improve detection of local function calls in methods
  2021-08-27 21:41 [PATCH] Improve detection of local function calls in methods akater
@ 2021-08-27 22:59 ` Stefan Monnier
  2021-08-29 11:25   ` akater
  0 siblings, 1 reply; 6+ messages in thread
From: Stefan Monnier @ 2021-08-27 22:59 UTC (permalink / raw)
  To: akater; +Cc: emacs-devel

> Instead of using unreliale and expensive macroexp--fgrep, we record the
> relevant calls in the macroexpansion, as suggested in the FIXME entry.

Yes, please!

> +  (defvar cl-generic--uses-cnm nil
> +    ;; It would be better to declare the variable special
> +    ;; locally where it's used
> +    ;; but there is no support for local special declarations in Elisp.

[ I'm not completely sure what you mean, but (defvar foo) has an effect
  limited to the current scope.  This said, I don't think it matters
  much here, because using a globally declared dynvar is perfectly fine
  IMO (the main reason not to use a globally declared dynvar is either
  because we really want to keep the global definition unbound or
  because we really don't want to give the var a namespace prefix).  ]

> -                        `(cl-flet ((cl-call-next-method ,cnm)
> -                                   (cl-next-method-p ,nmp))
> +                        `(cl-macrolet ((cl-call-next-method
> +                                        (&rest args)
> +                                        (prog1 `(funcall ,',cnm ,@args)
> +                                          (cl-pushnew
> +                                           ',cnm cl-generic--uses-cnm
> +                                           :test #'eq)))
> +                                       (cl-next-method-p
> +                                        ()
> +                                        (prog1 `(funcall ,',nmp)
> +                                          (cl-pushnew
> +                                           ',nmp cl-generic--uses-cnm
> +                                           :test #'eq))))

Hmm... IIUC this fails to account for the case where
#'cl-call-next-method is passed to a function (the most common case (or
more precisely, the only case I've seen so far) being when it's passed
to `apply`).


        Stefan




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Improve detection of local function calls in methods
  2021-08-27 22:59 ` Stefan Monnier
@ 2021-08-29 11:25   ` akater
  2021-09-02 18:34     ` Stefan Monnier
  0 siblings, 1 reply; 6+ messages in thread
From: akater @ 2021-08-29 11:25 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

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

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

> Hmm... IIUC this fails to account for the case where
> #'cl-call-next-method is passed to a function (the most common case (or
> more precisely, the only case I've seen so far) being when it's passed
> to `apply`).

Right.  cl-flet should have reminded me that cl-call-next-method is a
local function, not a local macro.

> [ I'm not completely sure what you mean, but (defvar foo) has an effect

I was referring to hypothetical

#+begin_example elisp
(let* (...
       cl-generic--uses-crm)
  (declare (special cl-generic--uses-crm))
  ...)
#+end_example

But it doesn't matter anymore.  See the new patches.

Notes:

- I splitted patches in two because the first diff looks significantly
  better this way.

- Normally I'd put uses-cnm binding/declaration right before where it's
  used.  However, putting it earlier gives a cleaner diff; also,
  uses-cnm means “parsed body uses cnm” so I think it's OK to put it
  right on the same line as (parsed-body ..), and this also utilizes the
  whitespace better.

I don't know if all this is appropriate style; I provide patches this
way in the hope it's acceptable.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: cl--generic-lambda fix --]
[-- Type: text/x-diff, Size: 4851 bytes --]

From 2fd4d66a93831a63d19a8ab2efb927136f196beb Mon Sep 17 00:00:00 2001
From: akater <nuclearspace@gmail.com>
Date: Thu, 26 Aug 2021 06:09:07 +0000
Subject: [PATCH 1/2] Improve detection of local function calls in methods

* lisp/emacs-lisp/cl-generic.el (cl--generic-lambda):
Rather than `grep' after the fact,
the macroexpansion records directly
when cl-call-next-method or cl-next-method-p are used.
---
 lisp/emacs-lisp/cl-generic.el | 51 ++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 12 deletions(-)

diff --git a/lisp/emacs-lisp/cl-generic.el b/lisp/emacs-lisp/cl-generic.el
index 4a69df15bc..8fdd905785 100644
--- a/lisp/emacs-lisp/cl-generic.el
+++ b/lisp/emacs-lisp/cl-generic.el
@@ -369,7 +369,7 @@ defun cl--generic-lambda (args body)
                  (macroenv (cons `(cl-generic-current-method-specializers
                                    . ,(lambda () spec-args))
                                  macroexpand-all-environment)))
-      (require 'cl-lib)        ;Needed to expand `cl-flet' and `cl-function'.
+      (require 'cl-lib)        ;Needed to expand `cl-function' and body.
       (when (interactive-form (cadr fun))
         (message "Interactive forms unsupported in generic functions: %S"
                  (interactive-form (cadr fun))))
@@ -377,24 +377,51 @@ defun cl--generic-lambda (args body)
       ;; destructuring args, `declare' and whatnot).
       (pcase (macroexpand fun macroenv)
         (`#'(lambda ,args . ,body)
-         (let* ((parsed-body (macroexp-parse-body body))
+         (let* ((parsed-body (macroexp-parse-body body)) uses-cnm
                 (cnm (make-symbol "cl--cnm"))
                 (nmp (make-symbol "cl--nmp"))
-                (nbody (macroexpand-all
-                        `(cl-flet ((cl-call-next-method ,cnm)
-                                   (cl-next-method-p ,nmp))
-                           ,@(cdr parsed-body))
-                        macroenv))
-                ;; FIXME: Rather than `grep' after the fact, the
-                ;; macroexpansion should directly set some flag when cnm
-                ;; is used.
+                (nbody
+                 ;; We duplicate the code from `cl-flet' augmenting it
+                 ;; with `cl-pushnew' forms to record the presence of
+                 ;; `cl-call-next-method', `cl-next-method-p'.
+                 ;; It would be better to avoid code duplication
+                 ;; but it's not clear how to do that reasonably enough.
+                 (let ((newenv
+                        (cons `(cl-call-next-method
+                                .
+                                ,(lambda (&rest args)
+                                   (cl-pushnew cnm uses-cnm :test #'eq)
+                                   (if (eq (car args) cl--labels-magic)
+                                       (list cl--labels-magic cnm)
+                                     `(funcall ,cnm ,@args))))
+                              (cons `(cl-next-method-p
+                                      .
+                                      ,(lambda (&rest args)
+                                         (cl-pushnew nmp uses-cnm :test #'eq)
+                                         (if (eq (car args) cl--labels-magic)
+                                             (list cl--labels-magic nmp)
+                                           `(funcall ,nmp ,@args))))
+                                    macroenv))))
+                   (macroexpand-all
+                    `(progn ,@(cdr parsed-body))
+                    ;; Don't override lexical-let's macro-expander
+                    (if (assq 'function newenv) newenv
+                      (cons (cons 'function
+                                  (lambda (f)
+                                    (cl-case f
+                                      (cl-call-next-method
+                                       (cl-pushnew cnm uses-cnm :test #'eq))
+                                      (cl-next-method-p
+                                       (cl-pushnew nmp uses-cnm :test #'eq)))
+                                    (cl--labels-convert f)))
+                            newenv)))))
                 ;; FIXME: Also, optimize the case where call-next-method is
                 ;; only called with explicit arguments.
-                (uses-cnm (macroexp--fgrep `((,cnm) (,nmp)) nbody)))
+                (uses-cnm uses-cnm))
            (cons (not (not uses-cnm))
                  `#'(lambda (,@(if uses-cnm (list cnm)) ,@args)
                       ,@(car parsed-body)
-                      ,(if (not (assq nmp uses-cnm))
+                      ,(if (not (memq nmp uses-cnm))
                            nbody
                          `(let ((,nmp (lambda ()
                                         (cl--generic-isnot-nnm-p ,cnm))))
-- 
2.31.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: cl--generic-lambda cleanup --]
[-- Type: text/x-diff, Size: 1171 bytes --]

From 9d506f6bdee01bce3dfe2fed8d159dfd4ce9d0ef Mon Sep 17 00:00:00 2001
From: akater <nuclearspace@gmail.com>
Date: Sun, 29 Aug 2021 10:38:12 +0000
Subject: [PATCH 2/2] ; * lisp/emacs-lisp/cl-generic.el (cl--generic-lambda):
 Cleanup

---
 lisp/emacs-lisp/cl-generic.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/cl-generic.el b/lisp/emacs-lisp/cl-generic.el
index 8fdd905785..fd57599b1e 100644
--- a/lisp/emacs-lisp/cl-generic.el
+++ b/lisp/emacs-lisp/cl-generic.el
@@ -415,9 +415,9 @@ defun cl--generic-lambda (args body)
                                        (cl-pushnew nmp uses-cnm :test #'eq)))
                                     (cl--labels-convert f)))
                             newenv)))))
-                ;; FIXME: Also, optimize the case where call-next-method is
+                ;; FIXME: Optimize the case where call-next-method is
                 ;; only called with explicit arguments.
-                (uses-cnm uses-cnm))
+                )
            (cons (not (not uses-cnm))
                  `#'(lambda (,@(if uses-cnm (list cnm)) ,@args)
                       ,@(car parsed-body)
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] Improve detection of local function calls in methods
  2021-08-29 11:25   ` akater
@ 2021-09-02 18:34     ` Stefan Monnier
  2021-09-10  6:25       ` akater
  0 siblings, 1 reply; 6+ messages in thread
From: Stefan Monnier @ 2021-09-02 18:34 UTC (permalink / raw)
  To: akater; +Cc: emacs-devel

> I don't know if all this is appropriate style; I provide patches this
> way in the hope it's acceptable.

Yes, that's very nice, thank you.

> @@ -377,24 +377,51 @@ defun cl--generic-lambda (args body)
>        ;; destructuring args, `declare' and whatnot).
>        (pcase (macroexpand fun macroenv)
>          (`#'(lambda ,args . ,body)
> -         (let* ((parsed-body (macroexp-parse-body body))
> +         (let* ((parsed-body (macroexp-parse-body body)) uses-cnm
>                  (cnm (make-symbol "cl--cnm"))
>                  (nmp (make-symbol "cl--nmp"))
> -                (nbody (macroexpand-all
> -                        `(cl-flet ((cl-call-next-method ,cnm)
> -                                   (cl-next-method-p ,nmp))
> -                           ,@(cdr parsed-body))
> -                        macroenv))
> -                ;; FIXME: Rather than `grep' after the fact, the
> -                ;; macroexpansion should directly set some flag when cnm
> -                ;; is used.
> +                (nbody
> +                 ;; We duplicate the code from `cl-flet' augmenting it
> +                 ;; with `cl-pushnew' forms to record the presence of
> +                 ;; `cl-call-next-method', `cl-next-method-p'.
> +                 ;; It would be better to avoid code duplication
> +                 ;; but it's not clear how to do that reasonably enough.
> +                 (let ((newenv
> +                        (cons `(cl-call-next-method
> +                                .
> +                                ,(lambda (&rest args)
> +                                   (cl-pushnew cnm uses-cnm :test #'eq)
> +                                   (if (eq (car args) cl--labels-magic)
> +                                       (list cl--labels-magic cnm)
> +                                     `(funcall ,cnm ,@args))))
> +                              (cons `(cl-next-method-p
> +                                      .
> +                                      ,(lambda (&rest args)
> +                                         (cl-pushnew nmp uses-cnm :test #'eq)
> +                                         (if (eq (car args) cl--labels-magic)
> +                                             (list cl--labels-magic nmp)
> +                                           `(funcall ,nmp ,@args))))
> +                                    macroenv))))
> +                   (macroexpand-all
> +                    `(progn ,@(cdr parsed-body))
> +                    ;; Don't override lexical-let's macro-expander
> +                    (if (assq 'function newenv) newenv
> +                      (cons (cons 'function
> +                                  (lambda (f)
> +                                    (cl-case f
> +                                      (cl-call-next-method
> +                                       (cl-pushnew cnm uses-cnm :test #'eq))
> +                                      (cl-next-method-p
> +                                       (cl-pushnew nmp uses-cnm :test #'eq)))
> +                                    (cl--labels-convert f)))
> +                            newenv)))))

Hmm... the reason why I didn't do that (when I wrote the comment
instead), is that I find this duplication ugly.

I think "the right way" would be for the `cl-flet` implementation to use
a `cl--expand-flet` function returning which functions are used
and which aren't.

Then we could use it here without such duplication, *and* we could use
it in `cl-flet` to emit warnings about unused functions.

WDYT?


        Stefan




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Improve detection of local function calls in methods
  2021-09-02 18:34     ` Stefan Monnier
@ 2021-09-10  6:25       ` akater
  2021-09-10 15:49         ` Stefan Monnier
  0 siblings, 1 reply; 6+ messages in thread
From: akater @ 2021-09-10  6:25 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

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

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

> I think "the right way" would be for the `cl-flet` implementation to use
> a `cl--expand-flet` function returning which functions are used
> and which aren't.
>
> Then we could use it here without such duplication, *and* we could use
> it in `cl-flet` to emit warnings about unused functions.
>
> WDYT?

I agree.  I think the root of the problem is, ~cl-flet~ is generally not
implemented well.  Feels like it was implemented in a rush.

- (func exp) is non-standard and ad-hoc.  Do you happen to remember,
  maybe it was invented solely for ~cl--generic-lambda~?

- (func exp) definition form is described incorrectly in the docstring:
  it should be (FUNCTION-NAME SYMBOL) and so on; this also affects
  cl-macrolet docstring which claims it's “like cl-flet” but in fact
  it's not as it doesn't support this definition form

- ~cl-flet~ with invalid function names should error but it doesn't

I implemented ~cl--expand-flet~ but I felt simply returning a list of
used symbols would be just another ad-hoc addition so I decided to
instead add an interface to execute arbitrary code during
macroexpansion.  It took longer than I expected but I'm satisfied with
the result.  I'll open a standalone bug (my implementation happens to
fix some outstanding issues); when changes are (hopefully) merged, we'll
continue on with this one.

Quick reference: if all goes well, the definition of
~cl--generic-lambda~ is to become
#+begin_example emacs-lisp
..
(let* ((parsed-body (macroexp-parse-body body)) uses-cnm
       (cnm (make-symbol "cl--cnm"))
       (nmp (make-symbol "cl--nmp"))
       (nbody (cl--expand-flet macroenv (cdr parsed-body)
                (cl-call-next-method (push cnm uses-cnm) cnm)
                (cl-next-method-p (push nmp uses-cnm) nmp)))
       ;; FIXME: Optimize the case where call-next-method is
       ;; only called with explicit arguments.
       )
  ..)
#+end_example

This involves a “macro” version of expand-flet; we'll see whether it's
worth keeping alongside the corresponding function.

If you can, please confirm my suspicion that (FUNC EXP) definitions were
indeed invented solely for ~cl--generic-lambda~.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 865 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Improve detection of local function calls in methods
  2021-09-10  6:25       ` akater
@ 2021-09-10 15:49         ` Stefan Monnier
  0 siblings, 0 replies; 6+ messages in thread
From: Stefan Monnier @ 2021-09-10 15:49 UTC (permalink / raw)
  To: akater; +Cc: emacs-devel

> - (func exp) is non-standard and ad-hoc.

(FUNC EXP) is indeed non-standard.  But I completely disagree with
`ad-hoc`: IMO this form is the fundamental form of `flet` (it
corresponds directly to `let`, just in the function namespace) and
the form (FUNC ARGS &rest BODY) should be viewed as a simple syntactic
sugar for the common case of (FUNC (lambda ARGS &rest BODY)).

>   Do you happen to remember, maybe it was invented solely for
>   ~cl--generic-lambda~?

It was added to Emacs's `master` for the purpose of
`cl--generic-lambda`, yes.  It was invented long before, OTOH ;-)
[ I still can't explain how I forgot to add it when we moved from
  `flet` to `cl-flet` in Emacs-24.3.  ]

> - (func exp) definition form is described incorrectly in the docstring:
>   it should be (FUNCTION-NAME SYMBOL) and so on;

Not sure what you mean here.  Do you mean `func` should be
`function-name` since this should be a symbol and not a function?
[ I would agree with that, tho `function-name` would be longish so
  I'd rather go maybe with just `name`.  ]
Or do you mean that `exp` should be `symbol`?
If so, I disagree because this allows an arbitrary expression that's
evaluated before its resulting value is bound to the specified function
symbol.

>   this also affects `cl-macrolet` docstring which claims it's “like
>   `cl-flet`” but in fact it's not as it doesn't support this definition
>   form

Indeed, this was not updated when `cl-flet` was extended.
Not sure what's best to do here:
- we could extend `cl-macrolet` to also accept (NAME EXP), but this is
  much less useful for macros since EXP is evaluated in the
  macroexpansion context.
- tweak the docstring to clarify the "like"ness.

> - ~cl-flet~ with invalid function names should error but it doesn't

You mean if the "function name" is not a symbol?
Indeed, it would be better to signal an error in that case.

> This involves a “macro” version of expand-flet; we'll see whether it's
> worth keeping alongside the corresponding function.

Your sample code looks OK, tho I don't think having a macro for it is
worth the trouble.


        Stefan




^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-09-10 15:49 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-27 21:41 [PATCH] Improve detection of local function calls in methods akater
2021-08-27 22:59 ` Stefan Monnier
2021-08-29 11:25   ` akater
2021-09-02 18:34     ` Stefan Monnier
2021-09-10  6:25       ` akater
2021-09-10 15:49         ` Stefan Monnier

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.