cmd/cover, cmd/trace, cmd/vendor/github.com/google/pprof/internal/report: fmt.Fprintln arg list ends with redundant newline

[dmitshur]

Issue #30436 was about improving cmd/vet to detect when a fmt.Fprintln arg list ends with a redundant newline regardless of syntax (previously, it worked only when the string was a ast.BasicLit).

CL 356830 resolved that issue recently, and when it is brought into the main tree (as is being done in CL 361094, it finds 3 problems in the standard library tree:

• cmd/cover/cover.go:43:2: fmt.Fprintln arg list ends with redundant newline

go/src/cmd/cover/cover.go

Line 43 in cfd016d

fmt.Fprintln(os.Stderr, usageMessage)

• cmd/trace/main.go:69:3: fmt.Fprintln arg list ends with redundant newline

go/src/cmd/trace/main.go

Line 69 in cfd016d

fmt.Fprintln(os.Stderr, usageMessage)

• cmd/vendor/github.com/google/pprof/internal/report/source.go:877:2: fmt.Fprintln arg list ends with redundant newline

go/src/cmd/vendor/github.com/google/pprof/internal/report/source.go

Line 877 in cfd016d

fmt.Fprintln(w, weblistPageClosing)

(Build log.)

Those packages fail to build because cmd/go (CC @bcmills, @matloob) considers this vet problem fatal during go test. E.g.:

$ go test cmd/cover
# cmd/cover
cmd/cover/cover.go:43:2: fmt.Fprintln arg list ends with redundant newline
FAIL	cmd/cover [build failed]
FAIL

This needs to be resolved to unblock the 1.18 release.

As far as I understand, the vet check is working as intended and we should fix the 3 places. The one in pprof will need to fixed upstream and vendored in.

The improvement to the vet check may be worth mentioning in 1.18 release notes (#47694).

CC @golang/release, @zpavlinovic, @timothy-king.

[bcmills]

I think the promotion to failure actually comes via buildall.bash:

go/src/buildall.bash

Lines 72 to 80 in 2ebe77a

	# Build and vet everything.
	# cmd/go/internal/work/exec.go enables the same vet flags during go test of std cmd
	# and should be kept in sync with any vet flag changes here.
	if ! "$GOROOT/bin/go" build std cmd || ! "$GOROOT/bin/go" vet -unsafeptr=false std cmd; then
	failed=true
	if $sete; then
	exit 1
	fi
	fi

[dmitshur]

@bcmills Ah, indeed, thanks. I misread the "[build failed]" line. Can confirm building is okay:

$ go test cmd/cover
# cmd/cover
cmd/cover/cover.go:43:2: fmt.Fprintln arg list ends with redundant newline
FAIL	cmd/cover [build failed]
FAIL
$ go build cmd/cover
$ echo $?
0

[zpavlinovic]

These look like TPs to me (although the severity might be low). Given that build actually succeeds, is this still considered a release-blocker?

[bcmills]

Yes: vet failures are test failures, and we can't cut a release with failing tests.

[rsc]

Yes, and for future vet changes, the tree must always be clean before committing the vet check. Thanks!

[gopherbot]

Change https://golang.org/cl/361265 mentions this issue: cmd/trace: use fmt.Print for newline-ending fixed string

[gopherbot]

Change https://golang.org/cl/361263 mentions this issue: cmd/cover: use fmt.Print for newline-ending fixed string

[hyangah]

Can someone teach me why it is a bad idea to use Println with a newline-ending fixed string? If someday I change the string not to include a new line, should I change the code to use fmt.Println? Or if I want to add one more new line for aesthetic reasons, using Println to add one more extra line is a misuse of the language?

[zpavlinovic]

It looks to me that catching patterns mentioned above was the intention of the checker from the outset. That is, the reason why this did not pop up earlier is due to a certain limitation. I might be wrong though.

[dmitshur]

from the outset

I was hoping to find some more detail about the motivation behind this check, and looked for the CL that added it. It looks like it was added in the "initial commit" for vet in 2010, CL 3522041. Back then, it worked on *ast.BasicLit only. It's possible #30436 is a bigger improvement for other print checks like "possible formatting directive", and maybe it scales less well for the newline check.

[gopherbot]

Change https://golang.org/cl/361294 mentions this issue: cmd/pprof: update vendored github.com/google/pprof