x/tools/passes/printf: conditionally printf-like functions are treated as printf-like, causing false positives

[nieomylnieja]

Go version

go1.24.1

Output of go env in your module/workspace:

AR='ar'
CC='gcc'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='g++'
GCCGO='gccgo'
GO111MODULE=''
GOAMD64='v1'
GOARCH='amd64'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/home/mh/.cache/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/home/mh/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build661215316=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='/home/mh/nobl9/govy/go.mod'
GOMODCACHE='/home/mh/go/pkg/mod'
GONOPROXY='github.com/nobl9/*'
GONOSUMDB='github.com/nobl9/*'
GOOS='linux'
GOPATH='/home/mh/go'
GOPRIVATE='github.com/nobl9/*'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/nix/store/g29rrn8qqlg4yjqv543ryrkimr7fk43h-go-1.24.1/share/go'
GOSUMDB='sum.golang.org'
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/mh/.config/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/nix/store/g29rrn8qqlg4yjqv543ryrkimr7fk43h-go-1.24.1/share/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.24.1'
GOWORK='/home/mh/nobl9/govy/go.work'
PKG_CONFIG='pkg-config'

What did you do?

I've bumped Go version in my project from 1.22 to 1.24 and go vet started failing.

The internal implementation of WithDetails method looks like this:

func (r Rule[T]) WithDetails(format string, a ...any) Rule[T] {
	if len(a) == 0 {
		r.details = format
	} else {
		r.details = fmt.Sprintf(format, a...)
	}
	return r
}

The failing test case looks like this:

func TestRule_WithMessage(t *testing.T) {
	tests := []struct {
		Error         string
		Message       string
		Details       string
		ExpectedError string
	}{
		{
			Error:         "this is error",
			Message:       "",
			Details:       "details",
			ExpectedError: "this is error; details",
		},
                // ...
	}

	for _, test := range tests {
		r := govy.NewRule(func(v int) error {
			if v < 0 {
				return errors.New(test.Error)
			}
			return nil
		}).
			WithErrorCode("test").
			WithMessage(test.Message).
			WithDetails(test.Details)

		// ...
	}
}

What helped? Changing the test and adding the args:

func TestRule_WithMessage(t *testing.T) {
	tests := []struct {
		Error         string
		Message       string
		Details       string
		DetailsArgs   []any
		ExpectedError string
	}{
		// ...
	}

	for _, test := range tests {
		r := govy.NewRule(func(v int) error {
			if v < 0 {
				return errors.New(test.Error)
			}
			return nil
		}).
			WithErrorCode("test").
			WithMessage(test.Message, test.DetailsArgs...).
			WithDetails(test.Details, test.DetailsArgs...)

		// ...
	}
}

While I can live with that fix, It seems to me like there's something potentially wrong with how go vet evaluates this rule. I tried reproducing that with a simpler code base, but couldn't reproduce it.

PR link: nobl9/govy#104
Affected changes (before the fix): https://github.com/nobl9/govy/pull/104/files/1d63aadbad94f2aae90129e061f53c211e842361

What did you see happen?

pkg/govy/rule_test.go:78:16: non-constant format string in call to (github.com/nobl9/govy/pkg/govy.Rule[T]).WithDetails
pkg/govy/rule_test.go:77:16: non-constant format string in call to (github.com/nobl9/govy/pkg/govy.Rule[T]).WithMessage
pkg/govy/rule_test.go:117:16: non-constant format string in call to (github.com/nobl9/govy/pkg/govy.Rule[T]).WithDetails

What did you expect to see?

No errors reported by go vet as these test scenarios and the usage of reported methods WithMessage and WithDetails are valid.

[nieomylnieja]

I tried reproducing it with the following code (without success):

package main

import (
	"fmt"
	"testing"
)

func NewRule[T any]() Rule[T] {
	return Rule[T]{}
}

type Rule[T any] struct {
	details string
}

func (r Rule[T]) WithDetails(format string, a ...any) Rule[T] {
	if len(a) == 0 {
		r.details = format
	} else {
		r.details = fmt.Sprintf(format, a...)
	}
	return r
}

func TestRule_WithDetails(t *testing.T) {
	tests := []struct {
		Details    string
		OtherStuff string
	}{
		{
			OtherStuff: "this",
			Details:    "details",
		},
		{
			Details: "",
		},
		{
			OtherStuff: "that",
			Details:    "details",
		},
	}

	for _, test := range tests {
		rule := NewRule[string]().
			WithDetails(test.Details)

		if rule.details == "foo" {
			return
		}
	}
}

[adonovan]

What's happening here is that the presence of the fmt.Sprintf(format, args...) call in WithDetails tells the static checker that, by induction, WithDetails is itself a "printf-like" function. This causes the call WithDetails(test.Details) to be checked as if it were a call to printf, resulting in the "non-constant format string" diagnostic.

func (r Rule[T]) WithDetails(format string, a ...any) Rule[T] {
	if len(a) == 0 {
		r.details = format
	} else {
		r.details = fmt.Sprintf(format, a...) // this call causes the analysis to inductively deduce that WithDetails is printf-like
	}
	return r
}

Of course, your intent is that WithDetails is not actually printf-like, or at least not always: it is conditionally printf-like depending on the presence of arguments. However it is beyond the ability of the static checker to make that kind of deduction.

As a matter of style, I would strongly suggest avoiding functions that are conditionally printf-like; it is as confusing to humans as it is to tools. (I routinely make this suggestion when reviewing Google's Go code.) Instead, rename it to WithDetailsf and remove the len(a)==0 part. Or, provide two variants, like the log package does.

[nieomylnieja]

I assumed I'm missing something here, and indeed I was 🤦
Thank you @adonovan for the super clean and precise explanation!

And yes, I think I will follow your advice and adjust the API of the package accordingly. While I personally prefer the ease of use of the single printf-like function I'd much rather sacrifice that in order to be in line with the Go standards.

Closing as resolved.