* [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 public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).