bug#41793: `comment-only-p' erroneously flags blank lines as comments

bug#41793: `comment-only-p' erroneously flags blank lines as comments

[Toby Cubitt]

In GNU Emacs 26.3 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.16)
 of 2020-04-28

On the following text, with point at |
(comment-only-p (point) (1+ (point)))
returns t when it should probably return nil:

"
|

test"

This impacts commands like `comment-or-uncomment-region'.

This is because the current implementation of `comment-only-p' fails to check the return value of (comment-forward):

(defun comment-only-p (beg end)
  "Return non-nil if the text between BEG and END is all comments."
  (save-excursion
    (goto-char beg)
    (comment-forward (point-max))
    (<= end (point))))

The correct implementation should probably be:

(defun comment-only-p (beg end)
  "Return non-nil if the text between BEG and END is all comments."
  (save-excursion
    (goto-char beg)
    (and (comment-forward (point-max))
         (<= end (point)))))

Tested on 26.3, but implementation of `comment-only-p' is unchanged in latest git.

Cheers,
Toby
-- 
Dr T. S. Cubitt
Reader (Associate Professor) in Quantum Information
Royal Society University Research Fellow
Department of Computer Science
University College London

email: tsc25@cantab.net
web:   www.dr-qubit.org

bug#41793: `comment-only-p' erroneously flags blank lines as comments

[Toby Cubitt]

On Wed, Jun 10, 2020 at 06:53:31PM +0100, Toby Cubitt wrote:
> In GNU Emacs 26.3 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.16)
>  of 2020-04-28
> 
> On the following text, with point at |
> (comment-only-p (point) (1+ (point)))
> returns t when it should probably return nil:
> 
> "
> |
> 
> test"
> 
> This impacts commands like `comment-or-uncomment-region'.
> 
> This is because the current implementation of `comment-only-p' fails to check the return value of (comment-forward):
> 
> The correct implementation should probably be:
[snip]

Gah. That attempt breaks flagging of comments separated by whitespace-only lines. Maybe this?

(defun comment-only-p (beg end)
  "Return non-nil if the text between BEG and END is all comments."
  (if (string-blank-p (buffer-substring beg end)) nil
    (save-excursion
      (goto-char beg)
      (comment-forward (point-max))
      (<= end (point)))))

This fix successfully makes `comment-or-uncomment-region' call `comment-region' when fed empty lines, instead of calling `uncomment-region' (which seems wrong).

It doesn't fix the main issue I was trying to address, namely that `comment-or-uncomment-region' fails to comment out a region consisting only of blank lines, even when `comment-empty-lines' is t. Without the above fix, it calls `uncomment-region' which does nothing as there's nothing to comment. With the above fix, it calls `comment-region'. But the latter refuses to comment out the lines, throwing a "Nothing to comment" error.

Fixing that (assuming it's considered a bug) would require more changes to `comment-region-default' and `comment-region-internal'.

The use-case for this was editing a LaTeX document, where empty lines are semantically significant (they demark paragraph breaks). It's fairly common (especially when editing co-authored documents) to comment out the empty lines in order to run two paragraphs together, whilst keeping the commented-out empty lines in the source so it's easy to revert.

Best,
Toby
-- 
Dr T. S. Cubitt
Reader (Associate Professor) in Quantum Information
Royal Society University Research Fellow
Department of Computer Science
University College London

email: tsc25@cantab.net
web:   www.dr-qubit.org

bug#41793: `comment-only-p' erroneously flags blank lines as comments

[Lars Ingebrigtsen]

Toby Cubitt <toby-dated-1593024851.318b83@dr-qubit.org> writes:

> Gah. That attempt breaks flagging of comments separated by whitespace-only lines. Maybe this?
>
> (defun comment-only-p (beg end)
>   "Return non-nil if the text between BEG and END is all comments."
>   (if (string-blank-p (buffer-substring beg end)) nil
>     (save-excursion
>       (goto-char beg)
>       (comment-forward (point-max))
>       (<= end (point)))))

I don't think that can be correct...  for instance, it'll return nil on
this C comment:

/*
|

*/

