bug#19342: auto-fill scan-error in sh-mode

bug#19342: auto-fill scan-error in sh-mode

[Glenn Morris]

Package: emacs
Version: 24.4

emacs -Q /tmp/foo.sh
M-x auto-fill-mode

Type a long string with spaces, past fill-column; eg:

foo="aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaaaaa aaaaaaaaaa

Auto-fill results in:

Error: (scan-error "Containing expression ends prematurely" 75 75)

bug#19342: auto-fill scan-error in sh-mode

[Charles A. Roelli]

> From: Glenn Morris <rgm@gnu.org>
> Date: Wed, 10 Dec 2014 16:51:22 -0500
> 
> Package: emacs
> Version: 24.4
> 
> emacs -Q /tmp/foo.sh
> M-x auto-fill-mode
> 
> Type a long string with spaces, past fill-column; eg:
> 
> foo="aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaaaaa aaaaaaaaaa
> 
> Auto-fill results in:
> 
> Error: (scan-error "Containing expression ends prematurely" 75 75)

Can't produce this in the latest master, it seems to work fine now.

bug#19342: auto-fill scan-error in sh-mode

[Charles A. Roelli]

> Date: Sun, 09 Dec 2018 11:35:32 +0100
> Date: Sun, 09 Dec 2018 11:35:32 +0100
> From: charles@aurox.ch (Charles A. Roelli)
> CC: 19342@debbugs.gnu.org
> Reply-to: charles@aurox.ch
> 
> > From: Glenn Morris <rgm@gnu.org>
> > Date: Wed, 10 Dec 2014 16:51:22 -0500
> > 
> > Package: emacs
> > Version: 24.4
> > 
> > emacs -Q /tmp/foo.sh
> > M-x auto-fill-mode
> > 
> > Type a long string with spaces, past fill-column; eg:
> > 
> > foo="aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaaaaa aaaaaaaaaa
> > 
> > Auto-fill results in:
> > 
> > Error: (scan-error "Containing expression ends prematurely" 75 75)
> 
> Can't produce this in the latest master, it seems to work fine now.

Nevermind that, it still does.  I missed a step.

bug#19342: auto-fill scan-error in sh-mode

[Stefan Kangas]

found 19342 28.0.50
tags 19342 + confirmed
thanks

Glenn Morris <rgm@gnu.org> writes:

> Package: emacs
> Version: 24.4
>
> emacs -Q /tmp/foo.sh
> M-x auto-fill-mode
>
> Type a long string with spaces, past fill-column; eg:
>
> foo="aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaaaaa aaaaaaaaaa
>
> Auto-fill results in:
>
> Error: (scan-error "Containing expression ends prematurely" 75 75)

I can reproduce this on current master.

Best regards,
Stefan Kangas

bug#19342: auto-fill scan-error in sh-mode

[Lars Ingebrigtsen]

Glenn Morris <rgm@gnu.org> writes:

> Type a long string with spaces, past fill-column; eg:
>
> foo="aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaaaaa aaaaaaaaaa
>
> Auto-fill results in:
>
> Error: (scan-error "Containing expression ends prematurely" 75 75)

This problem is still present in Emacs 28.  This is the backtrace:

