proposal: errors: simplified error inspection

[rogpeppe]

Issue #29934 proposes a way of inspecting errors by traversing a linked chain of errors.
A significant part of the implementation of this is about to land in Go 1.13. In this document, I outline some concerns with the current design and propose a simpler alternative design.

Analysis of the current proposal

I have a few concerns about the current design.

• Like @crawshaw I am concerned that the API has not been sufficiently tested yet.
• Go already has a type assertion statement. The fact that such a low level package as errors needs to resort to interface{} and reflect-based magic is surely not ideal. Beginners will need to be exposed to this from a very early stage, and it's really not easy to explain.
• The implementation of chained errors, with interactions between three interface types, two of which are unnamed, seems quite complex for a fundamental and low level part of the Go standard library. It means that the previously tiny errors package now depends on the fairly hefty internal/reflectlite package.
• There are potentially significant runtime costs: every error inspection involves multiple traversals of a linked list and use of reflection. Any use of As implies an allocation.
• the use of As and Is methods means that you can't in general ask what an error is; you can only ask whether it looks like some other error, which feels like it will make it hard to add definitive error information to log messages.
• dynamic Is method dispatch makes it easy to create unintuitive relationships between errors. For example, context.ErrDeadlineExceeded "is" both os.ErrTimeout and os.ErrTemporary, but os.ErrTimeout "isn't" os.ErrTemporary. A net.OpError that wraps a timeout error "is" os.ErrTimeout but "isn't" os.ErrTemporary. This seems like a recipe for confusion to me.

Although I've been concerned for a while, I did not speak up until now because I had no alternative suggestion that was simple enough for me to be happy with.

Background

I believe that the all the complexity of the current proposal and implementation stems from one design choice: the decision to expose all errors in the chain to inspection.

If inspection only checks a single underlying error rather than a chain, the need for Is and As goes away (you can use == and .() respectively), and with them, the need for the two unnamed Is and As interface types. Inspecting an error becomes O(1) instead of O(wrapDepth).

The proposal provides the following justification:

Some error packages intend for programs to act on a single error (the “Cause”) extracted from the chain of wrapped errors. We feel that a single error is too limited a view into the error chain. More than one error might be worth examining. The errors.As function can select any error from the chain; two calls with different types can return two different errors. For instance, a program could both ask whether an error is a PathError and also ask whether it is a permission error.

It seems to me that this justification rests on shaky ground: if you know you have a PathError, then you are in a position to ask whether that PathError also contains a permission error, so this test could be written as:

if is a permission error or (is PathError and the contained error is a permission error)

This seems like it might be clumsy in practice, but there are ways of working around that (see below). My point is that any given error type is still free to expose underlying errors even though there is only one underlying error (or "Cause").

Current state

As of 2019-06-02, the As and Is primitives have been merged into the Go master branch, along with some implementations of the Is method so that OS-related errors can be compared using errors.Is. Error printing and stack frame support were merged earlier in the cycle but those changes have recently been reverted.

The xerrors package implements more of the proposed API, but is still in experimental mode.

Proposed changes to the errors package

I propose that As and Is be removed, and the following API be added to the errors package:

// Error may be implemented by an error value to signify that
// the error value is adding metadata to some underlying error
// (the "E-value").
type Error interface {
	error

	// E returns the underlying error, or E-value. If it returns
	// nil, the receiver itself should be treated as the E-value.
	//
	// Implementations should return an E-value that has no
	// underlying E-value itself, usually by storing E(err) instead
	// of err. Although technically E-values form a chain, the
	// intermediate values in the chain should never been considered
	// for inspection and the chain will almost always have length
	// 1.
	E() error
}

// E returns the "E-value" of an error - the part of the error that
// should be used for error diagnosis.
//
// The E-value, E(err), is E(err.E()) when err implements Error and
// err.E() != nil, otherwise it's err itself.
//
// When writing code that makes a decision based on an error, the
// E-value should always be used in preference to the error value
// itself, because that allows functions to add metadata to the error,
// such as extra message annotations or source location information,
// without obscuring the actual error.
func E(err error) error {
	for {
		err1, ok := err.(Error)
		if !ok {
			return err
		}
		e := err1.E()
		if e == nil {
			return err
		}
		err = e
	}
}

I've used the name E rather than Cause to emphasise the fact that we're getting the actual underlying error; the error being passed around may include more information about the error, but the E value is the only important thing for error inspection. E also reflects the T name in the testing package.

Although the E method looks superficially similar to Unwrap, it's not the same, because error wrappers don't need to preserve the error chain - they can just keep the most recent E-value of the error that's being wrapped. This means that error inspection is usually O(1). The reason for the loop inside the E function is to keep error implementations honest, to avoid confusion and to ensure idempotency: errors.E(err) will always be the same as errors.E(errors.E(err)).

This playground example contains a working version of the above package.

Proposed changes to os errors

The changes in this part of the proposal are orthogonal to those in the previous section. I have included this section to indicate an alternative to the current use of Is methods on types throughout the standard library, which are, it seems to me, a significant motivating factor behind the current design.

The standard library has been retrofitted with implementations of the Is method to make some error types amenable to checking with errors.Is. Of the eleven implementation of the Is method in the standard library, all but two are there to implement temporary/timeout errors, which already have an associated convention (an optional Timeout/Temporary method). This means that there are now at least two possible ways of checking for a temporary error condition: check for a Temporary method that returns true, or using errors.Is(err, os.ErrTemporary).

The historic interface-based convention for temporary and timeout errors seems sufficient now and in the future. However, it would still be convenient to have an easy way to check wrapped errors against the errors defined as global variables in the os package.

I propose that an new OSError interface with an associated Error function be added to the os package:

package os

// OSError is implemented by errors that may be associated with
// a global variable error defined in this package.
type OSError interface {
	OSError() error
}

// Error returns the underlying OS error of the
// given error, if there is one, or nil otherwise.
//
// For example, os.IsNotExist(err) is equivalent to
//     os.Error(err) == os.ErrNotExist.
func Error(err error) error {
	err1, ok := errors.E(err).(OSError)
	if ok {
		return err1.OSError()
	}
	return nil
}

Then, instead of a custom Is method, any error type that wishes to (syscall.Errno, for example) can provide an os package error by implementing the OSError method.

This domain-specific check addresses this common case without complicating the whole error inspection API. It is not as general as the current proposal's error wrapping as it focuses only on the global variable errors in os and not on the wrapper types defined that package. For example, you cannot use this convention to check if a wrapped error is a *os.PathError. However, in almost all cases, that's what you want. In the very small number of cases where you want to look for a specific wrapper type, you can still do so by manually unwrapping the specific error types via a type switch.

Note that, as with historical Go, there will still be strong conventions about what kinds of errors may be returned from which functions. When we're inspecting errors, we are not doing so blind; we're doing so knowing that an error has come from a particular source, and thus what possible values or types it may have.

Discussion

As with the current proposal, it is important that this design does not break backward compatibility. All existing errors will be returned unwrapped from the standard library, so current error inspection code will continue to work.

This proposal is not as prescriptive as the current proposal: it proposes only a method for separating error metadata from the error value used for inspection. Other decisions as to how errors might be classified are left to convention. For example, an entry point could declare that returned errors conform to the current xerrors API.

The entire world of Go does not need to converge on a single error inspection convention; on the other hand we do need some way of wrapping an error with additional metadata without compromising the ability to inspect it. This proposal provides exactly that and no more.

As an experiment, I implemented this scheme in the standard library. The changes ended up with 330 lines less code (96 lines less production code), much of it simpler.

For example, it seems to me that this code:

// OSError implements the OSError interface by returning
// the OS error for e.
func (e Errno) OSError() error {
	switch e {
	case EACCES, EPERM:
		return oserror.ErrPermission
	case EEXIST, ENOTEMPTY:
		return oserror.ErrExist
	case ENOENT:
		return oserror.ErrNotExist
	}
	return nil
}

is easier to understand than its current equivalent:

func (e Errno) Is(target error) bool {
	switch target {
	case oserror.ErrTemporary:
		return e.Temporary()
	case oserror.ErrTimeout:
		return e.Timeout()
	case oserror.ErrPermission:
		return e == EACCES || e == EPERM
	case oserror.ErrExist:
		return e == EEXIST || e == ENOTEMPTY
	case oserror.ErrNotExist:
		return e == ENOENT
	}
	return false
}

This proposal does not affect the error printing proposals, which are orthogonal and can be implemented alongside this.

Comparison with other error inspection schemes

This proposal deliberately leaves out almost all of the functionality provided by other schemes, focusing only on the ability to discard error metadata. The issue of how to inspect the E-value of an error is left to be defined by any given API.

In this way, the proposed scheme is orthogonal to other error frameworks, and thus compatible with them.

For example, although it does not directly support Unwrap-based chain inspection or error hierarchies, there is nothing stopping any given API from documenting that errors returned from that API support those kinds of error inspections, just as existing APIs document that returned errors may be specific types or values. When bridging APIs with different error conventions, it should in most cases be possible to write an adaptor from one convention to another.

The E-value definition is quite similar to the Cause definition in the errgo package, but it has one important difference - the E-value is always its own E-value, unlike Cause, which can return an error which has another Cause. This eliminates one significant source of confusion, making a strict separation between error metadata and errors intended for inspection. The error metadata can naturally still hold the linked chain of errors, including stack frame and presentation information, but this is kept separate from the E-value - it should not be considered for error inspection purposes.

Summary

This proposal outlines a much simpler scheme that focuses entirely on separating error metadata from the error inspection value, leaving everything else to API convention.

I believe there are significant issues with the current design, and I would prefer that the new errors functionality not make it into Go 1.13, to give us more time to consider our future options.

[bcmills]

CC @neild @jba

[earthboundkid]

Are the semantics of E() as you propose them the same as pkg/errors.Cause()? If so, why not reuse the method? If not, how do they differ?

[rogpeppe]

@carlmjohnson No, the semantics are different, though not dissimilar. That Cause function returns nil if there's an error with a Cause method that returns nil, which means that errors.Cause of a non-nil error can return nil, which errors.E will not.

Also, the associated conventions are different. In pkg/errors, the Cause chain is used for both error diagnosis and for error metadata (e.g. stack frame information). This proposal keeps the two concepts entirely separate, and does not try to define how metadata is represented. The existing proposal for error formatting is still applicable.

Furthermore, the term Cause is used in different ways in several existing packages, all with subtly different semantics. I thought it was worth using a method with a distinctly different name so that the semantics and conventions are clear and it can be retrofitted to existing code without significant friction.

This was a good question BTW. I should mention this difference in the "comparison" section.

[networkimprov]

@rogpeppe, you might want to drop a note about this issue on golang-dev.

[kardianos]

@rogpeppe

There are potentially significant runtime costs: every error inspection involves multiple traversals of a linked list and use of reflection.

This is true, but I don't typically see this type of error inspection on the hot path.

the use of As and Is methods means that you can't in general ask what an error is; you can only ask whether it looks like some other error, which feels like it will make it hard to add definitive error information to log messages.

If you log an error, call err.Error(). If you want to test if an error is something to do a specific action, then use "Is" or "As". It isn't clear to me what you mean by "definitive". I don't consider the initial error to be "definitive"; the initial error typically has too little information to be useful.

dynamic Is method dispatch makes it easy to create unintuitive relationships between errors. For example, context.ErrDeadlineExceeded "is" both os.ErrTimeout and os.ErrTemporary, but os.ErrTimeout "isn't" os.ErrTemporary. A net.OpError that wraps a timeout error "is" os.ErrTimeout but "isn't" os.ErrTemporary. This seems like a recipe for confusion to me.

I don't understand why this is confusing. Could you clarify?

Here is my use case, why I appreciate the current 1.13 implementation, and where your proposal would fall short:

My applications work with databases a lot. This interaction can have errors at multiple places:

• Temporary conditions within the database itself (locked table).
• Invalid SQL.
• Network partitions and invalid configurations.
• Errors thrown from SQL procedures.

I also tend to use helper functions that may combine information from the SQL text with the SQL error to extract and highlight the line of the error. In this case the SQL driver that handles the error condition won't (and shouldn't) have access to the SQL text. This helper function will wrap the an error with additional query context information.

Furthermore, almost all SQL errors (of all types) are used for business logic and thus sometimes errors have distinct business logic ramifications that may need to be tested for.

So given the above, what would be the "cause", or what would the "definitive" error? It depends on what I'm doing. I sometimes want to look at the "cause" (maybe invalid SQL), but other times I need to see what business logic error and take some action based on that (maybe failed to fetch order).

The current proposal satisfies all these needs with very little code change. This will be much easier to explain to most new Go developers and is similar to what they may be used to from Java or C# with multiple catch statements. After reading your proposal and playground implementation, I don't think it would solve the issues my code bases have with error handling today.

[rogpeppe]

@kardianos

There are potentially significant runtime costs: every error inspection involves multiple traversals of a linked list and use of reflection.

This is true, but I don't typically see this type of error inspection on the hot path.

I think that may depend significantly on the application. Some error kinds (for example "not found" errors) can be very frequent.

the use of As and Is methods means that you can't in general ask what an error is; you can only ask whether it looks like some other error, which feels like it will make it hard to add definitive error information to log messages.

If you log an error, call err.Error(). If you want to test if an error is something to do a specific action, then use "Is" or "As". It isn't clear to me what you mean by "definitive". I don't consider the initial error to be "definitive"; the initial error typically has too little information to be useful.

If I want to understand why my program is behaving in a specific way when handling errors, I can't always stop it at runtime and ask "Is" or "As" questions of the error value. Often I'll need to create a log message that provides some reasonable post-mortem view of the error. With the current proposal, that's not easy to do, because the entire chain of errors contributes to that decision-making process, including dynamic method dispatch to As and Is methods in the chain. Why isn't my code responding to this "As" error correctly? I need to look at the types and values of all the errors in the chain and understand what dynamic "As" checks each one implements.

dynamic Is method dispatch makes it easy to create unintuitive relationships between errors. For example, context.ErrDeadlineExceeded "is" both os.ErrTimeout and os.ErrTemporary, but os.ErrTimeout "isn't" os.ErrTemporary. A net.OpError that wraps a timeout error "is" os.ErrTimeout but "isn't" os.ErrTemporary. This seems like a recipe for confusion to me.

I don't understand why this is confusing. Could you clarify?

"Is" feels like a kind of equality operator. This relationship sounds like A=B and A=C but B ≠ C, which is confusing to me at least.

Here is my use case, why I appreciate the current 1.13 implementation, and where your proposal would fall short:

My applications work with databases a lot. This interaction can have errors at multiple places:

• Temporary conditions within the database itself (locked table).
• Invalid SQL.
• Network partitions and invalid configurations.
• Errors thrown from SQL procedures.

I also tend to use helper functions that may combine information from the SQL text with the SQL error to extract and highlight the line of the error. In this case the SQL driver that handles the error condition won't (and shouldn't) have access to the SQL text. This helper function will wrap the an error with additional query context information.

Furthermore, almost all SQL errors (of all types) are used for business logic and thus sometimes errors have distinct business logic ramifications that may need to be tested for.

So given the above, what would be the "cause", or what would the "definitive" error? It depends on what I'm doing. I sometimes want to look at the "cause" (maybe invalid SQL), but other times I need to see what business logic error and take some action based on that (maybe failed to fetch order).

The current proposal satisfies all these needs with very little code change. This will be much easier to explain to most new Go developers and is similar to what they may be used to from Java or C# with multiple catch statements. After reading your proposal and playground implementation, I don't think it would solve the issues my code bases have with error handling today.

It seems to me like you're describing an error domain here. It would be up to you (or someone else, of course) to define the conventions for the way that errors work in that domain. This is not unrelated to the approach I've suggested for OS-related errors in the proposal above.

For example, you could define an interface that encapsulates information about an SQL error:

type SQLError interface {
	// DriverErr holds the driver-specific information about the SQL error encountered.
	// Alternatively, you might want this to return a more driver-specific error, such as *pq.Error.
	DriverErr() error
	// SQLText holds the text of the SQL statement that failed. This may return the
	// empty string if the text was not known when the error was returned.
	SQLText() string
}

Then you can implement specific types to represent specific business logic errors, each of which could implement SQLError to make available the underlying SQL error that caused the business logic error. Although, to be honest, if you've diagnosed a business logic error, you probably don't care about the underlying SQL error any more, so it probably wouldn't be that useful to include it.

In your code, you could do:

switch err := errors.E(err).(type) {
case *OrderNotFoundError:
	// Fields in OrderNotFound error could hold info
	// on the underlying SQL error or anything else.
	return http.StatusNotFound
case SQLError:
	// inspect err.DriverErr and SQLText as desired
	return http.StatusInternalServerError
}

The point I'm trying to make with this proposal is that there doesn't have to be a one-size-fits-all approach to error inspection. The only common aspect to all error handling schemes is that we need to be able to add metadata without affecting the error value that we use for making decisions, which is what I've proposed.

I suggest that it's actually quite rare to encounter a situation where we are really mixing and matching a bunch of different kinds of errors from different layers, which is what the current proposal facilitates. Instead, this proposal favours an encapsulation approach where each layer makes its own decision as to what conventions to support. Those conventions can and should be mentioned in the API documentation so the caller knows what kinds of errors to expect and can check accordingly.

It's my intuition that this approach has nicer compositional properties than the current proposal and will scale better to large code bases, making large-scale refactoring less error-prone due to less unexpected interactions between errors in different layers. Not to mention that's it's much simpler. :)

[kardianos]

"Is" feels like a kind of equality operator. This relationship sounds like A=B and A=C but B ≠ C, which is confusing to me at least.

A = io.ReadWriter
B = io.Reader
C = io.Writer

A is B
A is C
B is not C

This is not confusing to me.

I understand your design rational I believe. It is compelling.

I suggest that it's actually quite rare to encounter a situation where we are really mixing and matching a bunch of different kinds of errors from different layers, which is what the current proposal facilitates.

I suspect you may underestimate this, or the cost of refactoring other layers / shims to make this work over time.

[josharian]

A is B
A is C
B is not C

A is a B. A is a C. B is not a C.

“is” is a different relationship than “is a”.

[MOZGIII]

The core idea about just adding metadata without actually changing the actionable error is captured pretty good with this proposal.

However, from a tooling perspective, the implementation doesn't look complete.

From errors.E:

// When writing code that makes a decision based on an error, the
// E-value should always be used in preference to the error value
// itself, because that allows functions to add metadata to the error,
// such as extra message annotations or source location information,
// without obscuring the actual error.

With this in mind, if I have an intermediate value that wraps the raw error with a higher level error, that in turn is also wrapped again, how would I check for that middle error?

I can assert the top-level error with the error value, and the bottom-most with E-value, but to assert on mid-level (intermediate) error there seems to be no way around but to write custom logic for it.

While I agree that current proposal is too complicated (I fully agree that debugging the errors with dynamic dispatch does sound great - it's a solid argument against it) - this one seems to be lacking parts of an API to make life easier - like build-in support for walking around the error chain. Although, it's unclear if it's actually a requirement - maybe it's a good idea to force people to write their own code for that.

[creachadair]

I suggest that it's actually quite rare to encounter a situation where we are really mixing and matching a bunch of different kinds of errors from different layers, which is what the current proposal facilitates.

I suspect you may underestimate this, or the cost of refactoring other layers / shims to make this work over time.

That may be so. However, I think in practice the cases where this level of detail are needed are comparatively uncommon, and while we shouldn't prevent those domains from addressing their issues, I wonder whether it makes sense for the whole Go ecosystem to pay in a more complex API.

Domains where layered inspection is needed will need to define shared abstractions for errors in any case, even with proposal #29934. Some attractive aspects of #32405 here are a smaller, conceptually simpler API, and better separation of concerns: Cases where a chain of errors is really needed can still have their way, but my experience of using the xerrors prototype so far has been that the majority of cases mostly just walk to the end of the chain anyway.

I'd argue that it's preferable to make APIs that really want the complexity to pay the cost for it—and I think the cost wouldn't be that high, relative to an API big enough to care. The design proposal advocated chaining in a fairly general way ("more than one error might be worth examining") without motivating examples.