cmd/vet: don't complain about innocent newlines.

[robpike]

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

1.20

Does this issue reproduce with the latest release?

Yes

https://go.dev/play/p/_TMofzRghNL

package main

import "fmt"

const hello = `hello
`

func main() {
	fmt.Println(hello)
}

./x.go:9:2: fmt.Println arg list ends with redundant newline

I understand the desire to complain if there is an explicit newline in the text to fmt.Println, but when it's coming from a constant like this, I need to change the constant or use Print instead of Println. I keep hitting this problem with old code that has a nice header to be printed that desires a blank line. Requiring me to close the constant before the newline seems excessively fussy to me.

[ianlancetaylor]

CC @timothy-king

[rsc]

This particular message dates from the original commit of vet. (I reworded it to add the word 'redundant' later.) The extra precision of examining named constant arguments was introduced in Go 1.18.

% go1.16 vet /tmp/nl.go
% go1.17 vet /tmp/nl.go
% go1.18 vet /tmp/nl.go
# command-line-arguments
/tmp/nl.go:9:2: fmt.Println arg list ends with redundant newline
% 

This combination of factors suggests to me that we probably should leave things working as they are today:

• The named constant propagation happened as a side effect of this change to handle literal constant expressions. That was an important precision improvement and it found real errors in code with long literal Printf formats split across multiple source lines using + of "" strings. And the longer the format the more it needs the check.
• The expectation associated with the check has always been that if you really wanted a blank line you would write fmt.Print("abc\n\n") instead of fmt.Println("abc\n"), because the latter looks like a bug.
• Looking at the actual values of named constants seems like exactly the thing you'd want vet to do, since seeing bugs is that much harder for people reading the code when the argument is a named constant rather than a literal.
• In this case, just like the literal example, as written it is unclear whether the code really intends the blank line or its just a side effect of the way the code is written.
• I don't see how to exclude this one case without losing other similar cases that probably are bugs.

Overall, it seems reasonable for vet to flag this case, which does require rewriting code that truly wants a blank line to make that intent clearer, such as by adding the blank line to the large constant literal and using fmt.Fprint.

[timothy-king]

Relevant recent discussion #49350

I don't see how to exclude this one case without losing other similar cases that probably are bugs.

We could plausibly suppress the "*ln" checks when the ast is exactly an identifier for a constant. The challenge here is it is going to be hard for users to predict when this warning applies and when it does not. Why does fmt.Println("abc\n") warn when const abc = "abc\n"; fmt.Println(abc) does not?

The other plausible distinguishing feature is that the surprises seem to occur when a `-quoted string literal is involved. We could suppress "*ln" warnings on those and constants that are set to be equal to those.

[josharian]

I keep hitting this.

I do not believe that this check satisfies either vet's precision or its correctness criteria.

Writing:

fmt.Println("package main\n")

vs

fmt.Print("package main\n\n")

is a matter of style. (And when every other line of code nearby is fmt.Println, or when modifying existing code to add a \n, I would argue strongly for the former style.)

And anecdotally, I have personally never found this vet check to generate anything but false positives.

I believe that, were this proposed as a new vet check today, it would not be accepted. Accordingly, I think it should be removed.

[josharian]

cc @matloob as primary for go/vet according to go.dev/s/owners