GNU bug report logs - #64329
29.0.92; treesit/fill-paragraph syntax highlighting problem

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 64329 in the body.
You can then email your comments to 64329 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: Troy Brown <brownts <at> troybrown.dev>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.92; treesit/fill-paragraph syntax highlighting problem
Date: Wed, 28 Jun 2023 12:46:04 -0400

I've noticed this problem on multiple tree-sitter major modes including
c-ts-mode, c++-ts-mode, java-ts-mode, bash-ts-mode.  I haven't tried
others, but I suspect those might also suffer from this problem.

The issue occurs when attempting to fill the paragraph of a comment
block.  The following comment block can be used as an example to
reproduce the problem and happens with "emacs -Q" (assuming
corresponding tree-sitter libraries are available).

--8<---------------cut here---------------start------------->8---
// The quick brown fox jumps over the
// lazy dog.
// The quick brown fox jumps over the lazy dog.
--8<---------------cut here---------------end--------------->8---

Switch to one of the tree-sitter modes (e.g., M-x java-ts-mode).  Move
point to the first line of the comment block above and then execute the
fill-paragraph command (i.e., M-q).

The text which is wrapped onto the first line of the comment block will
be highlighted incorrectly.  The results appear as if the comment
delimiter was removed, fontification occurred, then the text was moved
to the first line of the comment block and never refontified with the
comment face.

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

From: Yuan Fu <casouri <at> gmail.com>
To: Troy Brown <brownts <at> troybrown.dev>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 64329 <at> debbugs.gnu.org
Subject: Re: bug#64329: 29.0.92; treesit/fill-paragraph syntax highlighting
 problem
Date: Wed, 28 Jun 2023 14:23:41 -0700

[Message part 1 (text/plain, inline)]

> On Jun 28, 2023, at 9:46 AM, Troy Brown <brownts <at> troybrown.dev> wrote:
> 
> I've noticed this problem on multiple tree-sitter major modes including
> c-ts-mode, c++-ts-mode, java-ts-mode, bash-ts-mode.  I haven't tried
> others, but I suspect those might also suffer from this problem.
> 
> The issue occurs when attempting to fill the paragraph of a comment
> block.  The following comment block can be used as an example to
> reproduce the problem and happens with "emacs -Q" (assuming
> corresponding tree-sitter libraries are available).
> 
> --8<---------------cut here---------------start------------->8---
> // The quick brown fox jumps over the
> // lazy dog.
> // The quick brown fox jumps over the lazy dog.
> --8<---------------cut here---------------end--------------->8---
> 
> Switch to one of the tree-sitter modes (e.g., M-x java-ts-mode).  Move
> point to the first line of the comment block above and then execute the
> fill-paragraph command (i.e., M-q).
> 
> The text which is wrapped onto the first line of the comment block will
> be highlighted incorrectly.  The results appear as if the comment
> delimiter was removed, fontification occurred, then the text was moved
> to the first line of the comment block and never refontified with the
> comment face.

Thank you very much! It’s funny that how long this went under the radar, presumably because we always use block comment.

The culprit is the subst-char-in-region function used by the filling function. It has a branch:

if (xxx)
  {
	replace_range (pos, pos + 1, string, ...);
  }
else
  {
	for (i = 0; i < len; i++) *p++ = tostr[i];
  }

I overlooked the else branch and thought subst-char-in-region always calls replace_range. replace_range notifies tree-sitter of the change it makes; but when subst-char-in-region manually replaces the text in the else branch, those edits are not notified to tree-sitter.

Please see the attached patch. Eli, is it more preferable to add a subroutine in insdel.c that does what "for (i = 0; i < len; i++) *p++ = tostr[I];” does, plus calling treesit_record_change, and make subst-char-in-region call that subroutine? (This way editfns.c don’t need to include treesit.h and call treesit_record_change itself.)

Yuan

[notify.patch (application/octet-stream, attachment)]

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

From: Yuan Fu <casouri <at> gmail.com>
To: Troy Brown <brownts <at> troybrown.dev>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 64329 <at> debbugs.gnu.org
Subject: Re: bug#64329: 29.0.92; treesit/fill-paragraph syntax highlighting
 problem
Date: Wed, 28 Jun 2023 17:17:14 -0700

[Message part 1 (text/plain, inline)]

> On Jun 28, 2023, at 2:23 PM, Yuan Fu <casouri <at> gmail.com> wrote:
> 
> 
> 
>> On Jun 28, 2023, at 9:46 AM, Troy Brown <brownts <at> troybrown.dev> wrote:
>> 
>> I've noticed this problem on multiple tree-sitter major modes including
>> c-ts-mode, c++-ts-mode, java-ts-mode, bash-ts-mode.  I haven't tried
>> others, but I suspect those might also suffer from this problem.
>> 
>> The issue occurs when attempting to fill the paragraph of a comment
>> block.  The following comment block can be used as an example to
>> reproduce the problem and happens with "emacs -Q" (assuming
>> corresponding tree-sitter libraries are available).
>> 
>> --8<---------------cut here---------------start------------->8---
>> // The quick brown fox jumps over the
>> // lazy dog.
>> // The quick brown fox jumps over the lazy dog.
>> --8<---------------cut here---------------end--------------->8---
>> 
>> Switch to one of the tree-sitter modes (e.g., M-x java-ts-mode).  Move
>> point to the first line of the comment block above and then execute the
>> fill-paragraph command (i.e., M-q).
>> 
>> The text which is wrapped onto the first line of the comment block will
>> be highlighted incorrectly.  The results appear as if the comment
>> delimiter was removed, fontification occurred, then the text was moved
>> to the first line of the comment block and never refontified with the
>> comment face.
> 
> Thank you very much! It’s funny that how long this went under the radar, presumably because we always use block comment.
> 
> The culprit is the subst-char-in-region function used by the filling function. It has a branch:
> 
> if (xxx)
>  {
> replace_range (pos, pos + 1, string, ...);
>  }
> else
>  {
> for (i = 0; i < len; i++) *p++ = tostr[i];
>  }
> 
> I overlooked the else branch and thought subst-char-in-region always calls replace_range. replace_range notifies tree-sitter of the change it makes; but when subst-char-in-region manually replaces the text in the else branch, those edits are not notified to tree-sitter.

Prompted by this, I went over all the functions that calls signal_after_change again, and found two other editfns.c functions that are missing calls to treesit_record_change. Please see the attached patches that follows the previous one. Sorry for the overlook. I believe I’ve found all places that needs to call treesit_record_change now.

> Please see the attached patch. Eli, is it more preferable to add a subroutine in insdel.c that does what "for (i = 0; i < len; i++) *p++ = tostr[I];” does, plus calling treesit_record_change, and make subst-char-in-region call that subroutine? (This way editfns.c don’t need to include treesit.h and call treesit_record_change itself.)

Since now there are three functions in editfns.c that needs to call treesit_record_change, we might as well just include treesit.h and call treesit_record_change directly.

Yuan

[add-two-other-missing-calls.patch (application/octet-stream, attachment)]

[Message part 3 (text/plain, inline)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Yuan Fu <casouri <at> gmail.com>
Cc: brownts <at> troybrown.dev, 64329 <at> debbugs.gnu.org
Subject: Re: bug#64329: 29.0.92; treesit/fill-paragraph syntax highlighting
 problem
Date: Thu, 29 Jun 2023 08:10:05 +0300

> From: Yuan Fu <casouri <at> gmail.com>
> Date: Wed, 28 Jun 2023 14:23:41 -0700
> Cc: 64329 <at> debbugs.gnu.org,
>  Eli Zaretskii <eliz <at> gnu.org>
> 
> The culprit is the subst-char-in-region function used by the filling function. It has a branch:
> 
> if (xxx)
>   {
> 	replace_range (pos, pos + 1, string, ...);
>   }
> else
>   {
> 	for (i = 0; i < len; i++) *p++ = tostr[i];
>   }
> 
> I overlooked the else branch and thought subst-char-in-region always calls replace_range. replace_range notifies tree-sitter of the change it makes; but when subst-char-in-region manually replaces the text in the else branch, those edits are not notified to tree-sitter.
> 
> Please see the attached patch. Eli, is it more preferable to add a subroutine in insdel.c that does what "for (i = 0; i < len; i++) *p++ = tostr[I];” does, plus calling treesit_record_change, and make subst-char-in-region call that subroutine? (This way editfns.c don’t need to include treesit.h and call treesit_record_change itself.)

I'd prefer to install the patch you show on the emacs-29 branch, and
then clean it up with a subroutine in insdel.c on master.

We could also leave this on master without adding a subroutine:
casefiddle.c already does something similar, so it's not like
including treesit.h is known to cause trouble or something.

Thanks.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Yuan Fu <casouri <at> gmail.com>
Cc: brownts <at> troybrown.dev, 64329 <at> debbugs.gnu.org
Subject: Re: bug#64329: 29.0.92; treesit/fill-paragraph syntax highlighting
 problem
Date: Thu, 29 Jun 2023 08:22:49 +0300

> From: Yuan Fu <casouri <at> gmail.com>
> Date: Wed, 28 Jun 2023 17:17:14 -0700
> Cc: 64329 <at> debbugs.gnu.org,
>  Eli Zaretskii <eliz <at> gnu.org>
> 
> Prompted by this, I went over all the functions that calls signal_after_change again, and found two other editfns.c functions that are missing calls to treesit_record_change. Please see the attached patches that follows the previous one. Sorry for the overlook. I believe I’ve found all places that needs to call treesit_record_change now.
> 
> > Please see the attached patch. Eli, is it more preferable to add a subroutine in insdel.c that does what "for (i = 0; i < len; i++) *p++ = tostr[I];” does, plus calling treesit_record_change, and make subst-char-in-region call that subroutine? (This way editfns.c don’t need to include treesit.h and call treesit_record_change itself.)
> 
> Since now there are three functions in editfns.c that needs to call treesit_record_change, we might as well just include treesit.h and call treesit_record_change directly.

Right.

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

From: Yuan Fu <casouri <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Troy Brown <brownts <at> troybrown.dev>, 64329-done <at> debbugs.gnu.org
Subject: Re: bug#64329: 29.0.92; treesit/fill-paragraph syntax highlighting
 problem
Date: Thu, 29 Jun 2023 11:17:26 -0700

> On Jun 28, 2023, at 10:22 PM, Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
>> From: Yuan Fu <casouri <at> gmail.com>
>> Date: Wed, 28 Jun 2023 17:17:14 -0700
>> Cc: 64329 <at> debbugs.gnu.org,
>> Eli Zaretskii <eliz <at> gnu.org>
>> 
>> Prompted by this, I went over all the functions that calls signal_after_change again, and found two other editfns.c functions that are missing calls to treesit_record_change. Please see the attached patches that follows the previous one. Sorry for the overlook. I believe I’ve found all places that needs to call treesit_record_change now.
>> 
>>> Please see the attached patch. Eli, is it more preferable to add a subroutine in insdel.c that does what "for (i = 0; i < len; i++) *p++ = tostr[I];” does, plus calling treesit_record_change, and make subst-char-in-region call that subroutine? (This way editfns.c don’t need to include treesit.h and call treesit_record_change itself.)
>> 
>> Since now there are three functions in editfns.c that needs to call treesit_record_change, we might as well just include treesit.h and call treesit_record_change directly.
> 
> Right.

Ok, I've pushed the changes.

Yuan

Previous Next

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