GNU bug report logs - #35286
26.2; indent-sexp broken

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 35286 in the body.
You can then email your comments to 35286 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: Leo Liu <sdl.web <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 26.2; indent-sexp broken
Date: Mon, 15 Apr 2019 20:14:58 +0800

I am only using the following ruby code to demonstrate the issue. I
noticed this issue while editing other programming text.

  def sum_eq_n?(arr, n)
    return true if arr.empty? && n == 0
    arr.product(arr).reject { |a,b| a == b }.any? !{ |a,b| a + b == n } #
  
  def sum_eq_n?(arr, n)
    return true if arr.empty? && n == 0
    arr.product(arr).reject { |a,b| a == b }.any? { |a,b| a + b == n }
  end

Put the above ruby code in a ruby-mode buffer. ! is used to indicate
where point is and is not part of the code.

M-x indent-sexp and see indentation of every line after the point
changed.


The bug is caused by the following unsafe part of indent-sexp introduced
in 26.2:

  (save-excursion
    (let ((eol (line-end-position)))
      (forward-sexp 1)
      (condition-case ()
          (while (and (< (point) eol) (not (eobp)))
            (forward-sexp 1))
        (scan-error nil)))
    (point))

which can easily include two or more sexps after point. This looks like
a major breakage.

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

From: Noam Postavsky <npostavs <at> gmail.com>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 35286 <at> debbugs.gnu.org
Subject: Re: bug#35286: 26.2; indent-sexp broken
Date: Mon, 15 Apr 2019 19:57:55 -0400

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

tags 35286 + patch
quit

Leo Liu <sdl.web <at> gmail.com> writes:

> The bug is caused by the following unsafe part of indent-sexp introduced
> in 26.2:
>
>   (save-excursion
>     (let ((eol (line-end-position)))
>       (forward-sexp 1)
>       (condition-case ()
>           (while (and (< (point) eol) (not (eobp)))
>             (forward-sexp 1))
>         (scan-error nil)))
>     (point))
>
> which can easily include two or more sexps after point. This looks like
> a major breakage.

I put that code there because Emacs 25 and earlier also indents more
than one sexps after point in some cases.  The regression is that the
Emacs 26 code gets confused by comments at the end of line.  Here's a
patch for emacs-26 to fix this:

[0001-Fix-indent-sexp-confusion-over-eol-comments-Bug-3528.patch (text/x-diff, inline)]

