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

Support multierr.Every #66

Closed
kdeberk opened this issue Nov 21, 2022 · 3 comments
Closed

Support multierr.Every #66

kdeberk opened this issue Nov 21, 2022 · 3 comments

Comments

@kdeberk
Copy link

kdeberk commented Nov 21, 2022

Hello,

There seems to be a correctness issue the implementation of multiError.Is. I say "seems" because I cannot find any consensus on what errors.Is(err, ErrIgnorable) should actually be doing when err is a multiError containing both ErrIgnorable and an important error (that you do want to handle).

Perhaps Is should only return true if all contained errors match the target, so instead

func (merr *multiError) Is(target error) bool {
        if len(merr.Errors()) == 0 {
                return false
        }
	for _, err := range merr.Errors() {
		if !errors.Is(err, target) {
			return false
		}
	}
	return true
}

an alternative would be that Is is not implemented at all, and then errors.Is should return false.

Again, I don't know if the golang developers ever specified some semantics for errors.Is, but I think that from both an intuitive and practical sense, errors.Is(err, ErrIgnorable) should return false. Intuitive, because a multiError is not equal to a single error, and practical because if the important error is not represented by a value, e.g. val ErrImportant = errors.New(...), then there is no way to easily ignore only ignorable error without calling Errors() and iterating over the individual errors..

@tchung1118
Copy link

We agree that this would be a good convenience feature to add to multierr, but we're still considering if this should work with errors.Is or should be provided as a separate function from this package.

@abhinav
Copy link
Collaborator

abhinav commented Mar 14, 2023

FWIW, the standard library's errors.Join matches multierr's behavior.
errors.Is will return true if any of the errors in a errors.Join-ed error matches.
Despite the name, errors.Is is not an equality check but an approximate match.

Perhaps we need an "all" variant of Is (multierr.AllAre or multierr.Every?), where every error in the multierr list gets an errors.Is call individually, but not at the top-level of the multierr.

@r-hang r-hang changed the title Correctness issue with Is Support multierr.Every Mar 28, 2023
@r-hang
Copy link

r-hang commented Mar 28, 2023

Internal ref: GO-1932

JacobOaks added a commit to JacobOaks/multierr that referenced this issue Mar 28, 2023
This PR introduces a new exported function in the multierr package
called `Every`, which checks every error within a given error
against a given target using `errors.Is`, and only returns true if
all checks return true.

```go
err := multierr.Combine(ErrIgnorable, errors.New("great sadness"))
fmt.Println(multierr.Every(err, ErrIgnorable))
// output = "false"

err := multierr.Combine(ErrIgnorable, ErrIgnorable)
fmt.Println(multierr.Every(err, ErrIgnorable)
// output = "true"
```

This also works when the error passed in as the first argument is not a `multiErr`,
including when it is a go1.20+ error that implements `Unwrap() []error`.

This solves uber-go#66.
JacobOaks added a commit that referenced this issue Mar 28, 2023
This PR introduces a new exported function in the multierr package
called `Every`, which checks every error within a given error against a
given target using `errors.Is`, and only returns true if all checks
return true.

```go
err := multierr.Combine(ErrIgnorable, errors.New("great sadness"))
fmt.Println(errors.Is(err, ErrIgnorable))      // output = "true"
fmt.Println(multierr.Every(err, ErrIgnorable)) // output = "false"

err := multierr.Combine(ErrIgnorable, ErrIgnorable)
fmt.Println(errors.Is(err, ErrIgnorable))      // output = "true"
fmt.Println(multierr.Every(err, ErrIgnorable)) // output = "true"
```

This also works when the error passed in as the first argument is not a
`multiErr`, including when it is a go1.20+ error that implements
`Unwrap() []error`.

This solves #66.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants