go: cmd/vet: warn when errors.As target has type *error

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

$ go version
go version go1.16.6 darwin/arm64

Does this issue reproduce with the latest release?

yes

What did you do?

Wrote this code https://play.golang.org/p/16cU0kc8Lku and wondered why the errors matched.

var Err = errors.New("sentinel")
err := errors.New("foo")
if errors.As(err, &Err) {
  fmt.Println("why?")
}

Discussion in https://groups.google.com/g/golang-nuts/c/MaYJy_IRbYA/m/uCHV6P87EQAJ?utm_medium=email&utm_source=footer

What did you expect to see?

Of course the code above is wrong, yet I didn’t spot it. It would be nice if this kind of programming error could be detected:

add a govet check that flags using plain errors with errors.As or
@merovius suggested errors.errorString should implement As() and return false, unless the pointers match.

I’m unsure if the latter is possible. It would strictly break BC (and the function’s description). On the other hand ist should only change behaviour in cases that seem invalid from the start?

About this issue

Original URL
State: closed
Created 3 years ago
Reactions: 4
Comments: 22 (20 by maintainers)

Commits related to this issue

go/analysis/passes/errorsas: update testdata for new warning A new testdata file was added since CL 339889 was authored. Update it in the same way for the new warning. Updates golang/go#47528. Fixes... — committed to golang/tools by dmitshur 2 years ago

Most upvoted comments

No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group

rsc on Aug 18, 2021

Perhaps cmd/vet should be changed to warn when the second argument to errors.As is statically known to have type *error?

That is pretty much guaranteed to indicate a bug, because it is trivially true that the first argument (of type error) “is assignable to the value pointed to by” a second argument of type *error.

bcmills on Aug 4, 2021

Change https://go.dev/cl/403034 mentions this issue: go/analysis/passes/errorsas: update testdata for new warning

gopherbot on Apr 29, 2022

Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group

rsc on Aug 11, 2021

Warning on *interface{} doesn’t seem like it would have any false positives, but I suspect it’s unlikely to show up in practice.

neild on Aug 4, 2021

Would it make sense to apply the same warning to pointers to empty interface types, too? They are trivial in essentially the same way, in that they cause errors.As to decay to a nil-check.

bcmills on Aug 4, 2021

But the complete paragraph is

An error matches target if the error’s concrete value is assignable to the value pointed to by target, or if the error has a method As(interface{}) bool such that As(target) returns true. In the latter case, the As method is responsible for setting target.

The existence of the As optional interface is specifically for cases like this, so that error types can overwrite the pure “assignability” behavior.

Merovius on Aug 4, 2021