polyfloyd / go-errorlint

A source code linter that can be used to find code that will cause problems with Go's error wrapping scheme
MIT License
249 stars 19 forks source link

Issues with multiple errors in fmt.Errorf() #36

Closed qfornaguera closed 1 year ago

qfornaguera commented 1 year ago

Hello!

We want to use this lint to watch out error wrapping correctness, but with infrastructure generated errors we usually parse them to a business error with attached details as following:

fmt.Errorf("%w: details=%s", common.ErrInfraOopsie, err)
fmt.Errorf("%w: details=%s", common.ErrInfraOopsie, err.Error())

This activates the lint regarding the second parameter... We have cases like this all over our codebase and we don't want to be forced to do in hundreds of places something like:

details := err.Error()
fmt.Errorf("%w: details=%s", common.ErrInfraOopsie, details)

Anyway, fmt.Errorf() allows %w verb only once so... I would like to ask that the lint activates only in the case there's an error as a parameter and no %w verb placed in the right position. Or at least give a setting flag to skip when there are multiple errors in fmt.Errorf() and the "secondary ones" use %s (I might understand that some people use to watch out multiple errors used in a single fmt.Errorf(). Pretty please 🙏

Thanks in advance for your attention 🙇

polyfloyd commented 1 year ago

Hi!

As of Go 1.20 which came out a few weeks ago, multiple %w wraps may be present in a single format string. This linter has been updated to support this behaviour.

You can opt to wrap everything of course, but that would leave you with unintentionally exposing errors of internal packages. This is what the caveat in the readme describes :)

If this caveat is not acceptable, I am open to implementing something to make this work for you. Your idea of having an additional flag to permit only one wrap sounds interesting, I will consider it.

I have also thought about having a kind of notation to allow errors be formatted as strings if it is explicit enough. %s with err.Error() is currently flagged as problematic, but we could re-assign this syntax to be valid. This would cover the caveat case while not needing a linter exception that could silence valid warnings. How does this sound to you?

qfornaguera commented 1 year ago

Thank you for your rapid response 😄

bruh! Screenshot 2023-02-22 at 12 34 48

I guess JetBrains Goland must update their inspections 😅 (I'm at latest version)

But you are right! double %w for go 1.20 works. Didn't know they added it. Great!

Regarding the review lint syntax to accept err.Error() for %s verbs, well... as fmt accepts also error for that verb we have simply err in the most of the cases 🥲. I want to reactivate the lint asap, so I'll try to do some wizard regex replace all over the code I guess 🧙

I'm glad you consider to add that flag to the lint configuration, I imagine other people might come upgrading from older codebase and older go versions, and that flag would give them less friction 👌

Let me thank you again for your attention and help 🙇 Have a great day! 😃

polyfloyd commented 1 year ago

Hi, I have just released version 1.2 which features the -errorf-multi flag. It defaults to true and permits Go 1.20's new behaviour. Set to false when still using Go 1.19 :)