bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions

bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions

[Evgeni Kolev]

M-x mark-defun doesn't work correctly when the function at point is
preceded by a comment.

I've noticed this bug in go-ts-mode, and have also checked it's
reproduced in rust-ts-mode:

To reproduce:
1. with the following code in go-ts-mode, "|" is the point (in function sum2)
```
package main

func sum(a, b int) int {
    return a + b
}

// comment
func sum2(a, b int) int {
    Ireturn a + b
}
```
2. Execute M-x mark-defun
3. The region selected is wrong - the empty line between the two
functions is marked. I expect only function sum2 and the preceding
comment to be marked.

Observations:
- The issue is reproducible with a fresh build of master, and
reproducible with emacs -Q.
- The issue is also reproducible in rust-ts-mode which makes me think
the root cause is in treesit code (treesit.el maybe).
- From what I've checked, (beginning-of-defun-comments) works
correctly, but (end-of-defun) doesn't. These two (and other) functions
are used by mark-defun.
- The issue does not exist when the function is not preceded by a
comment. In other words - the issue is not reproduced when no comments
exist between the functions.

Below is a Rust example which reproduces the issue, again "|" is the
point (in function main2). Steps to reproduce are identical to the
steps for go-ts-mode:
```
fn main() {
    println!("Hello World!");
}

// comment
fn main2() {
    |println!("Hello World!");
}
```

bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions

[Evgeni Kolev]

Sorry, I have a typo in my Go example, the point in the code is "I"
(capital i), instead of "|" (pipe). The Rust example is OK.

Fixed steps to reproduce for go-ts-mode:

To reproduce:
1. with the following code in go-ts-mode, "|" is the point (in function sum2)
```
package main

func sum(a, b int) int {
    return a + b
}

// comment
func sum2(a, b int) int {
    |return a + b
}
```
2. Execute M-x mark-defun
3. The region selected is wrong - the empty line between the two
functions is marked. I expect only function sum2 and the preceding
comment to be marked.

bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions

[Yuan Fu]

Evgeni Kolev <evgenysw@gmail.com> writes:

> Sorry, I have a typo in my Go example, the point in the code is "I"
> (capital i), instead of "|" (pipe). The Rust example is OK.
>
> Fixed steps to reproduce for go-ts-mode:
>
> To reproduce:
> 1. with the following code in go-ts-mode, "|" is the point (in function sum2)
> ```
> package main
>
> func sum(a, b int) int {
>     return a + b
> }
>
> // comment
> func sum2(a, b int) int {
>     |return a + b
> }
> ```
> 2. Execute M-x mark-defun
> 3. The region selected is wrong - the empty line between the two
> functions is marked. I expect only function sum2 and the preceding
> comment to be marked.

Huh, with or without comments, mark-defun always includes the empty
lines before the defun for me. I get the same behavior in rust-ts-mode.
This seems intentional, because this is at the end of the definition of
mark-defun:


(skip-chars-backward "[:space:]\n")
(unless (bobp)
  (forward-line 1))

Are you using emacs-29 or emacs-30? Theo might have changed something on master.

Yuan

bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions

[Evgeni Kolev]

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

On Sat, Feb 25, 2023 at 7:08 AM Yuan Fu <casouri@gmail.com> wrote:

> Huh, with or without comments, mark-defun always includes the empty
> lines before the defun for me. I get the same behavior in rust-ts-mode.
> This seems intentional, because this is at the end of the definition of
> mark-defun:
>
> (skip-chars-backward "[:space:]\n")
> (unless (bobp)
>   (forward-line 1))

Did you check with my example or another example? With my example, the
issue I get is that only the empty lines are marked, without the
defun, without the comments. I'm attaching two screenshots - before
and after mark-defun.

Note: I get the correct behaviour when there is just one defun in the
file. But if there are more, or the defun at point is not at the top
of the file - mark-defun does not work as expected.

>
> Are you using emacs-29 or emacs-30? Theo might have changed something on master.

I've observed the issue on both. If you can't reproduce it - I'll
re-test on a fresh docker image to make sure the issue is not in my
setup. However, I'm pretty sure it's not in my setup because I used a
freshly built emacs just for this purpose (emacs-30 maybe, I'm not
sure), and ran it with emacs -Q.

[-- Attachment #2: before-mark-defun.png --]
[-- Type: image/png, Size: 88615 bytes --]

[-- Attachment #3: after-mark-defun.png --]
[-- Type: image/png, Size: 85871 bytes --]

bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions

[Yuan Fu]

> On Feb 24, 2023, at 11:27 PM, Evgeni Kolev <evgenysw@gmail.com> wrote:
> 
> On Sat, Feb 25, 2023 at 7:08 AM Yuan Fu <casouri@gmail.com> wrote:
> 
>> Huh, with or without comments, mark-defun always includes the empty
>> lines before the defun for me. I get the same behavior in rust-ts-mode.
>> This seems intentional, because this is at the end of the definition of
>> mark-defun:
>> 
>> (skip-chars-backward "[:space:]\n")
>> (unless (bobp)
>>  (forward-line 1))
> 
> Did you check with my example or another example? With my example, the
> issue I get is that only the empty lines are marked, without the
> defun, without the comments. I'm attaching two screenshots - before
> and after mark-defun.
> 
> Note: I get the correct behaviour when there is just one defun in the
> file. But if there are more, or the defun at point is not at the top
> of the file - mark-defun does not work as expected.

Sorry, I used wrong go mode when testing your recipe. I used go-mode thinking it is go-ts-mode :-( Using go-ts-mode, I can reproduce what you see, yes.

The problem you discovered revealed some shortcoming in our tree-sitter navigation function, and requires quite a few non-trivial changes, I working on fixing it.

The direct cause is that (treesit-beginning-of-defun -1) is supposed to go to the beginning of next defun, but instead goes to the beginning of next next defun. That was a design decision I made but I can see it causing old functions to misbehave. 

Yuan

bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions

[Yuan Fu]

> On Feb 26, 2023, at 4:42 PM, Yuan Fu <casouri@gmail.com> wrote:
> 
> 
> 
>> On Feb 24, 2023, at 11:27 PM, Evgeni Kolev <evgenysw@gmail.com> wrote:
>> 
>> On Sat, Feb 25, 2023 at 7:08 AM Yuan Fu <casouri@gmail.com> wrote:
>> 
>>> Huh, with or without comments, mark-defun always includes the empty
>>> lines before the defun for me. I get the same behavior in rust-ts-mode.
>>> This seems intentional, because this is at the end of the definition of
>>> mark-defun:
>>> 
>>> (skip-chars-backward "[:space:]\n")
>>> (unless (bobp)
>>> (forward-line 1))
>> 
>> Did you check with my example or another example? With my example, the
>> issue I get is that only the empty lines are marked, without the
>> defun, without the comments. I'm attaching two screenshots - before
>> and after mark-defun.
>> 
>> Note: I get the correct behaviour when there is just one defun in the
>> file. But if there are more, or the defun at point is not at the top
>> of the file - mark-defun does not work as expected.
> 
> Sorry, I used wrong go mode when testing your recipe. I used go-mode thinking it is go-ts-mode :-( Using go-ts-mode, I can reproduce what you see, yes.
> 
> The problem you discovered revealed some shortcoming in our tree-sitter navigation function, and requires quite a few non-trivial changes, I working on fixing it.
> 
> The direct cause is that (treesit-beginning-of-defun -1) is supposed to go to the beginning of next defun, but instead goes to the beginning of next next defun. That was a design decision I made but I can see it causing old functions to misbehave. 
> 
> Yuan

The fix is brain-twisting but don’t embody a lot of code, yay! This now should be fixed.

Yuan

bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions

[Evgeni Kolev]

Great, thanks for the fix! I tested a few common (for me) scenarios
and mark-defun works as expected!

On Mon, Feb 27, 2023 at 11:24 AM Yuan Fu <casouri@gmail.com> wrote:
>
>
>
> > On Feb 26, 2023, at 4:42 PM, Yuan Fu <casouri@gmail.com> wrote:
> >
> >
> >
> >> On Feb 24, 2023, at 11:27 PM, Evgeni Kolev <evgenysw@gmail.com> wrote:
> >>
> >> On Sat, Feb 25, 2023 at 7:08 AM Yuan Fu <casouri@gmail.com> wrote:
> >>
> >>> Huh, with or without comments, mark-defun always includes the empty
> >>> lines before the defun for me. I get the same behavior in rust-ts-mode.
> >>> This seems intentional, because this is at the end of the definition of
> >>> mark-defun:
> >>>
> >>> (skip-chars-backward "[:space:]\n")
> >>> (unless (bobp)
> >>> (forward-line 1))
> >>
> >> Did you check with my example or another example? With my example, the
> >> issue I get is that only the empty lines are marked, without the
> >> defun, without the comments. I'm attaching two screenshots - before
> >> and after mark-defun.
> >>
> >> Note: I get the correct behaviour when there is just one defun in the
> >> file. But if there are more, or the defun at point is not at the top
> >> of the file - mark-defun does not work as expected.
> >
> > Sorry, I used wrong go mode when testing your recipe. I used go-mode thinking it is go-ts-mode :-( Using go-ts-mode, I can reproduce what you see, yes.
> >
> > The problem you discovered revealed some shortcoming in our tree-sitter navigation function, and requires quite a few non-trivial changes, I working on fixing it.
> >
> > The direct cause is that (treesit-beginning-of-defun -1) is supposed to go to the beginning of next defun, but instead goes to the beginning of next next defun. That was a design decision I made but I can see it causing old functions to misbehave.
> >
> > Yuan
>
> The fix is brain-twisting but don’t embody a lot of code, yay! This now should be fixed.
>
> Yuan