bug#43100: 28.0.50; pcase not binding variables conditionally

[Stefan Monnier]

>> IIUC you want
>>
>>     (pcase V
>>       ((or (pred symbolp) name)
>>        (let ((foo 'bar)) name)))
>>
>> to behave like
>>
>>     (cond
>>      ((symbolp V) (let ((foo 'bar)) name))
>>      (t (let ((name V)) (let ((foo 'bar)) name))))
>>
>> ?
>
> Yes, that's correct. It's also how (pcase V ((or (pred symbolp) name)
> name) behaves...

Indeed, but that's an accident.  Ideally it should either signal an
error at macro-expansion time, or return nil when V is a symbol.
Since the current implementation doesn't go to the effort of doing
either of those, we instead limit ourselves to recommend against using
such patterns (IOW, "use at your own risks").

>> I'd rather not go there
> You'd rather have the behavior of (pcase V ((or pred symbolp) name)
> EXPR) depend on the complexity of EXPR?

More specifically, I'd rather not choose a semantics that imposes
duplicating the branch body, since we have no control over its size and
that can hence lead to potential code size explosion.

A code size explosion due to a particular implementation choice is
undesirable, but a code size explosion imposed by the semantics is much
more problematic.

> I think it would be nice to have a lexical three-argument version of
> pcase which specifies which variables are output values, treating the
> remaining ones as input values, to make it easier to build
> non-constant patterns.

The design of `pcase` assumes you want to optimize away the tests that
are common to the various patterns.  That can't be done with dynamic
patterns.

> IOW, if you want to call a function with arguments determined by
> pcase-like patterns, why not introduce pcase-call so something like
> the following would work:
>
> (defun f (hello world) (cons hello world))
>
> (let ((space " ") (hw "hello world"))
>   (pcase-call 'f ((concat hello space world) hw)))

How do you intend to implement this?

> As for duplicating the body, that is an implementation detail. You can
> easily avoid it by producing
>
> (let ((name name))
>   (cond ((symbolp V) X)
>     (progn (setq name V) X)))

So it's more like my option of returning nil, except it would return
the value of a surrounding `name` variable?  That could be done, but I'm
not convinced it'd be more often useful.

> disallowing the modification of name in X.

That's rather hard to do (and I don't see what would be the benefit here).

>> The "intended" behavior instead would be to behave like
>>
>>     (cond
>>      ((symbolp V) (let ((name nil)) (let ((foo 'bar)) name)))
>>      (t (let ((name V)) (let ((foo 'bar)) name))))
>>
>> That's already the behavior you get if you switch the two:
>>
>>     (macroexpand '(pcase V
>>                     ((or (and (pred foo) name) (pred symbolp))
>>                      (let ((foo 'bar)) name))))
>>     =>
>>     (let* ((pcase-0 (lambda (name) (let ((foo 'bar)) name))))
>>       (cond ((foo V) (funcall pcase-0 V))
>>             ((symbolp V) (funcall pcase-0 nil))
>>             (t nil)))
>
> I don't see where the nil comes from, or why it's a useful choice for
> a default value.

It comes from the absence of a binding for `name` and was chosen because
nil is the standard default value in Elisp.

It comes from this code in pcase.el:

                    (let ((args (mapcar (lambda (pa)
                                          (let ((v (assq (car pa) vars)))
                                            (setq vars (delq v vars))
                                            (cdr v)))
                                        prevvars)))
                      ;; If some of `vars' were not found in `prevvars', that's
                      ;; OK it just means those vars aren't present in all
                      ;; branches, so they can be used within the pattern
                      ;; (e.g. by a `guard/let/pred') but not in the branch.
                      ;; FIXME: But if some of `prevvars' are not in `vars' we
                      ;; should remove them from `prevvars'!
                      `(funcall ,res ,@args)))))))

The computation of `args` searches in `vars` for the bindings expected
by the branch (stored in `prevvars` the first time we encountered that
branch).  The assq+cdr will return nil if a var from `prevvars` isn't
found in `vars`.

>> the fact that the behavior depends on the order of elements in `or` is
>> an undesirable side effect of the implementation technique.
> It also depends on the complexity of the branch.
> It seems to me there are at least three consistent ways of behaving
> (throw an error, bind name to nil, bind name to name), with an
> inconsistent fourth way being what's currently implemented.

The current implementation amounts to "we should signal an error but we
don't bother doing so and just warn against it in the manual".
Patch welcome ;-)

>> I don't know of a simple implementation.
> Here's my better-than-nothing attempt.  I don't think that's complex;
> if anything, it's too trivial.

So you give it a search-based semantics.
The problem with it for me is that if we turn

    `(,a ,@b)

into

    (append `(,a) b)

the pcase match will take a lot more time than the equivalent

    `(,a . ,b)

Of course, you can try and handle these "easy" cases more efficiently,
but then your ,@ will sometimes be very cheap and sometimes very
expensive (depending on when an optimization can be applied), which
I think is a misfeature (it's for this same reason that I dislike CL
keyword arguments for functions).

I think it's fine to have such a search-based `append` (because it's
"reliably expensive") but I'd rather not automatically use it for ,@ 

[ BTW, you don't need (nor want) `eval` in your definition.  ]


        Stefan