errors, cmd/vet: too easy to pass a pointer-to-pointer to `errors.As` when it should be a pointer-to-value

[hartzell]

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

pi@raspberrypi:~/temper/go/blort $ go version
go version go1.13 linux/arm
pi@raspberrypi:~/temper/go/blort $

Does this issue reproduce with the latest release?

I am using the latest release.

What operating system and processor architecture are you using (go env)?

go env Output

pi@raspberrypi:~/temper/go/blort $ go env
GO111MODULE=""
GOARCH="arm"
GOBIN=""
GOCACHE="/home/pi/.cache/go-build"
GOENV="/home/pi/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="arm"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/pi/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_arm"
GCCGO="gccgo"
GOARM="6"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/pi/temper/go/blort/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -marm -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build073091018=/tmp/go-build -gno-record-gcc-switches"
pi@raspberrypi:~/temper/go/blort $

What did you do?

I've been exploring the new errors.As() and wasn't able to get it to recognize a wrapped syscall.Errno. I've distilled a simpler case. Here is a playground link

package main

import (
    "fmt"
    "syscall"
    "errors"
    "os"
    "reflect"
)

func main() {
    e1 := &os.PathError{Op: "read", Path: "/", Err: errors.New("YIKES")}
    var pe *os.PathError
    if errors.As(e1, &pe) {
        fmt.Println("Found an os.PathError")
    }

    e2 := syscall.Errno(1)
    var se *syscall.Errno
    fmt.Println("Type of e2 is: ", reflect.TypeOf(e2))
    if errors.As(e2, &se) {
        fmt.Println("Found a syscall.Errno")
    }
}

What did you expect to see?

Found an os.PathError
Type of e2 is:  syscall.Errno
Found a syscall.Errno

What did you see instead?

Found an os.PathError
Type of e2 is:  syscall.Errno

[hartzell]

Ps: this comment on 29054 suggests that this should work (or something like it).

[hartzell]

After a bit of navel gazing, providing errors.As with the address of a syscall.Error, not a *syscall.Error works. E.g.

    e3 := syscall.Errno(1)
    var se2 syscall.Errno
    fmt.Println("Type of e3 is: ", reflect.TypeOf(e3))
    if errors.As(e3, &se2) {
        fmt.Println("Found a syscall.Errno in e3")
    }

but trying to call Error() on it gives:

se2.Error() = Operation not permitted

I've updated the playground.

[bcmills]

trying to call Error() on it gives:

se2.Error() = Operation not permitted

Yes, that's because errno 1 is literally operation not permitted: https://github.com/golang/go/blob/50bd1c4d4eb4fac8ddeb5f063c099daccfb71b26/src/syscall/zerrors_linux_amd64.go#L1381-L1382

[bcmills]

errors.As definitely works with syscall.Errno, because I used it within the go command to address some bugs on Windows: https://github.com/golang/go/blob/aae0b5b0b26bf4fd26cad0111535d703691a9083/src/cmd/go/internal/robustio/robustio_windows.go#L95-L97

That said, I think there should be a vet diagnostic of some sort for passing a pointer-to-pointer vs. pointer-to-value.

[bcmills]

CC @jba @neild @ianthehat

[tmthrgd]

There already seems to be a check for exactly this:

https://github.com/golang/tools/blob/0abef6e9ecb84abbb13f782a96b8ce932107c545/go/analysis/passes/errorsas/errorsas.go#L54

The problem here is that syscall.Errno implements error (Errno.Error) as a value receiver not a pointer receiver. Because go automatically derives pointer receiver methods for value receiver methods, it means that both syscall.Errno and *syscall.Errno implement the error interface. That means a pointer to both of those is valid when passed to errors.As.

This playground snippet should highlight this: https://play.golang.org/p/Gzf3XV_RXEY.

The errorsas vet check could be expanded to explicitly check for *syscall.Errno, but there's nothing generic that can be safely checked here. It's perfectly valid to define a value receiver and yet use a pointer for an error.

[bcmills]

@tmthrgd, a more precise generic check would presumably flag the use of errors.As with a pointer-to-pointer whose inner element type also implements the error interface.

The question then is, are there any value types that implement error but are nonetheless conventionally passed by pointer?

[hartzell]

Yes, that's because errono 1 is literally operation not permitted:

Sigh. Thanks for clearing that bit up....

[neild]

The same problem exists when using type assertions:

if _, ok := err.(*syscall.Errno); ok {
  fmt.Println("Found a *syscall.Errno")
}
if _, ok := err.(syscall.Errno); ok {
  fmt.Println("Found a syscall.Errno")
}

One of these type assertions is clearly wrong, but the compiler can't tell you which. This is a good reason to always define Error methods on a pointer receiver.

I think that if the errors.As vet check were to be expanded to catch this case, then it should only be done alongside additional checks to prohibit assigning a *T to an error where a T will do.