bug#60127: treesit-end-of-defun: possible bug with clojure grammar

bug#60127: treesit-end-of-defun: possible bug with clojure grammar

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

Hello,

I have been running into what I believe is a bug with
treesit-end-of-defun while developing clojure-ts-mode. There are some
positions in clojure buffers where treesit-end-of-defun will jump to the
end of the buffer.

Steps to reproduce:

Checkout the emacs-29 branch. I have built mine with commit
b01d0246d71a7a3fd92b2864a3c0c0bc9367ee0b and tree-sitter version 0.20.7

Clone my clojure-mode fork:
https://github.com/dannyfreeman/clojure-mode

check out the `end-of-defun-bug` branch (master works too, but I added
a helpful debug message in this branch)

Clone tree-sitter-module repo:
https://github.com/casouri/tree-sitter-module

Apply the patch `tree-sitter-module-clojure-support.patch` located in
clojure-mode repo to the casouri/tree-sitter-module repo.

Build the treesitter parser for clojure with

$ ./build clojure

Start emacs

$ emacs -q

Evaluate the following, replacing the paths with what is relevant to
your system

```
(progn
  (setq treesit-extra-load-path '("~/path/to/tree-sitter-module/dist"))
  (add-to-list 'load-path "~/path/to/clojure-mode/")
  (find-file "~/path/to/clojure-mode/clojure-ts-mode.el")
  (eval-buffer)
  (find-file "~/path/to/clojure-mode/test_end_of_defun.clj"))
```

Once you have opened test_end_of_defun.clj you can see the problem by
pressing

M-<
C-M-e
C-M-e

You should see the cursor move from

```
|(def w 1)

(def x 2)

(def y 3)

(def skip-to-here? 4)
```

to
```
(def w 1)
|
(def x 2)

(def y 3)

(def skip-to-here? 4)
```

then it unexpectedly jumps to the end of the buffer
```
(def w 1)

(def x 2)

(def y 3)

(def skip-to-here? 4)
|
```

The message buffer shows that when the point is between the forms

```
(def w 1)
|
(def x 2)
```

that `treesit-end-of-defun` thinks the current node is
`(def skip-to-here? 4)`, which does not seem right.

When the cursor is at other points in this buffer, this problem doesn't
seem to occur. For instance, between the (def x 2) and (def y 3) forms,
this doesn't happen.

I can also see this happening in the `test.clj` file of the clojure-mode
repo in a different spot but there is a lot more going on in that file.
(hit C-M-e in it until the problem occurs if you are curious).

I have the following vars relevant vars set in clojure-ts-mode.
Different combinations of them yield the same results.

```
(setq-local treesit-defun-prefer-top-level t
            treesit-defun-tactic 'top-level
            treesit-defun-type-regexp (cons (rx (or "list_lit" "vec_lit" "map_lit"))
                                            (lambda (node)
                                              (message "Node: %s" (treesit-node-text node t))
                                              t)))
```

The clojure code in question produces an error free parse tree. I can
see it with `treesit-explore-mode` and by running the file through
`tree-sitter parse`, which gives the following parse tree:

```
(source [0, 0] - [7, 0]
  (list_lit [0, 0] - [0, 9]
    value: (sym_lit [0, 1] - [0, 4]
      name: (sym_name [0, 1] - [0, 4]))
    value: (sym_lit [0, 5] - [0, 6]
      name: (sym_name [0, 5] - [0, 6]))
    value: (num_lit [0, 7] - [0, 8]))
  (list_lit [2, 0] - [2, 9]
    value: (sym_lit [2, 1] - [2, 4]
      name: (sym_name [2, 1] - [2, 4]))
    value: (sym_lit [2, 5] - [2, 6]
      name: (sym_name [2, 5] - [2, 6]))
    value: (num_lit [2, 7] - [2, 8]))
  (list_lit [4, 0] - [4, 9]
    value: (sym_lit [4, 1] - [4, 4]
      name: (sym_name [4, 1] - [4, 4]))
    value: (sym_lit [4, 5] - [4, 6]
      name: (sym_name [4, 5] - [4, 6]))
    value: (num_lit [4, 7] - [4, 8]))
  (list_lit [6, 0] - [6, 21]
    value: (sym_lit [6, 1] - [6, 4]
      name: (sym_name [6, 1] - [6, 4]))
    value: (sym_lit [6, 5] - [6, 18]
      name: (sym_name [6, 5] - [6, 18]))
    value: (num_lit [6, 19] - [6, 20])))
```

