bug#59686: 30.0.50; tree-sitter indentation in some loops and conditional statements is wrong

bug#59686: 30.0.50; tree-sitter indentation in some loops and conditional statements is wrong

[Bruce Stephens]

With the following file I would expect foo(i) to be indented to just two
indents (like the first one), but they are not. Similarly for many other
compound statements where the opening spans more than one line.

// -*- c-ts -*-
int main() {
    for (int i=0; i<10; ++i) {
      foo(i);
    }

    for (int i=0;
           i<10;
           ++i) {
             foo(i);
           }
}



In GNU Emacs 30.0.50 (build 1, x86_64-apple-darwin21.6.0, NS
appkit-2113.60 Version 12.6 (Build 21G115)) of 2022-11-29 built on
Bruces-MacBook-Pro.local
Repository revision: e90c21e6fb35d1c844c49d96f0b62c5fe55bb203
Repository branch: master
Windowing system distributor 'Apple', version 10.3.2113
System Description: macOS 12.6

Configured using:
'configure --with-native-compilation=aot --with-tree-sitter'

Configured features:
ACL GIF GLIB GMP GNUTLS JPEG JSON LCMS2 LIBXML2 MODULES NATIVE_COMP
NOTIFY KQUEUE NS PDUMPER PNG RSVG SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XIM ZLIB

Important settings:
value of $LANG: en_GB.UTF-8
locale-coding-system: utf-8-unix

Major mode: C

Minor modes in effect:
tooltip-mode: t
global-eldoc-mode: t
show-paren-mode: t
electric-indent-mode: t
mouse-wheel-mode: t
tool-bar-mode: t
menu-bar-mode: t
file-name-shadow-mode: t
global-font-lock-mode: t
font-lock-mode: t
blink-cursor-mode: t
line-number-mode: t
indent-tabs-mode: t
transient-mark-mode: t
auto-composition-mode: t
auto-encryption-mode: t
auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils comp comp-cstr
warnings icons subr-x rx cl-macs gv cl-extra help-mode bytecomp
byte-compile c-ts-mode treesit cl-seq cl-loaddefs cl-lib rmc iso-transl
tooltip cconv eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode mwheel term/ns-win ns-win ucs-normalize
mule-util term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads kqueue cocoa ns lcms2 multi-tty make-network-process
native-compile emacs)

Memory information:
((conses 16 81423 9034)
(symbols 48 7273 0)
(strings 32 20423 1742)
(string-bytes 1 634204)
(vectors 16 17092)
(vector-slots 8 341908 8957)
(floats 8 32 58)
(intervals 56 242 0)
(buffers 992 12))

bug#59686: 30.0.50; tree-sitter indentation in some loops and conditional statements is wrong

[Yuan Fu]

Bruce Stephens <bruce.stephens@isode.com> writes:

> With the following file I would expect foo(i) to be indented to just two
> indents (like the first one), but they are not. Similarly for many other
> compound statements where the opening spans more than one line.
>
> // -*- c-ts -*-
> int main() {
>    for (int i=0; i<10; ++i) {
>      foo(i);
>    }
>
>    for (int i=0;
>           i<10;
>           ++i) {
>             foo(i);
>           }
> }

I see, that’s because the indent rule finds the BOL of the line where
the "{" is on, and indents from there.  Theo, WDYT? Does indent style 
fix this, or we should change the indent rules?

Yuan

bug#59686: 30.0.50; tree-sitter indentation in some loops and conditional statements is wrong

[Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 2 December 2022 06:13:42 CET, Yuan Fu <casouri@gmail.com> wrote:
>
>Bruce Stephens <bruce.stephens@isode.com> writes:
>
>> With the following file I would expect foo(i) to be indented to just two
>> indents (like the first one), but they are not. Similarly for many other
>> compound statements where the opening spans more than one line.
>>
>> // -*- c-ts -*-
>> int main() {
>>    for (int i=0; i<10; ++i) {
>>      foo(i);
>>    }
>>
>>    for (int i=0;
>>           i<10;
>>           ++i) {
>>             foo(i);
>>           }
>> }
>
>I see, that’s because the indent rule finds the BOL of the line where
>the "{" is on, and indents from there.  Theo, WDYT? Does indent style 
>fix this, or we should change the indent rules?
>
>Yuan

I've seen this issue myself, and have tried several combinations to fix it. It is trivial to fix this particular case, but because compound_statement is used everywhere problems will pop up other places. The fix here would be some grand-parent-bol function, but if my memory serves that would mess up the else in an if else. I'm happy to see this fixed, but just remember that it's not _super_ trivial.

Other languages exhibit similar issues, but with other constructs.

Lastly, I _did_ fix most of these, but that required using query as the matcher in simple-indent-rules, which made indenting slower, and I needed one rule for almost every construct in a language.

I'm sure she fix can be simple, but I'm a little blind to it right now. My eyes are bleeding tree-sitter nodes at the moment, so I might not see clearly :P

bug#59686: 30.0.50; tree-sitter indentation in some loops and conditional statements is wrong

[Eli Zaretskii]

> Cc: 59686@debbugs.gnu.org
> Date: Fri, 02 Dec 2022 06:42:03 +0100
> From:  Theodor Thornhill via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> >> // -*- c-ts -*-
> >> int main() {
> >>    for (int i=0; i<10; ++i) {
> >>      foo(i);
> >>    }
> >>
> >>    for (int i=0;
> >>           i<10;
> >>           ++i) {
> >>             foo(i);
> >>           }
> >> }
> >
> >I see, that’s because the indent rule finds the BOL of the line where
> >the "{" is on, and indents from there.  Theo, WDYT? Does indent style 
> >fix this, or we should change the indent rules?
> >
> >Yuan
> 
> I've seen this issue myself, and have tried several combinations to fix it. It is trivial to fix this particular case, but because compound_statement is used everywhere problems will pop up other places. The fix here would be some grand-parent-bol function, but if my memory serves that would mess up the else in an if else. I'm happy to see this fixed, but just remember that it's not _super_ trivial.
> 
> Other languages exhibit similar issues, but with other constructs.

FWIW, this is an unusual style, so I see no catastrophe if it is not 110%
according to expectations.  Users can easily fix that by tweaking their BOLs
where important.

bug#59686: 30.0.50; tree-sitter indentation in some loops and conditional statements is wrong

[Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 59686@debbugs.gnu.org
>> Date: Fri, 02 Dec 2022 06:42:03 +0100
>> From:  Theodor Thornhill via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> >> // -*- c-ts -*-
>> >> int main() {
>> >>    for (int i=0; i<10; ++i) {
>> >>      foo(i);
>> >>    }
>> >>
>> >>    for (int i=0;
>> >>           i<10;
>> >>           ++i) {
>> >>             foo(i);
>> >>           }
>> >> }
>> >
>> >I see, that’s because the indent rule finds the BOL of the line where
>> >the "{" is on, and indents from there.  Theo, WDYT? Does indent style 
>> >fix this, or we should change the indent rules?
>> >
>> >Yuan
>> 
>> I've seen this issue myself, and have tried several combinations to fix it. It is trivial to fix this particular case, but because compound_statement is used everywhere problems will pop up other places. The fix here would be some grand-parent-bol function, but if my memory serves that would mess up the else in an if else. I'm happy to see this fixed, but just remember that it's not _super_ trivial.
>> 
>> Other languages exhibit similar issues, but with other constructs.
>
> FWIW, this is an unusual style, so I see no catastrophe if it is not 110%
> according to expectations.  Users can easily fix that by tweaking their BOLs
> where important.

Yeah, that was my reasoning as well.  I think we should come up with
some preset that can do this, but maybe when we know even more about how
to deal with similar edge cases?

bug#59686: 30.0.50; tree-sitter indentation in some loops and conditional statements is wrong

[Bruce Stephens]

On 02/12/2022 08:39, Eli Zaretskii wrote:

> FWIW, this is an unusual style, so I see no catastrophe if it is not 110%
> according to expectations.  Users can easily fix that by tweaking their BOLs
> where important.


The example I gave would be unusual, I think, but I'd argue that the 
situations where I saw the problem are quite natural.

For example,

                 } else if ( MYSTRCMP (attname, SOME_PREFIX_X400ADDRESS) ||
                             MYSTRCMP (attname, SOME_PREFIX_X400) ) {
                     FOO_ptr orp = foo_std2foo (val);

or a function declaration with several arguments with types that are 
rather long.

I agree it's not a critical bug but if there's no appropriate general 
fix it would be helpful to have some guidance for users to resolve our 
specific cases.

bug#59686: 30.0.50; tree-sitter indentation in some loops and conditional statements is wrong

[Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Bruce Stephens <bruce.stephens@isode.com> writes:

> On 02/12/2022 08:39, Eli Zaretskii wrote:
>
>> FWIW, this is an unusual style, so I see no catastrophe if it is not 110%
>> according to expectations.  Users can easily fix that by tweaking their BOLs
>> where important.
>
>
> The example I gave would be unusual, I think, but I'd argue that the 
> situations where I saw the problem are quite natural.
>
> For example,
>
>                  } else if ( MYSTRCMP (attname, SOME_PREFIX_X400ADDRESS) ||
>                              MYSTRCMP (attname, SOME_PREFIX_X400) ) {
>                      FOO_ptr orp = foo_std2foo (val);
>
> or a function declaration with several arguments with types that are 
> rather long.
>
> I agree it's not a critical bug but if there's no appropriate general 
> fix it would be helpful to have some guidance for users to resolve our 
> specific cases.

This is the case I was thinking of.  In the for-loop a grand-parent-bol
on compound_statement rule would match the 'for' keyword, so the
indentation will be correct, but this one will not, IIRC.  I plan to dig
into this some more soon, but motivation left me a little on that issue.
Maybe we could make a preset like:

```
(seq
 (parent-is "compound_statement") parent (parent-is "for_statement") bol)
```

In other words, make other presets execute sequentially, move point,
check again, and if all are true, pick indent offset.  Or allow multiple
captures, like so:

```
(for_statement @offset-anchor
  body: (compound_statement (_) @to-indent))
```

Here the @to-indent capture would get the new indent level based on
treesit-node-start of for_statement.

What do you think, Yuan?

Theo

bug#59686: 30.0.50; tree-sitter indentation in some loops and conditional statements is wrong

[Yuan Fu]

Theodor Thornhill <theo@thornhill.no> writes:

> Bruce Stephens <bruce.stephens@isode.com> writes:
>
>> On 02/12/2022 08:39, Eli Zaretskii wrote:
>>
>>> FWIW, this is an unusual style, so I see no catastrophe if it is not 110%
>>> according to expectations.  Users can easily fix that by tweaking their BOLs
>>> where important.
>>
>>
>> The example I gave would be unusual, I think, but I'd argue that the
>> situations where I saw the problem are quite natural.
>>
>> For example,
>>
>>                  } else if ( MYSTRCMP (attname, SOME_PREFIX_X400ADDRESS) ||
>>                              MYSTRCMP (attname, SOME_PREFIX_X400) ) {
>>                      FOO_ptr orp = foo_std2foo (val);
>>
>> or a function declaration with several arguments with types that are
>> rather long.
>>
>> I agree it's not a critical bug but if there's no appropriate general
>> fix it would be helpful to have some guidance for users to resolve our
>> specific cases.
>
> This is the case I was thinking of.  In the for-loop a grand-parent-bol
> on compound_statement rule would match the 'for' keyword, so the
> indentation will be correct, but this one will not, IIRC.  I plan to dig
> into this some more soon, but motivation left me a little on that issue.
> Maybe we could make a preset like:
>
> ```
> (seq
>  (parent-is "compound_statement") parent (parent-is "for_statement") bol)
> ```
>
>
> In other words, make other presets execute sequentially, move point,
> check again, and if all are true, pick indent offset.  Or allow multiple
> captures, like so:
>
> ```
> (for_statement @offset-anchor
>   body: (compound_statement (_) @to-indent))
> ```
>
> Here the @to-indent capture would get the new indent level based on
> treesit-node-start of for_statement.
>
> What do you think, Yuan?

I think we can just test for the grandparent, there is an
(undocumented) matcher n-p-gp which matches parent and grandparent.

Yuan

bug#59686: 30.0.50; tree-sitter indentation in some loops and conditional statements is wrong

[Theodor Thornhill via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On 3 December 2022 11:48:34 CET, Yuan Fu <casouri@gmail.com> wrote:
>
>Theodor Thornhill <theo@thornhill.no> writes:
>
>> Bruce Stephens <bruce.stephens@isode.com> writes:
>>
>>> On 02/12/2022 08:39, Eli Zaretskii wrote:
>>>
>>>> FWIW, this is an unusual style, so I see no catastrophe if it is not 110%
>>>> according to expectations.  Users can easily fix that by tweaking their BOLs
>>>> where important.
>>>
>>>
>>> The example I gave would be unusual, I think, but I'd argue that the
>>> situations where I saw the problem are quite natural.
>>>
>>> For example,
>>>
>>>                  } else if ( MYSTRCMP (attname, SOME_PREFIX_X400ADDRESS) ||
>>>                              MYSTRCMP (attname, SOME_PREFIX_X400) ) {
>>>                      FOO_ptr orp = foo_std2foo (val);
>>>
>>> or a function declaration with several arguments with types that are
>>> rather long.
>>>
>>> I agree it's not a critical bug but if there's no appropriate general
>>> fix it would be helpful to have some guidance for users to resolve our
>>> specific cases.
>>
>> This is the case I was thinking of.  In the for-loop a grand-parent-bol
>> on compound_statement rule would match the 'for' keyword, so the
>> indentation will be correct, but this one will not, IIRC.  I plan to dig
>> into this some more soon, but motivation left me a little on that issue.
>> Maybe we could make a preset like:
>>
>> ```
>> (seq
>>  (parent-is "compound_statement") parent (parent-is "for_statement") bol)
>> ```
>>
>>
>> In other words, make other presets execute sequentially, move point,
>> check again, and if all are true, pick indent offset.  Or allow multiple
>> captures, like so:
>>
>> ```
>> (for_statement @offset-anchor
>>   body: (compound_statement (_) @to-indent))
>> ```
>>
>> Here the @to-indent capture would get the new indent level based on
>> treesit-node-start of for_statement.
>>
>> What do you think, Yuan?
>
>I think we can just test for the grandparent, there is an
>(undocumented) matcher n-p-gp which matches parent and grandparent.
>
>Yuan

Yeah I know, but that doesn't work in every case we see this behavior.

bug#59686: 30.0.50; tree-sitter indentation in some loops and conditional statements is wrong

[Yuan Fu]

> On Dec 3, 2022, at 3:08 AM, Theodor Thornhill <theo@thornhill.no> wrote:
> 
> 
> 
> On 3 December 2022 11:48:34 CET, Yuan Fu <casouri@gmail.com> wrote:
>> 
>> Theodor Thornhill <theo@thornhill.no> writes:
>> 
>>> Bruce Stephens <bruce.stephens@isode.com> writes:
>>> 
>>>> On 02/12/2022 08:39, Eli Zaretskii wrote:
>>>> 
>>>>> FWIW, this is an unusual style, so I see no catastrophe if it is not 110%
>>>>> according to expectations.  Users can easily fix that by tweaking their BOLs
>>>>> where important.
>>>> 
>>>> 
>>>> The example I gave would be unusual, I think, but I'd argue that the
>>>> situations where I saw the problem are quite natural.
>>>> 
>>>> For example,
>>>> 
>>>>                 } else if ( MYSTRCMP (attname, SOME_PREFIX_X400ADDRESS) ||
>>>>                             MYSTRCMP (attname, SOME_PREFIX_X400) ) {
>>>>                     FOO_ptr orp = foo_std2foo (val);
>>>> 
>>>> or a function declaration with several arguments with types that are
>>>> rather long.
>>>> 
>>>> I agree it's not a critical bug but if there's no appropriate general
>>>> fix it would be helpful to have some guidance for users to resolve our
>>>> specific cases.
>>> 
>>> This is the case I was thinking of.  In the for-loop a grand-parent-bol
>>> on compound_statement rule would match the 'for' keyword, so the
>>> indentation will be correct, but this one will not, IIRC.  I plan to dig
>>> into this some more soon, but motivation left me a little on that issue.
>>> Maybe we could make a preset like:
>>> 
>>> ```
>>> (seq
>>> (parent-is "compound_statement") parent (parent-is "for_statement") bol)
>>> ```
>>> 
>>> 
>>> In other words, make other presets execute sequentially, move point,
>>> check again, and if all are true, pick indent offset.  Or allow multiple
>>> captures, like so:
>>> 
>>> ```
>>> (for_statement @offset-anchor
>>>  body: (compound_statement (_) @to-indent))
>>> ```
>>> 
>>> Here the @to-indent capture would get the new indent level based on
>>> treesit-node-start of for_statement.
>>> 
>>> What do you think, Yuan?
>> 
>> I think we can just test for the grandparent, there is an
>> (undocumented) matcher n-p-gp which matches parent and grandparent.
>> 
>> Yuan
> 
> Yeah I know, but that doesn't work in every case we see this behavior.

I see, but at least it fixes common cases that I can think of right now, namely if, for, while. What are some other cases?

Yuan