proposal: Go 2 error values

[jba]

This issue is for discussion of our Go 2 error values proposal, which is based on our draft designs for error information and formatting.

This proposal will follow the process outlined in the "Go 2, here we come" blog post: we will have everything ready to go and checked in at the start of the Go 1.13 cycle (Feb 1), we will spend the next three months using those features and soliciting feedback based on actual usage, and then at the start of the release freeze (May 1), we will make the "launch decision" about whether to include the work in Go 1.13 or not.

Update 2019-05-06: See launch decision here: https://github.com/golang/go/issues/29934#issuecomment-489682919

[jimmyfrasche]

Another way to go about this: https://play.golang.org/p/hqbmRk9Loh0

I'd argue that this is better since it catches a case the others don't, as far as I can tell. Say the returned error, A, wraps another error, B. If A has a Permission method that returns false and B has a Temporary method that returns true, the other pollX methods would stop looking at A but this would inspect both and select the correct case.

[JavierZunzunegui]

Ah I see, the issue is that matches in As is defined as exact type match.

Yes, that is the issue. I was highlighting the case of unexported error types (such as noWrapper, the return type of Opaque(...)) as instantiating a pointer to such types is very difficult.

Personally, I would argue that it should follow the definition of assignability according to the Go language, and provided by just calling the reflect.Type.AssignableTo. It seems entirely sensible that you would want to match against an interface.

Agreed that assignability makes more sense than exact type match. For As however that means passing a pointer to an interface, such as (given type Temporary interface{Temporary() bool}) a *Temporary. That is a very bad pattern, even the official FAQ itself warns about it.

The fact that doing something this simple (ultimately, its just a for loop over some wrappers and checking one error each time) should require something officially discouraged just shows that As is the wrong abstraction. The better solution is a Last(error, func(err) bool) error, there the provided function can be used with assignability fine (and exact match too, obviously).

[jba]

errors.As is a nightmare to use with un-exported error types.

It isn't intended for use with unexported types. If a package doesn't export a type, then you shouldn't be trying to get at that type.

We chose As because we observed a common pattern: code often checks to see if a returned error is of a certain (exported) type, and then usually wants to manipulate the error as that type. For example,

if e, ok := err.(*googleapi.Error); ok { 
    fmt.Printf("HTTP code = %d\n", e.Code)
}

where googleapi is google.golang.org/api/googleapi.

As extends that common pattern so it works with a chain of wrapped errors. It turns out to be pretty easy to use in practice, and looks a lot like the original code:

var e *googleapi.Error
if errors.As(err, &e) {
    fmt.Printf("HTTP code = %d\n", e.Code)
}

I'm not saying that your Last isn't useful, but it's more cumbersome to use in this case:

e := errors.Last(err, func(err error) bool { _, ok := err.(*googleapi.Error); return ok })
if e != nil {
    fmt.Printf("HTTP code = %d\n", e.(*googleapi.Error).Code)
}

Note particularly the need to duplicate the type assertion.

[creker]

I find As very confusing in the presence of concrete error types implemented as pointer and value receivers. Here's a couple of examples https://play.golang.org/p/BfweaGiCSMI

Examples 1, 3 work but 4 doesn't which is very confusing. Examples 1 and 4 are even identical in their implementation. You have to take into account how error is implemented, which you can only know from its source. Even more confusing is panic in example 2. Probably the implementation could take this case into account and adapt. In that case it would as confusing as the rest of the examples. If it isn't allowed, then it's even more confusing and would probably need to be documented.

Personally, I find Cause(err error) error easier to use and less error prone. Compared to the examples, its semantics are obvious and can be clearly defined in a couple of comment lines. It also goes in line with idiomatic way of handling errors - type switches. As feel less Go'ish.

[jba]

Here's a couple of examples https://play.golang.org/p/BfweaGiCSMI

When I click on that link, I see definitions of Wrapper, Unwrap and As. No examples.

Would you mind including them inline? It makes it easier to discuss that way.

[JavierZunzunegui]

It isn't intended for use with unexported types. If a package doesn't export a type, then you shouldn't be trying to get at that type.

Perhaps I was misleading by emphasizing unexported types, a clearer approach is to focus on interface assignability. As stated before As does not work if you are trying to find any error that implements a given interface (and modifying it to work with interfaces is possible but would involve using an anti-pattern). Last does work fine for interfaces an matching types alike.

I'm not saying that your Last isn't useful, but it's more cumbersome to use in this case:

As it stands it is more cumbersome. An easy solution (I have used it plenty with other wrapper error packages) is to define the type-specific Last method per error type (struct or interface). For example:

package googleapi

type Error struct {...}

func isError(err error) bool {_, ok := err.(*googleapi.Error); return ok}

func LastError(err error) *googleapi.Error {
  gErr := errors.Last(err, isError); 
  if gErr == nil {return nil}
  return gErr.(*googleapi.Error)
}

The usage now is trivial:

if e := googleapi.LastError(err); e != nil {
    fmt.Printf("HTTP code = %d\n", e.Code)
}

I don't think defining this helper function is an issue (chances are you'll check for the error more than once anyways), but either way it should be obvious that if generics are ever added this helpers will be trivial.

[jba]

Personally, I find Cause(err error) error easier to use and less error prone.

We prefer As to Cause because it lets you get any error in the chain, not just the last one.

As feel less Go'ish.

As follows a pattern that we've recently found useful in a couple of places. For example, see gocloud.dev/blob.Bucket.As and similar functions in the Go Cloud Development Kit, which let you retrieve underlying implementation types from our cloud-agnostic wrappers. We chose As there for the same reason—it lets you retrieve more than one type of value.

[creker]

@jba the examples are in the main. I made the runnable. Here's the relevant part

Error types

type ValueErr struct {
}

type PtrErr struct {
}

func (e ValueErr) Error() string {
    return "ValueErr"
}

func (e *PtrErr) Error() string {
    return "PtrErr"
}

func getValueErr() error {
    return ValueErr{}
}

func getPtrErr() error {
    return &PtrErr{}
}

The examples

//example 1
err := getValueErr()
var target ValueErr
fmt.Println("example1 works -", As(err, &target))

//example 2
err := getValueErr()
var target *ValueErr
fmt.Println("example2 panics at line 36 -", As(err, target))

//example 3
err := getPtrErr()
var target *PtrErr
fmt.Println("example3 works -", As(err, &target))

//example 4
err := getPtrErr()
var target PtrErr
fmt.Println("example4 doesn't -", As(err, &target))

[jba]

@jba the examples are in the main.

Ugh, sorry, I forgot to scroll down!

I agree that when you're first learning about As (or json.Unmarshal, for that matter), it takes a little thought to remember how to use it. But I think one quickly gets the idea, that you need to pass a pointer to the type you want. And sometimes that means a pointer to a pointer. The panic in example 2 is a helpful reminder that you're holding it wrong.

As to needing to know whether a type is a "value type" or "pointer type," that is usually something one has to understand about the types that one uses. It affects assignment, parameter passing, and method-call side effects, among other things. You don't need to examine the source, just the godoc. In this case, you'd look for the receiver type of the Error method.

[iand]

I agree that when you're first learning about As (or json.Unmarshal, for that matter), it takes a little thought to remember how to use it. But I think one quickly gets the idea, that you need to pass a pointer to the type you want. And sometimes that means a pointer to a pointer. The panic in example 2 is a helpful reminder that you're holding it wrong.

There is a vet check in the new analysis package that verifies that a pointer type is passed to Decode/Unmarshal functions in the standard encoding packages. That could be usefully extended to cover errors.As in the future.

[ericlagergren]

@jimmyfrasche if Match was defined as Match(error, ...func(error) bool) it’d be about as good as we’re going to get, I think. At least in conjunction with changing As to be about assignability.

[creker]

@jba

I agree that when you're first learning about As (or json.Unmarshal, for that matter), it takes a little thought to remember how to use it

