bug#70361: [PATCH] Add font-locking for operators in go-ts-mode.

bug#70361: [PATCH] Add font-locking for operators in go-ts-mode.

[Noah Peart]

[-- Attachment #1.1: Type: text/plain, Size: 854 bytes --]

Tags: patch


Adds font-locking for Go operators in `go-ts-mode'. The operator feature
is already added to `treesit-font-lock-feature-list' in `go-ts-mode` - this
just adds the missing rule to `go-ts-mode--font-lock-settings'.


In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.24.33, cairo version 1.16.0) of 2024-04-07 built on noah-X580VD
Repository revision: ee377aaddf9d73116b93c3d54b1ffef9a3a8b925
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
System Description: Ubuntu 22.04.4 LTS

Configured using:
 'configure --prefix=/usr/local --with-modules --with-tree-sitter
--with-threads --with-x-toolkit=gtk3 --with-xwidgets --with-gnutls
--with-json --with-mailutils --with-jpeg --with-png --with-rsvg
--with-tiff --with-xml2 --with-xpm --with-imagemagick CC=gcc-12
CXX=gcc-12'

[-- Attachment #1.2: Type: text/html, Size: 969 bytes --]

[-- Attachment #2: 0001-Add-font-locking-for-operators-in-go-ts-mode.patch --]
[-- Type: text/x-patch, Size: 888 bytes --]

From fd393a6259e5f3479a9f244764ef26a796505e28 Mon Sep 17 00:00:00 2001
From: Noah Peart <noah.v.peart@gmail.com>
Date: Fri, 12 Apr 2024 22:09:17 -0700
Subject: [PATCH] Add font-locking for operators in go-ts-mode.

* lisp/progmodes/go-ts-mode.el (go-ts-mode--font-lock-settings): Add
font-locking for Go operators.
---
 lisp/progmodes/go-ts-mode.el | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lisp/progmodes/go-ts-mode.el b/lisp/progmodes/go-ts-mode.el
index cc330688dc3..7ca3bfa5e65 100644
--- a/lisp/progmodes/go-ts-mode.el
+++ b/lisp/progmodes/go-ts-mode.el
@@ -135,6 +135,10 @@ go-ts-mode--font-lock-settings
    :feature 'delimiter
    '((["," "." ";" ":"]) @font-lock-delimiter-face)
 
+   :language 'go
+   :feature 'operator
+   `([,@go-ts-mode--operators] @font-lock-operator-face)
+
    :language 'go
    :feature 'definition
    '((function_declaration
-- 
2.34.1

bug#70361: [PATCH] Add font-locking for operators in go-ts-mode.

[Eli Zaretskii]

> From: Noah Peart <noah.v.peart@gmail.com>
> Date: Fri, 12 Apr 2024 22:23:05 -0700
> 
> Adds font-locking for Go operators in `go-ts-mode'. The operator feature
> is already added to `treesit-font-lock-feature-list' in `go-ts-mode` - this
> just adds the missing rule to `go-ts-mode--font-lock-settings'.

Thanks.

The operators will be fontified only if treesit-font-lock-level is 4,
right?  That's the level at which we decided to fontify operators.

bug#70361: [PATCH] Add font-locking for operators in go-ts-mode.

[Noah Peart]

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

> The operators will be fontified only if treesit-font-lock-level is 4,
right?

Yea, operator is at level 4 - I didn't change the
treesit-font-lock-feature-list.



On Sat, Apr 13, 2024 at 12:20 AM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Noah Peart <noah.v.peart@gmail.com>
> > Date: Fri, 12 Apr 2024 22:23:05 -0700
> >
> > Adds font-locking for Go operators in `go-ts-mode'. The operator feature
> > is already added to `treesit-font-lock-feature-list' in `go-ts-mode` -
> this
> > just adds the missing rule to `go-ts-mode--font-lock-settings'.
>
> Thanks.
>
> The operators will be fontified only if treesit-font-lock-level is 4,
> right?  That's the level at which we decided to fontify operators.
>

[-- Attachment #2: Type: text/html, Size: 1180 bytes --]

bug#70361: [PATCH] Add font-locking for operators in go-ts-mode.

[Noah Peart]

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

On a somewhat related note, I was wondering why the treesit modes in emacs
define their `treesit-font-lock-feature-list`s in the mode definitions.

Wouldn't it be more user-friendly to `defvar` the feature list?

On Sat, Apr 13, 2024 at 12:27 AM Noah Peart <noah.v.peart@gmail.com> wrote:

> > The operators will be fontified only if treesit-font-lock-level is 4,
> right?
>
> Yea, operator is at level 4 - I didn't change the
> treesit-font-lock-feature-list.
>
>
>
> On Sat, Apr 13, 2024 at 12:20 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
>> > From: Noah Peart <noah.v.peart@gmail.com>
>> > Date: Fri, 12 Apr 2024 22:23:05 -0700
>> >
>> > Adds font-locking for Go operators in `go-ts-mode'. The operator feature
>> > is already added to `treesit-font-lock-feature-list' in `go-ts-mode` -
>> this
>> > just adds the missing rule to `go-ts-mode--font-lock-settings'.
>>
>> Thanks.
>>
>> The operators will be fontified only if treesit-font-lock-level is 4,
>> right?  That's the level at which we decided to fontify operators.
>>
>

[-- Attachment #2: Type: text/html, Size: 1809 bytes --]

bug#70361: [PATCH] Add font-locking for operators in go-ts-mode.

[Eli Zaretskii]

> From: Noah Peart <noah.v.peart@gmail.com>
> Date: Sat, 13 Apr 2024 00:32:59 -0700
> Cc: 70361@debbugs.gnu.org
> 
> On a somewhat related note, I was wondering why the treesit modes in emacs
> define their `treesit-font-lock-feature-list`s in the mode definitions.
> 
> Wouldn't it be more user-friendly to `defvar` the feature list?

AFAIR, we do that in the mode's settings because the translation of
general categories into mode-specific settings is not easy, and
because we want users to control that via the fontification level, not
below that.

What problems do you see with the current approach that would require
more fine-grained user control?  And why do you think a defvar is the
proper way of providing such control?

bug#70361: [PATCH] Add font-locking for operators in go-ts-mode.

[Noah Peart]

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

> What problems do you see with the current approach that would require
more fine-grained user control?

The main issue for me is removing the `error` feature from any mode
that adds it with `:override t`. I find the override font-locking is
jarring - in some
Languages half the buffer can switch in and out of parse errors when you
do something as simple as removing a closing paren.

Also, allowing users to rearrange features at different levels seems like a
bonus.

> And why do you think a defvar is the proper way of providing such control?

You're right, I don't think it's the best way - it's just what I've been
doing in
treesit modes I've written till now.

On Sat, Apr 13, 2024 at 12:40 AM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Noah Peart <noah.v.peart@gmail.com>
> > Date: Sat, 13 Apr 2024 00:32:59 -0700
> > Cc: 70361@debbugs.gnu.org
> >
> > On a somewhat related note, I was wondering why the treesit modes in
> emacs
> > define their `treesit-font-lock-feature-list`s in the mode definitions.
> >
> > Wouldn't it be more user-friendly to `defvar` the feature list?
>
> AFAIR, we do that in the mode's settings because the translation of
> general categories into mode-specific settings is not easy, and
> because we want users to control that via the fontification level, not
> below that.
>
> What problems do you see with the current approach that would require
> more fine-grained user control?  And why do you think a defvar is the
> proper way of providing such control?
>

[-- Attachment #2: Type: text/html, Size: 2155 bytes --]

bug#70361: [PATCH] Add font-locking for operators in go-ts-mode.

[Eli Zaretskii]

> From: Noah Peart <noah.v.peart@gmail.com>
> Date: Sat, 13 Apr 2024 00:53:45 -0700
> Cc: Yuan Fu <casouri@gmail.com>, 70361@debbugs.gnu.org
> 
> > What problems do you see with the current approach that would require
> more fine-grained user control?
> 
> The main issue for me is removing the `error` feature from any mode
> that adds it with `:override t`. I find the override font-locking is jarring - in some
> Languages half the buffer can switch in and out of parse errors when you
> do something as simple as removing a closing paren.

Maybe we should have a user option to turn error feature on and off.
Yuan, WDYT?

> Also, allowing users to rearrange features at different levels seems like a bonus.

Not sure about this one.  We didn't have that in the traditional
font-lock, did we?

bug#70361: [PATCH] Add font-locking for operators in go-ts-mode.

[Yuan Fu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Noah Peart <noah.v.peart@gmail.com>
>> Date: Sat, 13 Apr 2024 00:53:45 -0700
>> Cc: Yuan Fu <casouri@gmail.com>, 70361@debbugs.gnu.org
>> 
>> > What problems do you see with the current approach that would require
>> more fine-grained user control?
>> 
>> The main issue for me is removing the `error` feature from any mode
>> that adds it with `:override t`. I find the override font-locking is jarring - in some
>> Languages half the buffer can switch in and out of parse errors when you
>> do something as simple as removing a closing paren.
>
> Maybe we should have a user option to turn error feature on and off.
> Yuan, WDYT?

Sure, I’m not a big advocate for highlighting the errors to begin with.
The tree-sitter parse errors aren’t meant to be used for highlighting
anyway. This is just something some major mode provides because it’s a
nice bonus, and it’s effectiveness probably varies by language grammar.

The user can turn off error feature with
treesit-font-lock-recompute-features, like they do for any other
features. Do we want to add a user option in addition? In any case, even
if we add such user option, it should be added at the major mode level,
since the error feature is at major mode level, it isn’t something
provided uniformly across all tree-sitter modes.

>> Also, allowing users to rearrange features at different levels seems like a bonus.
>
> Not sure about this one.  We didn't have that in the traditional
> font-lock, did we?

If major modes define a xxx-mode-feature-list, sure, the user can
redefine that list to their liking. Again that’s a major mode level
decision.

I CC’ed Randy since he’s the maintainer of go-ts-mode.

Yuan

bug#70361: [PATCH] Add font-locking for operators in go-ts-mode.

[Yuan Fu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Noah Peart <noah.v.peart@gmail.com>
>> Date: Sat, 13 Apr 2024 00:32:59 -0700
>> Cc: 70361@debbugs.gnu.org
>> 
>> On a somewhat related note, I was wondering why the treesit modes in emacs
>> define their `treesit-font-lock-feature-list`s in the mode definitions.
>> 
>> Wouldn't it be more user-friendly to `defvar` the feature list?
>
> AFAIR, we do that in the mode's settings because the translation of
> general categories into mode-specific settings is not easy, and
> because we want users to control that via the fontification level, not
> below that.
>
> What problems do you see with the current approach that would require
> more fine-grained user control?  And why do you think a defvar is the
> proper way of providing such control?

Replying to Noah, I don’t think there’s anything wrong for a major mode
to define a xxx-mode-feature-list and assign it to
treesit-font-lock-feature-list when setting up the major mode. We just
don’t want a central tree-sitter-font-lock-feature-list that controls
the feature list of every major mode in a single place.

But again, even if a xxx-mode defines xxx-mode-feature-list, it would be
a bit of a hassle to take out or remove a single feature from the list,
since it’s a list of list. (You can just setq the whole list, but then
you need to keep it in sync with upstream.) It’s probably easier to call
treesit-font-lock-recompute-features in the major mode hook anyway.

Yuan

bug#70361: [PATCH] Add font-locking for operators in go-ts-mode.

[Noah Peart]

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

Thankyou, this all makes sense.  I do think it would be nice if it was
easier to somehow
add user customizations to the font-locking, especially since it is so easy
to write tree-sitter
font-locking rules.

> We just don’t want a central tree-sitter-font-lock-feature-list that
controls
the feature list of every major mode in a single place.

What would you think of something like a `user` feature that could be
enabled
globally and users could add their mode-specific rules there? Eg.

    (defvar my-treesit-font-lock-rules
      (treesit-font-lock-rules
       :language 'go
       :feature 'user
       '((identifier))

       :language 'typescript
       :feature 'user
       '((identifier))
       ;; ...
       ))

This wouldn't work very well in certain cases where ordering of the rules
was important, however.

On Sun, Apr 14, 2024 at 4:34 PM Yuan Fu <casouri@gmail.com> wrote:

>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> From: Noah Peart <noah.v.peart@gmail.com>
> >> Date: Sat, 13 Apr 2024 00:32:59 -0700
> >> Cc: 70361@debbugs.gnu.org
> >>
> >> On a somewhat related note, I was wondering why the treesit modes in
> emacs
> >> define their `treesit-font-lock-feature-list`s in the mode definitions.
> >>
> >> Wouldn't it be more user-friendly to `defvar` the feature list?
> >
> > AFAIR, we do that in the mode's settings because the translation of
> > general categories into mode-specific settings is not easy, and
> > because we want users to control that via the fontification level, not
> > below that.
> >
> > What problems do you see with the current approach that would require
> > more fine-grained user control?  And why do you think a defvar is the
> > proper way of providing such control?
>
> Replying to Noah, I don’t think there’s anything wrong for a major mode
> to define a xxx-mode-feature-list and assign it to
> treesit-font-lock-feature-list when setting up the major mode. We just
> don’t want a central tree-sitter-font-lock-feature-list that controls
> the feature list of every major mode in a single place.
>
> But again, even if a xxx-mode defines xxx-mode-feature-list, it would be
> a bit of a hassle to take out or remove a single feature from the list,
> since it’s a list of list. (You can just setq the whole list, but then
> you need to keep it in sync with upstream.) It’s probably easier to call
> treesit-font-lock-recompute-features in the major mode hook anyway.
>
> Yuan
>

[-- Attachment #2: Type: text/html, Size: 3348 bytes --]

bug#70361: [PATCH] Add font-locking for operators in go-ts-mode.

[Randy Taylor]

On Saturday, April 13th, 2024 at 01:23, Noah Peart <noah.v.peart@gmail.com> wrote:
> Tags: patch
> 
> 
> Adds font-locking for Go operators in `go-ts-mode'. The operator feature
> is already added to `treesit-font-lock-feature-list' in `go-ts-mode` - this
> just adds the missing rule to `go-ts-mode--font-lock-settings'.

Thanks for working on this.

It looks good to me, would someone be able to install it? Thanks in advance.

bug#70361: [PATCH] Add font-locking for operators in go-ts-mode.

[Yuan Fu]

Noah Peart <noah.v.peart@gmail.com> writes:

> Thankyou, this all makes sense.  I do think it would be nice if it was easier to somehow 
> add user customizations to the font-locking, especially since it is so easy to write tree-sitter
> font-locking rules.

Right. We (well, I) were focusing on support for writing major modes and
didn’t give enough attention to the customization aspect, especially for
beginners.

>> We just don’t want a central tree-sitter-font-lock-feature-list that controls
> the feature list of every major mode in a single place.
>
> What would you think of something like a `user` feature that could be enabled
> globally and users could add their mode-specific rules there? Eg.
>
>     (defvar my-treesit-font-lock-rules
>       (treesit-font-lock-rules
>        :language 'go
>        :feature 'user
>        '((identifier))
>
>        :language 'typescript
>        :feature 'user
>        '((identifier))
>        ;; ...
>        ))
>
> This wouldn't work very well in certain cases where ordering of the rules
> was important, however.

I think that’s a good start. I like that it’s tied to each language
rather than a major mode, so it avoids problems with derived major
modes.

Indentation can also use some easy customization options.

On a separate note, something like this might be better off in a ELPA
package, such that it has more freedom to evolve and break things.
Things that are added to Emacs core tend to fossilize, or at least
become very hard to change. (To be precise, I mean the interface tend to
be hard to change, implementation details can and do change all the
time.)

I’d say that order wouldn’t be a problem since I imagine most of
tht time, the user wants their fontification override all else.

Yuan

bug#70361: [PATCH] Add font-locking for operators in go-ts-mode.

[Yuan Fu]

Randy Taylor <dev@rjt.dev> writes:

> On Saturday, April 13th, 2024 at 01:23, Noah Peart <noah.v.peart@gmail.com> wrote:
>> Tags: patch
>> 
>> 
>> Adds font-locking for Go operators in `go-ts-mode'. The operator feature
>> is already added to `treesit-font-lock-feature-list' in `go-ts-mode` - this
>> just adds the missing rule to `go-ts-mode--font-lock-settings'.
>
> Thanks for working on this.
>
> It looks good to me, would someone be able to install it? Thanks in advance.

Done.

Yuan

bug#70361: [PATCH] Add font-locking for operators in go-ts-mode.

[Eli Zaretskii]

> Cc: 70361@debbugs.gnu.org, noah.v.peart@gmail.com
> From: Yuan Fu <casouri@gmail.com>
> Date: Mon, 15 Apr 2024 22:58:39 -0700
> 
> 
> Randy Taylor <dev@rjt.dev> writes:
> 
> > Thanks for working on this.
> >
> > It looks good to me, would someone be able to install it? Thanks in advance.
> 
> Done.

Thanks.  Should this bug be closed now?

bug#70361: [PATCH] Add font-locking for operators in go-ts-mode.

[Yuan Fu]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 70361@debbugs.gnu.org, noah.v.peart@gmail.com
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Mon, 15 Apr 2024 22:58:39 -0700
>> 
>> 
>> Randy Taylor <dev@rjt.dev> writes:
>> 
>> > Thanks for working on this.
>> >
>> > It looks good to me, would someone be able to install it? Thanks in advance.
>> 
>> Done.
>
> Thanks.  Should this bug be closed now?

Yeah we can continue the discussion but close the bug.

Yuan