fkollmann commented 1 year ago

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 commented 1 year ago

I just want to note that I am aware that for Go v2 other proposals are available, like https://github.com/golang/go/issues/50554 . The focus of this proposal is Go v1.

ianlancetaylor commented 1 year ago

CC @golang/tools-team

timothy-king commented 1 year ago

This is closely related to https://github.com/golang/go/issues/52445. (A duplicate in spirit if not the same collection of suggestions.)

beoran commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/golang/go/issues/58340#issuecomment-1419144620 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

adonovan commented 1 year ago

Re-reading this thread I realized my first comment was addressing only a narrow part of the original proposal, which is more broadly about how to indicate in the type of a function that it is a printf wrapper, so that dynamic calls to func values and interface methods are checked. Apologies for not reading more carefully.

Let's reconsider the four ideas in the first note:

Named arguments and return values (e.g. func(printfFormat string, any...))
Add new fmt.FormatString and errors.FormatString (e.g. func(fmt.FormatString, any...))
Add declaration list to go.mod
Add new go.vet config file

In reverse order, my reactions are:

that (4), a vet config file, is the wrong place to write this information. It's a property of a particular declaration in the source, so the annotation (whatever form it takes) belongs in the source, where it can be easily found, and can evolve with the code. Generally I don't think any of the analysis flags for indicating sets of special functions have really enjoyed much success, and they represent an approach that predates the idea of modular analysis using facts to record lemmas (or types) discovered from analyzing previous units.
that (3), the go.mod file, is also the wrong place for this information, since it's not really related to modules.
that (2), a format string type, is better, but we don't really want to have to change the type of a function to enable vet checking, as this would break existing code, and even for new code it's inconvenient and surprising. However, one could define fmt.FormatString as a type alias for string with added intent. That would of course require a change to the standard library. A type alias for string isn't distinguishable from string in the type-checker's representation, so the information would not be available to other packages through the type system, but I think it would be possible for the printf checker to export an analysis fact about the parameter variable that would then be accessible from other packages. Still, users would probably prefer to read string in the source than a type alias.
that (1), a format parameter name, is even better, as it doesn't require changes to the type, or to the standard library, but only to the analyzer. Also, since it is used only on abstract functions, it doesn't have any inconvenient implications like forcing the use of a longer parameter name within the function body, as there is no body.

For completeness, let's not forget the implicit zero approach, which is some comment-based syntax as @beoran (and many others in the past) have suggested. We may eventually need a more powerful and general notation to express analysis facts in the source code, but I'd rather not design it until we have seen a handful of analyzers that need it.

I think approach 1 is worth prototyping in the printf checker. I put together a quick sketch in https://go.dev/cl/479175.

gopherbot commented 1 year ago

Change https://go.dev/cl/479175 mentions this issue: go/analysis/passes/printf: sketch of dynamic func/interface annotation

timothy-king commented 1 year ago

IIUC the annotations that are being requested are 2 slightly different cases:

This function in my package should belong to x/tools/go/analysis/passes/printf.isPrint.
This function is another package I imported should belong to x/tools/go/analysis/passes/printf.isPrint.

@adonovan We should be able to solve both with approach (1) in https://github.com/golang/go/issues/58340#issuecomment-1482797981.

We can annotate functions using a special argument name like this:

var _ func(printfFormat string, any...) = MyPrintf                        // same package function
var _ func(printfFormat string, any...) = MyLogger.MyPrintf      // same package method
var _ func(printfFormat string, any...) = logger.Printf                 // imported function
var _ func(printfFormat string, any...) = (logger.Logger).Printf // imported method

This is reusing the parameter name from the func type to pick up the annotation. I am not sure if the sketch https://go.dev/cl/479175 supports it.

This annotation only needs to be done once per package. We can also forward added isPrint annotations for reverse dependencies via Facts. So the # of annotations could be as low as the # of leaf packages in a module. Scaling arguments are all theoretical though as the number of printf wrappers one needs to label in order to do proper printf inference is likely very small. Given that this is not a bigger pain point, my hunch is the number of annotations in the wild would be quite small. So I think (1) would be a reasonable solution solution for printf.

