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 resolve nil error problem #40674

Closed
carnott-snap opened this issue Aug 10, 2020 · 12 comments
Closed

proposal: errors: Is resolve nil error problem #40674

carnott-snap opened this issue Aug 10, 2020 · 12 comments
Labels
Projects
Milestone

Comments

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Aug 10, 2020

background

A long standing problem with error handling, but really checking nilness of interface variables, is typed nils:

var p *MyError = nil
if bad() {
	p = ErrBad
}
return p // Will always return a non-nil error.

description

Since we already introduced a function than handles error comparisons, errors.Is, we could resolve this edge case by checking both err == nil and underlying value nilness:

var err error = (*MyError)(nil)
switch {
case err == nil:
        // false
case errors.Is(err, nil):
        // true
}

costs

This will make errors.Is more complex, and meant that developers should eschew err != nil in favour of !errors.Is(err, nil). At the same time, we already suggest errors.Is(err, ErrBad) over err == ErrBad, so this would serve to make the language more consistent.

alternatives

It may also be confusing that this only applies for nilable types. We could instead define that all default value types are nil:

type MyError struct{}

func (MyError) Error() string { return "" }

func main() {
	_ = errors.Is(MyError{}, nil) // == true
}

Since every implementer may want to handle the nil case differently, we could instead upon the implementation of interface{ Is(error) bool } to determine what should be done. This is a larger topic so see #40673. That being said, for errors that are implemented upon types, e.g. func (MyError) Error() string, we may need to have a standard resolution for (*MyError)(nil). Furthermore, if #40673 is accepted, we should allow it to override any defaults suggested here.

Since we are flattening nilness in the err parameter, should we allow the same in the target, errors.Is(err, (*MyError)(nil))?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 10, 2020

CC @neild @jba

Confusion about whether storing a nil pointer value into an interface variable gives you a non-nil interface arises most frequently with the error type, but it arises elsewhere as well. It's not clear to me that a fix that only applies to error values is a great idea.

Also, given that the language is careful to say that a nil pointer in an interface variable is != nil, I think that changing errors.Is(err, nil) to return true for a nil pointer would only add to the confusion.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Aug 10, 2020
@neild
Copy link
Contributor

@neild neild commented Aug 10, 2020

I agree with @ianlancetaylor that changing this would only add to the confusion.

It is a very deliberate choice that err == nil is equivalent to errors.Is(err, nil). I do not believe we should change that property.

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Aug 11, 2020

Confusion about whether storing a nil pointer value into an interface variable gives you a non-nil interface arises most frequently with the error type, but it arises elsewhere as well. It's not clear to me that a fix that only applies to error values is a great idea.

Agreed, though I like the idea of trying to clean up this thorn for new users, as we see it internally every now and then. Plus it tends to be weirdly hard to debug and hunt down in my experience.

After some additional thought, an intuitive way to implement this would be to map target == nil to the default value for any type: e.g. errors.Is(MyError{}, nil) == true. That being said, I do recognise the confusion, and we would need to clearly signal that nil is a sentinel, not an equality check. I would also be interested to get feedback from novice users before saying it is an improvement.

Also, given that the language is careful to say that a nil pointer in an interface variable is != nil, I think that changing errors.Is(err, nil) to return true for a nil pointer would only add to the confusion.

Correct, we would need to document that target == nil is a sentinel for empty errors, not a literal err == nil equality check.

@rsc
Copy link
Contributor

@rsc rsc commented Aug 11, 2020

I can't see what the difference is between this issue and #40673.

40673 is: proposal: errors: allow implementing err.Is(nil) == true
40675 is: proposal: errors: allow implementing err.Is(err) == false

It's clear to me how those two are different. But isn't this one the same as 40673?

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Aug 11, 2020

No, they are different. Though it may be better to think of them as complementary. This one only intends to solve the nil error problem called out in the faq:

type MyError struct{}

func (*MyError) Error() string { return "" }

func main() {
        fmt.Println(errors.Is((*MyError)(nil), nil))
        // Output: true
}

During the discussion, it was pointed out that only using the default value of nillable things (pointers, maps, slices, channels) was confusing, so the option to extend this to all default value types was added:

type ErrorString string
type ErrorStruct struct{}
type ErrorArray [0]struct{}

func main() {
        fmt.Println(
                errors.Is(ErrorString(""), nil)
                errors.Is(ErrorStruct{}, nil)
                errors.Is(ErrorArray{}, nil)
        )
        // Output: true, true, true
}
@rsc
Copy link
Contributor

@rsc rsc commented Aug 12, 2020

I've read this multiple times and am still not completely sure what you are proposing here.
It seems like you must be saying that errors.Is itself should be changed
such that errors.Is((*T)(nil), nil) == true for any pointer type *T.

Do I have that right?

(If errors.Is deferred to the implementation of *T, then that would be #40673.)

@rsc
Copy link
Contributor

@rsc rsc commented Aug 12, 2020

Based on the conversation above, this seems like a likely decline.
The weight of history weighs far too much against breaking all existing Go code in the world for a cleanup.

@rsc rsc moved this from Incoming to Likely Decline in Proposals Aug 12, 2020
@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Aug 12, 2020

Yes, that is correct. There are different flavours (just nil for pointers, nil maps too, any default value), but you have the gist. (This does not refer to the implementation of interface { Is(error) bool } on any type T*, you are correct that is #40673.)

This is not a breaking change.

@rsc
Copy link
Contributor

@rsc rsc commented Aug 14, 2020

Telling everyone that all their code using err == nil is now incorrect and must be updated to use errors.Is(err, nil) is a breaking change. We can't rewrite all the code in the world.

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Aug 14, 2020

While I agree we cannot mandate rewriting all the code, I had assumed that we were already doing that with errors.Is, apologies for my naivete on the topic.

That being said, as long as old code continues to compile (and run), it is not a breaking change to suggest that an old syntax, err == nil, is now an antipattern. See the introduction of context.Context, existing code lacking context is now deemed incorrect, and even the standard library has deprecated old interfaces. I think it is folly to suggest that this type of change is impossible, but am understanding if you think the clean up is not justified.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 14, 2020

Declaring that err == nil is an anti-pattern is tantamount to declaring that the Go standard library, millions of lines of Go code, and all existing Go books, articles, blog posts, and docs in general are using an anti-pattern. We simply aren't going to do that.

Let's move on.

@rsc
Copy link
Contributor

@rsc rsc commented Aug 26, 2020

No change in consensus, so declined.

@rsc rsc closed this Aug 26, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

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