Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/gofmt: Trailing newline in comments is removed since 1.19 #54489

Open
bjwrk opened this issue Aug 16, 2022 · 9 comments
Open

cmd/gofmt: Trailing newline in comments is removed since 1.19 #54489

bjwrk opened this issue Aug 16, 2022 · 9 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@bjwrk
Copy link

bjwrk commented Aug 16, 2022

What version of Go are you using (go version)?

$ go version
go version go1.19 darwin/arm64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

$ go env
GOARCH="arm64"
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOOS="darwin"
GOVERSION="go1.19"

What did you do?

I saved an existing file that had a dense and detailed comment followed by a dense and detailed function signature, that contained a deliberate trailing newline to separate the last paragraph from the function's signature to improve readability:

package yep

// Sed facilisis diam vulputate, lobortis lacus vel, volutpat mi. In quis dolor ac nisi
// volutpat dictum sed at lorem. Curabitur sodales diam a porttitor congue. Quisque diam
// sem, porta ac interdum consectetur, ornare consectetur quam. Sed imperdiet tincidunt
// turpis, at consectetur purus ullamcorper et. Curabitur egestas turpis at mauris
// sollicitudin convallis.
//
// In pulvinar purus nec lacus pulvinar placerat sit amet scelerisque elit. Donec tortor
// ante, laoreet ac congue at, facilisis vitae nulla. Vestibulum porta ultrices augue, ut
// hendrerit odio tincidunt id. In interdum sagittis neque, egestas placerat turpis luctus
// et. Sed fringilla purus a vehicula euismod. Phasellus ut lorem non quam aliquam tempus.
// Aliquam a dapibus ex. Maecenas erat purus, facilisis vitae elit at, aliquam eleifend.
//
func DoStuff[K comparable, V ~int64 | ~uint64](key K, value V) (stuff Stuff err error) {
    // ...
}

What did you expect to see?

I expected to see the final newline in the comment body preserved.

What did you see instead?

The final newline was removed:

// Sed facilisis diam vulputate, lobortis lacus vel, volutpat mi. In quis dolor ac nisi
// volutpat dictum sed at lorem. Curabitur sodales diam a porttitor congue. Quisque diam
// sem, porta ac interdum consectetur, ornare consectetur quam. Sed imperdiet tincidunt
// turpis, at consectetur purus ullamcorper et. Curabitur egestas turpis at mauris
// sollicitudin convallis.
//
// In pulvinar purus nec lacus pulvinar placerat sit amet scelerisque elit. Donec tortor
// ante, laoreet ac congue at, facilisis vitae nulla. Vestibulum porta ultrices augue, ut
// hendrerit odio tincidunt id. In interdum sagittis neque, egestas placerat turpis luctus
// et. Sed fringilla purus a vehicula euismod. Phasellus ut lorem non quam aliquam tempus.
// Aliquam a dapibus ex. Maecenas erat purus, facilisis vitae elit at, aliquam eleifend.
func DoStuff[K comparable, V ~int64 | ~uint64](key K, value V) (stuff Stuff err error) {
    // ...
}

I'd really like to see the old behaviour restored here as function signatures like the one in my example can be crowded out by the last line of the preceding paragraph.

@seankhliao seankhliao added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 16, 2022
@seankhliao
Copy link
Member

cc @rsc

@cespare
Copy link
Contributor

cespare commented Dec 18, 2022

The situation where I find this particularly annoying is when I'm using indented blocks. I think they look best with blank lines around them for symmetry. And gofmt apparently does too, because it will add those blank lines if they're missing. But it gets rid of the trailing blank line if it happens to be the last line in the comment. For example, this:

// Foo blah blah. Sed facilisis diam vulputate, lobortis lacus vel, volutpat mi.
// In quis dolor ac nisi volutpat dictum sed at lorem. Curabitur sodales diam a
// porttitor congue.
//	x := 3
//	Foo() // asdf
// Quisque diam sem, porta ac interdum consectetur, ornare consectetur quam. Sed
// imperdiet tincidunt turpis, at consectetur purus ullamcorper et. Curabitur
// egestas turpis at mauris sollicitudin convallis.
//	Blah()
//	Foo()
func Foo() {}

is turned into this:

// Foo blah blah. Sed facilisis diam vulputate, lobortis lacus vel, volutpat mi.
// In quis dolor ac nisi volutpat dictum sed at lorem. Curabitur sodales diam a
// porttitor congue.
//
//	x := 3
//	Foo() // asdf
//
// Quisque diam sem, porta ac interdum consectetur, ornare consectetur quam. Sed
// imperdiet tincidunt turpis, at consectetur purus ullamcorper et. Curabitur
// egestas turpis at mauris sollicitudin convallis.
//
//	Blah()
//	Foo()
func Foo() {}

