Subject: Re: bug#49809: [PATCH] Add macro 'pcase-setq'

From: Stefan Monnier <[email-masked]>
Date: Wed, 04 Aug 2021 19:06:11 -0400

> This patch adds a `setq`-like equivalent to `pcase-let`.
> This is convenient when one wants the bindings to exist outside of
> a `let` form.

Thanks.

> This macro expands into multiple `setq` calls that are combined where
> possible.

I don't think we should try and combine them: it's not worth the
code complexity.  Personally I'd even restrict the calling convention to
(pcase-setq PAT VAL), but if you want to accept the more general case
with multiple PAT+VAL, I'd prefer expanding it to a (progn (pcase-setq
PAT1 VAL1) ...).  I think the resulting code would be simpler/cleaner.

> Please let me know what should be changed.

See a few more comments below.

> Subject: [PATCH] Add macro 'pcase-setq'
>
> * doc/lispref/control.texi: Document this macro.
> * lisp/emacs-lisp/pcase.el: Add this macro.

Please include the "section/function" info,
e.g. I'd write the message as:

    * lisp/emacs-lisp/pcase.el (pcase-setq): New macro.

    This macro is the 'setq' equivalent of 'pcase-let'.

    * doc/lispref/control.texi (Destructuring with pcase Patterns): Document it.

> +@defmac pcase-setq pattern value@dots{}
> +Bind variables to values in a @code{setq} form, destructuring each
> +@var{value} according to its respective @var{pattern}.
> +@end defmac

I prefer keeping "bind" for the case where we create new variables
(i.e. let-bindings) rather than for assignments.

> +;;;###autoload
> +(defmacro pcase-setq (&rest args)
> +  "Bind values by destructuring with `pcase'.

Same here.

> +\(fn PATTERN VALUE PATTERN VALUE ...)"
> +  (declare (debug (&rest [pcase-PAT form])))
> +  (let ((results)
> +        (pattern)
> +        (value))
> +    (while args
> +      (setq pattern (pop args)
> +            value (pop args))
> +      (push (if (pcase--trivial-upat-p pattern)
> +                (list 'setq pattern value)
> +              (pcase-compile-patterns
> +               value
> +               (list (cons pattern
> +                           (lambda (varvals &rest _)
> +                             (cons 'setq
> +                                   (mapcan (lambda (varval)
> +                                             (let ((var (car varval))
> +                                                   (val (cadr varval)))
> +                                               (list var val)))
> +                                           varvals)))))))
> +            results))

Looks good.  But could you add a few corresponding tests to
`test/lisp/emacs-lisp/pcase-tests.el`, including tests for things like

    (pcase-setq `(,a ,b) nil)

-- Stefan