bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Ihor Radchenko]

Hi,

Consider the following

(pcase-dolist (`(,(and (pred stringp) a) .
		 ,(and (pred stringp) b))
	       '(("TODO") ("DONE" . "a")))
  (warn "%S :: %S" a b))

Executing the above yields

⛔ Warning (emacs): "TODO" :: nil
⛔ Warning (emacs): "DONE" :: "a"

even though ("TODO") does not match the pattern.

In contrast

(dolist (el '(("TODO") ("DONE" . "a")))
  (pcase el
    (`(,(and (pred stringp) a) .
       ,(and (pred stringp) b))
     (warn "%S :: %S" a b))))

correctly displays

⛔ Warning (emacs): "DONE" :: "a"


In GNU Emacs 30.0.50 (build 8, x86_64-pc-linux-gnu, GTK+ Version
 3.24.39, cairo version 1.18.0) of 2024-01-16 built on localhost
Repository revision: 5f73258753ee91c8648e0be55332f7dbaea9fa8b
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101010
System Description: Gentoo Linux


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Jim Porter]

On 1/16/2024 7:07 AM, Ihor Radchenko wrote:
> Consider the following
> 
> (pcase-dolist (`(,(and (pred stringp) a) .
> 		 ,(and (pred stringp) b))
> 	       '(("TODO") ("DONE" . "a")))
>    (warn "%S :: %S" a b))
> 
> Executing the above yields
> 
> ⛔ Warning (emacs): "TODO" :: nil
> ⛔ Warning (emacs): "DONE" :: "a"
> 
> even though ("TODO") does not match the pattern.

This isn't an issue with 'pcase-dolist', but rather a known/intentional 
limitation of 'pcase-let':

(pcase-let ((`(,(and (pred stringp) a) .
                ,(and (pred stringp) b))
              '("TODO")))
   (warn "%S :: %S" a b))
   -> Warning (emacs): "TODO" :: nil

The 'pcase-let' docstring says this:

> Each EXP should match (i.e. be of compatible structure) to its
> respective PATTERN; a mismatch may signal an error or may go
> undetected, binding variables to arbitrary values, such as nil.

I do think we should fix it somehow though. This behavior is extremely 
confusing, and as much as I'm a fan of 'pcase', I'm emphatically *not* a 
fan of how this part works.

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Ihor Radchenko]

Jim Porter <jporterbugs@gmail.com> writes:

