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: false positive about wrong type in printf when struct with unexported field is printed #23563

Closed
octomarat opened this issue Jan 26, 2018 · 9 comments

Comments

Projects
None yet
4 participants
@octomarat
Copy link

commented Jan 26, 2018

Please answer these questions before submitting your issue. Thanks!

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

go1.10rc1 darwin/amd64

Does this issue reproduce with the latest release?

Not reproduced in the latest stable release (1.9.3), only in 1.10rc1

What did you do?

Run go vet on the following file:

package main

import "fmt"

type unexportedError struct {
	e error
}

type errorer struct{ s string }

func (e errorer) Error() string { return "errorer" }

func main() {
	value := errorer{ s: "just field" }
	ue := unexportedError{ e: value }
	fmt.Printf("%s\n", ue)
}

What did you expect to see?

No errors as file executes normally and prints {{just field }}.

What did you see instead?

go vet reports Printf format %s has arg ue of wrong type main.unexportedError.

As fmt package doc states, Error method of e field cannot be used, as e is unexported. In this case, fmt just prints fields.

As far as I can see in go vet source code, it reports error if field has Error method but it is unexported. But the fact that e can be printed without Error() method invocation is not taken into account.

@mvdan mvdan self-assigned this Jan 26, 2018

@mvdan mvdan added this to the Go1.11 milestone Jan 26, 2018

@mvdan

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

This seems to be related to unexported interface fields - this is a smaller reproducer:

package main

import "fmt"

type T1 struct {
        t2 interface{}
}

func main() {
        v := T1{"foo"}
        fmt.Printf("%s\n", v)
}
@gopherbot

This comment has been minimized.

Copy link

commented Jan 29, 2018

Change https://golang.org/cl/90516 mentions this issue: cmd/vet: unexported interface fields on %s are ok

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2018

I'm going to redirect this to 1.10 because with 1.10 this occurs when running go test as well as when running go vet.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.10 Jan 29, 2018

@mvdan

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

@ianlancetaylor please see all the other recent vet issues that OP raised. These could all be considered regressions in 1.10, given how vet is now run as part of test. I have sent CLs for 4 of these already.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2018

As I more or less said on the CL, I'm not yet convinced that this is a bug. The right way to print a value of type error is to call the Error method. It's true that in this case fmt does the best it can and just prints the value, but I think that is not what people would reasonably expect. People would reasonably expect a call to the Error method. So this does seem worthy of a vet warning.

CC @robpike @martisch for other opinions.

@mvdan

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

@ianlancetaylor you may be right. However, note that my CL affects all interfaces, not just error and fmt.Stringer. Perhaps the CL should keep the warning for those types.

Edit: what I mean is that vet could warn on struct { f errorImpl } and struct { f error }, but not on struct { f interface{} } and any other field type that doesn't imply error or fmt.Stringer.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2018

@mvdan Is that not how it works today?

@mvdan

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

@ianlancetaylor not exactly - see my full example in #23563 (comment). The t2 interface{} field gets flagged, when the user couldn't reasonably expect String() string or Error() string to be present.

So I agree that OP's example is working as intended (even if the program technically works), but I think that the current heuristic is too aggressive. It should only cover types that we know to have String or Error.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2018

Thanks, that sounds reasonable. I guess we should also warn about uncallable Format and GoString methods.

@gopherbot gopherbot closed this in f54f780 Feb 6, 2018

asjoyner added a commit to asjoyner/shade that referenced this issue Oct 22, 2018

Resolve the pointer on AesKey and print as %v
I think this will fix an error in go 1.10 which may or may not be a
false positive.  I think it's the same as what's discussed here:
golang/go#23563 (comment)

Since I don't have go 1.10 locally, I'm going to push it up and see if
it fixes the Travis-CI build.  :)
https://travis-ci.org/asjoyner/shade/jobs/444793514

@golang golang locked and limited conversation to collaborators Feb 6, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.