bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently

[Wilhelm Kirschbaum <wkirschbaum@gmail.com>]

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


Dmitry Gutov <dmitry@gutov.dev> writes:

> On 25/11/2023 10:33, Andrey Listopadov wrote:
>
>>> And here's another aspect: the default built-in theme doesn't
>>> distinguish many of the faces (and the same is true for many 
>>> other
>>> built-in themes). E.g. it doesn't distinguish 
>>> variable-name-face from
>>> variable-use-face or function-name-face from 
>>> function-call-face.
>> I'm wondering if font-lock.el needs a bit more generic faces, 
>> as
>> packages often define their own faces, that aren't supported by 
>> themes
>> in any way.  Again, the example with elixir-mode isn't to bash 
>> the
>> developers, but before 2019 elixir-mode (not elixir-ts-mode) 
>> defined a
>> few faces with explicit colors.  Here's a commit that fixed 
>> that
>> https://github.com/elixir-editors/emacs-elixir/commit/f101c676cc9485aa22ec088a71d8afc72cda3d58
>> but before it, `elixir-atom-face' and `elixir-attribute-face' 
>> were
>> `RoyalBlue4' and `MediumPurple4' no matter what theme you were 
>> using.
>> IIRC the CIDER package also defines some faces like that, so 
>> it's
>> somewhat common.
>
> As long as the faces are for unusual contexts and have some 
> fallbacks
> (or preferably inherit from some of the core ones), that's fair
> practice.
>
>> I can't come up with missing faces, and most modes I use define 
>> extra
>> faces in terms of inheritance to the inbuilt faces,
>
> Right.
>
>> but maybe
>> font-lock-symbol-face is worth including, as some languages may 
>> want to
>> distinguish these like elixir does right now with 
>> `elixir-ts-atom-face'.
>
> I agree we could add more. E.g. a face like that could 
> automatically
> be used for "keywords" in Elisp (and Clojure, and other Lisps) 
> and
> "symbols" in Elixir in Ruby.
>
> What makes me pause is naming: the terminology is a mess here 
> across
> languages. "symbols" usually mean something else in Emacs (and 
> in Lisp
> languages in general), whereas "keywords" mean something else 
> across
> most other languages. Using the name font-lock-symbol-face is 
> bound to
> cause confusion at least across Lisp programmers. Luckily,
> 'font-lock-keyword-face' is already taken, so we don't have to
> consider this alternative (which would puzzle the rest of the
> programming world).
>
> The docstring of 'font-lock-constant-face' says "Face name to 
> use for
> constant and label names", but a name 'font-lock-label-name' 
> sounds
> pretty bland... OTOH, there are labels in C, but nothing with 
> that
> particular name in Elixir, Ruby or Lisp (aside from one macro, I
> suppose).


Here is a patch to address numerous issues flagged on Elixir 
slack,
Github and in this thread.  It will not be perfect, but since the
changes are pretty large I want to get this in and then we can 
pick on
specific issues afterwards if that makes sense?

I am making the assumption that it is okay to rename custom faces 
as
elixir-ts-mode is only for 30.

One thing I tried to get right is to ensure that each level works
relatively well, which means a bit more brute forcing queries.  I 
have
not seen a major performance issue on massic Elixir files, so 
think its
fine.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Various-improvements-to-font-lock-settings-for-elixi.patch --]
[-- Type: text/x-patch, Size: 18119 bytes --]

From ef3e6b3106cabdcaa000503a9b2c227110be36f3 Mon Sep 17 00:00:00 2001
From: Wilhelm H Kirschbaum <wkirschbaum@gmail.com>
Date: Wed, 15 Nov 2023 20:41:08 +0200
Subject: [PATCH] Various improvements to font-lock-settings for elixir-ts-mode

Changes and made from conversations from the Elixir slack channel,
the github issue
https://github.com/wkirschbaum/elixir-ts-mode/issues/35 and bug#67246.

* lisp/progmodes/elixir-ts-mode.el
(elixir-ts--font-lock-settings): Update features.
(elixir-ts-mode): Update treesit-font-lock-feature-list.
(elixir-ts-font-comment-doc-identifier-face): Rename to
elixir-ts-comment-doc-identifier.
(elixir-ts-font-comment-doc-attribute-face): Rename to
elixir-ts-comment-doc-attribute.
(elixir-ts-font-sigil-name-face): Rename to elixir-ts-sigil-name.
(elixir-ts-atom-key-face)
(elixir-ts-keyword-key-face)
(elixir-ts-attribute-face): Add new custom face.
---
 lisp/progmodes/elixir-ts-mode.el | 323 ++++++++++++++++++-------------
 1 file changed, 191 insertions(+), 132 deletions(-)

diff --git a/lisp/progmodes/elixir-ts-mode.el b/lisp/progmodes/elixir-ts-mode.el
index c687ed9d06b..62429308d96 100644
--- a/lisp/progmodes/elixir-ts-mode.el
+++ b/lisp/progmodes/elixir-ts-mode.el
@@ -86,17 +86,35 @@ elixir-ts-mode-hook
   :group 'elixir-ts
   :version "30.1")
 
-(defface elixir-ts-font-comment-doc-identifier-face
+(defface elixir-ts-comment-doc-identifier
   '((t (:inherit font-lock-doc-face)))
-  "Face used for @comment.doc tags in Elixir files.")
+  "Face used for doc identifiers in Elixir files."
+  :group 'elixir-ts)
 
-(defface elixir-ts-font-comment-doc-attribute-face
+(defface elixir-ts-comment-doc-attribute
   '((t (:inherit font-lock-doc-face)))
-  "Face used for @comment.doc.__attribute__ tags in Elixir files.")
+  "Face used for doc attributes in Elixir files."
+  :group 'elixir-ts)
 
-(defface elixir-ts-font-sigil-name-face
+(defface elixir-ts-sigil-name
   '((t (:inherit font-lock-string-face)))
-  "Face used for @__name__ tags in Elixir files.")
+  "Face used for sigils in Elixir files."
+  :group 'elixir-ts)
+
+(defface elixir-ts-atom
+  '((t (:inherit font-lock-constant-face)))
+  "Face used for atoms in Elixir files."
+  :group 'elixir-ts)
+
+(defface elixir-ts-keyword-key
+  '((t (:inherit elixir-ts-atom)))
+  "Face used for keyword keys in Elixir files."
+  :group 'elixir-ts)
+
+(defface elixir-ts-attribute
+  '((t (:inherit font-lock-preprocessor-face)))
+  "Face used for attributes in Elixir files."
+  :group 'elixir-ts)
 
 (defconst elixir-ts--sexp-regexp
   (rx bol
@@ -114,7 +132,10 @@ elixir-ts--definition-keywords
     "defoverridable" "defp" "defprotocol" "defstruct"))
 
 (defconst elixir-ts--definition-keywords-re
-  (concat "^" (regexp-opt elixir-ts--definition-keywords) "$"))
+  (concat "^" (regexp-opt
+               (append elixir-ts--definition-keywords
+                       elixir-ts--test-definition-keywords))
+          "$"))
 
 (defconst elixir-ts--kernel-keywords
   '("alias" "case" "cond" "else" "for" "if" "import" "quote"
@@ -334,56 +355,73 @@ elixir-ts--indent-rules
                   (treesit-node-start
                    (treesit-node-parent
                     (treesit-node-at (point) 'elixir))))
-                  0)))))
+                0)))))
 
 (defvar elixir-ts--font-lock-settings
   (treesit-font-lock-rules
    :language 'elixir
-   :feature 'elixir-comment
-   '((comment) @font-lock-comment-face)
-
-   :language 'elixir
-   :feature 'elixir-string
-   :override t
-   '([(string) (charlist)] @font-lock-string-face)
-
+   :feature 'elixir-function-name
+   `((call target: (identifier) @target-identifier
+           (arguments (identifier) @font-lock-function-name-face)
+           (:match ,elixir-ts--definition-keywords-re @target-identifier))
+     (call target: (identifier) @target-identifier
+           (arguments
+            (call target: (identifier) @font-lock-function-name-face))
+           (:match ,elixir-ts--definition-keywords-re @target-identifier))
+     (call target: (identifier) @target-identifier
+           (arguments
+            (binary_operator
+             left: (call target: (identifier) @font-lock-function-name-face)))
+           (:match ,elixir-ts--definition-keywords-re @target-identifier))
+     (call target: (identifier) @target-identifier
+           (arguments (identifier) @font-lock-function-name-face)
+           (do_block)
+           (:match ,elixir-ts--definition-keywords-re @target-identifier))
+     (call target: (identifier) @target-identifier
+           (arguments
+            (call target: (identifier) @font-lock-function-name-face))
+           (do_block)
+           (:match ,elixir-ts--definition-keywords-re @target-identifier))
+     (call target: (identifier) @target-identifier
+           (arguments
+            (binary_operator
+             left: (call target: (identifier) @font-lock-function-name-face)))
+           (do_block)
+           (:match ,elixir-ts--definition-keywords-re @target-identifier))
+     (unary_operator
+      operator: "@"
+      (call (arguments
+             (binary_operator
+              left: (call target: (identifier) @font-lock-function-name-face))))))
+
+   ;; A function definition like "def _foo" is valid, but we should
+   ;; not apply the comment-face unless its a non-function identifier, so
+   ;; the comment matches has to be after the function matches.
    :language 'elixir
-   :feature 'elixir-string-interpolation
-   :override t
-   '((string
-      [
-       quoted_end: _ @font-lock-string-face
-       quoted_start: _ @font-lock-string-face
-       (quoted_content) @font-lock-string-face
-       (interpolation
-        "#{" @font-lock-regexp-grouping-backslash "}"
-        @font-lock-regexp-grouping-backslash)
-       ])
-     (charlist
-      [
-       quoted_end: _ @font-lock-string-face
-       quoted_start: _ @font-lock-string-face
-       (quoted_content) @font-lock-string-face
-       (interpolation
-        "#{" @font-lock-regexp-grouping-backslash "}"
-        @font-lock-regexp-grouping-backslash)
-       ]))
+   :feature 'elixir-comment
+   '((comment) @font-lock-comment-face
+     ((identifier) @font-lock-comment-face
+      (:match "^_[a-z]\\|^_$" @font-lock-comment-face)))
 
    :language 'elixir
-   :feature 'elixir-keyword
-   `(,elixir-ts--reserved-keywords-vector
-     @font-lock-keyword-face
-     (binary_operator
-      operator: _ @font-lock-keyword-face
-      (:match ,elixir-ts--reserved-keywords-re @font-lock-keyword-face)))
+   :feature 'elixir-variable
+   `((call target: (identifier)
+           (arguments
+            (binary_operator
+             (call target: (identifier)
+                   (arguments ((identifier) @font-lock-variable-use-face))))))
+     (call target: (identifier)
+           (arguments
+            (call target: (identifier)
+                  (arguments ((identifier)) @font-lock-variable-use-face))))
+     (dot left: (identifier) @font-lock-variable-use-face operator: "." ))
 
    :language 'elixir
    :feature 'elixir-doc
