bug#64647: treesit-query-error due to a recent change to tree-sitter-javascript grammar definition

[Theodor Thornhill]

Vincenzo Pupillo <v.pupillo@gmail.com> writes:

> Hi,
>
> In data domenica 16 luglio 2023 20:19:38 CEST, Eli Zaretskii ha scritto:
>> > From: Vincenzo Pupillo <v.pupillo@gmail.com>
>> > Cc: 64647@debbugs.gnu.org, jostein@kjonigsen.net
>> > Date: Sun, 16 Jul 2023 20:00:43 +0200
>> > 
>> > In my patch for java-ts-mode I used treesit-query-capture to figure out
>> > whether a symbol was defined or not. Check out the
>> > java-ts-mode--string-highlight- helper function.
>> 
>> Can you do something similar in this case?  That would be good enough
>> for Emacs 29.1.
>
> In attachment you can find the new version of the patches (similar to the 
> patch 
> that i made for java-ts-mode).
> The patches were made on the branch emacs-29.  
>
> Both work with the latest grammar. The javascript version is reliable, the 
> typescrypt version seems reliable. In fact, drum roll, with typescrypt both 
> tests:
> 1. (treesit-query-capture 'typescript '((member_expression) @capture)) ;; the 
> new node type
> 2. (treesit-query-capture 'typescript '((nested_identifier) @capture)) ;; the 
> old node type
> both return nil !!!
> If you use #2, then treesitter-ts-mode font lock gives an error!
> This happens only for font-lock, while for indentation everything works 
> correctly.

For Typescript these changes should go into 'tsx-ts-mode, not
'typescript-ts-mode. That may be why you are seeing some strange results?

>
> No problem for javascript, it works as expected for font-lock. The old node 
> type returns an error with treesit-query-capture.
>
> @Theo: I am not sure if the indentation works if I add the new rules to 
> treesit-simple-indent-rules in the Init function. I am writing a major-mode 
> for php and, unless I am mistaken or due to problems with the various 
> treesitter parsers, the order seems to be important.

Thanks! Some minor comments (all of which apply to both patches, even
though the comment is only for one of them):

>  
> From c6a93b510378756f2eff01a11ef4f9127a5e5d17 Mon Sep 17 00:00:00 2001
> From: Vincenzo Pupillo <v.pupillo@gmail.com>
> Date: Mon, 17 Jul 2023 22:20:44 +0200
> Subject: [PATCH] Updated JSX support due to changes in tree-sitter-javascript
>
> A recent change in tree-sitter-javascript grammar support for
> JSX (commit bb1f97b), changed two things:
> 1. renamed nested_identifier to member_expression
> 2. removed jsx_fragment, jsx_text is used instead
>
> * lisp/progmodes/js.el: (js--treesit-indent-helper): indent helper
>   function for handle different tree-sitter-javascript version
> * lisp/progmodes/js.el: (js--treesit-indent-rules): use the new
>   function
> * lisp/progmodes/js.el: (js--treesit-font-lock-helper): font lock
>   helper function for handle different tree-sitter-javascript version
> * lisp/progmodes/js.el: (js--treesit-font-lock-settings): use the new
>   function
> ---

"... function to handle ..."

