bug#67809: [PATCH] Add font-locking for assignments in typescript-ts-mode

bug#67809: [PATCH] Add font-locking for assignments in typescript-ts-mode

[Noah Peart]

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

Tags: patch


This patch adds fontification for the left-hand side of assignments in
typescript-ts-mode.

The object and/or property being assigned to is fontified with
`font-lock-variable-name-face` and/or `font-lock-property-name-face`,
while any other identifiers/properties on the left-hand side used in
index expressions are fontified with
`font-lock-variable-use-face`/`font-lock-property-use-face`.

For example, fontification is added in `typescript-ts-mode` for
identifiers in statements like the following:

    arr[obj.x * obj.x] = 1;
    //^ font-lock-variable-name-face
    //    ^ font-lock-variable-use-face
    //      ^ font-lock-property-use-face
    obj.x.y = 0;
    //^ font-lock-variable-name-face
    //  ^ font-lock-property-name-face
    //    ^ font-lock-property-name-face
    ++mat[x][arr[0]];
    // ^ font-lock-variable-name-face
    //        ^ font-lock-variable-use-face

In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.24.33, cairo version 1.16.0) of 2023-12-12 built on noah-X580VD
Repository revision: 75fd7550ed6cede6c9e8224f1f2d62637c43fdd4
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
System Description: Ubuntu 22.04.3 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: 1745 bytes --]

[-- Attachment #2: typescript-ts-fontify-assignments.patch --]
[-- Type: text/x-patch, Size: 7685 bytes --]

From 0b8a4ae66d1ddcc286cf1b7a54fb82894dcb0b9d Mon Sep 17 00:00:00 2001
From: Noah Peart <noah.v.peart@gmail.com>
Date: Wed, 13 Dec 2023 00:10:19 -0800
Subject: [PATCH] Add font-locking for assignments in typescript-ts-mode

* lisp/progmodes/typescript-ts-mode.el
(typescript-ts-mode--font-lock-settings): Add font-locking for
left-hand side of assignments.
(typescript-ts-mode--fontify-assignment-lhs): New function to
fontify typescript-ts-mode assignments.
(typescript-ts-mode--assignment-lhs-query): Add tree-sitter query
to capture assignment nodes for font-locking.
* test/lisp/progmodes/typescript-ts-mode-tests.el
(typescript-ts-mode-test-font-lock): New test for font-locking in
typescript-ts-mode.
* test/lisp/progmodes/typescript-ts-mode-resources/font-lock.ts:
New file with font-lock tests for typescript-ts-mode.
---
 lisp/progmodes/typescript-ts-mode.el          | 51 ++++++++++++++++++-
 .../typescript-ts-mode-resources/font-lock.ts | 50 ++++++++++++++++++
 .../progmodes/typescript-ts-mode-tests.el     |  6 +++
 3 files changed, 106 insertions(+), 1 deletion(-)
 create mode 100644 test/lisp/progmodes/typescript-ts-mode-resources/font-lock.ts

diff --git a/lisp/progmodes/typescript-ts-mode.el b/lisp/progmodes/typescript-ts-mode.el
index 7f0b7236301..8c3370ddc93 100644
--- a/lisp/progmodes/typescript-ts-mode.el
+++ b/lisp/progmodes/typescript-ts-mode.el
@@ -199,6 +199,38 @@ tsx-ts-mode--font-lock-compatibility-bb1f97b
 	      [(nested_identifier (identifier)) (identifier)]
 	      @typescript-ts-jsx-tag-face)))))
 
+(defvar typescript-ts-mode--assignment-lhs-query
+  (when (treesit-available-p)
+    (treesit-query-compile
+     'typescript
+     '((subscript_expression object: (identifier) @id)
+       (subscript_expression index: (identifier) @index)
+       (member_expression object: (identifier) @id)
+       (member_expression property: (property_identifier) @property))))
+  "Query that captures object, index, and property identifiers.")
+
+(defun typescript-ts-mode--fontify-assignment-lhs (node override start end &rest _)
+  "Fontify the lhs NODE of an assignment_expression.
+For OVERRIDE, START, END, see `treesit-font-lock-rules'."
+  ;; when INDEX > 1, apply `-*use-face' to identifiers/properties
+  (let ((index 0))
+    (pcase-dolist (`(,name . ,node)
+                   (treesit-query-capture
+                    node typescript-ts-mode--assignment-lhs-query))
+      (treesit-fontify-with-override
+       (treesit-node-start node) (treesit-node-end node)
+       (pcase name
+         ('id (prog1 (if (zerop index)
+                         'font-lock-variable-name-face
+                       'font-lock-variable-use-face)
+                (cl-incf index)))
+         ('property (if (= 1 index)
+                        'font-lock-property-name-face
+                      'font-lock-property-use-face))
+         ('index 'font-lock-variable-use-face)
+         (_ nil))
+       override start end))))
+
 (defun typescript-ts-mode--font-lock-settings (language)
   "Tree-sitter font-lock settings.
 Argument LANGUAGE is either `typescript' or `tsx'."
@@ -314,6 +346,8 @@ typescript-ts-mode--font-lock-settings
    :feature 'property
    `((property_signature
       name: (property_identifier) @font-lock-property-name-face)
+     (index_signature
+      name: (identifier) @font-lock-property-name-face)
      (public_field_definition
       name: (property_identifier) @font-lock-property-name-face)
 
@@ -353,6 +387,21 @@ typescript-ts-mode--font-lock-settings
    (append (tsx-ts-mode--font-lock-compatibility-bb1f97b language)
 	   `((jsx_attribute (property_identifier) @typescript-ts-jsx-attribute-face)))
 
+   :language language
+   :feature 'assignment
+   '((assignment_expression
+      left: (identifier) @font-lock-variable-name-face)
+     (assignment_expression
+      left: (_) @typescript-ts-mode--fontify-assignment-lhs)
+     (augmented_assignment_expression
+      left: (identifier) @font-lock-variable-name-face)
+     (augmented_assignment_expression
+      left: (_) @typescript-ts-mode--fontify-assignment-lhs)
+     (update_expression
+      argument: (identifier) @font-lock-variable-name-face)
+     (update_expression
+      argument: (_) @typescript-ts-mode--fontify-assignment-lhs))
+
    :language language
    :feature 'number
    `((number) @font-lock-number-face
@@ -486,7 +535,7 @@ typescript-ts-mode
                 '((comment declaration)
                   (keyword string escape-sequence)
                   (constant expression identifier number pattern property)
-                  (operator function bracket delimiter)))
+                  (assignment operator function bracket delimiter)))
     (setq-local syntax-propertize-function #'typescript-ts--syntax-propertize)
 
     (treesit-major-mode-setup)))
diff --git a/test/lisp/progmodes/typescript-ts-mode-resources/font-lock.ts b/test/lisp/progmodes/typescript-ts-mode-resources/font-lock.ts
new file mode 100644
index 00000000000..f9680886c57
--- /dev/null
+++ b/test/lisp/progmodes/typescript-ts-mode-resources/font-lock.ts
@@ -0,0 +1,50 @@
+// Test assignment left-hand side fontification
+const assignment = (): number => {
+  //  ^ font-lock-function-name-face
+  //                   ^ font-lock-type-face
+  let foo = 0;
+  //   ^ font-lock-variable-name-face
+  foo += 1;
+  //^ font-lock-variable-name-face
+  let obj: {[key: string]: any} = {};
+  //  ^ font-lock-variable-name-face
+  //          ^ font-lock-property-name-face
+  //                ^ font-lock-type-face
+  //                        ^ font-lock-type-face
+  obj.x.y = 0;
+  //^ font-lock-variable-name-face
+  //  ^ font-lock-property-name-face
+  //    ^ font-lock-property-name-face
+  ++obj.x++;
+  // ^ font-lock-variable-name-face
+  //    ^ font-lock-property-name-face
+  arr[obj.x * obj.x] = 1;
+  //^ font-lock-variable-name-face
+  //   ^ font-lock-variable-use-face
+  //      ^ font-lock-property-use-face
+  let x = 0, y = 0, arr = [0,0], mat = [[0],[0]];
+  [ x, y] = [y, x];
+  //^ font-lock-variable-name-face
+  //   ^ font-lock-variable-name-face
+  if ((x = 1)) x++;
+  //   ^ font-lock-variable-name-face
+  //           ^ font-lock-variable-name-face
+  arr[arr[0]] = 2;
+  //^ font-lock-variable-name-face
+  //   ^ font-lock-variable-use-face
+  //      ^ font-lock-number-face
+  ++mat[x][arr[0]];
+  // ^ font-lock-variable-name-face
+  //        ^ font-lock-variable-use-face
+  mat[mat[arr[arr[obj.x]]][x]][arr[0]] = 2;
+  //^ font-lock-variable-name-face
+  //   ^ font-lock-variable-use-face
+  //       ^ font-lock-variable-use-face
+  //           ^ font-lock-variable-use-face
+  //               ^ font-lock-variable-use-face
+  //                  ^ font-lock-property-use-face
+  //                       ^ font-lock-variable-use-face
+  //                            ^ font-lock-variable-use-face
+  //                               ^ font-lock-number-face
+  return 0;
+};
diff --git a/test/lisp/progmodes/typescript-ts-mode-tests.el b/test/lisp/progmodes/typescript-ts-mode-tests.el
index 126f5e3298f..d426c4e50eb 100644
--- a/test/lisp/progmodes/typescript-ts-mode-tests.el
+++ b/test/lisp/progmodes/typescript-ts-mode-tests.el
@@ -27,5 +27,11 @@ typescript-ts-mode-test-indentation
   (skip-unless (treesit-ready-p 'typescript))
   (ert-test-erts-file (ert-resource-file "indent.erts")))
 
+(ert-deftest typescript-ts-mode-test-font-lock ()
+  (skip-unless (treesit-ready-p 'typescript))
+  (let ((treesit-font-lock-level 4))
+    (ert-font-lock-test-file
+     (ert-resource-file "font-lock.ts") 'typescript-ts-mode)))
+
 (provide 'typescript-ts-mode-tests)
 ;;; typescript-ts-mode-tests.el ends here
-- 
2.34.1

bug#67809: [PATCH] Add font-locking for assignments in typescript-ts-mode

[Dmitry Gutov]

Hi!

On 13/12/2023 10:33, Noah Peart wrote:
>      arr[obj.x * obj.x] = 1;
>      //^ font-lock-variable-name-face
>      //    ^ font-lock-variable-use-face
>      //      ^ font-lock-property-use-face
>      obj.x.y = 0;
>      //^ font-lock-variable-name-face
>      //  ^ font-lock-property-name-face
>      //    ^ font-lock-property-name-face
>      ++mat[x][arr[0]];
>      // ^ font-lock-variable-name-face
>      //        ^ font-lock-variable-use-face

I think in all of these cases font-lock-variable-name-face should not be 
used, since arr, and obj, and mat, are all introduced (declared) at a 
different place.

font-lock-variable-use-face is more appropriate.

bug#67809: [PATCH] Add font-locking for assignments in typescript-ts-mode

[Noah Peart]

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

I thought `font-lock-variable-name-face` was standard for assignments?
It's what is applied
in ruby-ts-mode, python-ts-mode, and c-ts-mode for example.  I like it
personally, cause
it allows for visual distinction between l/r values.

Either way, I think the patch can be simplified to only highlight the
variable being assigned
and not any others on the left-hand side.  It would be simpler and more
customizable
to highlight any remaining variables in a `variable` feature in a following
rule.

I noticed another issue where I forgot to handle `this.var = ` cases as
well. I could take
another stab at it unless there's no interest in this feature.

On Wed, Dec 13, 2023 at 10:31 AM Dmitry Gutov <dmitry@gutov.dev> wrote:

> Hi!
>
> On 13/12/2023 10:33, Noah Peart wrote:
> >      arr[obj.x * obj.x] = 1;
> >      //^ font-lock-variable-name-face
> >      //    ^ font-lock-variable-use-face
> >      //      ^ font-lock-property-use-face
> >      obj.x.y = 0;
> >      //^ font-lock-variable-name-face
> >      //  ^ font-lock-property-name-face
> >      //    ^ font-lock-property-name-face
> >      ++mat[x][arr[0]];
> >      // ^ font-lock-variable-name-face
> >      //        ^ font-lock-variable-use-face
>
> I think in all of these cases font-lock-variable-name-face should not be
> used, since arr, and obj, and mat, are all introduced (declared) at a
> different place.
>
> font-lock-variable-use-face is more appropriate.
>

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

bug#67809: [PATCH] Add font-locking for assignments in typescript-ts-mode

[Dmitry Gutov]

On 13/12/2023 21:26, Noah Peart wrote:
> I thought `font-lock-variable-name-face` was standard for assignments?  
> It's what is applied
> in ruby-ts-mode, python-ts-mode, and c-ts-mode for example.  I like it 
> personally, cause
> it allows for visual distinction between l/r values.

ruby and python's parsers cannot distinguish between assignments that 
introduce a new variable (thus working as an implicit declaration) and 
those that reassign an existing variable. Hopefully, we'll be able to 
improve that in the future.

TypeScript, however, has explicit variable declarations.

> Either way, I think the patch can be simplified to only highlight the 
> variable being assigned
> and not any others on the left-hand side.  It would be simpler and more 
> customizable
> to highlight any remaining variables in a `variable` feature in a 
> following rule.

I'm also not sure I agree that, for example, 'arr' is the variable being 
assigned to in the first example. Its value (the reference) doesn't change.

But it's totally fine to add a 'variable' feature to 
typescript-ts-mode's font-lock to apply font-lock-variable-use-face to it.

> I noticed another issue where I forgot to handle `this.var = ` cases as 
> well. I could take
> another stab at it unless there's no interest in this feature.
> 
> On Wed, Dec 13, 2023 at 10:31 AM Dmitry Gutov <dmitry@gutov.dev 
> <mailto:dmitry@gutov.dev>> wrote:
> 
>     Hi!
> 
>     On 13/12/2023 10:33, Noah Peart wrote:
>      >      arr[obj.x * obj.x] = 1;
>      >      //^ font-lock-variable-name-face
>      >      //    ^ font-lock-variable-use-face
>      >      //      ^ font-lock-property-use-face
>      >      obj.x.y = 0;
>      >      //^ font-lock-variable-name-face
>      >      //  ^ font-lock-property-name-face
>      >      //    ^ font-lock-property-name-face
>      >      ++mat[x][arr[0]];
>      >      // ^ font-lock-variable-name-face
>      >      //        ^ font-lock-variable-use-face
> 
>     I think in all of these cases font-lock-variable-name-face should
>     not be
>     used, since arr, and obj, and mat, are all introduced (declared) at a
>     different place.
> 
>     font-lock-variable-use-face is more appropriate.
> 

bug#67809: [PATCH] Add font-locking for assignments in typescript-ts-mode

[Eli Zaretskii]

> Cc: 67809@debbugs.gnu.org
> Date: Wed, 13 Dec 2023 22:45:51 +0200
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 13/12/2023 21:26, Noah Peart wrote:
> > I thought `font-lock-variable-name-face` was standard for assignments?  
> > It's what is applied
> > in ruby-ts-mode, python-ts-mode, and c-ts-mode for example.  I like it 
> > personally, cause
> > it allows for visual distinction between l/r values.
> 
> ruby and python's parsers cannot distinguish between assignments that 
> introduce a new variable (thus working as an implicit declaration) and 
> those that reassign an existing variable. Hopefully, we'll be able to 
> improve that in the future.
> 
> TypeScript, however, has explicit variable declarations.
> 
> > Either way, I think the patch can be simplified to only highlight the 
> > variable being assigned
> > and not any others on the left-hand side.  It would be simpler and more 
> > customizable
> > to highlight any remaining variables in a `variable` feature in a 
> > following rule.
> 
> I'm also not sure I agree that, for example, 'arr' is the variable being 
> assigned to in the first example. Its value (the reference) doesn't change.
> 
> But it's totally fine to add a 'variable' feature to 
> typescript-ts-mode's font-lock to apply font-lock-variable-use-face to it.

What is the conclusion here?  Do we have an agreed-upon patch to
install?

Thanks.

bug#67809: [PATCH] Add font-locking for assignments in typescript-ts-mode

[Noah Peart]

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

I think this should be closed for now.

On Sat, Dec 23, 2023 at 3:45 AM Eli Zaretskii <eliz@gnu.org> wrote:

> > Cc: 67809@debbugs.gnu.org
> > Date: Wed, 13 Dec 2023 22:45:51 +0200
> > From: Dmitry Gutov <dmitry@gutov.dev>
> >
> > On 13/12/2023 21:26, Noah Peart wrote:
> > > I thought `font-lock-variable-name-face` was standard for
> assignments?
> > > It's what is applied
> > > in ruby-ts-mode, python-ts-mode, and c-ts-mode for example.  I like it
> > > personally, cause
> > > it allows for visual distinction between l/r values.
> >
> > ruby and python's parsers cannot distinguish between assignments that
> > introduce a new variable (thus working as an implicit declaration) and
> > those that reassign an existing variable. Hopefully, we'll be able to
> > improve that in the future.
> >
> > TypeScript, however, has explicit variable declarations.
> >
> > > Either way, I think the patch can be simplified to only highlight the
> > > variable being assigned
> > > and not any others on the left-hand side.  It would be simpler and
> more
> > > customizable
> > > to highlight any remaining variables in a `variable` feature in a
> > > following rule.
> >
> > I'm also not sure I agree that, for example, 'arr' is the variable being
> > assigned to in the first example. Its value (the reference) doesn't
> change.
> >
> > But it's totally fine to add a 'variable' feature to
> > typescript-ts-mode's font-lock to apply font-lock-variable-use-face to
> it.
>
> What is the conclusion here?  Do we have an agreed-upon patch to
> install?
>
> Thanks.
>

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

bug#67809: [PATCH] Add font-locking for assignments in typescript-ts-mode

[Eli Zaretskii]

> From: Noah Peart <noah.v.peart@gmail.com>
> Date: Sat, 23 Dec 2023 05:39:17 -0500
> Cc: Dmitry Gutov <dmitry@gutov.dev>, 67809@debbugs.gnu.org
> 
> I think this should be closed for now.

If Dmitry agrees, it's fine by me.

bug#67809: [PATCH] Add font-locking for assignments in typescript-ts-mode

[Dmitry Gutov]

On 23/12/2023 13:21, Eli Zaretskii wrote:
>> From: Noah Peart<noah.v.peart@gmail.com>
>> Date: Sat, 23 Dec 2023 05:39:17 -0500
>> Cc: Dmitry Gutov<dmitry@gutov.dev>,67809@debbugs.gnu.org
>>
>> I think this should be closed for now.
> If Dmitry agrees, it's fine by me.

Closing. I'm sorry if this was a disappointing response.