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: add Errors as a standard way to represent multiple errors as a single error #47811

Closed
marksalpeter opened this issue Aug 19, 2021 · 44 comments
Labels
Projects
Milestone

Comments

@marksalpeter
Copy link

@marksalpeter marksalpeter commented Aug 19, 2021

Description

It is common practice in golang to return an error from a func in the event of a problem during its execution (e.g. func myFunc() error { ... }).

However, there are a few cases, such as field validation on a struct, where it is useful to return multiple errors from a function (e.g. func validate(m *Model) []error). Sometimes these errors need to be 'bubbled up' through a call stack that only returns one error. So, I find myself having to re-implement custom errors that represent many errors in each of my projects.

I think a generic solution could be added to the standard libraries' errors package that serves this purpose.

Example Implementation

I've updated the example implementation to incorporate the feedback from @neild and @D1CED

package util

import (
	"fmt"
)

// Errors are many errors that can be represented as a single error
type Errors interface {
	Errors() []error
	error
}

// NewErrors combine many errors into a single error
func NewErrors(errs ...error) error {
	if lenErrs := len(errs); lenErrs == 0 {
		return nil
	} else if lenErrs == 1 {
		return errs[0]
	}
	var es manyErrors
	for _, err := range errs {
		// Merge many slices of errors into a single slice
		if errs, ok := err.(Errors); ok {
			es = append(es, errs.Errors()...)
			continue
		}
		es = append(es, err)
	}
	return es
}

// manyErrors combines many errors into a single error
type manyErrors []error

// Is works with errors.Is
func (es errors) Is(find error) bool {
        for _, e := range es {
                if Is(e, find) {
                        return true
                }
        }
        return false
}       

// As works with errors.As
func (es errors) As(find interface{}) bool {
        for _, e := range es {
                if As(e, find) {
                        return true
                }
        }
        return false
}

// Errors implements the Errors interface
func (es manyErrors) Errors() []error {
	return []error(es)
}

// Error implements the error interface
func (es manyErrors) Error() string {
	return fmt.Sprintf("[%v", []error(es))
}

Example Usage

import "errors"

type Model struct {
	FieldOne   string `json:"field_one"`
	FieldTwo   string `json:"field_two"`
	FieldThree string `json:"field_three"`
}

func (m *Model) Validate() error {
	var errs []error
	if len(m.FieldOne) < 1 {
		errs = append(errs, errors.New("'field_one' is required"))
	}
	if len(m.FieldTwo) < 1 {
		errs = append(errs, errors.New("'field_two' is required"))
	}
	if len(m.FieldThree) < 1 {
		errs = append(errs, errors.New("'field_three' is required"))
	}
	return errors.NewErrors(errs...)
}

Conclusion

Whether or not this proposal is accepted, I'm curious how others have approached this problem in the past and if there's already a commonly referenced solution that I'm unfamiliar with. Thanks 🙏

@gopherbot gopherbot added this to the Proposal milestone Aug 19, 2021
@neild
Copy link
Contributor

@neild neild commented Aug 19, 2021

If err contains multiple errors, what does errors.Is(err, fs.ErrExist) return?

  • true if all errors in err are fs.ErrExist?
  • true if at least one error in err is fs.ErrExist?
  • always false?

If we use errors.As to convert a multierror value to a *os.PathError, does it:

  • return true if at least one error in err is an *os.PathError, and extract one of the *os.PathErrors?
  • return true if the only error in err is an *os.PathError?
  • always return false?

@ianlancetaylor ianlancetaylor changed the title proposal: errors.Errors - A standard way to represent multiple errors as a single error proposal: errors: add Errors as a standard way to represent multiple errors as a single error Aug 19, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Aug 19, 2021
@marksalpeter
Copy link
Author

@marksalpeter marksalpeter commented Aug 20, 2021

Thanks for the feedback @neild! Handling wrapped errors certainly complicates things.

Full disclosure: I don't generally use wrapped errors in production, so I would defer to the wisdom of those who do.

That said, here are my thoughts:

I think errors.Is can either remain as is (always false) or be modified to return true if at least one error in err is fs.ErrExists - whichever seems more useful or backwards compatible. If the former is deemed better, then it will be up to developers to use Contains instead of Is when they intend to check multiple errors. If the latter is deemed better, then we can combine the logic in Contains with the logic inside of Is and remove the Contains method altogether.

The errors.As implementation should match the behavior of errors.Is. So, if its decided that errors.Is should always return false, then so should errors.As. If it's decided that errors.Is should return true, then errors.As should return true and copy the first *os.PathError that it finds. In this case, the order of the errors in the slice returned by Errors.Errors() would be significant.

If all of the *os.PathErrors are required from the err, we could modify As to handle something like As(err, NewErrors(nilTypeOneErr, nilTypeTwoErr, nilTypeThreeErr)). Then, theoretically, we would be able to extract multiple errors (potentially of multiple types) from a single call to the As func without the addition of any other methods.

Which way would you prefer @neild, @D1CED?

@marksalpeter
Copy link
Author

@marksalpeter marksalpeter commented Aug 20, 2021

Thinking some more about your questions, maybe there's a simpler solution that wouldn't require modifying errors.Is or errors.As:

// Errors are many errors that can be represented as a single error
type Errors interface {
	Errors() []error
	error
}

// NewErrors combine many errors into a single error
func NewErrors(errs ...error) Errors {
	if len(errs) == 0 {
		return nil
	}
	var es errors
	for _, err := range errs {
		// Merge many slices of errors into a single slice
		if errs, ok := err.(Errors); ok {
			es = append(es, errs.Errors()...)
			continue
		}
		es = append(es, err)
	}
	return es
}

// errors combines many errors into a single error
type errors []error

// Unwrap works with errors.Unwrap
func (es errors) Unwrap() error {
	if len(es) > 0 {
		return es[1:]
	}
	return nil
}

// Is works with errors.Is
func (es errors) Is(err error) bool {
	if len(es) == 0 {
		return err != nil
	}
	return err.Error() == es[0].Error()
}

// Errors implements the Errors interface
func (es errors) Errors() []error {
	return []error(es)
}

// Error implements the error interface
func (es errors) Error() string {
	return fmt.Sprintf("%v", []error(es))
}

Thoughts @neild, @D1CED?

@D1CED
Copy link

@D1CED D1CED commented Aug 20, 2021

First let me start out by saying that there seems to be a clear need for this feature as seen by looking for 'multi error' on pkg.go.dev.
From the ones I skimmed through none had (documented) support for As or Is.
Providing the most information possible is still the right call here IMO. If we don't provide Is and As the users have to explicitly check for the Errors interface. One issue here you also targeted is that of performance. On a very large error slice Is and As would be (multiple) linear searches. Not great. A more efficient implementation here would be fairly difficult I assume.

@D1CED
Copy link

@D1CED D1CED commented Aug 20, 2021

@marksalpeter Your implementation of Is and Unwrap is incorrect here. For example if Errors only contains a single error Unwrap discards it instead of returning it unwrapped and Is does a string comparison on just the first error?!

@marksalpeter
Copy link
Author

@marksalpeter marksalpeter commented Aug 20, 2021

@D1CED My apologies. As I said before, I'm unfamiliar with the use case. My understanding of Unwrap is that it returns one frame down the error "call stack". So for example errors.Is would call err.Is and if that failed, it would call err.Unwrap until err.Is returns true. How is it supposed to be implemented?

@D1CED
Copy link

@D1CED D1CED commented Aug 20, 2021

Uh, I think my statement above was partially incorrect. Your implementation kind of does work when changing the comparison. But it seems like we have a very different idea here. My suggested implementation would be

package errors

func (es errors) Is(find error) bool {
        for _, e := range es {
                if Is(e, find) {
                        return true
                }
        }
        return false
}       

func (es errors) As(find interface{}) bool {
        for _, e := range es {
                if As(e, find) {
                        return true
                }
        }
        return false
}

and I'd not implement an Unwrap method at all.

Edit: Got a little confused with the names here. The package is called errors and type is called errors. Maybe we should call it multiError as this seems to be a common name.