Any help or advice here is appreciated.

Thank you,
-- 
Danny Freeman

bug#60127: treesit-end-of-defun: possible bug with clojure grammar

[Yuan Fu]

Danny Freeman <Danny@dfreeman.email> writes:

> Hello,
>
> I have been running into what I believe is a bug with
> treesit-end-of-defun while developing clojure-ts-mode. There are some
> positions in clojure buffers where treesit-end-of-defun will jump to the
> end of the buffer.
>
> Steps to reproduce:
>
> Checkout the emacs-29 branch. I have built mine with commit
> b01d0246d71a7a3fd92b2864a3c0c0bc9367ee0b and tree-sitter version 0.20.7
>
> Clone my clojure-mode fork:
> https://github.com/dannyfreeman/clojure-mode
>
> check out the `end-of-defun-bug` branch (master works too, but I added
> a helpful debug message in this branch)
>
> Clone tree-sitter-module repo:
> https://github.com/casouri/tree-sitter-module
>
> Apply the patch `tree-sitter-module-clojure-support.patch` located in
> clojure-mode repo to the casouri/tree-sitter-module repo.
>
> Build the treesitter parser for clojure with
>
> $ ./build clojure
>
> Start emacs
>
> $ emacs -q
>
> Evaluate the following, replacing the paths with what is relevant to
> your system
>
> ```
> (progn
>   (setq treesit-extra-load-path '("~/path/to/tree-sitter-module/dist"))
>   (add-to-list 'load-path "~/path/to/clojure-mode/")
>   (find-file "~/path/to/clojure-mode/clojure-ts-mode.el")
>   (eval-buffer)
>   (find-file "~/path/to/clojure-mode/test_end_of_defun.clj"))
> ```
>
>
> Once you have opened test_end_of_defun.clj you can see the problem by
> pressing
>
> M-<
> C-M-e
> C-M-e
>
> You should see the cursor move from
>
> ```
> |(def w 1)
>
> (def x 2)
>
> (def y 3)
>
> (def skip-to-here? 4)
> ```
>
> to
>
> ```
> (def w 1)
> |
> (def x 2)
>
> (def y 3)
>
> (def skip-to-here? 4)
> ```
>
>
> then it unexpectedly jumps to the end of the buffer
>
> ```
> (def w 1)
>
> (def x 2)
>
> (def y 3)
>
> (def skip-to-here? 4)
> |
> ```
>
>
> The message buffer shows that when the point is between the forms
>
> ```
> (def w 1)
> |
> (def x 2)
> ```
>
>
> that `treesit-end-of-defun` thinks the current node is
> `(def skip-to-here? 4)`, which does not seem right.
>
> When the cursor is at other points in this buffer, this problem doesn't
> seem to occur. For instance, between the (def x 2) and (def y 3) forms,
> this doesn't happen.
>
> I can also see this happening in the `test.clj` file of the clojure-mode
> repo in a different spot but there is a lot more going on in that file.
> (hit C-M-e in it until the problem occurs if you are curious).
>
> I have the following vars relevant vars set in clojure-ts-mode.
> Different combinations of them yield the same results.
>
> ```
> (setq-local treesit-defun-prefer-top-level t
>             treesit-defun-tactic 'top-level
>             treesit-defun-type-regexp (cons (rx (or "list_lit" "vec_lit" "map_lit"))
>                                             (lambda (node)
>                                               (message "Node: %s" (treesit-node-text node t))
>                                               t)))
> ```
>
>
> The clojure code in question produces an error free parse tree. I can
> see it with `treesit-explore-mode` and by running the file through
> `tree-sitter parse`, which gives the following parse tree:
>
> ```
> (source [0, 0] - [7, 0]
>   (list_lit [0, 0] - [0, 9]
>     value: (sym_lit [0, 1] - [0, 4]
>       name: (sym_name [0, 1] - [0, 4]))
>     value: (sym_lit [0, 5] - [0, 6]
>       name: (sym_name [0, 5] - [0, 6]))
>     value: (num_lit [0, 7] - [0, 8]))
>   (list_lit [2, 0] - [2, 9]
>     value: (sym_lit [2, 1] - [2, 4]
>       name: (sym_name [2, 1] - [2, 4]))
>     value: (sym_lit [2, 5] - [2, 6]
>       name: (sym_name [2, 5] - [2, 6]))
>     value: (num_lit [2, 7] - [2, 8]))
>   (list_lit [4, 0] - [4, 9]
>     value: (sym_lit [4, 1] - [4, 4]
>       name: (sym_name [4, 1] - [4, 4]))
>     value: (sym_lit [4, 5] - [4, 6]
>       name: (sym_name [4, 5] - [4, 6]))
>     value: (num_lit [4, 7] - [4, 8]))
>   (list_lit [6, 0] - [6, 21]
>     value: (sym_lit [6, 1] - [6, 4]
>       name: (sym_name [6, 1] - [6, 4]))
>     value: (sym_lit [6, 5] - [6, 18]
>       name: (sym_name [6, 5] - [6, 18]))
>     value: (num_lit [6, 19] - [6, 20])))
> ```
>
> Any help or advice here is appreciated.
>
> Thank you,

Thank you very much for this detailed report! This seems to be some
problem with either tree-sitter or tree-sitter-clojure, where
treesit-node-first-child-for-pos gives wrong result. We’ve had similar
problem with bash before[1].

At this point there aren’t much we can do in Emacs. If I find some time
I’ll make a reproduce recipe and submit an issue on tree-sitter-clojure.

[1] https://github.com/tree-sitter/tree-sitter-bash/issues/139

Yuan

bug#60127: treesit-end-of-defun: possible bug with clojure grammar

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

Yuan Fu <casouri@gmail.com> writes:

> Thank you very much for this detailed report! This seems to be some
> problem with either tree-sitter or tree-sitter-clojure, where
> treesit-node-first-child-for-pos gives wrong result. We’ve had similar
> problem with bash before[1].
>
> At this point there aren’t much we can do in Emacs. If I find some time
> I’ll make a reproduce recipe and submit an issue on tree-sitter-clojure.
>
> [1] https://github.com/tree-sitter/tree-sitter-bash/issues/139
>
> Yuan

This bash issue will be really helpful, thanks for sharing.
I've been doing work on tree-sitter-clojure, so I can start digging over
there. You don't need to worry about writing up a recipe unless you
really want to. (I will just test with Emacs since I do not know C very
well).

-- 
Danny Freeman

bug#60127: treesit-end-of-defun: possible bug with clojure grammar

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

Danny Freeman <danny@dfreeman.email> writes:


>> Thank you very much for this detailed report! This seems to be some
>> problem with either tree-sitter or tree-sitter-clojure, where
>> treesit-node-first-child-for-pos gives wrong result. We’ve had similar
>> problem with bash before[1].
>>
>> At this point there aren’t much we can do in Emacs. If I find some time
>> I’ll make a reproduce recipe and submit an issue on tree-sitter-clojure.
>>
>> [1] https://github.com/tree-sitter/tree-sitter-bash/issues/139
>>
>> Yuan

I have a c program that I believe reproduces the problem. I'm not sure
how we will go about fixing it in tree-sitter-clojure, but at least we
have a minimal way to re-create it.

https://paste.sr.ht/~dannyfreeman/8786059276c334036877159a5b0872cd2aa6fbc0

-- 
Danny Freeman

bug#60127: treesit-end-of-defun: possible bug with clojure grammar

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

Yuan,

>> Thank you very much for this detailed report! This seems to be some
>> problem with either tree-sitter or tree-sitter-clojure, where
>> treesit-node-first-child-for-pos gives wrong result. We’ve had similar
>> problem with bash before[1].
>>
>> At this point there aren’t much we can do in Emacs. If I find some time
>> I’ll make a reproduce recipe and submit an issue on tree-sitter-clojure.
>>
>> [1] https://github.com/tree-sitter/tree-sitter-bash/issues/139
>>
>> Yuan

I have a c program that I believe reproduces the problem. I'm not sure
how we will go about fixing it in tree-sitter-clojure, but at least we
have a minimal way to re-create it.

Here is a link to the issue I created:
https://github.com/sogaiu/tree-sitter-clojure/issues/32
that also includes the c-program. I've also put it on my source hut
https://paste.sr.ht/~dannyfreeman/8786059276c334036877159a5b0872cd2aa6fbc0 

If you find the time, I would appreciate it if you could confirm my
findings. I'm a little in over my head with this bug I'm afraid.

-- 
Danny Freeman

bug#60127: treesit-end-of-defun: possible bug with clojure grammar

[Yuan Fu]

Danny Freeman <danny@dfreeman.email> writes:

> Yuan,
>
>>> Thank you very much for this detailed report! This seems to be some
>>> problem with either tree-sitter or tree-sitter-clojure, where
>>> treesit-node-first-child-for-pos gives wrong result. We’ve had similar
>>> problem with bash before[1].
>>>
>>> At this point there aren’t much we can do in Emacs. If I find some time
>>> I’ll make a reproduce recipe and submit an issue on tree-sitter-clojure.
>>>
>>> [1] https://github.com/tree-sitter/tree-sitter-bash/issues/139
>>>
>>> Yuan
>
> I have a c program that I believe reproduces the problem. I'm not sure
> how we will go about fixing it in tree-sitter-clojure, but at least we
> have a minimal way to re-create it.
>
> Here is a link to the issue I created:
> https://github.com/sogaiu/tree-sitter-clojure/issues/32
> that also includes the c-program. I've also put it on my source hut
> https://paste.sr.ht/~dannyfreeman/8786059276c334036877159a5b0872cd2aa6fbc0 
>
> If you find the time, I would appreciate it if you could confirm my
> findings. I'm a little in over my head with this bug I'm afraid.

I think it’s pretty good (the reproduce), the author’s modified program
is even better. Let’s wait and see what tree-sitter’s author have to
say.

Yuan

bug#60127: treesit-end-of-defun: possible bug with clojure grammar

[Dmitry Gutov]

On 22/12/2022 10:55, Yuan Fu wrote:
> Danny Freeman<danny@dfreeman.email>  writes:
> 
>> Yuan,
>>
>>>> Thank you very much for this detailed report! This seems to be some
>>>> problem with either tree-sitter or tree-sitter-clojure, where
>>>> treesit-node-first-child-for-pos gives wrong result. We’ve had similar
>>>> problem with bash before[1].
>>>>
>>>> At this point there aren’t much we can do in Emacs. If I find some time
>>>> I’ll make a reproduce recipe and submit an issue on tree-sitter-clojure.
>>>>
>>>> [1]https://github.com/tree-sitter/tree-sitter-bash/issues/139
>>>>
>>>> Yuan
>> I have a c program that I believe reproduces the problem. I'm not sure
>> how we will go about fixing it in tree-sitter-clojure, but at least we
>> have a minimal way to re-create it.
>>
>> Here is a link to the issue I created:
>> https://github.com/sogaiu/tree-sitter-clojure/issues/32
>> that also includes the c-program. I've also put it on my source hut
>> https://paste.sr.ht/~dannyfreeman/8786059276c334036877159a5b0872cd2aa6fbc0  
>>
>> If you find the time, I would appreciate it if you could confirm my
>> findings. I'm a little in over my head with this bug I'm afraid.
> I think it’s pretty good (the reproduce), the author’s modified program
> is even better. Let’s wait and see what tree-sitter’s author have to
> say.

Just to keep everyone of the same page:

clojure-tree-sitter's author posted an issue to tree-sitter last week: 
https://github.com/tree-sitter/tree-sitter/issues/2012

and, previously, he also suggested we might be able to work around it 
using treesit_search_dfs: 
https://github.com/sogaiu/tree-sitter-clojure/issues/32#issuecomment-1364830835

bug#60127: treesit-end-of-defun: possible bug with clojure grammar

[Yuan Fu]

> On Jan 15, 2023, at 5:56 PM, Dmitry Gutov <dgutov@yandex.ru> wrote:
> 
> On 22/12/2022 10:55, Yuan Fu wrote:
>> Danny Freeman<danny@dfreeman.email>  writes:
>>> Yuan,
>>> 
>>>>> Thank you very much for this detailed report! This seems to be some
>>>>> problem with either tree-sitter or tree-sitter-clojure, where
>>>>> treesit-node-first-child-for-pos gives wrong result. We’ve had similar
>>>>> problem with bash before[1].
>>>>> 
>>>>> At this point there aren’t much we can do in Emacs. If I find some time
>>>>> I’ll make a reproduce recipe and submit an issue on tree-sitter-clojure.
>>>>> 
>>>>> [1]https://github.com/tree-sitter/tree-sitter-bash/issues/139
>>>>> 
>>>>> Yuan
>>> I have a c program that I believe reproduces the problem. I'm not sure
>>> how we will go about fixing it in tree-sitter-clojure, but at least we
>>> have a minimal way to re-create it.
>>> 
>>> Here is a link to the issue I created:
>>> https://github.com/sogaiu/tree-sitter-clojure/issues/32
>>> that also includes the c-program. I've also put it on my source hut
>>> https://paste.sr.ht/~dannyfreeman/8786059276c334036877159a5b0872cd2aa6fbc0  
>>> If you find the time, I would appreciate it if you could confirm my
>>> findings. I'm a little in over my head with this bug I'm afraid.
>> I think it’s pretty good (the reproduce), the author’s modified program
>> is even better. Let’s wait and see what tree-sitter’s author have to
>> say.
> 
> Just to keep everyone of the same page:
> 
> clojure-tree-sitter's author posted an issue to tree-sitter last week: https://github.com/tree-sitter/tree-sitter/issues/2012
> 
> and, previously, he also suggested we might be able to work around it using treesit_search_dfs: https://github.com/sogaiu/tree-sitter-clojure/issues/32#issuecomment-1364830835

