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: Is(err, target) should allow err to decide even when target is nil #39167

Open
pjebs opened this issue May 20, 2020 · 4 comments
Open
Labels
Projects
Milestone

Comments

@pjebs
Copy link
Contributor

@pjebs pjebs commented May 20, 2020

Currently Is looks like this:

func Is(err, target error) bool {
	if target == nil {
		return err == target     // <-------------
	}
	isComparable := reflectlite.TypeOf(target).Comparable()
	for {
		if isComparable && err == target {
			return true
		}
		if x, ok := err.(interface{ Is(error) bool }); ok && x.Is(target) {
			return true
		}
		if err = Unwrap(err); err == nil {
			return false
		}
	}
}

See:
https://golang.org/src/errors/wrap.go?s=1170:1201#L29 and also
https://github.com/golang/xerrors/blob/master/wrap.go#L50

@gopherbot gopherbot added this to the Proposal milestone May 20, 2020
@gopherbot gopherbot added the Proposal label May 20, 2020
@pjebs
Copy link
Contributor Author

@pjebs pjebs commented May 20, 2020

See: https://github.com/rocketlaunchr/dataframe-go/blob/master/error_collection.go#L11
Sometimes error objects are designed as containers of/for multiple errors.

type ErrorCollection struct {
	errors []error
}

The proposal suggests that errors.Is(err, target) should be adjusted such that when target is nil and err is for example an ErrorCollection, the ErrorCollection should be able to decide whether it is nil or not via implementing Is(err error) bool {.
See: https://github.com/rocketlaunchr/dataframe-go/blob/master/error_collection.go#L62

It is feasible that when the error collection holds no errors, it would like to allow the user to write:

	ec := NewErrorCollection()
	isNil := errors.Is(ec, nil) 

where isNil is set to true (since ec currently holds no errors).

But due to the implementation of errors.Is, this will return false.

@pjebs
Copy link
Contributor Author

@pjebs pjebs commented May 20, 2020

The documetation currently states:

An error is considered to match a target if it is equal to that target or if
it implements a method Is(error) bool such that Is(target) returns true.

Due to the phrasing, the current proposal is still consistent with the documentation and arguably should not be deemed to breach Go1 backward compatibility guarantees (as it could be interpreted as a bug).

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 20, 2020

The predeclared type error is an interface type. It is only nil if it is completely unset. Giving a type like ErrorCollection a way to decide whether it is nil is going to run straight into https://golang.org/doc/faq#nil_error. I think it would be a mistake to make the handling of a nil interface any more complex.

@rsc
Copy link
Contributor

@rsc rsc commented Jun 10, 2020

Absolutely. It would be very unfortunate if we permitted err != nil && errors.Is(err, nil) == true.

@rsc rsc added this to Incoming in Proposals Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.