errors: Is(nil) behaves unexpectedly

[carnott-snap]

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]

CC @jba @neild

[neild]

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]

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
}

[carnott-snap]

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]

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]

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.