I’d rather not use treesit_search_dfs, as it does much more work than the current code. You know what, I can write a custom ts_node_first_child_for_byte and use that in Emacs, I’ve already done that for ts_node_parent anyway.

Yuan

bug#60127: treesit-end-of-defun: possible bug with clojure grammar

[Yuan Fu]

> I’d rather not use treesit_search_dfs, as it does much more work than
> the current code. You know what, I can write a custom
> ts_node_first_child_for_byte and use that in Emacs, I’ve already done
> that for ts_node_parent anyway.

I did that. I tested with the Clojure example and it works fine now.

Eli, we don’t use ts_node_first_child_for_pos for now until tree-sitter
fix that function, should I removed the boilerplate for it or should I
leave it as-is? I saw you removed ts_node_parent when I did a similar
fix.

Yuan

bug#60127: treesit-end-of-defun: possible bug with clojure grammar

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Tue, 17 Jan 2023 01:30:22 -0800
> Cc: 60127@debbugs.gnu.org,
>  Danny Freeman <danny@dfreeman.email>,
>  Eli Zaretskii <eliz@gnu.org>
> 
> 
> > I’d rather not use treesit_search_dfs, as it does much more work than
> > the current code. You know what, I can write a custom
> > ts_node_first_child_for_byte and use that in Emacs, I’ve already done
> > that for ts_node_parent anyway.
> 
> I did that. I tested with the Clojure example and it works fine now.
> 
> Eli, we don’t use ts_node_first_child_for_pos for now until tree-sitter
> fix that function, should I removed the boilerplate for it or should I
> leave it as-is? I saw you removed ts_node_parent when I did a similar
> fix.

It should be either removed or #ifdef'ed away, because otherwise GCC
complains in the MS-Windows build about unused macro.

bug#60127: treesit-end-of-defun: possible bug with clojure grammar

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Yuan Fu <casouri@gmail.com>
>> Date: Tue, 17 Jan 2023 01:30:22 -0800
>> Cc: 60127@debbugs.gnu.org,
>>  Danny Freeman <danny@dfreeman.email>,
>>  Eli Zaretskii <eliz@gnu.org>
>> 
>> 
>> > I’d rather not use treesit_search_dfs, as it does much more work than
>> > the current code. You know what, I can write a custom
>> > ts_node_first_child_for_byte and use that in Emacs, I’ve already done
>> > that for ts_node_parent anyway.
>> 
>> I did that. I tested with the Clojure example and it works fine now.
>> 
>> Eli, we don’t use ts_node_first_child_for_pos for now until tree-sitter
>> fix that function, should I removed the boilerplate for it or should I
>> leave it as-is? I saw you removed ts_node_parent when I did a similar
>> fix.
>
> It should be either removed or #ifdef'ed away, because otherwise GCC
> complains in the MS-Windows build about unused macro.

Thanks for tackling this problem on the Emacs end Yuan. I tested it out
with clojure-ts-mode and it seems to have addressed my problems.
I also took the liberty of linking to your commit in the tree-sitter
github issue in case the maintainers want to take a look at it.

-- 
Danny Freeman

bug#60127: treesit-end-of-defun: possible bug with clojure grammar

[Stefan Kangas]

Danny Freeman <danny@dfreeman.email> writes:

> Thanks for tackling this problem on the Emacs end Yuan. I tested it out
> with clojure-ts-mode and it seems to have addressed my problems.
> I also took the liberty of linking to your commit in the tree-sitter
> github issue in case the maintainers want to take a look at it.

This bug seems to have been left open by mistake, so I'm closing it now.

Please reopen the bug if I missed something.