errors: new package

errors: new package

This is an attempt to provide a reasonable way
to keep error context around without bulking
up error messages too much.

It also provides a framework for attaching error
classes ("kinds") to errors.

https://code.launchpad.net/~rogpeppe/errgo/errors-package/+merge/163976

(do not edit description out of merge proposal)

[rog, 2013-05-15 15:36:22]

Please take a look.

[dimitern, 2013-05-15 16:37:09]

Excellent! LGTM with a bunch of trivials.

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode1
errors/errors.go:1: package errors
How about a package doc comment with a few examples, as discussed. Mostly:
wrapping (unobtrusive; w/ message), getting the augmented causes (x: y: ..) and
stacks (probably related to logging "best practices"), error kinds (how to use,
define new ones, and check for user-defined ones).

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode14
errors/errors.go:14: // String returns a location in filename.go:99 format.
/path/file.go:99 perhaps? (so not to imply the paths are stripped).

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode104
errors/errors.go:104: // The ErrorKind of the new error is nil.
// Equivalent to standard errors.New() ?

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode110
errors/errors.go:110: // message.  The ErrorKind of the new error is nil.
// Equivalent to fmt.Errorf ?

https://codereview.appspot.com/9426045/diff/2001/errors/errors_test.go
File errors/errors_test.go (right):

