Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use go error wrapping instead of go-errors in apps #383

Open
dpordomingo opened this issue Sep 16, 2019 · 15 comments
Open

Use go error wrapping instead of go-errors in apps #383

dpordomingo opened this issue Sep 16, 2019 · 15 comments

Comments

@dpordomingo
Copy link
Contributor

dpordomingo commented Sep 16, 2019

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?

@dpordomingo dpordomingo changed the title Start using builtin go error wraping instead of go-errors Start using go error wrapping instead of go-errors Sep 16, 2019
@kuba--
Copy link
Contributor

kuba-- commented Sep 16, 2019

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

@dpordomingo dpordomingo changed the title Start using go error wrapping instead of go-errors Use go error wrapping instead of go-errors in apps Sep 16, 2019
@kuba--
Copy link
Contributor

kuba-- commented Sep 16, 2019

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

@dpordomingo
Copy link
Contributor Author

dpordomingo commented Sep 16, 2019

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--
Copy link
Contributor

kuba-- commented Sep 16, 2019

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
Copy link
Contributor

dennwc commented Sep 16, 2019

@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
Copy link
Contributor

dennwc commented Sep 16, 2019

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--
Copy link
Contributor

kuba-- commented Sep 16, 2019

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
Copy link
Contributor

dennwc commented Sep 16, 2019

@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--
Copy link
Contributor

kuba-- commented Sep 16, 2019

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
Copy link
Contributor

jfontan commented Sep 16, 2019

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
Copy link
Contributor

dennwc commented Sep 16, 2019

@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
Copy link
Contributor

jfontan commented Sep 16, 2019

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
Copy link
Contributor

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
Copy link
Contributor

jfontan commented Sep 18, 2019

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
Copy link
Contributor

dennwc commented Oct 1, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants