error-is-as: *error argument should be considered valid

[orestisfl]

Description

I think the check for the require.ErrorAs arguments has a false-positive when passing an argument of type *error. require.ErrorAs is a wrapper for errors.As which mentions in its documentation:

// As panics if target is not a non-nil pointer to either a type that implements
// error, or to any interface type.

So, pointers to errors should be valid arguments for require.ErrorAs.

MWE

Original

Consider this test code:

package main

import (
	"encoding/json"
	"testing"

	"github.com/stretchr/testify/require"
)

func TestJsonSyntaxError(t *testing.T) {
	err := json.Unmarshal([]byte("bla bla"), &struct{}{})
	wrap(t, err, &json.SyntaxError{})
}

func wrap(t *testing.T, err error, target error) {
	require.ErrorAs(t, err, &target)
}

Which works fine:

$ go test ./...
ok  	example.com/mwe	(cached)

When running as:

$ testifylint mwe_test.go
/workdir/mwe_test.go:16:2: error-is-as: second argument to require.ErrorAs should not be *error

No pointer

When removing the pointer from wrap():

func wrap(t *testing.T, err error, target error) {
	require.ErrorAs(t, err, target)
}

testifylint correctly detects the problem:

$ testifylint mwe_test.go
/workdir/mwe_test.go:16:2: error-is-as: second argument to require.ErrorAs must be a non-nil pointer to either a type that implements error, or to any interface type

and the test panics:

$ go test ./...
--- FAIL: TestJsonSyntaxError (0.00s)
panic: errors: *target must be interface or implement error [recovered]
	panic: errors: *target must be interface or implement error

goroutine 7 [running]:
testing.tRunner.func1.2({0x5db960, 0x684410})
	/usr/local/go/src/testing/testing.go:1632 +0x230
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1635 +0x35e
panic({0x5db960?, 0x684410?})
	/usr/local/go/src/runtime/panic.go:785 +0x132
errors.As({0x6857a0, 0xc000010138}, {0x5e6040, 0xc000010150})
	/usr/local/go/src/errors/wrap.go:111 +0x1e9
github.com/stretchr/testify/assert.ErrorAs({0x7632b7000880, 0xc0001249c0}, {0x6857a0, 0xc000010138}, {0x5e6040, 0xc000010150}, {0x0, 0x0, 0x0})
	/go/pkg/mod/github.com/stretchr/[email protected]/assert/assertions.go:2081 +0xb3
github.com/stretchr/testify/require.ErrorAs({0x686150, 0xc0001249c0}, {0x6857a0, 0xc000010138}, {0x5e6040, 0xc000010150}, {0x0, 0x0, 0x0})
	/go/pkg/mod/github.com/stretchr/[email protected]/require/require.go:302 +0xe5
example.com/mwe.wrap(...)
	/workdir/mwe_test.go:16
example.com/mwe.TestJsonSyntaxError(0xc0001249c0)
	/workdir/mwe_test.go:12 +0x9a
testing.tRunner(0xc0001249c0, 0x64b4b8)
	/usr/local/go/src/testing/testing.go:1690 +0xf4
created by testing.(*T).Run in goroutine 1
	/usr/local/go/src/testing/testing.go:1743 +0x390
FAIL	example.com/mwe	0.067s
FAIL

Pointer to error as argument

As a last attempt, passing **json.SyntaxError as an argument to wrap() like so:

func TestJsonSyntaxError(t *testing.T) {
	err := json.Unmarshal([]byte("bla bla"), &struct{}{})
	wrap(t, err, new(*json.SyntaxError))
}

func wrap(t *testing.T, err error, target error) {
	require.ErrorAs(t, err, target)
}

also does not work because:

$ go test ./...
# example.com/mwe [example.com/mwe.test]
./mwe_test.go:12:15: cannot use new(*json.SyntaxError) (value of type **json.SyntaxError) as error value in argument to wrap: **json.SyntaxError does not implement error (missing method Error)
FAIL	example.com/mwe [build failed]
FAIL

Workaround

Using target any makes both testifylint and go test pass:

func TestJsonSyntaxError(t *testing.T) {
	err := json.Unmarshal([]byte("bla bla"), &struct{}{})
	wrap(t, err, new(*json.SyntaxError))
}

func wrap(t *testing.T, err error, target any) {
	require.ErrorAs(t, err, target)
}

Note

I know that the original

func wrap(t *testing.T, err error, target error) {
	require.ErrorAs(t, err, &target)
}

will not save the target error for its caller, it can still have valid usecases though.

[orestisfl]

Nevermind, now I get it, wrap() will always pass, that's the problem testifylint is screaming about. Example:

func TestJson(t *testing.T) {
	err := json.Unmarshal([]byte("bla bla"), &struct{}{})

	target := new(json.MarshalerError)
	wrap(t, err, target)             // passes
	require.ErrorAs(t, err, &target) // fails
}

func wrap(t *testing.T, err error, target error) {
	require.ErrorAs(t, err, &target)
}

[Antonboom]

@orestisfl, hi!

This is common gotcha not related to testify, but related to errors.As,
because any error is assignable to any error (link):

if reflectlite.TypeOf(err).AssignableTo(targetType) {

testifylint repeats go vet's erroras check logic in this case:

The errorsas analysis reports calls to errors.As where the type
of the second argument is not a pointer to a type implementing error.