Navigation Menu

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

Jump to bottom

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

Closed
dmitshur opened this issue Nov 3, 2021 · 12 comments
Closed

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

dmitshur opened this issue Nov 3, 2021 · 12 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.18

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Nov 3, 2021 •
edited

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.
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Nov 3, 2021
@dmitshur dmitshur added this to the Go1.18 milestone Nov 3, 2021
@dmitshur dmitshur mentioned this issue Nov 3, 2021
Closed
@dmitshur

This comment has been minimized.

Sign in to view
@bcmills
Copy link
Contributor

bcmills commented Nov 3, 2021

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
Copy link
Contributor Author

dmitshur commented Nov 3, 2021

@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
Copy link
Contributor

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
Copy link
Contributor

bcmills commented Nov 3, 2021

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

@rsc
Copy link
Contributor

rsc commented Nov 3, 2021

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Nov 4, 2021
@zpavlinovic
cmd/cover: use fmt.Print for newline-ending fixed string
9cf6711 
This redundancy is now caught by the improved printf vet checker
(#30436).

Updates #49322

Change-Id: Id450247adc6fa28a9244c019be3c1b52c2d17f49
Reviewed-on: https://go-review.googlesource.com/c/go/+/361263
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Zvonimir Pavlinovic <zpavlinovic@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit that referenced this issue Nov 4, 2021
@zpavlinovic
cmd/trace: use fmt.Print for newline-ending fixed string
1292e21 
This redundancy is now caught by the improved printf vet checker.

Updates #49322

Change-Id: Ic7a931b8d4838be02ebb855b69624b95093bd114
Reviewed-on: https://go-review.googlesource.com/c/go/+/361265
Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Zvonimir Pavlinovic <zpavlinovic@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@hyangah
Copy link
Contributor

hyangah commented Nov 4, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

dmitshur commented Nov 4, 2021 •
edited

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
Copy link

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

@hyangah hyangah mentioned this issue Nov 4, 2021
Closed
@golang golang locked and limited conversation to collaborators Nov 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests
6 participants
@rsc @zpavlinovic @dmitshur @hyangah @bcmills @gopherbot