bug#4030: forward-sexp parses character literal ?; as comment

bug#4030: forward-sexp parses character literal ?; as comment

[era+emacsbugs]

Package: emacs
Version: 23.1.50.1
Severity: normal
X-Debbugs-Cc: era+emacsbugs@iki.fi

This is a pared-down version of Ubuntu bug report #405498
https://bugs.launchpad.net/bugs/405498 -- please see the URL for the
full bug report.

It seems that forward-sexp (and its underlying C implementation) does
not cope correctly with a character literal semicolon, seeing instead
(effectively) end of line.

In the *scratch* buffer if you write (insert ?;) you can evaluate this
Lisp code and it behaves as intended (inserts a semicolon in the current
buffer) but doing M-x forward-sexp just before the expression results in
an "Unbalanced parentheses" error.

This causes problems e.g. when Customize wants to edit an .emacs file
which contains the character constant ?; anywhere in it, because
Customize uses forward-sexp to parse the user's .emacs file.

/* era */

-- 
If this were a real .signature, it would suck less.  Well, maybe not.

/* era */

-- 
If this were a real .signature, it would suck less.  Well, maybe not.

bug#4030: forward-sexp parses character literal ?; as comment

[martin rudalics]

 > It seems that forward-sexp (and its underlying C implementation) does
 > not cope correctly with a character literal semicolon, seeing instead
 > (effectively) end of line.
 >
 > In the *scratch* buffer if you write (insert ?;) you can evaluate this
 > Lisp code and it behaves as intended (inserts a semicolon in the current
 > buffer) but doing M-x forward-sexp just before the expression results in
 > an "Unbalanced parentheses" error.

