GNU bug report logs - #68899
Treesitter's forward-sexp-function

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 68899 in the body.
You can then email your comments to 68899 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: "simon254--- via Bug reports for GNU Emacs,
 the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>,
 Yuan Fu <casouri <at> gmail.com>
Subject: Treesitter's forward-sexp-function
Date: Fri, 2 Feb 2024 21:47:33 +0000

Hello Yuan,

In c++-mode, python-mode, and all other modes I know, pressing
C-M-f with point anywhere in the characters of a symbol brings
you to the end of that symbol.

In c++-ts-mode it only does something if you're in the beginning
of  the symbol.  Everywhere else point stays where it is.  I know
there are some intended differences for c++-ts-mode's
forward-sexp-function vs c++-mode's, but would this be one
such difference?

Here's a quick repro, in case you don't follow

  emacs -Q /tmp/something.cpp -f c++-ts-mode

  int main() {}

If point is 5 (on the 'm' of main), C-M-f will bring me to the
space after the closing ')'.  This is different from c++-mode,
but I think I can learn to live with this, in fact I think I like
it. However if point is anywhere on 'ain', point stays put, and
that's very jarring when compared to every other mode I've ever
worked with in Emacs.

Shouldn't the intervening treesit-end-of-thing go to the end of the
current thing?, i.e. to the '('?  I think it should, at least judging
from its docstring, and this patch makes that happen:

-              (setq pos (funcall advance (if (> arg 0) next prev)))
+              (setq pos (funcall advance (or (if (> arg 0) next prev)
+                                             parent)))

This doesn't seem to break tests, assuming it's not in these 3 there
were skipped because I don't have the grammar installed.

s treesit-defun-navigation-nested-3
s treesit-defun-navigation-nested-4
s treesit-multi-lang

If this patch isn't acceptable, is it possible to make this
customizable somehow?  I know I can set forward-sexp-function to
something else, but I now am actually getting used to this f-s-f,
only this bit is putting me off.

João

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: "simon254--- via Bug reports for GNU Emacs,
 the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>,
 Yuan Fu <casouri <at> gmail.com>
Subject: Re: Treesitter's forward-sexp-function
Date: Sat, 3 Feb 2024 00:42:44 +0000

On Fri, Feb 2, 2024 at 9:47 PM João Távora <joaotavora <at> gmail.com> wrote:

> Shouldn't the intervening treesit-end-of-thing go to the end of the
> current thing?, i.e. to the '('?  I think it should, at least judging
> from its docstring, and this patch makes that happen:
>
> -              (setq pos (funcall advance (if (> arg 0) next prev)))
> +              (setq pos (funcall advance (or (if (> arg 0) next prev)
> +                                             parent)))
>
> This doesn't seem to break tests, assuming it's not in these 3 there
> were skipped because I don't have the grammar installed.

Despite that, I think it's still wrong :-/ Now it moves too much,
i.e. it never stops moving.

Message #11 received at submit <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: "simon254--- via Bug reports for GNU Emacs,
 the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>,
 Yuan Fu <casouri <at> gmail.com>
Subject: Re: Treesitter's forward-sexp-function
Date: Sat, 3 Feb 2024 00:57:23 +0000

[Message part 1 (text/plain, inline)]

On Sat, Feb 3, 2024 at 12:42 AM João Távora <joaotavora <at> gmail.com> wrote:

> > This doesn't seem to break tests, assuming it's not in these 3 there
> > were skipped because I don't have the grammar installed.
>
> Despite that, I think it's still wrong :-/ Now it moves too much,
> i.e. it never stops moving.

This looks more promising.  Works well in my tests.

diff --git a/lisp/treesit.el b/lisp/treesit.el
index c6b9d8ff4bc..cad7497fb74 100644
--- a/lisp/treesit.el
+++ b/lisp/treesit.el
@@ -2579,9 +2579,12 @@ treesit--navigate-thing
             (setq parent (treesit-node-top-level parent thing t)
                   prev nil
                   next nil))
