GNU bug report logs - #15251
24.3.50; do-auto-fill "continues" comment from inside a string

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 15251 in the body.
You can then email your comments to 15251 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: Dmitry Gutov <dgutov <at> yandex.ru>
To: bug-gnu-emacs <at> gnu.org
Subject: 24.3.50; do-auto-fill "continues" comment from inside a string
Date: Tue, 03 Sep 2013 04:26:26 +0300

In certain conditions, if I press SPC, and a string on the current line
contains text matching `comment-start-skip', the filling is performed,
and the newly created line starts with a comment.

Examples (point is at |, fill-column is 70):

ruby-mode:

aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa#{}a" a|

press SPC =>

aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa#{}a"
                                #a

js-mode:

aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa//a" aa|

press SPC =>

aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa//a"
//aa 

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: 15251 <at> debbugs.gnu.org
Subject: Re: bug#15251: 24.3.50;
 do-auto-fill "continues" comment from inside a string
Date: Sun, 29 Sep 2013 06:59:17 +0300

Dmitry Gutov <dgutov <at> yandex.ru> writes:

> In certain conditions, if I press SPC, and a string on the current line
> contains text matching `comment-start-skip', the filling is performed,
> and the newly created line starts with a comment.
>
> Examples (point is at |, fill-column is 70):
>
> ruby-mode:
>
> aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa#{}a" a|
>
> press SPC =>
>
> aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa#{}a"
>                                 #a
>
> js-mode:
>
> aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa//a" aa|
>
> press SPC =>
>
> aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa//a"
> //aa 

I've tracked this to misbehaving `comment-beginning'. Since we start
outside of string, and the comment char(s) are inside the string on the
same line, the check comparing the face at point to
`font-lock-string-face' doesn't work.

And in ruby-mode's case, since interpolations use a different face, even
putting a similar guard to check at `cs' won't help.

As I see it, we either have to use `syntax-ppss' here, or trust
font-lock. The latter means not skipping other checks even if
`comment-end-skip' is defined and doesn't match at point, and also
only trusting "let's assume ... if we're on the same line" when
font-lock-mode is disabled.

How does this change look? Seems to work fine.

(As an aside, I'm having hard time understanding which search is
supposed to have set the match data used by `(match-end 0)' there. Is it
`comment-search-backward'? Or `(looking-at comment-end-skip)'?)

