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: 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
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.13

Comments

@agnivade
Copy link
Contributor

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 The path to resolution is known, but the work has not been done. label Mar 21, 2019
@agnivade agnivade added this to the Go1.13 milestone Mar 21, 2019
@jba
Copy link
Contributor

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

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

@jimmyfrasche
Copy link
Member

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

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 commented Mar 21, 2019

Returning false for a nil error seems reasonable to me.

@gopherbot
Copy link

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
@ahsun-ahmed @mpvl
errors: return false if nil error is passed to As
fda5e6d 
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>
@at15 at15 mentioned this issue Apr 13, 2019
Closed
2 tasks
@bcmills bcmills mentioned this issue Apr 18, 2019
Closed
@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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
6 participants
@neild @jimmyfrasche @agnivade @gopherbot @ahsun-ahmed @jba