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