> This isn't an issue with 'pcase-dolist', but rather a known/intentional 
> limitation of 'pcase-let':
>
> (pcase-let ((`(,(and (pred stringp) a) .
>                 ,(and (pred stringp) b))
>               '("TODO")))
>    (warn "%S :: %S" a b))
>    -> Warning (emacs): "TODO" :: nil
>
> The 'pcase-let' docstring says this:
>
>> Each EXP should match (i.e. be of compatible structure) to its
>> respective PATTERN; a mismatch may signal an error or may go
>> undetected, binding variables to arbitrary values, such as nil.
>
> I do think we should fix it somehow though. This behavior is extremely 
> confusing, and as much as I'm a fan of 'pcase', I'm emphatically *not* a 
> fan of how this part works.

Not sure about pcase-let, but pcase-dolist specifically may be
simplified not to use pcase-let:

(if (pcase--trivial-upat-p (car spec))
      `(dolist ,spec ,@body)
    (let ((tmpvar (gensym "x")))
      `(dolist (,tmpvar ,@(cdr spec))
         (pcase ,tmpvar (,(car spec) ,@body)))))

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Ihor Radchenko]

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

Ihor Radchenko <yantar92@posteo.net> writes:

> Not sure about pcase-let, but pcase-dolist specifically may be
> simplified not to use pcase-let:
>
> (if (pcase--trivial-upat-p (car spec))
>       `(dolist ,spec ,@body)
>     (let ((tmpvar (gensym "x")))
>       `(dolist (,tmpvar ,@(cdr spec))
>          (pcase ,tmpvar (,(car spec) ,@body)))))

See the attached patch.
If the patch is acceptable, we also need to update the manual.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-pcase-let-Skip-LIST-element-that-do-not-match-the-PA.patch --]
[-- Type: text/x-patch, Size: 3056 bytes --]

From f3e1362f7687c731e0ba4e410f005252309ffc3f Mon Sep 17 00:00:00 2001
Message-ID: <f3e1362f7687c731e0ba4e410f005252309ffc3f.1708337064.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Mon, 19 Feb 2024 13:02:21 +0300
Subject: [PATCH] pcase-let: Skip LIST element that do not match the PATTERN
 (bug#68509)

* lisp/emacs-lisp/pcase.el (pcase-dolist): Use `pcase' rather than
`pcase-let*' to match the list elements.  Update the docstring,
describing the behavior when list elements to not match the pattern.
The previous undefined behavior is removed.
* test/lisp/emacs-lisp/pcase-tests.el (pcase-tests-pcase-dolist): Add
new test.
---
 lisp/emacs-lisp/pcase.el            | 13 ++++++-------
 test/lisp/emacs-lisp/pcase-tests.el | 12 ++++++++++++
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/lisp/emacs-lisp/pcase.el b/lisp/emacs-lisp/pcase.el
index ae9bd87997c..8dc11b20a6f 100644
--- a/lisp/emacs-lisp/pcase.el
+++ b/lisp/emacs-lisp/pcase.el
@@ -329,21 +329,20 @@ pcase-let
 (defmacro pcase-dolist (spec &rest body)
   "Eval BODY once for each set of bindings defined by PATTERN and LIST elements.
 PATTERN should be a `pcase' pattern describing the structure of
-LIST elements, and LIST is a list of objects that match PATTERN,
-i.e. have a structure that is compatible with PATTERN.
+LIST elements, and LIST is a list of objects.
 For each element of LIST, this macro binds the variables in
 PATTERN to the corresponding subfields of the LIST element, and
-then evaluates BODY with these bindings in effect.  The
-destructuring bindings of variables in PATTERN to the subfields
-of the elements of LIST is performed as if by `pcase-let'.
+then evaluates BODY with these bindings in effect.  When an element does
+not match the pattern, such element is skipped.
+The destructuring bindings of variables in PATTERN to the subfields
+of the elements of LIST is performed as if by `pcase'.
 \n(fn (PATTERN LIST) BODY...)"
   (declare (indent 1) (debug ((pcase-PAT form) body)))
   (if (pcase--trivial-upat-p (car spec))
       `(dolist ,spec ,@body)
     (let ((tmpvar (gensym "x")))
       `(dolist (,tmpvar ,@(cdr spec))
-         (pcase-let* ((,(car spec) ,tmpvar))
-           ,@body)))))
+	 (pcase ,tmpvar (,(car spec) ,@body))))))
 
 ;;;###autoload
 (defmacro pcase-setq (pat val &rest args)
diff --git a/test/lisp/emacs-lisp/pcase-tests.el b/test/lisp/emacs-lisp/pcase-tests.el
index d062965952a..241729c108a 100644
--- a/test/lisp/emacs-lisp/pcase-tests.el
+++ b/test/lisp/emacs-lisp/pcase-tests.el
@@ -160,4 +160,16 @@ pcase-tests-setq
   (should-error (pcase-setq a)
                 :type '(wrong-number-of-arguments)))
 
+(ert-deftest pcase-tests-pcase-dolist ()
+  ;; Ignore non-matching elements.
+  (should
+   (equal
+    '(("DONE" . "a"))
+    (let (result)
+      (pcase-dolist (`(,(and (pred stringp) a) .
+		       ,(and (pred stringp) b))
+	             '(("TODO") ("DONE" . "a")))
+        (push (cons a b) result))
+      (nreverse result)))))
+
 ;;; pcase-tests.el ends here.
-- 
2.43.0


[-- Attachment #3: Type: text/plain, Size: 224 bytes --]


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Eli Zaretskii]

> Cc: 68509@debbugs.gnu.org
> From: Ihor Radchenko <yantar92@posteo.net>
> Date: Mon, 19 Feb 2024 10:05:33 +0000
> 
> Ihor Radchenko <yantar92@posteo.net> writes:
> 
> > Not sure about pcase-let, but pcase-dolist specifically may be
> > simplified not to use pcase-let:
> >
> > (if (pcase--trivial-upat-p (car spec))
> >       `(dolist ,spec ,@body)
> >     (let ((tmpvar (gensym "x")))
> >       `(dolist (,tmpvar ,@(cdr spec))
> >          (pcase ,tmpvar (,(car spec) ,@body)))))
> 
> See the attached patch.
> If the patch is acceptable, we also need to update the manual.

Thanks.  Adding Stefan to the discussion.

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Consider the following
>> (pcase-dolist (`(,(and (pred stringp) a) .
>> 		 ,(and (pred stringp) b))
>> 	       '(("TODO") ("DONE" . "a")))
>>    (warn "%S :: %S" a b))
>> Executing the above yields
>> ⛔ Warning (emacs): "TODO" :: nil
>> ⛔ Warning (emacs): "DONE" :: "a"
>> even though ("TODO") does not match the pattern.
>
> This isn't an issue with 'pcase-dolist', but rather a known/intentional
> limitation of 'pcase-let':

Indeed, I consider the above a pilot error.
`pcase-dolist` and `pcase-let` use Pcase patterns to do *destructuring*,
which is a different task than the one done by `pcase` (which decides
whether a value matches a pattern or not).

>> Each EXP should match (i.e. be of compatible structure) to its
>> respective PATTERN; a mismatch may signal an error or may go
>> undetected, binding variables to arbitrary values, such as nil.
> I do think we should fix it somehow though.  This behavior is
> extremely confusing, and as much as I'm a fan of 'pcase', I'm
> emphatically *not* a fan of how this part works.

I'm quite happy with the way it works when you use it as intended.
But I'm not really satisfied either with the way it behaves when the
coder doesn't understand its semantics, nor about the way we document
that semantics.

The difficulty in resolving this can be illustrated with the following
pattern:

    `(a . ,b)

This pattern leads to two tests: (consp VAL) and (eq 'a (car VAL)).
When destructuring, we want to throw away both tests (we want to throw
away most tests, except those needed to choose between two `or`
branches).

We could decide to emit a warning because we silently skip
the `eq` test, which would help the coders understand that the pattern
doesn't do what they think.
But emitting that same warning because we silently skip the `consp` test
would be really annoying because rewriting the pattern to avoid this
is impractical.

For a human, it's pretty easy to distinguish those two cases.  But it's
difficult to provide a precise definition that distinguishes those two cases.

We could also keep the tests and emit a warning or even an error when
they fail, but if that's the behavior you want, then you should
arguably use `pcase(-exhaustive)` instead.


        Stefan

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> * lisp/emacs-lisp/pcase.el (pcase-dolist): Use `pcase' rather than
> `pcase-let*' to match the list elements.  Update the docstring,

That changes its behavior.  It *will* break code.
This is basically not the same macro any more.  Instead of being a macro
that iterates over all the elements of the list, it becomes a macro
which iterates only over the matching elements.


        Stefan

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Ihor Radchenko]

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

>> * lisp/emacs-lisp/pcase.el (pcase-dolist): Use `pcase' rather than
>> `pcase-let*' to match the list elements.  Update the docstring,
>
> That changes its behavior.  It *will* break code.
> This is basically not the same macro any more.  Instead of being a macro
> that iterates over all the elements of the list, it becomes a macro
> which iterates only over the matching elements.

Currently, the docstring states:

    PATTERN should be a `pcase' pattern describing the structure of
    LIST elements, and LIST is a list of objects that match PATTERN,
    i.e. have a structure that is compatible with PATTERN.

It is undefined what happens when LIST objects do not match PATTERN.

So, any code relying upon iterating over non-matching elements is
relying upon undefined, undocumented behaviour.

Moreover, current implementation binds /partially matching/ list
elements, which is even more fragile. From `pcase-let' docstring:

    a mismatch may signal an error or may go
    undetected, binding variables to arbitrary values, such as nil.

What I suggest is to change what happens when PATTERN does not much more
explicitly.

In my patch, I proposed to skip such elements.
An alternative could be throwing an error.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Ihor Radchenko]

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

> The difficulty in resolving this can be illustrated with the following
> pattern:
>
>     `(a . ,b)
>
> This pattern leads to two tests: (consp VAL) and (eq 'a (car VAL)).
> When destructuring, we want to throw away both tests (we want to throw
> away most tests, except those needed to choose between two `or`
> branches).
>
> We could decide to emit a warning because we silently skip
> the `eq` test, which would help the coders understand that the pattern
> doesn't do what they think.
> But emitting that same warning because we silently skip the `consp` test
> would be really annoying because rewriting the pattern to avoid this
> is impractical.

May you please elaborate how this example is related to destructuring
from user perspective? It appears to me that you are talking about
implementation details of `pcase', which are elusive to me.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Ihor Radchenko [2024-02-19 18:14:24] wrote:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> * lisp/emacs-lisp/pcase.el (pcase-dolist): Use `pcase' rather than
>>> `pcase-let*' to match the list elements.  Update the docstring,
>>
>> That changes its behavior.  It *will* break code.
>> This is basically not the same macro any more.  Instead of being a macro
>> that iterates over all the elements of the list, it becomes a macro
>> which iterates only over the matching elements.
>
> Currently, the docstring states:
>
>     PATTERN should be a `pcase' pattern describing the structure of
>     LIST elements, and LIST is a list of objects that match PATTERN,
>     i.e. have a structure that is compatible with PATTERN.

The doc for `pcase-let` is a bit more precise:

    Each EXP should match (i.e. be of compatible structure) to its
    respective PATTERN; a mismatch may signal an error or may go
    undetected, binding variables to arbitrary values, such as nil.

> In my patch, I proposed to skip such elements.
> An alternative could be throwing an error.

As I said, what you propose is a *different* construct.
Maybe we should add some extra option/keyword to `pcase-dolist` to
indicate what to do with elements that don't match (such as signaling
an error, a warning, silently skipping it, ...).

Similar issues affect `pcase-let`, of course.


        Stefan

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> The difficulty in resolving this can be illustrated with the following
>> pattern:
>>
>>     `(a . ,b)
>>
>> This pattern leads to two tests: (consp VAL) and (eq 'a (car VAL)).
>> When destructuring, we want to throw away both tests (we want to throw
>> away most tests, except those needed to choose between two `or`
>> branches).
>>
>> We could decide to emit a warning because we silently skip
>> the `eq` test, which would help the coders understand that the pattern
>> doesn't do what they think.
>> But emitting that same warning because we silently skip the `consp` test
>> would be really annoying because rewriting the pattern to avoid this
>> is impractical.
>
> May you please elaborate how this example is related to destructuring
> from user perspective? It appears to me that you are talking about
> implementation details of `pcase', which are elusive to me.

I'm just reading aloud what the `(a . ,b) pattern means: it means "VAL
has to be a cons, and its car should be equal to `a`" (and in addition
to that, we'll extract the `cdr` and put it into the `b` variable).

We might want to warn the user that such a pattern in `pcase-let` and
`pcase-dolist` is probably an error because the `a` won't actually be
checked, so it will behave just like the `(,_ . ,b) pattern.

But warning the user that the `consp` check will also be dropped would
be annoying because it's not like the users can easily write a different
pattern that still behaves the same but does not imply such
a `consp` check.


        Stefan

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Ihor Radchenko]

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

>> In my patch, I proposed to skip such elements.
>> An alternative could be throwing an error.
>
> As I said, what you propose is a *different* construct.
> Maybe we should add some extra option/keyword to `pcase-dolist` to
> indicate what to do with elements that don't match (such as signaling
> an error, a warning, silently skipping it, ...).
>
> Similar issues affect `pcase-let`, of course.

What about adding extra option/keyword and also showing a runtime
warning when pcase-dolist/pcase-let encounter something that does not
match the pattern? In such situation, the code logic probably has a
problem that is worth highlighting.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> What about adding extra option/keyword and also showing a runtime
> warning when pcase-dolist/pcase-let encounter something that does not
> match the pattern?

In the current use of those primitives, it's routine/normal/common for
a pattern like `(,a ,b) to encounter lists of different length than 2,
so a warning should be emitted only if specifically requested via an
option/keyword.

I think it would be OK to emit a warning (even if not specifically
requested via an option/keyword) when a value like (help . me) fails to
match `(a . ,b), but as mentioned I don't know a good way to distinguish
those two kinds of cases.


        Stefan

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Ihor Radchenko]

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

>> What about adding extra option/keyword and also showing a runtime
>> warning when pcase-dolist/pcase-let encounter something that does not
>> match the pattern?
>
> In the current use of those primitives, it's routine/normal/common for
> a pattern like `(,a ,b) to encounter lists of different length than 2,
> so a warning should be emitted only if specifically requested via an
> option/keyword.

But isn't it undocumented?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> What about adding extra option/keyword and also showing a runtime
>>> warning when pcase-dolist/pcase-let encounter something that does not
>>> match the pattern?
>> In the current use of those primitives, it's routine/normal/common for
>> a pattern like `(,a ,b) to encounter lists of different length than 2,
>> so a warning should be emitted only if specifically requested via an
>> option/keyword.
> But isn't it undocumented?

AFAIK the "best" documentation we have for that is the one in
`pcase-let`, which is indeed unsatisfactory.


        Stefan

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Ihor Radchenko]

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

>>> In the current use of those primitives, it's routine/normal/common for
>>> a pattern like `(,a ,b) to encounter lists of different length than 2,
>>> so a warning should be emitted only if specifically requested via an
>>> option/keyword.
>> But isn't it undocumented?
>
> AFAIK the "best" documentation we have for that is the one in
> `pcase-let`, which is indeed unsatisfactory.

May you then describe in more details what is the intended behavior when
PATTERN does not match VALUE in destructuring?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>>> In the current use of those primitives, it's routine/normal/common for
>>>> a pattern like `(,a ,b) to encounter lists of different length than 2,
>>>> so a warning should be emitted only if specifically requested via an
>>>> option/keyword.
>>> But isn't it undocumented?
>>
>> AFAIK the "best" documentation we have for that is the one in
>> `pcase-let`, which is indeed unsatisfactory.
>
> May you then describe in more details what is the intended behavior when
> PATTERN does not match VALUE in destructuring?

How 'bout the patch below?


        Stefan


diff --git a/doc/lispref/control.texi b/doc/lispref/control.texi
index 78ad5b68a51..cf60cabe52b 100644
--- a/doc/lispref/control.texi
+++ b/doc/lispref/control.texi
@@ -1317,11 +1317,18 @@ Destructuring with pcase Patterns
 does the same as the previous example, except that it directly tries
 to extract @code{x} and @code{y} from @code{my-list} without first
 verifying if @code{my-list} is a list which has the right number of
-elements and has @code{add} as its first element.  The precise
-behavior when the object does not actually match the pattern is
-undefined, although the body will not be silently skipped: either an
-error is signaled or the body is run with some of the variables
-potentially bound to arbitrary values like @code{nil}.
+elements and has @code{add} as its first element.
+
+The precise behavior when the object does not actually match the pattern
+depends on the types, although the body will not be silently skipped:
+either an error is signaled or the body is run with some of the
+variables bound to arbitrary values like @code{nil}.
+For example, the above pattern will result in @var{x} and @var{y}
+being extracted with operations like @code{car} or @code{nth}, so they
+will get value @code{nil} when @var{my-list} is too short.  In contrast,
+with a pattern like @code{`[add ,x ,y]}, those same variables would
+be extracted using @code{aref} which would signal an error if
+@var{my-list} is not an array or is too short.
 
 The pcase patterns that are useful for destructuring bindings are
 generally those described in @ref{Backquote Patterns}, since they

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Ihor Radchenko]

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

>> May you then describe in more details what is the intended behavior when
>> PATTERN does not match VALUE in destructuring?
>
> How 'bout the patch below?
> ...
> +The precise behavior when the object does not actually match the pattern
> +depends on the types, although the body will not be silently skipped:
> +either an error is signaled or the body is run with some of the
> +variables bound to arbitrary values like @code{nil}.
> +For example, the above pattern will result in @var{x} and @var{y}
> +being extracted with operations like @code{car} or @code{nth}, so they
> +will get value @code{nil} when @var{my-list} is too short.  In contrast,
> +with a pattern like @code{`[add ,x ,y]}, those same variables would
> +be extracted using @code{aref} which would signal an error if
> +@var{my-list} is not an array or is too short.

This is confusing. In particular, "for example" implies that other
situations are possible. It is unclear how the reader will know when
"variables bound to arbitrary values" happen, when variables are bound
to nil, and when an error is signaled.

I would re-structure the paragraph, explicitly listing the possible
outcomes when the PATTERN does not match VALUE and stating that any
other non-matching PATTERN will lead to undefined outcome.

Also, with the current behavior of `pcase-let', it appears to be
impossible to distinguish between too short list (1 2 3) and
(1 2 3 nil nil) when assigning destructuring pattern.


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> How 'bout the patch below?
>> ...
>> +The precise behavior when the object does not actually match the pattern
>> +depends on the types, although the body will not be silently skipped:
                   ^^^^^
                  pattern

>> +either an error is signaled or the body is run with some of the
>> +variables bound to arbitrary values like @code{nil}.
>> +For example, the above pattern will result in @var{x} and @var{y}
>> +being extracted with operations like @code{car} or @code{nth}, so they
>> +will get value @code{nil} when @var{my-list} is too short.  In contrast,
>> +with a pattern like @code{`[add ,x ,y]}, those same variables would
>> +be extracted using @code{aref} which would signal an error if
>> +@var{my-list} is not an array or is too short.
>
> This is confusing. In particular, "for example" implies that other
> situations are possible. It is unclear how the reader will know when
> "variables bound to arbitrary values" happen, when variables are bound
> to nil, and when an error is signaled.

This part of the doc is generic, so it can't guarantee anything: the
behavior depends on the actual pattern and the way that pattern is defined.
In order to intuit what will happen, the reader needs to guess which
primitives are used to extract the info from value (e.g. `car/cdr/nth`
for cons cells, `aref` for arrays, ...).

> I would re-structure the paragraph, explicitly listing the possible
> outcomes when the PATTERN does not match VALUE and stating that any
> other non-matching PATTERN will lead to undefined outcome.

Could you try to do that?
[ I've found back-and-forth modifications of docs give better results
  than single-sided writing.  ]

> Also, with the current behavior of `pcase-let', it appears to be
> impossible to distinguish between too short list (1 2 3) and
> (1 2 3 nil nil) when assigning destructuring pattern.

Destructuring  by definition can't do "distinguish", so that's not a big
surprise 🙂

But of course, this isn't really true, you can do:

    (pcase-let ((`(,x ,y ,z . ,tail) FOO))
      (if (equal tail '(nil nil))
          ...
        ...))

or you can get hairy and do things like:

    (pcase-let (((or `(,x ,y ,z ,a ,_)
                     (and `(,x ,y ,z) (let a 'shortorlonglist)))
                 FOO))
      (if (eq a 'shortorlonglist)
          ...
        ...))

but that kind if Pcase pattern, while possible, is rarely worth the trouble.


        Stefan

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Ihor Radchenko]

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

>>> +For example, the above pattern will result in...
>>
>> This is confusing. In particular, "for example" implies that other
>> situations are possible. It is unclear how the reader will know when
>> "variables bound to arbitrary values" happen, when variables are bound
>> to nil, and when an error is signaled.
>
> This part of the doc is generic, so it can't guarantee anything: the
> behavior depends on the actual pattern and the way that pattern is defined.
> In order to intuit what will happen, the reader needs to guess which
> primitives are used to extract the info from value (e.g. `car/cdr/nth`
> for cons cells, `aref` for arrays, ...).

I do not think that "guess" is what we should ever put into docs.
Because implementation details may change and any kind of guess may then
go wrong.

>> I would re-structure the paragraph, explicitly listing the possible
>> outcomes when the PATTERN does not match VALUE and stating that any
>> other non-matching PATTERN will lead to undefined outcome.
>
> Could you try to do that?
> [ I've found back-and-forth modifications of docs give better results
>   than single-sided writing.  ]

I could, but I am not sure which non-matching outcomes we should
document and which we want to leave undefined.

I still don't have a clear picture about the differences between normal
pcase and destructuring pcase. Especially for non-trivial patterns.
For example,

(pcase-let (((and `(,a ,b ,c) (guard (eq a 'foo))) '(bar a)))
  (list a b c)) ; =>'(bar a nil)

is extremely confusing.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#68509: 30.0.50; pcase-dolist matches backquote pattern incorrectly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Could you try to do that?
>> [ I've found back-and-forth modifications of docs give better results
>>   than single-sided writing.  ]
>
> I could, but I am not sure which non-matching outcomes we should
> document and which we want to leave undefined.

Try your best.  Then I'll try to improve it, then you'll try to improve it...

> I still don't have a clear picture about the differences between normal
> pcase and destructuring pcase.

The best way to clarify it is by writing what you think happens.
Don't worry about the risk that what you write is "wrong", because
that's the part I can easily catch and fix.


        Stefan