bug#35286: 26.2; indent-sexp broken

bug#35286: 26.2; indent-sexp broken

[Leo Liu]

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.

bug#35286: 26.2; indent-sexp broken

[Noam Postavsky]

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

tags 35286 + patch
quit

Leo Liu <sdl.web@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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 2447 bytes --]

From 7678458e5431e3b5a365343ec172cc75cc41ffb7 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@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

bug#35286: 26.2; indent-sexp broken

[Leo Liu]

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 }.

bug#35286: 26.2; indent-sexp broken

[Noam Postavsky]

Leo Liu <sdl.web@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??

bug#35286: 26.2; indent-sexp broken

[Leo Liu]

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.

bug#35286: 26.2; indent-sexp broken

[Noam Postavsky]

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

Leo Liu <sdl.web@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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 3495 bytes --]

From 0739d40a3c1aa5690442f84cc7bf5fa093f5b06e Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@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

bug#35286: 26.2; indent-sexp broken

[Leo Liu]

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.

bug#35286: 26.2; indent-sexp broken

[Leo Liu]

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.

bug#35286: 26.2; indent-sexp broken

[Leo Liu]

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.

bug#35286: 26.2; indent-sexp broken

[Noam Postavsky]

tags 35286 fixed
close 35286 26.3
quit

Leo Liu <sdl.web@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