GNU bug report logs - #57502
29.0.50; Issue with `or' clause of buffer-match-p

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 57502 in the body.
You can then email your comments to 57502 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; Issue with `or' clause of buffer-match-p
Date: Wed, 31 Aug 2022 14:02:46 +0200

This buffer-match-p condition does the expected job:

     (buffer-match-p '(or "\\*" (derived-mode . special-mode))
                     (current-buffer))

But this presumably equivalent one gives a “(wrong-type-argument listp
special-mode)” error:

     (buffer-match-p '(or (and "\\*")
                          (derived-mode . special-mode))
                     (current-buffer))

Message #8 received at 57502 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 57502 <at> debbugs.gnu.org
Subject: Re: bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
Date: Wed, 31 Aug 2022 12:47:07 +0000

[Message part 1 (text/plain, inline)]

Augusto Stoffel <arstoffel <at> gmail.com> writes:

> This buffer-match-p condition does the expected job:
>
>      (buffer-match-p '(or "\\*" (derived-mode . special-mode))
>                      (current-buffer))
>
> But this presumably equivalent one gives a “(wrong-type-argument listp
> special-mode)” error:
>
>      (buffer-match-p '(or (and "\\*")
>                           (derived-mode . special-mode))
>                      (current-buffer))

It seems to me that the issue is related to the `and' being wrapped by
an `or', specifically because of a typo in the handling of `and':

[Message part 2 (text/plain, inline)]

diff --git a/lisp/subr.el b/lisp/subr.el
index 2ffc594997..0350c16ccf 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -7014,8 +7014,8 @@ buffer-match-p
                       (funcall match (cdr condition)))
                      ((eq (car-safe condition) 'and)
                       (catch 'fail
-                        (dolist (c (cdr conditions))
-                          (unless (funcall match c)
+                        (dolist (c (cdr condition))
+                          (unless (funcall match (list c))
                             (throw 'fail nil)))
                         t)))
                 (throw 'match t)))))))

[Message part 3 (text/plain, inline)]

As you have pointed out to me privately, it might make sense to rewrite
the case distinction using pcase, to avoid simple mistakes like these.

Message #11 received at 57502 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 57502 <at> debbugs.gnu.org
Subject: Re: bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
Date: Wed, 31 Aug 2022 12:50:14 +0000

[Message part 1 (text/plain, inline)]

Philip Kaludercic <philipk <at> posteo.net> writes:

> Augusto Stoffel <arstoffel <at> gmail.com> writes:
>
>> This buffer-match-p condition does the expected job:
>>
>>      (buffer-match-p '(or "\\*" (derived-mode . special-mode))
>>                      (current-buffer))
>>
>> But this presumably equivalent one gives a “(wrong-type-argument listp
>> special-mode)” error:
>>
>>      (buffer-match-p '(or (and "\\*")
>>                           (derived-mode . special-mode))
>>                      (current-buffer))
>
> It seems to me that the issue is related to the `and' being wrapped by
> an `or', specifically because of a typo in the handling of `and':
>
> diff --git a/lisp/subr.el b/lisp/subr.el
> index 2ffc594997..0350c16ccf 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -7014,8 +7014,8 @@ buffer-match-p
>                        (funcall match (cdr condition)))
>                       ((eq (car-safe condition) 'and)
>                        (catch 'fail
> -                        (dolist (c (cdr conditions))
> -                          (unless (funcall match c)
> +                        (dolist (c (cdr condition))
> +                          (unless (funcall match (list c))
>                              (throw 'fail nil)))
>                          t)))
>                  (throw 'match t)))))))
>
>
> As you have pointed out to me privately, it might make sense to rewrite
> the case distinction using pcase, to avoid simple mistakes like these.

That might look something like this:

[Message part 2 (text/plain, inline)]

diff --git a/lisp/subr.el b/lisp/subr.el
index 2ffc594997..db1dc25044 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -6992,32 +6992,32 @@ buffer-match-p
         (lambda (conditions)
           (catch 'match
             (dolist (condition conditions)
-              (when (cond
-                     ((eq condition t))
-                     ((stringp condition)
-                      (string-match-p condition (buffer-name buffer)))
-                     ((functionp condition)
-                      (if (eq 1 (cdr (func-arity condition)))
-                          (funcall condition buffer)
-                        (funcall condition buffer arg)))
-                     ((eq (car-safe condition) 'major-mode)
-                      (eq
-                       (buffer-local-value 'major-mode buffer)
-                       (cdr condition)))
-                     ((eq (car-safe condition) 'derived-mode)
-                      (provided-mode-derived-p
-                       (buffer-local-value 'major-mode buffer)
-                       (cdr condition)))
-                     ((eq (car-safe condition) 'not)
-                      (not (funcall match (cdr condition))))
-                     ((eq (car-safe condition) 'or)
-                      (funcall match (cdr condition)))
-                     ((eq (car-safe condition) 'and)
-                      (catch 'fail
-                        (dolist (c (cdr conditions))
-                          (unless (funcall match c)
-                            (throw 'fail nil)))
-                        t)))
+              (when (pcase condition
+                      ('t t)
+                      ((pred stringp)
+                       (string-match-p condition (buffer-name buffer)))
+                      ((pred functionp)
+                       (if (eq 1 (cdr (func-arity condition)))
+                           (funcall condition buffer)
+                         (funcall condition buffer arg)))
+                      (`(major-mode . ,mode)
+                       (eq
+                        (buffer-local-value 'major-mode buffer)
+                        mode))
+                      (`(derived-mode . ,mode)
+                       (provided-mode-derived-p
+                        (buffer-local-value 'major-mode buffer)
+                        mode))
+                      (`(not . cond)
+                       (not (funcall match cond)))
+                      (`(or . ,args)
+                       (funcall match args))
+                      (`(and . ,args)
+                       (catch 'fail
+                         (dolist (c args)
+                           (unless (funcall match (list c))
+                             (throw 'fail nil)))
+                         t)))
                 (throw 'match t)))))))
     (funcall match (list condition))))
 

Message #14 received at 57502 <at> debbugs.gnu.org (full text, mbox):

From: Augusto Stoffel <arstoffel <at> gmail.com>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 57502 <at> debbugs.gnu.org
Subject: Re: bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
Date: Wed, 31 Aug 2022 18:22:15 +0200

On Wed, 31 Aug 2022 at 12:50, Philip Kaludercic <philipk <at> posteo.net> wrote:

> That might look something like this:
>
[...]
> +                      (`(derived-mode . ,mode)
> +                       (provided-mode-derived-p
> +                        (buffer-local-value 'major-mode buffer)
> +                        mode))

On a tangent, wouldn't it be nice to allow a list of modes?  That is,

> +                      (`(derived-mode . ,modes)
> +                       (apply #'provided-mode-derived-p
> +                        (buffer-local-value 'major-mode buffer)
> +                        modes))

(with some extra care to keep backwards compatibility).

Message #17 received at 57502 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 57502 <at> debbugs.gnu.org
Subject: Re: bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
Date: Wed, 31 Aug 2022 16:30:23 +0000

Augusto Stoffel <arstoffel <at> gmail.com> writes:

> On Wed, 31 Aug 2022 at 12:50, Philip Kaludercic <philipk <at> posteo.net> wrote:
>
>> That might look something like this:
>>
> [...]
>> +                      (`(derived-mode . ,mode)
>> +                       (provided-mode-derived-p
>> +                        (buffer-local-value 'major-mode buffer)
>> +                        mode))
>
> On a tangent, wouldn't it be nice to allow a list of modes?  That is,
>
>> +                      (`(derived-mode . ,modes)
>> +                       (apply #'provided-mode-derived-p
>> +                        (buffer-local-value 'major-mode buffer)
>> +                        modes))
>
> (with some extra care to keep backwards compatibility).

Intuitively I feel as though this could be more problematic/confusing
than convenient.  If this is done, then it should be supported in every
case, so that

      (and (derived-mode foo-mode)
           (major-mode . bar-mode))

(I know this is a stupid example) doesn't make someone want to try

      (and (derived-mode foo-mode)
           (major-mode bar-mode))

and be confused why it fails.

Message #20 received at 57502 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Augusto Stoffel <arstoffel <at> gmail.com>
Cc: 57502 <at> debbugs.gnu.org
Subject: Re: bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
Date: Sat, 03 Sep 2022 11:04:20 +0000

Philip Kaludercic <philipk <at> posteo.net> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
>> Augusto Stoffel <arstoffel <at> gmail.com> writes:
>>
>>> This buffer-match-p condition does the expected job:
>>>
>>>      (buffer-match-p '(or "\\*" (derived-mode . special-mode))
>>>                      (current-buffer))
>>>
>>> But this presumably equivalent one gives a “(wrong-type-argument listp
>>> special-mode)” error:
>>>
>>>      (buffer-match-p '(or (and "\\*")
>>>                           (derived-mode . special-mode))
>>>                      (current-buffer))
>>
>> It seems to me that the issue is related to the `and' being wrapped by
>> an `or', specifically because of a typo in the handling of `and':
>>
>> diff --git a/lisp/subr.el b/lisp/subr.el
>> index 2ffc594997..0350c16ccf 100644
>> --- a/lisp/subr.el
>> +++ b/lisp/subr.el
>> @@ -7014,8 +7014,8 @@ buffer-match-p
>>                        (funcall match (cdr condition)))
>>                       ((eq (car-safe condition) 'and)
>>                        (catch 'fail
>> -                        (dolist (c (cdr conditions))
>> -                          (unless (funcall match c)
>> +                        (dolist (c (cdr condition))
>> +                          (unless (funcall match (list c))
>>                              (throw 'fail nil)))
>>                          t)))
>>                  (throw 'match t)))))))
>>
>>
>> As you have pointed out to me privately, it might make sense to rewrite
>> the case distinction using pcase, to avoid simple mistakes like these.
>
> That might look something like this:
>
> diff --git a/lisp/subr.el b/lisp/subr.el
> index 2ffc594997..db1dc25044 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -6992,32 +6992,32 @@ buffer-match-p
>          (lambda (conditions)
>            (catch 'match
>              (dolist (condition conditions)
> -              (when (cond
> -                     ((eq condition t))
> -                     ((stringp condition)
> -                      (string-match-p condition (buffer-name buffer)))
> -                     ((functionp condition)
> -                      (if (eq 1 (cdr (func-arity condition)))
> -                          (funcall condition buffer)
> -                        (funcall condition buffer arg)))
> -                     ((eq (car-safe condition) 'major-mode)
> -                      (eq
> -                       (buffer-local-value 'major-mode buffer)
> -                       (cdr condition)))
> -                     ((eq (car-safe condition) 'derived-mode)
> -                      (provided-mode-derived-p
> -                       (buffer-local-value 'major-mode buffer)
> -                       (cdr condition)))
> -                     ((eq (car-safe condition) 'not)
> -                      (not (funcall match (cdr condition))))
> -                     ((eq (car-safe condition) 'or)
> -                      (funcall match (cdr condition)))
> -                     ((eq (car-safe condition) 'and)
> -                      (catch 'fail
> -                        (dolist (c (cdr conditions))
> -                          (unless (funcall match c)
> -                            (throw 'fail nil)))
> -                        t)))
> +              (when (pcase condition
> +                      ('t t)
> +                      ((pred stringp)
> +                       (string-match-p condition (buffer-name buffer)))
> +                      ((pred functionp)
> +                       (if (eq 1 (cdr (func-arity condition)))
> +                           (funcall condition buffer)
> +                         (funcall condition buffer arg)))
> +                      (`(major-mode . ,mode)
> +                       (eq
> +                        (buffer-local-value 'major-mode buffer)
> +                        mode))
> +                      (`(derived-mode . ,mode)
> +                       (provided-mode-derived-p
> +                        (buffer-local-value 'major-mode buffer)
> +                        mode))
> +                      (`(not . cond)
> +                       (not (funcall match cond)))
> +                      (`(or . ,args)
> +                       (funcall match args))
> +                      (`(and . ,args)
> +                       (catch 'fail
> +                         (dolist (c args)
> +                           (unless (funcall match (list c))
> +                             (throw 'fail nil)))
> +                         t)))
>                  (throw 'match t)))))))
>      (funcall match (list condition))))

Are there any objections against applying this change?  From what I see
pcase is used elsewhere in seq, so it should be possible to recognise
and expand the macro, right?

Message #23 received at 57502 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 57502 <at> debbugs.gnu.org, arstoffel <at> gmail.com
Subject: Re: bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
Date: Sat, 03 Sep 2022 14:10:50 +0300

> Cc: 57502 <at> debbugs.gnu.org
> From: Philip Kaludercic <philipk <at> posteo.net>
> Date: Sat, 03 Sep 2022 11:04:20 +0000
> 
> Are there any objections against applying this change?  From what I see
> pcase is used elsewhere in seq, so it should be possible to recognise
> and expand the macro, right?

I think we should avoid using pcase in subr.el, since subr.el is
loaded by loadup.el before pcase.  But I see that this ship has sailed
already, sigh.

Message #26 received at 57502 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 57502 <at> debbugs.gnu.org, arstoffel <at> gmail.com
Subject: Re: bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
Date: Sat, 03 Sep 2022 11:19:55 +0000

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Cc: 57502 <at> debbugs.gnu.org
>> From: Philip Kaludercic <philipk <at> posteo.net>
>> Date: Sat, 03 Sep 2022 11:04:20 +0000
>> 
>> Are there any objections against applying this change?  From what I see
>> pcase is used elsewhere in seq, so it should be possible to recognise
>> and expand the macro, right?
>
> I think we should avoid using pcase in subr.el, since subr.el is
> loaded by loadup.el before pcase.  But I see that this ship has sailed
> already, sigh.

There is no insistence from my end to use pcase, especially because at
first I assumed it ought not to be used in subr.el.  But from what I see
there have been instances of the macro (in `called-interactively-p') for
almost ten years now.  So unless there is a plan to revert this trend,
I'd use pcase to avoid simple issues like the one that caused this bug.

Message #29 received at 57502 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 57502 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, arstoffel <at> gmail.com
Subject: Re: bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
Date: Sat, 03 Sep 2022 13:54:08 +0200

Philip Kaludercic <philipk <at> posteo.net> writes:

> So unless there is a plan to revert this trend, I'd use pcase to avoid
> simple issues like the one that caused this bug.

Looks good to me.

Message #34 received at 57502-done <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 57502-done <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 arstoffel <at> gmail.com
Subject: Re: bug#57502: 29.0.50; Issue with `or' clause of buffer-match-p
Date: Sat, 03 Sep 2022 12:56:13 +0000

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
>> So unless there is a plan to revert this trend, I'd use pcase to avoid
>> simple issues like the one that caused this bug.
>
> Looks good to me.

Ok, I've pushed the change.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.