all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions
@ 2023-02-19  8:39 Evgeni Kolev
  2023-02-20  8:30 ` Evgeni Kolev
  2023-02-25  5:08 ` Yuan Fu
  0 siblings, 2 replies; 7+ messages in thread
From: Evgeni Kolev @ 2023-02-19  8:39 UTC (permalink / raw)
  To: 61617

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!");
}
```





^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions
  2023-02-19  8:39 bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions Evgeni Kolev
@ 2023-02-20  8:30 ` Evgeni Kolev
  2023-02-25  5:08 ` Yuan Fu
  1 sibling, 0 replies; 7+ messages in thread
From: Evgeni Kolev @ 2023-02-20  8:30 UTC (permalink / raw)
  To: 61617

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.





^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions
  2023-02-19  8:39 bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions Evgeni Kolev
  2023-02-20  8:30 ` Evgeni Kolev
@ 2023-02-25  5:08 ` Yuan Fu
  2023-02-25  7:27   ` Evgeni Kolev
  1 sibling, 1 reply; 7+ messages in thread
From: Yuan Fu @ 2023-02-25  5:08 UTC (permalink / raw)
  To: evgenysw; +Cc: Theodor Thornhill, 61617


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





^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions
  2023-02-25  5:08 ` Yuan Fu
@ 2023-02-25  7:27   ` Evgeni Kolev
  2023-02-27  0:42     ` Yuan Fu
  0 siblings, 1 reply; 7+ messages in thread
From: Evgeni Kolev @ 2023-02-25  7:27 UTC (permalink / raw)
  To: Yuan Fu; +Cc: Theodor Thornhill, 61617

[-- 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions
  2023-02-25  7:27   ` Evgeni Kolev
@ 2023-02-27  0:42     ` Yuan Fu
  2023-02-27  9:24       ` Yuan Fu
  0 siblings, 1 reply; 7+ messages in thread
From: Yuan Fu @ 2023-02-27  0:42 UTC (permalink / raw)
  To: Evgeni Kolev; +Cc: Theodor Thornhill, 61617



> 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




^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions
  2023-02-27  0:42     ` Yuan Fu
@ 2023-02-27  9:24       ` Yuan Fu
  2023-02-27 16:03         ` Evgeni Kolev
  0 siblings, 1 reply; 7+ messages in thread
From: Yuan Fu @ 2023-02-27  9:24 UTC (permalink / raw)
  To: Evgeni Kolev; +Cc: Theodor Thornhill, 61617



> 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




^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions
  2023-02-27  9:24       ` Yuan Fu
@ 2023-02-27 16:03         ` Evgeni Kolev
  0 siblings, 0 replies; 7+ messages in thread
From: Evgeni Kolev @ 2023-02-27 16:03 UTC (permalink / raw)
  To: Yuan Fu; +Cc: Theodor Thornhill, 61617

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





^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-02-27 16:03 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-19  8:39 bug#61617: M-x mark-defun doesn't work correctly in tree-sitter modes when comments exist between functions Evgeni Kolev
2023-02-20  8:30 ` Evgeni Kolev
2023-02-25  5:08 ` Yuan Fu
2023-02-25  7:27   ` Evgeni Kolev
2023-02-27  0:42     ` Yuan Fu
2023-02-27  9:24       ` Yuan Fu
2023-02-27 16:03         ` Evgeni Kolev

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.