A similar thing happens with (insert ?") so why don't you escape such a
character by writing (insert ?\;) instead?  From the Elisp manual:

     You can use the same syntax for punctuation characters, but it is
  often a good idea to add a `\' so that the Emacs commands for editing
  Lisp code don't get confused.  For example, `?\(' is the way to write
  the open-paren character.  If the character is `\', you _must_ use a
  second `\' to quote it: `?\\'.

martin

bug#4030: forward-sexp parses character literal ?; as comment

[era+emacsbugs]

On Tue, 04 Aug 2009 14:43 +0200, "martin rudalics" <rudalics@gmx.at>
wrote:
>  > It seems that forward-sexp (and its underlying C implementation) does
>  > not cope correctly with a character literal semicolon, seeing instead
>  > (effectively) end of line.
>  >
>  > In the *scratch* buffer if you write (insert ?;) you can evaluate this
>  > Lisp code and it behaves as intended (inserts a semicolon in the current
>  > buffer) but doing M-x forward-sexp just before the expression results in
>  > an "Unbalanced parentheses" error.
> 
> A similar thing happens with (insert ?") so why don't you escape such a
> character by writing (insert ?\;) instead?

While the workaround is good (and documented in the Ubuntu bug as well),
the ability of Customize depends on this code working correctly, and it
should handle any nominally well-formed .emacs file.  Perhaps there are
other pieces of code which rely on forward-sexp et alii for Emacs Lisp
parsing as well.

I'll also point out that an "Unbalanced parentheses" error from deep
inside Customize is not a very helpful error message (especially as it
does not indicate in which buffer the unbalanced parentheses were
found); but perhaps Customize should be adapted to cope if forward-sexp
cannot easily be fixed.

It appears that src/syntax.c could perhaps be adapted to take into
account character literals as well as quoted strings, but I am not
familiar enough with Emacs internals to tell whether this is really a
feasible approach.

bug#4030: forward-sexp parses character literal ?; as comment

[martin rudalics]

 > While the workaround is good (and documented in the Ubuntu bug as well),
 > the ability of Customize depends on this code working correctly, and it
 > should handle any nominally well-formed .emacs file.  Perhaps there are
 > other pieces of code which rely on forward-sexp et alii for Emacs Lisp
 > parsing as well.

Using a backslash _is_ the canonical way for handling this problem.  If
some part of Emacs puts such a semicolon into an Elisp buffer without
escaping it, then that part of Emacs is wrong and has to be fixed.  If
you manually insert such a construct, then you are on your own (just as
when within a string you put a left paren in column zero).  We could try
to mark any `?;' or `?"' sequences appropriately when fontifying though.

 > I'll also point out that an "Unbalanced parentheses" error from deep
 > inside Customize is not a very helpful error message (especially as it
 > does not indicate in which buffer the unbalanced parentheses were
 > found); but perhaps Customize should be adapted to cope if forward-sexp
 > cannot easily be fixed.

Getting good diagnostics after a parsing error is hard.

 > It appears that src/syntax.c could perhaps be adapted to take into
 > account character literals as well as quoted strings, but I am not
 > familiar enough with Emacs internals to tell whether this is really a
 > feasible approach.

Let's say we give `?' character quote syntax in Elisp.  I suppose this
could be done.  But someone would have to rewrite the corresponding
parts of the parsing code.  I'm afraid there's hardly anyone around to
volunteer.  (And think of an `?' escaping a subsequent backslash.)

martin

bug#4030: forward-sexp parses character literal ?; as comment

[era+emacsbugs]

On Wed, 05 Aug 2009 16:29 +0200, "martin rudalics" <rudalics@gmx.at>
wrote:
>  > While the workaround is good (and documented in the Ubuntu bug as well),
>  > the ability of Customize depends on this code working correctly, and it
>  > should handle any nominally well-formed .emacs file.  Perhaps there are
>  > other pieces of code which rely on forward-sexp et alii for Emacs Lisp
>  > parsing as well.
> 
> Using a backslash _is_ the canonical way for handling this problem.  If
> some part of Emacs puts such a semicolon into an Elisp buffer without
> escaping it, then that part of Emacs is wrong and has to be fixed.  If
> you manually insert such a construct, then you are on your own (just as
> when within a string you put a left paren in column zero). 

Is this an authoritative statement from the Emacs maintainers (in which
case this bug should be closed as Invalid or Wontfix or whatever)?

>  > It appears that src/syntax.c could perhaps be adapted to take into
>  > account character literals as well as quoted strings, but I am not
>  > familiar enough with Emacs internals to tell whether this is really a
>  > feasible approach.
> 
> Let's say we give `?' character quote syntax in Elisp.  I suppose this
> could be done.  But someone would have to rewrite the corresponding
> parts of the parsing code.  I'm afraid there's hardly anyone around to
> volunteer.  (And think of an `?' escaping a subsequent backslash.)

Like I wrote, I'm not a good C programmer and not at all familiar with
the C internals of Emacs.  If you (the collective you) think this is not
feasible -- as opposed to there's nobody around to actually do it --
then that's another reason to close this bug.  I'm thinking it should
not be all that much harder than coping with quoted strings, which are
already (mostly) properly handled, but the actual code is idiosyncratic
and somewhat complex, so this is just a hunch of mine.

bug#4030: forward-sexp parses character literal ?; as comment

[Stefan Monnier]

>> While the workaround is good (and documented in the Ubuntu bug as well),
>> the ability of Customize depends on this code working correctly, and it
>> should handle any nominally well-formed .emacs file.  Perhaps there are
>> other pieces of code which rely on forward-sexp et alii for Emacs Lisp
>> parsing as well.

> Using a backslash _is_ the canonical way for handling this problem.  If
> some part of Emacs puts such a semicolon into an Elisp buffer without
> escaping it, then that part of Emacs is wrong and has to be fixed.

Indeed.

> If you manually insert such a construct, then you are on your own
> (just as when within a string you put a left paren in column zero).

Pretty much, yes.

> We could try to mark any `?;' or `?"' sequences appropriately when
> fontifying though.

We could/should/will improve the syntax parsing to handle those
things properly.  But it's a non-trivial amount of work, especially
since every major mode has similar issues but needs different
extra functionality.

>> I'll also point out that an "Unbalanced parentheses" error from deep
>> inside Customize is not a very helpful error message (especially as it
>> does not indicate in which buffer the unbalanced parentheses were
>> found); but perhaps Customize should be adapted to cope if forward-sexp
>> cannot easily be fixed.
> Getting good diagnostics after a parsing error is hard.

Agreed, but that doesn't mean we shouldn't intend to do better: the
current behavior (signalling an internal error to the user) is a bug
that needs to be fixed.

>> It appears that src/syntax.c could perhaps be adapted to take into
>> account character literals as well as quoted strings, but I am not
>> familiar enough with Emacs internals to tell whether this is really a
>> feasible approach.

Yes, clearly it deserves improvement, but that's nontrivial.


        Stefan

bug#4030: forward-sexp parses character literal ?; as comment

[martin rudalics]

 >> We could try to mark any `?;' or `?"' sequences appropriately when
 >> fontifying though.
 >
 > We could/should/will improve the syntax parsing to handle those
 > things properly.  But it's a non-trivial amount of work, especially
 > since every major mode has similar issues but needs different
 > extra functionality.

But fontification is mode-specific.  So it would be sufficient to look
for ?s that are not within strings or comments and followed by a
semicolon, paren or double-quote and mark that appropriately (obviously
the parser is derailed at that time but it still might help people spot
the bug).

 >>> I'll also point out that an "Unbalanced parentheses" error from deep
 >>> inside Customize is not a very helpful error message (especially as it
 >>> does not indicate in which buffer the unbalanced parentheses were
 >>> found); but perhaps Customize should be adapted to cope if forward-sexp
 >>> cannot easily be fixed.
 >> Getting good diagnostics after a parsing error is hard.
 >
 > Agreed, but that doesn't mean we shouldn't intend to do better: the
 > current behavior (signalling an internal error to the user) is a bug
 > that needs to be fixed.

If the problem comes from the unprotected

       (save-excursion (forward-sexp (buffer-size)))) ; Test for scan errors.

call in `custom-save-delete' we could simply wrap it as in the patch
below.  But do we really have to scan the buffer in the first place?

martin

*** cus-edit.el.~1.364.~	2009-07-27 08:09:05.997162900 +0200
--- cus-edit.el	2009-08-07 14:49:22.890625000 +0200
***************
*** 4341,4347 ****
     ;; Skip all whitespace and comments.
     (while (forward-comment 1))
     (or (eobp)
!       (save-excursion (forward-sexp (buffer-size)))) ; Test for scan errors.
     (let (first)
       (catch 'found
         (while t ;; We exit this loop only via throw.
--- 4341,4349 ----
     ;; Skip all whitespace and comments.
     (while (forward-comment 1))
     (or (eobp)
!       (condition-case nil
! 	  (save-excursion (forward-sexp (buffer-size))) ; Test for scan errors.
! 	(scan-error (error "Scan error, watch out for ... "))))
     (let (first)
       (catch 'found
         (while t ;; We exit this loop only via throw.

bug#4030: forward-sexp parses character literal ?; as comment

[Stefan Monnier]

>>> We could try to mark any `?;' or `?"' sequences appropriately when
>>> fontifying though.
>> We could/should/will improve the syntax parsing to handle those
>> things properly.  But it's a non-trivial amount of work, especially
>> since every major mode has similar issues but needs different
>> extra functionality.
> But fontification is mode-specific.  So it would be sufficient to look
> for ?s that are not within strings or comments and followed by a
> semicolon, paren or double-quote and mark that appropriately (obviously
> the parser is derailed at that time but it still might help people spot
> the bug).

Yes, but we need to do that even on chunks of code that have not yet
been (and may never be) displayed and in buffers where font-lock
is disabled.  IOW I'm not talking about fontification but about parsing.

>>>> I'll also point out that an "Unbalanced parentheses" error from deep
>>>> inside Customize is not a very helpful error message (especially as it
>>>> does not indicate in which buffer the unbalanced parentheses were
>>>> found); but perhaps Customize should be adapted to cope if forward-sexp
>>>> cannot easily be fixed.
>>> Getting good diagnostics after a parsing error is hard.
>> 
>> Agreed, but that doesn't mean we shouldn't intend to do better: the
>> current behavior (signalling an internal error to the user) is a bug
>> that needs to be fixed.

> If the problem comes from the unprotected

>       (save-excursion (forward-sexp (buffer-size)))) ; Test for scan errors.

> call in `custom-save-delete' we could simply wrap it as in the patch
> below.

Pretty much, yes, tho the error message should give more info (the OP
complained about lack of info in the error message).

> But do we really have to scan the buffer in the first place?

Don't know.  Maybe not, indeed.  Maybe it's just to detect the "too many
closing parens" case as well (i.e. rather than silently ignore trailing
code).


        Stefan

bug#4030: forward-sexp parses character literal ?; as comment

[martin rudalics]

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

 > Yes, but we need to do that even on chunks of code that have not yet
 > been (and may never be) displayed and in buffers where font-lock
 > is disabled.  IOW I'm not talking about fontification but about parsing.

Sure.  But I was only talking about the possibility to highlight such
instances just like we do with column 0 parens in strings already.

 > Pretty much, yes, tho the error message should give more info (the OP
 > complained about lack of info in the error message).

That's what the ellipsis stands for.  But what info?  Guessing a good
buffer position seems next to impossible.  Where else can `forward-sexp'
go astray when called from a top-level position?

 >> But do we really have to scan the buffer in the first place?
 >
 > Don't know.  Maybe not, indeed.  Maybe it's just to detect the "too many
 > closing parens" case as well (i.e. rather than silently ignore trailing
 > code).

We could simply search for (concat "^(" (symbol-name symbol))) and do a
`forward-sexp' over the form starting there as in the attached patch.  I
see no reason why we should try to handle an .emacs broken before or
after that form.

martin

[-- Attachment #2: cus-edit.el.diff --]
[-- Type: text/plain, Size: 2320 bytes --]

*** cus-edit.el.~1.364.~	2009-07-27 08:09:05.997162900 +0200
--- cus-edit.el	2009-08-11 10:51:21.812500000 +0200
***************
*** 4338,4374 ****
  
  This function does not save the buffer."
    (goto-char (point-min))
!   ;; Skip all whitespace and comments.
!   (while (forward-comment 1))
!   (or (eobp)
!       (save-excursion (forward-sexp (buffer-size)))) ; Test for scan errors.
!   (let (first)
!     (catch 'found
!       (while t ;; We exit this loop only via throw.
! 	;; Skip all whitespace and comments.
! 	(while (forward-comment 1))
! 	(let ((start (point))
! 	      (sexp (condition-case nil
! 			(read (current-buffer))
! 		      (end-of-file (throw 'found nil)))))
! 	  (when (and (listp sexp)
! 		     (eq (car sexp) symbol))
! 	    (delete-region start (point))
! 	    (unless first
! 	      (setq first (point)))))))
!     (if first
! 	(goto-char first)
!       ;; Move in front of local variables, otherwise long Custom
!       ;; entries would make them ineffective.
!       (let ((pos (point-max))
! 	    (case-fold-search t))
! 	(save-excursion
! 	  (goto-char (point-max))
! 	  (search-backward "\n\^L" (max (- (point-max) 3000) (point-min))
! 			   'move)
! 	  (when (search-forward "Local Variables:" nil t)
! 	    (setq pos (line-beginning-position))))
! 	(goto-char pos)))))
  
  (defun custom-save-variables ()
    "Save all customized variables in `custom-file'."
--- 4338,4364 ----
  
  This function does not save the buffer."
    (goto-char (point-min))
!   (if (re-search-forward (concat "^(" (symbol-name symbol)))
!       (let ((from (goto-char (match-beginning 0)))
! 	    (to (condition-case nil
! 		    (progn
! 		      (forward-sexp)
! 		      (point))
! 		  (error nil))))
! 	(if to
! 	    (delete-region from to)
! 	  (error "Malformed %s expression" symbol)))
!     ;; Move in front of local variables, otherwise long Custom
!     ;; entries would make them ineffective.
!     (let ((pos (point-max))
! 	  (case-fold-search t))
!       (save-excursion
! 	(goto-char (point-max))
! 	(search-backward "\n\^L" (max (- (point-max) 3000) (point-min))
! 			 'move)
! 	(when (search-forward "Local Variables:" nil t)
! 	  (setq pos (line-beginning-position))))
!       (goto-char pos))))
  
  (defun custom-save-variables ()
    "Save all customized variables in `custom-file'."

bug#4030: forward-sexp parses character literal ?; as comment

[Andrew Hyatt]

This problem still exists in Emacs 25, and this was the last message in
the thread.

When I try this, though, I do at least see the issue pretty clearly due
to font-locking.

Does anyone have any further thoughts?  It wasn't clear to me from this
discussion whether there was any consensus that this was actually a bug
or not.


martin rudalics <rudalics@gmx.at> writes:

>> Yes, but we need to do that even on chunks of code that have not yet
>> been (and may never be) displayed and in buffers where font-lock
>> is disabled.  IOW I'm not talking about fontification but about parsing.
>
> Sure.  But I was only talking about the possibility to highlight such
> instances just like we do with column 0 parens in strings already.
>
>> Pretty much, yes, tho the error message should give more info (the OP
>> complained about lack of info in the error message).
>
> That's what the ellipsis stands for.  But what info?  Guessing a good
> buffer position seems next to impossible.  Where else can `forward-sexp'
> go astray when called from a top-level position?
>
>>> But do we really have to scan the buffer in the first place?
>>
>> Don't know.  Maybe not, indeed.  Maybe it's just to detect the "too many
>> closing parens" case as well (i.e. rather than silently ignore trailing
>> code).
>
> We could simply search for (concat "^(" (symbol-name symbol))) and do a
> `forward-sexp' over the form starting there as in the attached patch.  I
> see no reason why we should try to handle an .emacs broken before or
> after that form.
>
> martin
>
> *** cus-edit.el.~1.364.~	2009-07-27 08:09:05.997162900 +0200
> --- cus-edit.el	2009-08-11 10:51:21.812500000 +0200
> ***************
> *** 4338,4374 ****
>   
>   This function does not save the buffer."
>     (goto-char (point-min))
> !   ;; Skip all whitespace and comments.
> !   (while (forward-comment 1))
> !   (or (eobp)
> !       (save-excursion (forward-sexp (buffer-size)))) ; Test for scan errors.
> !   (let (first)
> !     (catch 'found
> !       (while t ;; We exit this loop only via throw.
> ! 	;; Skip all whitespace and comments.
> ! 	(while (forward-comment 1))
> ! 	(let ((start (point))
> ! 	      (sexp (condition-case nil
> ! 			(read (current-buffer))
> ! 		      (end-of-file (throw 'found nil)))))
> ! 	  (when (and (listp sexp)
> ! 		     (eq (car sexp) symbol))
> ! 	    (delete-region start (point))
> ! 	    (unless first
> ! 	      (setq first (point)))))))
> !     (if first
> ! 	(goto-char first)
> !       ;; Move in front of local variables, otherwise long Custom
> !       ;; entries would make them ineffective.
> !       (let ((pos (point-max))
> ! 	    (case-fold-search t))
> ! 	(save-excursion
> ! 	  (goto-char (point-max))
> ! 	  (search-backward "\n\^L" (max (- (point-max) 3000) (point-min))
> ! 			   'move)
> ! 	  (when (search-forward "Local Variables:" nil t)
> ! 	    (setq pos (line-beginning-position))))
> ! 	(goto-char pos)))))
>   
>   (defun custom-save-variables ()
>     "Save all customized variables in `custom-file'."
> --- 4338,4364 ----
>   
>   This function does not save the buffer."
>     (goto-char (point-min))
> !   (if (re-search-forward (concat "^(" (symbol-name symbol)))
> !       (let ((from (goto-char (match-beginning 0)))
> ! 	    (to (condition-case nil
> ! 		    (progn
> ! 		      (forward-sexp)
> ! 		      (point))
> ! 		  (error nil))))
> ! 	(if to
> ! 	    (delete-region from to)
> ! 	  (error "Malformed %s expression" symbol)))
> !     ;; Move in front of local variables, otherwise long Custom
> !     ;; entries would make them ineffective.
> !     (let ((pos (point-max))
> ! 	  (case-fold-search t))
> !       (save-excursion
> ! 	(goto-char (point-max))
> ! 	(search-backward "\n\^L" (max (- (point-max) 3000) (point-min))
> ! 			 'move)
> ! 	(when (search-forward "Local Variables:" nil t)
> ! 	  (setq pos (line-beginning-position))))
> !       (goto-char pos))))
>   
>   (defun custom-save-variables ()
>     "Save all customized variables in `custom-file'."

bug#4030: forward-sexp parses character literal ?; as comment

[Stefan Monnier]

> Does anyone have any further thoughts?  It wasn't clear to me from this
> discussion whether there was any consensus that this was actually a bug
> or not.

With the support for syntax-fontification "on-the-fly" within
forward-sexp, we could actually make forward-sexp handle those
cases correctly nowadays.

So we can either:
- make it work with a syntax-propertize-function.  Not sure it's worth
  the cost.
- deprecate ?; and friends, probably adding font-lock highlights to help
  spot the problematic cases, and maybe changing the Lisp reader code to
  warn about those things as well (with intention to remove support for
  them).
- keep on living with this occasional problem.


        Stefan

bug#4030: forward-sexp parses character literal ?; as comment

[npostavs]

tags 4030 wontfix
close 4030
quit

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> Does anyone have any further thoughts?  It wasn't clear to me from this
>> discussion whether there was any consensus that this was actually a bug
>> or not.
>
>
> So we can either:
[...]
> - deprecate ?; and friends, probably adding font-lock highlights to help
>   spot the problematic cases, and maybe changing the Lisp reader code to
>   warn about those things as well (with intention to remove support for
>   them).

Since we now emit such warnings, I'm closing this bug as wontfix.

See also https://debbugs.gnu.org/cgi/bugreport.cgi?bug=20852

[1: c2bbdc3316]: 2017-05-01 20:39:10 +0200
  Warn about missing backslashes during load
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=c2bbdc3316487e34eba1470dd059c0c290431e00

[2: 3c4c8ca06e]: 2017-05-07 13:22:34 +0200
  Fix all unescaped character literals
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=3c4c8ca06e3306ccbcd07e354eb51abe53b52d22