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/vet: check against fmt.Errorf uses with more than one %w #34062

Closed
ainar-g opened this issue Sep 4, 2019 · 9 comments
Closed

cmd/vet: check against fmt.Errorf uses with more than one %w #34062

ainar-g opened this issue Sep 4, 2019 · 9 comments
Milestone

Comments

@ainar-g
Copy link
Contributor

@ainar-g ainar-g commented Sep 4, 2019

fmt.Println(fmt.Errorf("bad: %w %w", errors.New(""), errors.New("")))

https://play.golang.org/p/Tk4meXDxL7b

I expected Go 1.13's vet to mark this as a bad format, but it doesn't seem to do that. I think it should.

@katiehockman

This comment has been minimized.

Copy link
Member

@katiehockman katiehockman commented Sep 4, 2019

@katiehockman katiehockman added this to the Go1.14 milestone Sep 4, 2019
@alandonovan

This comment has been minimized.

Copy link
Contributor

@alandonovan alandonovan commented Sep 4, 2019

Why? Each verb is accompanied by a value of the appropriate type.

@ainar-g

This comment has been minimized.

Copy link
Contributor Author

@ainar-g ainar-g commented Sep 4, 2019

Are you asking, why should this be marked as an error at all or why it should be in vet? If the former, because only one %w is allowed, per the fmt docs. Some people were already confused, see #34060. As for the latter, vet already checks format strings, so it seems like an appropriate place.

@alandonovan

This comment has been minimized.

Copy link
Contributor

@alandonovan alandonovan commented Sep 4, 2019

Ah, I didn't realize fmt permitted only one. Then yes, that's an easy and appropriate thing for vet to catch.

@hasitpbhatt

This comment has been minimized.

Copy link
Contributor

@hasitpbhatt hasitpbhatt commented Sep 5, 2019

If it's not super critical, I'd like to take a stab at it.

@mtricht

This comment has been minimized.

Copy link

@mtricht mtricht commented Sep 9, 2019

The following CL already provides this functionality with tests included: https://go-review.googlesource.com/c/tools/+/177601
The vendored version of golang.org/x/tools in src/cmd refers to the version 25a4f137592f which does not contain this functionality. Updating golang.org/x/tools in there should solve this.

Hopefully this helps anyone, I'm rather new to this repository. I tried updating the vendored x/tools with:

cd src/cmd
go get -d golang.org/x/tools@latest
go mod tidy
go mod vendor

as README.vendor suggests me to, but this only resulted in the error no dependencies to vendor, the vendor directory being deleted and thus the build failing. I'm sure someone smarter than me will solve this in no time.

@hasitpbhatt

This comment has been minimized.

Copy link
Contributor

@hasitpbhatt hasitpbhatt commented Sep 24, 2019

@alandonovan Due to #34191, could not check for this specific test "TestScript". Change can be found in https://polymer2-go-review.googlesource.com/c/go/+/196843

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 24, 2019

Change https://golang.org/cl/196843 mentions this issue: cmd: update golang.org/x/tools

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 25, 2019

Change https://golang.org/cl/197338 mentions this issue: go/analysis: fix vet errors

gopherbot pushed a commit to golang/tools that referenced this issue Sep 25, 2019
Updating tools version in go fails the builds due to go vet errors as it can be observed in https://golang.org/cl/196843.

Fix vet errors in facts.go and assign.go

Updates golang/go#34062

Change-Id: I8e5a819a08d0bdc91c4fb21761065f026581bcd2
GitHub-Last-Rev: 57d8329
GitHub-Pull-Request: #164
Reviewed-on: https://go-review.googlesource.com/c/tools/+/197338
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
@gopherbot gopherbot closed this in 739bf6b Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.