Debugger entered--Lisp error: (scan-error "Containing expression ends prematurely" 95 95)
  signal(scan-error ("Containing expression ends prematurely" 95 95))
  (if (and (car res) (= pos (point)) (not (if forw (eobp) (bobp)))) (
  (let ((pos (point)) (res (if forw (smie-forward-sexp 'halfsexp) (sm
  (while (/= n 0) (setq n (- n (if forw 1 -1))) (let ((pos (point)) (
  (let ((forw (> n 0)) (forward-sexp-function nil)) (while (/= n 0) (
  smie-forward-sexp-command(1)
  forward-sexp(1)
  (cond ((< 0 (length tok)) (assoc tok smie-grammar)) ((looking-at "\
  (let ((tok (funcall smie-forward-token-function))) (cond ((< 0 (len
  smie-indent-forward-token()
  smie-indent-keyword()
  smie--funcall(smie-indent-keyword)
  run-hook-wrapped(smie--funcall smie-indent-keyword)
  smie-indent-calculate()
  smie-auto-fill(#f(compiled-function (&rest args) #<bytecode -0x1d0b7391ba5340b6>))

The code that errors out is:

(defun smie-indent-forward-token ()
[...]
     ((looking-at "\\s\"\\|\\s|")
      (forward-sexp 1)

That is, if we're auto-filling an unterminated string, it'll always bug
out, apparently?

I've added Stefan to the CCs.

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

bug#19342: auto-fill scan-error in sh-mode

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen [2021-08-19 15:29:10] wrote:

> Glenn Morris <rgm@gnu.org> writes:
>
>> Type a long string with spaces, past fill-column; eg:
>>
>> foo="aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaaaaa aaaaaaaaaa
>>
>> Auto-fill results in:
>>
>> Error: (scan-error "Containing expression ends prematurely" 75 75)
>
> This problem is still present in Emacs 28.  This is the backtrace:
>
> Debugger entered--Lisp error: (scan-error "Containing expression ends prematurely" 95 95)
>   signal(scan-error ("Containing expression ends prematurely" 95 95))
>   (if (and (car res) (= pos (point)) (not (if forw (eobp) (bobp)))) (
>   (let ((pos (point)) (res (if forw (smie-forward-sexp 'halfsexp) (sm
>   (while (/= n 0) (setq n (- n (if forw 1 -1))) (let ((pos (point)) (
>   (let ((forw (> n 0)) (forward-sexp-function nil)) (while (/= n 0) (
>   smie-forward-sexp-command(1)
>   forward-sexp(1)
>   (cond ((< 0 (length tok)) (assoc tok smie-grammar)) ((looking-at "\
>   (let ((tok (funcall smie-forward-token-function))) (cond ((< 0 (len
>   smie-indent-forward-token()
>   smie-indent-keyword()
>   smie--funcall(smie-indent-keyword)
>   run-hook-wrapped(smie--funcall smie-indent-keyword)
>   smie-indent-calculate()
>   smie-auto-fill(#f(compiled-function (&rest args) #<bytecode -0x1d0b7391ba5340b6>))
>
> The code that errors out is:
>
> (defun smie-indent-forward-token ()
> [...]
>      ((looking-at "\\s\"\\|\\s|")
>       (forward-sexp 1)
>
> That is, if we're auto-filling an unterminated string, it'll always bug
> out, apparently?

The behavior I see in the example above is that a newline
is inserted right before the last "aaaaaaaaaa", and an error message is
displayed in the echo area.

I'm not completely sure what behavior we'd like to see here instead.
Just a less scary error message?

We could also emit no error message at all, but since this is using
smie-auto-fill (i.e. syntax-aware auto-fill), it seems important to
point out that we couldn't use syntax-aware auto-filling because of
a problem in the syntax, which is what the error message is trying
to say.


        Stefan

bug#19342: auto-fill scan-error in sh-mode

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> The behavior I see in the example above is that a newline
> is inserted right before the last "aaaaaaaaaa", and an error message is
> displayed in the echo area.
>
> I'm not completely sure what behavior we'd like to see here instead.
> Just a less scary error message?

Perhaps no filling at all?  Certainly not any sort of error message.

> We could also emit no error message at all, but since this is using
> smie-auto-fill (i.e. syntax-aware auto-fill), it seems important to
> point out that we couldn't use syntax-aware auto-filling because of
> a problem in the syntax, which is what the error message is trying
> to say.

Well, there's no problem in the syntax, is there?  The user is just
typing a long string, which is pretty normal behaviour...

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

bug#19342: auto-fill scan-error in sh-mode

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Well, there's no problem in the syntax, is there?  The user is just
> typing a long string, which is pretty normal behaviour...

There's an error in the syntax because the string is not closed.


        Stefan

bug#19342: auto-fill scan-error in sh-mode

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Well, there's no problem in the syntax, is there?  The user is just
>> typing a long string, which is pretty normal behaviour...
>
> There's an error in the syntax because the string is not closed.

It rather sounds like you're saying it would be fine if Emacs started
signalling errors immediately after a user has typed a " character --
which I don't think you are.  So I'm not sure why you're fine with Emacs
issuing an error after the user has typed a " character and then some
more characters.

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

bug#19342: auto-fill scan-error in sh-mode

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen [2021-08-30 02:44:59] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> Well, there's no problem in the syntax, is there?  The user is just
>>> typing a long string, which is pretty normal behaviour...
>>
>> There's an error in the syntax because the string is not closed.
>
> It rather sounds like you're saying it would be fine if Emacs started
> signalling errors immediately after a user has typed a " character --
> which I don't think you are.  So I'm not sure why you're fine with Emacs
> issuing an error after the user has typed a " character and then some
> more characters.

It's not issuing an error.  It's emitting a message, the purpose
of which is to warn the user that the behavior was affected by
that problem.


        Stefan

bug#19342: auto-fill scan-error in sh-mode

[Glenn Morris]

It works correctly in Emacs 24.3; ie before the use of smie.

bug#19342: auto-fill scan-error in sh-mode

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Glenn Morris [2021-08-30 11:03:07] wrote:
> It works correctly in Emacs 24.3; ie before the use of smie.

AFAICT in this specific case it works just the same as in 24.3.
It just emits an additional message.


        Stefan

bug#19342: auto-fill scan-error in sh-mode

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> It's not issuing an error.  It's emitting a message, the purpose
> of which is to warn the user that the behavior was affected by
> that problem.

I get your point, and as you say, it works fine -- it auto-fills as it's
supposed to.

But it's literally signalling an error.  (And this is one of those rare
instances where "literally" isn't a general intensifier.)  Changing it
to a message would solve the issue, I think?

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

bug#19342: auto-fill scan-error in sh-mode

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen [2021-08-31 03:24:35] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> It's not issuing an error.  It's emitting a message, the purpose
>> of which is to warn the user that the behavior was affected by
>> that problem.
> I get your point, and as you say, it works fine -- it auto-fills as it's
> supposed to.
> But it's literally signalling an error.  (And this is one of those rare
> instances where "literally" isn't a general intensifier.)  Changing it
> to a message would solve the issue, I think?

Should I just silence the message altogether?
After all, I don't really what message to give if not the error itself,
because it can be potentially any kind of error.


        Stefan

bug#19342: auto-fill scan-error in sh-mode

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Should I just silence the message altogether?
> After all, I don't really what message to give if not the error itself,
> because it can be potentially any kind of error.

I think silencing the message might be the right solution here (unless
that leads to bad behaviour in other cases).

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

bug#19342: auto-fill scan-error in sh-mode

[Lars Ingebrigtsen]

I took a stab at fixing this with the following patch.  All tests pass
after applying it, but I'm unsure how much test coverage there is in
this area...

Any comments?

diff --git a/lisp/emacs-lisp/smie.el b/lisp/emacs-lisp/smie.el
index 2bab131913..3b9f983cdb 100644
--- a/lisp/emacs-lisp/smie.el
+++ b/lisp/emacs-lisp/smie.el
@@ -1850,20 +1850,26 @@ smie-auto-fill
                    (save-excursion
                      (let ((end (point))
                            (bsf nil)    ;Best-so-far.
-                           (gain 0))
+                           (gain 0)
+                           newcol)
                        (beginning-of-line)
                        (while (progn
                                 (smie-indent-forward-token)
-                                (and (<= (point) end)
-                                     (<= (current-column) fc)))
-                         ;; FIXME?  `smie-indent-calculate' can (and often
-                         ;; does) return a result that actually depends on the
-                         ;; presence/absence of a newline, so the gain computed
-                         ;; here may not be accurate, but in practice it seems
-                         ;; to work well enough.
-                         (skip-chars-forward " \t")
-                         (let* ((newcol (smie-indent-calculate))
-                                (newgain (- (current-column) newcol)))
+                                (and
+                                 (<= (point) end)
+                                 (<= (current-column) fc)
+                                 ;; FIXME?  `smie-indent-calculate' can
+                                 ;; (and often does) return a result that
+                                 ;; actually depends on the presence/absence
+                                 ;; of a newline, so the gain computed here
+                                 ;; may not be accurate, but in practice it
+                                 ;; seems to work well enough.
+                                 (progn
+                                   (skip-chars-forward " \t")
+                                   (setq newcol
+                                         (ignore-error 'scan-error
+                                           (smie-indent-calculate))))))
+                         (let ((newgain (- (current-column) newcol)))
                            (when (> newgain gain)
                              (setq gain newgain)
                              (setq bsf (point)))))


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

bug#19342: auto-fill scan-error in sh-mode

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Any comments?

Looks OK to me.  Tho maybe in the interest of minimizing the indentation
depth (and the amount of text changes in the patch), maybe we could
just use something like:

    (let ((newcol (condition-case nil
                      (smie-indent-calculate)
                    (scan-error (point-max))))
      [...]

where `point-max` is just a large enough value that the subsequent
computation of `newgain` leads to skipping this choice.


        Stefan

bug#19342: auto-fill scan-error in sh-mode

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Looks OK to me.  Tho maybe in the interest of minimizing the indentation
> depth (and the amount of text changes in the patch), maybe we could
> just use something like:
>
>     (let ((newcol (condition-case nil
>                       (smie-indent-calculate)
>                     (scan-error (point-max))))
>       [...]
>
> where `point-max` is just a large enough value that the subsequent
> computation of `newgain` leads to skipping this choice.

Sure, sounds simpler...  uhm...  but what would a good point-max be
here?  Just the `fc' point?

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

bug#19342: auto-fill scan-error in sh-mode

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Sure, sounds simpler...  uhm...  but what would a good point-max be
> here?  Just the `fc' point?

I'd think `point-max` does the trick.


        Stefan

bug#19342: auto-fill scan-error in sh-mode

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Sure, sounds simpler...  uhm...  but what would a good point-max be
>> here?  Just the `fc' point?
>
> I'd think `point-max` does the trick.

Tried that, and it leads to breakages in other parts of the loop:

Debugger entered--Lisp error: (scan-error "Containing expression ends prematurely" 74 74)
  signal(scan-error ("Containing expression ends prematurely" 74 74))
  smie-forward-sexp-command(1)
  forward-sexp(1)
  smie-indent-forward-token()
  smie-auto-fill(#f(compiled-function (&rest args) #<bytecode -0x1ddee5fc6c36b776>))
  apply(smie-auto-fill #f(compiled-function (&rest args) #<bytecode -0x1ddee5fc6c36b776>) nil)
  #f(advice smie-auto-fill :around #f(compiled-function (&rest args) #<bytecode -0x1ddee5fc6c36b776>))()
  internal-auto-fill()

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

bug#19342: auto-fill scan-error in sh-mode

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen [2022-05-06 14:26:38] wrote:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> Sure, sounds simpler...  uhm...  but what would a good point-max be
>>> here?  Just the `fc' point?
>>
>> I'd think `point-max` does the trick.
>
> Tried that, and it leads to breakages in other parts of the loop:
>
> Debugger entered--Lisp error: (scan-error "Containing expression ends prematurely" 74 74)
>   signal(scan-error ("Containing expression ends prematurely" 74 74))
>   smie-forward-sexp-command(1)
>   forward-sexp(1)
>   smie-indent-forward-token()
>   smie-auto-fill(#f(compiled-function (&rest args) #<bytecode -0x1ddee5fc6c36b776>))
>   apply(smie-auto-fill #f(compiled-function (&rest args) #<bytecode -0x1ddee5fc6c36b776>) nil)
>   #f(advice smie-auto-fill :around #f(compiled-function (&rest args)
> #<bytecode -0x1ddee5fc6c36b776>))()
>   internal-auto-fill()

I can't begin to imagine why we'd get that.
But how 'bout the patch below?


        Stefan


diff --git a/lisp/emacs-lisp/smie.el b/lisp/emacs-lisp/smie.el
index 2bab1319132..0da403c7053 100644
--- a/lisp/emacs-lisp/smie.el
+++ b/lisp/emacs-lisp/smie.el
@@ -1862,8 +1862,10 @@ smie-auto-fill
                          ;; here may not be accurate, but in practice it seems
                          ;; to work well enough.
                          (skip-chars-forward " \t")
-                         (let* ((newcol (smie-indent-calculate))
-                                (newgain (- (current-column) newcol)))
+                         (let* ((newgain (condition-case nil
+                                             (- (current-column)
+                                                (smie-indent-calculate))
+                                           (scan-error -1)))
                            (when (> newgain gain)
                              (setq gain newgain)
                              (setq bsf (point)))))

bug#19342: auto-fill scan-error in sh-mode

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I can't begin to imagine why we'd get that.
> But how 'bout the patch below?

Nope, same problem.  We don't seem to be breaking out of the while loop,
so it's erroring out in the next iteration at:

                       (while (progn
                                (smie-indent-forward-token)


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

bug#19342: auto-fill scan-error in sh-mode

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen [2021-08-31 03:24:35] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> It's not issuing an error.  It's emitting a message, the purpose
>> of which is to warn the user that the behavior was affected by
>> that problem.
>
> I get your point, and as you say, it works fine -- it auto-fills as it's
> supposed to.
>
> But it's literally signalling an error.  (And this is one of those rare
> instances where "literally" isn't a general intensifier.)  Changing it
> to a message would solve the issue, I think?

BTW, AFAICT it does not *literally* signal an error: the error is caught
inside `smie-auto-fill` and turned into a message (by
`with-demoted-errors`).

So if we want to silence the message, all it takes is to use
`ignore-error` instead as in the patch below, no?


        Stefan


diff --git a/lisp/emacs-lisp/smie.el b/lisp/emacs-lisp/smie.el
index 2bab1319132..2be1ca3218b 100644
--- a/lisp/emacs-lisp/smie.el
+++ b/lisp/emacs-lisp/smie.el
@@ -1846,7 +1846,7 @@ smie-auto-fill
                            (move-to-column fc)
                            (syntax-ppss))))
         (while
-            (and (with-demoted-errors "SMIE Error: %S"
+            (and (ignore-error scan-error
                    (save-excursion
                      (let ((end (point))
                            (bsf nil)    ;Best-so-far.

bug#19342: auto-fill scan-error in sh-mode

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> So if we want to silence the message, all it takes is to use
> `ignore-error` instead as in the patch below, no?

[...]

>          (while
> -            (and (with-demoted-errors "SMIE Error: %S"
> +            (and (ignore-error scan-error

Yup; that fixes the problem.

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

bug#19342: auto-fill scan-error in sh-mode

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen [2022-05-06 17:03:24] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> So if we want to silence the message, all it takes is to use
>> `ignore-error` instead as in the patch below, no?
>
> [...]
>
>>          (while
>> -            (and (with-demoted-errors "SMIE Error: %S"
>> +            (and (ignore-error scan-error
>
> Yup; that fixes the problem.

Pushed, thanks,


        Stefan