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 for user-defined verbs #36564

Open
kortschak opened this issue Jan 15, 2020 · 10 comments
Open

cmd/vet: false positive for user-defined verbs #36564

kortschak opened this issue Jan 15, 2020 · 10 comments

Comments

@kortschak
Copy link
Contributor

@kortschak kortschak commented Jan 15, 2020

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

$ go version
go version go1.13.6 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN="/home/user/bin"
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/user"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/user/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/user/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/user/src/github.com/kortschak/loopy/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build878536771=/tmp/go-build -gno-record-gcc-switches"

What did you do?

~/src/github.com/kortschak/loopy/cmd/bundle [master*]$ go test
# github.com/kortschak/loopy/cmd/bundle
./bundle.go:64:3: Fprintf format %60a has unknown verb a
~/src/github.com/kortschak/loopy/cmd/bundle [master*]$ echo $?
2

What did you expect to see?

No complaint.

What did you see instead?

Vet failure causing test failure (well, it would be a test failure if there were a test).

Additional information

The %a verb is non-standard verb, an addition to the biological sequence types provided by bíogo through their fmt.Formatter satisfaction. However, they have been in existence since 2013 and when they were included (as is still true), there was no documentation stating that only verbs listed in the fmt package for the built-in types should be used.

@robpike
Copy link
Contributor

@robpike robpike commented Jan 15, 2020

If the type being printed implements fmt.Format, vet is supposed to allow any verb. I distinctly remember writing that code and I see its implementation in the source (although much has changed since then), but it depends on type checking working.

Or maybe it never worked, but it should.

@bcmills bcmills added this to the Backlog milestone Jan 22, 2020
@bcmills bcmills added the help wanted label Jan 22, 2020
@tiriplicamihai
Copy link

@tiriplicamihai tiriplicamihai commented Feb 2, 2020

Can I help with this?

@josharian
Copy link
Contributor

@josharian josharian commented Feb 2, 2020

@tiriplicamihai go for it

@tiriplicamihai
Copy link

@tiriplicamihai tiriplicamihai commented Feb 2, 2020

I will try to investigate tomorrow evening (EET here).

@tiriplicamihai
Copy link

@tiriplicamihai tiriplicamihai commented Feb 2, 2020

Hey @kortschak vet seems to properly work. I wrote this dummy example and the command runs without any issues:

package main

import (
    "fmt"
    "os"
)

type A struct{}

func (_ *A)  Format(f fmt.State, c rune) {}

func main() {
     out, _ := os.Create("cucu")
     defer out.Close()
     a := &A{}
     fmt.Fprintf(out, "%a", a)
}

I looked a bit over your code and it seems the dependency does not actually implement the Formatter interface: https://github.com/biogo/biogo/blob/master/seq/seq.go (at least not in the version I found on GH).

My 2c here is that you either change '%a' to an allowed verb or submit a request to have sequnece implement the Formatter interface.

@josharian I think this issue can be closed.

@josharian
Copy link
Contributor

@josharian josharian commented Feb 2, 2020

I’ll let @kortschak confirm and close if appropriate.

@kortschak
Copy link
Contributor Author

@kortschak kortschak commented Feb 2, 2020

It's not correct that the type doesn't implement fmt.Formatter. The interface doesn't, but the concrete type does.

@kortschak
Copy link
Contributor Author

@kortschak kortschak commented Feb 2, 2020

It does however explain what the problem is. Clearly vet is not considering whether the arg to fmt.Printf is an interface type when it's looking at the method set. Probably it should not try to look at interface types since the concrete value held in the interface may satisfy the fmt.Formatter interface.

You can see this kind of behaviour in vet for the common verbs here

package main

import "fmt"

func main() {
	var i interface{} = 1
	fmt.Printf("%s", i)
}

This does not cause a vet warning, but the equivalent without the interface {} type decl does. The vet check on fmt.Formatter should follow this behaviour.

In src/cmd/vendor/golang.org/x/tools/go/analysis/passes/printf/printf.go, isFormatter should optimistically return true if the type is an interface, something along the lines of (although named types may need to be unwrapped)...

func isFormatter(typ types.Type) bool {
        if _, ok := typ.Underlying().(*types.Interface); ok {
                return true
        }
        obj, _, _ := types.LookupFieldOrMethod(typ, false, nil, "Format")
        fn, ok := obj.(*types.Func)
        if !ok {
                return false
        }
        sig := fn.Type().(*types.Signature)
        return sig.Params().Len() == 2 &&
                sig.Results().Len() == 0 &&
                isNamed(sig.Params().At(0).Type(), "fmt", "State") &&
                types.Identical(sig.Params().At(1).Type(), types.Typ[types.Rune])
}
@kortschak
Copy link
Contributor Author

@kortschak kortschak commented Feb 2, 2020

If that's acceptable, I can send a CL. Presumably this goes to x/tools and will then get vendored in to the project source from there.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 3, 2020

Change https://golang.org/cl/217180 mentions this issue: go/analysis/passes/printf: give leeway for fmt.Formatter satisfaction

gopherbot pushed a commit to golang/tools that referenced this issue Mar 11, 2020
We have no way of knowing the concrete type of an interface value;
it might be a fmt.Formatter. To avoid false positives, assume that
all interface values are fmt.Formatters.

Updates golang/go#36564

Change-Id: Iaf18ba2794e4d3095d0018502c1c6c459a360b42
Reviewed-on: https://go-review.googlesource.com/c/tools/+/217180
Reviewed-by: Rob Pike <r@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants