x/tools/go/analysis/passes/printf: missing recursive call issue for type paramaterized String

[zpavlinovic]

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

$ go version
go version go1.20-pre3 cl/474093167 +a813be86df 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=""

GOARCH="amd64"

GOBIN=""

GOCACHE="/.../.cache/go-build"

GOENV="/.../.config/go/env"

GOEXE=""

GOEXPERIMENT=""

GOFLAGS=""

GOHOSTARCH="amd64"

GOHOSTOS="linux"

GOINSECURE=""

GOMODCACHE="/.../go/pkg/mod"

GONOPROXY="..."

GONOSUMDB="..."

GOOS="linux"

GOPATH="/.../go"

GOPRIVATE=".git.corp.google.com"

GOPROXY="https://proxy.golang.org,direct"

GOROOT="/usr/lib/...golang"

GOSUMDB="sum.golang.org"

GOTMPDIR=""

GOTOOLDIR="/usr/lib/...golang/pkg/tool/linux_amd64"

GOVCS=""

GOVERSION="go1.20-pre3 cl/474093167 +a813be86df"

GCCGO="gccgo"

GOAMD64="v1"

AR="ar"

CC="clang"

CXX="clang++"

CGO_ENABLED="1"

GOMOD="/.../play/go.mod"

GOWORK=""

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 -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build4025868520=/tmp/go-build -gno-record-gcc-switches"

What did you do?

go test

package main

import "fmt"

type A[T any] struct {
        t T
}

func (a A[T]) String() T {
        fmt.Println(a)
        return a.t
}

What did you expect to see?

main.go:10:2: fmt.Println arg a causes recursive call to (A[T]).String method

What did you see instead?

Everything ok.

I believe the issue is that the type of String method here is func() T and the analyzer expects func() string. We could flag the program if string satisfies the constraints of T. This could lead to false positives if A[T] is never instantiated with string, but that might happen very rarely.

Note that the analyzer currently correctly reports the issue when the return type is string.

func (a A[T]) String() string {
        fmt.Println(a)
        return ""
}

@timothy-king @findleyr

[findleyr]

This could lead to false positives if A[T] is never instantiated with string, but that might happen very rarely.

Elsewhere we consider explicit elements of the type set, if they exist. So we'd report cases where, for example, T is ~string. We could apply that pattern here: if the declaration indicates string as a specific type, we can treat that as a statement of intent.

Or we could flag any cases where string implements T: in this case the infinite recursion may be a latent bug, but it is still a bug.

I also think doing nothing is a reasonable decision: it seems extremely rare for there to be a String method that returns a type parameter.

[timothy-king]

I think we need to do something simple and clear here. I like the options of checking when T contains ~string or ignoring type params here.

I also think doing nothing is a reasonable decision: it seems extremely rare for there to be a String method that returns a type parameter.

I think this is also a different checker's concern. Probably stdmethods. Having the type be either an fmt.Stringer or not based on the type instantiation seems like a bug on its own. Such a type's interaction with fmt would be kinda wild.

[zpavlinovic]

This could lead to false positives if A[T] is never instantiated with string, but that might happen very rarely.

Elsewhere we consider explicit elements of the type set, if they exist. So we'd report cases where, for example, T is ~string. We could apply that pattern here: if the declaration indicates string as a specific type, we can treat that as a statement of intent.

I think that if we should do anything here, then this is the approach to take as the user intent is clearer and the solution should be simple. I also feel that we should have a different error message in this case. The message would indicate that there could be a recursive call when T is ~string.

I also think that we shouldn't do anything right now. We can add support for the above approach if needed.