bug#18764: 24.4; electric-indent in *scratch* signals an error

bug#18764: 24.4; electric-indent in *scratch* signals an error

[Eli Zaretskii]

In "emacs -Q":

1) Insert the following long sequence of left parens followed by a somewhat
shorter sequence of right parens:

((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((()))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))))

2) Put cursor after the last right parenthesis, and type RET

3) Emacs signals an error:

   lisp-indent-line: Wrong type argument: number-or-marker-p, nil

Here's the Lisp backtrace:

  Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
    calculate-lisp-indent()
    lisp-indent-line()
    indent-according-to-mode()
    electric-indent-post-self-insert-function()
    self-insert-command(1)
    newline(nil 1)
    funcall-interactively(newline nil 1)
    call-interactively(newline nil nil)
    command-execute(newline)




In GNU Emacs 24.4.1 (i686-pc-mingw32)
 of 2014-10-17 on HOME-C4E4A596F7
Windowing system distributor `Microsoft Corp.', version 5.1.2600
Configured using:
 `configure --prefix=/d/usr'

Important settings:
  value of $LANG: ENU
  locale-coding-system: cp1255

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( 
( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( 
( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( 
( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( ( <return> 
C-/ ) <return> C-/ ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) 
) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) 
) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) <return> 
C-/ ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) 
<return> C-/ ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) ) <return> 
<backspace> <backspace> <backspace> <backspace> <return> 
<help-echo> <up> <up> <up> <C-end> <return> ( ( ( ( 
( ( ( ( ( ( ( ( ( ) ) ) ) ) ) ) ) ) ) ) <return> C-/ 
) <return> C-/ ) <return> C-/ ) <return> <up> <up> 
<C-end> <return> <return> C-y <return> C-/ M-x r e 
p o r t - e m <tab> <return>

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
Undo! [4 times]
Mark set
Undo! [3 times]
Mark set [2 times]
lisp-indent-line: Wrong type argument: number-or-marker-p, nil
Undo!

Load-path shadows:
None found.

Features:
(shadow sort gnus-util mail-extr emacsbug message format-spec rfc822 mml
easymenu mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util help-fns mail-prsvr mail-utils time-date tooltip electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel dos-w32 ls-lisp
w32-common-fns disp-table w32-win w32-vars tool-bar dnd fontset image
regexp-opt fringe tabulated-list newcomment lisp-mode prog-mode register
page menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock
font-lock syntax facemenu font-core frame cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese hebrew
greek romanian slovak czech european ethiopic indian cyrillic chinese
case-table epa-hook jka-cmpr-hook help simple abbrev minibuffer nadvice
loaddefs button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process w32notify w32
multi-tty emacs)

Memory information:
((conses 8 74585 7049)
 (symbols 32 17521 0)
 (miscs 32 45 339)
 (strings 16 10814 3613)
 (string-bytes 1 269937)
 (vectors 8 9552)
 (vector-slots 4 385033 4696)
 (floats 8 58 351)
 (intervals 28 279 94)
 (buffers 508 11))

bug#18764: 24.4; electric-indent in *scratch* signals an error

[Noam Postavsky]

tags 18764 + confirmed
found 18764 26.1
quit

Eli Zaretskii <eliz@gnu.org> writes:

> In "emacs -Q":
>
> 1) Insert the following long sequence of left parens followed by a somewhat
> shorter sequence of right parens:

Specifically, the sequence of left parens must be more than 100, to hit
this limit:

static void
scan_sexps_forward(...)
{...
  struct level levelstart[100];
  struct level *curlevel = levelstart;
  struct level *endlevel = levelstart + 100;
  ...
        case Sopen:
          ...
          if (++curlevel == endlevel)
            curlevel--; /* error ("Nesting too deep for parser"); */

and a sequence of 100 close parens so that the curlevel stack will be
completely popped:

        case Sclose:
          ...
          if (curlevel != levelstart)
            curlevel--;

In this case parse-partial-sexp will have the right depth, but
incorrectly have nil for element 2 "character address of start of last
complete sexp terminated."

Not sure exactly what to do about this though.  Maybe parse-partial-sexp
should (optionally?) signal an error when the curlevel stack overflows?

bug#18764: 24.4; electric-indent in *scratch* signals an error

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: 18764@debbugs.gnu.org
> Date: Thu, 14 Jun 2018 22:34:35 -0400
> 
> Not sure exactly what to do about this though.  Maybe parse-partial-sexp
> should (optionally?) signal an error when the curlevel stack overflows?

Why do we produce an error instead of just inserting the character?

bug#18764: 24.4; electric-indent in *scratch* signals an error

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Noam Postavsky <npostavs@gmail.com>
>> Cc: 18764@debbugs.gnu.org
>> Date: Thu, 14 Jun 2018 22:34:35 -0400
>> 
>> Not sure exactly what to do about this though.  Maybe parse-partial-sexp
>> should (optionally?) signal an error when the curlevel stack overflows?
>
> Why do we produce an error instead of just inserting the character?

I don't quite follow; the newline character has already been inserted as
far as I can tell.  Then we signal an error while trying to indent.

bug#18764: 24.4; electric-indent in *scratch* signals an error

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: 18764@debbugs.gnu.org
> Date: Sat, 16 Jun 2018 09:48:18 -0400
> 
> > Why do we produce an error instead of just inserting the character?
> 
> I don't quite follow; the newline character has already been inserted as
> far as I can tell.  Then we signal an error while trying to indent.

Then let me rephrase: why are we signaling an error instead of
silently doing nothing?  IOW, if something cannot be indented, let's
leave it unindented.  Does that make sense?

bug#18764: 24.4; electric-indent in *scratch* signals an error

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

> Then let me rephrase: why are we signaling an error instead of
> silently doing nothing?  IOW, if something cannot be indented, let's
> leave it unindented.  Does that make sense?

So just suppress the errors?  Worries me a bit that some future problems
could be harder to find because of that.

--- i/lisp/electric.el
+++ w/lisp/electric.el
@@ -262,6 +262,7 @@ electric-indent-post-self-insert-function
                     (unless (eq act 'do-indent) (nth 8 (syntax-ppss))))))))
       ;; For newline, we want to reindent both lines and basically behave like
       ;; reindent-then-newline-and-indent (whose code we hence copied).
+      (ignore-errors
         (let ((at-newline (<= pos (line-beginning-position))))
           (when at-newline
             (let ((before (copy-marker (1- pos) t)))
@@ -285,7 +286,7 @@ electric-indent-post-self-insert-function
                   (delete-horizontal-space t)))))
           (unless (and electric-indent-inhibit
                        (not at-newline))
-          (indent-according-to-mode))))))
+            (indent-according-to-mode)))))))
 

bug#18764: 24.4; electric-indent in *scratch* signals an error

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: 18764@debbugs.gnu.org
> Date: Sat, 16 Jun 2018 19:13:29 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Then let me rephrase: why are we signaling an error instead of
> > silently doing nothing?  IOW, if something cannot be indented, let's
> > leave it unindented.  Does that make sense?
> 
> So just suppress the errors?

Yes.

> Worries me a bit that some future problems could be harder to find
> because of that.

I submit that any errors in code that tries to indent should be
suppressed, as they are not relevant to what the user wanted.  If
those errors are important in other contexts, they will pop up there,
and can then decide what to do with them.

Thanks.

bug#18764: 24.4; electric-indent in *scratch* signals an error

[Dmitry Gutov]

On 6/17/18 6:48 AM, Eli Zaretskii wrote:

> I submit that any errors in code that tries to indent should be
> suppressed, as they are not relevant to what the user wanted.  If
> those errors are important in other contexts, they will pop up there,
> and can then decide what to do with them.

That's going to make user complaints like "why it doesn't indent" or "it 
doesn't indent right" harder to debug and address.

bug#18764: 24.4; electric-indent in *scratch* signals an error

[Eli Zaretskii]

> Cc: 18764@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 18 Jun 2018 12:33:34 +0300
> 
> On 6/17/18 6:48 AM, Eli Zaretskii wrote:
> 
> > I submit that any errors in code that tries to indent should be
> > suppressed, as they are not relevant to what the user wanted.  If
> > those errors are important in other contexts, they will pop up there,
> > and can then decide what to do with them.
> 
> That's going to make user complaints like "why it doesn't indent" or "it 
> doesn't indent right" harder to debug and address.

IMO, this is a lesser evil.  And if it turns out to be a real problem,
we could always add a variable to control whether such errors are
silently ignored.

bug#18764: 24.4; electric-indent in *scratch* signals an error

[Noam Postavsky]

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

tags 18764 + patch
quit

Eli Zaretskii <eliz@gnu.org> writes:

>> That's going to make user complaints like "why it doesn't indent" or "it 
>> doesn't indent right" harder to debug and address.
>
> IMO, this is a lesser evil.  And if it turns out to be a real problem,
> we could always add a variable to control whether such errors are
> silently ignored.

We can be a little more discriminating than my initial blanket
`ignore-errors', see commit message in the attached patch:


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 4509 bytes --]

From 4aa45c829f8be801f626d169ac412185e5c9c509 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 20 Jun 2018 20:12:23 -0400
Subject: [PATCH v2] Suppress indent errors during electric indentation
 (Bug#18764)

* lisp/electric.el (electric-indent-post-self-insert-function):
Suppress errors from indent code, but don't suppress errors from
elsewhere in this function.  That way, if trouble is encountered with
electric indent "not working", the error should be reproducible by
calling indent directly (as is the case for Bug#18764), or else it's
from the electric indent code and will be reported normally.
---
 lisp/electric.el | 59 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 25 deletions(-)

diff --git a/lisp/electric.el b/lisp/electric.el
index c00e7c00a5..7a998c0ff3 100644
--- a/lisp/electric.el
+++ b/lisp/electric.el
@@ -260,32 +260,41 @@ electric-indent-post-self-insert-function
                 (or (memq act '(nil no-indent))
                     ;; In a string or comment.
                     (unless (eq act 'do-indent) (nth 8 (syntax-ppss))))))))
-      ;; For newline, we want to reindent both lines and basically behave like
-      ;; reindent-then-newline-and-indent (whose code we hence copied).
-      (let ((at-newline (<= pos (line-beginning-position))))
-        (when at-newline
-          (let ((before (copy-marker (1- pos) t)))
-            (save-excursion
-              (unless (or (memq indent-line-function
-                                electric-indent-functions-without-reindent)
-                          electric-indent-inhibit)
-                ;; Don't reindent the previous line if the indentation function
-                ;; is not a real one.
+      ;; If we error during indent, give up, but don't suppress errors
+      ;; from elsewhere in *this* function (Bug#18764).
+      (catch 'indent-error
+        ;; For newline, we want to reindent both lines and basically
+        ;; behave like reindent-then-newline-and-indent (whose code we
+        ;; hence copied).
+        (let ((at-newline (<= pos (line-beginning-position))))
+          (when at-newline
+            (let ((before (copy-marker (1- pos) t)))
+              (save-excursion
+                (unless (or (memq indent-line-function
+                                  electric-indent-functions-without-reindent)
+                            electric-indent-inhibit)
+                  ;; Don't reindent the previous line if the
+                  ;; indentation function is not a real one.
+                  (goto-char before)
+                  (condition-case ()
+                      (indent-according-to-mode)
+                    ((debug error) (throw 'indent-error nil))))
+                ;; We are at EOL before the call to
+                ;; `indent-according-to-mode', and after it we usually
+                ;; are as well, but not always.  We tried to address
+                ;; it with `save-excursion' but that uses a normal
+                ;; marker whereas we need `move after insertion', so
+                ;; we do the save/restore by hand.
                 (goto-char before)
-                (indent-according-to-mode))
-              ;; We are at EOL before the call to indent-according-to-mode, and
-              ;; after it we usually are as well, but not always.  We tried to
-              ;; address it with `save-excursion' but that uses a normal marker
-              ;; whereas we need `move after insertion', so we do the
-              ;; save/restore by hand.
-              (goto-char before)
-              (when (eolp)
-                ;; Remove the trailing whitespace after indentation because
-                ;; indentation may (re)introduce the whitespace.
-                (delete-horizontal-space t)))))
-        (unless (and electric-indent-inhibit
-                     (not at-newline))
-          (indent-according-to-mode))))))
+                (when (eolp)
+                  ;; Remove the trailing whitespace after indentation because
+                  ;; indentation may (re)introduce the whitespace.
+                  (delete-horizontal-space t)))))
+          (unless (and electric-indent-inhibit
+                       (not at-newline))
+            (condition-case ()
+                (indent-according-to-mode)
+              ((debug error) (throw 'indent-error nil)))))))))
 
 (put 'electric-indent-post-self-insert-function 'priority  60)
 
-- 
2.11.0

bug#18764: 24.4; electric-indent in *scratch* signals an error

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: Dmitry Gutov <dgutov@yandex.ru>,  18764@debbugs.gnu.org
> Date: Wed, 20 Jun 2018 20:19:16 -0400
> 
> >> That's going to make user complaints like "why it doesn't indent" or "it 
> >> doesn't indent right" harder to debug and address.
> >
> > IMO, this is a lesser evil.  And if it turns out to be a real problem,
> > we could always add a variable to control whether such errors are
> > silently ignored.
> 
> We can be a little more discriminating than my initial blanket
> `ignore-errors', see commit message in the attached patch:

Fine with me, but I'd prefer a comment in the code explaining why we
catch errors here.

Thanks.

bug#18764: 24.4; electric-indent in *scratch* signals an error

[Noam Postavsky]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> We can be a little more discriminating than my initial blanket
>> `ignore-errors', see commit message in the attached patch:
>
> Fine with me, but I'd prefer a comment in the code explaining why we
> catch errors here.

I added some more explanation in the comment over the `catch', and
remembered to use condition-case-unless-debug.


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 4673 bytes --]

From 5daa803e6e00445570f66116f7ec4681cfdccb92 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 20 Jun 2018 20:12:23 -0400
Subject: [PATCH v3] Suppress indent errors during electric indentation
 (Bug#18764)

* lisp/electric.el (electric-indent-post-self-insert-function):
Suppress errors from indent code, but don't suppress errors from
elsewhere in this function.  That way, if trouble is encountered with
electric indent "not working", the error should be reproducible by
calling indent directly (as is the case for Bug#18764), or else it's
from the electric indent code and will be reported normally.
---
 lisp/electric.el | 61 +++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 25 deletions(-)

diff --git a/lisp/electric.el b/lisp/electric.el
index c00e7c00a5..a45faf2dbb 100644
--- a/lisp/electric.el
+++ b/lisp/electric.el
@@ -260,32 +260,43 @@ electric-indent-post-self-insert-function
                 (or (memq act '(nil no-indent))
                     ;; In a string or comment.
                     (unless (eq act 'do-indent) (nth 8 (syntax-ppss))))))))
-      ;; For newline, we want to reindent both lines and basically behave like
-      ;; reindent-then-newline-and-indent (whose code we hence copied).
-      (let ((at-newline (<= pos (line-beginning-position))))
-        (when at-newline
-          (let ((before (copy-marker (1- pos) t)))
-            (save-excursion
-              (unless (or (memq indent-line-function
-                                electric-indent-functions-without-reindent)
-                          electric-indent-inhibit)
-                ;; Don't reindent the previous line if the indentation function
-                ;; is not a real one.
+      ;; If we error during indent, silently give up since this is an
+      ;; automatic action that the user didn't explicitly request.
+      ;; But we don't want to suppress errors from elsewhere in *this*
+      ;; function, hence the `condition-case' and `throw' (Bug#18764).
+      (catch 'indent-error
+        ;; For newline, we want to reindent both lines and basically
+        ;; behave like reindent-then-newline-and-indent (whose code we
+        ;; hence copied).
+        (let ((at-newline (<= pos (line-beginning-position))))
+          (when at-newline
+            (let ((before (copy-marker (1- pos) t)))
+              (save-excursion
+                (unless (or (memq indent-line-function
+                                  electric-indent-functions-without-reindent)
+                            electric-indent-inhibit)
+                  ;; Don't reindent the previous line if the
+                  ;; indentation function is not a real one.
+                  (goto-char before)
+                  (condition-case-unless-debug ()
+                      (indent-according-to-mode)
+                    (error (throw 'indent-error nil))))
+                ;; We are at EOL before the call to
+                ;; `indent-according-to-mode', and after it we usually
+                ;; are as well, but not always.  We tried to address
+                ;; it with `save-excursion' but that uses a normal
+                ;; marker whereas we need `move after insertion', so
+                ;; we do the save/restore by hand.
                 (goto-char before)
-                (indent-according-to-mode))
-              ;; We are at EOL before the call to indent-according-to-mode, and
-              ;; after it we usually are as well, but not always.  We tried to
-              ;; address it with `save-excursion' but that uses a normal marker
-              ;; whereas we need `move after insertion', so we do the
-              ;; save/restore by hand.
-              (goto-char before)
-              (when (eolp)
-                ;; Remove the trailing whitespace after indentation because
-                ;; indentation may (re)introduce the whitespace.
-                (delete-horizontal-space t)))))
-        (unless (and electric-indent-inhibit
-                     (not at-newline))
-          (indent-according-to-mode))))))
+                (when (eolp)
+                  ;; Remove the trailing whitespace after indentation because
+                  ;; indentation may (re)introduce the whitespace.
+                  (delete-horizontal-space t)))))
+          (unless (and electric-indent-inhibit
+                       (not at-newline))
+            (condition-case-unless-debug ()
+                (indent-according-to-mode)
+              (error (throw 'indent-error nil)))))))))
 
 (put 'electric-indent-post-self-insert-function 'priority  60)
 
-- 
2.11.0

bug#18764: 24.4; electric-indent in *scratch* signals an error

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: 18764@debbugs.gnu.org,  dgutov@yandex.ru
> Date: Sat, 23 Jun 2018 21:40:25 -0400
> 
> > Fine with me, but I'd prefer a comment in the code explaining why we
> > catch errors here.
> 
> I added some more explanation in the comment over the `catch', and
> remembered to use condition-case-unless-debug.

LGTM, thanks.

bug#18764: 24.4; electric-indent in *scratch* signals an error

[Noam Postavsky]

tags 18764 fixed
close 18764 27.1
quit

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Noam Postavsky <npostavs@gmail.com>
>> Cc: 18764@debbugs.gnu.org,  dgutov@yandex.ru
>> Date: Sat, 23 Jun 2018 21:40:25 -0400
>> 
>> > Fine with me, but I'd prefer a comment in the code explaining why we
>> > catch errors here.
>> 
>> I added some more explanation in the comment over the `catch', and
>> remembered to use condition-case-unless-debug.
>
> LGTM, thanks.

Pushed to master.

[1: c71fb6b0cd]: 2018-06-25 19:18:55 -0400
  Suppress indent errors during electric indentation (Bug#18764)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=c71fb6b0cdb7043e2828a6843496ab20f4577cbb