bug#68664: 29.1.50; treesit defun commands broken with nested functions

bug#68664: 29.1.50; treesit defun commands broken with nested functions

[Troy Brown]

I've noticed that "defun" related treesit commands do not appear to work
correctly when nested functions are involved.  I've seen this behavior
in multiple languages and believe the problem is an issue in the
treesit.el library.

According to the Emacs manual section "Moving by Defuns", the
beginning/end-of-defun commands should "... find the beginning and end
of the innermost defun around point".  I don't see this behavior with
the corresponding treesit-beginning-of-defun and treesit-end-of-defun
commands.  The following example uses python-ts-mode to demonstrate the
issue.

--8<---------------cut here---------------start------------->8---
# -*- mode: python-ts -*-

def outerFunction(text):
    text = text

    def innerFunction(text):
        print(text)

    innerFunction()

    def innerFunction2(text):
        print(text)

    innerFunction2()

--8<---------------cut here---------------end--------------->8---

To reproduce the issue, place point on line 9 (i.e., the call to
"innerFunction").  When point is at this location, I've noticed
incorrect behavior for the following commands:

M-x treesit-beginning-of-defun RET

Moves point to the beginning of the "innerFunction" function (line 6)
instead of the beginning of the "outerFunction" function (line 3).

M-x treesit-end-of-defun RET

Moves point to the end of "innerFunction2" function (line 13) instead of
the end of the "outerFunction" function (line 15).

For comparison, the buffer can be switched to python-mode and the above
repeated with the non-treesit versions of these commands to demonstrate
that the regular python-mode does work as expected.

bug#68664: 29.1.50; treesit defun commands broken with nested functions

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Troy Brown <brownts@troybrown.dev> writes:

> I've noticed that "defun" related treesit commands do not appear to work
> correctly when nested functions are involved.  I've seen this behavior
> in multiple languages and believe the problem is an issue in the
> treesit.el library.

Customize the treesit-defun-tactic variable to 'top-level to ignore
nested defuns in navigation commands.

bug#68664: 29.1.50; treesit defun commands broken with nested functions

[Troy Brown]

On Mon, Jan 22, 2024 at 7:32 PM Daniel Martín <mardani29@yahoo.es> wrote:
>
> Troy Brown <brownts@troybrown.dev> writes:
>
> > I've noticed that "defun" related treesit commands do not appear to work
> > correctly when nested functions are involved.  I've seen this behavior
> > in multiple languages and believe the problem is an issue in the
> > treesit.el library.
>
> Customize the treesit-defun-tactic variable to 'top-level to ignore
> nested defuns in navigation commands.

But I don't want it to just go to the top-level, I want it to respect
the current
nesting level.  If I insert yet another level in the example, and
point is within
the second level of nesting, I want it to move to the beginning and end of that
nested function (i.e., "secondLevel" in the sample below when point is on the
call to innerFunction).  As mentioned in my original email, python-mode does
respect the nesting level correctly, but python-ts-mode, and other "ts" modes
that support nesting, don't respect it.

--8<---------------cut here---------------start------------->8---
# -*- mode: python-ts -*-

def outerFunction(text):
    text = text

    def secondLevel(text):
        print(text)

        def innerFunction(text):
            print(text)

        innerFunction()

        def innerFunction2(text):
            print(text)

        innerFunction2()

    secondLevel()

--8<---------------cut here---------------end--------------->8---

bug#68664: 29.1.50; treesit defun commands broken with nested functions

[Yuan Fu]

> On Jan 23, 2024, at 6:30 AM, Troy Brown <brownts@troybrown.dev> wrote:
> 
> On Mon, Jan 22, 2024 at 7:32 PM Daniel Martín <mardani29@yahoo.es> wrote:
>> 
>> Troy Brown <brownts@troybrown.dev> writes:
>> 
>>> I've noticed that "defun" related treesit commands do not appear to work
>>> correctly when nested functions are involved.  I've seen this behavior
>>> in multiple languages and believe the problem is an issue in the
>>> treesit.el library.
>> 
>> Customize the treesit-defun-tactic variable to 'top-level to ignore
>> nested defuns in navigation commands.
> 
> But I don't want it to just go to the top-level, I want it to respect
> the current
> nesting level.  If I insert yet another level in the example, and
> point is within
> the second level of nesting, I want it to move to the beginning and end of that
> nested function (i.e., "secondLevel" in the sample below when point is on the
> call to innerFunction).  As mentioned in my original email, python-mode does
> respect the nesting level correctly, but python-ts-mode, and other "ts" modes
> that support nesting, don't respect it.

