Suggest errors.New over fmt.Errorf with single string parameter and no formatting

[Jacalz]

I have seen a lot of code creating errors like fmt.Errorf("something went wrong") instead of errors.New("something went wrong"). Not only is the latter faster, but I also think that a lot of the former cases are wrong in subtle ways. If you use fmt.Errorf with a single string and no formatting verbs, you either forgot to add formatting (kind of related to #1528, I guess) or you should have used a regular error instead. As such, I think it be flagged as a problem and suggest replacing the call. In the case where you don't want to wrap or format anything, I also think it is a lot clearer to see a regular errors.New() as I can easily know that it is a static error string.

[ccoVeille]

I assumed revive or another golangci-lint linter would detect it but it doesn't.

While it does detect fmt.Sprintf("whatever") is detected by gosimple S1039

[dominikh]

My concern with this check would be consistency with surrounding code. Should fmt.Errorf("static string") be permitted if somewhere else in the same file we have fmt.Errorf("dynamic %d", n)? How about somewhere else in the same package? I could see myself using fmt.Errorf if I was already using it and it means I didn't have to import errors.

[ccoVeille]

please note govet detects

 fmt.Errorf("whatever", "whatever")

printf: fmt.Errorf call has arguments but no formatting directives (govet)

[ccoVeille]

My concern with this check would be consistency with surrounding code. Should fmt.Errorf("static string") be permitted if somewhere else in the same file we have fmt.Errorf("dynamic %d", n)? How about somewhere else in the same package? I could see myself using fmt.Errorf if I was already using it and it means I didn't have to import errors.

I would recommend using errors.New anyway. Go "imports" errors in fmt.Errorf so it would change nothing I think.

https://github.com/golang/go/blob/fa08befb25e6f4993021429aa222dad71a27ed07/src/fmt/errors.go#L8

Side remark, maybe some linter would recommend using a sentinel error at some point when using errors.New inside code.
Nothing does this for now anyway, as far as I know, but it could be interesting somehow

[arp242]

Not only is the latter faster

To put a number on that:

func BenchmarkNew(b *testing.B) {
    for n := 0; n < b.N; n++ {
        _ = errors.New("Well hello, this is a static string that doesn't change")
    }
}
func BenchmarkErrorf(b *testing.B) {
    for n := 0; n < b.N; n++ {
        _ = fmt.Errorf("Well hello, this is a static string that doesn't change")
    }
}

I get:

% go test -bench=.
goos: linux
goarch: amd64
pkg: tgo_20240529_FxuaSwr8
cpu: Intel(R) Core(TM) i5-8350U CPU @ 1.70GHz
BenchmarkNew-8            36486226          31.1 ns/op
BenchmarkErrorf-8          9721765         139.3 ns/op

That was more than I had expected, but ~100ns on a 2018 laptop is very little, especially since error paths are not often hot paths.

IMHO performance is not important here except in very special and rare cases.

---

With #1542 using a variable will get flagged; that can catch real errors – I have run into "accidental format string" with that, but never really with fixed strings.

This is in the same category as replacing fmt.Errorf("%d", n) with strconv.Itoa() (#1036): while there are real objective differences between the two, in a great many cases these differences don't matter, so practically speaking very often it becomes mostly a stylistic preference.

(The case for strconv is a bit stronger, because while most cases don't matter, it's more likely to lead to real measurable performance impact).

[ccoVeille]

So maybe something for perfsprintf linter ?

golangci/golangci-lint#3714
catenacyber/perfsprint#1

[dominikh]

BenchmarkNew-8 1000000000 0.2808 ns/op

The compiler is optimizing away your benchmark. errors.New gets inlined and the allocation gets optimized away. Try storing the result to a global variable instead of _.

[dominikh]

I would recommend using errors.New anyway. Go "imports" errors in fmt.Errorf so it would change nothing I think.

My concern isn't the import graph, but the number of direct imports (and thus API made available) in a file. That is, I posit that having only one way of creating errors produces less mental burden than having two.

I'm not convinced by performance arguments either way, because the error path isn't the happy path.

As for other linters: any linter that wants to suggest replacing calls to errors.New with exported error variables can do the same check for fmt.Errorf with a single argument.

[arp242]

Ah yeah, of course >_< It's ~31ns. Edited my message.

[Jacalz]

I'm not convinced by performance arguments either way, because the error path isn't the happy path.

Agreed. That was not the reason for my suggestion related to this linter check. My main motivator is from a readability standpoint. I personally find errors.New a bit clearer and faster to parse when reading code as I can move on after just seeing errors.N instead of reading the whole error string and then trying to understand any formatting rules etc.

[ccoVeille]

Apparently, perfsprintf already reports it

catenacyber/perfsprint#27 (comment)

I missed it, because I launched golangci-lint, but I missed that a local .golangci.yaml (very basic) file was present in the repository and hadn't perfsprintf enabled.

So, perfsprintf reports it already

[Jacalz]

Hmm. This seems to actually be triggered now with SA1006 as a call to fmt.Errorf() with a single string parameter results in a message like "printf-style function with dynamic format string and no further arguments should use print-style function instead" but that had me very confused initially. Perhaps the message should be adapted to be worded better in this case and we can close this issue afterwards?