bug#73404: 30.0.50; [forward/kill/etc]-sexp commands do not behave as expected in tree-sitter modes

[Yuan Fu <casouri@gmail.com>]

> On Dec 10, 2024, at 9:20 AM, Juri Linkov <juri@linkov.net> wrote:
> 
>>>    export const add = (a, b) -!- => a + b;
>>> 
>>> typing 'C-M-f' should jump to the end of the next sexp
>>> (to the end of whole "binary_expression"):
>>> 
>>>    export const add = (a, b) => a + b-!-;
>>> 
>>> since only tree-sitter knows about "binary_expression",
>>> so 'forward-sexp-default-function' can't be used here.
>> 
>> Actually, I have one idea of possible heuristics:
>> 
>> 1. first try 'forward-sexp-default-function'
>> 2. if it crosses the boundary of sexp defined by 'treesit-thing-settings'
>>   then use 'treesit-end-of-thing' instead
>> 
>> This should work.  Ok, will try.
> 
> This is implemented now in the attached patch, and it works nicely.
> 
> The main rule is the following: 'forward-sexp-default-function'
> should not go out of the current thing, neither go inside a sibling.
> So we use 'treesit-end-of-thing' in such cases.  But when inside
> a thing or outside a thing, use the default function.
> 
> This supposes that such things as "identifier" in js should be
> removed from 'treesit-thing-settings' since identifiers should be
> navigated the same way as such keywords as "export" and "const"
> using 'forward-sexp-default-function'.
> 
> What should remain in 'treesit-thing-settings' are only grouping
> constructs such as "parenthesized_expression" and "statement_block".

Ah, this matches my idea of defining sexp in other languages as “repeatable construct/list-like construct”. We went with “every syntactic construct” at the time, which I didn’t object to, but I’m definitely happier with the repeatable construct approach. Including Stefan and Theo since they were part of the original sexp navigation discussion.

My only concern is that would the result be a bit unpredictable/confusing when we mix the result of two logic together in such an involved way? We can push to master and try it out for a while. I use tree-sitter sexp navigation for work every day, albeit strictly for navigating list-like constructs—I use forward/backward-word for smaller navigation.

> 
> Removing "identifier" from 'treesit-thing-settings' exposed a problem
> in 'treesit-navigate-thing'.  This line
> 
>  ((and (null next) (null prev)) parent)
> 
> tries to go out of the current thing to its parent,
> thus breaking the main principle that 'forward-sexp'
> should move forward across siblings only.  But removing
> this line fixed the problem:

Thanks, LGTM.

> 
> diff --git a/lisp/treesit.el b/lisp/treesit.el
> index db8f7a7595d..4fcdbe7fc56 100644
> --- a/lisp/treesit.el
> +++ b/lisp/treesit.el
> @@ -2373,21 +2373,41 @@ treesit-forward-sexp
> What constitutes as text and source code sexp is determined
> by `text' and `sexp' in `treesit-thing-settings'."
>   (interactive "^p")
> -  (let ((arg (or arg 1))
> -        (pred (or treesit-sexp-type-regexp 'sexp))
> -        (node-at-point
> -         (treesit-node-at (point) (treesit-language-at (point)))))
> -    (or (when (and node-at-point
> -                   ;; Make sure point is strictly inside node.
> -                   (< (treesit-node-start node-at-point)
> -                      (point)
> -                      (treesit-node-end node-at-point))
> -                   (treesit-node-match-p node-at-point 'text t))
> -          (forward-sexp-default-function arg)
> -          t)
> -        (if (> arg 0)
> -            (treesit-end-of-thing pred (abs arg) 'restricted)
> -          (treesit-beginning-of-thing pred (abs arg) 'restricted))
> +  (let* ((arg (or arg 1))
> +         (pred (or treesit-sexp-type-regexp 'sexp))
> +         (current-thing (treesit-thing-at (point) pred t))
> +         (default-pos
> +          (condition-case _
> +              (save-excursion
> +                (forward-sexp-default-function arg)
> +                (point))
> +            (scan-error nil)))
> +         (default-pos (unless (eq (point) default-pos) default-pos))
> +         (sibling-pos
> +          (save-excursion
> +            (and (if (> arg 0)
> +                     (treesit-end-of-thing pred (abs arg) 'restricted)
> +                   (treesit-beginning-of-thing pred (abs arg) 'restricted))
> +                 (point))))
> +         (sibling (when sibling-pos
> +                    (if (> arg 0)
> +                        (treesit-thing-prev sibling-pos pred)
> +                      (treesit-thing-next sibling-pos pred)))))
> +
> +    ;; 'forward-sexp-default-function' should not go out of the current thing,
> +    ;; neither go inside the next thing, neither go over the next thing
> +    (or (when (and default-pos
> +                   (or (null current-thing)
> +                       (if (> arg 0)
> +                           (< default-pos (treesit-node-end current-thing))
> +                         (> default-pos (treesit-node-start current-thing))))
> +                   (or (null sibling)
> +                       (if (> arg 0)
> +                           (< default-pos (treesit-node-start sibling))
> +                         (> default-pos (treesit-node-end sibling)))))
> +          (goto-char default-pos))
> +        (when sibling-pos
> +          (goto-char sibling-pos))
>         ;; If we couldn't move, we should signal an error and report
>         ;; the obstacle, like `forward-sexp' does.  If we couldn't
>         ;; find a parent, we simply return nil without moving point,
> @@ -2849,8 +2869,7 @@ treesit-navigate-thing
>           (if (eq tactic 'restricted)
>               (setq pos (funcall
>                          advance
> -                         (cond ((and (null next) (null prev)) parent)
> -                               ((> arg 0) next)
> +                         (cond ((> arg 0) next)
>                                (t prev))))
>             ;; For `nested', it's a bit more work:
>             ;; Move...