Re: [PATCH, RFC] Macros expansion changes, robust symbol macros, and constant propagation

[Stefan Monnier]

Thanks, Daniel,

I'm not happy about all of it, but I do like parts of it.

>  - removes Fmacroexpand and implements macroexpand-1 and macroexpand in lisp

Sounds good (indeed for gv.el we want macroexpand-1).  Do you have some
measurement of the performance impact?

>  - adds a facility for telling macroexpand-all not to re-expand macros
>    (effectively, providing for a variable macro expansion order)

The current code already lets you do "variable macro expansion order"
since calling the macroexpander redundantly will simply have no effect
(other than waste time).  So the need to prevent re-expansion
is only performance, AFAIK.

BTW, I see you sometimes strip the macroexpand-already-expanded tag.
Can't this lead again to repeated expansion?

>  - re-implements symbol-macros using this new facility
>    * no more problems with EQ symbol names

Good.

>    * shadowable and non-shadowable (let becomes letf) varieties

Not sure how important that is.

>    * macros expanded only once, so no need to rename symbols

I don't know what renaming this refers to.  Can you give details.

>  - adds symbol-macros to the core, in macroexp

I don't think I like this: CL's symbol-macro is *very* rarely used, so
for now I'd rather keep it in cl-macs.el.

>  - adds variable-capture analysis to macroexp

That's a fairly large function, which re-implements an analysis largely
already performed in cconv.el (tho it might not be completely
straightforward to reuse that code for your purpose).

It's only used for cl--expr-contains, so it should be in cl-macs.el, not
in macroexp.el.  And I'm really not sure this complexity is warranted
for those uses: does it fix actual bugs?

>  - changes cl-defsubst so that it works like regular defsubst

Doesn't this break setf on defstruct slots?

>  - fixes various cl-lib bugs using the new macrexp features and symbol-macros

Did you keep track of the bugs it fixes?  It would be good to know.

>  - makes the generic variable support expand one macro at a time

Good.

>  - adds new byte-optimize code for `let' and `let*' that uses
>    symbol-macros to perform constant propagation

See below.

> +(defun byte-optimize-do-constant-propagation (let-form)
> +  (let* ((bindings (cadr let-form))
> +         (body (cddr let-form))
> +         (const-bindings)
> +         (nonconst-bindings))
> +    ;; Split bindings into const, nonconst sets.  We'll collapse
> +    ;; redundant nonconst bindings in the `symbol-macrolet'.
> +    (let ((bc bindings))
> +      (while bc
> +        (let* ((binding (pop bc))
> +               (var (or (car-safe binding) binding))
> +               (val (and (consp binding) (cadr binding))))
> +          (cond ((and (not (special-variable-p var))
> +                      (macroexp-const-p val))
> +                 (push (list var val) const-bindings))
> +                ((and (eq (car let-form) 'let*)
> +                      const-bindings
> +                      nonconst-bindings)
> +                 ;; Handle the rest of the let* forms as a child;
> +                 ;; we'll combine the nested let*s later.
> +                 (setq body `((let* (,binding ,@bc) ,@body)))
> +                 (setq bc nil))
> +                (t (push binding nonconst-bindings))))))
> +    (if (not const-bindings)
> +        form
> +      `(,(car let-form)
> +        ,nonconst-bindings
> +        ,@(macroexp-unprogn
> +           (macroexpand-all
> +            `(symbol-macrolet-shadowable
> +                 ,(nreverse const-bindings)
> +               ,@body)))))))

How does this handle the case where the var is first bound to a constant
and later setq'd?

>               (push (list temp) cl--loop-bindings)
>               (setq form `(if (setq ,temp ,cond)
> -                                ,@(cl-subst temp 'it form))))
> +                                (symbol-macrolet-shadowable ((it ,temp))
> +                                  ,@form))))

Why not (setq form `(let ((it ,cond)) (when it ,@form)))?

> +(defalias 'cl-letf 'letf)
> +(defalias 'cl-letf* 'letf*)

I don't think I want to have letf in the core (but I'd like to change
cl-letf so that in the case of simple-variable bindings, those should be
dynamically scoped, even in lexical-binding code).

> +(defun cl--struct-setf-expander (x name accessor pred-form pos)

Aha!  I wonder if I don't rely on cl-defsubst's previous behavior
elsewhere as well.

> +                (push `(define-setf-expander ,accessor (cl-x)

You can't use define-setf-expander here since it requires cl.el.

> -                                               nil)
> +                                               (and unsafe (list argn argv)))

Why undo this recent change?

> +    (`(lambda ,arglist . ,body)
> +     (macroexp-analyze-free-variables

This form should never appear in macroexpanded code.

> +    (`(closure ,cells ,arglist . ,body)

And this one shouldn't either.

> +(defconst macroexp--sm-environment-tag
> +  (if (boundp 'macroexp--sm-environment-tag)
> +      (symbol-value 'macroexp--sm-environment-tag)
> +    (make-symbol "--macroexp--sm-environment-tag--"))

Why not

   (defvar macroexp--sm-environment-tag
     (make-symbol "--macroexp--sm-environment-tag--")

Also, IIUC we could just use `:symbol-macro' instead.
     
> +    (`(lambda ,arglist . ,_)
> +     ;; Lambda arguments always shadow symbol macros

Again, such a form should never appear in macroexpanded code.

> +    (`(function (lambda . ,_))
> +     ;; macroexpand-all has special logic to detect lambda in function
> +     ;; position, so we need a special case here too.

I don't understand the comment.

> +(defconst macroexpand-already-expanded
> +  (if (boundp 'macroexpand-already-expanded)
> +      (symbol-value 'macroexpand-already-expanded)
> +    (make-symbol "--macroexpand-already-expanded--"))

Again: why not a defvar?  Also, we could define it as an interned
well-known symbol.  E.g. macroexp--already-expanded.

Also, all that macroexpand-1 code should ideally be in macroexp.el
rather than in subr.el.  Maybe that introduces bootstrapping problems,
but IIRC it can be made to work (I played around with such a thing
during the cl-lib.el work).

> +;; It may seem odd to macro expansion hooks in the macro environment
> +;; instead of dynamically-binding a hypothetical
> +;; macroexpand-1-functions variable.

Indeed.

> +;; The reason we do it this way is
> +;; so that we can expand macros from outside the dynamic extent of a
> +;; form that introduces a macro expansion hook --- e.g.,
> +;; `cl-symbol-macrolet'.

I don't understand the argument.  Do you have a concrete/detailed
example where it's needed/useful/better?

> +;; We can store other state in the environment as well: all that's
> +;; required for compatibility with naive users of the macro
> +;; environment is to ensure that no car of any cons in the macro
> +;; environment refers to a form we might try to expand.  That's why
> +;; all the macro tags should be uninterned.

Keywords should work as well.

> +(defun memq-car (key alist)
> +  "Find cons with car KEY in ALIST.
> +Like `assq', except that instead of returning
> +the cons cell whose car is `eq' to KEY, it returns
> +the cons cell whose car is that cons cell and whose
> +cdr is the rest of the alist."

Let's leave it close to its users, with a macroexp- prefix for now.


        Stefan