The behavior is expected. But I can see that it doesn’t match your expectations. The logic behind the current behavior is to first move between siblings in the same level; if there’s no sibling to move across anymore, move to the beginning/end of the immediate parent, and so on.

To get the behavior you want, we would need to add a fourth defun navigation tactic, in addition to the existing three: nested, top-level, and restricted.

If you are interested and able, maybe you can look into adding it to treesit--navigate-thing or treesit-beginning/end-of-defun?

Yuan

bug#68664: 29.1.50; treesit defun commands broken with nested functions

[Troy Brown]

On Wed, Jan 24, 2024 at 1:29 AM Yuan Fu <casouri@gmail.com> wrote:
>
> The behavior is expected. But I can see that it doesn’t match your expectations. The logic behind the current behavior is to first move between siblings in the same level; if there’s no sibling to move across anymore, move to the beginning/end of the immediate parent, and so on.
>
> To get the behavior you want, we would need to add a fourth defun navigation tactic, in addition to the existing three: nested, top-level, and restricted.
>
> If you are interested and able, maybe you can look into adding it to treesit--navigate-thing or treesit-beginning/end-of-defun?
>
> Yuan

I find it quite odd that this is the expected behavior.  Per the Emacs
manual (section "Moving by Defuns"), I expected the point to be moved
to the "innermost defun around point", since treesit-defun-tactic is
set to "nested", as that is precisely what is documented there.  I
interpret "innermost defun around point" to mean the innermost defun
that encompasses point.  Additionally, the documentation strings for
treesit-beginning-of-defun and treesit-end-of-defun indicate that they
are a tree-sitter equivalent of the beginning-of-defun and
end-of-defun commands respectively.  If so, and since they are mapped
to the same key bindings in the tree-sitter modes, shouldn't the
expectation be that they behave the same way as the non-tree-sitter
commands?  If not, people transitioning between the non-tree-sitter
mode and the tree-sitter mode are in for a surprise when the commands
behave differently.

With the current behavior there is no way to move the point directly
to the beginning of the function without moving through all of the
nested functions first, which could be significant.  When you say the
behavior is to "move between siblings in the same level", should the
level refer to where point is, or to the level corresponding to the
function encompassing the point?  I think it probably makes sense to
move between siblings if you are at a function boundary (there is a
function immediately before or after the point), but if you are
already deep in a function, I think it makes sense to first move to
that function's begin/end before attempting to move between siblings.
I believe this behavior would be consistent with the non-tree-sitter
modes and expectations based on the description in the manual.

bug#68664: 29.1.50; treesit defun commands broken with nested functions

[Troy Brown]

On Wed, Jan 24, 2024 at 9:13 AM Troy Brown <brownts@troybrown.dev> wrote:
>
> On Wed, Jan 24, 2024 at 1:29 AM Yuan Fu <casouri@gmail.com> wrote:
> >
> > The behavior is expected. But I can see that it doesn’t match your expectations. The logic behind the current behavior is to first move between siblings in the same level; if there’s no sibling to move across anymore, move to the beginning/end of the immediate parent, and so on.
> >
> > To get the behavior you want, we would need to add a fourth defun navigation tactic, in addition to the existing three: nested, top-level, and restricted.
> >
> > If you are interested and able, maybe you can look into adding it to treesit--navigate-thing or treesit-beginning/end-of-defun?
> >
> > Yuan
>
> I find it quite odd that this is the expected behavior.  Per the Emacs
> manual (section "Moving by Defuns"), I expected the point to be moved
> to the "innermost defun around point", since treesit-defun-tactic is
> set to "nested", as that is precisely what is documented there.  I
> interpret "innermost defun around point" to mean the innermost defun
> that encompasses point.  Additionally, the documentation strings for
> treesit-beginning-of-defun and treesit-end-of-defun indicate that they
> are a tree-sitter equivalent of the beginning-of-defun and
> end-of-defun commands respectively.  If so, and since they are mapped
> to the same key bindings in the tree-sitter modes, shouldn't the
> expectation be that they behave the same way as the non-tree-sitter
> commands?  If not, people transitioning between the non-tree-sitter
> mode and the tree-sitter mode are in for a surprise when the commands
> behave differently.
>
> With the current behavior there is no way to move the point directly
> to the beginning of the function without moving through all of the
> nested functions first, which could be significant.  When you say the
> behavior is to "move between siblings in the same level", should the
> level refer to where point is, or to the level corresponding to the
> function encompassing the point?  I think it probably makes sense to
> move between siblings if you are at a function boundary (there is a
> function immediately before or after the point), but if you are
> already deep in a function, I think it makes sense to first move to
> that function's begin/end before attempting to move between siblings.
> I believe this behavior would be consistent with the non-tree-sitter
> modes and expectations based on the description in the manual.

To add further support to my belief that the current implementation is
not the expected behavior, consider how the current implementation
behaves when used with mark-defun.  When the point is on the call to
innerFunction and I execute "M-x mark-defun RET", the nested function
following the point (i.e., innerFunction2) is selected rather than the
function containing point.  For comparison, the non-tree-sitter
python-mode behaves correctly and selects the function containing
point, not the next nested function.

bug#68664: 29.1.50; treesit defun commands broken with nested functions

[Yuan Fu]

> On Jan 24, 2024, at 9:25 AM, Troy Brown <brownts@troybrown.dev> wrote:
> 
> On Wed, Jan 24, 2024 at 9:13 AM Troy Brown <brownts@troybrown.dev> wrote:
>> 
>> On Wed, Jan 24, 2024 at 1:29 AM Yuan Fu <casouri@gmail.com> wrote:
>>> 
>>> The behavior is expected. But I can see that it doesn’t match your expectations. The logic behind the current behavior is to first move between siblings in the same level; if there’s no sibling to move across anymore, move to the beginning/end of the immediate parent, and so on.
>>> 
>>> To get the behavior you want, we would need to add a fourth defun navigation tactic, in addition to the existing three: nested, top-level, and restricted.
>>> 
>>> If you are interested and able, maybe you can look into adding it to treesit--navigate-thing or treesit-beginning/end-of-defun?
>>> 
>>> Yuan
>> 
>> I find it quite odd that this is the expected behavior.  Per the Emacs
>> manual (section "Moving by Defuns"), I expected the point to be moved
>> to the "innermost defun around point", since treesit-defun-tactic is
>> set to "nested", as that is precisely what is documented there.  I
>> interpret "innermost defun around point" to mean the innermost defun
>> that encompasses point.  Additionally, the documentation strings for
>> treesit-beginning-of-defun and treesit-end-of-defun indicate that they
>> are a tree-sitter equivalent of the beginning-of-defun and
>> end-of-defun commands respectively.  If so, and since they are mapped
>> to the same key bindings in the tree-sitter modes, shouldn't the
>> expectation be that they behave the same way as the non-tree-sitter
>> commands?  If not, people transitioning between the non-tree-sitter
>> mode and the tree-sitter mode are in for a surprise when the commands
>> behave differently.
>> 
>> With the current behavior there is no way to move the point directly
>> to the beginning of the function without moving through all of the
>> nested functions first, which could be significant.  When you say the
>> behavior is to "move between siblings in the same level", should the
>> level refer to where point is, or to the level corresponding to the
>> function encompassing the point?  I think it probably makes sense to
>> move between siblings if you are at a function boundary (there is a
>> function immediately before or after the point), but if you are
>> already deep in a function, I think it makes sense to first move to
>> that function's begin/end before attempting to move between siblings.
>> I believe this behavior would be consistent with the non-tree-sitter
>> modes and expectations based on the description in the manual.
> 
> To add further support to my belief that the current implementation is
> not the expected behavior, consider how the current implementation
> behaves when used with mark-defun.  When the point is on the call to
> innerFunction and I execute "M-x mark-defun RET", the nested function
> following the point (i.e., innerFunction2) is selected rather than the
> function containing point.  For comparison, the non-tree-sitter
> python-mode behaves correctly and selects the function containing
> point, not the next nested function.