-          ;; If TACTIC is `restricted', the implementation is very simple.
+          ;; If TACTIC is `restricted', the implementation is reasonably
simple.
           (if (eq tactic 'restricted)
-              (setq pos (funcall advance (if (> arg 0) next prev)))
+              (setq pos (funcall advance (cond ((and (null next) (null
prev))
+                                                parent)
+                                               ((> arg 0) next)
+                                               (t prev))))
             ;; For `nested', it's a bit more work:
             ;; Move...
             (if (> arg 0)

[Message part 2 (text/html, inline)]

Message #14 received at 68899 <at> debbugs.gnu.org (full text, mbox):

From: Yuan Fu <casouri <at> gmail.com>
To: João Távora <joaotavora <at> gmail.com>
Cc: 68899 <at> debbugs.gnu.org
Subject: Re: bug#68899: Treesitter's forward-sexp-function
Date: Sat, 3 Feb 2024 21:35:00 -0800

[Message part 1 (text/plain, inline)]

> On Feb 2, 2024, at 4:57 PM, João Távora <joaotavora <at> gmail.com> wrote:
> 
> On Sat, Feb 3, 2024 at 12:42 AM João Távora <joaotavora <at> gmail.com> wrote:
> 
> > > This doesn't seem to break tests, assuming it's not in these 3 there
> > > were skipped because I don't have the grammar installed.
> >
> > Despite that, I think it's still wrong :-/ Now it moves too much,
> > i.e. it never stops moving.
> 
> This looks more promising.  Works well in my tests.
> 
> diff --git a/lisp/treesit.el b/lisp/treesit.el
> index c6b9d8ff4bc..cad7497fb74 100644
> --- a/lisp/treesit.el
> +++ b/lisp/treesit.el
> @@ -2579,9 +2579,12 @@ treesit--navigate-thing
>              (setq parent (treesit-node-top-level parent thing t)
>                    prev nil
>                    next nil))
> -          ;; If TACTIC is `restricted', the implementation is very simple.
> +          ;; If TACTIC is `restricted', the implementation is reasonably simple.
>            (if (eq tactic 'restricted)
> -              (setq pos (funcall advance (if (> arg 0) next prev)))
> +              (setq pos (funcall advance (cond ((and (null next) (null prev))
> +                                                parent)
> +                                               ((> arg 0) next)
> +                                               (t prev))))
>              ;; For `nested', it's a bit more work:
>              ;; Move...
>              (if (> arg 0)

Thanks for looking into this, Joao. IME a very useful characteristic of forward-sexp is that it stays in the same “level” and doesn’t go up automatically when there’s no siblings (when there’s a closing delimiter). Eg, in an Elisp buffer, forward-sexp stops at the closing parenthesis, in C, it should stop at the closing bracket.

Also you don’t want to check for prev when moving forward, and vice versa, ie, we don’t want to check (null next) and (null prev) together.

So, how do you like this patch:

Yuan

[forward-sexp.patch (application/octet-stream, attachment)]

[Message part 3 (text/plain, inline)]

Message #17 received at 68899 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Yuan Fu <casouri <at> gmail.com>
Cc: 68899 <at> debbugs.gnu.org
Subject: Re: bug#68899: Treesitter's forward-sexp-function
Date: Sun, 4 Feb 2024 12:40:33 +0000

On Sun, Feb 4, 2024 at 5:35 AM Yuan Fu <casouri <at> gmail.com> wrote:

> Thanks for looking into this, Joao. IME a very useful characteristic of forward-sexp is that it stays in the same “level” and doesn’t go up automatically when there’s no siblings (when there’s a closing delimiter). Eg, in an Elisp buffer, forward-sexp stops at the closing parenthesis, in C, it should stop at the closing bracket.

I agree.  There are other useful characteristics, but this is one of them.
It allows be to mark regions of text up to points that I'm not even seeing.

> Also you don’t want to check for prev when moving forward, and vice
> versa, ie, we don’t want to check (null next) and (null prev) together.

I get it.  I used those existing results as a proxy to know if we're
in the middle
of a leaf.  I _think_ it's sound (maybe I'm wrong).

> So, how do you like this patch:

It works fine, but as far as I can tell does exactly the same as mine, and
looks to be slightly more difficult to read and uses a further treesit
query to check if this is a leaf node.  But it's absolutley fine really.

One way my patch can be described in plain english is
"if we're not at an inter-thing boundary, we navigate to such boundary"
And then the meaning of checking prev _and_ next becomes obvious
and it isn't necessary to perform the additional check that we're in a
leaf node.

So go ahead and push whichever patch you prefer, and thanks.

João

Message #20 received at 68899 <at> debbugs.gnu.org (full text, mbox):

From: Yuan Fu <casouri <at> gmail.com>
To: João Távora <joaotavora <at> gmail.com>
Cc: 68899 <at> debbugs.gnu.org
Subject: Re: bug#68899: Treesitter's forward-sexp-function
Date: Sun, 4 Feb 2024 16:50:28 -0800

> On Feb 4, 2024, at 4:40 AM, João Távora <joaotavora <at> gmail.com> wrote:
> 
> On Sun, Feb 4, 2024 at 5:35 AM Yuan Fu <casouri <at> gmail.com> wrote:
> 
>> Thanks for looking into this, Joao. IME a very useful characteristic of forward-sexp is that it stays in the same “level” and doesn’t go up automatically when there’s no siblings (when there’s a closing delimiter). Eg, in an Elisp buffer, forward-sexp stops at the closing parenthesis, in C, it should stop at the closing bracket.
> 
> I agree.  There are other useful characteristics, but this is one of them.
> It allows be to mark regions of text up to points that I'm not even seeing.
> 
>> Also you don’t want to check for prev when moving forward, and vice
>> versa, ie, we don’t want to check (null next) and (null prev) together.
> 
> I get it.  I used those existing results as a proxy to know if we're
> in the middle
> of a leaf.  I _think_ it's sound (maybe I'm wrong).
> 
>> So, how do you like this patch:
> 
> It works fine, but as far as I can tell does exactly the same as mine, and
> looks to be slightly more difficult to read and uses a further treesit
> query to check if this is a leaf node.  But it's absolutley fine really.
> 
> One way my patch can be described in plain english is
> "if we're not at an inter-thing boundary, we navigate to such boundary"
> And then the meaning of checking prev _and_ next becomes obvious
> and it isn't necessary to perform the additional check that we're in a
> leaf node.
> 
> So go ahead and push whichever patch you prefer, and thanks.
> 
> João

Initially I was worried about the case below:

void main (void) {
  <point>
}<will move to here>

But I get you now; if we define “leaf thing” as not having any nested child thing, and we allow ourselves to move to the end of leaf thing, then in this case we indeed should move out of the closing bracket. I like your definition of “leaf thing” better since it’s more general than “leaf node”, and I like the simpler code too.

So applied your patch with some comments added, thanks!

Yuan

Message #23 received at 68899 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Yuan Fu <casouri <at> gmail.com>
Cc: 68899 <at> debbugs.gnu.org
Subject: Re: bug#68899: Treesitter's forward-sexp-function
Date: Mon, 5 Feb 2024 01:08:51 +0000

[Message part 1 (text/plain, inline)]

On Mon, Feb 5, 2024 at 12:50 AM Yuan Fu <casouri <at> gmail.com> wrote:

> void main (void) {
>   <point>
> }<will move to here>

Ohhh, I didn't think about this case

> But I get you now; if we define “leaf thing” as not having any nested
child thing, and we allow ourselves to move to the end of leaf thing, then
in this case we indeed should move out of the closing bracket. I like your
definition of “leaf thing” better since it’s more general than “leaf node”,
and I like the simpler code too.
>
> So applied your patch with some comments added, thanks!

Errr... I'm very sorry, but now I think your previous patch makes more
sense -- precisely because of the above case, which now I understand
what you were arguing for.  I assume your patch indeed preserves that
property of NOT leaving the braces.

I think that's also how c++-mode works (and about all other
sexp-navigation) works.

