bug#74386: Tree-sitter javascript indentation

bug#74386: Tree-sitter javascript indentation

[Marius Kjeldahl]

When using js-ts-mode, after multiline function parameter lists, or
multiline if clauses, Emacs typically adds two spaces indentation
after where the previous line ends. But because that line is already
indented, the effective indentation becomes four spaces (compare to
where the function definition or if clause starts). I would like this
to be only two. This is most likely a preference, but still.

I haven't been able to figure out if it is possible to customize Emacs
to do what I want. Or even learn where I can dig in and figure out
what I need to change to accomplish it. I've tried a reddit group and
also posted an issue on the tree-sitter github repo. But was told the
correct place to report it is as an Emacs bug.

So any pointers or suggestion?

Thanks,

Marius K.

bug#74386: Tree-sitter javascript indentation

[Dmitry Gutov]

Hi!

On 17/11/2024 01:26, Marius Kjeldahl wrote:
> When using js-ts-mode, after multiline function parameter lists, or
> multiline if clauses, Emacs typically adds two spaces indentation
> after where the previous line ends. But because that line is already
> indented, the effective indentation becomes four spaces (compare to
> where the function definition or if clause starts). I would like this
> to be only two. This is most likely a preference, but still.
> 
> I haven't been able to figure out if it is possible to customize Emacs
> to do what I want. Or even learn where I can dig in and figure out
> what I need to change to accomplish it. I've tried a reddit group and
> also posted an issue on the tree-sitter github repo. But was told the
> correct place to report it is as an Emacs bug.
> 
> So any pointers or suggestion?

It would help if you also give specific examples of code where incorrect 
indentation occurs. People can guess, but they might not guess all the 
cases you want.

Just paste the code inside email, assuming monospaced text.

