bug#20268: 25.0.50; pcase-lambda broken

bug#20268: 25.0.50; pcase-lambda broken

[Leo Liu]

After the recent rewrite, pcase-lambda is broken. For example, eval the
following to get 46422 instead of the correct value 65535.

   (cl-some (pcase-lambda (`[fullsweep_after ,v]) v)
            '([min_bin_vheap_size 46422]
              [min_heap_size 233]
              [fullsweep_after 65535]
              [minor_gcs 40]))

Thanks,
Leo

bug#20268: 25.0.50; pcase-lambda broken

[Leo Liu]

Forgive me to rely here. None of our messages shown up in bug#19670.

> After recent rewrite of pcase-lambda I am seeing:

> (cl-some (pcase-lambda (`[fullsweep_after ,v]) v)
>         '([min_bin_vheap_size 46422]
>           [min_heap_size 233]
>           [fullsweep_after 65535]
>
>           [minor_gcs 10]))
>
> Indeed, that's the semantics I chose.
> The previous semantics was for the function to do nothing and return nil
> if the arg doesn't match.  The new semantics is the same as the one used
> by pcase-let.  It's not without its fault of course, but at least it does
> correspond to the usual idea of "destructuring" and generates more
> efficient code.

I don't mind new semantics but I want to make sense of it so as to use
it with confidence. What I am seeing is:

(funcall (pcase-lambda (`[fullsweep_after ,v]) v) [min_bin_vheap_size 46422])

=> 46422

That doesn't make sense to me. Could you explain why this is the right
thing?

> I think if you prefer to return nil, then the macro should look like

>   (pcase-lambda ((`[fullsweep_after ,v]) v))

> which would then naturally let you add additional cases like

>   (pcase-lambda ((`[fullsweep_after ,v]) v)
>                 ((`[min_heap_size ,v]) (/ v 2)))

I didn't choose the multiple-clause version because it is a cheap
shortcut for (lambda (...) (pcase ... ...)).

The single-clause pcase-lambda makes a lot of higher-order functions fun
to use and that is where I am reaping the benefits.

> Admittedly, for the current pcase-lambda (and pcase-let) macro, the
> pcase.el code should be refined so as to emit a warning when it ends up
> ignoring a constant like `fullsweep_after' above.

Yes, I am having a lot of trouble understanding and using pcase-let*
except for trivial cases. I don't want the same thing to happen to
pcase-lambda.

>        Stefan

Thanks,
Leo

bug#20268: 25.0.50; pcase-lambda broken

[Stefan Monnier]

> I don't mind new semantics but I want to make sense of it so as to use
> it with confidence. What I am seeing is:
> (funcall (pcase-lambda (`[fullsweep_after ,v]) v) [min_bin_vheap_size 46422])

When destructuring (as opposed to performing case-analysis), pcase.el
takes as a given that the pattern does match, so the pattern is
basically only used to decide from where to extract the information to
bind the variables.  So the following patterns are equivalent:

   `[fullsweep_after ,v]

   `[,_ ,v]

   `[,(pred foo) ,v]

If you want to test that the pattern matches, that means someone needs
to decide what happens when the pattern doesn't match.  The previous
behavior was to "do nothing and return nil", which is too arbitrary for
my taste, so if you want that, you need to write it

   (lambda (x) (pcase (`[fullsweep_after ,v] v)))


-- Stefan

bug#20268: 25.0.50; pcase-lambda broken

[Stefan Monnier]

> After the recent rewrite, pcase-lambda is broken. For example, eval the
> following to get 46422 instead of the correct value 65535.

>    (cl-some (pcase-lambda (`[fullsweep_after ,v]) v)
>             '([min_bin_vheap_size 46422]
>               [min_heap_size 233]
>               [fullsweep_after 65535]
>               [minor_gcs 40]))

Indeed, that's the semantics I chose.
The previous semantics was for the function to do nothing and return nil
if the arg doesn't match.  The new semantics is the same as the one used
by pcase-let.  It's not without its fault of course, but at least it does
correspond to the usual idea of "destructuring" and generates more
efficient code.

I think if you prefer to return nil, then the macro should look like

   (pcase-lambda ((`[fullsweep_after ,v]) v))

which would then naturally let you add additional cases like

   (pcase-lambda ((`[fullsweep_after ,v]) v)
                 ((`[min_heap_size ,v]) (/ v 2)))

Admittedly, for the current pcase-lambda (and pcase-let) macro, the
pcase.el code should be refined so as to emit a warning when it ends up
ignoring a constant like `fullsweep_after' above.


        Stefan

bug#20268: 25.0.50; pcase-lambda broken

[Andy Moreton]

On Tue 07 Apr 2015, Stefan Monnier wrote:

>> After the recent rewrite, pcase-lambda is broken. For example, eval the
>> following to get 46422 instead of the correct value 65535.
>
>>    (cl-some (pcase-lambda (`[fullsweep_after ,v]) v)
>>             '([min_bin_vheap_size 46422]
>>               [min_heap_size 233]
>>               [fullsweep_after 65535]
>>               [minor_gcs 40]))
>
> Indeed, that's the semantics I chose.
> The previous semantics was for the function to do nothing and return nil
> if the arg doesn't match.  The new semantics is the same as the one used
> by pcase-let.  It's not without its fault of course, but at least it does
> correspond to the usual idea of "destructuring" and generates more
> efficient code.

