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 should allow target to be a nil pointer to a concrete type #31541

Closed
bcmills opened this issue Apr 18, 2019 · 7 comments
Closed

errors: As should allow target to be a nil pointer to a concrete type #31541

bcmills opened this issue Apr 18, 2019 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Apr 18, 2019

What did you do?

example.com$ cat >./main.go <<EOF
package main

import (
        "errors"
        "fmt"
        "net"
)

func main() {
        const addr = "••••••••"
        conn, err := net.Dial("tcp", addr)
        if err != nil {
                if errors.As(err, (*net.InvalidAddrError)(nil)) {
                        fmt.Println("invalid address: %s")
                }
        }
        defer conn.Close()
}
EOF

example.com$ cat >./go.mod <<EOF
module example.com

go 1.12
EOF

example.com$ go version
go version devel +33fdc10a30 Wed Apr 17 13:37:58 2019 -0400 linux/amd64

example.com$ go run main.go

What did you expect to see?

errors.As being useful for type-checks even if I don't want to allocate space to store the result.

For this specific example:

invalid address: ••••••••

What did you see instead?

panic: errors: target must be a non-nil pointer

goroutine 1 [running]:
errors.As(0x54de60, 0xc000098000, 0x507020, 0x0, 0x0)
        /usr/local/google/home/bcmills/go/src/errors/wrap.go:86 +0x5eb
main.main()
        /tmp/tmp.8hp5hhWBDQ/example.com/main.go:13 +0xd5
exit status 2

This is the documented behavior of errors.As, but it seems strictly less useful than allowing a nil pointer of a concrete type.

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 18, 2019
@bcmills bcmills added this to the Go1.13 milestone Apr 18, 2019
@bcmills
Copy link
Contributor Author

bcmills commented Apr 18, 2019

(See previously #30970, which relaxed a somewhat related constraint on err.)

CC @neild @jba @mpvl

@jba
Copy link
Contributor

jba commented May 24, 2019

I don't see a problem with this.

@adg
Copy link
Contributor

adg commented May 31, 2019

An error matches a type ... if it has a method As(interface{}) bool such that As(target) returns true.

So what should the As method of the error do if target is nil? Should all As method implementation check for nil pointers? Wouldn't this place a burden on all interface implementors?

--

An aside: the As function is currently documented:

An error matches a type if it is assignable to the target type,

Should that be "assignable through"? It only matches if the given error (or an unwrapped error) can be assigned to the "element" value of target. This seems like it needs some clarification.

@gopherbot
Copy link

Change https://golang.org/cl/179978 mentions this issue: errors: As target can be a typed nil pointer

@jba
Copy link
Contributor

jba commented Jun 1, 2019

Should all As method implementation check for nil pointers? Wouldn't this place a burden on all interface implementors?

That's true.

Should that be "assignable through"?

In the same CL linked above, I reworded that part of the doc. PTAL.

@rsc
Copy link
Contributor

rsc commented Jun 6, 2019

I think this API change is a mistake, for the reasons adg gives above.
It's a minor performance optimization that will cause problems in every As method implementation. It doens't balance out.

@jba
Copy link
Contributor

jba commented Jun 6, 2019

Abandoned the CL.

@jba jba closed this as completed Jun 6, 2019
@golang golang locked and limited conversation to collaborators Jun 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants