GNU bug report logs - #65234
[PATCH] Fix jsx font-lock in older tree-sitter-js grammars

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 65234 in the body.
You can then email your comments to 65234 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: Danny Freeman <danny <at> dfreeman.email>
To: bug-gnu-emacs <at> gnu.org
Cc: Vincenzo Pupillo <v.pupillo <at> gmail.com>,
 Daniel Colascione <dancol <at> dancol.org>
Subject: [PATCH] Fix jsx font-lock in older tree-sitter-js grammars
Date: Fri, 11 Aug 2023 17:18:32 -0400

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

I have a patch to fix a bug I found in js-ts-mode.

Description of Problem:
A new function was wrtiten to account for a breaking change in the
tree-sitter-javascript grammar, see [0]

This function returns some tree-sitter queries used by the js-ts-mode
font lock settings.

We attempted to check for the breaking change by running a query
(treesit-query-capture 'javascript '((member_expression) @capture))
which would in theory fail when an old grammar is being used.

The problem is that this query does not fail on old grammars, so the
backwards incompatible font-lock queries are always used. When
font-locking a JSX file, the queries that look like
(jsx_opening_element [(member_expression) ...
throw errors and font locking fails (only on jsx elements).

Solution:
Instead of fixing the bb1f97b compatibility check with something like

(treesit-query-capture 'javascript
  '((jsx_opening_element (member_expression) @capture)))

I have opted re-write the font lock rules that capture the same
nodes by the "name:" field instead of by the specific node names (where
the backwards incompatible change took place).

The possible nodes that can be in the "name" field: 
After [1]: "identifier" "member_expression" "jsx_namespace_name"
Before [2]: "identifier" "nested_identifier" "jsx_namespace_name"

"jsx_namespace_name" is an additional node captured by these rules. As
an example, in the JSX expression

<Foo:bar/>

"Foo:bar" is a node of type "jsx_namespace_name" and would be captured
with @font-lock-function-call-face, where before this change it was not
captured or highlighted at all. If there is a reason this should not be
captured and highlighted, a new query can be added in to capture these
nodes first with @default so they are not highlighted like before.

Some thoughts:
These backwards incompatible grammar changes are rough to deal with, but
I'm glad they are taken into account. I use nixos, and the current
stable version 23.05 packages the older version of the JS grammar for
emacs which is how I noticed this. I wish the developers of the js
grammar were more careful about backwards compatible changes. The fact
that they were renaming a node without much reason didn't seem to bother
any of the maintainers.

[0001-Fix-jsx-font-lock-in-older-tree-sitter-js-grammars.patch (text/patch, attachment)]

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

Sources
[0]:
https://github.com/tree-sitter/tree-sitter-javascript/commit/bb1f97b643b77fc1f082d621bf533b4b14cf0c30
[1]:
https://github.com/tree-sitter/tree-sitter-javascript/blob/bb1f97b643b77fc1f082d621bf533b4b14cf0c30/grammar.js#L637-L641
[2]:
https://github.com/tree-sitter/tree-sitter-javascript/blob/5720b249490b3c17245ba772f6be4a43edb4e3b7/grammar.js#L635-L639

Thank you,
-- 
Danny Freeman

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Danny Freeman <danny <at> dfreeman.email>, Yuan Fu <casouri <at> gmail.com>,
 Theodor Thornhill <theo <at> thornhill.no>
Cc: 65234 <at> debbugs.gnu.org, v.pupillo <at> gmail.com, dancol <at> dancol.org
Subject: Re: bug#65234: [PATCH] Fix jsx font-lock in older tree-sitter-js
 grammars
Date: Sat, 12 Aug 2023 08:41:45 +0300

> Cc: Vincenzo Pupillo <v.pupillo <at> gmail.com>,
>  Daniel Colascione <dancol <at> dancol.org>
> Date: Fri, 11 Aug 2023 17:18:32 -0400
> From:  Danny Freeman via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> I have a patch to fix a bug I found in js-ts-mode.
> 
> Description of Problem:
> A new function was wrtiten to account for a breaking change in the
> tree-sitter-javascript grammar, see [0]
> 
> This function returns some tree-sitter queries used by the js-ts-mode
> font lock settings.
> 
> We attempted to check for the breaking change by running a query
> (treesit-query-capture 'javascript '((member_expression) @capture))
> which would in theory fail when an old grammar is being used.
> 
> The problem is that this query does not fail on old grammars, so the
> backwards incompatible font-lock queries are always used. When
> font-locking a JSX file, the queries that look like
> (jsx_opening_element [(member_expression) ...
> throw errors and font locking fails (only on jsx elements).
> 
> Solution:
> Instead of fixing the bb1f97b compatibility check with something like
> 
> (treesit-query-capture 'javascript
>   '((jsx_opening_element (member_expression) @capture)))
> 
> I have opted re-write the font lock rules that capture the same
> nodes by the "name:" field instead of by the specific node names (where
> the backwards incompatible change took place).
> 
> The possible nodes that can be in the "name" field: 
> After [1]: "identifier" "member_expression" "jsx_namespace_name"
> Before [2]: "identifier" "nested_identifier" "jsx_namespace_name"
> 
> "jsx_namespace_name" is an additional node captured by these rules. As
> an example, in the JSX expression
> 
> <Foo:bar/>
> 
> "Foo:bar" is a node of type "jsx_namespace_name" and would be captured
> with @font-lock-function-call-face, where before this change it was not
> captured or highlighted at all. If there is a reason this should not be
> captured and highlighted, a new query can be added in to capture these
> nodes first with @default so they are not highlighted like before.
> 
> Some thoughts:
> These backwards incompatible grammar changes are rough to deal with, but
> I'm glad they are taken into account. I use nixos, and the current
> stable version 23.05 packages the older version of the JS grammar for
> emacs which is how I noticed this. I wish the developers of the js
> grammar were more careful about backwards compatible changes. The fact
> that they were renaming a node without much reason didn't seem to bother
> any of the maintainers.

Yuan, Theo: any comments on this?  Is this safe enough to go into the
emacs-29 branch?

Thanks.

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

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Eli Zaretskii <eliz <at> gnu.org>, Danny Freeman <danny <at> dfreeman.email>,
 Yuan Fu <casouri <at> gmail.com>, Theodor Thornhill <theo <at> thornhill.no>
Cc: 65234 <at> debbugs.gnu.org, v.pupillo <at> gmail.com, dancol <at> dancol.org
Subject: Re: bug#65234: [PATCH] Fix jsx font-lock in older tree-sitter-js
 grammars
Date: Wed, 16 Aug 2023 05:04:10 +0300

On 12/08/2023 08:41, Eli Zaretskii wrote:
> Yuan, Theo: any comments on this?  Is this safe enough to go into the
> emacs-29 branch?

It looks good to me (and works okay too). I also saw the same error in 
my testing.

Especially since Emacs 29.2 is not quite imminent, I think it's a good 
idea to install this: queries are simpler and thus more reliable. And 
the fixed-fix, of course.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: casouri <at> gmail.com, 65234-done <at> debbugs.gnu.org, danny <at> dfreeman.email,
 v.pupillo <at> gmail.com, theo <at> thornhill.no, dancol <at> dancol.org
Subject: Re: bug#65234: [PATCH] Fix jsx font-lock in older tree-sitter-js
 grammars
Date: Thu, 17 Aug 2023 11:06:45 +0300

> Date: Wed, 16 Aug 2023 05:04:10 +0300
> Cc: 65234 <at> debbugs.gnu.org, v.pupillo <at> gmail.com, dancol <at> dancol.org
> From: Dmitry Gutov <dmitry <at> gutov.dev>
> 
> On 12/08/2023 08:41, Eli Zaretskii wrote:
> > Yuan, Theo: any comments on this?  Is this safe enough to go into the
> > emacs-29 branch?
> 
> It looks good to me (and works okay too). I also saw the same error in 
> my testing.
> 
> Especially since Emacs 29.2 is not quite imminent, I think it's a good 
> idea to install this: queries are simpler and thus more reliable. And 
> the fixed-fix, of course.

Thanks, installed on the emacs-29 branch, and closing the bug.

Previous Next

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