bug#62158: treesit-end-of-defun error

bug#62158: treesit-end-of-defun error

[Juri Linkov]

X-Debbugs-Cc: Yuan Fu <casouri@gmail.com>, Dmitry Gutov <dgutov@yandex.ru>

Since this is a separate problem, I'm closing bug#62086
and opening a new bug report:

>>>> I don't know if the second bug is related to this, but while
>>>> in the same file, also type 'C-M-l' ('reposition-window').
>>>> It raises the error:
>>>>   Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
>>>>     treesit-end-of-defun()
>>>>     end-of-defun(-1)
>>>>     reposition-window(nil nil)
>>>>     reposition-window(nil 89)
>>>>     funcall-interactively(reposition-window nil 89)
>>>>     command-execute(reposition-window)
>> I see it only in some files in test/lisp/progmodes/ruby-mode-resources/
>> e.g. ruby-parenless-call-arguments-indent.rb, ruby-method-call-indent.rb,
>> ruby-block-indent.rb.  But not in e.g. ruby-after-operator-indent.rb.
>> Also everywhere in test/lisp/progmodes/js-resources/js-indent-init-dynamic.js,
>> js-indent-init-t.js.  But not in e.g. js-chain.js.
>
> Thanks, I can repro. I might have been trying the wrong binding at the end
> last night (C-l instead of C-M-l).
>
> The fix seems to be easy:
>
> diff --git a/lisp/treesit.el b/lisp/treesit.el
> index c118f5d52a4..b271a1f0c4b 100644
> --- a/lisp/treesit.el
> +++ b/lisp/treesit.el
> @@ -1882,6 +1882,7 @@ treesit-end-of-defun
>  `treesit-defun-skipper'."
>    (interactive "^p\nd")
>    (let ((orig-point (point)))
> +    (if (or (null arg) (= arg 0)) (setq arg 1))
>      (catch 'done
>        (dotimes (_ 2) ; Not making progress is better than infloop.
>
> But I'm not quite sure if that is what we want to do.
>
> More naturally, I think, would be to remove the argument from
> treesit-end-of-defun altogether (and adjust the code accordingly), because
> end-of-defun-function is documented to take no arguments.
>
> The only other place where treesit-end-of-defun seems to be used is the
> <remap> <end-of-defun> binding set up by treesit-major-mode-setup.
>
> Why not keep the default bindings for these? When
> beginning-of-defun-function and end-of-defun-function are set
> appropriately, they should work fine. Don't they?
>
> Cc'ing Yuan on that subject.

bug#62158: treesit-end-of-defun error

[Yuan Fu]

> On Mar 13, 2023, at 12:28 AM, Juri Linkov <juri@linkov.net> wrote:
> 
> X-Debbugs-Cc: Yuan Fu <casouri@gmail.com>, Dmitry Gutov <dgutov@yandex.ru>
> 
> Since this is a separate problem, I'm closing bug#62086
> and opening a new bug report:
> 
>>>>> I don't know if the second bug is related to this, but while
>>>>> in the same file, also type 'C-M-l' ('reposition-window').
>>>>> It raises the error:
>>>>>  Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
>>>>>    treesit-end-of-defun()
>>>>>    end-of-defun(-1)
>>>>>    reposition-window(nil nil)
>>>>>    reposition-window(nil 89)
>>>>>    funcall-interactively(reposition-window nil 89)
>>>>>    command-execute(reposition-window)
>>> I see it only in some files in test/lisp/progmodes/ruby-mode-resources/
>>> e.g. ruby-parenless-call-arguments-indent.rb, ruby-method-call-indent.rb,
>>> ruby-block-indent.rb.  But not in e.g. ruby-after-operator-indent.rb.
>>> Also everywhere in test/lisp/progmodes/js-resources/js-indent-init-dynamic.js,
>>> js-indent-init-t.js.  But not in e.g. js-chain.js.
>> 
>> Thanks, I can repro. I might have been trying the wrong binding at the end
>> last night (C-l instead of C-M-l).
>> 
>> The fix seems to be easy:
>> 
>> diff --git a/lisp/treesit.el b/lisp/treesit.el
>> index c118f5d52a4..b271a1f0c4b 100644
>> --- a/lisp/treesit.el
>> +++ b/lisp/treesit.el
>> @@ -1882,6 +1882,7 @@ treesit-end-of-defun
>> `treesit-defun-skipper'."
>>   (interactive "^p\nd")
>>   (let ((orig-point (point)))
>> +    (if (or (null arg) (= arg 0)) (setq arg 1))
>>     (catch 'done
>>       (dotimes (_ 2) ; Not making progress is better than infloop.
>> 
>> But I'm not quite sure if that is what we want to do.

This looks good to me.

>> 
>> More naturally, I think, would be to remove the argument from
>> treesit-end-of-defun altogether (and adjust the code accordingly), because
>> end-of-defun-function is documented to take no arguments.
>> 
>> The only other place where treesit-end-of-defun seems to be used is the
>> <remap> <end-of-defun> binding set up by treesit-major-mode-setup.
>> 
>> Why not keep the default bindings for these? When
>> beginning-of-defun-function and end-of-defun-function are set
>> appropriately, they should work fine. Don't they?

We tried that initially, but end-of-defun doesn’t have the notion of nested defuns, which leads to problems when end-of-defun-function recognizes nested defuns. In the following code

(defun xxx ()
  |
  (defun yyy () ...)

  (defun zzz () ...)
  )

If point is at “|” and you call end-of-defun, you’d expect point to move to the end of yyy, but instead it moves to the end of xxx. That’s because end-of-defun first runs (beginning-of-defun -1) followed by (end-of-defun 1) to check if the starting point is in a defun or between two defuns. This is fine in non-nested defuns, but in this example, the point first goes to the beginning of xxx, then goes to the end of xxx. And end-of-defun thinks that we started in a defun and now is at an end of defun, job’s done, and finishes.

The plan is to improve end-of-defun to support nested defuns in Emacs 30. For now we rebind end-of-defun to treesit-end-of-defun.

Yuan

bug#62158: treesit-end-of-defun error

[Juri Linkov]

close 62158 29.0.60
thanks

>>> The fix seems to be easy:
>>> 
>>> diff --git a/lisp/treesit.el b/lisp/treesit.el
>>> index c118f5d52a4..b271a1f0c4b 100644
>>> --- a/lisp/treesit.el
>>> +++ b/lisp/treesit.el
>>> @@ -1882,6 +1882,7 @@ treesit-end-of-defun
>>> `treesit-defun-skipper'."
>>>   (interactive "^p\nd")
>>>   (let ((orig-point (point)))
>>> +    (if (or (null arg) (= arg 0)) (setq arg 1))
>>>     (catch 'done
>>>       (dotimes (_ 2) ; Not making progress is better than infloop.
>>> 
>>> But I'm not quite sure if that is what we want to do.
>
> This looks good to me.

So Dmitry's fix is pushed to emacs-29.

bug#62158: treesit-end-of-defun error

[Dmitry Gutov]

On 14/03/2023 00:04, Yuan Fu wrote:
> We tried that initially, but end-of-defun doesn’t have the notion of nested defuns, which leads to problems when end-of-defun-function recognizes nested defuns. In the following code
> 
> (defun xxx ()
>    |
>    (defun yyy () ...)
> 
>    (defun zzz () ...)
>    )
> 
> If point is at “|” and you call end-of-defun, you’d expect point to move to the end of yyy, but instead it moves to the end of xxx. That’s because end-of-defun first runs (beginning-of-defun -1) followed by (end-of-defun 1) to check if the starting point is in a defun or between two defuns. This is fine in non-nested defuns, but in this example, the point first goes to the beginning of xxx, then goes to the end of xxx. And end-of-defun thinks that we started in a defun and now is at an end of defun, job’s done, and finishes.
> 
> The plan is to improve end-of-defun to support nested defuns in Emacs 30. For now we rebind end-of-defun to treesit-end-of-defun.

That makes sense, thanks!

I guess one of the things to try is to call end-of-defun-function first, 
followed by beginning-of-defun-function. And see if the resulting 
position is below the original point.

bug#62158: treesit-end-of-defun error

[Yuan Fu]

> On Mar 20, 2023, at 11:43 AM, Dmitry Gutov <dgutov@yandex.ru> wrote:
> 
> On 14/03/2023 00:04, Yuan Fu wrote:
>> We tried that initially, but end-of-defun doesn’t have the notion of nested defuns, which leads to problems when end-of-defun-function recognizes nested defuns. In the following code
>> (defun xxx ()
>>   |
>>   (defun yyy () ...)
>>   (defun zzz () ...)
>>   )
>> If point is at “|” and you call end-of-defun, you’d expect point to move to the end of yyy, but instead it moves to the end of xxx. That’s because end-of-defun first runs (beginning-of-defun -1) followed by (end-of-defun 1) to check if the starting point is in a defun or between two defuns. This is fine in non-nested defuns, but in this example, the point first goes to the beginning of xxx, then goes to the end of xxx. And end-of-defun thinks that we started in a defun and now is at an end of defun, job’s done, and finishes.
>> The plan is to improve end-of-defun to support nested defuns in Emacs 30. For now we rebind end-of-defun to treesit-end-of-defun.
> 
> That makes sense, thanks!
> 
> I guess one of the things to try is to call end-of-defun-function first, followed by beginning-of-defun-function. And see if the resulting position is below the original point.

Unfortunately, end-of-defun-function must be called from the beginning of a defun, according to its docstring. So that won’t do. We got to upgrade beginning/end-of-defun to support nested defuns.

Yuan