bug#58951: [PATCH] ; Fix handling of 'not' by 'buffer-match-p'

bug#58951: [PATCH] ; Fix handling of 'not' by 'buffer-match-p'

[Philip Kaludercic]

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

Tags: patch


In preparing bug#58950 I noticed that the 'not' clause is confusing, and
is misused on the place I could find it being used in the core
(show-paren-predicate).  The current implementation would require a
negation to be written as

    (not . CONDITION)

while it is more natural to write

    (not CONDITION)

which is more in line with (and ...) and (or ...).

The issue appears to go back to `project--buffer-check', that takes a
list of conditions instead of a single one.  This means that the above
are equivalent. since (not CONDITION) will check each element in the
unary list in (not . (CONDITION)).

I believe this is preferable to fixing `show-paren-predicate', as this
is the kind of issue a lot of people could trip over.

This patch is based on the patch from bug#58950, but can be back-ported
to the previous implementations if there are any issues with that report.

In GNU Emacs 29.0.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version
 3.24.30, cairo version 1.16.0) of 2022-10-31 built on heron
Repository revision: 462a66e79edcc34ecbeef7cc1604765adfdc038e
Repository branch: feature/package+vc
System Description: Guix System

Configured using:
 'configure --with-pgtk --with-imagemagick
 PKG_CONFIG_PATH=/gnu/store/ssg343s6ldqdwh30136pnawhbgd0cb6i-profile/lib/pkgconfig:/gnu/store/ssg343s6ldqdwh30136pnawhbgd0cb6i-profile/share/pkgconfig'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-handling-of-not-by-buffer-match-p.patch --]
[-- Type: text/patch, Size: 1351 bytes --]

From cb88a512eacb3ad73c22682c9c794cdc80445f43 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Tue, 1 Nov 2022 20:27:17 +0100
Subject: [PATCH] ; Fix handling of 'not' by 'buffer-match-p'

* lisp/subr.el (buffer-match-p): Look up the cadr instead of the cdr
for the negation in 'not'.
---
 lisp/subr.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index b667339db9..0252e66686 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -7023,7 +7023,7 @@ string-lines
                `(provided-mode-derived-p
                  (buffer-local-value 'major-mode ,buffer-sym)
                  ',mode))
-              (`(not . ,cond)
+              (`(not ,cond)
                `(not ,(funcall translate cond)))
               (`(or . ,conds)
                `(or ,@(mapcar translate conds)))
@@ -7045,7 +7045,7 @@ string-lines
   * `major-mode': the buffer matches if the buffer's major mode
     is eq to the cons-cell's cdr.  Prefer using `derived-mode'
     instead when both can work.
-  * `not': the cdr is interpreted as a negation of a condition.
+  * `not': the cadr is interpreted as a negation of a condition.
   * `and': the cdr is a list of recursive conditions, that all have
     to be met.
   * `or': the cdr is a list of recursive condition, of which at
-- 
2.38.0

bug#58951: Acknowledgement ([PATCH] ; Fix handling of 'not' by 'buffer-match-p')

[Philip Kaludercic]

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


I forgot to update the documentation in buffers.texi:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-handling-of-not-by-buffer-match-p.patch --]
[-- Type: text/x-patch, Size: 2691 bytes --]

From 279f9561dd115d707b086efdbc19072fceb7a99f Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Tue, 1 Nov 2022 20:27:17 +0100
Subject: [PATCH] ; Fix handling of 'not' by 'buffer-match-p'

* lisp/subr.el (buffer-match-p): Look up the cadr instead of the cdr
for the negation in 'not'.
* doc/lispref/buffers.texi (Buffer List): Update documentation.
---
 doc/lispref/buffers.texi | 18 ++++++++----------
 lisp/subr.el             |  4 ++--
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/doc/lispref/buffers.texi b/doc/lispref/buffers.texi
index c40e088293..8405e28387 100644
--- a/doc/lispref/buffers.texi
+++ b/doc/lispref/buffers.texi
@@ -977,17 +977,15 @@ Buffer List
 A cons-cell @code{(@var{oper} . @var{expr})} where @var{oper} is one
 of
 @table @code
-@item not
-Satisfied if @var{expr} doesn't satisfy @code{buffer-match-p} with
+@item (not @var{cond})
+Satisfied if @var{cond} doesn't satisfy @code{buffer-match-p} with
 the same buffer and @code{arg}.
-@item or
-Satisfied if @var{expr} is a list and @emph{any} condition in
-@var{expr} satisfies @code{buffer-match-p}, with the same buffer and
-@code{arg}.
-@item and
-Satisfied if @var{expr} is a list and @emph{all} conditions in
-@var{expr} satisfy @code{buffer-match-p}, with the same buffer and
-@code{arg}.
+@item (or @var{conds}@dots{})
+Satisfied if and @emph{any} condition in @var{conds} satisfies
+@code{buffer-match-p}, with the same buffer and @code{arg}.
+@item (and @var{conds}@dots{})
+Satisfied if @emph{all} conditions in @var{conds} satisfy
+@code{buffer-match-p}, with the same buffer and @code{arg}.
 @item derived-mode
 Satisfied if the buffer's major mode derives from @var{expr}.
 @item major-mode
diff --git a/lisp/subr.el b/lisp/subr.el
index b667339db9..0252e66686 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -7023,7 +7023,7 @@ string-lines
                `(provided-mode-derived-p
                  (buffer-local-value 'major-mode ,buffer-sym)
                  ',mode))
-              (`(not . ,cond)
+              (`(not ,cond)
                `(not ,(funcall translate cond)))
               (`(or . ,conds)
                `(or ,@(mapcar translate conds)))
@@ -7045,7 +7045,7 @@ string-lines
   * `major-mode': the buffer matches if the buffer's major mode
     is eq to the cons-cell's cdr.  Prefer using `derived-mode'
     instead when both can work.
-  * `not': the cdr is interpreted as a negation of a condition.
+  * `not': the cadr is interpreted as a negation of a condition.
   * `and': the cdr is a list of recursive conditions, that all have
     to be met.
   * `or': the cdr is a list of recursive condition, of which at
-- 
2.38.0

bug#58951: [PATCH] ; Fix handling of 'not' by 'buffer-match-p'

[Dmitry Gutov]

On 01.11.2022 21:36, Philip Kaludercic wrote:
> I believe this is preferable to fixing `show-paren-predicate', as this
> is the kind of issue a lot of people could trip over.
> 
> This patch is based on the patch from bug#58950, but can be back-ported
> to the previous implementations if there are any issues with that report.

I do agree that (not foo) is more convenient than (not . foo). Though 
the latter would be more regular compared to the rest of the syntax. I 
made just that mistake yesterday.

Let's change 'pcase' to 'pcase-exhaustive', though? So it will signal an 
error on invalid syntax.

bug#58951: [PATCH] ; Fix handling of 'not' by 'buffer-match-p'

[Philip Kaludercic]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 01.11.2022 21:36, Philip Kaludercic wrote:
>> I believe this is preferable to fixing `show-paren-predicate', as this
>> is the kind of issue a lot of people could trip over.
>> This patch is based on the patch from bug#58950, but can be
>> back-ported
>> to the previous implementations if there are any issues with that report.
>
> I do agree that (not foo) is more convenient than (not . foo). Though
> the latter would be more regular compared to the rest of the syntax. I
> made just that mistake yesterday.
>
> Let's change 'pcase' to 'pcase-exhaustive', though? So it will signal
> an error on invalid syntax.

Right, that is what I did in bug#58950 and what led me to notice the mistake.

bug#58951: [PATCH] ; Fix handling of 'not' by 'buffer-match-p'

[Dmitry Gutov]

On 02.11.2022 00:18, Philip Kaludercic wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
> 
>> On 01.11.2022 21:36, Philip Kaludercic wrote:
>>> I believe this is preferable to fixing `show-paren-predicate', as this
>>> is the kind of issue a lot of people could trip over.
>>> This patch is based on the patch from bug#58950, but can be
>>> back-ported
>>> to the previous implementations if there are any issues with that report.
>> I do agree that (not foo) is more convenient than (not . foo). Though
>> the latter would be more regular compared to the rest of the syntax. I
>> made just that mistake yesterday.
>>
>> Let's change 'pcase' to 'pcase-exhaustive', though? So it will signal
>> an error on invalid syntax.
> Right, that is what I did in bug#58950 and what led me to notice the mistake.

Oh, sorry. I didn't review that patch in detail yet.

LGTM then.

bug#58951: Acknowledgement ([PATCH] ; Fix handling of 'not' by 'buffer-match-p')

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Date: Tue, 01 Nov 2022 20:19:06 +0000
> 
> +@item (or @var{conds}@dots{})
> +Satisfied if and @emph{any} condition in @var{conds} satisfies
                ^^^
That "and" should be deleted.

> +@item (and @var{conds}@dots{})
> +Satisfied if @emph{all} conditions in @var{conds} satisfy
                ^^^^^^^^^^^^^^^^^^^^^
"@emph{all} the conditions"

Thanks.

bug#58951: [PATCH] ; Fix handling of 'not' by 'buffer-match-p'

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Date: Tue, 01 Nov 2022 20:19:06 +0000
>> 
>> +@item (or @var{conds}@dots{})
>> +Satisfied if and @emph{any} condition in @var{conds} satisfies
>                 ^^^
> That "and" should be deleted.
>
>> +@item (and @var{conds}@dots{})
>> +Satisfied if @emph{all} conditions in @var{conds} satisfy
>                 ^^^^^^^^^^^^^^^^^^^^^
> "@emph{all} the conditions"
>
> Thanks.

I have applied these changes and will push the fix to the release
branch.

bug#58951: [PATCH] ; Fix handling of 'not' by 'buffer-match-p'

[Mattias Engdegård]

>     (not . CONDITION)
> 
> while it is more natural to write
> 
>     (not CONDITION)

Shouldn't then the `derived-mode` and `major-mode` operators be changed in the same way?

And shouldn't there be tests? For all of `buffer-match-p`, in fact?

bug#58951: [PATCH] ; Fix handling of 'not' by 'buffer-match-p'

[Eli Zaretskii]

> Cc: 58951@debbugs.gnu.org
> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 31 Dec 2022 17:26:02 +0100
> 
> >     (not . CONDITION)
> > 
> > while it is more natural to write
> > 
> >     (not CONDITION)
> 
> Shouldn't then the `derived-mode` and `major-mode` operators be changed in the same way?

In fact, that change cause errors in timer function of
show-paren-mode, so I've reverted it for now.

> And shouldn't there be tests? For all of `buffer-match-p`, in fact?

Ideally, yes.