I have to disagree. json.Unmarshal is much more obvious in what it expects than As. It always expects pointer to a struct. No pointer to pointer in some cases and pointer in others. Even when something is wrong, you get a clear error.

The panic in example 2 is a helpful reminder that you're holding it wrong.

Then it has to be explicitly documented. The only thing this panic reminds me is that reflect package is hard to work with and leads to exactly this kind of errors. I think API consumer doesn't need to be reminded of that or even know there's reflection inside.

that is usually something one has to understand about the types that one uses

But that's not the case with errors. I don't use them, I only want to check for their presence, hence I have no idea how they're implemented and, frankly, shouldn't even care. Go goes a long way to try to hide the difference between value/pointer, making As feel alien as it forces you back into C/C++ kind of mental model and pointer to pointer nightmares. Even when Go reminds of them, for example, when type asserting errors, I get compilation error instead of runtime error or, even worse, silent bug as is the case with example 4.

As follows a pattern that we've recently found useful in a couple of places

I understand that it is useful but there's a reason github.com/pkg/errors became such an extremely popular package. Looking at how both of your examples are from Google libraries, I can't help but feel certain bias towards the Google way of doing things. I'm not against As entirely and do not propose to remove it, it is a useful pattern. Although, I wouldn't object from removing or changing it either. But maybe Cause or similar API should also be included or some middle-ground found.

@iand I think vet became the definition of slippery slope. More and more corner cases in API are left to vet instead of fixing it. The need for vet before the API is even implemented is a reminder that there's something wrong with the design.

If we look at upspin errors, there's similar Match method but it completely avoids all of the problems by:

1. Accepting only upspin errors
2. Not using second argument as a kind of return value

Clearly we cannot impose similar restrictions but it certainly something to think about. Maybe using second argument as both input and output is not a good idea.

[ericlagergren]

I like the comment but one thing stood out to me:

I don't use them, I only want to check for their presence ... there's a reason github.com/pkg/errors became such an extremely popular package

In Go, errors are data. They’re meant to be used and manipulated. They contain useful information.

I’ve seen this sentiment cause projects to handle errors by wrapping them in some minimal (string and/or stack trace) context, kicking them up the call stack until they hit the original caller. The caller then has zero clue what to do with the error, so it’s either treated as fatal, blindly logged, or ignored.

I don’t think the stdlib should endorse punting errors. The tools it provides should assume errors contain useful data.

[jba]

Then it has to be explicitly documented.

It is: "As will panic if target is nil or not a pointer." (See https://godoc.org/golang.org/x/xerrors#As.)

[creker]

@jba example 2 doesn't trigger nil check, it triggers panic later in the code. There's also a problem with my example now that I see it - wrapping pointer interface masked the fact that it is a nil pointer and confused me once again. That can be solved by a simple check and documentation will hold true. But other examples are still relevant.

@ericlagergren I didn't mean it that way. What I meant is that I don't care about implementation details. Developer is free to implement errors as pointer receivers or value receivers. And don't want these details leaking into my code and causing hard to debug problems. And when I do need to know about the details, it's better be a compilation error. That doesn't mean I don't care about what's inside the error. In the example 4 the problem silently causes a bug. I pass a pointer but not the right one - I should be passing pointer to pointer. And the fact that this bug is in the error code path makes it even worse as there's high probability it will go unnoticed until it's too late.

[JavierZunzunegui]

It is: "As will panic if target is nil or not a pointer." (See https://godoc.org/golang.org/x/xerrors#As.)

there are other scenarios in which As should panic:

• not a pointer to an type implementing error (*os.PathError or ***os.PathError)
• a pointer to an interface, implementing error or not (*MyErrorInterface)

(also typed nil pointer, not sure if implicitly stated in your comment) ((**os.PathError)(nil))

I think that list of panic scenarios alone should disqualify As for use in such a basic library. If that is not enough:

And the fact that this bug is in the error code path makes it even worse as there's high probability it will go unnoticed until it's too late.

That is a very good point, adding As risks causing panics on error scenarios, completely hiding the error we were trying to give better context in the first place. It's not like we have to have As to get identical functionality anyways, and the alternatives don't have such risks.

[neild]

One of the more common patterns in error handling is the association of errors with an enumerated set of error codes. Code using errors like this often follows one of the following approaches:

// Type assertion to a concrete type.
if e, ok := err.(CodedError); ok && e.Code == CodeRed { ... }

// Type assertion to an interface type.
if e, ok := err.(Coder); ok && e.Code() == CodeRed { ... }

https://play.golang.org/p/Wew4MU8cDmR

As is intended to permit the use of this pattern with wrapped errors.

// As a concrete type.
if e := (CodedError{}); As(err, &e) && e.code == CodeRed {}

// As an interface type. (Assuming As is extended to match on assignability.)
if e := Coder(nil); As(err, &e) && e.Code() == CodeRed {}

This isn't the only way in which to do this, but all of the alternatives I've seen so far are a substantial increase in verbosity over the existing type-assertion based approaches. Given the prevalence of this pattern, I think it's worth supporting it succinctly with wrapped errors.

[JavierZunzunegui]

@neild regarding the pattern you bring up:

Following the pattern in https://github.com/golang/go/issues/29934#issuecomment-460254148 without As you can do the same, with the same verbosity and with compile time safety:

// As a concrete type
if e, ok := LastCodedError(err); ok && e.code == CodeRed {...} 

// As an interface type
if e := LastCoderError(err); e != nil && e.Code() == CodeRed {...} 

In both cases I assume alongside CodedError / CodedError you define the methods:

// As a concrete type
func LastCodedError(err error) (CodedError, bool) {
  lastErr := errors.Last(err, func(err error) bool {_, ok := err.(CodedError); return ok})
  if lastErr == nil {return CodedError{}, false}
  return lastErr.(CodedError), true
}

// As an interface type
func LastCoderError(err error) CoderError {
  lastErr := errors.Last(err, func(err error) bool {_, ok := err.(CoderError); return ok})
  if lastErr == nil {return nil}
  return lastErr.(CoderError)
}

These are amortized between all CodedError / CoderError checks. If you don't like writing them I can provide a simple go generate ... command to make them. And when (if) generics arrive they'll be even more trivial.

// As an interface type. (Assuming As is extended to match on assignability.) if e := Coder(nil); As(err, &e) && e.Code() == CodeRed {}

As should not be extended to match assignability, above &e is a pointer to an interface with is an anti pattern. See official FAQ.

[neild]

As should not be extended to match assignability, above &e is a pointer to an interface with is an anti pattern. See official FAQ.

As the FAQ you reference says, you should almost never use a pointer to an interface. This is one of the cases when you want one.

As's target parameter is an out parameter. Out parameters are very uncommon in Go, since the ability to return multiple values usually renders them unnecessary. A case where out parameters are useful is when the type of value returned varies, as in the parameters to fmt.Scan. (Other examples include the "github.com/ghemawat/re" package and the "cloud.google.com/go/spanner"'s Row type.) This is also the case with As.

[creker]

@neild

but all of the alternatives I've seen so far are a substantial increase in verbosity over the existing type-assertion based approaches.

Cause handles these examples even better and doesn't introduces problems I described. It also plays nicely with type switches - the idiomatic way of handling errors.

var err error = CodedError{"error", CodeRed}

//this is not required, you can also pass err and get the same result
wrapped := errors.Wrap(err, "wrapped") 

if e, ok := errors.Cause(wrapped).(CodedError); ok && e.code == CodeRed {
    fmt.Println("1:code red")
}

if e, ok := errors.Cause(wrapped).(Coder); ok && e.Code() == CodeRed {
    fmt.Println("2:code red")
}

It's really weird to see As call in the right part of a condition and the fact that it allocates a variable. That's doesn't feel like Go at all. It also harder to read - you have to scan to the right. Right now the majority if not every case of error handling requires you to only look at the left part of a condition if you want to see which errors the code checks. As example uses left part only to allocate block-scoped variable to save one extra line of code that As requires in order to put something as a second argument.

As is useful, no denying that. It wouldn't be used if it wasn't useful. But as it stands right now, I think it needs more thought put into it. There's too much problems with it as I see it and there's no clear benefit to justify it. Let's not rush this and regret when people will constantly stumble upon the same bugs only to hear - you should've used vet or read documentation more carefully. That really wouldn't be Go way of doing things. There's already enough weird cases in Go right now.

[jimmyfrasche]

Cause assumes that only the first error in the chain is relevant and that every other is just a wrapper for context. That's not necessarily true. If you have a "data store not writable" error that's the relevant error, but it may wrap something lower-level like "disk quota exceeded" or "permission denied" for extra context. That's an important detail but not what I need to respond to my application logic. With Cause you skip right over the relevant part. That is not useful to me.

[JavierZunzunegui]

Please see https://github.com/JavierZunzunegui/Go2_error_values_feedback. There I show just how easily one can use the Last pattern with go generate to do exactly what As is trying to to, with full type safety (for value errors, pointer errors or interface errors).

package mypkg

//go:generate genny ... 
// provides LastMyError(error) *MyError
type MyError struct {
  Code int
  ...
}

if e := LastMyError(err); e != nil {
  fmt.Println(e, e.Code) // e is type *MyError !
}

How can As be seen as being better than this, with all it's gotcha's that cause panics or silently failing? It's even shorter than the otherwise proposed syntax:

if e := (CodedError{}); As(err, &e) && e.code == CodeRed {}

[creker]

@jimmyfrasche that's the specifics of a particular implementation. No one is stopping us from extending this approach to handle your case. I propose we rethink As and fix its problems, not copy/paste github.com/pkg/errors.

What would be a perfect solution is generics. That way we could probably replace output part of the second argument with properly typed return value.

That's an important detail but not what I need to respond to my application logic. With Cause you skip right over the relevant part. That is not useful to me.

Then don't use Cause. Cause handles very specific but still extremely popular idiomatic case. For me, this was the only type of error handling I ever needed. Maybe some of my comments sounded like that, but I'm not saying that's the only way and everything else should be ignored.

[neild]

@JavierZunzunegui You're comparing an example with a helper function to one without. It's hardly surprising that the former is shorter.

[networkimprov]

Reposting this, regarding proposed changes to fmt.Errorf():

The issue is not simply magical strings in the API; it's error-case output generation. It is essential to let your customers decide how/when to upgrade that kind of code. Note that if "existing programs automatically start working better" (@rsc in https://github.com/golang/go/issues/29934#issuecomment-459824434) due to altered error-case output, that will cause error condition tests to fail.

And I think you've overlooked the benefits of go fmt -r and go fix.

The fmt API below is one correct way to provision these features (and is partly supported by @agnivade in https://github.com/golang/go/issues/29934#issuecomment-460021288). Please provide a correct API, and then address how existing programs would upgrade to it.

Another reason to keep APIs clean is that far more code will be written in Go than has been written in it.

func Error(a ...interface{}) error                        // counterpart of F/S/Print()

func Errorw(e error, a ...interface{}) error              // wraps
func Errorwf(e error, f string, a ...interface{}) error

func FmtError(a ...interface{}) error                     // formats; FormatError returns nil
func FmtErrorf(f string, a ...interface{}) error

func FmtErrorw(e error, a ...interface{}) error           // both; FormatError returns e
func FmtErrorwf(e error, f string, a ...interface{}) error

[neild]

The issue is not simply magical strings in the API; it's error-case output generation. It is essential to let your customers decide how/when to upgrade that kind of code.

Can you provide a specific example of the type of case you think will be broken by these changes?

[nhooyr]

func Error(a ...interface{}) error                        // counterpart of F/S/Print()

func Errorw(e error, a ...interface{}) error              // wraps
func Errorwf(e error, f string, a ...interface{}) error

func FmtError(a ...interface{}) error                     // formats; FormatError returns nil
func FmtErrorf(f string, a ...interface{}) error

func FmtErrorw(e error, a ...interface{}) error           // both; FormatError returns e
func FmtErrorwf(e error, f string, a ...interface{}) error

I understand your reservation regarding the new API being magical but that does not look like a clean API. Could you elaborate on how exactly it works and why so many new functions are necessary?

[networkimprov]

@nhooyr the proposed Errorf() API has two switches. One returns a Formatter instance, the other returns a wrapped error. That yields three new features, expressed above as FmtError(), Errorw(), and FmtErrorw(). Each one has a *f() variant like fmt.Printf() etc.

Error() should have appeared with Errorf() long ago (and errors.New() should have been deprecated then).

If the Go team intends that all errors now implement the Formatter interface, then the API would be:

func Error(a ...interface{}) error                     // FormatError returns nil
func Errorf(f string, a ...interface{}) error

func Errorw(e error, a ...interface{}) error           // FormatError returns e
func Errorwf(e error, f string, a ...interface{}) error

[networkimprov]

@neild the only mention in the proposal of impact on existing code is:

When not in detail mode (%v, or in Print and Println functions and their variants), errors print on a single line.

Would the "single line" be identical to the one printed today? If so, how does your Errorf() plan make "existing programs automatically start working better"?

[jimmyfrasche]

Ignoring all the arguments for and against the changes to fmt.Errorf, if there were a Wrap(err error, context string) equivalent to Errorf("%s: %w", context, err), I would use that 90%+ of the time.

[neild]

@networkimprov

Would the "single line" be identical to the one printed today?

Yes.

If so, how does your Errorf() plan make "existing programs automatically start working better"?

Printing errors using detail formatting (%+v) will print location and other information for the entire annotation chain without the need to update existing code.

@jimmyfrasche

Ignoring all the arguments for and against the changes to fmt.Errorf, if there were a Wrap(err error, context string) equivalent to Errorf("%s: %w", context, err), I would use that 90%+ of the time.

Note that it's entirely possible to write this function yourself. A key goal of this proposal is to enable different error implementations to interoperate vis-a-vis wrapping, formatting, etc. There are no magical features only available in errors.New or fmt.Errorf.

[jimmyfrasche]

Certainly. I've already written mine. I've also written a few that predate the interfaces that this proposal standardizes. They have Unwrap() error methods because I was ahead of the curve on that one, but they don't collect the stack frame [edit: or with the printer].

I imagine a lot of people will write their own, too, and that a lot of people have written their own before.

But now we have many different ways of doing one of the most common things—some of them work correctly and some don't.

The primary benefit of having one version is that you don't need to think when reading it. You see it and you know what it's doing and that it works correctly. You don't have to double check that this version of Wrap produces an error with the correct Unwrapmethod or if it is a Wrap that predates xerrors and hasn't been updated yet.

Is, As, and Unwrap could also be written outside this proposal. They're just helpers for the common cases.

[neild]

Is, As, and Unwrap could also be written outside this proposal. They're just helpers for the common cases.

This is true. The greatest value (in my opinion) of placing Is, As, and Unwrap in the errors package is as a reference implementation.

Anyway, my intent was not to argue for or against Wrap(err, "context") vs. fmt.Errorf("context: %w", err), but to emphasize that errors.New and fmt.Errorf aren't the end of the story.

[networkimprov]

Printing errors using detail formatting (%+v) will print location and other information for the entire annotation chain without the need to update existing code.

@neild I gather the value of : %v magic is to make imported packages produce clearer error messages when their client apps use %+v. What happens to the error-case output of an unchanged client app (using %v) when an imported package starts using %+v?

@jimmyfrasche the Wrap() function you suggest is among those I proposed above:

func Errorw(e error, a ...interface{}) error

[JavierZunzunegui]

I imagine a lot of people will write their own, too, and that a lot of people have written their own before.

I've been doing wrapping with stack frame, but produced output as JSON (with zap) because that way it was consumed best by some tooling. Comes to show the "%s: %s" pattern is not universal nor always best.

[neild]

What happens to the error-case output of an unchanged client app (using %v) when an imported package starts using %+v?

@networkimprov I do not understand the scenario you are describing. Without a concrete example, it's very difficult to tell what the problem you envision is.

[networkimprov]

Does this change to package im break its app caller?

package main                            // app; unchanged
import "github.com/.../im"
...
func f() error {
   err := im.Im()
   if err != nil {
      return fmt.Errorf("f: %v", err)   // unit test relies on this string
   }
   ...
}

package im                              // third-party library; updated
...
func Im() error {
   err := x()
   if err != nil {
      return fmt.Errorf("Im: %+v", err) // formerly used %v here
   }
   ...
}

[neild]

@networkimprov I'm afraid I don't understand that example.

• There is never a reason to use detail formatting of an error (%+v) in a fmt.Errorf format string. Detail formatting is for outputting errors, not for constructing them.
• Your example shows the impact of package im changing a format string. If anything is broken by the change, the root cause is the change to package im.

[networkimprov]

It sure seems like you understood it.

There is never a reason to use detail formatting of an error (%+v) in a fmt.Errorf format string.

OK, so %+v should be vet-flagged in Errorf(). And the above statement should appear in the design doc.

To summarize, you've heard three good reasons to provide error-wrapping fmt APIs:

1. %w is not really a sensible verb, and wouldn't enhance existing code - @agnivade
2. fmt.<wrap>(err, context) is the typical use case - @jimmyfrasche
3. : %w doesn't work in structured strings like JSON - @JavierZunzunegui

Are they not sufficient justification?

[jimmyfrasche]

To clarify, I was assuming it would be errors.Wrap(err, "context"). It's the analogue of errors.New but for wrapping an existing error.

[networkimprov]

OK, but note that errors.New is only in use because fmt.Error(a ...interface{}) was not added along with fmt.Errorf, breaking the X() & Xf() pattern of fmt APIs.

[jba]

@networkimprov First, there are two separate issues:

1. Should we include %w?
2. Should we add a wrapping function?

They might not have the same answer.

Regarding %w, it's true that it doesn't exactly enhance existing code, and it's not purely about formatting. But it's very close to what people write already, so we expect that it will see a lot of use out of sheer convenience.

I don't understand the argument that it doesn't work for structured logging. Nothing about fmt.Errorf works for structured logging, so why pick on %w? I'm probably missing something. Maybe an example would help.

As to whether we should add a wrapping function, I think we've added enough for now. And it's not clear what the form of that function should be. In my experience, it is not <wrap>(err, context); it would be more like <wrapf>(err, depth, format, ...args), since I often construct errors from helper functions (so I need a depth for Caller) and I usually want printf-style formatting. But that's not really right either, because I often want to construct my own error type, not whatever type the standard library will give me.

In short, once we go beyond the simple convenience of %w, the territory gets murky.

[networkimprov]

@jba could you give an example or link showing use of a depth argument?

Note that above I suggested fmt.Errorw/f() APIs which take ...interface{}

Maybe the proposal should simply drop %w and revisit stdlib wrapping tools after the community has accumulated hands-on experience.

[jba]

could you give an example or link showing use of a depth argument?

From https://github.com/google/go-cloud/blob/master/internal/gcerr/gcerr.go#L118:

// New returns a new error with the given code, underlying error and message. Pass 1
// for the call depth if New is called from the function raising the error; pass 2 if
// it is called from a helper function that was invoked by the original function; and
// so on.
func New(c ErrorCode, err error, callDepth int, msg string) *Error {
    return &Error{
        Code:  c,
        msg:   msg,
        frame: xerrors.Caller(callDepth),
        err:   err,
    }
}

[josharian]

A general comment about depth arguments: They are fragile, in the sense that regular refactoring and code changes require apparently unrelated changes elsewhere. Early discussion of testing.T.Helper considered various depth arguments, and we ended up with a design that kept stack handling information local. See https://github.com/golang/go/issues/4899.

[JavierZunzunegui]

I don't understand the argument that it doesn't work for structured logging. Nothing about fmt.Errorf works for structured logging, so why pick on %w? I'm probably missing something. Maybe an example would help.

tl;dr - for my use case at least, the structured logging counter-argument can be dropped (the proposal satisfies my requirements)

@jba my use case is: I use zap to produce JSON error logs of the form {"KEY_1":"VALUE_1", ..., "KEY_N","VALUE_N","base_error":"..."} To produce this I use wrapping errors that have some zap fields (key-value) pairs, something like:

type zapWrapError struct {payload []zap.Field, wrapped error}
// standard Unwrap() method, Error() is JSON form of 'payload'

The base error is a non-wrapping (or nil-wrapping) error. Often from the std library or 3rd party libraries.

Under the new proposal, if I get an error consisting of a message "a" wrapped by "b" and "c" I would like to be able to produce: {..., "KEY_N","VALUE_N","unlabelled_3":"c","unlabelled_2":"b","unlabelled_1":"c"}

I can do this using a custom Printer though (that pre-appends with a key and encodes as JSON). I was under the impression the wrapped error message was included in the message of the wrapping error, but it is not the case https://github.com/golang/exp/blob/master/xerrors/fmt.go#L35.

[JavierZunzunegui]

type Printer interface {
    // ...

    // Detail reports whether error detail is requested.
    // After the first call to Detail, all text written to the Printer
    // is formatted as additional detail, or ignored when
    // detail has not been requested.
    // If Detail returns false, the caller can avoid printing the detail at all.
    Detail() bool
}

The Detail method above is very limited. It was clearly introduced to allow turning frame printing on/off (see https://github.com/golang/exp/search?q=Detail%28%29&unscoped_q=Detail%28%29) but other 'optional' errors will be introduced later that may not want to be printed under the same circumstances as frames. Why not split frame from wrapError, make it a separate error and replace Detail for

Omit(error) bool

Of course the new frameErr would have to implement some public interface that allows for it to be identified for omission, as would any other private error type that wants to support this option.

Also, Detail is enforced by the error type, a strange abstraction, it should instead be enforced by the printer ('detail' is not a global concept, detail to me may be different to detail for you)

[JavierZunzunegui]

tl;dr - don't add %w, do the %v and %s magic in fmt.Errorf (but mark it deprecated), add new method(s) in errors package without any % magic.

Doing the 'magical' wrapping for %s and %v may be reasonable due to historical reasons, but introducing a new %w is just actively going down a path we know is flawed. Let me explain:

It has been acknowledged the proposed API is imperfect: (@rsc https://github.com/golang/go/issues/29934#issuecomment-459824434)

If we were doing it from scratch, we would not do that.

Historical arguments aside, the natural API would be something like

// args does not contain err, format does not have any %X for err
Errorf(err error, format string, args ...interface{})` 

In fact in the proposed implementation the ("{{format_prefix}} %v", args ...interface{}) pattern is literally converted to the ("{{format_prefix}}", args[:len(args)-1]) + error format by errorx.Errorf.

Given the valid migration concerns (also @rsc https://github.com/golang/go/issues/29934#issuecomment-459824434)

But an explicit goal here is to make as many existing programs automatically start working better

doing the %s and %v 'magic' may be reasonable, but why add %w? That still means users are migrating to the new API, and if they have to migrate they might as well do it to the known good pattern, not the known anti-pattern (either way, migrating is super easy). Since the fmt.Errorf would now be considered an anti-pattern, mark it deprecated (// Deprecated, use X instead!) and encourage the new errors.Errorf-like apis.

[networkimprov]

It seems clear that the proposal authors won't revise it before pushing the code they've got into the 1.13 tree. That's fine, but should have been stated up front, so folks with critiques could hold their fire (and save their time!) until the code lands.

@JavierZunzunegui fmt.Errorf() is largely fine, the anti-pattern is only fmt.Errorf("... %v", err) which could be flagged by vet.

@jba @neild thanks for your work on this; I look forward to trying out the code. I'll file an issue re fmt APIs during the 1.13 cycle :-)

[neild]

A small update to the proposal: As changed to match on assignability rather than type equivalence. Thanks to @ericlagergren and others for pointing out that it should.

https://golang.org/cl/161200 https://golang.org/cl/161017