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: As crashes if nil error is passed #30970

Closed
agnivade opened this issue Mar 21, 2019 · 6 comments
Closed

errors: As crashes if nil error is passed #30970

agnivade opened this issue Mar 21, 2019 · 6 comments

Comments

@agnivade
Copy link
Contributor

@agnivade agnivade commented Mar 21, 2019

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

$ go version
go version devel +e96c4ace9c Mon Mar 18 10:50:57 2019 +0530 linux/amd64

What did you do?

package main

import (
	"errors"
)

type ErrNoUser struct {
	err error
}

func (e ErrNoUser) Error() string {
	return e.err.Error()
}

func (e ErrNoUser) Unwrap() error {
	return e.err
}

func main() {
	err := doSth()
	ce := ErrNoUser{}
	errors.As(err, &ce)
}

func doSth() error {
	return nil
}

What did you expect to see?

No output or an intentional error

What did you see instead?

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x45487d]

goroutine 1 [running]:
errors.As(0x0, 0x0, 0x465440, 0xc000010020, 0xc00001e118)
	/home/agniva/play/gosource/go/src/errors/wrap.go:93 +0x23d
main.main()
	/home/agniva/play/go/src/osexpand.go:22 +0x50
exit status 2

Note that this does not happen with errors.Is. Something like this works fine:

ce := ErrNoUser{}
errors.Is(err, ce)

Possibly due to this https://github.com/golang/go/blob/master/src/errors/wrap.go#L55

/cc @mpvl @jba

@agnivade agnivade added the NeedsFix label Mar 21, 2019
@agnivade agnivade added this to the Go1.13 milestone Mar 21, 2019
@jba
Copy link
Contributor

@jba jba commented Mar 21, 2019

@neild

Should errors.As(nil, ...) panic or return false? Returning false seems reasonable to me.

Either way, we should state the behavior in the docstring.

@ahsun-ahmed
Copy link
Contributor

@ahsun-ahmed ahsun-ahmed commented Mar 21, 2019

returned false if nil error is passed. Pushed the changes to Gerrit for code review.

@jimmyfrasche
Copy link
Member

@jimmyfrasche jimmyfrasche commented Mar 21, 2019

Logically, if there is no error, then it cannot be a particular error, so false makes sense.

Practically, I might want to write something like

err := f()
if errors.As(err, &something) {
  // handle special case of a something error
}
// otherwise return err whether or not it is nil
return err

so false is useful.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 21, 2019

Change https://golang.org/cl/168598 mentions this issue: errors: Fixes #30970 invalid memory address or nil pointer dereference runtime error. This change returns false if nil error is passed

@neild
Copy link
Contributor

@neild neild commented Mar 21, 2019

Returning false for a nil error seems reasonable to me.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 9, 2019

Change https://golang.org/cl/170960 mentions this issue: xerrors: fix crash in As

gopherbot pushed a commit that referenced this issue Apr 10, 2019
Fixes #30970

Change-Id: I333676b55a2364e329fffeafca8fc57d45a0b84b
Reviewed-on: https://go-review.googlesource.com/c/go/+/168598
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
Run-TryBot: Marcel van Lohuizen <mpvl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Apr 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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