What stands out to me most is the serialization API is disjoint. In particular, it’s not immediately clear how to use it (unlike in the original proposal) and requiring bytes.Buffer as part of the API is problematic.

I like the idea of the FrameError interface, but the nameFrameLocation is redundant and returning (string, string, int) is unfortunate. Perhaps it should just be a struct, not an interface.

I like howLast takes a predicate function, but it’s misnamed. It should really be First, or something else entirely different.

Having to replace err.Error() with String(err) is a a bit of a non-starter.

To me, the features it provides makes it seem more like a third-party library than something I’d expect in the stdlib.

Thanks for the feedback, @ericlagergren

What stands out to me most is the serialization interface is disjoint. In particular, it’s not immediately clear how to use it (unlike in the original proposal) and requiring bytes.Buffer as part of the API is problematic.

For how to use it, see test file in the example package. Basically a printer is initialized with a serializer, then use printer.Write on a io.Writer and error.

The bytes.Buffer in Serializer is no accident (an earlier version used []byte), but it allows more efficient implementations. See the newly added String(error) benchmark results. This is inspired by the zap library.

To me, the features it provides makes it seem more like a third-party library that something I’d expect in the stdlib.

Unfortunately the moment you add error wrapping to the std library and these wrapping does not allow to recover the individual error message (without the wrapped ones), this can't be achieved. This is not possible as a third-party library alongside the official proposal.

{FrameError} Perhaps it should just be a struct, not an interface.

An interface because frame testing is very complicated (and extremely brittle) as a struct. I can be made a struct but bare in mind that.

It should really be First

Last => 'the last error to be added'
First => 'the first error in the chain'
Depends how you look at it. I have no inconvenience either way.

CC @mpvl @neild

I expected to see a section on why this approach is better than the one in the original proposal. You refer to the original proposal but you don't clearly explain what you are improving.

@ianlancetaylor I should have made the distinctions clear. I'll do it here:

Formatter(original) vs Serializer(this)
Formatter is a property of the error. That means if an error type, FooError, decides is prints it's content like "{FooError}: {wrapped}", that is final. If a different error, BarError, wraps it and uses a different format, say "{BarError} - {wrapped}", that makes it inconsistent: "{BarError} - {FooError}: {...}". Equally, in FormatError(p Printer) (next error) the 'next' is effectively the filter on what errors get printed, what not. That can't be changed - what if I logging time my criteria is different to the that of the person who created the error? Again, set in stone too soon. Printer's Detail, used to pick or ignore frame info, is a hidden acknowledgment of this issue, but much too narrow.
Serializer is a separate component that decides all the above (which errors are printed via Keep, how each error is printed via CustomFormat, how errors are put together via Append). That means errors can come from different libraries, follow different patterns and still be put together in a consistent way of my choosing.

Is and As(original) vs Last(this)
Has been discussed a fair bit already, mostly with @neild and @jba. As can't be type checked and may panic if mis-used, an issue I have strong reservations with. Is is just a special case of Last. Last is slightly more verbose, but is safe and more versatile - it can match errors by properties other that type match. In my proposal, the implementations of Cause, Similar, Contains and part of the Serializer examples logic becomes trivial with Later in a way neither As nor Is can support, showing right away the deficiencies in the original solution.

Frames
The original proposal doesn't put much thought about custom errors types, and as such the support for frames ends in providing a Frame type and Caller.
In this one, simply embed Wrapping in the error an you get Unwrap implementation and frame for free. See fooError usage in this test for an example.

Performance
The original proposal provides no benchmarks, but I am pretty sure once they are provided they will show a lot of unnecessary memory allocations - either when the error is created, or when Error() is called.
I have taken this into account, and thanks to Printer and Serializer, this proposal can really minimize memory allocation, as shown in these benchmarks.
I recommend reviewing zap performance comparison, I take inspiration from it's implementation and shows how many wasteful allocations careless loggers can have.