>  lisp/progmodes/js.el | 65 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 48 insertions(+), 17 deletions(-)
>
> diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
> index a05bd758dbc..f5158195500 100644
> --- a/lisp/progmodes/js.el
> +++ b/lisp/progmodes/js.el
> @@ -3427,6 +3427,18 @@ This function is intended for use in 
> `after-change-functions'."
>  
>  ;;; Tree sitter integration
>  
> +(defun js--treesit-indent-helper ()
> +    "Indent rules helper, for handle different release of 
> tree-sitter-javascript.
> +Check if a node type is available, then return the right indent rules."

"Indent rules helper, to handle different releases of tree-sitter-javascript."

> +    ;; handle commit bb1f97b
> +    (condition-case nil
> +        (progn (treesit-query-capture 'javascript '((jsx_fragment) @capture))
> +               `(((match "<" "jsx_fragment") parent 0)
> +                 ((parent-is "jsx_fragment") parent js-indent-level)))
> +      (error
> +       `(((match "<" "jsx_text") parent 0)
> +         ((parent-is "jsx_text") parent js-indent-level)))))
> +
>  (defvar js--treesit-indent-rules
>    (let ((switch-case (rx "switch_" (or "case" "default"))))
>      `((javascript
> @@ -3462,8 +3474,9 @@ This function is intended for use in 
> `after-change-functions'."
>         ((parent-is "statement_block") parent-bol js-indent-level)
>  
>         ;; JSX
> -       ((match "<" "jsx_fragment") parent 0)
> -       ((parent-is "jsx_fragment") parent js-indent-level)
> +       ;; ((match "<" "jsx_fragment") parent 0)
> +       ;; ((parent-is "jsx_fragment") parent js-indent-level)
> +       (js--treesit-indent-helper)
>         ((node-is "jsx_closing_element") parent 0)
>         ((match "jsx_element" "statement") parent js-indent-level)
>         ((parent-is "jsx_element") parent js-indent-level)
> @@ -3490,6 +3503,36 @@ This function is intended for use in 
> `after-change-functions'."
>      "&&" "||" "!")
>    "JavaScript operators for tree-sitter font-locking.")
>  
> +(defun js--treesit-font-lock-helper ()
> +  "Font lock rules helper, for handle different release of 
> tree-sitter-javascript.
> +Check if a node type is available, then return the right font lock rules."

Same little comment here.

> +  ;; handle commit bb1f97b
> +  (condition-case nil
> +      (progn (treesit-query-capture 'javascript '((member_expression) 
> @capture))
> +          '((jsx_opening_element
> +             [(member_expression (identifier)) (identifier)]
> +             @font-lock-function-call-face)
> +
> +            (jsx_closing_element
> +             [(member_expression (identifier)) (identifier)]
> +             @font-lock-function-call-face)
> +
> +            (jsx_self_closing_element
> +             [(member_expression (identifier)) (identifier)]
> +             @font-lock-function-call-face)))
> +    (error
> +     '((jsx_opening_element
> +     [(nested_identifier (identifier)) (identifier)]
> +     @font-lock-function-call-face)
> +
> +       (jsx_closing_element
> +     [(nested_identifier (identifier)) (identifier)]
> +     @font-lock-function-call-face)
> +
> +       (jsx_self_closing_element
> +     [(nested_identifier (identifier)) (identifier)]
> +     @font-lock-function-call-face)))))
> +

The indentation here looks off. Can you format this?