Yeah, I mean, I can definitely see the validity of the behavior you’re describing. But I think the current behavior is equally valid. Right now you can easily go to the previous/next sibling in the same level, _and_ go to the beginning/end of the parent. You just need to press a few more times. OTOH if you go straight to the parent, there’s no way to go to siblings.

As for mark-defun, I think it’s similarly equally valid to either mark the next sibling or the parent. Right now mark-defun doesn’t really have a notion of nested defun, we should upgrade it to support nested defun like we did beginning/end-of-defun, either by a toggle like mark-defun-tactic or let user control which defun to mark interactively.

Yuan

bug#68664: 29.1.50; treesit defun commands broken with nested functions

[Eli Zaretskii]

> Cc: 68664@debbugs.gnu.org, Daniel Martín <mardani29@yahoo.es>
> From: Yuan Fu <casouri@gmail.com>
> Date: Fri, 26 Jan 2024 20:26:38 -0800
> 
> > To add further support to my belief that the current implementation is
> > not the expected behavior, consider how the current implementation
> > behaves when used with mark-defun.  When the point is on the call to
> > innerFunction and I execute "M-x mark-defun RET", the nested function
> > following the point (i.e., innerFunction2) is selected rather than the
> > function containing point.  For comparison, the non-tree-sitter
> > python-mode behaves correctly and selects the function containing
> > point, not the next nested function.
> 
> Yeah, I mean, I can definitely see the validity of the behavior you’re describing. But I think the current behavior is equally valid. Right now you can easily go to the previous/next sibling in the same level, _and_ go to the beginning/end of the parent. You just need to press a few more times. OTOH if you go straight to the parent, there’s no way to go to siblings.

Maybe we could support both behaviors via specially-valued prefix
arguments?  Like "C-u" means something, "C-u C-u" means something
else, etc.?

> As for mark-defun, I think it’s similarly equally valid to either mark the next sibling or the parent. Right now mark-defun doesn’t really have a notion of nested defun, we should upgrade it to support nested defun like we did beginning/end-of-defun, either by a toggle like mark-defun-tactic or let user control which defun to mark interactively.

Same here.

WDYT?

bug#68664: 29.1.50; treesit defun commands broken with nested functions

[Yuan Fu]

> On Jan 26, 2024, at 11:32 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> Cc: 68664@debbugs.gnu.org, Daniel Martín <mardani29@yahoo.es>
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Fri, 26 Jan 2024 20:26:38 -0800
>> 
>>> To add further support to my belief that the current implementation is
>>> not the expected behavior, consider how the current implementation
>>> behaves when used with mark-defun.  When the point is on the call to
>>> innerFunction and I execute "M-x mark-defun RET", the nested function
>>> following the point (i.e., innerFunction2) is selected rather than the
>>> function containing point.  For comparison, the non-tree-sitter
>>> python-mode behaves correctly and selects the function containing
>>> point, not the next nested function.
>> 
>> Yeah, I mean, I can definitely see the validity of the behavior you’re describing. But I think the current behavior is equally valid. Right now you can easily go to the previous/next sibling in the same level, _and_ go to the beginning/end of the parent. You just need to press a few more times. OTOH if you go straight to the parent, there’s no way to go to siblings.
> 
> Maybe we could support both behaviors via specially-valued prefix
> arguments?  Like "C-u" means something, "C-u C-u" means something
> else, etc.?

Beginning/end-of-defun already take a numerical interactive arg, unless I missed something we can’t add another. If we want to change behavior interactively we would need something more elaborate, maybe transient maps.

> 
>> As for mark-defun, I think it’s similarly equally valid to either mark the next sibling or the parent. Right now mark-defun doesn’t really have a notion of nested defun, we should upgrade it to support nested defun like we did beginning/end-of-defun, either by a toggle like mark-defun-tactic or let user control which defun to mark interactively.
> 
> Same here.
> 
> WDYT?

Same for mark-defun, it also has an interactive arg already.

I feel like I missed something, surely you know they already have interactive args :-)

Yuan

bug#68664: 29.1.50; treesit defun commands broken with nested functions

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Sat, 27 Jan 2024 20:03:30 -0800
> Cc: Troy Brown <brownts@troybrown.dev>,
>  68664@debbugs.gnu.org,
>  mardani29@yahoo.es
> 
> > Maybe we could support both behaviors via specially-valued prefix
> > arguments?  Like "C-u" means something, "C-u C-u" means something
> > else, etc.?
> 
> Beginning/end-of-defun already take a numerical interactive arg, unless I missed something we can’t add another. If we want to change behavior interactively we would need something more elaborate, maybe transient maps.
> 
> > 
> >> As for mark-defun, I think it’s similarly equally valid to either mark the next sibling or the parent. Right now mark-defun doesn’t really have a notion of nested defun, we should upgrade it to support nested defun like we did beginning/end-of-defun, either by a toggle like mark-defun-tactic or let user control which defun to mark interactively.
> > 
> > Same here.
> > 
> > WDYT?
> 
> Same for mark-defun, it also has an interactive arg already.
> 
> I feel like I missed something, surely you know they already have interactive args :-)

"C-u" and "C-u 4" are not the same, and can be distinguished by the
function's body, right?

bug#68664: 29.1.50; treesit defun commands broken with nested functions

[Yuan Fu]

> On Jan 27, 2024, at 10:53 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Yuan Fu <casouri@gmail.com>
>> Date: Sat, 27 Jan 2024 20:03:30 -0800
>> Cc: Troy Brown <brownts@troybrown.dev>,
>> 68664@debbugs.gnu.org,
>> mardani29@yahoo.es
>> 
>>> Maybe we could support both behaviors via specially-valued prefix
>>> arguments?  Like "C-u" means something, "C-u C-u" means something
>>> else, etc.?
>> 
>> Beginning/end-of-defun already take a numerical interactive arg, unless I missed something we can’t add another. If we want to change behavior interactively we would need something more elaborate, maybe transient maps.
>> 
>>> 
>>>> As for mark-defun, I think it’s similarly equally valid to either mark the next sibling or the parent. Right now mark-defun doesn’t really have a notion of nested defun, we should upgrade it to support nested defun like we did beginning/end-of-defun, either by a toggle like mark-defun-tactic or let user control which defun to mark interactively.
>>> 
>>> Same here.
>>> 
>>> WDYT?
>> 
>> Same for mark-defun, it also has an interactive arg already.
>> 
>> I feel like I missed something, surely you know they already have interactive args :-)
> 
> "C-u" and "C-u 4" are not the same, and can be distinguished by the
> function's body, right?

Ah, you’re right. I didn’t know that. If I use (interactive "P”), C-u gives my '(4) and C-u 4 give me 4. That’s what you mean right?

In that case, yeah I think it could be useful for C-u mark-defun to mark the encoding parent rather than next sibling, and C-u beginning-of-defun to go straight to beg of parent.

Yuan

bug#68664: 29.1.50; treesit defun commands broken with nested functions

[Eli Zaretskii]

> From: Yuan Fu <casouri@gmail.com>
> Date: Sat, 27 Jan 2024 23:29:36 -0800
> Cc: Troy Brown <brownts@troybrown.dev>,
>  68664@debbugs.gnu.org,
>  mardani29@yahoo.es
> 
> >> I feel like I missed something, surely you know they already have interactive args :-)
> > 
> > "C-u" and "C-u 4" are not the same, and can be distinguished by the
> > function's body, right?
> 
> Ah, you’re right. I didn’t know that. If I use (interactive "P”), C-u gives my '(4) and C-u 4 give me 4. That’s what you mean right?

Yes.

> In that case, yeah I think it could be useful for C-u mark-defun to mark the encoding parent rather than next sibling, and C-u beginning-of-defun to go straight to beg of parent.