proposal: cmd/vet: add declarative support for identifying functions for printf check

[fkollmann]

Goals

Add support for printf (and errorf) for function pointers and interfaces:

func Infof(string, any...)
func Errorf(string, any...)

func NewLogger() func(string, any...) {}
func SetLogger(func(string, any...))

type Logger interface {
  Infof(string, any...)
}

var infof func(string, any...)

Why change is needed

Currently the implementation does work by (a) having either functions mapping a specified list (which can be provided to vet via commandline argument) or by (b) calling the fmt.Printf or fmt.Print function.

a) Providing a specific function mapping does not work with modules that are not imported within the source file. Also there is no config file which could be use provide this list project-wide or inherit it from imported modules. For the full mapping list, see here.

b) Calling the fmt.Printf or fmt.Print function directly does not work for arguments, return values, and interfaces.

c) Also IDEs like VS Code and Goland add support for vet-based rules. Adding this feature to vet will likely also trigger changes in the IDE support.

For more details on the implementation, see here.

Solutions

There are different ways to solve this issue. I would like to brainstorm different ways, so please feel free to add proposals in the comments.

I) Named Arguments and Return Values

Use named arguments and return values by either using printfFormat or errorfFormat as argument name:

func Infof(printfFormat string, any...)
func Errorf(errorfFormat string, any...)

func NewLogger() func(printfFormat string, any...) {}
func SetLogger(func(printfFormat string, any...))

type Logger interface {
  Infof(printfFormat string, any...)
}

var infof func(printfFormat string, any...)

Pros: Easy to use, unlikely to cause side effects with existing code, clean upgrade path for existing modules, does not increase required Go version
Cons: Bloated code, argument names where non should be required

II) Add new fmt.FormatString and errors.FormatString

Add new fmt.FormatString and errors.FormatString types (which point to string) to declare the functions/methods as formatted:

func Infof(fmt.FormatString, any...)
func Errorf(errors.FormatString, any...)

func NewLogger() func(fmt.FormatString, any...) {}
func SetLogger(func(fmt.FormatString, any...))

type Logger interface {
  Infof(fmt.FormatString, any...)
}

var infof func(fmt.FormatString, any...)

Pros: Safe from side effects with existing code
Cons: Impact on reflection, requires build tag on upgrading code, requires addition to core packages, requires upgrade to higer Go version

III) Add declaration list to go.mod

Allow additional printf-like functions to be declared within the go.mod file and have those settings inherited from imported modules. Package names are resolved relative to the module root.

module hello/world

go 1.21

vet printf logging.Infof
vet errorf logging.Errorf

vet printf logging.InfofFunc

Examples:

package logging

func Infof(string, any...)
func Errorf(string, any...)

type InfofFunc func(string, any...)

func NewLogger() InfofFunc {}
func SetLogger(InfofFunc)

type Logger interface {
  Infof(string, any...)
}

var infof InfofFunc

Pros: no side effects on existing code
Cons: requires upgrade to higher Go version

IV) Add new go.vet config file

Allow additional printf-like functions to be declared within a new go.vet file and have those settings inherited from imported modules. Package names are resolved relative to the module root. (Similar to III but with less issues.)

The file should always be backward compatible and unknown and illegal entries should be ignored with a warning.

printf (
  logging.Infof
  logging.InfofFunc
)

errorf logging.Errorf

Pros: no side effects on existing code, clean upgrade path for existing modules, does not increase required Go version, could be used for additional go tool vet settings
Cons: new file added to Go universe

Please let me know what you think.

Best Regards, Felix

[fkollmann]

I just want to note that I am aware that for Go v2 other proposals are available, like #50554 . The focus of this proposal is Go v1.

[ianlancetaylor]

CC @golang/tools-team

[timothy-king]

This is closely related to #52445. (A duplicate in spirit if not the same collection of suggestions.)

[beoran]

Perhaps this issue and the linked one could both be solved by letting go vet recognize pragmas of the form

// govet: printf
// govet: stdmethod

To turn on the matching vet check for the function defined just after this pragma?

[adonovan]

There are already simple ways of marking a function as a printf wrapper when the analysis cannot infer this:

func f(format string args ...any) {
     if false {
        _ = fmt.Sprintf(format, args...) // cause vet to treat f as a printf wrapper
     }
     ...rest of function body...
}

and to do the reverse, to hide from vet the fact that a function should not be treated like a printf wrapper even though it does delegate to printf:

func f(format string, args ...any) {
    format += "" // hide from vet that this function is a printf wrapper
    ...rest of function body...
}

Since this proposal is about the specific case of printf, I think we should reject it. There are other analyzers for which the idea of a configuration file or annotation mechanism is more compelling, but this proposal is not about them.

[hyangah]

I agree that this printf check issue alone is not strong enough to justify introduction of new configuration mechanism.
I want to note that Alan's suggestion #58340 (comment) is clever, but it is hard to discover. Can we eventually provide a method or handy utility to do this? One may still argue that this pollutes code as much as annotations would do.

And It looks to me this workaround depends a lot on the current implementation details of the printf check. If this is the recommended official way, we need to make it future-proof (e.g. add testing in the printf check and ensure we don't accidentally break it while optimizing or tuning the check).

[adonovan]

hard to discover

The first of my two code examples above is documented:

$ go help vet
usage: go vet [-n] [-x] [-vettool prog] [build flags] [vet flags] [packages]

Vet runs the Go vet command on the packages named by the import paths.
...
For details of a specific checker such as 'printf', see 'go tool vet help printf'.

$ go tool vet help printf
...
A function that wants to avail itself of printf checking but is not
found by this analyzer's heuristics (for example, due to use of
dynamic calls) can insert a bogus call:

	if false {
		_ = fmt.Sprintf(format, args...) // enable printf checking
	}
...

There's also a web version of the same documentation here:
https://github.com/golang/tools/blob/master/gopls/doc/analyzers.md#printf

We could document the other trick here if that would help, but I think it's pretty rarely wanted.

You're right that these tricks do depend on the strength of the printf analysis, but the documentation is effectively a promise that this trick is supported. Also, making vet analyses flow-sensitive enough to ignore "if false" would cause them to skip conditionally compiled code such as if runtime.GOOS == "notyourlaptop" { ... }, and output would be much less predictable across OS and architecture variations.

[timothy-king]

I don't think it is currently clear what the goal of the proposal is (or what the problem is). I am guessing that all four suggestions are trying to do the equivalent of adding a new entry to x/tools/go/analysis/passes/printf.isPrint. Can you explain in more detail which functions you intend on being able to annotate, where you would like the annotations to live, and what the annotations are supposed to mean?

[adonovan]

The printf case is a weak justification for analysis annotations because it is already well served by simple and unobjectionable code changes that are (at least partly) documented by the analyzer. Other analyzers may provide more a compelling motive for a richer annotation or configuration mechanism. I suggest we either broaden the proposal to examine typical patterns across a range of analyzers, or reject the proposal.

[gopherbot]

Change https://go.dev/cl/476635 mentions this issue: go/analysis/passes/printf: document workarounds