Use go error wrapping instead of go-errors in apps

[dpordomingo]

go 1.13 has been released, and our guide says that

our applications support only latest stable Go version

go 1.13 offers error wrapping (errors.Is, errors.Wrap...)

For error handling, our guide request to use go-errors, which also supports error wrapping.

Then it was raised the following question on our [private link] Slack

should we stop using src-d/go-errors for applications in favor of the new go 1.13 errors.Is, errors.Wrap?

[kuba--]

My understanding is - because we want to be backward compatible with the latest two versions of go, we have to wait till 1.14

[kuba--]

...but to prepare for that I would archive go-errors project.

[dpordomingo]

I edited the issue desc to clarify that it's about our applications, which must support only last stable 1.13

And I assume that the consensus could (should) be applied to libraries once 1.14 has been released.

[kuba--]

Yes, I heard about that, but frankly still don't get it. What's the difference between library and application. In theory almost every package should behave like library. Application is just a main wrapper on top of API.

The pilosa would be a good example. We started using it as an external service, but in next iteration we extracted API and got rid of the server part and continue use it as a library.
If they switched to the new go's feature (saying that pilosa is an app) we would have to do the same in go-mysql-server which is a library.

[dennwc]

@kuba-- I think the main difference is that we don't promise any internal API compatibility for libraries that are part of our app projects. So you could use it as a library, but it may break over time if you don't lock the version.

The cause of this difference was mentioned by @creachadair on multiple occasions: SemVer is good, but it's not enough. For example, v1.0 means two different things for application and a library.

For the library, v1.0 means that all APIs will work the same way in any future v1.x release (ideally).

And for the application, v1.0 means that the application will have the same CLI, on-disk format, external APIs. No promises are given about the structure of internal libraries and APIs.

To your point, the solution would be to always write an app as a library in a separate project, have it versioned (thus provide a compatibility promise) and then have a separate project with a server/CLI that calls into the library, and version it separately. But this again forces us to think about libs being different from apps.

[dennwc]

To the topic of the issue, I agree that we should start using the new functionality provided by stdlib errors package in apps at least.

Also, before archiving src-d/go-errors we should merge src-d/go-errors#15 and update all apps/libraries to use the new version. This will at least make src-d/go-errors compatible with a new stdlib interface.

I think adding it to apps first also makes sense because we can try the new API and see if it fully supersedes the src-d/errors. We can make a better decision after actually trying the new functionality.

[kuba--]

In my opinion we should paraphrase it - "for main and non exportable packages".
Because IMO there is no obvious difference between app and lib.

For instance if you have a main/internal/a/b/c then you can use the latest go, but if you have a/b/c/internal/d/e/f and even if you use the latest go under internal, I can still import a/b/c which uses internal stuff and it will break my compilation.
So, we need precise and consistent description, but IMO it's not really related to apps and libs.

[dennwc]

@kuba-- I totally agree, it's a good definition and it may solve the concern I raised when mentioning the project split to main and lib repos.

But there are still problems with this approach, if managed in a single repo. You'll need to version the internal library, but it's nearly impossible to do correctly, because the versions in the repo correspond to app versions, while library users will use them as lib versions. Which is, of course, incorrect since app versions assume a different type of API compatibility in mind. This is why I brought up versioning.

[kuba--]

I think versioning is something orthogonal to this. Here we are talking about the new release (or master development).
But if we talk generally about dependencies then you can always stick to some old version in your deps file and use go1.4

[jfontan]

I would wait to take this decision until we are used to the new syntax. I'm not really happy with wrapping an error that you want to catch. Example:

if err != nil {
  return fmt.Errorf("object not found %w", err)
}

If I want to catch "object not found" error this system is not useful. I have to either wrap two errors or create a new struct error with Unwrap:

var ErrObjectNotFound = fmt.Errorf("object not found")

if err != nil {
  return fmt.Errorf("%w %w", ErrObjectNotFound, err)
}

I find the Wrap function from go-errors more convenient. We only need to make it compatible with the new system.

[dennwc]

@jfontan Can you please give an example where this would be useful? Do you mean that ErrObjectNotFound would be a Kind in src-d/errors?

We have a similar case in Babelfish, specifically when processing a source file. We may generate multiple errors for it, but instead of wrapping them that way we have a single type ErrSyntax []error which will not implement Unwrap in our case (no single root cause).

I think it may be true for your use case as well: your ErrObjectNotFound should probably be a separate type that wraps an error. But if you find Kind useful for other purposes, you may still use it to wrap things, assuming src-d/go-errors#15 is merged. It will also work fine with our custom error types like ErrSyntax. So it doesn't prevent us from using new interface from Go 1.13.

Note that Kind cannot cover our use case, since it's impossible to inspect when passing more than one error cause. So we cannot rely on Kind to search for it. On the other hand, we can search for it with new methods from Go 1.13.

Also, I find custom types more descriptive: if you try to debug an unknown codebase and print a type of received error, it will always be *src-d/errors.Error in your case. So it's not that different from fmt.Errorf in that regard (type is too generic). But if you define a custom type, it will be *borges.ErrObjectNotFound, which is far more descriptive and useful, in my opinion.

[jfontan]

I agree that a custom type for errors is the way to go. What I don't like is that there's not syntactic sugar for something as common as wrapping a normal error whenever is needed and forcing you to create custom types with Unwrap defined.

type ErrObjectNotFound {
  WrappedErr error
}

func (e *ErrObjectNotFound) Error() string {
  return "object not found"
}

func (e *ErrObjectNotFound) Unwrap() error {
  return e.WrappedErr
}

...

if err != nil {
  return &ErrObjectNotFound{WrappedErr: err}
}

I believe that all this boilerplate could be done by a library, be it go-error or something new.

[creachadair]

I just want to clarify one thing about the new errors methods (or xerrors, in Go 1.12): If you construct an error like fmt.Errorf("something went wrong: %w", err), the resulting error already supports unwrapping to recover err. That's what the %w format escape signals in this case, as opposed to the more usual %v.

(If you need compatibility with Go < 1.13, use xerrors.Errorf instead, but otherwise it works the same way, and the latter is forward compatible)

I don't have a strong position on the API question yet, but I wanted to point that out, since it means you do not (necessarily) need to introduce new custom error types for some of the simple common cases.

[jfontan]

The problem is that fmt.Errorf and it's wrapping mechanism can only be used for annotating errors. The error created is anonymous and cannot be checked by upper layers, that is, the caller cannot check if it's a something went wrong error.

[dennwc]

OK, I think the consensus so far is that we can try using it for projects that support 1.13 exclusively (apps).