@D1CED
Copy link

@D1CED D1CED commented Aug 20, 2021

One small feature I found when looking at other implementations is that if there is exactly one error it is simply returned unwrapped. This should be considered here too.

@marksalpeter
Copy link
Author

@marksalpeter marksalpeter commented Aug 20, 2021

@D1CED I'm not sure that recursively calling errors.Is or errors.As like this is necessary, wise or even correct. Keep in mind, errors.As will be calling errors.errors.As and errors.Is will be calling errors.errors.Is. I'll have to think about this some more.

@marksalpeter
Copy link
Author

@marksalpeter marksalpeter commented Aug 20, 2021

@D1CED You can see on line 90 of wrap.go that errors.errors.Unwrap will be called until errors.errors.As succeeds. So therefore, I think a more correct implementation of errors.errors.As would be

// As works with errors.As
func (es errors) As(target interface{}) bool {
	if len(es) == 0 {
		return false
	}
	err := es[0]
	val := reflectlite.ValueOf(target)
	targetType := val.Type()
	if reflectlite.TypeOf(err).AssignableTo(targetType) {
		val.Elem().Set(reflectlite.ValueOf(err))
		return true
	}
	return false
}

I also think the version of errors.errors.Is I wrote would pass any tests of errors.Is functionality you subjected it to.

@marksalpeter
Copy link
Author

@marksalpeter marksalpeter commented Aug 20, 2021

I've added the example implementations of As and Is to the original example in the proposal with a note at the top. Thanks again for pointing out this use case @neild 🙏

@marksalpeter
Copy link
Author

@marksalpeter marksalpeter commented Aug 20, 2021

Uh, I think my statement above was partially incorrect. Your implementation kind of does work when changing the comparison. But it seems like we have a very different idea here. My suggested implementation would be

package errors

func (es errors) Is(find error) bool {
        for _, e := range es {
                if Is(e, find) {
                        return true
                }
        }
        return false
}       

func (es errors) As(find interface{}) bool {
        for _, e := range es {
                if As(e, find) {
                        return true
                }
        }
        return false
}

and I'd not implement an Unwrap method at all.

Edit: Got a little confused with the names here. The package is called errors and type is called errors. Maybe we should call it multiError as this seems to be a common name.

Ok, so I traced this and the recursive call is definitely more correct than either of my previous attempts. Sorry about that 😅.

I still think we can use Uwrap to treat the []errors like any other stack of errors instead of looping over the slice in the func. I've included the changes in the example at the top. Let me know what you think @D1CED

@neild
Copy link
Contributor

@neild neild commented Aug 20, 2021

Which way would you prefer @neild, @D1CED?

I don't know. Any multiple-error type needs to come up with good answers for these questions, however.

Another question without an obviously correct universal answer is what the error string for a collection of errors should look like: Concatenate all the error strings with some separator? One line or multiple lines? Summarize in some fashion, such as deduplicating multiple instances of the same error text?

The fact that there didn't seem to be an obviously correct universal answer to these questions was a primary reason we didn't introduce a standard multierror type in Go 1.13. There are now a number of multiple-error types implemented outside the standard library; perhaps considering the experience of these implementations would point at some answers.

@marksalpeter
Copy link
Author

@marksalpeter marksalpeter commented Aug 20, 2021

Thanks for bringing other package implementations to my attention. It looks like hashicorp has a pretty good one that handles the Unwrap, As, and Is cases similarly.

Do you have a reference to the go 1.13 discussion around multierrors @neild? Maybe this will help me get up to speed.

As for formatting the errors, I’m inclined towards an approach with a global DefaultFormatter var and then anything else (de-duping, summarizing, fancy printing,etc) could be left to a custom Errors implementation. But maybe there are better ideas already in the discussion.

@neild
Copy link
Contributor

@neild neild commented Aug 20, 2021

Do you have a reference to the go 1.13 discussion around multierrors @neild?

Not any more, sorry.

The key points as I recall them were:

  • It is not obvious how multiple errors unwrap. (errors.Is, etc.)
  • It seems possible that different users have different requirements for unwrapping.
  • There is no single, obvious answer to what the error text of a multierror is.
  • There is no obstacle to implementing a multierror outside the standard library.

This all argued that we should avoid standardizing a multierror type at the time.

My personal experience has been that general-purpose error aggregating types push complexity to the users of an API that is better handled internally, and that it is usually best to either explicitly return an []error or an opaque error with useful error text describing the failures encountered and no way to access the individual errors.

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Aug 21, 2021

I have a multierror package: https://github.com/carlmjohnson/errutil. Mine is better than other people’s because it is compatible with both Hashicorp and Uber multierrors. :-) More seriously, I think the design space here is a little too ambiguous to fit into the standard library. There isn’t one right way to handle unwrapping.

@marksalpeter
Copy link
Author

@marksalpeter marksalpeter commented Aug 22, 2021

@neild, I think the use of an interface can sidestep the issues of specific formatting requirements and even incompatible unwrapping requirements, much like the error interface itself.

As for there being "no obstacle to implementing multierror outside of the standard library", this is also true of many standard library packages. I think the real question is whether or not this is a common enough use case to warrant a standard solution. I agree with @D1CED that the answer is a firm "yes". While its possible to pass a []error or create a custom error type on a per use case basis, it's clear that many developers prefer to use a multierror implementation instead.

@neild @carlmjohnson Just so I understand the problem set better, do either of you have an example of incompatible unwrapping requirements?

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Aug 22, 2021

Suppose you have error 1 which is a normal error and error 2 which fmt.Errorf("my prefix: %w", Multierror{err3, err4}). What happens if you combine error 1 and 2? Does it flatten the tree? If it doesn’t flatten the list, you get unexpected results when you try to iterate through all suberrors. But if it does flatten the list, you lose “my prefix” which might be important.

@marksalpeter
Copy link
Author

@marksalpeter marksalpeter commented Aug 23, 2021

Thanks for clarifying @carlmjohnson

If it doesn’t flatten the list, you get unexpected results when you try to iterate through all suberrors.

This is a good point and it kind of unpacks what @neild said in the beginning.

Imagine traversing an error stack like this:

err3 := fmt.Errorf("d %w",  errors.New("e"))
err2 := fmt.Errorf("b %w",  &customError{ "c" })
err1 := fmt.Errorf("a %w", errors.NewErrors(err2, err3))

// Traverse the error stack
for err := err1; err != nil;  err = errors.Unwrap(err1) {
  fmt.Println(err1)
}

Ether the output looks like this:

a [b c d e]
[b c d e]
[c d e]
[d e]
[e]

Or it looks like this:

a [b c d e]
[b c d e]

Maybe the second way is more clear. Multierrors are wrapped together, so they should be unwrapped together. Therefore, its better traverse them differently, using the Errors() method. Although doing it this way might mean calling errors.Is and errors.As on each sub-error, as @D1CED suggested.

The question then becomes, "Does errors.As(err1, &customErr) return true?". To me, the obvious answer is yes. So my questions to you, @neild, @carlmjohnson, @D1CED, are:

  1. Do some people prefer errors.As(err1, &customErr) to return false in this case?
  2. Are there example implementations that work like point 1?
  3. Also I think what some of you are alluding to in terms of the prefix and formatting questions are support for multierror inside of fmt.Errorf (eg fmt.Errorf("one: %w, two: %w, three:%w", err1, err2, err3)). This is opens up a can of worms. I see no reason why a scaled down proposal like this one requires multi error support for fmt.Errorf or if that's even possible given the constraints. Is this point where proposal got 'stuck'?

@jakobmaier
Copy link

@jakobmaier jakobmaier commented Aug 25, 2021

I used hashicorp for a while, but ran into the same problems as you already discovered:

What happens when appending/merging/flattening errors that are multi-errors? How to format errors? What about As/Is?

A lot of those questions need to be decided on a case-by-case basis (even within the same project), so in the end I wrote my own multierr implementation which I now use for all of my private- and work projects. It covers all those situations gracefully. Only the As/Is/Unwrap behavior can be controversial, which is why I implement Inspect().

Usually I prefer the Append() function, which does not flatten the tree. I only use Merge() (=flatten errors) when writing validation functions on smaller, nested structs.

So yes, this is a pretty common issue. But there's no universal solution, and already a few libraries. So I don't think this should be added to the standard library.

@rsc rsc moved this from Incoming to Active in Proposals Oct 13, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Oct 13, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

@rsc rsc commented Oct 20, 2021

Based on the discussion above, especially #47811 (comment), it sounds like there's no good single answer for this, so we should leave it to continue to be provided by packages outside the standard library.

@rsc rsc moved this from Active to Likely Decline in Proposals Oct 20, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Oct 20, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@mvdan
Copy link
Member

@mvdan mvdan commented Oct 21, 2021

My personal experience has been that general-purpose error aggregating types push complexity to the users of an API that is better handled internally, and that it is usually best to either explicitly return an []error or an opaque error with useful error text describing the failures encountered and no way to access the individual errors.

If that's going to be our position while rejecting a standard Errors type, I think we should make that advice more clear. For example, as a bit of godoc in the errors package.

I keep seeing Go developers copying the idea of a "multiple errors" type, from places like hashicorp's module or https://pkg.go.dev/go/scanner#ErrorList. I think the majority assume that using an opaque error or an []error slice is a worse idea, and right now there isn't any text advising them otherwise.

The alternative is that we're still considering adding an Errors type in the future. If that's the case, then I think we should leave an umbrella issue open about it, to clarify the intent.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 27, 2021

I don't think we have any intent to add a standard Errors type in the future (or else we would have discussed this proposal more and tried to refine it). Exactly what it means to have more than one error will vary by use case.

@rsc rsc moved this from Likely Decline to Declined in Proposals Oct 27, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Oct 27, 2021

No change in consensus, so declined.
— rsc for the proposal review group

@D1CED
Copy link

@D1CED D1CED commented Oct 29, 2021

For people interested: there is currently a proposal to add a similar concept to python PEP-654.

@marksalpeter
Copy link
Author

@marksalpeter marksalpeter commented Oct 29, 2021

I would just like to say to everyone involved that I really appreciate the lively discussion and consideration of this proposal. It uncovered a number of issues, use cases and concerns regarding the unwrapping and equality of errors that I was previously unaware of. And now I have a much deeper understanding of how error wrapping is being used in practice and all of the ways gophers are interested in potentially using a multierror.

I still think there's an opportunity to bring a more integrated, stripped back, proposal of an Errors interface to the standard library - one that side steps many of these concerns, but is perhaps less useful on its own. I'll be sure to link that proposal to this one once it's ready.

Again, a big thank you to everyone involved. This was, if nothing else, enlightening 🙏

@jba
Copy link
Contributor

@jba jba commented May 10, 2022

I'm sorry I missed this discussion, although at the time I would have probably agreed with the decision. Now I think there are clear answers to the questions posed here. Namely:

  • the Multierror type does not implement Unwrap (this is the key new idea)
  • errors.Is reports true for a MultiError if it is true for any of the contained errors
  • errors.As does the same; it traverses the contained errors left to right
  • An Errors method returns the flattened list of errors, where the flattening only occurs if the contained error is itself a Multierror. Since a Multierror is nothing but a collection of errors, no information is lost.

This matches Tailscale's design.

Are any of these controversial? It seems to me that once you get rid of Unwrap, these are all more or less obvious. I was always hung up on figuring out what Unwrap would mean, on the assumption that it would be used a lot. (People used to talk about errors "causes" all the time, meaning the last error in the chain.) My understanding now is that we were successful in promoting Is and As, so that it's rare to call Unwrap. If that's correct, then dropping it here will cause little pain.

Formatting is still an open question (here is one cute idea).

To be clear, I'm not asking to reopen this proposal yet, nor yet asking for formatting ideas. I'm conducting a poll: are the semantics I presented wrong for anyone? Because if not, then the main reason for rejection–there are too many ways to do this—is in question.

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented May 10, 2022