So if we could go back ~12 hours and allow me to respond positively
to your initial patch, I think that would be perfect :-)

But I _guess_ you could defend many behaviours.  Maybe this "tactic"
argument should be exposed to the user in a variable.

Anyway, what there is now is already much less jarring than what there
was before.  The empty body is fairly rare.

João

[Message part 2 (text/html, inline)]

Message #26 received at 68899 <at> debbugs.gnu.org (full text, mbox):

From: Yuan Fu <casouri <at> gmail.com>
To: João Távora <joaotavora <at> gmail.com>
Cc: 68899 <at> debbugs.gnu.org
Subject: Re: bug#68899: Treesitter's forward-sexp-function
Date: Mon, 5 Feb 2024 23:25:02 -0800

> On Feb 4, 2024, at 5:08 PM, João Távora <joaotavora <at> gmail.com> wrote:
> 
> On Mon, Feb 5, 2024 at 12:50 AM Yuan Fu <casouri <at> gmail.com> wrote:
> 
> > void main (void) {
> >   <point>
> > }<will move to here>
> 
> Ohhh, I didn't think about this case
> 
> > But I get you now; if we define “leaf thing” as not having any nested child thing, and we allow ourselves to move to the end of leaf thing, then in this case we indeed should move out of the closing bracket. I like your definition of “leaf thing” better since it’s more general than “leaf node”, and I like the simpler code too.
> >
> > So applied your patch with some comments added, thanks!
> 
> Errr... I'm very sorry, but now I think your previous patch makes more
> sense -- precisely because of the above case, which now I understand
> what you were arguing for.  I assume your patch indeed preserves that
> property of NOT leaving the braces.
> 
> I think that's also how c++-mode works (and about all other
> sexp-navigation) works.
> 
> So if we could go back ~12 hours and allow me to respond positively
> to your initial patch, I think that would be perfect :-)
> 
> But I _guess_ you could defend many behaviours.  Maybe this "tactic"
> argument should be exposed to the user in a variable.
> 
> Anyway, what there is now is already much less jarring than what there
> was before.  The empty body is fairly rare.
> 
> João

No sweat. The empty body is fairly rare, and the current behavior isn’t annoying for empty body, I’d say. Consider that treesit--navigate-thing will be used on many other things besides sexp, a more general “leaf thing” is probably better suited for it.

Yuan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.