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.

Sign up for GitHub

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

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

Open
zpavlinovic opened this issue Sep 28, 2022 · 3 comments
Open

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

zpavlinovic opened this issue Sep 28, 2022 · 3 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@zpavlinovic
Copy link
Contributor

zpavlinovic commented Sep 28, 2022 •
edited

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
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 28, 2022
@gopherbot gopherbot added this to the Unreleased milestone Sep 28, 2022
@zpavlinovic zpavlinovic added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 28, 2022
@zpavlinovic zpavlinovic changed the title x/tools/go/analysis/passes/printf: missing recursive call for type paramaterized String x/tools/go/analysis/passes/printf: missing recursive call issue for type paramaterized String Sep 28, 2022
@findleyr
Copy link
Contributor

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

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

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.

@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@timothy-king @zpavlinovic @adonovan @gopherbot @findleyr