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

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

cmd/vet: printfunc false negative on struct without String method #17798

josharian opened this issue Nov 4, 2016 · 11 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.10

Comments

@josharian
Copy link
Contributor 

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 The path to resolution is known, but the work has not been done. label Nov 4, 2016
@josharian josharian added this to the Go1.9 milestone Nov 4, 2016
@valyala
Copy link
Contributor

valyala commented Nov 4, 2016

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

@valyala
Copy link
Contributor

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

valyala commented Nov 4, 2016

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

@josharian
Copy link
Contributor Author

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

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

Btw, this is intentional. See #16314.

@valyala
Copy link
Contributor

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

valyala commented Jun 2, 2017

cc'ing @ALTree , @martisch and @robpike

@martisch
Copy link
Contributor

martisch commented Jun 2, 2017 •
edited 

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

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

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

mvdan commented Jun 16, 2017

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

@gopherbot
Copy link

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
@mvdan @rsc
cmd/vet: unexported interface{} fields on %s are ok
f54f780 
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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
5 participants
@josharian @martisch @valyala @mvdan @gopherbot