bug#63790: 30.0.50; prog-fill-reindent-defun regression

bug#63790: 30.0.50; prog-fill-reindent-defun regression

[Juri Linkov]

This regression is in master, not in emacs-29.

0. emacs-30 -Q
1. add to the beginning of the *scratch* buffer a list, so that
*scratch* looks like this:

(+
 1
    2
 3)

;; This buffer is for text that is not saved, and for Lisp evaluation.
;; To create a file, visit it with C-x C-f and enter text in its buffer.

2. Activate the region with the beginning at the start of the line with
the first comment, and the region end with point at the end of the buffer.

3. Type 'M-q' (prog-fill-reindent-defun)

It indents the list instead of the comment.

But when point is at the region beginning then 'M-q' correctly indents
the comments.

bug#63790: 30.0.50; prog-fill-reindent-defun regression

[Dmitry Gutov]

On 29/05/2023 19:53, Juri Linkov wrote:
> This regression is in master, not in emacs-29.

If it is a regression, then compared to what? emacs-29 doesn't have this 
function. Compared to some earlier revision?

> 0. emacs-30 -Q
> 1. add to the beginning of the *scratch* buffer a list, so that
> *scratch* looks like this:
> 
> (+
>   1
>      2
>   3)
> 
> ;; This buffer is for text that is not saved, and for Lisp evaluation.
> ;; To create a file, visit it with C-x C-f and enter text in its buffer.
> 
> 2. Activate the region with the beginning at the start of the line with
> the first comment, and the region end with point at the end of the buffer.
> 
> 3. Type 'M-q' (prog-fill-reindent-defun)
> 
> It indents the list instead of the comment.
> 
> But when point is at the region beginning then 'M-q' correctly indents
> the comments.

This happens because in this scenario point ends up outside of the 
comment (at eob). So when the function is called, in looks for a defun.

Did we at some point add (or decide to add) a condition when, if a 
region is active, it should only refill and not reindent?

bug#63790: 30.0.50; prog-fill-reindent-defun regression

[Juri Linkov]

>> This regression is in master, not in emacs-29.
>
> If it is a regression, then compared to what? emacs-29 doesn't have this
> function. Compared to some earlier revision?

Sorry, the subject was not precise.  This is more elaborate:
the new function 'prog-fill-reindent-defun' caused a regression for
'M-q' compared to emacs-29 where 'M-q' was bound to 'fill-paragraph'.

>> 0. emacs-30 -Q
>> 1. add to the beginning of the *scratch* buffer a list, so that
>> *scratch* looks like this:
>> (+
>>   1
>>      2
>>   3)
>> ;; This buffer is for text that is not saved, and for Lisp evaluation.
>> ;; To create a file, visit it with C-x C-f and enter text in its buffer.
>> 2. Activate the region with the beginning at the start of the line with
>> the first comment, and the region end with point at the end of the buffer.
>> 3. Type 'M-q' (prog-fill-reindent-defun)
>> It indents the list instead of the comment.
>> But when point is at the region beginning then 'M-q' correctly indents
>> the comments.
>
> This happens because in this scenario point ends up outside of the comment
> (at eob). So when the function is called, in looks for a defun.
>
> Did we at some point add (or decide to add) a condition when, if a region
> is active, it should only refill and not reindent?

Maybe the logic of region detecting/handling could be copied from
'fill-paragraph' to 'prog-fill-reindent-defun'?

bug#63790: 30.0.50; prog-fill-reindent-defun regression

[Dmitry Gutov]

On 04/06/2023 20:14, Juri Linkov wrote:
>>> This regression is in master, not in emacs-29.
>> If it is a regression, then compared to what? emacs-29 doesn't have this
>> function. Compared to some earlier revision?
> Sorry, the subject was not precise.  This is more elaborate:
> the new function 'prog-fill-reindent-defun' caused a regression for
> 'M-q' compared to emacs-29 where 'M-q' was bound to 'fill-paragraph'.

Thanks for the clarification.

