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

blog: unintuitive code in errors post example #35361

Closed
kevinburkemeter opened this issue Nov 5, 2019 · 6 comments
Closed

blog: unintuitive code in errors post example #35361

kevinburkemeter opened this issue Nov 5, 2019 · 6 comments

Comments

@kevinburkemeter
Copy link

@kevinburkemeter kevinburkemeter commented Nov 5, 2019

I am reading about how to use the new Go errors code. In this blog post, there's the following example:

// The As function tests whether an error is a specific type.

// Similar to:
//   if e, ok := err.(*QueryError); ok { … }
var e *QueryError
if errors.As(err, &e) {
    // err is a *QueryError, and e is set to the error's value
}

My initial read was that this can't be right - it's taking the address of a *QueryError, which gives a **QueryError, which a) does not match the type of the "Similar to" immediately above it, and b) a pointer to a pointer is almost never what you want in Go.

However I wrote some tests and yeah a **QueryError is actually what we want here:

type QueryError struct{}

func (e *QueryError) Error() string {
	return "an error occurred"
}

func TestErrorsAs(t *testing.T) {
	q1 := new(QueryError)
	subErr := fmt.Errorf("a sub error occurred: %w", q1)
	var q2 *QueryError
	fmt.Printf("%#v\n", &q2)
	if errors.As(subErr, &q2) {
		fmt.Println("subErr unwraps to q2")
	} else {
		t.Fatal("errors.As comparison failure")
	}
}

The blog post doesn't really mention this weirdness at all though.

I presume some folks like me are going to out of instinct remove either the * or the & in order to get one layer of nesting and then be confused when that's not correct. It might be good to add a note that, yes, this is the correct thing to do.

@kevinburkemeter kevinburkemeter changed the title blog: unintuitive code in example blog: unintuitive code in errors post example Nov 5, 2019
@kevinburkemeter
Copy link
Author

@kevinburkemeter kevinburkemeter commented Nov 5, 2019

Also, sorry, I'm sure the pros and cons of this design were covered in the spec and that there are good reasons for doing it this way, I am not saying it's a bad design but we could do a better job of communicating it to folks.

@cespare
Copy link
Contributor

@cespare cespare commented Nov 5, 2019

FWIW this surprised me at first too.

I had to think about it for a bit to realize why it makes sense this way. (My rough reasoning is something like: we already have a *QueryError, so declaring var e *QueryError and then changing e to the existing *QueryError is analogous to the type assertion. Plus, this way we don't make a copy of the underlying value.)

I think the reason it feels weird at first is that the code looks similar to a common pattern used in JSON unmarshaling and similar contexts, where you might well do (supposing the QueryError could unmarshal itself from JSON):

var e QueryError
if err := json.Unmarshal(b, &e); err != nil { ... }

In that case, there was no QueryError to begin with, so we need to create one and fill it in; in the errors.As context, we already have the value and we just need to get the pointer to it again.

@antong
Copy link
Contributor

@antong antong commented Apr 16, 2020

Related to #37625 .

@antong
Copy link
Contributor

@antong antong commented Apr 17, 2020

The way I understood this is that the second argument to errors.As() (aptly named target), is an output argument. An output argument needs to be passed a pointer to the target, because you want the function to assign to it. And the type of the output (error value) is *QueryError, so you clearly need to pass a pointer to a *QueryError. I think one problem in the blog post is that the full function signature wasn't shown (func As(err error, target interface{}) bool). If you see that the argument is named target, it makes more sense.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 3, 2020

Change https://golang.org/cl/252825 mentions this issue: content: explain double-pointer in errors.As example

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 3, 2020

Change https://golang.org/cl/252877 mentions this issue: x/blog: add non-pointer note for errors.As example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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