cmd/vet: Formatting string check cannot see through type assertions or varargs

[mcy]

Go version

go version go1.23.0 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/mcyoung/Library/Caches/go-build'
GOENV='/Users/mcyoung/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/mcyoung/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/mcyoung/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.23.0/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='local'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.23.0/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.23.0'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/mcyoung/Library/Application Support/go/telemetry'
GCCGO='gccgo'
GOARM64='v8.0'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/Users/mcyoung/code/protocompile/go.mod'
GOWORK='/Users/mcyoung/code/protocompile/go.work'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/5l/2_ns6hbx1gj92vv9pkrl_jdc0000gn/T/go-build442821329=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

Consider the following program:

package a

import "fmt"

func MyPrintf(args ...any) {
  if len(args) == 0 {
    return
  }

  fmt.Printf(args[0].(string), args[1:]...)
}

var _ = MyPrintf("bad formatting %v")

I ran go vet on this program.

What did you see happen?

When running go vet, I expect to see a finding for an invalid formatting string in the call to MyPrintf. This is following the guidance in go vet's documentation to use a call to a fmt function to help it identify arguments that are formatting strings.

Unfortunately, it appears that vet cannot see through either the varargs access (with constant index) nor through the type assertion.

What did you expect to see?

No findings.

I expect a finding in this case, because taking an ...any to convert into formatting in this way is a popular mechanism for making functions appear to be overloaded between a formatting a non-formatting version. The popular assert package uses this pattern heavily: https://pkg.go.dev/github.com/stretchr/testify/assert, so it is probably worthwhile to make sure this actually gets linted correctly (at the very least, the case of having vet see through both type assertions and constant indexing).

[gabyhelp]

Related Issues

• cmd/vet: printf analyzer cannot analyze anonymous functions #44350
• cmd/vet: printf analyzer fails with generics #70572 (closed)
• go vet incorrectly recognizes format verb in string #38131 (closed)
• cmd/vet: false positives and wrong types across files, needs automatic recursive checking #20514 (closed)
• cmd/vet: (or maybe cmd/go or maybe go/types) doesn't handle tests in separate package well #6389 (closed)
• cmd/vet: go vet failing on directory with no non-test files #23395 (closed)
• x/tools/go/analysis/passes/printf: false positive breaks tests in package #69831 (closed)
• cmd/vet: false positive in Printf recursive Stringer check with non-matching String() functions #30441 (closed)

Related Code Changes

• cmd/vet: don't suggest ... if it breaks a program
• cmd/vet: improve detecting printf-like format argument

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[ianlancetaylor]

CC @golang/tools-team

[adonovan]

MyPrintf has a bug: it crashes if passed as non-string first argument. If you change the type of the first parameter to string, the static checker will do the right thing.

Even assuming the real MyPrintf (from which you distilled this example) is more robust, it is still very confusing to the human reader to encounter a function named for printf that doesn't have a format string parameter.

I don't think there's anything for the tools to do here.

[adonovan]

I expect a finding in this case, because taking an ...any to convert into formatting in this way is a popular mechanism for making functions appear to be overloaded between a formatting a non-formatting version.

This kind of overloading is a recipe for mistakes. (How popular is it really? I would certainly push back if I ever saw it in a code review.)

It is sufficient to expose just the printf variant and require the caller to say printf("%s", x) if they already have a string. Alternatively, expose a println variant as well.

[timothy-king]

The real code appears to be https://github.com/stretchr/testify/blob/master/assert/assertions.go#L286-L301 :

func messageFromMsgAndArgs(msgAndArgs ...interface{}) string {
	if len(msgAndArgs) == 0 || msgAndArgs == nil {
		return ""
	}
	if len(msgAndArgs) == 1 {
		msg := msgAndArgs[0]
		if msgAsStr, ok := msg.(string); ok {
			return msgAsStr
		}
		return fmt.Sprintf("%+v", msg)
	}
	if len(msgAndArgs) > 1 {
		return fmt.Sprintf(msgAndArgs[0].(string), msgAndArgs[1:]...)
	}
	return ""
}

This function seems to be buried kinda deep down under multiple layers. I agree with the skepticism that this overloading is a good idea compared to an explicit formatting string, but it is out there.

The repo https://github.com/stretchr/testify has 23.6k stars on github.

[mcy]

I don't think there's anything for the tools to do here.

Yes there is? This code already exists in the wild; I've also encountered it in logging frameworks. Simply saying that it's not something you would write is a little bit dismissive. Of course, I could simply write an analyzer myself, but I would prefer users actually got a warning for this.

MyPrintf has a bug: it crashes if passed as non-string first argument. If you change the type of the first parameter to string, the static checker will do the right thing.

It's not a bug, it's a documented part of the function that it panics if the first argument isn't a string. This perfectly sensible program, and connecting the argument to the use is straightforward dataflow analysis.

You can argue about whether or not this should be able to see through conditional selection of values, or through manifestly constant (i.e, it would be constant-evaluated in SSA), but that is not what I suggested in my original post.

[timothy-king]

I would write the contract for MyPrintf(args ...any) as roughly:

// MyPrintf prints args. If any args are present, then args[0] must be a fmt.Printf formatting string
// that applies to args[1:]. args[1:] is then printed formatted by arg[0].

That second sentence is a doozy. It is also TBH a foot gun as the "printf formatting string" is easy to silently miss and get through testing. It is what the new check is trying to address. (I would argue this is not actually a very good API personally.)

FWIW we applied this to our old code, mostly uncovered latent bugs, found a few examples like the MyPrintf one you gave, and all of them improved by updating them (at a very small verbosity price).

Going back to https://github.com/stretchr/testify , the first function I happened to look at was Containsf. I then looked at Contains. That calls Fail which calls messageFromMsgAndArgs. I wrote the following test:

func TestContains(t *testing.T) {
	Contains(t, "needle", "haystack", "first argument", "second argument")
}

I ran this with go1.23.1 and got the following output:

--- FAIL: TestContains (0.00s)
    assertion_order_test.go:9: 
                Error Trace:
                Error:          "needle" does not contain "haystack"
                Test:           TestContains
                Messages:       first argument%!(EXTRA string=second argument)

The interesting bit is the "%!(EXTRA string=second argument)" in the output. It is interpreting "first argument" as a format string and complaining there is a second. This is Contains not Containsf so I think this is unexpected and probably a bug. I would suggest trying to update messageFromMsgAndArgs .