(And to clarify how this mailing list works: please use "reply all", so 
that the bug# email address is retained in To:).

bug#74386: Tree-sitter javascript indentation

[Marius Kjeldahl]

Great, thanks.

Here are two examples (fingers crossed):

function a(b,
  c) {
....d();

and

if (clause1
  && clause2) {
....callSomeFunc();

Notice in both cases the four dots "...." representing spaces used for
indentation. I would like only two dots (two spaces) for those
specific examples.

Thanks,

Marius K.

On Sun, 17 Nov 2024 at 20:18, Dmitry Gutov <dmitry@gutov.dev> wrote:
>
> Hi!
>
> On 17/11/2024 01:26, Marius Kjeldahl wrote:
> > When using js-ts-mode, after multiline function parameter lists, or
> > multiline if clauses, Emacs typically adds two spaces indentation
> > after where the previous line ends. But because that line is already
> > indented, the effective indentation becomes four spaces (compare to
> > where the function definition or if clause starts). I would like this
> > to be only two. This is most likely a preference, but still.
> >
> > I haven't been able to figure out if it is possible to customize Emacs
> > to do what I want. Or even learn where I can dig in and figure out
> > what I need to change to accomplish it. I've tried a reddit group and
> > also posted an issue on the tree-sitter github repo. But was told the
> > correct place to report it is as an Emacs bug.
> >
> > So any pointers or suggestion?
>
> It would help if you also give specific examples of code where incorrect
> indentation occurs. People can guess, but they might not guess all the
> cases you want.
>
> Just paste the code inside email, assuming monospaced text.
>
> (And to clarify how this mailing list works: please use "reply all", so
> that the bug# email address is retained in To:).

bug#74386: Tree-sitter javascript indentation

[Dmitry Gutov]

On 17/11/2024 21:21, Marius Kjeldahl wrote:
> Great, thanks.
> 
> Here are two examples (fingers crossed):
> 
> function a(b,
>    c) {
> ....d();
> 
> and
> 
> if (clause1
>    && clause2) {
> ....callSomeFunc();
> 
> Notice in both cases the four dots "...." representing spaces used for
> indentation. I would like only two dots (two spaces) for those
> specific examples.

Thank you, these two examples should be handled by the patch below.

It's a change which could affect a fair amount of other examples too - 
for example

Foobar
   .find()
   .catch((err) => {
     return 2;
   })
   .then((num) => {
     console.log(num);
   });

turns into

Foobar
   .find()
   .catch((err) => {
   return 2;
})
   .then((num) => {
   console.log(num);
});


I'm not 100% sure if that's good or bad (maybe worse compatibility with 
js-mode, that we could say), so it would be useful to go through a bunch 
of other examples and decide whether what we had is a bug, or these 
should be supported as different styles, or etc.

Cc'ing the original author of js-ts-mode, Theodor, for consultation.

BTW, to try out such a patch you need to apply it, evaluate the defvar 
form (C-M-x, for example), and then re-enable the major mode in a given 
buffer (M-x js-ts-mode).

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index f74b8ab1c46..17a9509dd45 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -3453,7 +3453,7 @@ js--treesit-indent-rules
    (let ((switch-case (rx "switch_" (or "case" "default"))))
      `((javascript
         ((parent-is "program") parent-bol 0)
-       ((node-is "}") parent-bol 0)
+       ((node-is "}") standalone-parent 0)
         ((node-is ")") parent-bol 0)
         ((node-is "]") parent-bol 0)
         ((node-is ">") parent-bol 0)
@@ -3466,7 +3466,7 @@ js--treesit-indent-rules
         ;; "{" on the newline.
         ((node-is "statement_block") parent-bol js-indent-level)
         ((parent-is "named_imports") parent-bol js-indent-level)
-       ((parent-is "statement_block") parent-bol js-indent-level)
+       ((parent-is "statement_block") standalone-parent js-indent-level)
         ((parent-is "variable_declarator") parent-bol js-indent-level)
         ((parent-is "arguments") parent-bol js-indent-level)
         ((parent-is "array") parent-bol js-indent-level)
@@ -3481,7 +3481,6 @@ js--treesit-indent-rules
         ((parent-is "binary_expression") parent-bol js-indent-level)
         ((parent-is "class_body") parent-bol js-indent-level)
         ((parent-is ,switch-case) parent-bol js-indent-level)
-       ((parent-is "statement_block") parent-bol js-indent-level)
         ((match "while" "do_statement") parent-bol 0)
         ((match "else" "if_statement") parent-bol 0)
         ((parent-is ,(rx (or (seq (or "if" "for" "for_in" "while" "do") 
"_statement")

bug#74386: Tree-sitter javascript indentation

[Marius Kjeldahl]

Sounds good, will try. I'm not sure I like you other examples much, so
customizability for these things would be preferable.

Thanks,

Marius K.

On Sun, 17 Nov 2024 at 23:12, Dmitry Gutov <dmitry@gutov.dev> wrote:
>
> On 17/11/2024 21:21, Marius Kjeldahl wrote:
> > Great, thanks.
> >
> > Here are two examples (fingers crossed):
> >
> > function a(b,
> >    c) {
> > ....d();
> >
> > and
> >
> > if (clause1
> >    && clause2) {
> > ....callSomeFunc();
> >
> > Notice in both cases the four dots "...." representing spaces used for
> > indentation. I would like only two dots (two spaces) for those
> > specific examples.
>
> Thank you, these two examples should be handled by the patch below.
>
> It's a change which could affect a fair amount of other examples too -
> for example
>
> Foobar
>    .find()
>    .catch((err) => {
>      return 2;
>    })
>    .then((num) => {
>      console.log(num);
>    });
>
> turns into
>
> Foobar
>    .find()
>    .catch((err) => {
>    return 2;
> })
>    .then((num) => {
>    console.log(num);
> });
>
>
> I'm not 100% sure if that's good or bad (maybe worse compatibility with
> js-mode, that we could say), so it would be useful to go through a bunch
> of other examples and decide whether what we had is a bug, or these
> should be supported as different styles, or etc.
>
> Cc'ing the original author of js-ts-mode, Theodor, for consultation.
>
> BTW, to try out such a patch you need to apply it, evaluate the defvar
> form (C-M-x, for example), and then re-enable the major mode in a given
> buffer (M-x js-ts-mode).
>
> diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
> index f74b8ab1c46..17a9509dd45 100644
> --- a/lisp/progmodes/js.el
> +++ b/lisp/progmodes/js.el
> @@ -3453,7 +3453,7 @@ js--treesit-indent-rules
>     (let ((switch-case (rx "switch_" (or "case" "default"))))
>       `((javascript
>          ((parent-is "program") parent-bol 0)
> -       ((node-is "}") parent-bol 0)
> +       ((node-is "}") standalone-parent 0)
>          ((node-is ")") parent-bol 0)
>          ((node-is "]") parent-bol 0)
>          ((node-is ">") parent-bol 0)
> @@ -3466,7 +3466,7 @@ js--treesit-indent-rules
>          ;; "{" on the newline.
>          ((node-is "statement_block") parent-bol js-indent-level)
>          ((parent-is "named_imports") parent-bol js-indent-level)
> -       ((parent-is "statement_block") parent-bol js-indent-level)
> +       ((parent-is "statement_block") standalone-parent js-indent-level)
>          ((parent-is "variable_declarator") parent-bol js-indent-level)
>          ((parent-is "arguments") parent-bol js-indent-level)
>          ((parent-is "array") parent-bol js-indent-level)
> @@ -3481,7 +3481,6 @@ js--treesit-indent-rules
>          ((parent-is "binary_expression") parent-bol js-indent-level)
>          ((parent-is "class_body") parent-bol js-indent-level)
>          ((parent-is ,switch-case) parent-bol js-indent-level)
> -       ((parent-is "statement_block") parent-bol js-indent-level)
>          ((match "while" "do_statement") parent-bol 0)
>          ((match "else" "if_statement") parent-bol 0)
>          ((parent-is ,(rx (or (seq (or "if" "for" "for_in" "while" "do")
> "_statement")
>

bug#74386: Tree-sitter javascript indentation

[Dmitry Gutov]

On 18/11/2024 00:21, Marius Kjeldahl wrote:
> Sounds good, will try. I'm not sure I like you other examples much, so
> customizability for these things would be preferable.

These particular cases could also be distinguished using custom logic, 
or even using a separate query.

But we might still want to allow customizing them separately (or not?) - 
and check other examples as well.

bug#74386: Tree-sitter javascript indentation

[Marius Kjeldahl]

Can confirm your patch solves both the variants I showed. This will
also help me to dig into and understand more of what is going on, and
allows me to experiment more with my preferences (and tree-sitter in
general). So thank you very much.

When I get a clue, or when someone with a clue comes up with it, a
patch which make these things configurable with easy settings would
probably be. a good idea (instead of diving into "tree-sitter
internals").

Thanks,

Marius K.

On Sun, 17 Nov 2024 at 23:12, Dmitry Gutov <dmitry@gutov.dev> wrote:
>
> On 17/11/2024 21:21, Marius Kjeldahl wrote:
> > Great, thanks.
> >
> > Here are two examples (fingers crossed):
> >
> > function a(b,
> >    c) {
> > ....d();
> >
> > and
> >
> > if (clause1
> >    && clause2) {
> > ....callSomeFunc();
> >
> > Notice in both cases the four dots "...." representing spaces used for
> > indentation. I would like only two dots (two spaces) for those
> > specific examples.
>
> Thank you, these two examples should be handled by the patch below.
>
> It's a change which could affect a fair amount of other examples too -
> for example
>
> Foobar
>    .find()
>    .catch((err) => {
>      return 2;
>    })
>    .then((num) => {
>      console.log(num);
>    });
>
> turns into
>
> Foobar
>    .find()
>    .catch((err) => {
>    return 2;
> })
>    .then((num) => {
>    console.log(num);
> });
>
>
> I'm not 100% sure if that's good or bad (maybe worse compatibility with
> js-mode, that we could say), so it would be useful to go through a bunch
> of other examples and decide whether what we had is a bug, or these
> should be supported as different styles, or etc.
>
> Cc'ing the original author of js-ts-mode, Theodor, for consultation.
>
> BTW, to try out such a patch you need to apply it, evaluate the defvar
> form (C-M-x, for example), and then re-enable the major mode in a given
> buffer (M-x js-ts-mode).
>
> diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
> index f74b8ab1c46..17a9509dd45 100644
> --- a/lisp/progmodes/js.el
> +++ b/lisp/progmodes/js.el
> @@ -3453,7 +3453,7 @@ js--treesit-indent-rules
>     (let ((switch-case (rx "switch_" (or "case" "default"))))
>       `((javascript
>          ((parent-is "program") parent-bol 0)
> -       ((node-is "}") parent-bol 0)
> +       ((node-is "}") standalone-parent 0)
>          ((node-is ")") parent-bol 0)
>          ((node-is "]") parent-bol 0)
>          ((node-is ">") parent-bol 0)
> @@ -3466,7 +3466,7 @@ js--treesit-indent-rules
>          ;; "{" on the newline.
>          ((node-is "statement_block") parent-bol js-indent-level)
>          ((parent-is "named_imports") parent-bol js-indent-level)
> -       ((parent-is "statement_block") parent-bol js-indent-level)
> +       ((parent-is "statement_block") standalone-parent js-indent-level)
>          ((parent-is "variable_declarator") parent-bol js-indent-level)
>          ((parent-is "arguments") parent-bol js-indent-level)
>          ((parent-is "array") parent-bol js-indent-level)
> @@ -3481,7 +3481,6 @@ js--treesit-indent-rules
>          ((parent-is "binary_expression") parent-bol js-indent-level)
>          ((parent-is "class_body") parent-bol js-indent-level)
>          ((parent-is ,switch-case) parent-bol js-indent-level)
> -       ((parent-is "statement_block") parent-bol js-indent-level)
>          ((match "while" "do_statement") parent-bol 0)
>          ((match "else" "if_statement") parent-bol 0)
>          ((parent-is ,(rx (or (seq (or "if" "for" "for_in" "while" "do")
> "_statement")
>

bug#74386: Tree-sitter javascript indentation

[Dmitry Gutov]

On 18/11/2024 10:35, Marius Kjeldahl wrote:
> Can confirm your patch solves both the variants I showed. This will
> also help me to dig into and understand more of what is going on, and
> allows me to experiment more with my preferences (and tree-sitter in
> general). So thank you very much.

Great!

For more experimenting, also check out 'M-x treesit-explore-mode'.

And here's how to immediately find out which indentation rule was used:

   (setq treesit--indent-verbose t)

bug#74386: Tree-sitter javascript indentation

[Eli Zaretskii]

> Cc: 74386@debbugs.gnu.org, Theodor Thornhill <theo@thornhill.no>
> Date: Mon, 18 Nov 2024 17:29:18 +0200
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 18/11/2024 10:35, Marius Kjeldahl wrote:
> > Can confirm your patch solves both the variants I showed. This will
> > also help me to dig into and understand more of what is going on, and
> > allows me to experiment more with my preferences (and tree-sitter in
> > general). So thank you very much.
> 
> Great!
> 
> For more experimenting, also check out 'M-x treesit-explore-mode'.
> 
> And here's how to immediately find out which indentation rule was used:
> 
>    (setq treesit--indent-verbose t)

Theo, please chime in.  Should Dmitry install his changes?

bug#74386: Tree-sitter javascript indentation

[Yuan Fu]

> On Nov 30, 2024, at 2:01 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> Cc: 74386@debbugs.gnu.org, Theodor Thornhill <theo@thornhill.no>
>> Date: Mon, 18 Nov 2024 17:29:18 +0200
>> From: Dmitry Gutov <dmitry@gutov.dev>
>> 
>> On 18/11/2024 10:35, Marius Kjeldahl wrote:
>>> Can confirm your patch solves both the variants I showed. This will
>>> also help me to dig into and understand more of what is going on, and
>>> allows me to experiment more with my preferences (and tree-sitter in
>>> general). So thank you very much.
>> 
>> Great!
>> 
>> For more experimenting, also check out 'M-x treesit-explore-mode'.
>> 
>> And here's how to immediately find out which indentation rule was used:
>> 
>>   (setq treesit--indent-verbose t)
> 
> Theo, please chime in.  Should Dmitry install his changes?

The standalone-parent anchor is designed specifically to solve the bug described here, and changing parent-bol to standalone-parent is very safe. So IMO we can merge this (either to emacs-30 or master).

Yuan

bug#74386: Tree-sitter javascript indentation

[Dmitry Gutov]

On 01/12/2024 07:23, Yuan Fu wrote:
>>> For more experimenting, also check out 'M-x treesit-explore-mode'.
>>>
>>> And here's how to immediately find out which indentation rule was used:
>>>
>>>    (setq treesit--indent-verbose t)
>> Theo, please chime in.  Should Dmitry install his changes?
> The standalone-parent anchor is designed specifically to solve the bug described here, and changing parent-bol to standalone-parent is very safe. So IMO we can merge this (either to emacs-30 or master).

Thanks for your answer.

What I was wondering, is whether we might need some user option(s) to 
switch back to parent-bol instead of standalone-parent for some 
constructs, and whether we should replace more uses of parent-bol with 
standaline-parent, and if so, which ones.

Also see the example of the indentation change with that patch 
(switching from one fairly popular indentation style to one slightly 
less popular - AFAIK).

bug#74386: Tree-sitter javascript indentation

[Yuan Fu]

> On Dec 1, 2024, at 5:11 AM, Dmitry Gutov <dmitry@gutov.dev> wrote:
> 
> On 01/12/2024 07:23, Yuan Fu wrote:
>>>> For more experimenting, also check out 'M-x treesit-explore-mode'.
>>>> 
>>>> And here's how to immediately find out which indentation rule was used:
>>>> 
>>>>   (setq treesit--indent-verbose t)
>>> Theo, please chime in.  Should Dmitry install his changes?
>> The standalone-parent anchor is designed specifically to solve the bug described here, and changing parent-bol to standalone-parent is very safe. So IMO we can merge this (either to emacs-30 or master).
> 
> Thanks for your answer.
> 
> What I was wondering, is whether we might need some user option(s) to switch back to parent-bol instead of standalone-parent for some constructs, and whether we should replace more uses of parent-bol with standaline-parent, and if so, which ones.
> 
> Also see the example of the indentation change with that patch (switching from one fairly popular indentation style to one slightly less popular - AFAIK).

Ah, I see. That’s a good point, and I definitely prefer the indentation result of parent-bol here. The one produced by standalone-parent is just wrong. What we can do is make standalone-parent ignore “.” when checking for “standaloneness”. And perhaps make it configurable so it’s enabled only for modes that this waiver makes sense (C-like languages excluding C and C++).

Yuan

bug#74386: Tree-sitter javascript indentation

[Dmitry Gutov]

On 01/12/2024 21:10, Yuan Fu wrote:
> Ah, I see. That’s a good point, and I definitely prefer the indentation result of parent-bol here. The one produced by standalone-parent is just wrong. What we can do is make standalone-parent ignore “.” when checking for “standaloneness”. And perhaps make it configurable so it’s enabled only for modes that this waiver makes sense (C-like languages excluding C and C++).

Maybe not by hardcoding this in inside the 'standalone-parent' matcher, 
but writing this in the indentation rules? Different languages might 
have differing ASTs for such construct.

Or if you meant to do a text search, a period might start a method call, 
but it could also continue a "range" literal in some other language, or 
some struct initializer (I think?) in C/C++. Also, some languages allow 
(and style guides suggest) to have the previous at the end of the line, 
then followed by newline and then the method name.

bug#74386: Tree-sitter javascript indentation

[Yuan Fu]

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



> On Dec 1, 2024, at 2:33 PM, Dmitry Gutov <dmitry@gutov.dev> wrote:
> 
> On 01/12/2024 21:10, Yuan Fu wrote:
>> Ah, I see. That’s a good point, and I definitely prefer the indentation result of parent-bol here. The one produced by standalone-parent is just wrong. What we can do is make standalone-parent ignore “.” when checking for “standaloneness”. And perhaps make it configurable so it’s enabled only for modes that this waiver makes sense (C-like languages excluding C and C++).
> 
> Maybe not by hardcoding this in inside the 'standalone-parent' matcher, but writing this in the indentation rules? Different languages might have differing ASTs for such construct.
> 
> Or if you meant to do a text search, a period might start a method call, but it could also continue a "range" literal in some other language, or some struct initializer (I think?) in C/C++. Also, some languages allow (and style guides suggest) to have the previous at the end of the line, then followed by newline and then the method name.

Not hard-coded, but customizable, like this:

Yuan


[-- Attachment #2: standalone-predicate-poc.patch --]
[-- Type: application/octet-stream, Size: 2553 bytes --]

From 301e8a55a14f61258c505a48a973379ac3156079 Mon Sep 17 00:00:00 2001
From: Yuan Fu <casouri@gmail.com>
Date: Sun, 1 Dec 2024 18:26:40 -0800
Subject: [PATCH] Standalone predicate POC

---
 lisp/treesit.el | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/lisp/treesit.el b/lisp/treesit.el
index 2acb46ab105..513f58eeffd 100644
--- a/lisp/treesit.el
+++ b/lisp/treesit.el
@@ -1597,6 +1597,37 @@ treesit--indent-prev-line-node
       (back-to-indentation)
       (treesit--indent-largest-node-at (point)))))
 
+(defvar treesit-simple-indent-standalone-predicate nil
+  "Function used to determine if a node is \"standalone\".
+
+\"Standalone\" means the node starts on a new line.  For example, if we
+look at the opening bracket, then it's standalone in this case:
+
+    {            <-- Standalone.
+      return 1;
+    }
+
+but not in this case:
+
+    if (true) {  <-- Not standalone.
+      return 1;
+    }
+
+The value of this variable affects the `standalone-parent' indent preset
+for treesit-simple-indent.  If the value is nil, the standlone condition
+is as described.  Some major mode might want to relax the condition a
+little bit, so that it ignores some punctuation like \".\".  For
+example, a Javascript mode might want to consider the method call below
+to be standalone too:
+
+    obj
+    .method(() => {   <-- Consider \".method\" to be standalone,
+      return 1;       <-- so this line anchors on \".method\".
+    });
+
+The value should be a function that takes a node, and return t if it's
+standalone.")
+
 (defvar treesit-simple-indent-presets
   (list (cons 'match
               (lambda
@@ -1736,8 +1767,10 @@ treesit-simple-indent-presets
                   (catch 'term
                     (while parent
                       (goto-char (treesit-node-start parent))
-                      (when (looking-back (rx bol (* whitespace))
-                                          (line-beginning-position))
+                      (when (if (null treesit-simple-indent-standalone-predicate)
+                                (looking-back (rx bol (* whitespace))
+                                              (line-beginning-position))
+                              (funcall parent))
                         (throw 'term (point)))
                       (setq parent (treesit-node-parent parent)))))))
         (cons 'prev-sibling (lambda (node parent bol &rest _)
-- 
2.39.5 (Apple Git-151)