Consider whitelisting cases where function has single error return case

[invidian]

With the following code:

https://github.com/flexkube/libflexkube/blob/9e0182c5c9e97e748e5945d122d686aebd4b4e6e/cli/flexkube/resource.go#L352-L374

I think the linter should allow to skip error wrapping, as the caller should do the wrapping. If error occurs, it is known that the error comes from this line. What do you think? Or maybe this should be somehow configurable?

[invidian]

The similar case would apply to anonymous functions like https://github.com/flexkube/libflexkube/blob/9e0182c5c9e97e748e5945d122d686aebd4b4e6e/pkg/helm/release/release.go#L187-L194, where again, there is single place from where the error can be returned.

[powerman]

I believe this is critical feature to make linter useful.

[tomarrell]

Apologies for the delayed response.

If you use this linter as part of golangci-lint@1.32.0 or greater, then you should have this functionality as part of that. I do agree having this as part of the linter would be good as well.

I'll have a look into adding this in the future so that people can use this linter standalone more effectively.

[powerman]

I'm using golangci-lint. How exactly I can use this functionality, is there any option to enable or something?

[tomarrell]

Yes, you should be able to specify the line you would like it to ignore the output of wrapcheck on. Some more documentation can be found here, https://golangci-lint.run/usage/false-positives/.

Let me know if that works out, or we need to look into another solution.

[powerman]

Ah, you're talking about //nolint. No, it's hardly an option, because there will be too many such lines. If it was only few of them in typical project I won't bother about this issue, but it's too many of them and as result I had to disable whole linter, which is bad. This should be either default behaviour or an option in golangci-lint config to enable it, otherwise it's too inconvenient to use this linter. :disappointed:

[tomarrell]

I see. Maybe you could help me out a little more in understanding, as the examples that are provided above seem to me like they should be wrapped.

The first one, calling into an external package ioutil, regardless if there's only a single error case in the function, should still be wrapped. This is due to the possibility of another function in the same package also calling ioutil.Readfile and you needing to discern between the two when debugging. Wrapping here would is still useful. Of course, I'm not saying that's concretely what you are doing here, but this needs to be taken into account for the linter to be useful.

The second example is also a call into a separate package, I would expect it to be wrapped at the call site of client.Run on line 191. If this is the case, then it is expected behaviour.

[invidian]

@tomarrell I think I understand your point of view and it indeed make sense when you think about the code on the package level. But if you trace function call by function call, in both my examples it is not ambiguous from where the error has been returned. Though I agree that wrapping the errors would make this code greppable.

Another argument for actually wrapping those errors would be to give opportunity to use Package-level errors, which can be then used with errors.Is etc.

To summarize, I think the linter behavior is actually correct, but having a better documentation (explanation), why it brings a value to wrap those errors, e.g. in README.md would be nice, to avoid other people getting confused.

Ah, and of course, thanks a lot for creating and maintaining this linter @tomarrell, it's awesome!

[tomarrell]

Hey glad that it was clarified. I will certainly work on the documentation more. I have a diagram a drew which may help the understanding as well. Probably useful in the README.

Quick question. Have you read the blog post where I went into a bit more depth? If you did, no worries, but wondering if any info in that makes it more clear in which case maybe I should bring it up to the README. https://blog.tomarrell.com/post/introducing_wrapcheck_linter_for_go

Cheers

[powerman]

@tomarrell I see your point, and the linter is actually correct accordingly to your spec, so you can just close this issue. BUT. There is another point, and maybe you can consider it too:

Receiving error from another package or an interface is a place where you probably should wrap errors, but function returning errors from more than one source (even if that source is another function in same package!) must wrap errors.

As you can see, the focus here is shifted from the case when some ambiguity may arise to the case when it will happens for sure.

As a concrete example consider very usual CRUD service which expose some API. It's very usual to setup some middleware which will append name of called API (HTTP endpoint, gRPC method name, etc.) to everything logged while processing this call. Also it's very usual for CRUD to have very simple business-logic implementation, so in terms of Clean/Hexagonal Architecture we'll have a chain of calls: API adapter → App business-logic → DB adapter, with only place where error might happens in DB adapter (for 95% of API calls, and the rest will have some function with multiple sources of errors at some of these layers), and all other layers (which are all in different packages!) doing nothing than if err != nil { return err }.

Using your current spec we'll have to wrap errors at each layer (because they're in different packages), but this won't add anything useful and just increase amount of boilerplate code and length of error message (by prepending useless prefixes like "GET /users: api: app: dal: sql: no rows in result set") because possible ambiguity of "sql: no rows in result set" error was already resolved by "GET /users:" prefix.

So, my point is: your current spec is much more stricter than some projects needs and this result in disabling the whole linter on these projects. If you'll provide two ways to use this linter - one which require wrapping only in case function receive errors from multiple sources, and second adding extra requirement to wrap everything from another package or interface - this will make this linter useful for more projects than now.

[tomarrell]

Closing as the linter supports ignoring packages.

[kulti]

@tomarrell, the original issue is about special case, when method has only one error return statement. Like this:

func foo() error {
  if err := bar(); err != nil {
    return err
  }

  doSmth()
  return nil
}

The ignoring package feature does not help with that, I guess.

[tomarrell]

@kulti I understand this case, however it may be that there are two functions which call the same external function, leading to a diamond call pattern. This would be useful to have them wrapped in order to identify the call path. It may also be the case that they have different arguments etc.

I think this is enough of a case for maintaining the rule as it is.