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, cmd/vet: too easy to pass a pointer-to-pointer to `errors.As` when it should be a pointer-to-value #34091

Open
hartzell opened this issue Sep 4, 2019 · 9 comments

Comments

@hartzell
Copy link

commented Sep 4, 2019

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

This comment has been minimized.

Copy link
Author

commented Sep 4, 2019

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

@hartzell

This comment has been minimized.

Copy link
Author

commented Sep 4, 2019

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

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

trying to call Error() on it gives:

se2.Error() = Operation not permitted

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

var errors = [...]string{
1: "operation not permitted",

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

errors.As definitely works with syscall.Errno, because I used it within the go command to address some bugs on Windows:

var errno syscall.Errno
if errors.As(err, &errno) {
switch errno {

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

@bcmills bcmills changed the title Can't use errors.As with syscall.Errno in go1.13 errors, cmd/vet: too easy to pass a pointer-to-pointer to `errors.As` when it should be a pointer-to-value Sep 5, 2019

@bcmills bcmills added this to the Go1.14 milestone Sep 5, 2019

@bcmills

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

@tmthrgd

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

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

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

@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

This comment has been minimized.

Copy link
Author

commented Sep 5, 2019

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

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

@neild

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.