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

errors: Is(nil) behaves unexpectedly #40442

Closed
carnott-snap opened this issue Jul 28, 2020 · 6 comments
Closed

errors: Is(nil) behaves unexpectedly #40442

carnott-snap opened this issue Jul 28, 2020 · 6 comments
Milestone

Comments

@carnott-snap
Copy link

@carnott-snap carnott-snap commented Jul 28, 2020

What version of Go are you using (go version)?

$ go version
go version go1.14.6 linux/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOOS="linux"
GOPATH="/home/user/go"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/user/.local/share/umake/go/go-lang"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/user/.local/share/umake/go/go-lang/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build313466002=/tmp/go-build -gno-record-gcc-switches"

What did you do?

While it is suggested that one use errors.Is(err, ErrSentinel) over err == ErrSentinel, I am unclear if this translates to errors.Is(err, nil). Assuming it does, and because errors.Is(err, ErrSentinel) == (err == ErrSentinel), I would expect that a custom Is method, but not Unwrap method since nil is the signal for this is the base error, could signal that a non-nil error interface is not an error: (https://play.golang.org/p/BsSRGGzpxZL)

package main

import (
	"errors"
	"fmt"
)

type err struct{}

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

func (err) Is(error) bool { return true }

func main() {
	fmt.Println(errors.Is(error(nil), nil), errors.Is(err{}, nil))
}

While this example is intentionally contrived for simplicity, I have a use case where I would like to provide consistent debugging information from my custom error, but also allow categories to evaluate to nil.

What did you expect to see?

true true

What did you see instead?

true false

What about Unwrap?

Clearly, func (err) Unwrap() error { return nil } cannot imply errors.Is(err{}, nil) == true, however I think it is reasonable for fmt.Errorf("doing a thing: %w", nil) to. We could also expose errors.Nil as:

package errors

var Nil = success{}

type success struct{}

func (success) Error() string { return "" }
func (success) Is(err error) bool { return err == nil }
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 30, 2020

@neild
Copy link
Contributor

@neild neild commented Jul 30, 2020

Perhaps the documentation around the semantics of errors.Is(err, nil) could be improved, but I don't think we want to introduce the concept of a non-nil error which matches nil. The concept that error conditions may be tested with err != nil is a long standing one which we very much want to preserve; for errors.Is(err, nil) to ever return true when err != nil would work against that.

@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jul 30, 2020

I guess I am wanting a single, simple way to do error handling. I get that people love the if err != nil { mantra, and I do not want to break that for existing apis, but I think it is confusing to say that nilness should be checked by calling != and equality via errors.Is.

Plus, we can finally solve the nil error problem if we push people to use !errors.Is(err, nil) and document the expected behaviour:

var err error = (*myError)(nil)
switch {
case err == nil:
        // false
case errors.Is(err, nil):
        // could be true
case errors.Is(err, (*otherError)(nil):
        // not totally sure about this one
}
@cagedmantis cagedmantis added this to the Backlog milestone Jul 30, 2020
@carnott-snap
Copy link
Author

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

Just for clarity, can I get a decree on if errors.Is(err, nil) is an anti-pattern? Also, are there concrete things that need to be investigated, or more that this problem requires further discussion?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 5, 2020

Today errors.Is(err, nil) is equivalent to err == nil.

Personally I think that changing that would just introduce more complexity and confusion to a somewhat area that is already complex and confusing. So I don't think we should change it.

Which is pretty much what @neild said above.

So I don't think there is anything to do here.

@carnott-snap
Copy link
Author

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

Since this is a bug, and we have identified that everything is working as expected. I think we can close this out. Personally, I will transition to exclusively using errors.Is(err, nil) for consistency, and it would be nice for the community to advocate that as cannon, but I understand that may not be popular.

That being said, I am interested in proposing the two discussed changes: see #40673, #40674, #40675.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
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.