Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

proposal: errors/errd: helper package for deferred error handlers #32676

Open
rsc opened this issue Jun 19, 2019 · 35 comments

Comments

@rsc
Copy link
Contributor

commented Jun 19, 2019

The try proposal (#32437) uses a placeholder fmt.HandleErrorf to annotate errors.
We do not intend that to be the actual function name. It was a blank to be filled in.
Here is one possible way to fill in that blank.

// Package errd provides error-handling functions.
// All of them are meant to be deferred,
// with the address of the function's error result
// as the first argument. For example:
//
//	defer errd.Trace(&err)
//
// All of the functions take action only when err != nil
// (meaning the function in which the defer appears is
// itself returning a non-nil error); they are no-ops when err == nil.
package errd // import "errors/errd"

// Trace prints the error and the current goroutine's stack trace.
// It does nothing when *errp == nil.
//
// Example:
//
//	defer errd.Trace(&err)
//
func Trace(errp *error)

// Apply applies the function f to the error (*errp = f(*errp)).
// It does nothing when *errp == nil.
//
// Example:
//
//	defer errd.Apply(&err, func(err error) error {
//		if err == io.EOF {
//			err = io.ErrUnexpectedEOF
//		}
//	})
//
func Apply(errp *error, f func(err error) error)

// Add adds context to the error.
// The result cannot be unwrapped to recover the original error.
// It does nothing when *errp == nil.
//
// Example:
//
//	defer errd.Add(&err, "copy %s %s", src, dst)
//
// This example is equivalent to:
//
//	defer errd.Apply(&err, func(err error) error {
//		err = fmt.Errorf("copy %s %s: %v", src, dst)
//	})
//
// See Wrap for an equivalent function that allows
// the result to be unwrapped.
func Add(errp *error, format string, args ...interface{})

// Wrap adds context to the error and allows
// unwrapping the result to recover the original error.
//
// Example:
//
//	defer errd.Wrap(&err, "copy %s %s", src, dst)
//
// This example is equivalent to:
//
//	defer errd.Apply(&err, func(err error) error {
//		err = fmt.Errorf("copy %s %s: %w", src, dst)
//	})
//
// See Add for an equivalent function that does not allow
// the result to be unwrapped.
func Wrap(errp *error, format string, args ...interface{})

Thoughts and improvements welcome.

/cc @mpvl @jba @neild

@thepudds

This comment has been minimized.

Copy link

commented Jun 19, 2019

errd.Tracef might be natural as well, or having the current errd.Trace also take format string, args ...interface{}.

@cespare

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

I don't think Apply carries its weight. (It seems helpful here largely to describe how Add and Wrap work.)

The example code isn't quite right; I think you meant:

defer errd.Apply(&err, func(err error) error {
	if err == io.EOF {
		return io.ErrUnexpectedEOF
	}
	return err
})

But if I were going to do that, I would instead write

defer func() {
	// Don't even need a specific nil check in this case.
	if err == io.EOF {
		err = io.ErrUnexpectedEOF
	}
}()

That is clearer and faster: it only need refer to the single err variable; it doesn't need the err != nil check at all since it only cares about io.EOF; it only involves a single function call instead of two; it's lighter on the page; and so on.

If you had a more complicated function to apply, it seems fine to just make it take a *error and do the if *err == nil { return } check itself:

func f() (err error) {
	defer errd.Apply(&err, complexFunc1)
	defer complexFunc2(&err)
	 // ...
}

func complexFunc1(err error) error {
	// lots of stuff
	return err
}

func complexFunc2(errp *error) {
	if *errp == nil {
		return
	}
	// lots of stuff
}

The version without Apply isn't obviously better here, as above, but it's not clearly worse either.

@cespare

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

This is probably too bike-shed-y at this early phase, but I wonder if there is a better name than Trace since the concept of "trace" has an unrelated antecedent in the Go library and runtime. Perhaps:

  • Print
  • Stack
  • Dump
  • Examine
  • Debug
@sonatard

This comment has been minimized.

Copy link

commented Jun 19, 2019

Following two sample codes missing err in last argument.

//	defer errd.Apply(&err, func(err error) error {
//		err = fmt.Errorf("copy %s %s: %v", src, dst)
//	})
//	defer errd.Apply(&err, func(err error) error {
//		err = fmt.Errorf("copy %s %s: %w", src, dst)
//	})
@eandre

This comment has been minimized.

Copy link

commented Jun 19, 2019

Thanks @rsc, this looks great to me. Personally this addresses the only remaining concern I had with the main try proposal, which was around how to make it convenient to "do the right thing" with adding context to errors.

@beoran

This comment has been minimized.

Copy link

commented Jun 19, 2019

Whether or not try() is going to be implemented, this errd package is a great idea, because, I have to admit, that after thinking about it, using defer for error handlers makes sense in many cases, as long as performance is not critical.

Unlike @cespare I think the errd.Apply() is a great idea because it allows to define named and resuable error handlers in advance and then just pass them to errd.Apply(), without having to define an inline lambda. A few common use cases could have predefined error handlers in the standard library. For example, to close a file on an error, we could have a method func (f File) CloseErr(err error) error, which closes the file if err is not nil, and then we could do defer errd.Apply(file.CloseErr) . The CloseErr function could then be implemented for every standard library type that has a Close(), to encourage this pattern.

One use case which seems to be missing is logging the error using the log package. I think and errd.Log() function could be very useful.

However, I think there is one way that would make this even more useful and that would be to make it a "fluid" API, a bit like this:

package errd // import "errors/errd"

type errd  * error

func New(err * error) errd {
   return errd(err)
}

func (e errd) Trace() errd 
func (e errd) Log() errd
func (e errd) Logf(fmt string, args...interface{}) errd
func (e errd) Apply( f func(err error) error) errd
func (e errd) Add(errp *error, format string, args ...interface{}) errd
func (e errd) Wrap(errp *error, format string, args ...interface{}) errd

Then we can easily compose error handlers like this:
defer errd.New(&err).Log().Add("%s", "some context").Handle(file.CloseErr)

@johanbrandhorst

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

What is the rationale for the name? errors/errd doesn't say anything to me. Is it supposed to be short for error-defer? What about something like errors/deferred instead? Could still be aliased to errd if deferred isn't an obvious namespace.

@DisposaBoy

This comment has been minimized.

Copy link

commented Jun 19, 2019

I personally like the idea of try(f(), handler) more, but my only concern with this proposal is the introduction of a new package when we already have the errors helper package.

@mikeschinkel

This comment has been minimized.

Copy link

commented Jun 19, 2019

Simple question as I am obviously missing something. Why a pointer to error when error is already an interface?

Also, does errd mean "ERRor Defer?" Being able to read abbreviations as if not abbreviated always helps me reason about code.

@jba

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

Why do you feel the need for a separate package, rather than putting these in errors? (Aside from the implementation detail of accessing formatting.)

I don't see why Trace is here. It has a few problems:

  • It's generally a mistake to both print an error and return it. That typically leads to multiple output lines per error, creating log spam.
  • Where does Trace print to? Stdout? Stderr? The log? I usually log errors when I print them, but a command-line tool might prefer stderr. If this package is only going to have one function for this, I don't see an obvious best choice.
  • Printing the full stack trace will likely lead to duplicate traces, since the error is being returned up the stack where other Trace calls can get ahold of it. Consider printing just the current frame.

Shouldn't the two formatting functions end in f?

I've seen the name Annotatef for what you're calling Add. That is a bit long, but Notef isn't, and pairs nicely with Wrapf.

@beoran

This comment has been minimized.

Copy link

commented Jun 19, 2019

I made a quick hack and the results are interesting:

https://gitlab.com/beoran/errd

So now I can handle an error with where closer has a Close method like this:
defer errd.New(&err).Add("closer %v", closer).Close(closer).Log().Do()
The Do() was neccesary to avoid having to wrap everything in a func(){} block. Perhaps even that could be avoided by chaining lambdas in stead of structs. But for the rest, the power is amazing. :)

The potential is limitless, there could even be .If(), .Switch(), so the error handling could get as complex as I would like it to be. But I guess not everyone likes method chaining, so maybe it's better that this remains an alternate package, much like how we have std lib log and logrus.

@rsc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

@jba, I would expect everything in errors to be about errors and not about error pointers. Putting the deferrables in their own package lets them have a different name space. I think that solves one of the big naming problems we had with clunkers like fmt.HandleErrorf.

@rsc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

@cespare, I agree that Apply doesn't carry its weight for func literals. It might if people write other (named) helper functions.

@rsc

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

@johanbrandhorst, Yes, errd is short for err-defer. If these are going to be used a lot, it makes sense for them to have a short name (like fmt.Printf not format.Printf). And if the package were errors/deferred then the uses would look like:

defer deferred.Trace(&err)

which is a bit repetitive.

@cespare

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

@cespare, I agree that Apply doesn't carry its weight for func literals. It might if people write other (named) helper functions.

It only helps insofar is it allows such helpers have the signature func(error) error rather than func(*error) (and skip the nil check). And the call site is a little more clunky:

defer errd.Apply(&err, handle)
// vs
defer handle(&err)

So even then it seems like a wash. I'd rather establish the convention that custom error helpers have the signature func(*error, ... other args ...), just like Trace/Add/Wrap.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

