GNU bug report logs - #45328
28.0.50; [PATCH] Compare raw syntax descriptors with equal in `python-indent-region'

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 45328 in the body.
You can then email your comments to 45328 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: Andrea Corallo <akrl <at> sdf.org>
To: bug-gnu-emacs <at> gnu.org
Subject: 28.0.50; [PATCH] Compare raw syntax descriptors with equal in
 `python-indent-region'
Date: Sat, 19 Dec 2020 23:03:55 +0000

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

Hi,

debugging a native-comp regression on the branch I'm working on I came
to this.

In `python-indent-region' we are comparing raw syntax descriptors using
`eq' but I think we should use `equal' (as we do underneath in
`python-fill-string').

I'm quite convinced the fix is correct but; should this go also in 27
where the code is the same?

Regards

  Andrea

[0001-Compare-raw-syntax-descriptors-with-equal-in-python-.patch (text/x-diff, inline)]

From 9163724fcd8eea5c9e2908ecb38d90406669a8d3 Mon Sep 17 00:00:00 2001
From: Andrea Corallo <akrl <at> sdf.org>
Date: Sat, 19 Dec 2020 23:51:36 +0100
Subject: [PATCH] * Compare raw syntax descriptors with equal in
 `python-indent-region'.

	* lisp/progmodes/python.el (python-indent-region): Compare raw
	syntax descriptors with equal.
---
 lisp/progmodes/python.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index d75944a702..dc2efdfcf7 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -1259,7 +1259,7 @@ Called from a program, START and END specify the region to indent."
                    ;; Don't mess with strings, unless it's the
                    ;; enclosing set of quotes or a docstring.
                    (or (not (python-syntax-context 'string))
-                       (eq
+                       (equal
                         (syntax-after
                          (+ (1- (point))
                             (current-indentation)
-- 
2.20.1

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

From: Stefan Kangas <stefan <at> marxist.se>
To: Andrea Corallo <akrl <at> sdf.org>
Cc: 45328 <at> debbugs.gnu.org
Subject: Re: bug#45328: 28.0.50; [PATCH] Compare raw syntax descriptors with
 equal in `python-indent-region'
Date: Fri, 19 Mar 2021 21:51:39 -0500

Andrea Corallo <akrl <at> sdf.org> writes:

> In `python-indent-region' we are comparing raw syntax descriptors using
> `eq' but I think we should use `equal' (as we do underneath in
> `python-fill-string').
>
> I'm quite convinced the fix is correct but; should this go also in 27
> where the code is the same?

That was in December, and there were no followups.

We are one week away from Emacs 27.2, so I don't think we want this on
emacs-27 right now.  If you think the fix is correct, I don't see why we
couldn't push it there after Emacs 27.2 is released.

Should we perhaps start by getting the fix pushed to master?

>>From 9163724fcd8eea5c9e2908ecb38d90406669a8d3 Mon Sep 17 00:00:00 2001
> From: Andrea Corallo <akrl <at> sdf.org>
> Date: Sat, 19 Dec 2020 23:51:36 +0100
> Subject: [PATCH] * Compare raw syntax descriptors with equal in
>  `python-indent-region'.
>
> 	* lisp/progmodes/python.el (python-indent-region): Compare raw
> 	syntax descriptors with equal.
> ---
>  lisp/progmodes/python.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
> index d75944a702..dc2efdfcf7 100644
> --- a/lisp/progmodes/python.el
> +++ b/lisp/progmodes/python.el
> @@ -1259,7 +1259,7 @@ Called from a program, START and END specify the region to indent."
>                     ;; Don't mess with strings, unless it's the
>                     ;; enclosing set of quotes or a docstring.
>                     (or (not (python-syntax-context 'string))
> -                       (eq
> +                       (equal
>                          (syntax-after
>                           (+ (1- (point))
>                              (current-indentation)

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

From: Andrea Corallo <akrl <at> sdf.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 45328 <at> debbugs.gnu.org
Subject: Re: bug#45328: 28.0.50; [PATCH] Compare raw syntax descriptors with
 equal in `python-indent-region'
Date: Sun, 21 Mar 2021 21:19:33 +0000

Stefan Kangas <stefan <at> marxist.se> writes:

> Andrea Corallo <akrl <at> sdf.org> writes:
>
>> In `python-indent-region' we are comparing raw syntax descriptors using
>> `eq' but I think we should use `equal' (as we do underneath in
>> `python-fill-string').
>>
>> I'm quite convinced the fix is correct but; should this go also in 27
>> where the code is the same?
>
> That was in December, and there were no followups.
>
> We are one week away from Emacs 27.2, so I don't think we want this on
> emacs-27 right now.  If you think the fix is correct, I don't see why we
> couldn't push it there after Emacs 27.2 is released.
>
> Should we perhaps start by getting the fix pushed to master?

Uhmm, I thought the patch was correct but re-testing it this is now
breaking the `python-indent-region-5' test.

Maybe there's a reason why we should use eq there or maybe this is
unveiling a real bug?  Not sure, I guess some expert of that code should
probably have a look.

  Andrea

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Andrea Corallo <akrl <at> sdf.org>
Cc: Fabián Ezequiel Gallina <fgallina <at> gnu.org>,
 45328 <at> debbugs.gnu.org, Stefan Kangas <stefan <at> marxist.se>
Subject: Re: bug#45328: 28.0.50; [PATCH] Compare raw syntax descriptors with
 equal in `python-indent-region'
Date: Mon, 17 May 2021 17:06:03 +0200

Andrea Corallo <akrl <at> sdf.org> writes:

> Uhmm, I thought the patch was correct but re-testing it this is now
> breaking the `python-indent-region-5' test.
>
> Maybe there's a reason why we should use eq there or maybe this is
> unveiling a real bug?  Not sure, I guess some expert of that code should
> probably have a look.

The code in question is:

                   ;; Don't mess with strings, unless it's the
                   ;; enclosing set of quotes or a docstring.
                   (or (not (python-syntax-context 'string))
                       (eq
                        (syntax-after
                         (+ (1- (point))
                            (current-indentation)
                            (python-syntax-count-quotes (char-after) (point))))
                        (string-to-syntax "|"))
                       (python-info-docstring-p))

That `eq' form will never return anything other than nil, so removing it
gives us identical results.  So I'm not quite sure what it's trying to
do -- it's probably meant to be `equal', but that will (as you've found
out) lead to mis-indenting some code.

So I've added Fabián to the CCs; perhaps he has some comments.

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

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Andrea Corallo <akrl <at> sdf.org>
Cc: 45328 <at> debbugs.gnu.org, Stefan Kangas <stefan <at> marxist.se>
Subject: Re: bug#45328: 28.0.50; [PATCH] Compare raw syntax descriptors with
 equal in `python-indent-region'
Date: Mon, 09 May 2022 15:54:56 +0200

Andrea Corallo <akrl <at> sdf.org> writes:

> Uhmm, I thought the patch was correct but re-testing it this is now
> breaking the `python-indent-region-5' test.
>
> Maybe there's a reason why we should use eq there or maybe this is
> unveiling a real bug?  Not sure, I guess some expert of that code should
> probably have a look.

I tried applying Andrea's patch to Emacs 29, and it didn't lead to any
test failures that I can see (and the patch looks "obviously correct"),
so I've now pushed it to Emacs 29.

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

Previous Next

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