> This fix successfully makes `comment-or-uncomment-region' call
> `comment-region' when fed empty lines, instead of calling
> `uncomment-region' (which seems wrong).
>
> It doesn't fix the main issue I was trying to address, namely that
> `comment-or-uncomment-region' fails to comment out a region consisting
> only of blank lines, even when `comment-empty-lines' is t. Without the
> above fix, it calls `uncomment-region' which does nothing as there's
> nothing to comment. With the above fix, it calls `comment-region'. But
> the latter refuses to comment out the lines, throwing a "Nothing to
> comment" error.
>
> Fixing that (assuming it's considered a bug) would require more
> changes to `comment-region-default' and `comment-region-internal'.
>
> The use-case for this was editing a LaTeX document, where empty lines
> are semantically significant (they demark paragraph breaks). It's
> fairly common (especially when editing co-authored documents) to
> comment out the empty lines in order to run two paragraphs together,
> whilst keeping the commented-out empty lines in the source so it's
> easy to revert.

Right -- `M-x comment-region' on a blank region will just say "Nothing
to comment" here.  Which makes sense in most modes, but not in LaTeX
mode, I think.

I've now fixed `M-x comment-region' for blank lines in `latex-mode' in
Emacs 28, I think, but `M-x comment-or-uncomment-region' still doesn't
work because `comment-only-p' is, as you say, wrong.  Your first
attempt looked like it had promise, but then you said:

> Gah. That attempt breaks flagging of comments separated by
> whitespace-only lines. Maybe this?

Do you have an example of when that fails?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#41793: `comment-only-p' erroneously flags blank lines as comments

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> Gah. That attempt breaks flagging of comments separated by
>> whitespace-only lines. Maybe this?
>
> Do you have an example of when that fails?

The test was

;; foo
;;       
;; bar

where comment-only-p would give the wrong results.

Anyway, I think it's ambiguous whether whitespace only should be a
comment or not.  The current algo considers this to be one comment:

;; foo

;; bar

And I think that's correct.  If we consider whitespace lines to not be
comments, this would change how our comment navigation commands work.

So there's some DWIM in this area, but I think how it currently works is
basically the way we want it to work, so I'm closing this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#41793: `comment-only-p' erroneously flags blank lines as comments

[Drew Adams]

> Anyway, I think it's ambiguous whether whitespace only 
> should be a comment or not.  The current algo considers
> this to be one comment:
> 
> ;; foo
> 
> ;; bar
> 
> And I think that's correct.  If we consider whitespace lines to not be
> comments, this would change how our comment navigation commands work.

We do NOT consider that to be the case.  Never have.

> So there's some DWIM in this area, but I think how it currently works
> is basically the way we want it to work, so I'm closing this bug report.

Those are two separate comments.  The line between them
is NOT commented out.

A comment is delimited by `comment-start' and
`comment-end'.  (And `comment-(start|end)-skip' and
`comment-end-can-be-escaped', if you like.)

Comments that have "" as `comment-end' "are terminated
by end-of-line" (to quote the doc string).

The doc is quite clear about all of this, IMO - in doc
strings, in the Emacs manual, and in the Elisp manual.

A Lisp comment "continues to the end of the line" - no
further.  ((elisp) `Comments'.)

Beyond the doc, this is fundamental to Elisp behavior.
Try `(forward-comment 1) on your test case, for example.

Code depends on `forward-comment' and the like,
including thingatpt.el code.
___

Whether `comment-only-p' should or should not report
`t' here is a different question from what I addressed
above.

That depends on what the intention of `comment-only-p
is.  I suggest you find out what that function was
intended for, and what existing code might depend on
its current behavior.

But one thing is certain: what you said about the
example you show being a single comment is 100% wrong.
It is two comments.

The same thing is true for this example, BTW:

;; foo
;; bar

Those are two comments.  It's possible, maybe even
likely, that the intention of `comment-only-p' is
to return non-nil when one comment is immediately
followed by another.  Or maybe even when one is
followed by whitespace and then by another.  TBD.

But those are two separate comments, each delimited
by the first `;' on each line and the first
end-of-line that follows that first `;'.