cmd/vet: doesn't handle formatting strings in variables

[raelyz]

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

$ go version
go version go1.15.6

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
C:\Projects\Go\src\testing>go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Ray\AppData\Local\go-build
set GOENV=C:\Users\Ray\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Projects\GoLib\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Projects\GoLib;C:\Projects\Go;
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=c:\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\Ray\AppData\Local\Temp\go-build204160270=/tmp/go-build -gno-record-gcc-switches

What did you do?

type invalidSide struct {
	erroneousSide float64
	errorMessage  string
}

func (invalidSide *invalidSide) Error() string {
	format := "%v given for one of the sides. values must be more than 0."
	return fmt.Sprintf(format, invalidSide)
	return fmt.Sprintf("%v given for one of the sides. values must be more than 0.", invalidSide)
}

Here is a sample of the code that resulted in a small issue I noticed.

What did you expect to see?

The first return statement should be flagged out by Go-Vet as it results in recursive Error calls.

What did you see instead?

The file saved without issue and crashes during run time. The 2nd return statement is able to be detected by Go-Vet perfectly fine.

[seankhliao]

observation: it works if the format string is constant:

func (invalidSide *invalidSide) Error() string {
	const format = "%v given for one of the sides. values must be more than 0."
	return fmt.Sprintf(format, invalidSide)
}

[timothy-king]

It is possible to support this exact case in vet. A string constant flowing through a non-package scope string variable that is set exactly once into the format position of an fmt.*printf function. Handling more than this (handling control flow, being set conditionally, flow through globals, etc.) would require a fairly different checker.

@raelyz how did you end up encountering this? Is it possible to see the original? Also is the above example minimized? I am a bit worried that if we extend vet to handle this, it would not actually have helped you.

[raelyz]

Hi Timothy, I'm currently learning Go in a course and the instructor demonstrated error handling with that particular example except for the fact she used invalidSide.errorMessage as the second argument to Sprintf.

A classmate of mine used invalidSide instead with the entire string as the first argument and vet prompted her to fix the issue due to the recursive calls on Error(). She changed it to a variable as demonstrated in our lecture slides and the error disappeared which was rather puzzling. This lead me to dig deeper to this as I couldn't find much documentation on the best practices per say in utilizing Sprintf.

I understand vet utilizes heuristics to check and as a result it may not be comprehensive. Mainly raising the issue for consideration in the event others may have encountered similar instances but since no errors were flagged by vet they couldn't pinpoint the issue. I'm still fairly new to programming in general so I would leave my inputs as that and maybe ya'll could make the call on this issue. Thanks!

[timothy-king]

Thank you for the additional context. It sounds like this might be frequent enough for vet to raise an alert.

Some potential extensions that could have done something:

1. Suggest replacing the variable with const. (@seankhliao 's observation )
2. Suggest replacing the variable by inlining the constant.
3. Improve the printf checker to allow the format argument to also be an identifiers when it can only be a single constant value. Should be able to figure this out from the TypeInfo

[raelyz]

Totally agree, 1 and 2 are current best practices per say to prevent such errors from happening in run time.

Just a possible scenario but in the event of 3, hypothetically if someone is trying to print dynamically with different verbs in various cases, this may potentially break their code. The tradeoff here would then be the relative occurrence of such a practice vs the safety or potential confusion to people new to go should they encounter something similar to what I've seen.

Would a plausible hotfix in the short run be to highlight it within the printf package to either 1 or 2 when utilizing printf?

[timothy-king]

Just a possible scenario but in the event of 3, hypothetically if someone is trying to print dynamically with different verbs in various cases, this may potentially break their code.

My understanding of 3 is that it would not flag this case as it only would look for values that syntactically can only be a single constant value. This is effectively just variables that are assigned to a constant when declared. (This could be done more thoroughly with a constant propagation domain, but this would likely be significantly slower for unclear additional benefit.)

Would a plausible hotfix in the short run be to highlight it within the printf package to either 1 or 2 when utilizing printf?

There is no obvious "bug" preventing someone using go, getting an incorrect answer, or a security problem. This is a feature request for improved capabilities for vet. I do not see a "hotfix" as necessary.

[adonovan]

I think this issue is waiting for evidence that the problem is frequent enough to justify the extra checker effort to notice "effectively constant" format strings.