Re: Call for volunteers: add tree-sitter support to major modes

[Yuan Fu <casouri@gmail.com>]

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



> On Oct 10, 2022, at 12:08 AM, Theodor Thornhill <theo@thornhill.no> wrote:
> 
> Hi!
> 
>> Looks good! Here are some comments.
>> 
>> +
>> +     ;; FIXME: We need to be able to set the priority for font-locking
>> +     ;; somehow.  We cannot just override all of the template string,
>> +     ;; as that would mess up interpolated expressions
>> +     ;;
>> +     ;; (template_string) @font-lock-string-face
>> +     (template_substitution ["${" "}"] @font-lock-constant-face)
>> +     )))
>> 
>> What exactly do you mean by priority here? Why doesn't :override t
>> work?
>> 
> 
> In template strings in JavaScript we have issues with precedence in that
> we default to string font locking for everything that does _not_ have
> specific rules.  An image speaks a thousand words, so I'll add
> screenshots.  What I need is some sort of way to ensure that inside of
> template strings font-locking should _not_ happen
> 
>> +
>> +(defvar js-treesit--defun-query
>> +  "[(class_declaration)
>> +    (method_definition)
>> +    (function_declaration)
>> +    (variable_declarator)] @defun")
>> 
>> This should be compiled.
>> 
>> +
>> +(defun js--treesit-enable ()
>> +  (unless (and (treesit-can-enable-p)
>> +               (treesit-language-available-p 'javascript))
>> +    (error "Tree sitter isn't available"))
>> 
>> I don't think we should error here, I'd displaying a message instead.
>> 
>> +
>> +  ;; Comments
>> +  (setq-local comment-start "// ")
>> +  (setq-local comment-start-skip "\\(?://+\\|/\\*+\\)\\s *")
>> +  (setq-local comment-end "")
>> 
>> I think it's best to not repeat code, could you move this outside the
>> (if tree-sitter) form and have it run regardless?
>> 
>> +(defun js--json-treesit-enable ()
>> +  (unless (and (treesit-can-enable-p)
>> +               (treesit-language-available-p 'json))
>> +    (error "Tree sitter isn't available"))
>> 
>> Same as above, IMO message is better.
>> 
> 
> I added some variartion of this.  I think also message is better,
> because user-error makes the logic a little harder.  What do you think?

Yeah user-error is mostly the same as a message, since nobody (I think) turns on beeping anymore. Though I hope there is a way to warn users that’s not missable. Messages (and user-error) could be covered by later messages. Hmm, maybe we can use warnings.

For template string please see that patch that applies on top of your patch. It seems to work for me. (Also see screenshot attached, brackets have different colors because I forgot to turn off rainbow-delimiter-mode when taking the shot). Also some of the lines are longer than 70 columns. Could you wrap those lines?

Thanks!
Yuan


[-- Attachment #2: template-string.patch --]
[-- Type: application/octet-stream, Size: 2241 bytes --]

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index d1f655cdc48..387acb4ebb1 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -3402,7 +3402,7 @@ c-paragraph-start
   js-mode "\\(@[[:alpha:]]+\\>\\|$\\)")
 
 ;;; Tree sitter integration
-(defcustom js-use-treesitter nil
+(defcustom js-use-tree-sitter nil
   "If non-nil, `js-mode' tries to use tree-sitter.
 Currently `js-mode' uses tree-sitter for font-locking,
 indentation, which-function and movement functions."
@@ -3459,7 +3459,9 @@ js--treesit-settings
   (treesit-font-lock-rules
    :language 'javascript
    :override t
-   `(
+   `(;; Everything overrides template string.
+     (template_string) @font-lock-string-face
+
      ((identifier) @font-lock-constant-face
       (:match "^[A-Z_][A-Z_\\d]*$" @font-lock-constant-face))
 
@@ -3551,13 +3553,7 @@ js--treesit-settings
      (comment) @font-lock-comment-face
      [,@js--treesit-keywords] @font-lock-keyword-face
 
-     ;; FIXME: We need to be able to set the priority for font-locking
-     ;; somehow.  We cannot just override all of the template string,
-     ;; as that would mess up interpolated expressions
-     ;;
-     ;; (template_string) @font-lock-string-face
-     (template_substitution ["${" "}"] @font-lock-constant-face)
-     )))
+     (template_substitution ["${" "}"] @font-lock-constant-face))))
 
 
 (defun js-treesit-current-defun ()
@@ -3604,7 +3600,7 @@ js-treesit--defun-query
     (variable_declarator)] @defun"))
 
 (defun js--treesit-can-enable-p ()
-  (if (and js-use-treesitter
+  (if (and js-use-tree-sitter
            (treesit-can-enable-p)
            (treesit-language-available-p 'javascript))
       t
@@ -3716,7 +3712,7 @@ js-mode
     ;;(syntax-propertize (point-max))
     ))
 
-(defcustom js-json-use-treesitter nil
+(defcustom js-json-use-tree-sitter nil
   "If non-nil, `js-json-mode' tries to use tree-sitter.
 Currently `js-json-mode' uses tree-sitter for font-locking and
 indentation."
@@ -3755,7 +3751,7 @@ js--json-treesit-indent-rules
 
 
 (defun js--json-treesit-can-enable-p ()
-  (if (and js-json-use-treesitter
+  (if (and js-json-use-tree-sitter
            (treesit-can-enable-p)
            (treesit-language-available-p 'json))
       t

[-- Attachment #3: Screen Shot 2022-10-10 at 8.37.34 AM.png --]
[-- Type: image/png, Size: 47813 bytes --]

[-- Attachment #4: Type: text/plain, Size: 2 bytes --]