bug#68899: Treesitter's forward-sexp-function

bug#68899: Treesitter's forward-sexp-function

[João Távora]

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

bug#68899: Treesitter's forward-sexp-function

[João Távora]

On Fri, Feb 2, 2024 at 9:47 PM João Távora <joaotavora@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.

bug#68899: Treesitter's forward-sexp-function

[João Távora]

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

On Sat, Feb 3, 2024 at 12:42 AM João Távora <joaotavora@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)

[-- Attachment #2: Type: text/html, Size: 1666 bytes --]

bug#68899: Treesitter's forward-sexp-function

[Yuan Fu]

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



> On Feb 2, 2024, at 4:57 PM, João Távora <joaotavora@gmail.com> wrote:
> 
> On Sat, Feb 3, 2024 at 12:42 AM João Távora <joaotavora@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


[-- Attachment #2: forward-sexp.patch --]
[-- Type: application/octet-stream, Size: 2539 bytes --]

From 0afe667244caba04f061270cd2fc052ff6021130 Mon Sep 17 00:00:00 2001
From: Yuan Fu <casouri@gmail.com>
Date: Sat, 3 Feb 2024 21:24:29 -0800
Subject: [PATCH] Improve treesit-forward-sexp behavior for leaf nodes
 (bug#68899)

treesit-forward-sexp uses treesit--navigate-thing with 'restricted'
tactic.  In this tactic we don't move over the parent thing.  However,
this makes forward-sexp useless for symbols when point is in the
symbol rather than at the beginning of it: in that case, the symbol is
considered parent and treesit-forward-sexp won't move to the end of
it.

To solve that, we allow to move across the parent even in 'restricted'
mode if the parent is a leaf node.

Here, "leaf node" is defined as "doesn't have named child node", i.e.,
if the node has only anonymous child nodes, it's still considered a
leaf node.  I don't know how useful this heuristic is, but it feels
like a good idea.

* lisp/treesit.el (treesit--navigate-thing): Move over parent in
'restricted' tactic if the parent is a leaf node.
---
 lisp/treesit.el | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/lisp/treesit.el b/lisp/treesit.el
index fab2ddd88e6..bf6b8dffde4 100644
--- a/lisp/treesit.el
+++ b/lisp/treesit.el
@@ -2662,9 +2662,21 @@ 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 simple.
+          ;; In principle we don't go to parent's beg/end for
+          ;; `restricted' tactic, but if the parent is a leaf node
+          ;; (e.g., a symbol), we do, lest you can't go to the end of
+          ;; a symbol with `forward-sexp' when point is in the symbol
+          ;; rather than at the beg of it.
           (if (eq tactic 'restricted)
-              (setq pos (funcall advance (if (> arg 0) next prev)))
+              (let ((restricted-parent
+                     (and (not (treesit-node-child parent 0 t))
+                          parent)))
+                (setq pos (funcall
+                           advance
+                           (if (> arg 0)
+                               (or next restricted-parent)
+                             (or prev restricted-parent)))))
             ;; For `nested', it's a bit more work:
             ;; Move...
             (if (> arg 0)
-- 
2.41.0


[-- Attachment #3: Type: text/plain, Size: 2 bytes --]

bug#68899: Treesitter's forward-sexp-function

[João Távora]

On Sun, Feb 4, 2024 at 5:35 AM Yuan Fu <casouri@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

bug#68899: Treesitter's forward-sexp-function

[Yuan Fu]

> On Feb 4, 2024, at 4:40 AM, João Távora <joaotavora@gmail.com> wrote:
> 
> On Sun, Feb 4, 2024 at 5:35 AM Yuan Fu <casouri@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

bug#68899: Treesitter's forward-sexp-function

[João Távora]

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

On Mon, Feb 5, 2024 at 12:50 AM Yuan Fu <casouri@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

[-- Attachment #2: Type: text/html, Size: 1554 bytes --]

bug#68899: Treesitter's forward-sexp-function

[Yuan Fu]

> On Feb 4, 2024, at 5:08 PM, João Távora <joaotavora@gmail.com> wrote:
> 
> On Mon, Feb 5, 2024 at 12:50 AM Yuan Fu <casouri@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