A limitation I can see from a user's perspective is that this does not allow for the following: MyPackage imports OtherPackage which imports YetAnotherPackage. We cannot influence isPrint inference during OtherPackage by adding a label for YetAnotherPackage.Printf in MyPackage, i.e. "injecting" annotations for analysis. A config file can do this by being a root dependency. IMO I think it is okay for MyPackage to just do more labeling for all of the inferred isPrint functions it is potentially missing from OtherPackage. So IMO this is not a compelling case for config files yet.

There is a minor implementation issue that an added isPrint annotation from another package should be a package Fact instead of an object Fact.

adonovan commented 1 year ago

On the question of "injecting" annotations into a lower package, the answer has to be a clear no: the framework allows information to flow only upwards, from p's imports to p itself. I'm not sure it's practical even to support the restricted version of injection in which, given A->B->C, package B expresses a retroactive annotation on a declaration in C so that A benefits, because, as you say, object facts can only be exported by the declaring package. If one of your dependencies forgot to add an annotation, your best choice remains to send them a PR.

On the question of the notation of annotations, one approach we should consider is for each analyzer to provide a subpackage of declarations with no run-time effect--just types, constants, and empty function bodies--which the client code imports and uses to express annotations. For example:

import printfannot "golang.org/x/tools/go/analysis/passes/printf/annot"

// (FormatString is defined as a type alias for string, with intentionality.)
func MyPrintf(format printfannot.FormatString, args ...any) { ... }

// or perhaps:

var _() {
    // The existence of this call tells the printf checker to export a fact about MyPrintf.
    printfannot.Check(MyPrintf)
    ...
}

Using Go syntax for annotations has a number of benefits: unlike comments, they stay up to date as the code evolves, they can be navigated like ordinary references, and documentation is instantly available.

timothy-king commented 1 year ago

I'm not sure it's practical even to support the restricted version of injection in which, given A->B->C, package B expresses a retroactive annotation on a declaration in C so that A benefits, because, as you say, object facts can only be exported by the declaring package.

You can do this with package facts where {C.Printf} is added as a package fact on B. The major downside is that A's package facts are expected to be a superset of B's package facts (for A's reverse dependencies). This might all be too much for too little benefit for this checker.

var _() { // The existence of this call tells the printf checker to export a fact about MyPrintf. printfannot.Check(MyPrintf) ... }

I've been thinking about this kind of annotations for a while. I definitely like these as a high level idea. It feels like we just need more use cases before pursuing this though (guarded by checking would be a good candidate). But this is kinda getting off topic.

For the printf checker though, maybe it is good enough to using the name of the first function argument? That was my understanding of (1) https://github.com/golang/go/issues/58340#issuecomment-1482797981. The name might be a magic constant, but we do accept magic constants for things like //go: directives/comments.

adonovan commented 1 year ago

You can do this with package facts where {C.Printf} is added as a package fact on B. The major downside is that A's package facts are expected to be a superset of B's package facts (for A's reverse dependencies). This might all be too much for too little benefit for this checker.

I agree. At the risk of stating the obvious, package facts are for facts about packages, and objects facts are for objects. (The superset property is not essential, BTW. "Package P has package doc comment C" would be a totally valid package fact.)

I definitely like these as a high level idea. It feels like we just need more use cases before pursuing this though (guarded by checking would be a good candidate).

Glad we're on the same page wrt Go syntax for annotations; and, yes, there are many details still to work out.

For the printf checker though, maybe it is good enough to using the name of the first function argument?

I'd be happy with that approach (specifically: bullet point 1, sketched in CL 479175, not the var _ func... = MyPrintf approach in the later note). It seems unobtrusive enough that if we later add a more general notion of annotations we won't regret this step.

gopherbot commented 1 year ago

Change https://go.dev/cl/489835 mentions this issue: go/analysis/passes/unusedresult: support annotations

golang / go

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

Goals

Why change is needed

Solutions

I) Named Arguments and Return Values

II) Add new `fmt.FormatString` and `errors.FormatString`

III) Add declaration list to `go.mod`

IV) Add new `go.vet` config file

golang / go

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

Goals

Why change is needed

Solutions

I) Named Arguments and Return Values

II) Add new fmt.FormatString and errors.FormatString

III) Add declaration list to go.mod

IV) Add new go.vet config file

II) Add new `fmt.FormatString` and `errors.FormatString`

III) Add declaration list to `go.mod`

IV) Add new `go.vet` config file