-   :override t
    `((unary_operator
-      operator: "@" @elixir-ts-font-comment-doc-attribute-face
+      operator: "@" @elixir-ts-comment-doc-attribute
       operand: (call
-                target: (identifier) @elixir-ts-font-comment-doc-identifier-face
+                target: (identifier) @elixir-ts-comment-doc-identifier
                 ;; Arguments can be optional, so adding another
                 ;; entry without arguments.
                 ;; If we don't handle then we don't apply font
@@ -395,109 +433,128 @@ elixir-ts--font-lock-settings
                   (charlist) @font-lock-doc-face
                   (sigil) @font-lock-doc-face
                   (boolean) @font-lock-doc-face
+                  (keywords) @font-lock-doc-face
                   ]))
       (:match ,elixir-ts--doc-keywords-re
-              @elixir-ts-font-comment-doc-identifier-face))
+              @elixir-ts-comment-doc-identifier))
      (unary_operator
-      operator: "@" @elixir-ts-font-comment-doc-attribute-face
+      operator: "@" @elixir-ts-comment-doc-attribute
       operand: (call
-                target: (identifier) @elixir-ts-font-comment-doc-identifier-face)
+                target: (identifier) @elixir-ts-comment-doc-identifier)
       (:match ,elixir-ts--doc-keywords-re
-              @elixir-ts-font-comment-doc-identifier-face)))
+              @elixir-ts-comment-doc-identifier)))
 
    :language 'elixir
-   :feature 'elixir-unary-operator
-   `((unary_operator operator: "@" @font-lock-preprocessor-face
-                     operand: [
-                               (identifier)  @font-lock-preprocessor-face
-                               (call target: (identifier)
-                                     @font-lock-preprocessor-face)
-                               (boolean)  @font-lock-preprocessor-face
-                               (nil)  @font-lock-preprocessor-face
-                               ])
+   :feature 'elixir-string
+   '((interpolation
+      "#{" @font-lock-escape-face
+      "}" @font-lock-escape-face)
+     (string (quoted_content) @font-lock-string-face)
+     (quoted_keyword (quoted_content) @font-lock-string-face)
+     (charlist (quoted_content) @font-lock-string-face)
+     ["\"" "'" "\"\"\""] @font-lock-string-face)
 
-     (unary_operator operator: "&") @font-lock-function-name-face
-     (operator_identifier) @font-lock-operator-face)
+   :language 'elixir
+   :feature 'elixir-sigil
+   `((sigil
+      (sigil_name) @elixir-ts-sigil-name
+      (quoted_content) @font-lock-string-face
+      ;; HEEx and Surface templates will handled by
+      ;; heex-ts-mode if its available.
+      (:match "^[^HF]$" @elixir-ts-sigil-name))
+     @font-lock-string-face
+     (sigil
+      (sigil_name) @font-lock-regexp-face
+      (:match "^[rR]$" @font-lock-regexp-face))
+     @font-lock-regexp-face
+     (sigil
+      "~" @font-lock-string-face
+      (sigil_name) @font-lock-string-face
+      quoted_start: _ @font-lock-string-face
+      quoted_end: _ @font-lock-string-face))
 
    :language 'elixir
    :feature 'elixir-operator
-   '((binary_operator operator: _ @font-lock-operator-face)
-     (dot operator: _ @font-lock-operator-face)
-     (stab_clause operator: _ @font-lock-operator-face)
-
-     [(boolean) (nil)] @font-lock-constant-face
-     [(integer) (float)] @font-lock-number-face
-     (alias) @font-lock-type-face
-     (call target: (dot left: (atom) @font-lock-type-face))
-     (char) @font-lock-constant-face
-     [(atom) (quoted_atom)] @font-lock-type-face
-     [(keyword) (quoted_keyword)] @font-lock-builtin-face)
+   `(["!"] @font-lock-negation-char-face
+     ["%"] @font-lock-bracket-face
+     ["," ";"] @font-lock-operator-face
+     ["(" ")" "[" "]" "{" "}" "<<" ">>"] @font-lock-bracket-face)
 
    :language 'elixir
-   :feature 'elixir-call
-   `((call
-      target: (identifier) @font-lock-keyword-face
-      (:match ,elixir-ts--definition-keywords-re @font-lock-keyword-face))
-     (call
-      target: (identifier) @font-lock-keyword-face
-      (:match ,elixir-ts--kernel-keywords-re @font-lock-keyword-face))
-     (call
-      target: [(identifier) @font-lock-function-name-face
-               (dot right: (identifier) @font-lock-keyword-face)])
+   :feature 'elixir-data-type
+   '([(atom) (alias)] @font-lock-type-face
+     (keywords (pair key: (keyword) @elixir-ts-keyword-key))
+     [(keyword) (quoted_keyword)] @elixir-ts-atom
+     [(boolean) (nil)] @elixir-ts-atom
+     (unary_operator operator: "@" @elixir-ts-attribute
+                     operand: [
+                               (identifier) @elixir-ts-attribute
+                               (call target: (identifier)
+                                     @elixir-ts-attribute)
+                               (boolean) @elixir-ts-attribute
+                               (nil) @elixir-ts-attribute
+                               ])
+     (operator_identifier) @font-lock-operator-face)
+
+   :language 'elixir
+   :feature 'elixir-keyword
+   `(,elixir-ts--reserved-keywords-vector
+     @font-lock-keyword-face
+     (binary_operator
+      operator: _ @font-lock-keyword-face
+      (:match ,elixir-ts--reserved-keywords-re @font-lock-keyword-face))
+     (binary_operator operator: _ @font-lock-operator-face)
      (call
       target: (identifier) @font-lock-keyword-face
-      (arguments
-       [
-        (identifier) @font-lock-keyword-face
-        (binary_operator
-         left: (identifier) @font-lock-keyword-face
-         operator: "when")
-        ])
       (:match ,elixir-ts--definition-keywords-re @font-lock-keyword-face))
      (call
       target: (identifier) @font-lock-keyword-face
-      (arguments
-       (binary_operator
-        operator: "|>"
-        right: (identifier)))
-      (:match ,elixir-ts--definition-keywords-re @font-lock-keyword-face)))
+      (:match ,elixir-ts--kernel-keywords-re @font-lock-keyword-face)))
 
    :language 'elixir