https://codereview.appspot.com/9426045/diff/2001/errors/errors_test.go#newcode67
errors/errors_test.go:67: t.Fatalf("expected nil kind; got %#v",
errors.Kind(err))
please use ` instead of escaping \"

[fwereade, 2013-05-15 16:53:52]

I'm finding the API a bit confusing -- it's not very clear how to use this to
best effect. Thoughts?

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode27
errors/errors.go:27: // If it implements ErrorKinder or
..?

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode32
errors/errors.go:32: // which must be comparable for equality.
What behaviour depends on this?

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode43
errors/errors.go:43: return "<no error>"
I think that's either a panic or an "<unknown error>", isn't it?

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode49
errors/errors.go:49: return fmt.Sprintf("%s: %v", e.Message, e.Cause)
I'm a little surprised that we don't use the stack or the kind at all here. Why
not?

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode93
errors/errors.go:93: func NewError(calldepth int, kind interface{}, cause error,
message string) error {
Does this really want to be exported?

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode130
errors/errors.go:130: 
Would you explain the use cases you're thinking of in a bit more detail please?
Without much more detailed docs it's hard to tell how you expect these to be
used -- my own gut feeling is that Wrap and Wrapf are actively harmful, on the
basis that Kind info is only really helpful if it's sticky. I'd probably be fine
if you implemented WrapKind (as Wrap), renamed WrapKindf to Wrapf, and
implemented new WrapKind[f] funcs that accept (err, kind[, f, a...]), thereby
letting us clear or change an error kind easily, but *not* automatically.

I'm probably missing something, but the rationale should probably be recorded
for those who follow.

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode151
errors/errors.go:151: // WithKindf returns a new Error with the given kind
NewKindf?

[rog, 2013-05-15 17:27:20]

initial response. will make changes tomorrow.

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode32
errors/errors.go:32: // which must be comparable for equality.
On 2013/05/15 16:53:52, fwereade wrote:
> What behaviour depends on this?

no behaviour in this package.
the convention is intended to be:

if errors.Kind(err) == someKind {
}

if error kinds aren't comparable for equality, this won't work (or might panic)

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode43
errors/errors.go:43: return "<no error>"
On 2013/05/15 16:53:52, fwereade wrote:
> I think that's either a panic or an "<unknown error>", isn't it?

it's not really unknown - it's more likely to be someone using a zero Err.
perhaps <unset error> ?

i don't want to panic.

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode49
errors/errors.go:49: return fmt.Sprintf("%s: %v", e.Message, e.Cause)
On 2013/05/15 16:53:52, fwereade wrote:
> I'm a little surprised that we don't use the stack or the kind at all here.
Why
> not?

I took a deliberate decision to isolate the message from the stack trace - that
way we can craft a nice message for users while still keeping information around
for log files and developers.

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode93
errors/errors.go:93: func NewError(calldepth int, kind interface{}, cause error,
message string) error {
On 2013/05/15 16:53:52, fwereade wrote:
> Does this really want to be exported?

Yes, i think so - it makes it possible to define new error-creating primitives
elsewhere. The calldepth argument in particular isn't possible to get through
any of the other calls.

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode130
errors/errors.go:130: 
On 2013/05/15 16:53:52, fwereade wrote:
> Would you explain the use cases you're thinking of in a bit more detail
please?
> Without much more detailed docs it's hard to tell how you expect these to be
> used -- my own gut feeling is that Wrap and Wrapf are actively harmful, on the
> basis that Kind info is only really helpful if it's sticky.

I think I disagree. I think the Kind is part of the contract of the function
that's returning the error, and it should have to make an explicit decision to
return the kind of the cause, rather than its own kind. That means that it is
clear in the code when the kind of a returned error depends on the error
returned by the function it's calling.

I'm pretty sure this is the right default.

> I'm probably missing something, but the rationale should probably be recorded
> for those who follow.

agreed.

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode151
errors/errors.go:151: // WithKindf returns a new Error with the given kind
On 2013/05/15 16:53:52, fwereade wrote:
> NewKindf?

hmm. not sure. that sounds like we're making a new kind.
i'm not that happy with WithKind either though.

i'll continue thinking.

[fwereade, 2013-05-15 23:05:31]

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode43
errors/errors.go:43: return "<no error>"
On 2013/05/15 17:27:20, rog wrote:
> On 2013/05/15 16:53:52, fwereade wrote:
> > I think that's either a panic or an "<unknown error>", isn't it?
> 
> it's not really unknown - it's more likely to be someone using a zero Err.
> perhaps <unset error> ?
> 
> i don't want to panic.

STM like a clear case of programmer error... what's the use case for our
depending on this not mattering? I'm having visions of embarrassing "error: <no
error>" output ;p.

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode49
errors/errors.go:49: return fmt.Sprintf("%s: %v", e.Message, e.Cause)
On 2013/05/15 17:27:20, rog wrote:
> On 2013/05/15 16:53:52, fwereade wrote:
> > I'm a little surprised that we don't use the stack or the kind at all here.
> Why
> > not?
> 
> I took a deliberate decision to isolate the message from the stack trace -
that
> way we can craft a nice message for users while still keeping information
around
> for log files and developers.

In the context we're considering -- deeply nested "WTF" errors -- I think it's
counterproductive to discard the kind/trace information. It's *exactly* the
information that makes the difference between "eh, run with --debug until it
happens again" and "ah, I see what happened there".

I guess that's just one layer of log-the-stack-trace in cmd, but I'm still a bit
wary; will we make log (loggo?) depend on this package so we get automatic
logging of what we care about? Or will we just have to remember it every time?
I'm not sure that "discard potentially-irreplaceable information" is the right
default here.

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode93
errors/errors.go:93: func NewError(calldepth int, kind interface{}, cause error,
message string) error {
On 2013/05/15 17:27:20, rog wrote:
> On 2013/05/15 16:53:52, fwereade wrote:
> > Does this really want to be exported?
> 
> Yes, i think so - it makes it possible to define new error-creating primitives
> elsewhere. The calldepth argument in particular isn't possible to get through
> any of the other calls.

It's calldepth in particular that makes Err.Location a potential lie. Can't we
just add primitives to this package as we need them?

(hmm, I don't really see why any of the Err fields are exported... everything
necessary is accessible via top-level functions. Use case for modification?)

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode130
errors/errors.go:130: 
On 2013/05/15 17:27:20, rog wrote:
> I think I disagree. I think the Kind is part of the contract of the function
> that's returning the error, and it should have to make an explicit decision to
> return the kind of the cause, rather than its own kind. That means that it is
> clear in the code when the kind of a returned error depends on the error
> returned by the function it's calling.
> 
> I'm pretty sure this is the right default.

Hmm. I think it's better to be able to wrap ErrTerminateAgent without breaking
it (also the various NotFoundy things from state and environs). If you consider
this mechanism to be an exception-analogue, as I think it fundamentally is (and
as I expect the "average developer" to view it) it's equivalent to nesting every
exception in a base exception at every level -- and I submit that this violates
Least Surprise most impolitely. I think a *change* of error kind is the abnormal
case that needs to be flagged, if only so that developers from other languages
don't end up hunting us for sport.

tomb.ErrStop (or, uh, whatever it is) feels like another interesting case -- we
can't in general wrap those without breaking them; and, worse, we can't depend
on tomb being updated to make use of these either. Bah.

https://codereview.appspot.com/9426045/diff/2001/errors/errors.go#newcode151
errors/errors.go:151: // WithKindf returns a new Error with the given kind
On 2013/05/15 17:27:20, rog wrote:
> On 2013/05/15 16:53:52, fwereade wrote:
> > NewKindf?
> 
> hmm. not sure. that sounds like we're making a new kind.
> i'm not that happy with WithKind either though.
> 
> i'll continue thinking.

NwWithKindf() :)?