>  (defvar js--treesit-font-lock-settings
>    (treesit-font-lock-rules
>  
> @@ -3599,21 +3642,9 @@ This function is intended for use in 
> `after-change-functions'."
>  
>     :language 'javascript
>     :feature 'jsx
> -   '((jsx_opening_element
> -      [(nested_identifier (identifier)) (identifier)]
> -      @font-lock-function-call-face)
> -
> -     (jsx_closing_element
> -      [(nested_identifier (identifier)) (identifier)]
> -      @font-lock-function-call-face)
> -
> -     (jsx_self_closing_element
> -      [(nested_identifier (identifier)) (identifier)]
> -      @font-lock-function-call-face)
> -
> -     (jsx_attribute
> -      (property_identifier)
> -      @font-lock-constant-face))
> +   (append
> +    (js--treesit-font-lock-helper)
> +    '((jsx_attribute (property_identifier) @font-lock-constant-face)))
>  
>     :language 'javascript
>     :feature 'number
> -- 
> 2.41.0
>
> From 263c9f0eca3a7df7cb29306297d32358f0e6537c Mon Sep 17 00:00:00 2001
> From: Vincenzo Pupillo <v.pupillo@gmail.com>
> Date: Mon, 17 Jul 2023 22:32:13 +0200
> Subject: [PATCH] Updated JSX support due to changes in tree-sitter-typescript
>
> A recent change in tree-sitter-typescript grammar support for
> JSX (commit b893426), changed two things:
> 1. renamed nested_identifier to member_expression
> 2. removed jsx_fragment, jsx_text is used instead
>
> * lisp/progmodes/typescript-ts-mode.el: (typescript-ts-mode--indent-helper): 
> indent helper
>   function for handle different tree-sitter-javascript version
> * lisp/progmodes/typescript-ts-mode.el: (typescript-ts-mode--indent-rules): 
> use the new
>   function
> * lisp/progmodes/typescript-ts-mode.el: 
> (typescript-ts-mode--font-lock-helper): font lock
>   helper function for handle different tree-sitter-javascript version
> * lisp/progmodes/typescript-ts-mode.el: 
> (typescript-ts-mode--font-lock-settings): use the new
>   function
> ---
>  lisp/progmodes/typescript-ts-mode.el | 64 +++++++++++++++++++++-------
>  1 file changed, 49 insertions(+), 15 deletions(-)
>
> diff --git a/lisp/progmodes/typescript-ts-mode.el 
> b/lisp/progmodes/typescript-ts-mode.el
> index 5df34de0472..2de7587e43a 100644
> --- a/lisp/progmodes/typescript-ts-mode.el
> +++ b/lisp/progmodes/typescript-ts-mode.el
> @@ -75,6 +75,18 @@
>      table)
>    "Syntax table for `typescript-ts-mode'.")
>  

This seems to not be properly converted to tsx from javascript, both in
the docstring and code.  Also, I think the name is wrong. Maybe it
should describe its intent a little more closely, something like
"tsx-ts-mode--indent-compatibility-b893426"?

> +(defun typescript-ts-mode--indent-helper ()
          ^^^^^^^tsx-ts-mode
> +    "Indent rules helper, for handle different release of 
> tree-sitter-typescript.
> +Check if a node type is available, then return the right indent rules."
> +    ;; handle commit b893426
> +    (condition-case nil
> +        (progn (treesit-query-capture 'javascript '((jsx_fragment) @capture))
                                         'tsx^^^^^^^, right?
> +               `(((match "<" "jsx_fragment") parent 0)
> +                 ((parent-is "jsx_fragment") parent 
> typescript-ts-mode-indent-offset)))
> +      (error
> +       `(((match "<" "jsx_text") parent 0)
> +         ((parent-is "jsx_text") parent typescript-ts-mode-indent-offset)))))
> +
>  (defun typescript-ts-mode--indent-rules (language)
>    "Rules used for indentation.
>  Argument LANGUAGE is either `typescript' or `tsx'."
> @@ -110,8 +122,7 @@ Argument LANGUAGE is either `typescript' or `tsx'."
>       ((parent-is "binary_expression") parent-bol 
> typescript-ts-mode-indent-offset)
>  
>       ,@(when (eq language 'tsx)
> -         `(((match "<" "jsx_fragment") parent 0)
> -           ((parent-is "jsx_fragment") parent 
> typescript-ts-mode-indent-offset)
> +         `((typescript-ts-mode--indent-helper)
>             ((node-is "jsx_closing_element") parent 0)
>             ((match "jsx_element" "statement") parent 
> typescript-ts-mode-indent-offset)
>             ((parent-is "jsx_element") parent 
> typescript-ts-mode-indent-offset)
> @@ -142,6 +153,39 @@ Argument LANGUAGE is either `typescript' or `tsx'."
>      "&&" "||" "!" "?.")
>    "TypeScript operators for tree-sitter font-locking.")
>  

Same naming comment here, and the language is wrong. It should be 'tsx,
not 'typescript. 

> +(defun typescript-ts-mode--font-lock-helper ()
> +  "Font lock rules helper, for handle different release of 
> tree-sitter-typescript.
> +Check if a node type is available, then return the right font lock rules."
> +  ;; handle commit bb1f97b
> +  ;; Warning: treesitter-query-capture says both node types are valid,
> +  ;; but then raises an error if the wrong node type is used. So it is
> +  ;; important to check with the new node type (member_expression)
> +  (condition-case nil
> +      (progn (treesit-query-capture 'typescript '((member_expression) 
> @capture))
> +          '((jsx_opening_element
> +             [(member_expression (identifier)) (identifier)]
> +             @typescript-ts-jsx-tag-face)
> +
> +            (jsx_closing_element
> +             [(member_expression (identifier)) (identifier)]
> +             @typescript-ts-jsx-tag-face)
> +
> +            (jsx_self_closing_element
> +             [(member_expression (identifier)) (identifier)]
> +             @typescript-ts-jsx-tag-face)))
> +    (error
> +     '((jsx_opening_element
> +     [(nested_identifier (identifier)) (identifier)]
> +     @typescript-ts-jsx-tag-face)
> +
> +       (jsx_closing_element
> +     [(nested_identifier (identifier)) (identifier)]
> +     @typescript-ts-jsx-tag-face)
> +
> +       (jsx_self_closing_element
> +     [(nested_identifier (identifier)) (identifier)]
> +     @typescript-ts-jsx-tag-face)))))
> +
>  (defun typescript-ts-mode--font-lock-settings (language)
>    "Tree-sitter font-lock settings.
>  Argument LANGUAGE is either `typescript' or `tsx'."
> @@ -293,19 +337,9 @@ Argument LANGUAGE is either `typescript' or `tsx'."
>  
>     :language language
>     :feature 'jsx
> -   `((jsx_opening_element
> -      [(nested_identifier (identifier)) (identifier)]
> -      @typescript-ts-jsx-tag-face)
> -
> -     (jsx_closing_element
> -      [(nested_identifier (identifier)) (identifier)]
> -      @typescript-ts-jsx-tag-face)
> -
> -     (jsx_self_closing_element
> -      [(nested_identifier (identifier)) (identifier)]
> -      @typescript-ts-jsx-tag-face)
> -
> -     (jsx_attribute (property_identifier) @typescript-ts-jsx-attribute-face))
> +   (append
> +    (typescript-ts-mode--font-lock-helper)
> +    `((jsx_attribute (property_identifier) 
> @typescript-ts-jsx-attribute-face)))
>  
>     :language language
>     :feature 'number
> -- 
> 2.41.0

Thanks for the patch :)

Theo