Re: [PATCH] Improve detection of local function calls in methods

[Stefan Monnier <monnier@iro.umontreal.ca>]

> 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