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
kevinburke1 opened this issue Nov 5, 2019 · 6 comments
Closed

blog: unintuitive code in errors post example #35361

kevinburke1 opened this issue Nov 5, 2019 · 6 comments
Labels
Documentation FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@kevinburke1
Copy link

kevinburke1 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.

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

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 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.

@FiloSottile FiloSottile added Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 5, 2019
@FiloSottile FiloSottile added this to the Unreleased milestone Nov 5, 2019
@antong
Copy link
Contributor

antong commented Apr 16, 2020

Related to #37625 .

@antong
Copy link
Contributor

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

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

@gopherbot
Copy link

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

gopherbot pushed a commit to golang/website that referenced this issue May 26, 2021
Since this is a very high SEO article and it's a copy-pasteable block, it gets
copy pasted without much thought, which leads to problems when the error is a
concrete type / interface / etc.

This CL adds a small note that users should watch out for the case when the
error type is not a pointer.

Fixes golang/go#35361

Change-Id: Ibbd950c2a73a5f30cdab3517e042f69465adff97
Reviewed-on: https://go-review.googlesource.com/c/blog/+/252877
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Kevin Burke <kev@inburke.com>
Trust: Jean de Klerk <deklerk@google.com>
Trust: Damien Neil <dneil@google.com>
X-Blog-Commit: 989b9fc7a2626e387b19079b084798a016de1018
@golang golang locked and limited conversation to collaborators Oct 20, 2021
passionSeven added a commit to passionSeven/website that referenced this issue Oct 18, 2022
Since this is a very high SEO article and it's a copy-pasteable block, it gets
copy pasted without much thought, which leads to problems when the error is a
concrete type / interface / etc.

This CL adds a small note that users should watch out for the case when the
error type is not a pointer.

Fixes golang/go#35361

Change-Id: Ibbd950c2a73a5f30cdab3517e042f69465adff97
Reviewed-on: https://go-review.googlesource.com/c/blog/+/252877
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Kevin Burke <kev@inburke.com>
Trust: Jean de Klerk <deklerk@google.com>
Trust: Damien Neil <dneil@google.com>
X-Blog-Commit: 989b9fc7a2626e387b19079b084798a016de1018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation 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