-   :feature 'elixir-constant
-   `((binary_operator operator: "|>" right: (identifier)
-                      @font-lock-function-name-face)
-     ((identifier) @font-lock-keyword-face
-      (:match ,elixir-ts--builtin-keywords-re
-              @font-lock-keyword-face))
-     ((identifier) @font-lock-comment-face
-      (:match "^_" @font-lock-comment-face))
-     (identifier) @font-lock-function-name-face
-     ["%"] @font-lock-keyward-face
-     ["," ";"] @font-lock-keyword-face
-     ["(" ")" "[" "]" "{" "}" "<<" ">>"] @font-lock-keyword-face)
+   :feature 'elixir-function-call
+   '((call target: (identifier) @font-lock-function-call-face)
+     (unary_operator operator: "&" @font-lock-operator-face
+                     operand: (binary_operator
+                               left: (identifier)
+                               @font-lock-function-call-face
+                               operator: "/" right: (integer)))
+     (call
+      target: (dot right: (identifier) @font-lock-function-call-face))
+     (unary_operator operator: "&" @font-lock-variable-name-face
+                     operand: (integer) @font-lock-variable-name-face)
+     (unary_operator operator: "&" @font-lock-operator-face
+                     operand: (list)))
 
    :language 'elixir
-   :feature 'elixir-sigil
+   :feature 'elixir-string-escape
    :override t
-   `((sigil
-      (sigil_name) @elixir-ts-font-sigil-name-face
-      (:match "^[^HF]$" @elixir-ts-font-sigil-name-face))
-     @font-lock-string-face
-     (sigil
-      (sigil_name) @font-lock-regexp-face
-      (:match "^[rR]$" @font-lock-regexp-face))
-     @font-lock-regexp-face
-     (sigil
-      "~" @font-lock-string-face
-      (sigil_name) @elixir-ts-font-sigil-name-face
-      quoted_start: _ @font-lock-string-face
-      quoted_end: _ @font-lock-string-face
-      (:match "^[HF]$" @elixir-ts-font-sigil-name-face)))
+   `((escape_sequence) @font-lock-escape-face)
 
    :language 'elixir
-   :feature 'elixir-string-escape
+   :feature 'elixir-number
+   '([(integer) (float)] @font-lock-number-face)
+
+   :language 'elixir
+   :feature 'elixir-variable
+   '((binary_operator left: (identifier) @font-lock-variable-name-face)
+     (binary_operator right: (identifier) @font-lock-variable-name-face)
+     (arguments ( (identifier) @font-lock-variable-name-face))
+     (tuple (identifier) @font-lock-variable-name-face)
+     (list (identifier) @font-lock-variable-name-face)
+     (pair value: (identifier) @font-lock-variable-name-face)
+     (body (identifier) @font-lock-variable-name-face)
+     (unary_operator operand: (identifier) @font-lock-variable-name-face)
+     (interpolation (identifier) @font-lock-variable-name-face)
+     (do_block (identifier) @font-lock-variable-name-face))
+
+   :language 'elixir
+   :feature 'elixir-builtin
    :override t
-   `((escape_sequence) @font-lock-regexp-grouping-backslash))
+   `(((identifier) @font-lock-builtin-face
+      (:match ,elixir-ts--builtin-keywords-re
+              @font-lock-builtin-face))))
+
   "Tree-sitter font-lock settings.")
 
 (defvar elixir-ts--treesit-range-rules
@@ -640,10 +697,12 @@ elixir-ts-mode
     ;; Font-lock.
     (setq-local treesit-font-lock-settings elixir-ts--font-lock-settings)
     (setq-local treesit-font-lock-feature-list
-                '(( elixir-comment elixir-constant elixir-doc )
-                  ( elixir-string elixir-keyword elixir-unary-operator
-                    elixir-call elixir-operator )
-                  ( elixir-sigil elixir-string-escape elixir-string-interpolation)))
+                '(( elixir-comment elixir-doc elixir-function-name)
+                  ( elixir-string elixir-keyword elixir-data-type)
+                  ( elixir-sigil elixir-variable elixir-builtin
+                    elixir-string-escape)
+                  ( elixir-function-call elixir-operator elixir-number )))
+
 
     ;; Imenu.
     (setq-local treesit-simple-imenu-settings
@@ -675,13 +734,13 @@ elixir-ts-mode
                           heex-ts--indent-rules))
 
       (setq-local treesit-font-lock-feature-list
-                  '(( elixir-comment elixir-constant elixir-doc
+                  '(( elixir-comment elixir-doc elixir-function-name
                       heex-comment heex-keyword heex-doctype )
-                    ( elixir-string elixir-keyword elixir-unary-operator
-                      elixir-call elixir-operator
-                      heex-component heex-tag heex-attribute heex-string)
-                    ( elixir-sigil elixir-string-escape
-                      elixir-string-interpolation ))))
+                    ( elixir-string elixir-keyword elixir-data-type
+                      heex-component heex-tag heex-attribute heex-string )
+                    ( elixir-sigil elixir-variable elixir-builtin
+                      elixir-string-escape)
+                    ( elixir-function-call elixir-operator elixir-number ))))
 
     (treesit-major-mode-setup)
     (setq-local syntax-propertize-function #'elixir-ts--syntax-propertize)))
-- 
2.43.0