x/tools/go/analysis/passes/printf: Errorf functions are treated as fmt.Errorf-wrappers

[chressie]

Background

The printf analyzer has a predefined list of functions and methods that are treated as functions that print something (as defined in https://pkg.go.dev/fmt, like fmt.Sprint or fmt.Sprintf). It is possible to make the analyzer aware of more such functions via a flag.

Problem statement

The analyzer treats all functions and methods named Errorf as wrappers of fmt.Errorf (code). That effectively means it accepts all usages of the %w verb in formatting strings to Errorf calls.

For example, a call like t.Errorf("%w", err) is not flagged by the analyzer. Neither are calls to logging functions that have an error level logging, like logrus.Errorf or glog.Errorf.

Proposal

First, only the fmt.Errorf function should be recognized by default as a function that accepts the %w verb. Other Errorf functions should be treated like regular formatting functions.

Second, it would probably be nice to have a way to make the printf analyzer aware of additional fmt.Errorf-wrapper functions. This could be done through an additional flag (e.g. errorffuncs), or a special syntax for passing function names to the existing funcs flag. Other ideas?

The two things are independent of each other. For the first step I already sent https://golang.org/cl/340409 (which actually led me into creating this issue).

[gopherbot]

Change https://golang.org/cl/340409 mentions this issue: go/analysis/passes/printf: fix %w for non-fmt.Errorf functions

[zpavlinovic]

...
The analyzer treats all functions and methods named Errorf as wrappers of fmt.Errorf (code). That effectively means it accepts all usages of the %w verb in formatting strings to Errorf calls.

For example, a call like t.Errorf("%w", err) is not flagged by the analyzer. Neither are calls to logging functions that have an error level logging, like logrus.Errorf or glog.Errorf.

This indeed seems like a problem. The issue is that fmt.Errorf produces an error and as such accepts %w. Other Errorf functions, such as testing.Errorf, really do print something and do not accept %w.

Proposal

First, only the fmt.Errorf function should be recognized by default as a function that accepts the %w verb. Other Errorf functions should be treated like regular formatting functions.

Agreed. The fix should make this distinction. The good thing is that the only supported "root" function producing errors is indeed fmt.Errorf.

Second, it would probably be nice to have a way to make the printf analyzer aware of additional fmt.Errorf-wrapper functions. This could be done through an additional flag (e.g. errorffuncs), or a special syntax for passing function names to the existing funcs flag. Other ideas?

I am assuming you mean a flag for specifying other fmt.Errorf wrappers that are not simple?

[timothy-king]

It is maybe plausible to assume an Errorf function whose result type is error does support the "%w" verb regardless of the package. This would distinguish between fmt.Errorf vs logrus.Errorf, glog.Errorf and testing.{T,B}.Errorf.

This heuristic might suppress warnings on "%w" verbs in Errorf funcs that can fail and return an error when they fail.

Second, it would probably be nice to have a way to make the printf analyzer aware of additional fmt.Errorf-wrapper functions. This could be done through an additional flag (e.g. errorffuncs), or a special syntax for passing function names to the existing funcs flag.

Do we have examples of functions we would want to specify with this flag?

[zpavlinovic]

It is maybe plausible to assume an Errorf function whose result type is error does support the "%w" verb regardless of the package. This would distinguish between fmt.Errorf vs logrus.Errorf, glog.Errorf and testing.{T,B}.Errorf.

Right. For an already detected formatting function or wrapper whose name is Errorf:

If return type is error: the function kind is Errorf
Else: the function kind is Printf

This heuristic might suppress warnings on "%w" verbs in Errorf funcs that can fail and return an error when they fail.

If my understanding is correct, then such functions will also return something else in case of non-failures. No formatting functions with such signatures are supported right now by default: they all either return a single value or no value at all.

Second, it would probably be nice to have a way to make the printf analyzer aware of additional fmt.Errorf-wrapper functions. This could be done through an additional flag (e.g. errorffuncs), or a special syntax for passing function names to the existing funcs flag.

Do we have examples of functions we would want to specify with this flag?

+1 But this also seems like a separate issue.

[gopherbot]

Change https://golang.org/cl/342109 mentions this issue: go/analysis/passes/printf: don't classify testing.T.Errorf as supporting %w

[neild]

Second, it would probably be nice to have a way to make the printf analyzer aware of additional fmt.Errorf-wrapper functions.

The printf analyzer is already capable of identifying functions which wrap formatting functions like fmt.Printf or fmt.Errorf. This should generally just work without any user intervention.

[chressie]

Do we have examples of functions we would want to specify with this flag?

I'm not aware of any yet.

The printf analyzer is already capable of identifying functions which wrap formatting functions like fmt.Printf or fmt.Errorf. This should generally just work without any user intervention.

Yes, but, if I understand correctly, identifying wrapper functinos only works for functions that are implemented in the same package. So, if you have a generic fmt.Errorf wrapper, this check will not detect it.

It is maybe plausible to assume an Errorf function whose result type is error does support the "%w" verb regardless of the package. This would distinguish between fmt.Errorf vs logrus.Errorf, glog.Errorf and testing.{T,B}.Errorf.

Right. For an already detected formatting function or wrapper whose name is Errorf:

If return type is error: the function kind is Errorf
Else: the function kind is Printf

This heuristic might suppress warnings on "%w" verbs in Errorf funcs that can fail and return an error when they fail.

If my understanding is correct, then such functions will also return something else in case of non-failures. No formatting functions with such signatures are supported right now by default: they all either return a single value or no value at all.

I had a similar logic in mind, but forgot to mention it in the issue description :)

Another edge case is when a method doesn't return an error, but instead updates an error field in a struct. It might be reasonable to accept a %w verb here, but it would be flagged as invalid usage. However, I believe such cases are not the norm, so maybe we won't need to bother.

I guess we have two CLs doing the ~same thing (tests are different): https://golang.org/cl/342109 and https://golang.org/cl/340409. I'm fine with submitting either of these :)

[neild]

Yes, but, if I understand correctly, identifying wrapper functinos only works for functions that are implemented in the same package. So, if you have a generic fmt.Errorf wrapper, this check will not detect it.

This should work cross-package.

Empirically, go vet on the following program correctly detects the error in the logrus.Debugf call in the following, even though the Debugf call is defined in a different module:

package main

import "github.com/sirupsen/logrus"

func main() {
  var err error
  logrus.Debugf("%w", err)
}

$ go vet m.go
./m.go:7:2: Debugf call has error-wrapping directive %w, which is only supported by Errorf

[neild]

Fixed by https://golang.org/cl/340409.

[benbuzbee]

Doesn't this break specifying xerrors.Errorf as a printf function?
https://pkg.go.dev/golang.org/x/xerrors#Errorf

[ianlancetaylor]

@benbuzbee Yes, that seems likely. Do we care, though? Is there any reason to use xerrors.Errorf rather than fmt.Errorf? Perhaps this can encourage people to migrate to the standard library version.

[benbuzbee]

My team uses it because it captures the stack frame where created which I find super valuable. I would love for standard libraries to support that but :)

In the meantime, it's not the end of the world that we can't use go vet to...vet...our errors but it is a bit too bad

If I can tell you more about why I like stack frames though...

[ianlancetaylor]

@benbuzbee Fair enough, I suggest opening a new issue to add xerrors.Errorf back to this vet test.