=== modified file 'lisp/newcomment.el'
--- lisp/newcomment.el	2013-06-18 17:57:56 +0000
+++ lisp/newcomment.el	2013-09-29 03:41:18 +0000
@@ -526,12 +526,13 @@
 	      (and
 	       ;; For modes where comment-start and comment-end are the same,
 	       ;; the search above may have found a `ce' rather than a `cs'.
-	       (or (if comment-end-skip (not (looking-at comment-end-skip)))
-		   ;; Maybe font-lock knows that it's a `cs'?
+               (if comment-end-skip (not (looking-at comment-end-skip)))
+	       (or ;; Maybe font-lock knows that it's a `cs'?
 		   (eq (get-text-property (match-end 0) 'face)
 		       'font-lock-comment-face)
-		   (unless (eq (get-text-property (point) 'face)
-			       'font-lock-comment-face)
+		   (unless (or (eq (get-text-property (point) 'face)
+                                   'font-lock-comment-face)
+                               font-lock-mode)
 		     ;; Let's assume it's a `cs' if we're on the same line.
 		     (>= (line-end-position) pt)))
 	       ;; Make sure that PT is not past the end of the comment.

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

From: Andreas Röhler <andreas.roehler <at> easy-emacs.de>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#15251: 24.3.50; do-auto-fill "continues" comment from inside
 a string
Date: Sun, 29 Sep 2013 15:12:57 +0200

Am 29.09.2013 05:59, schrieb Dmitry Gutov:
> Dmitry Gutov <dgutov <at> yandex.ru> writes:
>
>> In certain conditions, if I press SPC, and a string on the current line
>> contains text matching `comment-start-skip', the filling is performed,
>> and the newly created line starts with a comment.
>>
>> Examples (point is at |, fill-column is 70):
>>
>> ruby-mode:
>>
>> aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa#{}a" a|
>>
>> press SPC =>
>>
>> aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa#{}a"
>>                                  #a
>>
>> js-mode:
>>
>> aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa//a" aa|
>>
>> press SPC =>
>>
>> aa = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa//a"
>> //aa
>
> I've tracked this to misbehaving `comment-beginning'. Since we start
> outside of string, and the comment char(s) are inside the string on the
> same line, the check comparing the face at point to
> `font-lock-string-face' doesn't work.
>
> And in ruby-mode's case, since interpolations use a different face, even
> putting a similar guard to check at `cs' won't help.
>
> As I see it, we either have to use `syntax-ppss' here, or trust
> font-lock. The latter means not skipping other checks even if
> `comment-end-skip' is defined and doesn't match at point, and also
> only trusting "let's assume ... if we're on the same line" when
> font-lock-mode is disabled.
>
> How does this change look? Seems to work fine.
>
> (As an aside, I'm having hard time understanding which search is
> supposed to have set the match data used by `(match-end 0)' there. Is it
> `comment-search-backward'? Or `(looking-at comment-end-skip)'?)
>
> === modified file 'lisp/newcomment.el'
> --- lisp/newcomment.el	2013-06-18 17:57:56 +0000
> +++ lisp/newcomment.el	2013-09-29 03:41:18 +0000
> @@ -526,12 +526,13 @@
>   	      (and
>   	       ;; For modes where comment-start and comment-end are the same,
>   	       ;; the search above may have found a `ce' rather than a `cs'.
> -	       (or (if comment-end-skip (not (looking-at comment-end-skip)))
> -		   ;; Maybe font-lock knows that it's a `cs'?
> +               (if comment-end-skip (not (looking-at comment-end-skip)))
> +	       (or ;; Maybe font-lock knows that it's a `cs'?
>   		   (eq (get-text-property (match-end 0) 'face)
>   		       'font-lock-comment-face)
> -		   (unless (eq (get-text-property (point) 'face)
> -			       'font-lock-comment-face)
> +		   (unless (or (eq (get-text-property (point) 'face)
> +                                   'font-lock-comment-face)
> +                               font-lock-mode)
>   		     ;; Let's assume it's a `cs' if we're on the same line.
>   		     (>= (line-end-position) pt)))
>   	       ;; Make sure that PT is not past the end of the comment.
>
>
>
>
>

Reading the face for detecting basic things like comment is terrible.
What did the author smoke when writing this ;)

BTW from my experience jumping to (nth 8 (syntax-ppss)) works well.

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Andreas Röhler <andreas.roehler <at> easy-emacs.de>
Cc: 15251 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#15251: 24.3.50;
 do-auto-fill "continues" comment from inside a string
Date: Sun, 29 Sep 2013 18:16:29 +0300

Andreas Röhler <andreas.roehler <at> easy-emacs.de> writes:
> Reading the face for detecting basic things like comment is terrible.

It sure is. It can be justified for very hot functions, but otherwise
`syntax-ppss' does a better job in (more or less) constant time.

But that code was written in 2001, a few months before `syntax-ppss' was
introduced.

> What did the author smoke when writing this ;)

Let's ask him. :)

Stefan, can we consider `syntax-ppss' fast enough at this point?

It will mean re-implementing `comment-search-backward' in terms of it,
and removing most of the code in `comment-beginning' definition.

AFAICT, both of these functions are not particularly hot, and are called
once or twice per user action, at most.

`comment-search-backward' can also be made to respect
`comment-use-syntax' (and use its current definition if that var's value
is nil), but if comments don't have valid entries in the syntax table,
font-lock won't recognize them (or will it?), and then the dance with
face properties is not useful either.

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

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 15251 <at> debbugs.gnu.org
Subject: Re: bug#15251: 24.3.50;
 do-auto-fill "continues" comment from inside a string
Date: Mon, 30 Sep 2013 14:26:41 -0400

> As I see it, we either have to use `syntax-ppss' here, or trust
> font-lock.

Relying on syntax-ppss would be better than relying font-lock faces.


        Stefan

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

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 15251 <at> debbugs.gnu.org,
 Andreas Röhler <andreas.roehler <at> easy-emacs.de>
Subject: Re: bug#15251: 24.3.50;
 do-auto-fill "continues" comment from inside a string
Date: Mon, 30 Sep 2013 14:27:39 -0400

> Stefan, can we consider `syntax-ppss' fast enough at this point?

If comment-use-syntax is t, yes.


        Stefan

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 15251-done <at> debbugs.gnu.org,
 Andreas Röhler <andreas.roehler <at> easy-emacs.de>
Subject: Re: bug#15251: 24.3.50; do-auto-fill "continues" comment from inside
 a string
Date: Tue, 01 Oct 2013 04:18:25 +0300

Version: 24.4

On 30.09.2013 21:27, Stefan Monnier wrote:
>> Stefan, can we consider `syntax-ppss' fast enough at this point?
>
> If comment-use-syntax is t, yes.

Done, in 114486.

I kept the rest of `comment-beginning' definition, in case 
non-syntax-table-using comments can still be font-locked in some modes.

Also, `comment-use-global-state' looks like a better var to base the 
choice on, semantically, but it's not auto-detected, and it's only used 
in `comment-search-forward'.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 15251-done <at> debbugs.gnu.org,
 Andreas Röhler <andreas.roehler <at> easy-emacs.de>
Subject: Re: bug#15251: 24.3.50;
 do-auto-fill "continues" comment from inside a string
Date: Tue, 01 Oct 2013 10:11:57 -0400

> Also, `comment-use-global-state' looks like a better var to base the choice

Good point.

> on, semantically, but it's not auto-detected, and it's only used in
> comment-search-forward'.

We should probably rework the code to merge comment-use-syntax and
comment-use-global-state.


        Stefan

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 15251-done <at> debbugs.gnu.org
Subject: Re: bug#15251: 24.3.50; do-auto-fill "continues" comment from inside
 a string
Date: Tue, 01 Oct 2013 18:40:43 +0300

On 01.10.2013 17:11, Stefan Monnier wrote:
> We should probably rework the code to merge comment-use-syntax and
> comment-use-global-state.

I wonder what modes have the former variable set to t, but the latter to 
nil, and which situation this combination handles.

Maybe enhance the check in `comment-normalize-vars' and set 
`comment-use-syntax' to nil when the syntax table values are not good 
enough for `comment-use-global-state'?

Then make `comment-use-global-state' an obsolete alias for 
`comment-use-syntax'.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 15251-done <at> debbugs.gnu.org
Subject: Re: bug#15251: 24.3.50;
 do-auto-fill "continues" comment from inside a string
Date: Tue, 01 Oct 2013 20:32:06 -0400

>> We should probably rework the code to merge comment-use-syntax and
>> comment-use-global-state.

> I wonder what modes have the former variable set to t, but the latter to
> nil, and which situation this combination handles.

> Maybe enhance the check in `comment-normalize-vars' and set
> comment-use-syntax' to nil when the syntax table values are not good enough
> for `comment-use-global-state'?

> Then make `comment-use-global-state' an obsolete alias for
> comment-use-syntax'.

Maybe a better way to do it is:
- Change code that uses comment-use-global-state to use (and
  comment-use-syntax comment-use-global-state) instead.
- Set comment-use-global-state to t by default.
- Major modes where comment-use-global-state is problematic (if those
  exist) can then set comment-use-global-state to nil.
- Maybe make comment-use-global-state obsolete.

Context: the problem with syntax-ppss is not so much performance as
correctness, because syntax-ppss can get confused if you use several
syntax-tables in the same buffer (e.g. via font-lock-syntax-table), or
if you use narrowing.

As time goes on, more and more code relies on syntax-ppss so more and
more code gets rewritten to avoid font-lock-syntax-table (or at least
use it in "harmless" ways, e.g. only changing syntax from symbol to
word) and narrowing.  In turn, this makes syntax-ppss more robust and
more attractive.


        Stefan

Message #37 received at 15251-done <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 15251-done <at> debbugs.gnu.org
Subject: Re: bug#15251: 24.3.50; do-auto-fill "continues" comment from inside
 a string
Date: Wed, 02 Oct 2013 03:50:34 +0300

On 02.10.2013 03:32, Stefan Monnier wrote:
> - Change code that uses comment-use-global-state to use (and
>    comment-use-syntax comment-use-global-state) instead.

This is already the case: `comment-search-forward' consults the value of 
`comment-use-global-state' only when `comment-use-syntax' is truthy.

> - Maybe make comment-use-global-state obsolete.
>
> Context: the problem with syntax-ppss is not so much performance as
> correctness, because syntax-ppss can get confused if you use several
> syntax-tables in the same buffer (e.g. via font-lock-syntax-table), or
> if you use narrowing.

Yes, that seems hard to test for.

And if `syntax-ppss' can be borked in some modes, using its value in 
`comment-beginning' like I did in the respective patch, also seems unsafe.

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

From: Andreas Röhler <andreas.roehler <at> easy-emacs.de>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>, Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 15251 <at> debbugs.gnu.org
Subject: Re: bug#15251: 24.3.50; do-auto-fill "continues" comment from inside
 a string
Date: Wed, 02 Oct 2013 09:22:44 +0200

Am 30.09.2013 20:27, schrieb Stefan Monnier:
>> Stefan, can we consider `syntax-ppss' fast enough at this point?
>
> If comment-use-syntax is t, yes.
>
>
>          Stefan
>

In case not, an alternative to faces should be introduced still.
Setting of comment-face might fail for several reasons.
Comment-move should not rely on that.

What about searching for comment-start than?

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Andreas Röhler <andreas.roehler <at> easy-emacs.de>
Cc: 15251 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Subject: Re: bug#15251: 24.3.50;
 do-auto-fill "continues" comment from inside a string
Date: Wed, 02 Oct 2013 14:19:58 +0300

Andreas Röhler <andreas.roehler <at> easy-emacs.de> writes:
> In case not, an alternative to faces should be introduced still.
> Setting of comment-face might fail for several reasons.
> Comment-move should not rely on that.
>
> What about searching for comment-start than?

`comment-beginning' calls `comment-search-backward', which searches for
`comment-start-skip'.

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

From: Andreas Röhler <andreas.roehler <at> easy-emacs.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 15251 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Subject: Re: bug#15251: 24.3.50; do-auto-fill "continues" comment from inside
 a string
Date: Wed, 02 Oct 2013 14:24:01 +0200

Am 02.10.2013 13:19, schrieb Dmitry Gutov:
> Andreas Röhler <andreas.roehler <at> easy-emacs.de> writes:
>> In case not, an alternative to faces should be introduced still.
>> Setting of comment-face might fail for several reasons.
>> Comment-move should not rely on that.
>>
>> What about searching for comment-start than?
>
> `comment-beginning' calls `comment-search-backward', which searches for
> `comment-start-skip'.
>

Okay, so it got rid of the faces-search?

Cheers

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Andreas Röhler <andreas.roehler <at> easy-emacs.de>
Cc: 15251 <at> debbugs.gnu.org
Subject: Re: bug#15251: 24.3.50; do-auto-fill "continues" comment from inside
 a string
Date: Wed, 02 Oct 2013 15:25:01 +0300

On 02.10.2013 15:24, Andreas Röhler wrote:
> Okay, so it got rid of the faces-search?

Why don't you look at the source code?

`comment-beginning' looks at faces to make sure it's not inside a 
string, or that the `comment-start-skip' it found doesn't end the 
comment instead of starting it, in case comment-start and comment-end 
delimiters are the same.

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

From: Andreas Röhler <andreas.roehler <at> easy-emacs.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 15251 <at> debbugs.gnu.org
Subject: Re: bug#15251: 24.3.50; do-auto-fill "continues" comment from inside
 a string
Date: Wed, 02 Oct 2013 16:08:11 +0200

Am 02.10.2013 14:25, schrieb Dmitry Gutov:
> On 02.10.2013 15:24, Andreas Röhler wrote:
>> Okay, so it got rid of the faces-search?
>
> Why don't you look at the source code?
>
> `comment-beginning' looks at faces to make sure it's not inside a string,

Doesn't make sense for me. In which way is a string supposed to bear string-face, if syntax doesn't exist?
That's shooting into the dark IMHO.

 or that the `comment-start-skip' it found doesn't end the comment instead of starting it, in case
> comment-start and comment-end delimiters are the same.
>

Same here.

If syntax doesn't exist, only a parser may help.

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

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Andreas Röhler <andreas.roehler <at> easy-emacs.de>
Cc: 15251 <at> debbugs.gnu.org
Subject: Re: bug#15251: 24.3.50; do-auto-fill "continues" comment from inside
 a string
Date: Fri, 04 Oct 2013 05:09:21 +0300

On 02.10.2013 17:08, Andreas Röhler wrote:
> Doesn't make sense for me. In which way is a string supposed to bear
> string-face, if syntax doesn't exist?

I'm just guessing, but syntax table may have syntax entries for string 
delimiters, but not for comments (e.g. because their syntax is too weird 
to be adequately expressed using existing syntax table classes), so 
comments may be fontified via explicit font-lock-keywords entries.

Another option is, like Stefan mentioned, if font-lock-syntax-table is 
defined, it's used for fontification, and it's very different from the 
actual buffer syntax table.

> That's shooting into the dark IMHO.

That code is old. I'm pretty sure nobody would've written it if there 
weren't any modes in the wild at the time that benefited from it.

Previous Next

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