I think it looks best like this:

// Foo blah blah. Sed facilisis diam vulputate, lobortis lacus vel, volutpat mi.
// In quis dolor ac nisi volutpat dictum sed at lorem. Curabitur sodales diam a
// porttitor congue.
//
//	x := 3
//	Foo() // asdf
//
// Quisque diam sem, porta ac interdum consectetur, ornare consectetur quam. Sed
// imperdiet tincidunt turpis, at consectetur purus ullamcorper et. Curabitur
// egestas turpis at mauris sollicitudin convallis.
//
//	Blah()
//	Foo()
//
func Foo() {}

but gofmt deletes the last blank line.

There are some examples of this in the stdlib; for instance, compare these from strings:

// SplitN slices s into subslices separated by sep and returns a slice of
// the subslices between those separators.
// If sep is empty, SplitN splits after each UTF-8 sequence.
// The count determines the number of subslices to return:
//
//	n > 0: at most n subslices; the last subslice will be the unsplit remainder.
//	n == 0: the result is nil (zero subslices)
//	n < 0: all subslices
//
// To split around the first instance of a separator, see Cut.
func SplitN(s, sep []byte, n int) [][]byte { return genSplit(s, sep, 0, n) }

// SplitAfterN slices s into subslices after each instance of sep and
// returns a slice of those subslices.
// If sep is empty, SplitAfterN splits after each UTF-8 sequence.
// The count determines the number of subslices to return:
//
//	n > 0: at most n subslices; the last subslice will be the unsplit remainder.
//	n == 0: the result is nil (zero subslices)
//	n < 0: all subslices
func SplitAfterN(s, sep []byte, n int) [][]byte {
	return genSplit(s, sep, len(sep), n)
}

@glycerine
Copy link

See #57388 (closed as duplicate of this issue) for additional commentary and insight.

@mvdan
Copy link
Member

mvdan commented Jan 16, 2023

cc @griesemer for thoughts. My intuition is that I prefer godoc styles to be consistent, and when a project has a mix of godocs in both styles (with and without the trailing empty line), it distracts my reading and editing of godocs. So I lean against reverting to the old system, where we let the user choose whether to leave the trailing line on each godoc.

If there are cases where a trailing empty line helps readability, such as when a godoc ends in an indented section like @cespare says, then perhaps we should consider enforcing the trailing empty line in those cases. One could argue that always requiring empty lines around indented blocks, even when they are at the end of a godoc, would improve consistency, since the following is not properly formatted:

// Regular paragraph.
//
//	block(quote)
// Another regular paragraph.
func F()

If a godoc doesn't end in a block quote, I don't think a trailing line helps, and most of the code I've read and written doesn't use one in that case. So I'd definitely be against always enforcing a trailing empty line.

@changkun
Copy link
Member

See also robpike/typo#3 (comment)

@bjwrk
Copy link
Author

bjwrk commented Mar 18, 2023

So I'd definitely be against always enforcing a trailing empty line.

@mvdan - It shouldn't enforce a trailing empty line, it should just leave one alone if it's present. There's plenty of prior art in gofmt for this approach of "if it can go both ways and it's not too bad then leave it alone". I realise that as the author of gofumpt, that approach is probably more loose than you'd tolerate, but the situation we have now where gofmt is a bit more relaxed and teams that want to be more strict can opt-in to gofumpt is the optimal strategy for being the "most things to the most people".

Having gofmt apply a gofmt level of strictness to code, but a gofumpt level of strictness to comments, seems inconsistent and overly restrictive to me.

@mvdan
Copy link
Member

mvdan commented Mar 18, 2023

I don't think gofumpt is part of the equation here. Per this issue, gofmt already seems to be a bit too strict, and that strictness came with the godoc redesign and reformatting, not gofumpt.

In my previous comment, I advocate that perhaps gofmt should require an empty line after every indented block in a godoc, as I think that would solve the problem that @cespare mentions. And I can agree that, in the example that he shows, the new behavior hurts readability, and breaks the symmetry with code blocks to some extent.

As for allowing a trailing empty line in other cases, like your original example - I personally don't think such an empty line helps readability in those other cases. That's of course subjective, but gofmt is always a balance between consistency and user preference. The vast majority of Go users seem to not mind the new strictness, so I'm inclined to only fix indented blocks.

I'm only a secondary maintainer for gofmt, though. I'd prefer to get @griesemer's input to make a decision :)

@HeCorr

This comment was marked as off-topic.

@mvdan

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

7 participants