bug#72328: [PATCH] Nested backquote in pcase

bug#72328: [PATCH] Nested backquote in pcase

[Thuna]

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

Assuming a hypothetical `(pcase-assert OBJ PAT)' I would expect the
following to succeed:

  (pcase-assert '`,nil ``,nil)
  (pcase-assert '`,1 ``,,(pred integerp))
  (pcase-assert '`,@(list) ``,@,`(list . ,_))

which is to say: a comma should match its argument as a pattern only if
it escapes the original backquote (currently comma at all depths
"escape" the original backquote).

In case there is any interest in this, I have prepared a patch which
both adds the above as tests to pcase-tests.el and implements them in
pcase.el.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Handle-nested-backquotes-in-pcase.patch --]
[-- Type: text/x-patch, Size: 3037 bytes --]

From 9e7ab85e84bbdaf2520dee52aeec8b650b349c6a Mon Sep 17 00:00:00 2001
From: Thuna <thuna.cing@gmail.com>
Date: Sun, 28 Jul 2024 02:34:20 +0200
Subject: [PATCH] Handle nested backquotes in pcase

* lisp/emacs-lisp/pcase.el (pcase--expand-\`): Treat nested commas and
comma-ats as literal symbol matches, only consider their arguments as
patterns (or error in the case of comma-at) if the comma escapes the
original backquote.
* test/lisp/emacs-lisp/pcase-tests.el (pcase-tests-nested-backquote):
Add tests for the behavior of nested backquote patterns.
---
 lisp/emacs-lisp/pcase.el            | 17 +++++++++++------
 test/lisp/emacs-lisp/pcase-tests.el |  5 +++++
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/lisp/emacs-lisp/pcase.el b/lisp/emacs-lisp/pcase.el
index 5a7f3995311..a4c855432ce 100644
--- a/lisp/emacs-lisp/pcase.el
+++ b/lisp/emacs-lisp/pcase.el
@@ -1142,17 +1142,18 @@ \`
   (declare (debug (pcase-QPAT)))
   (pcase--expand-\` qpat))
 
-(defun pcase--expand-\` (qpat)
+(defun pcase--expand-\` (qpat &optional depth)
+  (unless depth (setq depth 0))
   (cond
-   ((eq (car-safe qpat) '\,) (cadr qpat))
-   ((or (eq (car-safe qpat) '\,@) (eq qpat '...))
+   ((and (<= depth 0) (eq (car-safe qpat) '\,)) (cadr qpat))
+   ((and (<= depth 0) (or (eq (car-safe qpat) '\,@) (eq qpat '...)))
     (error "Unsupported QPAT: %S" qpat))
    ((vectorp qpat)
     (let* ((trivial t)
            (contents nil)
            (upats nil))
       (dotimes (i (length qpat))
-        (let* ((upat (pcase--expand-\` (aref qpat i))))
+        (let* ((upat (pcase--expand-\` (aref qpat i) depth)))
           (if (eq (car-safe upat) 'quote)
               (push (cadr upat) contents)
             (setq trivial nil))
@@ -1163,8 +1164,12 @@ pcase--expand-\`
               (app length ,(length qpat))
               ,@(nreverse upats)))))
    ((consp qpat)
-    (let ((upata (pcase--expand-\` (car qpat)))
-          (upatd (pcase--expand-\` (cdr qpat))))
+    (let ((upata (pcase--expand-\` (car qpat) depth))
+          (upatd (pcase--expand-\`
+                  (cdr qpat)
+                  (cond ((eq (car qpat) '\`) (1+ depth))
+                        ((memq (car qpat) '(\, \,@)) (1- depth))
+                        (t depth)))))
       (if (and (eq (car-safe upata) 'quote) (eq (car-safe upatd) 'quote))
           `'(,(cadr upata) . ,(cadr upatd))
         `(and (pred consp)
diff --git a/test/lisp/emacs-lisp/pcase-tests.el b/test/lisp/emacs-lisp/pcase-tests.el
index e777b71920c..2649e754beb 100644
--- a/test/lisp/emacs-lisp/pcase-tests.el
+++ b/test/lisp/emacs-lisp/pcase-tests.el
@@ -191,4 +191,9 @@ pcase-tests-mutually-exclusive
         (should (pcase--mutually-exclusive-p (nth 1 x) (nth 0 x)))
       (should-not (pcase--mutually-exclusive-p (nth 1 x) (nth 0 x))))))
 
+(ert-deftest pcase-tests-nested-backquote ()
+  (should (pcase '`,nil (``,nil t)))
+  (should (pcase '`,1 (``,,(pred integerp) t)))
+  (should (pcase '`,@(list) (``,@,`(list . ,_) t))))
+
 ;;; pcase-tests.el ends here.
-- 
2.44.2

bug#72328: [PATCH] Nested backquote in pcase

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

Hello Thuna,

> a comma should match its argument as a pattern only if it escapes the
> original backquote (currently comma at all depths "escape" the
> original backquote).

why do you want to behave `pcase' like that?

This would be a backward-incompatible change, breaking existing code.

And this change would break the symmetry between `backquote' the macro
and backquote patterns in `pcase'.  This is an important design idea.
And I don't see any advantage either.


Michael.

bug#72328: [PATCH] Nested backquote in pcase

[Thuna]

> This would be a backward-incompatible change, breaking existing code.

Yes, I forgot to mention this in my original report, but given how
unintuitive (IMO) the current behavior is, I expect that very few people
will be using patterns which would be effected by this change. 

> And this change would break the symmetry between `backquote' the macro
> and backquote patterns in `pcase'.  This is an important design idea.

I am not quite sure why you think that this breaks symmetry with the
backquote macro; the purpose of this patch was to establish a symmetry
in the first place.  Would you consider the examples provided as
unnatural?

> And I don't see any advantage either.

AFAICT right now the only way to match an unevaluated (or quoted) `,foo
is with `(,'\` (,'\, foo)) which is quite unideal.  It is not often that
you need this but having this not be representable in this way is not
particularly useful.

bug#72328: [PATCH] Nested backquote in pcase

[Eli Zaretskii]

> Cc: 72328@debbugs.gnu.org
> From: Thuna <thuna.cing@gmail.com>
> Date: Sun, 28 Jul 2024 17:51:17 +0200
> 
> 
> > This would be a backward-incompatible change, breaking existing code.
> 
> Yes, I forgot to mention this in my original report, but given how
> unintuitive (IMO) the current behavior is, I expect that very few people
> will be using patterns which would be effected by this change. 

Famous last words ;-)

Bitter experience has taught us that this kind of arguments invariably
causes changes that break someone's code or setup.  So we try to do
that only if absolutely necessary.  Which doesn't seem to be the case
here, AFAIU.

bug#72328: [PATCH] Nested backquote in pcase

[Thuna]

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

Fair enough.  It would not be particularly difficult to keep the old
behavior while giving an obsoletion warning in case someone is using it
"wrong", but that would require that the new behavior be considered
"correct".  What is overall opinion here?

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

bug#72328: [PATCH] Nested backquote in pcase

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

Thuna <thuna.cing@gmail.com> writes:

> > This would be a backward-incompatible change, breaking existing code.
>
> Yes, I forgot to mention this in my original report, but given how
> unintuitive (IMO) the current behavior is, I expect that very few people
> will be using patterns which would be effected by this change.

We have _dozens_ of such patterns in the Emacs Elisp sources that would
break.  Your expectation is wrong.


> > And this change would break the symmetry between `backquote' the macro
> > and backquote patterns in `pcase'.  This is an important design idea.
>
> I am not quite sure why you think that this breaks symmetry with the
> backquote macro;

We want that something like this works as expected:

#+begin_src emacs-lisp
(let ((a 1) (b 2))
  (pcase `(69 foo (97 ,a ((,b))))
    (`(69 foo (97 ,a ((,b))))
    (list a b)))) ==> (1 2)
#+end_src

I think it's obvious what I mean with symmetry between backquote and
pcase backquote without giving a formal definition.


> the purpose of this patch was to establish a symmetry
> in the first place.  Would you consider the examples provided as
> unnatural?

I'm more concerned about what we would loose.


> AFAICT right now the only way to match an unevaluated (or quoted) `,foo
> is with `(,'\` (,'\, foo)) which is quite unideal.

If `foo` is the literal symbol you can simply match using a quote pattern
like in

  (pcase '`,foo
    ('`,foo t))
 ==> t


> It is not often that you need this but having this not be
> representable in this way is not particularly useful.

It indeed gets ugly when one wants to match a non-constant
backquote expression using a backquote pcase pattern with partial
unquotes (the "mixed case").

But making pcase backquote patterns less expressive just to make this
special case simpler doesn't make sense to me.  OTOH I agree that having
a convenient solution for this kind of problem would be nice.


Michael.

bug#72328: [PATCH] Nested backquote in pcase

[Eli Zaretskii]

> Cc: 72328@debbugs.gnu.org
> Date: Mon, 29 Jul 2024 18:03:59 +0200
> From:  Michael Heerdegen via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Thuna <thuna.cing@gmail.com> writes:
> 
> > > This would be a backward-incompatible change, breaking existing code.
> >
> > Yes, I forgot to mention this in my original report, but given how
> > unintuitive (IMO) the current behavior is, I expect that very few people
> > will be using patterns which would be effected by this change.
> 
> We have _dozens_ of such patterns in the Emacs Elisp sources that would
> break.  Your expectation is wrong.
> 
> 
> > > And this change would break the symmetry between `backquote' the macro
> > > and backquote patterns in `pcase'.  This is an important design idea.
> >
> > I am not quite sure why you think that this breaks symmetry with the
> > backquote macro;
> 
> We want that something like this works as expected:
> 
> #+begin_src emacs-lisp
> (let ((a 1) (b 2))
>   (pcase `(69 foo (97 ,a ((,b))))
>     (`(69 foo (97 ,a ((,b))))
>     (list a b)))) ==> (1 2)
> #+end_src
> 
> I think it's obvious what I mean with symmetry between backquote and
> pcase backquote without giving a formal definition.
> 
> 
> > the purpose of this patch was to establish a symmetry
> > in the first place.  Would you consider the examples provided as
> > unnatural?
> 
> I'm more concerned about what we would loose.
> 
> 
> > AFAICT right now the only way to match an unevaluated (or quoted) `,foo
> > is with `(,'\` (,'\, foo)) which is quite unideal.
> 
> If `foo` is the literal symbol you can simply match using a quote pattern
> like in
> 
>   (pcase '`,foo
>     ('`,foo t))
>  ==> t
> 
> 
> > It is not often that you need this but having this not be
> > representable in this way is not particularly useful.
> 
> It indeed gets ugly when one wants to match a non-constant
> backquote expression using a backquote pcase pattern with partial
> unquotes (the "mixed case").
> 
> But making pcase backquote patterns less expressive just to make this
> special case simpler doesn't make sense to me.  OTOH I agree that having
> a convenient solution for this kind of problem would be nice.

Stefan, any comments?

bug#72328: [PATCH] Nested backquote in pcase

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

> Stefan, any comments?

FWIW, when I (re)implemented the Pcase backquote with `pcase-defmacro`
I hesitated between doing what Thuna suggests and what we have now.

I opted for the current behavior because it's simpler (at least from
the implementation's point of view), even though it
admittedly breaks the symmetry with the backquote macro.

I think I'd be interested to hear about "real life" cases out there
where this choice would make a difference (in either direction).

If we indent to change the behavior, I think we'd first need to introduce
a warning for patterns that would be affected by the change.


        Stefan

bug#72328: [PATCH] Nested backquote in pcase

[Thuna]

> I think I'd be interested to hear about "real life" cases out there
> where this choice would make a difference (in either direction).

For what its worth, a very imprecise search through all melpa packages
revealed exactly zero pcase patterns which this would effect.

I will write down the process in case someone wants to try and replicate
it themselves:

- Clone all repositories in melpa via `make', `package-build-all', or if
  you are in the horrible situation that I am where every other query
  doesn't resolve do (in the /path/to/melpa/package-build/ directory)

  emacs --batch --eval '(push default-directory load-path)'            \
    -l package-build -l package-recipe                                 \
    --eval '(mapc (lambda (file)
          (message "cloning %s" file)
          (ignore-errors
            (package-build--run-process "git" "clone"
                                        (package-recipe--upstream-url
                                         (package-recipe-lookup file))
                                        (concat "../working/" file))))
        (cl-remove-if
         (lambda (file) (file-exists-p (concat "../working/" file)))
         (package-recipe-packages)))'

- Obtain a list of .el files which contain both "pcase" and "`".

- Obtain the pcase forms in those files via

  (let ((print-level nil) (print-circle nil)
        (original-buffer (current-buffer)))
    (with-temp-file "~/pcase-output.eld"
      (let ((output-buffer (current-buffer)))
        (with-current-buffer original-buffer
          (mapc (lambda (file)
                  (kill-buffer
                   (with-current-buffer (find-file-noselect file)
                     (while (re-search-forward "\\_<pcase" nil t)
                       (unless (or (nth 3 (syntax-ppss))
                                   (nth 4 (syntax-ppss)))
                         (save-excursion
                           (backward-up-list)
                           (condition-case err
                               (prin1 (read (current-buffer)) output-buffer)
                             (error (print `(error ',file ,err)))))))
                     (current-buffer))))
                (split-string (buffer-string) "\n"))))))

- Replace those forms (when recognized) with a quoted list of the
  patterns via

  (let ((print-length nil)
        (print-level nil))
    (while-let ((form (ignore-errors (read (current-buffer)))))
      (when (consp form)
        (cl-case (car form)
          ((pcase pcase-exhaustive)
           (replace-region-contents
            (scan-sexps (point) -1) (point)
            (cl-constantly
             (prin1-to-string `'(,@(mapcar #'car (cddr form)))))))
          ((pcase-let pcase-let*)
           (replace-region-contents
            (scan-sexps (point) -1) (point)
            (cl-constantly
             (prin1-to-string `'(,@(mapcar #'car-safe (cadr form)))))))
          ((pcase-lambda)
           (replace-region-contents
            (scan-sexps (point) -1) (point)
            (cl-constantly
             (prin1-to-string
              `'(,@(remq '&optional (remq '&rest (cadr form))))))))
          ((pcase-dolist)
           (replace-region-contents
            (scan-sexps (point) -1) (point)
            (cl-constantly
             (prin1-to-string `'(,@(caadr form))))))
          ((require pcase-defmacro)
           (delete-region (scan-sexps (point) -1) (point)))))))

- Filter out all patterns which do not contain at least two backquotes
  via M-x keep-lines RET .*`.*`.*

- Manually filter out all patterns which do not qualify

- Observe the empty buffer

bug#72328: [PATCH] Nested backquote in pcase

[Eli Zaretskii]

> From: Thuna <thuna.cing@gmail.com>
> Cc: Michael Heerdegen <michael_heerdegen@web.de>, 72328@debbugs.gnu.org
> Date: Sat, 03 Aug 2024 02:07:59 +0200
> 
> 
> > I think I'd be interested to hear about "real life" cases out there
> > where this choice would make a difference (in either direction).
> 
> For what its worth, a very imprecise search through all melpa packages
> revealed exactly zero pcase patterns which this would effect.

Thanks, but I think Stefan meant to say that we would need real-life
use cases to change the current behavior.  So if there are no such
cases, it makes the decision less likely.

bug#72328: [PATCH] Nested backquote in pcase

[Thuna]

>> > I think I'd be interested to hear about "real life" cases out there
>> > where this choice would make a difference (in either direction).
>> 
>> For what its worth, a very imprecise search through all melpa packages
>> revealed exactly zero pcase patterns which this would effect.
>
> Thanks, but I think Stefan meant to say that we would need real-life
> use cases to change the current behavior.  So if there are no such
> cases, it makes the decision less likely.

I don't see how Stefan's message could be interpreted that way.  If
almost no code is effected by a change then whether to apply that change
or not should depend solely on whether it is considered an improvement,
even if theoretically.

bug#72328: [PATCH] Nested backquote in pcase

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

Thuna <thuna.cing@gmail.com> writes:

> > Thanks, but I think Stefan meant to say that we would need real-life
> > use cases to change the current behavior.  So if there are no such
> > cases, it makes the decision less likely.
>
> I don't see how Stefan's message could be interpreted that way.

FWIW, I interpreted it that way too.


> If almost no code is effected by a change then whether to apply that
> change or not should depend solely on whether it is considered an
> improvement, even if theoretically.

From what I understand, coming to decisions as a maintainer is not that
simple.  And Stefan had reasons to choose the current implementation.

So this is a relevant question and trying to answer it would probably
help to come to an decision.

Michael.

bug#72328: [PATCH] Nested backquote in pcase

[Thuna]

>>> Thanks, but I think Stefan meant to say that we would need real-life
>>> use cases to change the current behavior.  So if there are no such
>>> cases, it makes the decision less likely.
>>
>> I don't see how Stefan's message could be interpreted that way.
>
> FWIW, I interpreted it that way too.

My understaning is that the utility of going through use cases for or
against the new behavior is to determine whether it would be an
improvement or not.  The absence of any such use cases does not mean
anything in either direction, but simply that this is not a usable tool.

Furthermore, just intuitively, why would the absence of users of a
feature be a reason to not improve it?  If the contention is that this
is not an improvement then I would rather that argument be made instead.

Note that I do not believe that there are no people who would be
effected by this, positively or negatively.  However, my search through
melpa shows that producing such examples will be difficult and this
thread will likely not see many (if any) replies, aside from the one
shared by Michael and the one I provide below.

>> If almost no code is effected by a change then whether to apply that
>> change or not should depend solely on whether it is considered an
>> improvement, even if theoretically.
>
> From what I understand, coming to decisions as a maintainer is not that
> simple.  And Stefan had reasons to choose the current implementation.

The stated reasoning was that implementational simplicity was valued
over symmetry with backquote.  There is an argument to be made that the
current behavior, though at odds with backquote, is preferable in the
context of pcase.  Such an argument has yet to have been made, however.

> So this is a relevant question and trying to answer it would probably
> help to come to an decision.

Very well.  In that case, here's the original case which prompted me to
look into how pcase's backquote behaves:

(defun macroexp-null (exp)
  "Return non-nil if EXP will always evaluate to nil.
This form does not take non-local exits or side-effects into account."
  (pcase exp
    ((or 'nil ''nil '#'nil '`nil ``,,(pred macroexp-null))
     t)))

which without this change would read as:

(defun macroexp-null (exp)
  "Return non-nil if EXP will always evaluate to nil.
This form does not take non-local exits or side-effects into account."
  (pcase exp
    ((or 'nil ''nil '#'nil '`nil
         `(,'\` (,'\, ,(pred macroexp-null))))
     t)))

bug#72328: [PATCH] Nested backquote in pcase

[Eli Zaretskii]

> From: Thuna <thuna.cing@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, monnier@iro.umontreal.ca,
>  72328@debbugs.gnu.org
> Date: Sun, 04 Aug 2024 23:27:52 +0200
> 
> >>> Thanks, but I think Stefan meant to say that we would need real-life
> >>> use cases to change the current behavior.  So if there are no such
> >>> cases, it makes the decision less likely.
> >>
> >> I don't see how Stefan's message could be interpreted that way.
> >
> > FWIW, I interpreted it that way too.
> 
> My understaning is that the utility of going through use cases for or
> against the new behavior is to determine whether it would be an
> improvement or not.  The absence of any such use cases does not mean
> anything in either direction, but simply that this is not a usable tool.

The existing implementation has an advantage up front, because it
exists and presumably can be used by some Lisp program.  We try very
hard not to break anyone's code, even if we are not aware of such
code, and therefore an incompatible change in behavior can be
justified only if there's a very good reason.  Thus the request for
real-life use cases.

AFAIU, those real-life use cases don't have to be from existing
packages, they can be from your own practice.  You just need to
explain why you needed the behavior you are requesting.

> Furthermore, just intuitively, why would the absence of users of a
> feature be a reason to not improve it?

Because the danger of breaking someone's code is not something we
ignore lightly.  So the improvement must be significant, and the use
case must be a practical one, to trump that.

> Note that I do not believe that there are no people who would be
> effected by this, positively or negatively.

We've learned from bitter experience that such arguments are usually
false.  IOW, we don't really know enough to make such assertions.

> > From what I understand, coming to decisions as a maintainer is not that
> > simple.  And Stefan had reasons to choose the current implementation.
> 
> The stated reasoning was that implementational simplicity was valued
> over symmetry with backquote.

That' too.  But the mere existence of the current behavior is also
important.

> Very well.  In that case, here's the original case which prompted me to
> look into how pcase's backquote behaves:
> 
> (defun macroexp-null (exp)
>   "Return non-nil if EXP will always evaluate to nil.
> This form does not take non-local exits or side-effects into account."
>   (pcase exp
>     ((or 'nil ''nil '#'nil '`nil ``,,(pred macroexp-null))
>      t)))
> 
> which without this change would read as:
> 
> (defun macroexp-null (exp)
>   "Return non-nil if EXP will always evaluate to nil.
> This form does not take non-local exits or side-effects into account."
>   (pcase exp
>     ((or 'nil ''nil '#'nil '`nil
>          `(,'\` (,'\, ,(pred macroexp-null))))
>      t)))

Thanks, now you just need to explain why you needed this code and what
did its caller do to require this.

bug#72328: [PATCH] Nested backquote in pcase

[Thuna]

>> Note that I do not believe that there are no people who would be
>> effected by this, positively or negatively.
>
> We've learned from bitter experience that such arguments are usually
> false.  IOW, we don't really know enough to make such assertions.

I think there is a misunderstanding.  I am not saying that there isn't
anyone who would be effected by this, it is the opposite.  I understand
that this will effect people, and I agree that at minimum there needs to
be a decent period where the current behavior is maintained but marked
as obsolete.  An indefinite feature-freeze is where I have a problem.

>> Very well.  In that case, here's the original case which prompted me to
>> look into how pcase's backquote behaves:
>> 
>> (defun macroexp-null (exp)
>>   "Return non-nil if EXP will always evaluate to nil.
>> This form does not take non-local exits or side-effects into account."
>>   (pcase exp
>>     ((or 'nil ''nil '#'nil '`nil ``,,(pred macroexp-null))
>>      t)))
>> 
>> which without this change would read as:
>> 
>> (defun macroexp-null (exp)
>>   "Return non-nil if EXP will always evaluate to nil.
>> This form does not take non-local exits or side-effects into account."
>>   (pcase exp
>>     ((or 'nil ''nil '#'nil '`nil
>>          `(,'\` (,'\, ,(pred macroexp-null))))
>>      t)))
>
> Thanks, now you just need to explain why you needed this code and what
> did its caller do to require this.

I do not understand what you are asking for.  Whether `macroexp-null'
should exist or not, what it is trying to do should be fairly clear, so
should the way in which it benefits from the changed behavior.

I also cannot provide any justification for this patch above and beyond
what I have already mentioned in my initial message: This patch
establishes a symmetry between pcase's backquote pattern and quasiquote,
which allows trivially matching against the result of a quasiquote form.

I would appreciate it if you would state your opinion on this patch,
putting aside concerns of backwards compatibility for a moment.  I am
working under the assumption that this is an improvement and is
desirable, yet I have not yet heard from you or Stefan as to whether you
see it that way or not.

bug#72328: [PATCH] Nested backquote in pcase

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

> I think there is a misunderstanding.  I am not saying that there isn't
> anyone who would be effected by this, it is the opposite.  I understand
> that this will effect people, and I agree that at minimum there needs to
> be a decent period where the current behavior is maintained but marked
> as obsolete.  An indefinite feature-freeze is where I have a problem.

Maybe the simplest is to add a warning when we encounter a nested
backquote, stating that this is not supported.


        Stefan

bug#72328: [PATCH] Nested backquote in pcase

[Eli Zaretskii]

> From: Thuna <thuna.cing@gmail.com>
> Cc: michael_heerdegen@web.de, monnier@iro.umontreal.ca, 72328@debbugs.gnu.org
> Date: Mon, 05 Aug 2024 21:32:43 +0200
> 
> >> Note that I do not believe that there are no people who would be
> >> effected by this, positively or negatively.
> >
> > We've learned from bitter experience that such arguments are usually
> > false.  IOW, we don't really know enough to make such assertions.
> 
> I think there is a misunderstanding.  I am not saying that there isn't
> anyone who would be effected by this, it is the opposite.  I understand
> that this will effect people, and I agree that at minimum there needs to
> be a decent period where the current behavior is maintained but marked
> as obsolete.

How do you envision making such a behavior change in a way that will
leave the current behavior still maintained (and obsolete)?

> An indefinite feature-freeze is where I have a problem.

Disagreeing with a specific change is not tantamount to an indefinite
feature-freeze.  It is quite possible that someone will come up with a
different idea of a change, which we will be able to reconcile easier
with the previous behavior.

> >> (defun macroexp-null (exp)
> >>   "Return non-nil if EXP will always evaluate to nil.
> >> This form does not take non-local exits or side-effects into account."
> >>   (pcase exp
> >>     ((or 'nil ''nil '#'nil '`nil ``,,(pred macroexp-null))
> >>      t)))
> >> 
> >> which without this change would read as:
> >> 
> >> (defun macroexp-null (exp)
> >>   "Return non-nil if EXP will always evaluate to nil.
> >> This form does not take non-local exits or side-effects into account."
> >>   (pcase exp
> >>     ((or 'nil ''nil '#'nil '`nil
> >>          `(,'\` (,'\, ,(pred macroexp-null))))
> >>      t)))
> >
> > Thanks, now you just need to explain why you needed this code and what
> > did its caller do to require this.
> 
> I do not understand what you are asking for.  Whether `macroexp-null'
> should exist or not, what it is trying to do should be fairly clear, so
> should the way in which it benefits from the changed behavior.

I asked to explain _why_ you need this.  Risking to say the obvious, a
program exists to do some job, and a function like the above is
therefore part of some larger job.  We are asking you to describe the
higher-level context, which we could then use to try to decide whether
the need is important enough to justify the backward-incompatible
change.

> I also cannot provide any justification for this patch above and beyond
> what I have already mentioned in my initial message: This patch
> establishes a symmetry between pcase's backquote pattern and quasiquote,
> which allows trivially matching against the result of a quasiquote form.

We would like to hear reasons for wanting this.

> I would appreciate it if you would state your opinion on this patch,
> putting aside concerns of backwards compatibility for a moment.  I am
> working under the assumption that this is an improvement and is
> desirable, yet I have not yet heard from you or Stefan as to whether you
> see it that way or not.

I believe Stefan did say that.  Me, my only stake here is the concern
of backwards compatibility, which is why I'm talking only about that.

Thanks.

bug#72328: [PATCH] Nested backquote in pcase

[Thuna]

>> >> Note that I do not believe that there are no people who would be
>> >> effected by this, positively or negatively.
>> >
>> > We've learned from bitter experience that such arguments are usually
>> > false.  IOW, we don't really know enough to make such assertions.
>> 
>> I think there is a misunderstanding.  I am not saying that there isn't
>> anyone who would be effected by this, it is the opposite.  I understand
>> that this will effect people, and I agree that at minimum there needs to
>> be a decent period where the current behavior is maintained but marked
>> as obsolete.
>
> How do you envision making such a behavior change in a way that will
> leave the current behavior still maintained (and obsolete)?

In the patch I provided the check for `(<= depth 0)' can be moved out of
the conditions and into the branches as

   `(unless (<= depth 0) (macroexp-warn-and-return ...))'

and it should work identical to how it does now.  We can also
semi-support the new behavior at the same time by making it so that if
there are nested commas in the pattern it uses the new behavior.  This
should not cause any backwards-incompatibility because a nested comma in
a pcase is currently just an error.  If this is something that would be
of interest I can look into providing a patch for it.

>> An indefinite feature-freeze is where I have a problem.
>
> Disagreeing with a specific change is not tantamount to an indefinite
> feature-freeze.  It is quite possible that someone will come up with a
> different idea of a change, which we will be able to reconcile easier
> with the previous behavior.

As long as the goal of such a change is to establish symmetry with
quasiquote, you can not reconcile the fact that the current behavior and
the expected behavior on the pattern ``,foo are incompatible.

>> >> (defun macroexp-null (exp)
>> >>   "Return non-nil if EXP will always evaluate to nil.
>> >> This form does not take non-local exits or side-effects into account."
>> >>   (pcase exp
>> >>     ((or 'nil ''nil '#'nil '`nil ``,,(pred macroexp-null))
>> >>      t)))
>> >> 
>> >> which without this change would read as:
>> >> 
>> >> (defun macroexp-null (exp)
>> >>   "Return non-nil if EXP will always evaluate to nil.
>> >> This form does not take non-local exits or side-effects into account."
>> >>   (pcase exp
>> >>     ((or 'nil ''nil '#'nil '`nil
>> >>          `(,'\` (,'\, ,(pred macroexp-null))))
>> >>      t)))
>> >
>> > Thanks, now you just need to explain why you needed this code and what
>> > did its caller do to require this.
>> 
>> I do not understand what you are asking for.  Whether `macroexp-null'
>> should exist or not, what it is trying to do should be fairly clear, so
>> should the way in which it benefits from the changed behavior.
>
> I asked to explain _why_ you need this.  Risking to say the obvious, a
> program exists to do some job, and a function like the above is
> therefore part of some larger job.  We are asking you to describe the
> higher-level context, which we could then use to try to decide whether
> the need is important enough to justify the backward-incompatible
> change.

And I am saying that that broader context of this example is not
meaningful in any way; I provided it solely because of the ask for a
real-life use-case.  I would appreciate not being put in a situation
where I must defend multiple patches (one not even proposed) at once.

Whether this new behavior is or will be used in emacs does not matter.
I am not proposing this change to make further patches more convenient,
but because I believe that it is the correct way for pcase to behave.

>> I also cannot provide any justification for this patch above and beyond
>> what I have already mentioned in my initial message: This patch
>> establishes a symmetry between pcase's backquote pattern and quasiquote,
>> which allows trivially matching against the result of a quasiquote form.
>
> We would like to hear reasons for wanting this.

At the risk of repeating myself: (pcase ``,foo (``,foo foo)) should not
return (\, foo).  The current behavior is unintuitive and makes patterns
actually matching against quasiquote forms essentially write-only.

>> I would appreciate it if you would state your opinion on this patch,
>> putting aside concerns of backwards compatibility for a moment.  I am
>> working under the assumption that this is an improvement and is
>> desirable, yet I have not yet heard from you or Stefan as to whether you
>> see it that way or not.
> I believe Stefan did say that.  Me, my only stake here is the concern
> of backwards compatibility, which is why I'm talking only about that.

I don't see how we can discuss whether this patch has enough benefits to
clear the hurdle of backwards-incompatibility if you are not willing to
engage with the discussion of what this patch's benefits *are* to begin
with.

bug#72328: [PATCH] Nested backquote in pcase

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

Thuna <thuna.cing@gmail.com> writes:

> I don't see how we can discuss whether this patch has enough benefits to
> clear the hurdle of backwards-incompatibility if you are not willing to
> engage with the discussion of what this patch's benefits *are* to begin
> with.

He sees that of course, but it is not what he has asked for because it
is obviously not enough.

Eli has to ask these questions, he is just doing his job.  He has to
weight backward incompatibilities and potential breakage against
potential further improvements.  The second is what his question is
about.  If you as the one who proposes this change can't answer it with
concrete examples, how could he come to a decision?

So I suggest to avoid meta discussions about what we expect and
how others should behave and, even if it sucks, try to answer Eli's
questions without questioning his reasons and intentions all the time.


Michael.

bug#72328: [PATCH] Nested backquote in pcase

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  monnier@iro.umontreal.ca,
>   72328@debbugs.gnu.org
> Date: Wed, 07 Aug 2024 05:33:40 +0200
> 
> Thuna <thuna.cing@gmail.com> writes:
> 
> > I don't see how we can discuss whether this patch has enough benefits to
> > clear the hurdle of backwards-incompatibility if you are not willing to
> > engage with the discussion of what this patch's benefits *are* to begin
> > with.
> 
> He sees that of course, but it is not what he has asked for because it
> is obviously not enough.
> 
> Eli has to ask these questions, he is just doing his job.  He has to
> weight backward incompatibilities and potential breakage against
> potential further improvements.  The second is what his question is
> about.  If you as the one who proposes this change can't answer it with
> concrete examples, how could he come to a decision?
> 
> So I suggest to avoid meta discussions about what we expect and
> how others should behave and, even if it sucks, try to answer Eli's
> questions without questioning his reasons and intentions all the time.

Thanks for representing my POV so clearly.

bug#72328: [PATCH] Nested backquote in pcase

[Thuna]

>> I don't see how we can discuss whether this patch has enough benefits to
>> clear the hurdle of backwards-incompatibility if you are not willing to
>> engage with the discussion of what this patch's benefits *are* to begin
>> with.
>
> He sees that of course, but it is not what he has asked for because it
> is obviously not enough.

It may not be enough but it is a necessary step.  I am currently having
to caveat all usage of the word "improvement" with either a weakener
like "IMO" or a conditional like "if this is considered an improvement".
Having to repeatedly try to appease implicit, unstated, and
unchallangable contentions is rather tiring.

> So I suggest to avoid meta discussions about what we expect and
> how others should behave and, even if it sucks, try to answer Eli's
> questions without questioning his reasons and intentions all the time.

What I question is not Eli's intentions or reasons, but the consequences
of complying with those requests: I fear that the moment I start
defending and giving justifications for the existence of macroexp-null,
this report will forever be reduced to just a "companion patch" whose
sole purpose is to make a later patch *look* nice.  My concern is that
this will end up the same way me showing no code in melpa would be
effected by this patch did - with my case being hurt on false grounds.

I have evidently been unable to get my point across, so let me try to
explain it yet again: The "broader context" has no effect on the
decision to use pcase in macroexp-null, you might as well be asking my
why I used cond.  I use pcase in macroexp-null because for pattern
matching it is by far the best tool there is and it makes for a much
more readable and extensible code than

  (defun macroexp-null (exp)
    "Return non-nil if EXP will always evaluate to nil.
  This form does not take non-local exits or side-effects into account."
    (or (member exp '(nil 'nil #'nil `nil))
        (and (eq '\` (car-safe exp))
             (length= exp 2)
             (eq '\, (car-safe (nth 1 exp)))
             (length= (nth 1 exp) 2)
             (macroexp-null (nth 1 (nth 1 exp))))))

Furhermore, even if the resulting discussion concludes that
macroexp-null should exist, that does not mean anything about this
change; I have already demonstrated that the program can be written
without it[1].  And the reverse implication is true: This patch can be
accepted without macroexp-null.  In fact, it is a stronger implication:
This patch can be accepted without ever acknowledging the existence of
macroexp-null.

Given these two are separate things that do not need each other, and the
example alone demonstrates the value of this new behavior in this
specific situation, the continued inquiry on a justification can only
bring about a single result: If I somehow fail to convince people that
macroexp-null should exist, this makes a case against this patch.  The
opposite will not be true however (if I convince you that macroexp-null
should exist will you not simply tell me to use the version which works
pre-patch?).

And let me also mention again why I think that this is a genuine
improvement: You currently cannot match against a nested backquote
object in any sane way.  The pattern you need to match against the
unevaluated form ``(,,<integer>) is
  `(,'\` (,'\` (,'\, (,'\, ,(pred integerp)))))
and... how is this not considered a problem?  That this thing works at
all can only really be thought of as a coincidence, and it explicitly
relies on `foo being (\` foo), which I would personally consider an
internal implementation detail not to be used by outside code.

There is a genuine loss with switching over to the new behavior: We run
into the same issue as before when matching against an unevaluated form
``(,<symbol> ,,<integer>).

This is also why I want to have more discussion about the patch itself:
The current behavior is bad at actually matching backquote objects as
obtained by a single quasiquote macro, but the proposed behavior is bad
at matching backquote objects obtained by multiple different-but-close
quasiquote macros[2].  It is very likely that the result of that
discussion will end up changing the way this patch actually works and
maybe even result in the original behavior being maintained in the first
place.

[1] With or without pcase, though in this instance I am referring to the
with-pcase version I provided in my previous messages.

[2] You can only obtain an unevaluated form ```(,<symbol> ,,,<integer>)
via an evaluated form like ````(,<symbol> ,,,,<integer-returning-form>)
where <symbol> is a fixed symbol.  If the pcase pattern is matching
against all possible <symbol> objects, that means that the objects it is
matching must be coming from different quasiquote forms.

bug#72328: [PATCH] Nested backquote in pcase

[Eli Zaretskii]

> From: Thuna <thuna.cing@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, monnier@iro.umontreal.ca,
>  72328@debbugs.gnu.org
> Date: Wed, 07 Aug 2024 19:34:32 +0200
> 
> What I question is not Eli's intentions or reasons, but the consequences
> of complying with those requests: I fear that the moment I start
> defending and giving justifications for the existence of macroexp-null,
> this report will forever be reduced to just a "companion patch" whose
> sole purpose is to make a later patch *look* nice.  My concern is that
> this will end up the same way me showing no code in melpa would be
> effected by this patch did - with my case being hurt on false grounds.

You make it sound as if there's some hidden agenda in this discussion.
There isn't.

> I have evidently been unable to get my point across, so let me try to
> explain it yet again: The "broader context" has no effect on the
> decision to use pcase in macroexp-null, you might as well be asking my
> why I used cond.  I use pcase in macroexp-null because for pattern
> matching it is by far the best tool there is and it makes for a much
> more readable and extensible code than
> 
>   (defun macroexp-null (exp)
>     "Return non-nil if EXP will always evaluate to nil.
>   This form does not take non-local exits or side-effects into account."
>     (or (member exp '(nil 'nil #'nil `nil))
>         (and (eq '\` (car-safe exp))
>              (length= exp 2)
>              (eq '\, (car-safe (nth 1 exp)))
>              (length= (nth 1 exp) 2)
>              (macroexp-null (nth 1 (nth 1 exp))))))
> 
> Furhermore, even if the resulting discussion concludes that
> macroexp-null should exist, that does not mean anything about this
> change; I have already demonstrated that the program can be written
> without it[1].  And the reverse implication is true: This patch can be
> accepted without macroexp-null.  In fact, it is a stronger implication:
> This patch can be accepted without ever acknowledging the existence of
> macroexp-null.
> 
> Given these two are separate things that do not need each other, and the
> example alone demonstrates the value of this new behavior in this
> specific situation, the continued inquiry on a justification can only
> bring about a single result: If I somehow fail to convince people that
> macroexp-null should exist, this makes a case against this patch.  The
> opposite will not be true however (if I convince you that macroexp-null
> should exist will you not simply tell me to use the version which works
> pre-patch?).
> 
> And let me also mention again why I think that this is a genuine
> improvement: You currently cannot match against a nested backquote
> object in any sane way.  The pattern you need to match against the
> unevaluated form ``(,,<integer>) is
>   `(,'\` (,'\` (,'\, (,'\, ,(pred integerp)))))
> and... how is this not considered a problem?  That this thing works at
> all can only really be thought of as a coincidence, and it explicitly
> relies on `foo being (\` foo), which I would personally consider an
> internal implementation detail not to be used by outside code.
> 
> There is a genuine loss with switching over to the new behavior: We run
> into the same issue as before when matching against an unevaluated form
> ``(,<symbol> ,,<integer>).
> 
> This is also why I want to have more discussion about the patch itself:
> The current behavior is bad at actually matching backquote objects as
> obtained by a single quasiquote macro, but the proposed behavior is bad
> at matching backquote objects obtained by multiple different-but-close
> quasiquote macros[2].  It is very likely that the result of that
> discussion will end up changing the way this patch actually works and
> maybe even result in the original behavior being maintained in the first
> place.

I think we all agree that your suggestion has some advantages in some
situation.  We also all agree that the problem you are trying to solve
is solvable already by other means, so it isn't like the problem
doesn't have workarounds, and the workarounds are not in general
terribly kludgey or inconvenient.

So justification for introducing such change in behavior is actually
the main point that needs to be discussed, because it will be a main
factor in the decision whether we want to install such a change.  And
the justifications that we are interested to hear are the situations
where using the available behavior would cause such significant
inconvenience or unclean code that having this new behavior would
avoid.  Then we will have to decide whether those situations are
important enough for us to risk the incompatibilities, complicate the
documentation, add backward-compatibility shims, etc. -- all of which
make Emacs slightly more complex and slightly harder to maintain.

This process and these considerations are inherent to the maintainers'
decision process when a change is proposed that means incompatible
behavior changes in Emacs.  Talking about the advantages alone doesn't
cut it.

bug#72328: [PATCH] Nested backquote in pcase

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

Eli Zaretskii <eliz@gnu.org> writes:

[I'm trying to keep this discussion alive...]

> > What I question is not Eli's intentions or reasons, but the consequences
> > of complying with those requests: I fear that the moment I start
> > defending and giving justifications for the existence of macroexp-null,
> > this report will forever be reduced to just a "companion patch" whose
> > sole purpose is to make a later patch *look* nice.  My concern is that
> > this will end up the same way me showing no code in melpa would be
> > effected by this patch did - with my case being hurt on false grounds.
>
> You make it sound as if there's some hidden agenda in this discussion.
> There isn't.

Yes, Thuna, from experience I know that the way Eli asks questions for
some people (including myself) sometimes suggests intentions that are
not there.  It's not a mistake to just answer his questions, IME.

> So justification for introducing such change in behavior is actually
> the main point that needs to be discussed, because it will be a main
> factor in the decision whether we want to install such a change.  And
> the justifications that we are interested to hear are the situations
> where using the available behavior would cause such significant
> inconvenience or unclean code that having this new behavior would
> avoid.  Then we will have to decide whether those situations are
> important enough for us to risk the incompatibilities, complicate the
> documentation, add backward-compatibility shims, etc. -- all of which
> make Emacs slightly more complex and slightly harder to maintain.


Would be good to hear from Stefan how he would estimate the risk of
potential incompatibilities.  And the value of the change of semantics.


[ The rest of my message is more or less a summary of aspects already
given, from my perspective. ]

Thinking more about it, from the things I can see, I think I'm in favor
of this change.  In the long run it makes Elisp more consistent.

I don't know how often backquote values are needed to be matched.  I
guess not terribly often.  OTOH, this ,'\, circus really can drive
one crazy, it's not obvious what one needs to do at all to match a
backquote value correctly, so it's also an improvement to enable people
to do this in a reachable way, and the code also gets better readable.

OTOH, I think the current semantics of pcase ` are easier to explain in
a formal way.  Dunno if this is an advantage though, since most people
don't seem to understand them anyway.


Thuna, do you have any other ideas where your patch would be a
significant improvement - practical use cases?  I must admit I'm not yet
completely sure how the impact of the patch would on using el-search
- although I often use it to search backquote expressions.  Guess I'm
too used to the current semantics now :-(  What I can say is that I often
wished that it would be easier, in one way or the other.


Michael.

bug#72328: [PATCH] Nested backquote in pcase

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

> Would be good to hear from Stefan

The first step is to declare nested backquotes unsupported and add
a warning when we encounter them.  Until this is done, further
discussion seems pointless.

> how he would estimate the risk of potential incompatibilities.
> And the value of the change of semantics.

The risk of potential incompatibilities is exactly equal to the
potential gain.


        Stefan

bug#72328: [PATCH] Nested backquote in pcase

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Thuna <thuna.cing@gmail.com>,
>   72328@debbugs.gnu.org
> Date: Fri, 23 Aug 2024 10:49:01 -0400
> 
> > Would be good to hear from Stefan
> 
> The first step is to declare nested backquotes unsupported and add
> a warning when we encounter them.

Where and how would you suggest to do that?

bug#72328: [PATCH] Nested backquote in pcase

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

>> > Would be good to hear from Stefan
>> The first step is to declare nested backquotes unsupported and add
>> a warning when we encounter them.
> Where and how would you suggest to do that?

Somethink like...


        Stefan


diff --git a/lisp/emacs-lisp/pcase.el b/lisp/emacs-lisp/pcase.el
index fd6b0c8db5c..fe62820f0cb 100644
--- a/lisp/emacs-lisp/pcase.el
+++ b/lisp/emacs-lisp/pcase.el
@@ -1172,7 +1172,10 @@ pcase--expand-\`
           (upatd (pcase--expand-\` (cdr qpat))))
       (if (and (eq (car-safe upata) 'quote) (eq (car-safe upatd) 'quote))
           `'(,(cadr upata) . ,(cadr upatd))
-        `(and (pred consp)
+        `(and ,@(when (eq (car qpat) '\`)
+                  `((guard ,(macroexp-warn-and-return
+                             "Nested ` are not supported" t nil nil qpat))))
+              (pred consp)
               (app car-safe ,upata)
               (app cdr-safe ,upatd)))))
    ((or (stringp qpat) (numberp qpat) (symbolp qpat)) `',qpat)

bug#72328: [PATCH] Nested backquote in pcase

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: michael_heerdegen@web.de,  thuna.cing@gmail.com,  72328@debbugs.gnu.org
> Date: Fri, 23 Aug 2024 15:11:04 -0400
> 
> >> > Would be good to hear from Stefan
> >> The first step is to declare nested backquotes unsupported and add
> >> a warning when we encounter them.
> > Where and how would you suggest to do that?
> 
> Somethink like...
> 
> 
>         Stefan
> 
> 
> diff --git a/lisp/emacs-lisp/pcase.el b/lisp/emacs-lisp/pcase.el
> index fd6b0c8db5c..fe62820f0cb 100644
> --- a/lisp/emacs-lisp/pcase.el
> +++ b/lisp/emacs-lisp/pcase.el
> @@ -1172,7 +1172,10 @@ pcase--expand-\`
>            (upatd (pcase--expand-\` (cdr qpat))))
>        (if (and (eq (car-safe upata) 'quote) (eq (car-safe upatd) 'quote))
>            `'(,(cadr upata) . ,(cadr upatd))
> -        `(and (pred consp)
> +        `(and ,@(when (eq (car qpat) '\`)
> +                  `((guard ,(macroexp-warn-and-return
> +                             "Nested ` are not supported" t nil nil qpat))))
> +              (pred consp)
>                (app car-safe ,upata)
>                (app cdr-safe ,upatd)))))
>     ((or (stringp qpat) (numberp qpat) (symbolp qpat)) `',qpat)

Feel free to install on master, and thanks.

bug#72328: [PATCH] Nested backquote in pcase

[Eli Zaretskii]

Ping!  Stefan, should I install this in your name?

> Cc: michael_heerdegen@web.de, thuna.cing@gmail.com, 72328@debbugs.gnu.org
> Date: Fri, 23 Aug 2024 22:29:10 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Stefan Monnier <monnier@iro.umontreal.ca>
> > Cc: michael_heerdegen@web.de,  thuna.cing@gmail.com,  72328@debbugs.gnu.org
> > Date: Fri, 23 Aug 2024 15:11:04 -0400
> > 
> > >> > Would be good to hear from Stefan
> > >> The first step is to declare nested backquotes unsupported and add
> > >> a warning when we encounter them.
> > > Where and how would you suggest to do that?
> > 
> > Somethink like...
> > 
> > 
> >         Stefan
> > 
> > 
> > diff --git a/lisp/emacs-lisp/pcase.el b/lisp/emacs-lisp/pcase.el
> > index fd6b0c8db5c..fe62820f0cb 100644
> > --- a/lisp/emacs-lisp/pcase.el
> > +++ b/lisp/emacs-lisp/pcase.el
> > @@ -1172,7 +1172,10 @@ pcase--expand-\`
> >            (upatd (pcase--expand-\` (cdr qpat))))
> >        (if (and (eq (car-safe upata) 'quote) (eq (car-safe upatd) 'quote))
> >            `'(,(cadr upata) . ,(cadr upatd))
> > -        `(and (pred consp)
> > +        `(and ,@(when (eq (car qpat) '\`)
> > +                  `((guard ,(macroexp-warn-and-return
> > +                             "Nested ` are not supported" t nil nil qpat))))
> > +              (pred consp)
> >                (app car-safe ,upata)
> >                (app cdr-safe ,upatd)))))
> >     ((or (stringp qpat) (numberp qpat) (symbolp qpat)) `',qpat)
> 
> Feel free to install on master, and thanks.
> 
> 
> 
> 

bug#72328: [PATCH] Nested backquote in pcase

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

Eli Zaretskii <eliz@gnu.org> writes:

> Ping!  Stefan, should I install this in your name?

In the meantime I installed the patch locally and got two warnings when
rebuilding Emacs completely:

| In testcover-analyze-coverage:
| emacs-lisp/testcover.el:472:8: Warning: Nested ` are not supported
|   ELC      emacs-lisp/timer-list.elc
|
| In testcover-analyze-coverage-wrapped-form:
| emacs-lisp/testcover.el:551:8: Warning: Nested ` are not supported

But they should be trivial to fix (they are only warnings anyway).


Michael.

bug#72328: [PATCH] Nested backquote in pcase

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: monnier@iro.umontreal.ca,  thuna.cing@gmail.com,  72328@debbugs.gnu.org
> Date: Sat, 07 Sep 2024 14:24:29 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Ping!  Stefan, should I install this in your name?
> 
> In the meantime I installed the patch locally and got two warnings when
> rebuilding Emacs completely:
> 
> | In testcover-analyze-coverage:
> | emacs-lisp/testcover.el:472:8: Warning: Nested ` are not supported
> |   ELC      emacs-lisp/timer-list.elc
> |
> | In testcover-analyze-coverage-wrapped-form:
> | emacs-lisp/testcover.el:551:8: Warning: Nested ` are not supported
> 
> But they should be trivial to fix (they are only warnings anyway).

Ping! Ping! Stefan, could you please respond?

bug#72328: [PATCH] Nested backquote in pcase

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

>> Ping!  Stefan, should I install this in your name?
> In the meantime I installed the patch locally and got two warnings when
> rebuilding Emacs completely:
> | In testcover-analyze-coverage:
> | emacs-lisp/testcover.el:472:8: Warning: Nested ` are not supported
> |   ELC      emacs-lisp/timer-list.elc
> |
> | In testcover-analyze-coverage-wrapped-form:
> | emacs-lisp/testcover.el:551:8: Warning: Nested ` are not supported
>
> But they should be trivial to fix (they are only warnings anyway).

Indeed, these are cases which would change meaning under the semantics
that was proposed in this bug report.  The patch below would get rid of
the nested backquotes, thus making that code "agnostic" to their semantics.


	Stefan


diff --git a/lisp/emacs-lisp/testcover.el b/lisp/emacs-lisp/testcover.el
index fb4a2a82d07..d916ca0f76a 100644
--- a/lisp/emacs-lisp/testcover.el
+++ b/lisp/emacs-lisp/testcover.el
@@ -469,7 +469,7 @@ testcover-analyze-coverage
      ;; form to look odd. See bug#25316.
      'testcover-1value)
 
-    (`(\` ,bq-form)
+    (`(,'\` ,bq-form)
      (testcover-analyze-coverage-backquote-form bq-form))
 
     ((or 't 'nil (pred keywordp))
@@ -548,7 +548,7 @@ testcover-analyze-coverage-wrapped-form
        'testcover-1value))
     ((pred atom)
      'testcover-1value)
-    (`(\` ,bq-form)
+    (`(,'\` ,bq-form)
      (testcover-analyze-coverage-backquote-form bq-form))
     (`(defconst ,sym ,val . ,_)
      (push sym testcover-module-constants)

bug#72328: [PATCH] Nested backquote in pcase

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  thuna.cing@gmail.com,  72328@debbugs.gnu.org
> Date: Sun, 22 Sep 2024 09:30:25 -0400
> 
> >> Ping!  Stefan, should I install this in your name?
> > In the meantime I installed the patch locally and got two warnings when
> > rebuilding Emacs completely:
> > | In testcover-analyze-coverage:
> > | emacs-lisp/testcover.el:472:8: Warning: Nested ` are not supported
> > |   ELC      emacs-lisp/timer-list.elc
> > |
> > | In testcover-analyze-coverage-wrapped-form:
> > | emacs-lisp/testcover.el:551:8: Warning: Nested ` are not supported
> >
> > But they should be trivial to fix (they are only warnings anyway).
> 
> Indeed, these are cases which would change meaning under the semantics
> that was proposed in this bug report.  The patch below would get rid of
> the nested backquotes, thus making that code "agnostic" to their semantics.

I'm not sure I understand: this is instead of the patch you posted
previously, or in addition to it?

bug#72328: [PATCH] Nested backquote in pcase

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

Eli Zaretskii <eliz@gnu.org> writes:

> > Indeed, these are cases which would change meaning under the semantics
> > that was proposed in this bug report.  The patch below would get rid of
> > the nested backquotes, thus making that code "agnostic" to their
> > semantics.
>
> I'm not sure I understand: this is instead of the patch you posted
> previously, or in addition to it?

In addition, Eli: the first patch revealed two uses in "testcover.el"
that now generate warnings when compiled.  Stefan's second patch
silences these new warnings.

Michael.

bug#72328: [PATCH] Nested backquote in pcase

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  thuna.cing@gmail.com,
>   72328@debbugs.gnu.org
> Date: Sun, 22 Sep 2024 17:21:27 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > > Indeed, these are cases which would change meaning under the semantics
> > > that was proposed in this bug report.  The patch below would get rid of
> > > the nested backquotes, thus making that code "agnostic" to their
> > > semantics.
> >
> > I'm not sure I understand: this is instead of the patch you posted
> > previously, or in addition to it?
> 
> In addition, Eli: the first patch revealed two uses in "testcover.el"
> that now generate warnings when compiled.  Stefan's second patch
> silences these new warnings.

Thanks, then Stefan, please feel free to install.

bug#72328: [PATCH] Nested backquote in pcase

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

> Thanks, then Stefan, please feel free to install.

Done, thanks,


        Stefan

bug#72328: [PATCH] Nested backquote in pcase

[Adam Porter]

Hi all,

Please forgive the intrusion, just a quick suggestion: Should the 
warning message include "pcase" somewhere?  Unless I'm missing 
something, the message, not mentioning that, leaves little clue as to 
the source of the warning, reminiscent of the "Please avoid it" error 
which I had to grep the Emacs sources to find when I received it.  :)

--Adam

bug#72328: [PATCH] Nested backquote in pcase

[Eli Zaretskii]

> Date: Wed, 25 Sep 2024 14:04:57 -0500
> Cc: eliz@gnu.org, michael_heerdegen@web.de, monnier@iro.umontreal.ca,
>  thuna.cing@gmail.com
> From: Adam Porter <adam@alphapapa.net>
> 
> Please forgive the intrusion, just a quick suggestion: Should the 
> warning message include "pcase" somewhere?  Unless I'm missing 
> something, the message, not mentioning that, leaves little clue as to 
> the source of the warning, reminiscent of the "Please avoid it" error 
> which I had to grep the Emacs sources to find when I received it.  :)

I agree.

bug#72328: [PATCH] Nested backquote in pcase

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

> Please forgive the intrusion, just a quick suggestion: Should the warning
> message include "pcase" somewhere?  Unless I'm missing something, the
> message, not mentioning that, leaves little clue as to the source of the
> warning, reminiscent of the "Please avoid it" error which I had to grep the
> Emacs sources to find when I received it.  :)

Excellent point, thanks.  I'll try and fix it later today along with the
problem Andrea bumped into.


        Stefan

bug#72328: [PATCH] Nested backquote in pcase

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

> Excellent point, thanks.  I'll try and fix it later today along with the
> problem Andrea bumped into.

Should be fixed now.


        Stefan

bug#72328: [PATCH] Nested backquote in pcase

[Thuna]

(Just to address the mail I'm replying to: )
>>> Would be good to hear from Stefan
>> The first step is to declare nested backquotes unsupported and add a
>> warning when we encounter them.
> Where and how would you suggest to do that?

If we say that a leading backquote[1] is unsupported in the backquote
pattern, then we can just add at to the start of the `(consp qpat)'
branch in `pcase--expand-\`' a check like

  (when (eq (car qpat) '\`)
    (warn ...))

and that would do.[2]

I do not really think that making the nested backquotes unsupported is a
good idea - this leaves the only (legal) way to express them in a
backquote pattern as `(,'\` <thing>) which is the thing I am trying to
fix for commas in the first place.  I would rather the original behavior
be documented and marked as intended than to have backquotes be illegal
entirely.

---

Sorry for the radio silence.  I have been a tiny bit occupied and
haven't been able to spend enough time on this.  I still do not really
have any examples I can show, but I have been slowly gathering some
thoughts on this, and I want to put them out there.

>>> Thinking more about it, from the things I can see, I think I'm in favor
>>> of this change.  In the long run it makes Elisp more consistent.

I went in the opposite direction.  The more I think about it, and the
more I try to write examples this would negatively impact, the less
attractive this change starts to look.

>>> I don't know how often backquote values are needed to be matched.  I
>>> guess not terribly often.

This is unfortunately a part of the reason this discussion is not really
going anywhere (with the other part being me, sorry about that!); this
is a *really* niche feature.  I expect that the number of people using
it does not exceed two digits, if even that.

>>> OTOH, this ,'\, circus really can drive none crazy, it's not obvious
>>> what one needs to do at all to match a backquote value correctly, so
>>> it's also an improvement to enable people to do this in a reachable
>>> way, and the code also gets better readable.

Yes, that is what I was hoping for with this change, too, but...

>>> OTOH, I think the current semantics of pcase ` are easier to explain in
>>> a formal way.  Dunno if this is an advantage though, since most people
>>> don't seem to understand them anyway.

...this is really the issue: The old behavior is very easy to wrap your
head around, and its only downside is its inability to express a literal
comma match, from which extends the inability to match against /results/
of a backquote form.

On the other hand, when you are "out of bounds" of the new behavior -
which is whenever you are trying to apply pcase patterns to a
technically quoted form within the pcase[3] - you need to create a
pattern where the normal backquotes go only as deep as your shallowest
comma[4] with the result of the backquotes written using ,'\`.

>>> Thuna, do you have any other ideas where your patch would be a
>>> significant improvement - practical use cases?  I must admit I'm not yet
>>> completely sure how the impact of the patch would on using el-search
>>> - although I often use it to search backquote expressions.

Unfortunately, given the problem above, I would need to discover an
improvement that I don't even know of yet to justify advocating for my
patch.

---

This idea is not yet fleshed out, and it is very complicated one, both
conceptually and implementationally, but it would help maintain
backwards compatibility while allowing a reasonable representation of
literal comma forms in backquote patterns:

What if a ,PAT form would match against PAT as though it were a pcase
pattern /if any only if/ PAT would not error due to a misplaced call to
a comma pattern, and otherwise ,PAT would match against a pcase
backquote subpattern (\, PAT)?

The way it's stated above is not really illustrative, so let me also
explain how it works (as far as everyone will be concerned): Any comma
you put before the actual final comma is matched literally.

Trying to match against an (unevaluated) object of the form

  `(<sybmol> ,<integer>)

would then be done using the pattern

  `(,(pred symbolp) ,,(pred integerp))

and an (evaluated) object like

  ``(let ,(list . <anything>)
      ,,body)

would be matched using the pattern

  ``(let ,(list . ,_)
      ,body)


This comes with its downsides, one of which is that you need to
eventually match /some/ pcase pattern to be able to insert a comma.  For
example, if we had swtiched the object above to

  ``(let ,(list)
      ,,body)

you would not be able to do

  ``(let ,(list)
      ,body)

or

  ``(let ,,(list)
      ,body)

because they would both try to match against `(list)' as though it were
a pcase pattern, but there is a solution, which is to just put a quote
afterwards:

  ``(let ,,'(list)
      ,body)

The actual thing that I have not yet managed to work out is whether this
could cause a significant amount of false positives, that is, silently
matching against a literal comma where an escaping one was intended.  I
am unable to even construct an example, so it may be that it's not
actually possible, but it may also be that I am just not finding it
which I can't discount.

There is some issue with the way this works "backwards", in that you
need to give it more commas the less commas it has (or rather as many
commas as it is apart from the nesting of backquotes - this is difficult
to even explain!), but I think a simplistic interpretation of it as
"give it as many commas as you want to match" would alleviate this
somewhat.

I have a very rough implementation, but it's not actually working
properly and needs a very big overhaul, so I won't (can't) provide it
for others to check out how this would work.  It should be feasible to
do the calculation in your head, so please do that, sorry!

---

[1] Currently, pcase doesn't actually check that a comma form is
"valid", in that it must be a proper list of length 2, but only checks
that the first element in the list is a comma.  As a result, something
like

  (pcase '(1) (`((\, (pred integerp) (error))) t)) ;; => t

"works" even though it should either be a macroexpansion error or
equivalent to the pattern '((\, (pred integerp) (error))).

[2] We could be a bit fancier and have a single pcase form only warn
once with some bookkeeping, but that would still essentially be
something like

  (when (and (eq (car qpat) '\`)
             (not pcase--bad-\`-warnedp))
    (setq pcase--bad-\`-warnedp t)
    (warn ...))

[3] Something like the unevaluated `(<sybmol> ,<integer>) where <symbol>
is quoted as far as the backquote is concerned.

[4] This means that in the example in [3] you have to escape ALL
backquotes with ',\`.

bug#72328: [PATCH] Nested backquote in pcase

[Thuna]

>> > And this change would break the symmetry between `backquote' the macro
>> > and backquote patterns in `pcase'.  This is an important design idea.
>>
>> I am not quite sure why you think that this breaks symmetry with the
>> backquote macro;
>
> We want that something like this works as expected:
>
> #+begin_src emacs-lisp
> (let ((a 1) (b 2))
>   (pcase `(69 foo (97 ,a ((,b))))
>     (`(69 foo (97 ,a ((,b))))
>     (list a b)))) ==> (1 2)
> #+end_src

I feel like there is a possible misunderstanding.  I am not preventing a
backquote pattern from going deeper into a list.  Your example would
remain perfectly consistent under the new behavior.  What *wouldn't*,
however, is something like

(let ((a 1) (b 2))
 (pcase `(69 foo `(,bar ,,a ((,,b))))
   (`(69 foo `(,bar ,,a ((,,b))))
    (list a b))))

Which currently errors on two counts: 1. ,,a in the pattern first
expands the initial , as though it escape the backquote pattern (not the
one before (,bar...) but the one before (69...))  and tries to match the
object - which is (\, 1) - against the pattern ,a which is of course
nonsense so the macroexpansion fails.  2. ,bar in the pattern tries to
match the corresponding object - which is (\, bar) - against the pattern
`bar' which binds the object - once again (\, bar) - to `bar'.

Do you disagree that the result of this form should also be (1 2)?

> I think it's obvious what I mean with symmetry between backquote and
> pcase backquote without giving a formal definition.

Unless our definitions of symmetry are different, the example above is a
clear situation where the current behavior does not have symmetry with
the backquote macro.

> It indeed gets ugly when one wants to match a non-constant
> backquote expression using a backquote pcase pattern with partial
> unquotes (the "mixed case").
>
> But making pcase backquote patterns less expressive just to make this
> special case simpler doesn't make sense to me.  OTOH I agree that having
> a convenient solution for this kind of problem would be nice.

The drawback of this new behavior would be that you could not match,
say, the unevaluated object ``10 against ``<some-pattern> where
<some-pattern> indicates an integer, as ``,,(pred integer) would match
against a `(,'\` (,'\, ,(pred integer))).  We can still match it, but
only with the same ugly syntax as before: `(,'\` ,(pred integer)).

While this is not ideal, I still consider the new behavior an
improvement.

bug#72328: [PATCH] Nested backquote in pcase

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

Thuna <thuna.cing@gmail.com> writes:

> Your example would remain perfectly consistent under the new behavior.
> What *wouldn't*, however, is something like
>
> (let ((a 1) (b 2))
>  (pcase `(69 foo `(,bar ,,a ((,,b))))
>    (`(69 foo `(,bar ,,a ((,,b))))
>     (list a b))))
>
> Which currently errors on two counts: 1. ,,a in the pattern first
> expands the initial , as though it escape the backquote pattern (not the
> one before (,bar...) but the one before (69...))  and tries to match the
> object - which is (\, 1) - against the pattern ,a which is of course
> nonsense so the macroexpansion fails.  2. ,bar in the pattern tries to
> match the corresponding object - which is (\, bar) - against the pattern
> `bar' which binds the object - once again (\, bar) - to `bar'.

I see, thanks.  Yes, I misinterpreted what you wanted to do in your
patch.


> Do you disagree that the result of this form should also be (1 2)?

Maybe.  I have to think about it.


Michael.

bug#72328: [PATCH] Nested backquote in pcase

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

Thuna <thuna.cing@gmail.com> writes:

> I feel like there is a possible misunderstanding.  I am not preventing a
> backquote pattern from going deeper into a list.  Your example would
> remain perfectly consistent under the new behavior.  What *wouldn't*,
> however, is something like
>
> (let ((a 1) (b 2))
>  (pcase `(69 foo `(,bar ,,a ((,,b))))
>    (`(69 foo `(,bar ,,a ((,,b))))
>     (list a b))))

Ok - so backquote the macro handles nested invocations of backquote
specially (the nested calls are not expanded individually, only the
outermost backquote expression gets expanded), while the pcase' backquote
implementation is backquote agnostic, it currently treats it like any
random symbol.  Your patch tries to adapt pcase backquote to the
backquote macro semantics.  Correct?

If there aren't any downsides then this makes a lot of sense and would
be a good thing to do indeed.

Let's wait for Stefan then.


Michael.

bug#72328: [PATCH] Nested backquote in pcase

[Thuna]

>> I feel like there is a possible misunderstanding.  I am not preventing a
>> backquote pattern from going deeper into a list.  Your example would
>> remain perfectly consistent under the new behavior.  What *wouldn't*,
>> however, is something like
>>
>> (let ((a 1) (b 2))
>>  (pcase `(69 foo `(,bar ,,a ((,,b))))
>>    (`(69 foo `(,bar ,,a ((,,b))))
>>     (list a b))))
>
> Ok - so backquote the macro handles nested invocations of backquote
> specially (the nested calls are not expanded individually, only the
> outermost backquote expression gets expanded), while the pcase' backquote
> implementation is backquote agnostic, it currently treats it like any
> random symbol.  Your patch tries to adapt pcase backquote to the
> backquote macro semantics.  Correct?

Yes, that is precisely the case.  I'm afraid I wasn't able to explain it
well enough.

> If there aren't any downsides then this makes a lot of sense and would
> be a good thing to do indeed.

The one downside that I can see is the situation I mentioned in my
previous message where you explicitly *want* to bypass all backquotes,
plus the fact that this is backwards incompatible, even though to be
effected you would need to be making use of the fact that comma behaves
this way - which I still don't believe will be too common - though there
could be an obsoletion period where comma still bypasses backquotes but
warns the user to change off of that behavior.  All code making use of
the new behavior will firmly be incompatible with older versions of
emacs for which this is not patched, however.

bug#72328: [PATCH] Nested backquote in pcase

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

Thuna <thuna.cing@gmail.com> writes:

> The one downside that I can see is the situation I mentioned in my
> previous message where you explicitly *want* to bypass all backquotes,
> plus the fact that this is backwards incompatible, even though to be
> effected you would need to be making use of the fact that comma behaves
> this way - which I still don't believe will be too common - though there
> could be an obsoletion period where comma still bypasses backquotes but
> warns the user to change off of that behavior.  All code making use of
> the new behavior will firmly be incompatible with older versions of
> emacs for which this is not patched, however.

Ok.

I actually found something I wrote myself.  In "el-search" I am using a
pattern like used here:

  (pcase '`qpat (``,qpat qpat)) --> qpat

i.e. a pattern checking for backquoted QPATS.  When your patch is
installed this pattern doesn't match any more.  I could rewrite that of
course - I just wanted to share the example without implying anything
else.


Michael.