Please improve the documentation for the pcase macros:

 a) The elisp manual has reference documentation for pcase, but it is
    confusing and short on examples. The first example would be clearer
    if it was more like the second example, by showing some example
    forms that invoke pcase and the result of evaluating them.

    The description would flow more logically if the second example
    followed the reference that describes the allowed patterns, and 
    included an example of each type of pattern.

 b) Add meaningful help strings for pcase-lambda, pcase-let* and
    pcase-let. The existing help strings all say that these constructs
    are "the same as another thing only different" which only serves to
    obscure what they do. A user should be able to discern what the
    interface contract is without reading the implementation. A short
    motivating example for each macro would be helpful.

These improvements would make code using the pcase macros more readable,
and aid understanding of the intended semantics.

    AndyM

bug#20268: 25.0.50; pcase-lambda broken

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 1635 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> schrieb am Mi., 8. Apr. 2015 um
04:15 Uhr:

> > After the recent rewrite, pcase-lambda is broken. For example, eval the
> > following to get 46422 instead of the correct value 65535.
>
> >    (cl-some (pcase-lambda (`[fullsweep_after ,v]) v)
> >             '([min_bin_vheap_size 46422]
> >               [min_heap_size 233]
> >               [fullsweep_after 65535]
> >               [minor_gcs 40]))
>
> Indeed, that's the semantics I chose.
> The previous semantics was for the function to do nothing and return nil
> if the arg doesn't match.  The new semantics is the same as the one used
> by pcase-let.  It's not without its fault of course, but at least it does
> correspond to the usual idea of "destructuring" and generates more
> efficient code.
>
> I think if you prefer to return nil, then the macro should look like
>
>    (pcase-lambda ((`[fullsweep_after ,v]) v))
>
> which would then naturally let you add additional cases like
>
>    (pcase-lambda ((`[fullsweep_after ,v]) v)
>                  ((`[min_heap_size ,v]) (/ v 2)))
>
> Admittedly, for the current pcase-lambda (and pcase-let) macro, the
> pcase.el code should be refined so as to emit a warning when it ends up
> ignoring a constant like `fullsweep_after' above.
>
>
Why not raise an error instead if there is no match (or even if any quoted
or self-quoting expressions are used at all)? The current behavior is quite
confusing, and warnings tend to be ignored.

Given that pcase-let, pcase-lambda and pcase-dolist don't do any
case-by-case analysis, maybe they should even be renamed and moved out of
pcase.el.

[-- Attachment #2: Type: text/html, Size: 2114 bytes --]

bug#20268: 25.0.50; pcase-lambda broken

[Stefan Monnier]

> Why not raise an error instead if there is no match

That would be an acceptable semantics, yes.  I happen to dislike it
because of the performance cost, where in most use-cases you know the
pattern will match and just want to replace a bunch of car/cdr/aref with
a single pattern.

I.e. the code you'd have written without pcase-let would have been

      (let ((a (foo))
            (b (car foo))
            (c (nth 2 foo)))

which would naturally turn into a pattern like `(,b ,_ ,c)
but if you want to signal an error when the pattern fails to match, then
you get 3 additional consp tests plus a null test, at which point the
performance impact can become noticeable.  You can get rid of the null
test with a pattern like `(,b ,_ ,c . ,_) but you can't get rid of the
3 consp tests.


        Stefan

bug#20268: 25.0.50; pcase-lambda broken

[Drew Adams]

> I.e. the code you'd have written without pcase-let would have been
> 
>       (let ((a (foo))
>             (b (car foo))
>             (c (nth 2 foo)))
> 
> which would naturally turn into a pattern like `(,b ,_ ,c)

Just out of curiosity, what was the reason we didn't go in the
direction of Common Lisp's `destructuring-bind' etc., which would
let you write just (a b c) instead of `(,b ,_ ,c)?

bug#20268: 25.0.50; pcase-lambda broken

[Stefan Monnier]

> Just out of curiosity, what was the reason we didn't go in the
> direction of Common Lisp's `destructuring-bind' etc., which would
> let you write just (a b c) instead of `(,b ,_ ,c)?

For example because it doesn't let you write

   (pcase-let ((cl-struct mytype slot1 slot3) (myexp))
     ...)


-- Stefan

bug#20268: 25.0.50; pcase-lambda broken

[Drew Adams]

> > Just out of curiosity, what was the reason we didn't go in the
> > direction of Common Lisp's `destructuring-bind' etc., which would
> > let you write just (a b c) instead of `(,b ,_ ,c)?
> 
> For example because it doesn't let you write
>    (pcase-let ((cl-struct mytype slot1 slot3) (myexp)) ...)

What part of "go in the direction of Common Lisp's
`destructuring-bind' etc." implies that you cannot go beyond it
to support that kind of thing?

bug#20268: 25.0.50; pcase-lambda broken

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> After the recent rewrite, pcase-lambda is broken. For example, eval the
>> following to get 46422 instead of the correct value 65535.
>
>>    (cl-some (pcase-lambda (`[fullsweep_after ,v]) v)
>>             '([min_bin_vheap_size 46422]
>>               [min_heap_size 233]
>>               [fullsweep_after 65535]
>>               [minor_gcs 40]))
>
> Indeed, that's the semantics I chose.

So I think everything here works as designed, and I'm therefore closing
this bug report.

I see that pcase-lambda wasn't documented in the manual, so I've now
done so in Emacs 29.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no