>>> 0. emacs-30 -Q
>>> 1. add to the beginning of the*scratch*  buffer a list, so that
>>> *scratch*  looks like this:
>>> (+
>>>    1
>>>       2
>>>    3)
>>> ;; This buffer is for text that is not saved, and for Lisp evaluation.
>>> ;; To create a file, visit it with C-x C-f and enter text in its buffer.
>>> 2. Activate the region with the beginning at the start of the line with
>>> the first comment, and the region end with point at the end of the buffer.
>>> 3. Type 'M-q' (prog-fill-reindent-defun)
>>> It indents the list instead of the comment.
>>> But when point is at the region beginning then 'M-q' correctly indents
>>> the comments.
>> This happens because in this scenario point ends up outside of the comment
>> (at eob). So when the function is called, in looks for a defun.
>>
>> Did we at some point add (or decide to add) a condition when, if a region
>> is active, it should only refill and not reindent?
> Maybe the logic of region detecting/handling could be copied from
> 'fill-paragraph' to 'prog-fill-reindent-defun'?

Makes sense. Do you want to suggest a patch?

bug#63790: 30.0.50; prog-fill-reindent-defun regression

[Juri Linkov]

>>>> 3. Type 'M-q' (prog-fill-reindent-defun)
>>>> It indents the list instead of the comment.
>>>> But when point is at the region beginning then 'M-q' correctly indents
>>>> the comments.
>>> This happens because in this scenario point ends up outside of the comment
>>> (at eob). So when the function is called, in looks for a defun.
>>>
>>> Did we at some point add (or decide to add) a condition when, if a region
>>> is active, it should only refill and not reindent?
>> Maybe the logic of region detecting/handling could be copied from
>> 'fill-paragraph' to 'prog-fill-reindent-defun'?
>
> Makes sense. Do you want to suggest a patch?

Sorry, can't do, because I don't understand what this line is intended to do,
and there are no comments with explanations:

  (re-search-forward "\\s-*\\s<" (line-end-position) t)

It's nil in the reported case, so 'fill-paragraph' is not called.

bug#63790: 30.0.50; prog-fill-reindent-defun regression

[Dmitry Gutov]

On 06/06/2023 21:55, Juri Linkov wrote:
>>>>> 3. Type 'M-q' (prog-fill-reindent-defun)
>>>>> It indents the list instead of the comment.
>>>>> But when point is at the region beginning then 'M-q' correctly indents
>>>>> the comments.
>>>> This happens because in this scenario point ends up outside of the comment
>>>> (at eob). So when the function is called, in looks for a defun.
>>>>
>>>> Did we at some point add (or decide to add) a condition when, if a region
>>>> is active, it should only refill and not reindent?
>>> Maybe the logic of region detecting/handling could be copied from
>>> 'fill-paragraph' to 'prog-fill-reindent-defun'?
>> Makes sense. Do you want to suggest a patch?
> Sorry, can't do, because I don't understand what this line is intended to do,
> and there are no comments with explanations:
> 
>    (re-search-forward "\\s-*\\s<" (line-end-position) t)

It's looking for a comment that begins after point (possibly preceded by 
whitespace). There is no comment after point in the presented scenario.

> It's nil in the reported case, so 'fill-paragraph' is not called.

I guess when there is an active region, we would force the behavior to 
"refill" the region, no matter whether it is inside a comment, or 
contains a comment, or outside of any comments and simply contains code?

bug#63790: 30.0.50; prog-fill-reindent-defun regression

[Juri Linkov]

>>    (re-search-forward "\\s-*\\s<" (line-end-position) t)
>
> It's looking for a comment that begins after point (possibly preceded by
> whitespace). There is no comment after point in the presented scenario.
>
>> It's nil in the reported case, so 'fill-paragraph' is not called.
>
> I guess when there is an active region, we would force the behavior to
> "refill" the region, no matter whether it is inside a comment, or contains
> a comment, or outside of any comments and simply contains code?

While 'prog-fill-reindent-defun' doesn't support indentation of an
arbitrary region of code and indents only the top-level list (defun),
it looks like the right thing is to fill the region.

