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

Jump to bottom

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
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@carnott-snap
Copy link

carnott-snap commented Jul 28, 2020 •
edited

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

CC @jba @neild

@neild
Copy link
Contributor

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 commented Jul 30, 2020 •
edited

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
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 30, 2020
@carnott-snap
Copy link
Author

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

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 commented Aug 6, 2020 •
edited

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.

@neild neild mentioned this issue Aug 10, 2020
Closed
@golang golang locked and limited conversation to collaborators Aug 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@neild @cagedmantis @ianlancetaylor @gopherbot @carnott-snap