The name Trace suggests that it is reasonable to call it as you return errors up the stack, but that would lead to a lot of duplicate stack traces. Like @jba I suspect that Trace ought to report just the current function, and a different function, perhaps Stack, would dump the full stack trace. I don't know if we need both.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

defer+err = deferr, obviously.

Of course, that stutters (defer deferr.Trace(...)), so the obvious fix is to allow zero-width spaces and call this package r: defer​r.Trace(...).

@tv42

This comment has been minimized.

Copy link

commented Jun 19, 2019

I read the name as "erred", as in to err --- someone erred, now it's time to deal with consequences.

@mmcloughlin

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

I am generally in favor of this proposal, but share concerns about the name. That said, it's not an easy package to name! I'm wondering about handle or errhandle?

defer handle.Trace(&err)
defer handle.Add(&err, "copy %s %s", src, dst)
defer handle.Wrap(&err, "copy %s %s", src, dst)
@beoran

This comment has been minimized.

Copy link

commented Jun 20, 2019

I see several people dislike my experiment. I have a feeling why this might be so, although I'd prefer to hear why. After all, if the idea of doing error handing using defer() is now to become the preferred way in Go, then people will start using libraries like the one I wrote, probably even more powerful and convoluted, because of the convenience gained, despite the loss of readability. I myself had never considered to do error handling in defer, but now that I know this clever hack, I can see the appeal of it. But I'm not sure myself that this way is better than just an if statement. :/

@jba

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

I'd prefer to hear why.

I'm actually fond of fluent APIs myself, but I don't think this one is necessary. In the relatively rare cases where you need multiple error handlers, you can just write multiple defers. That is simpler, and there is no danger of forgetting to call Do.

Another problem is that the only way to add another action is to modify your package.

@freeformz

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

Thanks!

I'd like to see this as part of errors. errors.Trace(), errors.Apply(), errors.Add(), & errors.Wrap() read much better than errd.Func()

@yiyus

This comment has been minimized.

Copy link

commented Jun 21, 2019

I'd rather establish the convention that custom error helpers have the signature func(*error, ... other args ...), just like Trace/Add/Wrap.

This would also make easier to add some simpler way to defer error handlers at a later stage. For example, try may have the signature (in pseudo-code):

func try(expr, handlerFunc, args ...) (T1, T2, … Tn)

with the added benefit that it would not be needed to name return values.

I think it is obvious enough how this would work (and this issue is not the right place to discuss it) but I think this is a nice example of why a more consistent definition is desirable, instead of having the choice between func(*error, args ...) and func(error) error.

@as

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

After all, if the idea of doing error handing using defer() is now to become the preferred way in Go

@beoran as someone who does not have the spare time to ramp up 100% on this new error handling stuff, the current error handling proposals are frankly, terrifying to me from a complexity standpoint if the above statement is accurate. Note that my perspective coming from that of a new user looking at this for the first time, maybe second time. I would be very surprised if other people did not share this opinion under the premise that they did not want to aimlessly re-iterate this concern, especially without concrete feedback and reasoning for their claims.

I thought your experiment was useful, even though I did not agree with it, because it was able to provide a discussion point of what could happen once other users started to adopt this idiom.

Food for thought: my initial reaction to this is simply the question of how this is better than exception handling as a whole. In addition to looking very complex, it also looks unfamiliar.

@guonaihong

This comment was marked as off-topic.

Copy link

commented Jun 27, 2019

I am happy to see improvements to the code below..

if err != nil {
     return err
}

Surprisingly, seeing a niche language (zig) also uses try to reduce the if err != nil code.
As shown below, see the full screen try, do you still feel good?

var args_it = process.args();
    const exe = try unwrapArg(args_it.next(allocator).?);
    var catted_anything = false;
    var stdout_file = try io.getStdOut();

    while (args_it.next(allocator)) |arg_or_err| {
        const arg = try unwrapArg(arg_or_err);
        if (mem.eql(u8, arg, "-")) {
            catted_anything = true;
            var stdin_file = try io.getStdIn();
            try cat_file(&stdout_file, &stdin_file);
        } else if (arg[0] == '-') {
            return usage(exe);
        } else {
            var file = File.openRead(arg) catch |err| {
                warn("Unable to open file: {}\n", @errorName(err));
                return err;
            };
            defer file.close();

            catted_anything = true;
            try cat_file(&stdout_file, &file);
        }
    }
    if (!catted_anything) {
        var stdin_file = try io.getStdIn();
        try cat_file(&stdout_file, &stdin_file);
    }