I still don't understand why 'M-q' now does the same what 'C-M-q' was
doing all the time with code indentation?  Also why 'prog-fill-reindent-defun'
can't indent the region of code, but only the region of comments?

Shouldn't 'M-q' only refill comments, and 'C-M-q' only indent code, as before?

bug#63790: 30.0.50; prog-fill-reindent-defun regression

[Dmitry Gutov]

On 08/06/2023 19:59, Juri Linkov wrote:
>>>     (re-search-forward "\\s-*\\s<" (line-end-position) t)
>>
>> It's looking for a comment that begins after point (possibly preceded by
>> whitespace). There is no comment after point in the presented scenario.
>>
>>> It's nil in the reported case, so 'fill-paragraph' is not called.
>>
>> I guess when there is an active region, we would force the behavior to
>> "refill" the region, no matter whether it is inside a comment, or contains
>> a comment, or outside of any comments and simply contains code?
> 
> While 'prog-fill-reindent-defun' doesn't support indentation of an
> arbitrary region of code and indents only the top-level list (defun),
> it looks like the right thing is to fill the region.

Okay?

> I still don't understand why 'M-q' now does the same what 'C-M-q' was
> doing all the time with code indentation?

C-M-q (indent-pp-sexp) reindents the list that follows point. Not the 
same thing. And it's only available in Lisp.

 > Also why 'prog-fill-reindent-defun'
 > can't indent the region of code, but only the region of comments?

Do you want it to?

> Shouldn't 'M-q' only refill comments, and 'C-M-q' only indent code, as before?

Up until now, we thought that making two actions on one key binding 
available is a good thing, given that the context usually helps to 
disambiguate. This one seems like an exception, but IMHO not a strong 
enough one to roll back the change.

bug#63790: 30.0.50; prog-fill-reindent-defun regression

[Juri Linkov]

>>>>     (re-search-forward "\\s-*\\s<" (line-end-position) t)
>>>
>>> It's looking for a comment that begins after point (possibly preceded by
>>> whitespace). There is no comment after point in the presented scenario.
>>>
>>>> It's nil in the reported case, so 'fill-paragraph' is not called.
>>>
>>> I guess when there is an active region, we would force the behavior to
>>> "refill" the region, no matter whether it is inside a comment, or contains
>>> a comment, or outside of any comments and simply contains code?
>> While 'prog-fill-reindent-defun' doesn't support indentation of an
>> arbitrary region of code and indents only the top-level list (defun),
>> it looks like the right thing is to fill the region.
>
> Okay?

Unless it's possible to make 'M-q' more predictable.

>> I still don't understand why 'M-q' now does the same what 'C-M-q' was
>> doing all the time with code indentation?
>
> C-M-q (indent-pp-sexp) reindents the list that follows point. Not the same
> thing. And it's only available in Lisp.

I tried in emacs-28 and in ruby-mode 'C-M-q' reindents the code.
This is from the Help window:

  C-M-q runs the command prog-indent-sexp (found in ruby-mode-map),
  which is an interactive byte-compiled Lisp function in ‘prog-mode.el’.

  It is bound to C-M-q, <menu-bar> <ruby> <Indent Sexp-9>.

  (prog-indent-sexp &optional DEFUN)

  Indent the expression after point.
  When interactively called with prefix, indent the enclosing defun
  instead.

>> Also why 'prog-fill-reindent-defun'
>> can't indent the region of code, but only the region of comments?
>
> Do you want it to?

It would be nice, and it's easy to implement just by calling 'indent-region'.

>> Shouldn't 'M-q' only refill comments, and 'C-M-q' only indent code, as before?
>
> Up until now, we thought that making two actions on one key binding
> available is a good thing, given that the context usually helps to
> disambiguate. This one seems like an exception, but IMHO not a strong
> enough one to roll back the change.

Before the change, the distinction was clear: 'C-M-q' reindents code, 'M-q'
refills text in comments.  Whereas I admit that 'M-q' is useless on code,
now the distinction is blurred, and DWIM is not reliable.