Commenting is too restrictive for toggling uncomment

[seblj]

Problem

Currently the built in comment module is using the commentstring option as is, and does not assume anything about the format of it. I understand that this is intended, and I am all for this when adding a comment. However, IMO, it is a bit restrictive when toggling a comment (uncommenting).

Say I have the following file. I am working together with someone, and they have added a comment here and does so without a trailing space. Maybe they have done this without a comment plugin or something

--local foo = 10

My commentstring is the default one -- %s.
Toggling the comment will result in this:

-- --local foo = 10

This can easily be fixed with using visual block mode, or adding a space and then uncommenting.

However, say I have a project in C, and someone is commenting with the default commentstring of /*%s*/.

int main() {
    /*int foo = 10;*/
}

Then I have the autocmd from the PR that always add a space, or maybe I have changed the commentstring to have a space: /* %s */. This is a surrounding comment, and will result in:

int main() {
    /* /*int foo = 10;*/ */
}

This is even more annoying,, and I have to delete it from both sides of the line(s)

Expected behavior

I would expect it to respect the commentstring 100% when commenting, and adding however many spaces there is in the commentstring option. However, I would also expect it to properly uncomment both of these examples, since they actually are comments, and I technically have the correct commentstring.

I am open to looking into this, and sending a PR, but I would like to open an issue first to see what the general opinion is for this.

[clason]

Currently the built in comment module is using the commentstring option as is, and does not assume anything about the format of it. I understand that this is intended, and I am all for this when adding a comment.

Indeed, and this is a deliberate choice in order to avoid configuration. "Smarter" commenting is left to dedicated plugins.

[seblj]

It won't add any configuration though... It will just properly be able to uncomment when it makes sense, instead of adding nested comments

[clason]

The point is (and this was discussed at length) that the mapping is documented to act depending on whether a line matches commentstring. This change would break that.

[seblj]

I know. But I couldn't see anything related to this specific issue. It 100% makes sense for commenting, but it seems like it is only cons for uncommenting (except for maybe 5 lines of extra code)

[clason]

And 20 lines of documentation. The current limitations were the price to get it included at all; anything beyond the bare minimum was deemed out of scope for bundling.

[seblj]

Oh well, I'll give up argumenting, and accept the decision. I'll just continue to use vim-commentary then. Thank you for taking the time to explain

[gpanders]

IMO we should match vim-commentary's behavior here. This is a common use case that we should support out of the box.

[lewis6991]

And 20 lines of documentation.

Seems like a good trade-off to me.

The current limitations were the price to get it included at all; anything beyond the bare minimum was deemed out of scope for bundling.

My understanding was that there was a general consensus to include something that was vim-commentary-like. Anything in that should be in scope for inclusion (with extra consideration where necessary).

[dundargoc]

My understanding was that there was a general consensus to include something
that was vim-commentary-like.

I think that was more because it was the path of least resistance to get
something in core that we could iterate on rather than getting stuck in
bikeshed limbo. I don't think mimicking vim-commentary is inherently desirable
or an explicit goal. Case in point is that we didn't opt to upstream
vim-commentary because it lacked necessary features we wanted.

That said I don't personally mind changing the behavior in this case as it
seems useful enough.

[echasnovski]

My understanding was that there was a general consensus to include something that was vim-commentary-like. Anything in that should be in scope for inclusion (with extra consideration where necessary).

My understanding about general consensus after discussions and PR reviews was that suggested implementation was enough vim-commentary-like while differing in all relevant cases (tree-sitter support, tests, sane implementation, etc.). The use case in issue question even has explicit test.

---

The core of the issue seems to stem from the requirement that commenting functionality needs to be not configurable and work out of the box. This lead to the adoption of "100% rely on 'commentstring' when making a decision about comment structure". This means all checks (decide whether a line is commented or not), comment, and uncomment rely on the same 'commentstring'.

Treating "--aaa" line as commented with -- %s commentstring is reasonable for Lua. Yet, I don't think "ignore whitespace in comment parts when checking for comment" is universal for any language. In particular, b flag in 'comments' option is there for a reason and is used in the wild for simple line commenting (Python, bdf, netrc, and maybe more).

The most proper solution would be to have a separate options for "comment structure for adding comment" and "comment regex to match commented line", but there is no such thing. The closest is 'comments' option but it is more about how to place proper prefixes during editing (like after <CR>, O, J, etc.). It is also quite popularly used to define bulleted lists and has somewhat complicated set of flags to implement.

---

I personally would like to avoid adding special behavior which is not (at least very-very near) 100% correct. That said, after looking at the code, this is already kind of done for blank lines. So a line "--" in Lua and "/**/" in C (with "/* %s */" commentstring) are treated as commented.

What I can suggest as a solution is to use the following (for better wording I might need @clason's help):

• Line is commented if it matches 'commentstring' with trimmed parts.
• Adding comment is 100% relying on 'commentstring' structure (as it is now).
• Removing comment is first trying explicit 'commentstring' with fallback on trying its trimmed parts.

Here is the patch which passes tests:

Trimmed comment parts fallback patch

diff --git a/runtime/lua/vim/_comment.lua b/runtime/lua/vim/_comment.lua
index b6cb6c988..044cd6971 100644
--- a/runtime/lua/vim/_comment.lua
+++ b/runtime/lua/vim/_comment.lua
@@ -77,14 +77,11 @@ local function make_comment_check(parts)
   local l_esc, r_esc = vim.pesc(parts.left), vim.pesc(parts.right)
 
   -- Commented line has the following structure:
-  -- <possible whitespace> <left> <anything> <right> <possible whitespace>
-  local nonblank_regex = '^%s-' .. l_esc .. '.*' .. r_esc .. '%s-$'
-
-  -- Commented blank line can have any amount of whitespace around parts
-  local blank_regex = '^%s-' .. vim.trim(l_esc) .. '%s*' .. vim.trim(r_esc) .. '%s-$'
+  -- <whitespace> <trimmed left> <anything> <trimmed right> <whitespace>
+  local regex = '^%s-' .. vim.trim(l_esc) .. '.*' .. vim.trim(r_esc) .. '%s-$'
 
   return function(line)
-    return line:find(nonblank_regex) ~= nil or line:find(blank_regex) ~= nil
+    return line:find(regex) ~= nil
   end
 end
 
@@ -153,14 +150,14 @@ end
 ---@return fun(line: string): string
 local function make_uncomment_function(parts)
   local l_esc, r_esc = vim.pesc(parts.left), vim.pesc(parts.right)
-  local nonblank_regex = '^(%s*)' .. l_esc .. '(.*)' .. r_esc .. '(%s-)$'
-  local blank_regex = '^(%s*)' .. vim.trim(l_esc) .. '(%s*)' .. vim.trim(r_esc) .. '(%s-)$'
+  local regex = '^(%s*)' .. l_esc .. '(.*)' .. r_esc .. '(%s-)$'
+  local regex_trimmed = '^(%s*)' .. vim.trim(l_esc) .. '(.*)' .. vim.trim(r_esc) .. '(%s-)$'
 
   return function(line)
-    -- Try both non-blank and blank regexes
-    local indent, new_line, trail = line:match(nonblank_regex)
+    -- Try regex with exact comment parts first, fall back to trimmed parts
+    local indent, new_line, trail = line:match(regex)
     if new_line == nil then
-      indent, new_line, trail = line:match(blank_regex)
+      indent, new_line, trail = line:match(regex_trimmed)
     end
 
     -- Return original if line is not commented
diff --git a/test/functional/lua/comment_spec.lua b/test/functional/lua/comment_spec.lua
index 9b1d9613c..0ca4f2158 100644
--- a/test/functional/lua/comment_spec.lua
+++ b/test/functional/lua/comment_spec.lua
@@ -314,13 +314,13 @@ describe('commenting', function()
       validate(3, 3, { '#aa', '# aa', '  aa' })
 
       set_commentstring('# %s')
-      validate(1, 3, { '# #aa', '# # aa', '# #  aa' })
+      validate(1, 3, { 'aa', 'aa', ' aa' })
       validate(2, 3, { '#aa', 'aa', ' aa' })
       validate(3, 3, { '#aa', '# aa', ' aa' })
 
       set_commentstring('#  %s')
-      validate(1, 3, { '#  #aa', '#  # aa', '#  #  aa' })
-      validate(2, 3, { '#aa', '#  # aa', '#  #  aa' })
+      validate(1, 3, { 'aa', ' aa', 'aa' })
+      validate(2, 3, { '#aa', ' aa', 'aa' })
       validate(3, 3, { '#aa', '# aa', 'aa' })
     end)

@gpanders, @lewis6991, @clason: will this be enough? Please, look closely at how test cases changed.

[clason]

If it's reasonably straightforward to document understandably and -- crucially -- works for every commentstring (yes, even COBOL) then I don't mind.

Having support for multi-valued commentstrings would be interesting as well but would require upstream buy-in (and is effectively blocked by option refactor). (But since Vim recently added their own built-in commenting, they may be interested in this as well; this would allow supporting both /*%s*/ and // %s in C, for example.)

[seblj]

What I can suggest as a solution is to use the following (for better wording I might need @clason's help):

• Line is commented if it matches 'commentstring' with trimmed parts.
• Adding comment is 100% relying on 'commentstring' structure (as it is now).
• Removing comment is first trying explicit 'commentstring' with fallback on trying its trimmed parts.

This is exactly what I was thinking at least

[echasnovski]

If it's reasonably straightforward to document understandably ...

My best attempt at documenting was in the comment. May be not concise enough for help.

... and -- crucially -- works for every commentstring (yes, even COBOL) then I don't mind.

It works in a sense of "does not break editing", but not necessarily in a sense "aligns with how comments are treated by the language" (as those might require having whitespace). And COBOL fieltype does not have 'commentstring' set 🤔

[clason]

It does:

setlocal commentstring=\ \ \ \ \ \ *%s

[gpanders]

It works in a sense of "does not break editing", but not necessarily in a sense "aligns with how comments are treated by the language" (as those might require having whitespace).

I still am not aware of a single language in existence that requires whitespace after a comment character. Even the examples you gave in your earlier comment are sus: the Python ftplugin uses the b flag in the 'comments' option, but you certainly do not need whitespace after # for a comment in Python. So I would not trust the existing runtime files in that regard.

[gpanders]

@echasnovski Your patch looks good to me and gives the behavior I would expect.

[ribru17]

I tested and it also works perfectly with two-sided commentstrings like <!--\ %s\ -->!

[clason]

And also COBOL?

[ribru17]

Yes! (Maybe the two cases should get their own additional tests)