From 7678458e5431e3b5a365343ec172cc75cc41ffb7 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs <at> gmail.com>
Date: Mon, 15 Apr 2019 18:49:57 -0400
Subject: [PATCH] Fix indent-sexp confusion over eol comments (Bug#35286)

* lisp/emacs-lisp/lisp-mode.el (indent-sexp): Skip over comments
before checking for end of line.
---
 lisp/emacs-lisp/lisp-mode.el            |  6 +++++-
 test/lisp/emacs-lisp/lisp-mode-tests.el | 12 ++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index 57f57175c5..a0caaf7475 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -1212,7 +1212,11 @@ (defun indent-sexp (&optional endpos)
                         ;; indent things like #s(...).  This might not
                         ;; be needed if Bug#15998 is fixed.
                         (condition-case ()
-                            (while (and (< (point) eol) (not (eobp)))
+                            (while (progn (while (and (forward-comment 1)
+                                                      (if (< (point) eol) t
+                                                        (goto-char eol)
+                                                        nil)))
+                                          (and (< (point) eol) (not (eobp))))
                               (forward-sexp 1))
                           ;; But don't signal an error for incomplete
                           ;; sexps following the first complete sexp
diff --git a/test/lisp/emacs-lisp/lisp-mode-tests.el b/test/lisp/emacs-lisp/lisp-mode-tests.el
index a6370742ab..3782bad315 100644
--- a/test/lisp/emacs-lisp/lisp-mode-tests.el
+++ b/test/lisp/emacs-lisp/lisp-mode-tests.el
@@ -136,6 +136,18 @@ (ert-deftest indent-sexp-cant-go ()
     (indent-sexp)
     (should (equal (buffer-string) "(())"))))
 
+(ert-deftest indent-sexp-stop-before-eol-comment ()
+  "`indent-sexp' shouldn't look for more sexps after an eol comment."
+  ;; See https://debbugs.gnu.org/cgi/bugreport.cgi?bug=35286.
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (let ((str "() ;;\n  x"))
+      (insert str)
+      (goto-char (point-min))
+      (indent-sexp)
+      ;; The "x" is in the next sexp, so it shouldn't get indented.
+      (should (equal (buffer-string) str)))))
+
 (ert-deftest lisp-indent-region ()
   "Test basics of `lisp-indent-region'."
   (with-temp-buffer
-- 
2.11.0

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

From: Leo Liu <sdl.web <at> gmail.com>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 35286 <at> debbugs.gnu.org
Subject: Re: bug#35286: 26.2; indent-sexp broken
Date: Tue, 16 Apr 2019 08:35:48 +0800

On 2019-04-15 19:57 -0400, Noam Postavsky wrote:
> I put that code there because Emacs 25 and earlier also indents more
> than one sexps after point in some cases.

I didn't know that. Are those cases obscure?

> The regression is that the Emacs 26 code gets confused by comments at
> the end of line. Here's a patch for emacs-26 to fix this:

Not just comments though it can be anything. For example, put the
following in prolog-mode. Move point to the first `{' char and then M-x
indent-sexp.

iso_time(Hr, Min, Sec) -->
    hour(H),
    timezone(DH, DM, DS),
    { Hr is H + DH, Min is DM, Sec is DS }.

timezone(Hr, Min, 0) -->
    "+", hour(H), ":", minute(M), { Hr is -1 * H, Min is -1 * M }.

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

From: Noam Postavsky <npostavs <at> gmail.com>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 35286 <at> debbugs.gnu.org
Subject: Re: bug#35286: 26.2; indent-sexp broken
Date: Mon, 15 Apr 2019 20:54:22 -0400

Leo Liu <sdl.web <at> gmail.com> writes:

> On 2019-04-15 19:57 -0400, Noam Postavsky wrote:
>> I put that code there because Emacs 25 and earlier also indents more
>> than one sexps after point in some cases.
>
> I didn't know that. Are those cases obscure?

The minimal example would be (with point before "foo", the "xx)"
part gets indented too):

foo (bar
 xx)

I'm not sure if anyone relies on that (I broke it in 26.1 and nobody
reported it).  The main motivating example is

#s(foo
bar)

which is due to forward-sexp not handling #s correctly for Emacs Lisp
(it's treated as a separate sexp rather than as a whole record
expression).

>> The regression is that the Emacs 26 code gets confused by comments at
>> the end of line. Here's a patch for emacs-26 to fix this:
>
> Not just comments though it can be anything. For example, put the
> following in prolog-mode. Move point to the first `{' char and then M-x
> indent-sexp.

Hmm, I'm a bit confused here.  I thought indent-sexp is only good for
Lisp-based languages.  Do lisp-indent-calc-next/calculate-lisp-indent
give usable results for prolog or ruby??

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

From: Leo Liu <sdl.web <at> gmail.com>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 35286 <at> debbugs.gnu.org
Subject: Re: bug#35286: 26.2; indent-sexp broken
Date: Tue, 16 Apr 2019 11:19:00 +0800

On 2019-04-15 20:54 -0400, Noam Postavsky wrote:
> The minimal example would be (with point before "foo", the "xx)"
> part gets indented too):
>
> foo (bar
>  xx)
>
> I'm not sure if anyone relies on that (I broke it in 26.1 and nobody
> reported it).  The main motivating example is
>
> #s(foo
> bar)
>
> which is due to forward-sexp not handling #s correctly for Emacs Lisp
> (it's treated as a separate sexp rather than as a whole record
> expression).

Thanks for the examples.

>> Not just comments though it can be anything. For example, put the
>> following in prolog-mode. Move point to the first `{' char and then M-x
>> indent-sexp.
>
> Hmm, I'm a bit confused here.  I thought indent-sexp is only good for
> Lisp-based languages.  Do lisp-indent-calc-next/calculate-lisp-indent
> give usable results for prolog or ruby??

Treating things between parentheses as sexp and using indent-sexp to
indent it is not lisp-specific (imagine if we couldn't use forward-sexp
on parentheses in non-lisp languages). I think I have been doing it for
over a decade now. Sometimes indirectly. For example, I use paredit to
input parentheses everywhere and paredit uses indent-sexp to indent the
inserted pair. It's been working nicely forever until the change in 26.2
breaks it.

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

From: Noam Postavsky <npostavs <at> gmail.com>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 35286 <at> debbugs.gnu.org
Subject: Re: bug#35286: 26.2; indent-sexp broken
Date: Tue, 16 Apr 2019 08:16:31 -0400

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

Leo Liu <sdl.web <at> gmail.com> writes:

> Treating things between parentheses as sexp [...] is not lisp-specific

This part makes sense to me.

> using indent-sexp to indent it is not lisp-specific

This is what surprises me.  How does
lisp-indent-calc-next/calculate-lisp-indent give correct results for
non-lisp?  I think using prog-indent-sexp for non-lisp would make more
sense.

Anyway, the following patch seems to work with your examples.

[0001-Be-more-careful-about-indent-sexp-going-over-eol-Bug.patch (text/x-diff, inline)]

From 0739d40a3c1aa5690442f84cc7bf5fa093f5b06e Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs <at> gmail.com>
Date: Mon, 15 Apr 2019 18:49:57 -0400
Subject: [PATCH] Be more careful about indent-sexp going over eol (Bug#35286)

* lisp/emacs-lisp/lisp-mode.el (indent-sexp): Only go over multiple
sexps if the end of line is within a sexp.
---
 lisp/emacs-lisp/lisp-mode.el            | 22 ++++++++++++++--------
 test/lisp/emacs-lisp/lisp-mode-tests.el | 12 ++++++++++++
 2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/lisp/emacs-lisp/lisp-mode.el b/lisp/emacs-lisp/lisp-mode.el
index 57f57175c5..74bf0c87c5 100644
--- a/lisp/emacs-lisp/lisp-mode.el
+++ b/lisp/emacs-lisp/lisp-mode.el
@@ -1205,19 +1205,25 @@ (defun indent-sexp (&optional endpos)
                     ;; Get error now if we don't have a complete sexp
                     ;; after point.
                     (save-excursion
+                      (forward-sexp 1)
                       (let ((eol (line-end-position)))
-                        (forward-sexp 1)
                         ;; We actually look for a sexp which ends
                         ;; after the current line so that we properly
                         ;; indent things like #s(...).  This might not
                         ;; be needed if Bug#15998 is fixed.
-                        (condition-case ()
-                            (while (and (< (point) eol) (not (eobp)))
-                              (forward-sexp 1))
-                          ;; But don't signal an error for incomplete
-                          ;; sexps following the first complete sexp
-                          ;; after point.
-                          (scan-error nil)))
+                        (when (and (< (point) eol)
+                                   ;; Check if eol is within a sexp.
+                                   (> (nth 0 (save-excursion
+                                               (parse-partial-sexp
+                                                (point) eol)))
+                                      0))
+                          (condition-case ()
+                              (while (< (point) eol)
+                                (forward-sexp 1))
+                            ;; But don't signal an error for incomplete
+                            ;; sexps following the first complete sexp
+                            ;; after point.
+                            (scan-error nil))))
                       (point)))))
     (save-excursion
       (while (let ((indent (lisp-indent-calc-next parse-state))
diff --git a/test/lisp/emacs-lisp/lisp-mode-tests.el b/test/lisp/emacs-lisp/lisp-mode-tests.el
index a6370742ab..3782bad315 100644
--- a/test/lisp/emacs-lisp/lisp-mode-tests.el
+++ b/test/lisp/emacs-lisp/lisp-mode-tests.el
@@ -136,6 +136,18 @@ (ert-deftest indent-sexp-cant-go ()
     (indent-sexp)
     (should (equal (buffer-string) "(())"))))
 
+(ert-deftest indent-sexp-stop-before-eol-comment ()
+  "`indent-sexp' shouldn't look for more sexps after an eol comment."
+  ;; See https://debbugs.gnu.org/cgi/bugreport.cgi?bug=35286.
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (let ((str "() ;;\n  x"))
+      (insert str)
+      (goto-char (point-min))
+      (indent-sexp)
+      ;; The "x" is in the next sexp, so it shouldn't get indented.
+      (should (equal (buffer-string) str)))))
+
 (ert-deftest lisp-indent-region ()
   "Test basics of `lisp-indent-region'."
   (with-temp-buffer
-- 
2.11.0

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

From: Leo Liu <sdl.web <at> gmail.com>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 35286 <at> debbugs.gnu.org
Subject: Re: bug#35286: 26.2; indent-sexp broken
Date: Tue, 16 Apr 2019 20:57:03 +0800

On 2019-04-16 08:16 -0400, Noam Postavsky wrote:
> This is what surprises me.  How does
> lisp-indent-calc-next/calculate-lisp-indent give correct results for
> non-lisp?  I think using prog-indent-sexp for non-lisp would make more
> sense.

I think for indenting parentheses most modes behave similarly. paredit
is mostly used with lispy-modes so it has reasons to use indent-sexp.

> Anyway, the following patch seems to work with your examples.

Thanks, it seems to work well. I am testing the patched indent-sexp in
my emacs now and will report back if something breaks.

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

From: Leo Liu <sdl.web <at> gmail.com>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 35286 <at> debbugs.gnu.org
Subject: Re: bug#35286: 26.2; indent-sexp broken
Date: Tue, 16 Apr 2019 21:13:39 +0800

On 2019-04-16 20:57 +0800, Leo Liu wrote:
>> This is what surprises me. How does
>> lisp-indent-calc-next/calculate-lisp-indent give correct results for
>> non-lisp?

To correct my last comment.

You are right it gives incorrect indentations if the text spans multiple
lines. In most cases I am selecting text on the same line to put pair
delimiters around it. I also have indent-defun at my finger tip. It
seems I have become oblivious to it.

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

From: Leo Liu <sdl.web <at> gmail.com>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 35286 <at> debbugs.gnu.org
Subject: Re: bug#35286: 26.2; indent-sexp broken
Date: Sun, 21 Apr 2019 16:41:46 +0800

On 2019-04-16 08:16 -0400, Noam Postavsky wrote:
> Anyway, the following patch seems to work with your examples.

I have been running the patched index-sexp in 26.2 and haven't noticed
any breakage.

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

From: Noam Postavsky <npostavs <at> gmail.com>
To: Leo Liu <sdl.web <at> gmail.com>
Cc: 35286 <at> debbugs.gnu.org
Subject: Re: bug#35286: 26.2; indent-sexp broken
Date: Mon, 22 Apr 2019 12:51:53 -0400

tags 35286 fixed
close 35286 26.3
quit

Leo Liu <sdl.web <at> gmail.com> writes:

> On 2019-04-16 08:16 -0400, Noam Postavsky wrote:
>> Anyway, the following patch seems to work with your examples.
>
> I have been running the patched index-sexp in 26.2 and haven't noticed
> any breakage.

Okay, added a test with a reduced version of your prolog example, and
pushed to emacs-26.

93912baefd 2019-04-22T12:49:36-04:00 "Be more careful about indent-sexp going over eol (Bug#35286)"
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=93912baefd10ccb3e6e2e9696cda3b813c056c87

Previous Next

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