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

Closed
pjebs opened this issue May 20, 2020 · 9 comments

Comments

@pjebs
Copy link
Contributor

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
@pjebs
Copy link
Contributor Author

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 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

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 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 (old) Jun 10, 2020
@rsc rsc moved this from Incoming to Active in Proposals (old) Jul 21, 2021
@rsc
Copy link
Contributor

rsc commented Jul 21, 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

@urandom2
Copy link
Contributor

urandom2 commented Jul 21, 2021

this feels like a duplicate of #40673? (or maybe #40674)

@kortschak
Copy link
Contributor

The essence of this behaviour can be achieved by defining an empty error type/value that the error collection can compare itself to in the Is call. Alternatively (and probably preferably), the error collection can be created with a NewErrorCollection(errs []error) error function that returns nil if len(errs) == 0 at the end of the region that generates the collection of errors.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Jul 28, 2021
@rsc
Copy link
Contributor

rsc commented Jul 28, 2021

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

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Aug 4, 2021
@rsc
Copy link
Contributor

rsc commented Aug 4, 2021

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

@rsc rsc closed this as completed Aug 4, 2021
@golang golang locked and limited conversation to collaborators Aug 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

6 participants