Interesting. I think one of the keys has to be that multierror(multierror) gets flattened but multierror(wrapper(multierror)) does not, so that a wrapper is never lost in the process of creating the multierror. Once you do that, one of the main objections I had to this proposal goes away.

@maja42
Copy link

@maja42 maja42 commented May 10, 2022

I don't like multierrors where flattening is done automatically/implicitly. I need to be in control of whether something gets flattened or nested, and also how such an error is printed.
https://pkg.go.dev/tailscale.com/util/multierr wouldn't fit my needs, and I'd end up continuing to use my own implementation.

But this is mainly an API question. Since I don't use unwrap, I'm fine if it's missing. Though it feels a bit odd that As() iterates the error left-to-right, but unwrap doesn't. https://github.com/maja42/multierr solves this and makes sure that As() and Unwrap() are consistent.

@jimmyfrasche
Copy link
Member

@jimmyfrasche jimmyfrasche commented May 10, 2022

I don't have the problem with flattening but it seems like it would be easier for that to be done it in the multierror factory

What is the meaning of an error list of length 0?

Is there a way to define this with an interface and helpers so there can be multiple multierrors that interoperate but format differently? Maybe something like:

type Multi interface {
  error
  Is(error) bool
  As(any) bool

  // bad name but disallows both Unwraps
  // being implemented on the same type
  Unwrap() []error
}

// return nil if not a Multi error?
func Errors(error) []error

Defining just that and punting on an implementation would give time for others to experiment and settle on best formatting practices.

However, I suppose fmt.Errorf with multiple %w patterns could be defined naturally as it already returns a different error type depending on the number of %w included.

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented May 10, 2022

What is the meaning of an error list of length 0?

Early versions of my multierror package defined a multierror as type Errors []error, but I changed it because of that problem, so now you cannot construct an error list of length zero from outside the package. The Tailscale package also has this design. I think if it's worth adding to the standard library, it's worth creating the constructor so that users can't accidentally construct an invalid state, as opposed to just defining the interface.

@jimmyfrasche
Copy link
Member

@jimmyfrasche jimmyfrasche commented May 10, 2022

Wouldn't it suffice to specify in the interface that you must return a nonzero length and have the helper panic if len == 0?

@neild
Copy link
Contributor

@neild neild commented May 12, 2022

I'm still of the opinion that just about every place I've seen a multierror used would have been better served by an explicit []error. However, this is a common request and there are a number of competing packages providing multierror implementations. If there's enough commonality between those implementations, that may argue for standardizing on a common interface.

My own modest proposal:

An error wraps multiple errors if its type has the method

Unwrap() []error

Reusing the name Unwrap is proposed by @jimmyfrasche above, and has the nice characteristic of avoiding ambiguity when an error has singular and multiple unwraps. It also avoids changing behavior, since I don't think any existing multierror implementations implement this method signature. Returning a 0-length list from Unwrap means the error doesn't wrap anything.

The errors.Split function provides a mechanism for retrieving the original errors:

// Split returns the result of calling the Unwrap method on err,
// if err's type contains an Unwrap method returning []error.
// Otherwise, Split returns nil.
func Split(err error) []error

The errors.Join function provides a simple implementation of a multierr. It does not flatten errors.

// Join returns an error that wraps the given errors.
// The error formats as the text of the given errors, separated by sep.
// Join returns nil if len(errs) is 0.
func Join(sep string, errs ...error) error

The errors.Is and errors.As functions are updated to unwrap multiple errors. We replace the term "error chain" with "error tree". Is reports a match if any error in the tree matches. As finds the first error in a preorder traversal of the tree that matches.

This proposal:

  • Codifies a representation of a tree of errors.
  • Places the responsibility for tree traversal in errors.Is/errors.As rather than leaving it up to multierror implementations to implement their own Is/As methods. This standardizes the tree traversal, and follows the model used for wrapping singular errors.
  • Leaves the question of formatting multiple errors largely up to the user.
  • Allows for third-party implementations that flatten errors, provide alternate formatting, etc.

@jimmyfrasche
Copy link
Member

@jimmyfrasche jimmyfrasche commented May 12, 2022

fmt.Errorf should be updated to take multiple %w and when there's more than one return a type with Unwrap() []error