I am more than 10 hours per day using go, try joining will make golang not look good before, can you change words or symbols? If I join try, I only want to use golang for 5 hours a day.I really don't want to see the full screen try

@cespare

This comment was marked as resolved.

Copy link
Contributor

commented Jun 27, 2019

@guonaihong I think you meant to put your comment on #32437, not this issue. I've marked your comment as off-topic.

@guonaihong

This comment has been minimized.

Copy link

commented Jun 27, 2019

@guonaihong I think you meant to put your comment on #32437, not this issue. I've marked your comment as off-topic.

thanks for your reminder.
#32437 The state is locked.

@mpvl

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

This package (or another) could also add a handler for closing, like Close(errp *error, io.Closer) to do proper error handling when closing a writer or even to solve the "panic problem" for CloseWithError.

The point is, error handling like this may be a bit odd, but it allows us to solve a few subtle gotchas in an idiomatic and straightforward way.

@beoran

This comment has been minimized.

Copy link

commented Jun 27, 2019

@mpvl, yes I already added a .Close(closer) method to my errd experimental package, and it works well. Ok, maybe we don' t need a fluid interface in this case, but a closer is useful, as experience shows.

@balasanjay

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

As someone pointed out on another issue, try can also be used for per-callsite error-wrapping like so:

d, err := someFn()
try(errors.Wrap(err, "some msg"))

To support this use-case, maybe these functions should return an error value.
And maybe this package shouldn't be named something specific to defers.

@Groxx

This comment has been minimized.

Copy link

commented Jul 1, 2019

I'll chime in with greatly preferring try(f(), handler). Handlers like this are clearly scoped to the single call being tried, a defer ... that manipulates err is not:

func() (err error) {
  defer errd.Apply(&err, func(err error) error {
    // this check will apply to errs from both fn1, and fn2's potentially-applied-err,
    // and any new code that appears below it in the future
    if err == ... {
      err = ...
    }
  })
  _, err = try(fn1())

  defer errd.Apply(&err, func(err error) error {
    // currently only applies to fn2's err
    if err == ... {
      err = ...
    }
  })
  _, err = try(fn2())
}

It seems much safer and easier to me to pass the same fn to try(..., handler) than to require some kind of "only apply this defer-err-thing to the immediate err, not all" logic.
TBH I'm not even sure what that kind of defer would look like, especially for it to be a safe pattern for multiple locations / nested errs (e.g. from a try-defer'd func call's result). It needs to avoid wrapping "itself" while simultaneously wrapping "itself" if the wrapped err came from a fn call rather than just an earlier defer in the same scope. Certainly it's possible, but likely annoying / strange looking.

@velovix

This comment has been minimized.

Copy link

commented Jul 11, 2019

In the Go 2 Error Values proposal (#29934), one of the most common complaints I saw was the lack of an explicit Wrap function, which couldn't be added because errors cannot import fmt. This proposal introduces that function in a new package that can import fmt, albeit by using an error pointer instead of returning the wrapped error. This may make #29934 more palatable to those who were concerned about the absence of Wrap. In general, these three proposals seem to fit together very nicely.

@gertcuykens

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2019

I was hoping it also returned the error so you can use it in a plain return also?

func test() error {
    file, err := os.Open("test.txt")
    if err != nil {
        return errd.Wrap(&err, "Doh!")
    }
    defer file.Close()
}
@mvndaai

This comment has been minimized.

Copy link

commented Jul 18, 2019

This should probably be split into two distinct packages errors/decorate and errors/handle.

Add, Apply, Wrap, and the gathering the stack trace part of Trace decorate an error. If those are called multiple times in a function tree it is okay.

The part of Trace that "prints the error" actually handles the error. Errors should only be handled once. If you handle it more than once it makes it look like there are more errors than there actually are. Handling should happen at the top of a tree, usually in main or before returning from a defer/go function.

My biggest concern is that print the error is making a choice on how an error should be handled. Is that log.Println, fmt.Println, or any myriad of ways to print things? That choice should not be made by packages, see #33162.

@velovix

This comment has been minimized.

Copy link

commented Jul 19, 2019

What is the fate of this proposal now that #32437 has been declined? It is of course still possible to use defer to handle errors even without try, but the value of doing so may need to be re-evaluated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.