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: printfunc false negative on struct without String method #17798

Closed
josharian opened this issue Nov 4, 2016 · 11 comments

Comments

Projects
None yet
5 participants
@josharian
Copy link
Contributor

commented Nov 4, 2016

package main

import "fmt"

type T int

func (T) String() string { return "T" }

type S struct {
	t T
}

func main() {
	var t T
	fmt.Printf("%s\n", t)
	var s S
	fmt.Printf("%s\n", s)
}
$ go tool vet x.go
$ go run x.go 
T
{%!s(main.T=0)}

vet should know that S isn't a fmt.Stringer. The printfunc's "satisfies interface" check needs improvements. Related is #17057.

cc @robpike @valyala

@josharian josharian added the NeedsFix label Nov 4, 2016

@josharian josharian added this to the Go1.9 milestone Nov 4, 2016

@valyala

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

I'll look into this and the #17057 on the weekend.

@valyala

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

FYI, it looks like the cl 28959 breaks printf check on errror interfaces:

err := errors.New("foobar")
fmt.Printf("%d", err) // vet silently skips this
@valyala

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2016

I think it would be better to fix these issues in go 1.8.

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2016

CL 28959 doesn't change cmd/vet at all. It only changes cmd/vet/all, which is a script that runs cmd/vet over the standard library.

I think it would be better to fix these issues in go 1.8.

If the changes are fairly safe and we can get them in soon, I think that's fine.

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2016

FYI, it looks like the cl 28959 breaks printf check on errror interfaces:

Btw, this is intentional. See #16314.

@valyala

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2017

This looks like a bug in package fmt - it must properly print all the struct fields if they implement fmt.Stringer according to the rule:

For compound operands such as slices and structs, the format applies to the elements of each operand, recursively, not to the operand as a whole.

In this case struct S contains only a single field of type T, which implements Stringer. So fmt.Printf must print contents of the struct - {T} instead of printing {%!s(main.T=0)}.

fmt.Printf works as expected for slices, arrays and maps with fmt.Stringer values, but doesn't work for structs:

type S [5]T
type S []T
type S map[string]T
@valyala

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2017

cc'ing @ALTree , @martisch and @robpike

@martisch

This comment has been minimized.

Copy link
Member

commented Jun 2, 2017

type S struct {
	t T
}

the field t is not exported. If t is changed to be exported fmt will print {T}. fmt is in another package and does not have access to call the String method of t if it is not exported.

see also #18281 #17409 #16698

@gopherbot

This comment has been minimized.

Copy link

commented Jun 5, 2017

CL https://golang.org/cl/44831 mentions this issue.

@mvdan mvdan modified the milestones: Go1.10, Go1.9 Jun 16, 2017

@mvdan

This comment has been minimized.

Copy link
Member

commented Jun 16, 2017

CL was marked as R=go1.10, so moving the issue milestone too.

@gopherbot gopherbot closed this in 6959087 Aug 29, 2017

@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

gopherbot pushed a commit that referenced this issue Feb 6, 2018

cmd/vet: unexported interface{} fields on %s are ok
For example, the following program is valid:

	type T struct {
		f interface{}
	}

	func main() {
		fmt.Printf("%s", T{"foo"}) // prints {foo}
	}

Since the field is of type interface{}, we might have any value in it.
For example, if we had T{3}, fmt would complain. However, not knowing
what the type under the interface is, we must be conservative.

However, as shown in #17798, we should issue an error if the field's
type is statically known to implement the error or fmt.Stringer
interfaces. In those cases, the user likely wanted the %s format to call
those methods. Keep the vet error in those cases.

While at it, add more field type test cases, such as custom error types,
and interfaces that extend the error interface.

Fixes #23563.

Change-Id: I063885955555917c59da000391b603f0d6dce432
Reviewed-on: https://go-review.googlesource.com/90516
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

@golang golang locked and limited conversation to collaborators Jan 29, 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.