Migration
The 'automatic upgrade' suggested in the original proposal is already proving to not be completely seamless - error comparison with reflect.DeepEqual is broken.
This proposal acknowledges a automatic, transparent upgrade is not possible, and gives priority getting to a good solution over getting to a solution in which wrapping works everywhere right away. Supporting wrapping will require moving away from fmt.Printf to Wrap or custom errors with embedded Wrapping.
One significant (negative) aspect of this proposal is that error's Error method has basically got to stop being called in normal code and replaced for String(error) or %s, interface{}, equivalent. This is why the migration to my proposal involves a 2-step process (prepare for, and then merge changes) and will break more than just error comparison in tests if not done correctly or rushed.

I understand the motivation for Serializer, but I think you are missing one of the important design considerations of the proposal: we want to separate human-readable display of errors from the program's view of errors. If a custom error type wants to display an underlying error so someone can diagnose the problem, but doesn't want to expose that error to programs because it's an implementation detail, then it would not return that underlying error from Unwrap or make it visible to programs in any other way. That means that a Serializer implemented outside of the custom error's package could not format the underlying error. So I think it is necessary that errors control their own formatting, but we should encourage a uniform output style by example to avoid the problem that Serializer is trying to solve.

I don't think you can conclude from the fact that we only provide Frame and Caller that we didn't "put much thought" into custom error types. We chose a small API from which users could build what they needed. Your Wrapping type provides some slight additional convenience, but at the cost of several exported symbols (Wrapping, NewWrapping, WrapOptionFunc, OmitFrame and SkipNFrames) and a use of the problematic option-function pattern (e.g. what happens if I pass both OmitFrame and SkipNFrames?).

As you mentioned, we've discussed Last vs. Is/As elsewhere, but I want to make a general point. In your last comment you claim that because Last is more general, Is/As has "deficiencies." If there's one thing Go has taught us (or at least me) in the last decade, it's that more general is not necessarily better.

Lastly, I want to point out that Serializer, Last and Wapping can all coexist with the original proposal.

Thanks for the feedback @jba

I don't quite follow 100% of your first paragraph (mind clarifying the missing one of the important design considerations part?), but from what I understand:

a Serializer implemented outside of the custom error's package could not format the underlying error

Not format it in a custom way, but by default it would use the Error() output. If that is not enough, it can always drop it (Keep() = false) altogether, using as criteria 'doesn't satisfy any of the custom error interfaces I support'. I don't see how the original proposal fares any better.

So I think it is necessary that errors control their own formatting, but we should encourage a uniform output style by example to avoid the problem that Serializer is trying to solve.

Problem is, I don't think that enough. The normal, general, logical style may be "{err}: {err}", but for some purposes the error is not meant for human but machine use and may want frames only, or in JSON form, etc. I don't think a 'one format fits all' is realistic, so the error itself should not be committed to a format at all.

Re Frame and Caller:
I should have expressed it differently, but I find the original incomplete in this area. Custom errors are supposed to call Caller, hold the Frame, implement Detail, ... Assuming everyone will use them as expected and correctly is unrealistic. I found Wrapping a good way to simplify and unify usage. The implementation, including OmitFrame/SkipNFrames, etc., may require improving, happy to take suggestions. Currently OmitFrame trumps SkipNFrames.

In your last comment you claim that because Last is more general, Is/As has "deficiencies."

My point is In and As alone are not enough, so they need Last (or other similar solution) anyways. Also, note Contains in this proposal is equivalent to Is anyways (also supporting comparing wrapped targets).

Lastly, I want to point out that Serializer, Last and Wrapping can all coexist with the original proposal.

Last can, trivially easily. Wrapping probably, I guess with some changes to support Detail. I don't think Serializer can, since the individual error's Error() returns the entire error message (as in including wrapped ones), so would not be able to enforce its pattern if the error is not of a known type (or interface).

Ping @jba @neild Any other comments on this proposal?

Nothing new from me.

Addressing a few specific points:

Formatter is a property of the error. That means if an error type, FooError, decides is prints it's content like "{FooError}: {wrapped}", that is final.

Under #29934, errors that implement a FormatError do not have control over the ordering and relative presentation of their text and that of their wrapped error, so this is factually incorrect.

The original proposal provides no benchmarks, but I am pretty sure once they are provided they will show a lot of unnecessary memory allocations

This is an unsupported assertion. It is also unclear what specific operations this is referring to.

One significant (negative) aspect of this proposal is that error's Error method has basically got to stop being called in normal code

This strikes me as untenable.

Thank you for your attention @ianlancetaylor @jba @neild @ericlagergren

I am re-designing some parts of this proposal based on ideas shared in the main proposal discussion and other feedback received, so do not spend any unnecessary time on this one.

Answering @neild :

Fomatter -> my point is it is the Formatter (the error) that implements FormatError(p Printer) (next error). What if the error serialises itself (i.e. calls printer) it in a way that doesn't suit me (because it is a 3rd party error)? Best thing I can do is hack the Printer, which I'm pretty sure is not the desired intention.
Also, if a non-Formatter error exists in the chain wrapping a Formatter, the Printer will not be passed on (can only call Error()), so even hacking the Printer won't be much good.
In short, I must admit I really don't get what Formatter is trying to achieve.

This is an unsupported assertion. It is also unclear what specific operations this is referring to.

True, but I don't have bandwidth to define benchmarks for #29934 , would be great if some of the owners did. The operations I would like to see benchmarked are wrapped error creation (including any fmt-%w logic and frame capturing) and serialization of wrapped errors (Error() or FormatError).

This strikes me as untenable.

I've come to agree. It is the main aspect I am trying to fix on the next iteration

@JavierZunzunegui

The meaning of error's Error() string is conceptually different to that of the original proposal.

You cannot change the meaning of Error() string. That is an existing API.

This is an unsupported assertion. It is also unclear what specific operations this is referring to.
True, but I don't have bandwidth to define benchmarks for #29934 , would be great if some of the owners did. The operations I would like to see benchmarked are wrapped error creation (including any fmt-%w logic and frame capturing) and serialization of wrapped errors (Error() or FormatError).

What @neild means is that there are benchmarks. It is unclear what you mean.

One of the issues errors.Printer and errors.Formatter address is allowing localization of errors. I'm unclear how this is addressed in this design.

Equally, in FormatError(p Printer) (next error) the 'next' is effectively the filter on what errors get printed, what not. That can't be changed - what if I logging time my criteria is different to the that of the person who created the error?
Best thing I can do is hack the Printer, which I'm pretty sure is not the desired intention.

Printer was designed to allow for custom implementations, for instance supporting localization or converting the error chain into a structured form. There is a lot of semantics reflected in the interfaces of proposal 29934. For instance, it maintains two error chains: one for semantics and one for detailed analysis purposes only. The APIs for both are deliberately very different to ensure there remains little confusion as to which is which. Another example is ability to localize errors through the Printer interface (see how x/text, and other localization frameworks, allows localized formatting of strings and how this is reflected in the Printer API). This proposal seems to largely discard these semantics. Unless I'm missing something.

It would be good to see some examples of writing an error works in this proposal. I have to admit I don't fully understand it.

About the differences:

separates the responsibility of carrying error data (the error interface) and that of turning errors into a string (a new serializer interface).
for error {"foo"} wrapping {"bar"} wrapping {"cause"}, can produce:
"foo - bar - cause" or
"foo: bar: cause" or
"{"foo":{"bar":{"cause"}}} or

This can be done in the original proposal by defining another printer.

discourages the use of string-like errors, favours using custom error types

Why? It is quite easy to write custom errors in the original proposal.

easy use of stack frames alongside custom errors.
struct MyError {/* custom parameters ; */ errors.Wrapping}
func (err *MyError) Error string {...}
func NewMyError(wrap error) error {return &MyError{Wrapping: NewWrapping(wrap)}

In your proposal, xerrors.New does not include stack information. Only wrapped errors do. How would you add stack information in errors.New or for any error that does not wrap an error?

It is not hard to use it in the original proposal:

struct MyError {/* custom parameters ; */ frame errors.Frame}
func (err *MyError) Error string {fmt.Sprint(err}
func (err *MyError) FormatError(p *errors.Printer) error {...; err.frame.Format(p) }
func NewMyError(wrap error) error {return &MyError{Formatter: fmt.Errorf("msg: %v", wrap)}

Assuming that one has to write an additional FormatError, this is about the same work. Having to add a FormatError and Unwrap method can be tedious.

But nothing in the original proposal prohibits one from writing a similar pattern:

// type in some lib somewhere
type Wrapped struct {
msg format
// Not formatting at the time of error creation significantly improves creation
// time and also improves the ability to localize.
args []interface{}
wrapped error
frame errors.Frame
}
func (e *Wrapped) Error() error { fmt.Sprint(e) }
func (e *Wrapped) Unwrap() error {return e.wrapped}
func (e *Wrapped) FormatError(p *errors.Printer) error {
p.Printf(e.msg, e.args...); e.frame.Format(p); return e.wrapped
}
func Wrapf(err error, format string, args ...interface{}) error {
return &Wrapped{format, args, err, errors.Caller(1)}
}

Then:

struct MyError {/* custom parameters ; */ x.Wrapped}
func NewMyError(wrap error) error {return &MyError{Wrapped: x.Wrapf("msg: %v", wrap)}

This is even shorter (sure you can do that with your proposal too, but that is not the point.

efficient error serialisation.

Did you compare the benchmarks against the proposal implementation in Core?

It seems you're benchmarking the printing, not the wrapping. It is actually the wrapping that is the interesting thing to measure. BTW, the original proposal allows for a significant performance gain in Errorf in many cases, this allows us to amortize the cost of storing the frame somewhat in these cases. Your implementation seems to do quite a bit of work for wrapping.
Why would you think printing is more efficient, though?

easy to navigate the wrapped error chain including finding wrapped errors, equality comparison and cause error.

Why is this different? The Unwrap function is identical. Is can be used for easy comparison. Admittedly, informative-only errors are harder to navigate, but this was intentional to avoid having users rely on them for programmatic decisions. We omitted Cause as we thought Is is the preferred idiom (and semantically more correct).

abandons any intention to automatically have existing errors upgraded - users will need to update code to start wrapping errors.

I agree with this difference. Relying on the redefinition of Error() is a deal breaker, though. Have you thought about how to adapt your design to not do that?

Other observations:

How would printing work in conjunction with fmt functions? Many existing packages, like pkg/errors, rely on the %+v format to work. The original proposal allows for such packages to be adapted to use the proposed interfaces and keep working.

Is there a way to print detail information other than stack frames? I didn't see it but might have missed it.

Thanks for the feedback @mpvl

I am re-designing some parts of this proposal based on ideas shared in the main proposal discussion and other feedback received, so do not spend any unnecessary time on this one.

Just highlighting that since the answer to some of the below is 'yeah, needs improvement, I'm working on it'

discourages the use of string-like errors, favours using custom error types

Why? It is quite easy to write custom errors in the original proposal.

I just mean this discourages string errors by not having wrapping support via Printf(...%w). The original, by supporting wrapping via Printf, naturally makes string errors more accessible. This ties in with Formatter/Printer vs Serializer, see below.

separates the responsibility of carrying error data (the error interface) and that of turning errors into a string (a new serializer interface). [...]

This can be done in the original proposal by defining another printer.

Formatter/Printer vs Serializer - I give you a scenario:
say PersonError implements Formatter, calls it Printer.Print with [name (string), age (int)]
say HTTPError implements Formatter, calls it Printer.Print with [msg (string), code (int)]
As far as Printer is concerned, these are two cases of Print(string, int), so can't apply separate logic to each. With Serializer, CustomFormat receives the type, which serves to distinguish them. Also a printer can't decide what gets printed, a Serializer can (if say I want to show PersonError but not HTTPError.

Formatter/Printer - a separate concern - I give you a scenario:
say FError implements Formatter
say NotFError does not implement Formatter
Both implement Wrapper
Assume the wrapping order is FError-0 wraps NotFError-0 wraps FError-1 wraps ...
Then the Printer p provided to FError-0 formats that error, but can't be passed on to NotFError-0 since it does not implement Formatter, which must fall back to NotFError-0.Error() and so calls FError-1.Error() or FError.FomatError(...) with whatever default Printer it is set to use, but not p.
My point being using custom Printers seems to rely on all errors implementing Formatter, one that doesn't breaks the chain.

In your proposal, xerrors.New does not include stack information. Only wrapped errors do. How would you add stack information in errors.New or for any error that does not wrap an error?

Correct, only wrapped errors have stack information (or any other wrap-related functionality). The idea was to create xerrors.New or other possibly non-wrapping errors, then if adding more context via Wrapper-implementing errors. This is being improved on the next iteration.

Did you compare the benchmarks against the proposal implementation in Core?

Sorry, been looking at the original proposal not golang master. Seen the benchmarks, will make sure to make comparable benchmarks.

It seems you're benchmarking the printing, not the wrapping. It is actually the wrapping that is the interesting thing to measure. BTW, the original proposal allows for a significant performance gain in Errorf in many cases, this allows us to amortize the cost of storing the frame somewhat in these cases. Your implementation seems to do quite a bit of work for wrapping.
Why would you think printing is more efficient, though?

(regarding my proposal)
It is wrapping that is the interesting thing to measure - with the exception of stacks (which I haven't tried to optimize yet), there is basically no logic in wrapping, just build your error with the embedded Wrapping. The output of Error() is in no way calculated at wrapping time.
It seems you're benchmarking the printing - yes, in particular I want to make sure I don't allocate the intermediate error values in the heap, i.e. for output foo: bar: cause I don't want to allocate bar: cause. I do that via using a buffer, building it up and then generating the string, i.e. (as bytes) "" -> "foo" -> "foo: " -> "foo: bar" -> "foo: bar: " -> "foo: bar: cause". By using sync.Pools I can do just that, allocating only the output string, if the wrapped errors can produce their individual output without allocating themselves (a condition I am somewhat improving on the next iteration).
(regarding the original proposal)
My concern is that there is some in intermediate allocation of those strings. Maybe not for errors implementing Formatter if they use Printer correctly (newPrinter() is basically doing what I do regarding buffering), but certainly for errors not implementing it, which must produce a string.

easy to navigate the wrapped error chain including finding wrapped errors, equality comparison and cause error.

Why is this different?

As - I REALLY don't like this, but some of you must be tired of hearing me go on and on about it, I won't repeat it again. I just prefer Last, or pretty much any viable solution better. More info in Is and As(original) vs Last(this).
More generally, I think the fundamental problem navigating (and particularly comparing) wrapped errors in the original is how to compare errors which are identical in every respect, except that they wrapped different errors?. I think the solution being proposed (if I understand it correctly), have reflect.DeepEqual not look at errors or look at them specially, is unsatisfactory. In my proposal, because Error() does not touch the wrapped error, one can use type equality and Error() equality (same string) as a means of comparing errors, that's fundamentally what powers Contains and Similar in my proposal which I think can't be replicated in the original.

abandons any intention to automatically have existing errors upgraded - users will need to update code to start wrapping errors.

I agree with this difference. Relying on the redefinition of Error() is a deal breaker, though. Have you thought about how to adapt your design to not do that?

Yes 😄, should be posting that very soon

How would printing work in conjunction with fmt functions? Many existing packages, like pkg/errors, rely on the %+v format to work. The original proposal allows for such packages to be adapted to use the proposed interfaces and keep working.

For each separate printing form (%s, %v?, %+v, ?) a Serializer needs to be implemented that satisfies that format, and be called explicitly by Printf-like functions. Should be no different output than it is now if the old errors satisfied some pattern (such as "... : %s", err) that the Serializer can replicate. Having said that, since migration to wrapping is not automatic, the main thing here is when updating some error to wrapping, make sure the new wrapped format matches whatever output the un-wrapped produced, or face breaking %s, etc. comparisons.

Is there a way to print detail information other than stack frames? I didn't see it but might have missed it.

As it stands, no, but there is nothing special about frames other than they are added by default by the standard library and the way the default serializer handles them. If you don't use these default implementations you could use completely custom stack formats, or whatever it is you may need.

If you managed to read up to here - thank you!

If you managed to read up to here - thank you!

I did :)

So who would implement custom serializers? I'm still quite unclear about the concepts and who would implement what. It seems that custom formatters must have explicit knowledge of the errors it handles I don't understand how such an approach would scale.

Should be no different output than it is now if the old errors satisfied some pattern (such as "... : %s", err) that the Serializer can replicate.

How would a serializer have access to such patterns? These patterns are usually gone by the time an error is created. I agree that it is useful to keep these (i.e. for localization), but even changing Errorf to store those would be a breaking change. Actually, storing err as we do know is not fully backwards compatible, but that is unlikely to cause problems. The Printf method of Printer allows for errors to convey such structure, still, in a generalized way so that text extractors can pick it up for translation.

say PersonError implements Formatter, calls it Printer.Print with [name (string), age (int)]
say HTTPError implements Formatter, calls it Printer.Print with [msg (string), code (int)]
As far as Printer is concerned, these are two cases of Print(string, int), so can't apply separate logic to each. With Serializer, CustomFormat receives the type, which serves to distinguish them. Also a printer can't decide what gets printed, a Serializer can (if say I want to show PersonError but not HTTPError.

How is that scalable? I still think I'm not grasping your proposal. Clearly a Serializer cannot be expected to know of all errors. I must be missing something.

Note that in the original implementation information about the error chain is available as well, and thus one could write a formatter that selects on error types. But generally it would be infeasible for a formatter to know about all errors. It doesn't seem like a practical avenue.

Maybe things will be clearer to me once this design no longer assumes the semantics of the Error() method changes.

So who would implement custom serializers? I'm still quite unclear about the concepts and who would implement what.

Custom serializers are meant to be much rarer (and harder to produce) that custom errors. I'd imagine besides the default serialiser ("{err}: {err}: ...") only a handful of serialisers will be produced and shared widely, for example with the below functionalities:

• print frames (in shorter / longer form)
• add type info (a bit like %#v or %+v)
• timing info
• ...

A good reference is zapcore.Encoder, from which I draw much inspiration.

The other category of serialisers are those more tied to whatever libraries you are using, unlikely to be shared as widely. These likely rely more on Keep to only serialise some errors. Some examples:

• HTTP error message, printing only errors deemed 'publicly visible, non-internal', so the user has rich context but not any implementation details
• as JSON encoded Key-Value pairs for log aggregator consumption
• hide errors not safe for logging to some systems, think of "**********" hidden fields
• encode to protobuf
• ...

It seems that custom formatters must have explicit knowledge of the errors it handles I don't understand how such an approach would scale.

yes, some serialisers may have to have some explicit knowledge of the errors it handles.
Some highlights in this regards:

• Some may want to reject altogether errors that don't match certain criteria, use Keep for that
• Similarly, some may want to exclude errors that do match certain criteria
• Others may want to keep all errors and yet apply some custom logic to errors of unrecognised type, by relying on the output of Error() (e.g. applying JSON encoding).

Note also this custom functionality is best supported via interfaces, not types matching. To facilitate a scalable approach and easier integration of errors in 3rd party libraries, it would be advisable to encourage some minimal standards amongst errors such as having certain method(s):
ErrorData() []interface{}
and/or
StringErrorData() []string
and/or
KeyValueErrorData() [][2]interface{}
and/or
...

Also all errors (even private ones) should have some public method through which one can, if need to, identify them and either drop them or access their raw data. This is particularly significant for 3rd party error types, since one can't know what users will want to do with the error down the line.

How would a serializer have access to such patterns? These patterns are usually gone by the time an error is created. I agree that it is useful to keep these (i.e. for localization), but even changing Errorf to store those would be a breaking change. Actually, storing err as we do know is not fully backwards compatible, but that is unlikely to cause problems. The Printf method of Printer allows for errors to convey such structure, still, in a generalized way so that text extractors can pick it up for translation.

I can't understand what you mean here, sorry.

What I am trying to say though is that there is no automatic wrap error upgrading - adding these changes, in itself, makes no change whatsoever. When users begin to upgrade their fmt.Errorf to a wrapping equivalent, if they were following the "{err}: {err}" pattern they may still be able to produce errors with the same Error() output. But migrating is a breaking change regardless (type comparison, type assertion, ... won't behave in the same way). But I think this is only slightly more breaking that the original proposal, since most issues are common.

@ianlancetaylor @ericlagergren @jba @neild @mpvl

As promised, please see the revised proposal #31111.

It preserves many features from this one, the main improvement is that Error() is no longer changing it's meaning nor has to be phased out, instead all wrapped errors are in fact a single error type which holds instances of the errors that the user wants wrapped.

Thank you for all the feedback, I am closing this issue in favor of the new one #31111