@jba
Copy link
Contributor

@jba jba commented May 13, 2022

The discrepancy between the Unwrap method and the Split function is unfortunate. We could call them both Unwraps.

Join should elide nil errors, returning nil if they're all nil.

To be precise, the traversal of As should be specified as preorder left-to-right.

@maja42, I think you could use this to simplify your implementation. By removing Unwrap, you also can get rid of the allocation and copying you need in order to make a chain.

@maja42
Copy link

@maja42 maja42 commented May 14, 2022

Unwrap, similar to Is and As has a predefined/well-understood meaning when implemented in an error-type. So reusing that name, albeit with a different signature, can be unexpected/confusing to developers.
In my library, I implemented the Inspect method which does exactly what you suggest: returning the underlying list of errors (or nil if there are no errors).

It's also not possible to construct an empty multierr, or a multierr containing nil-errors. (It will always turn out to be nil itself). This is a nice feature that prevents questions like "what if the error-list is empty".

My goal was that a multierr behaves exactly like a normal error, and the library itself also doesn't care what kind of error you feed it in it's various methods. In most cases (especially for validation purposes) a caller of a function doesn't care if the returned error is a single error or a multi error. So existing code can start using multi-errors instead (and producing more complete error messages for the user) without the fear of breaking anything, be it any method signatures/APIs, or code that unwraps/inspects the underlying causes.

In fact, my Inspect() method is not a member-method but a global function, also taking a single error argument and not caring if it's actually a multierr or a conventional error (which will result in a slice of length 1).

A lot of thoughts went into it (this is actually the 4th iteration of the library), and after years of using it in production I'm pretty happy with it - at least for the projects I'm working on. (There's a minor inconvenience that i might fix in the future though).

So if go started with it's own multi-error standard/package, there is a high chance that instead of replacing my library, I instead add compatibility with the go standard - completely defeating its purpose. (Under the assumption that the standard library will have less features/functionality than mine). And if other library authors think the same ... this proposal should indeed be kept close.

Go could define an interface/minimum set of functions a multierr must implement to get some common ground - but considering that methods should continue to return error (and not any other interface-type), this doesn't sound very useful/discoverable.

@jba
Copy link
Contributor

@jba jba commented May 16, 2022

@maja42, thanks for your detailed comparison.

Say we changed the name of the new Unwrap function to something else (Unwraps is my suggestion, but maybe it could be Inspect). That would also be the name of the global function. And say errors.Join function returned nil if the list was empty, and dropped nil errors.

Those seem to be the two properties of this proposal that don't match your package.

I still think you'd want your own package, because it lets you control formatting. But what if you just deleted the Is, As and Unwrap methods, and added an Unwraps instead? You could also probably delete your Inspect and use errors.Unwraps instead. Actually you couldn't, because you return []error{err} in the single-error case and the proposal's function returns nil.

@jba
Copy link
Contributor

@jba jba commented May 16, 2022

@neild, one problem with Unwrap() []error is that the implementor must either copy, or risk exposing their internal state to mischief. The latter is probably the right choice but I think we should add "don't modify the slice" to the method doc.

@maja42
Copy link

@maja42 maja42 commented May 16, 2022

Ah, I understand.
So the global "Unwrap() []error" would actually replace my Inspect().

How would the error-string look like, after constructing a multierr with Join?
As long as it doesn't automatically flatten errors, this looks fine. Maybe a "JoinFlat" (or similar) also makes sense, but I guess could be added later.

@jba
Copy link
Contributor

@jba jba commented May 16, 2022

After errors.Join(";", err1, err2), the result would print like this: fmt.Sprintf("%s; %s", err1, err2).

But the nice thing about the proposal is that you don't have to use errors.Join if you don't want to. The important part is the Unwraps method and function and the changes to Is and As, allowing multierrors of all kinds to behave the same.

@josharian
Copy link
Contributor

@josharian josharian commented May 16, 2022

Filed #52936 as a different way to think about printing multierrs.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 17, 2022

Another new proposal at #53435.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Proposals
Declined
Development

No branches or pull requests