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
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@ainar-g
Copy link
Contributor

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

/cc @alandonovan

@katiehockman katiehockman added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 4, 2019
@katiehockman katiehockman added this to the Go1.14 milestone Sep 4, 2019
@alandonovan
Copy link
Contributor

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

@ainar-g
Copy link
Contributor Author

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

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

@katiehockman katiehockman added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 4, 2019
@hasitpbhatt
Copy link
Contributor

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

@mtricht
Copy link

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

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

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

@gopherbot
Copy link

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>
@golang golang locked and limited conversation to collaborators Sep 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants