proposal: cmpopts: add convenience functions for errors

[dsnet]

I've been frustrated lately with the number of tests that do error checking by performing string equality on the error message. As it stands, cmp is better than reflect.DeepEqual in that it usually fails when comparing errors since many errors have an unexported field somewhere in it forcing users to think about how to properly compare them. I believe cmp (or rather cmpopts) can go one further and assist users with comparing errors.

Consider the following:

// EquateErrors returns a Comparer option that determines errors to be equal
// if both are nil or both are non-nil. When both errors are non-nil,
// the Comparer checks whether either error is a CheckError and if so,
// calls it on the other error to determine equality.
// If neither error are a CheckError, then they are compared as if they
// are sentinel errors (i.e., using the == operator).
func EquateErrors() cmp.Option {
	return cmp.Comparer(func(e1, e2 error) bool {
		if e1 == nil || e2 == nil {
			return e1 == nil && e2 == nil
		}
		f1, _ := e1.(CheckError)
		f2, _ := e2.(CheckError)
		switch {
		case f1 == nil && f2 == nil:
			return e1 == e2
		case f1 != nil && f2 == nil:
			return f1(e2)
		case f1 == nil && f2 != nil:
			return f2(e1)
		default:
			return false
		}
	})
}

// CheckError is an error used by EquateErrors for finer comparisons on errors.
type CheckError func(error) bool

func (c CheckError) Error() string {
	return fmt.Sprintf("%#v", c)
}

It seem odd at first that CheckError is both a function and also an error, but it gives flexibility in being able to control how comparisons with errors works, by configuring the comparison as data, rather than through an option.

Consider the following example usage:

func Test(t *testing.T) {
	// nonNilError equates any non-nil error.
	// This is usual for just checking that the result failed, but not how it failed.
	nonNilError := CheckError(func(error) bool {
		return true
	})

	// notExistError equates any error based on a function predicate.
	notExistError := CheckError(os.IsNotExist)

	// timeoutError equates any error that is a timeout error.
	timeoutError := CheckError(func(e error) bool {
		ne, ok := e.(net.Error)
		return ok && ne.Timeout()
	})

	// containsEOF equates any error with EOF in the string.
	// NOTE: string matching on error messages is heavily frowned upon.
	containsEOF := CheckError(func(e error) bool {
		return strings.Contains(e.Error(), "EOF")
	})

	tests := []struct {
		err1 error
		err2 error
		want bool
	}{
		{io.EOF, io.EOF, true},
		{io.EOF, errors.New("EOF"), false},
		{io.EOF, io.ErrUnexpectedEOF, false},

		{io.EOF, nonNilError, true},
		{nonNilError, io.EOF, true},
		{nil, nonNilError, false},

		{io.EOF, notExistError, false},
		{os.ErrNotExist, notExistError, true},
		{os.ErrPermission, notExistError, false},

		{timeoutError, io.EOF, false},
		{timeoutError, &net.AddrError{}, false},
		{timeoutError, syscall.ETIMEDOUT, true},

		{io.EOF, containsEOF, true},
		{io.ErrUnexpectedEOF, containsEOF, true},
		{&net.ParseError{}, containsEOF, false},
	}

	for _, tt := range tests {
		got := cmp.Equal(&tt.err1, &tt.err2, EquateErrors())
		if got != tt.want {
			t.Errorf("Equal(%v, %v) = %v, want %v", tt.err1, tt.err2, got, tt.want)
		}
	}
}

Thus, EquateErrors has the following properties:

• It is still symmetric, which is one of the required properties of cmp.Comparer. That is, the result the same regardless of whether CheckError is on the left or right side.
• By default, it compares errors as if they are sentinel error.
• By default, it does not perform string comparisons on error messages, but does not prevent it either.
• It is extensible to handle all other error types through the use of a predicate function, which handles the other two common ways to distinguish errors (which are type assertions to an interface or type, or by using a predicate function like os.IsNotExist).

Related to #24

\cc @neild

[dsnet]

\cc @gnahckire @shurcooL (from #24)

[dmitshur]

@dsnet What you've described sounds quite positive and has nice properties.

I just want to point out, at least with the example given, I don't see why cmp itself is needed to achieve that functionality. The code above is very readable, but to write it from scratch, you have to know in advance that CheckError relates to EquateErrors(). Without the latter, CheckError won't do the right thing.

I could see the code above simplified to just:

type CheckError func(error) bool

func Test(t *testing.T) {
	// nonNilError equates any non-nil error.
	// This is usual for just checking that the result failed, but not how it failed.
	nonNilError := CheckError(func(error) bool {
		return true
	})

	// notExistError equates any error based on a function predicate.
	notExistError := CheckError(os.IsNotExist)

	// timeoutError equates any error that is a timeout error.
	timeoutError := CheckError(func(e error) bool {
		ne, ok := e.(net.Error)
		return ok && ne.Timeout()
	})

	// containsEOF equates any error with EOF in the string.
	// NOTE: string matching on error messages is heavily frowned upon.
	containsEOF := CheckError(func(e error) bool {
		return strings.Contains(e.Error(), "EOF")
	})

	tests := []struct {
		err   error
		check CheckError
		want  bool
	}{
		{io.EOF, nonNilError, true},
		{nil, nonNilError, false},

		{io.EOF, notExistError, false},
		{os.ErrNotExist, notExistError, true},
		{os.ErrPermission, notExistError, false},

		{io.EOF, timeoutError, false},
		{&net.AddrError{}, timeoutError, false},
		{syscall.ETIMEDOUT, timeoutError, true},

		{io.EOF, containsEOF, true},
		{io.ErrUnexpectedEOF, containsEOF, true},
		{&net.ParseError{}, containsEOF, false},
	}

	for _, tt := range tests {
		got := tt.check(tt.err)
		if got != tt.want {
			t.Errorf("check(%v) = %v, want %v", tt.err, got, tt.want)
		}
	}
}

Perhaps the mechanism you've described is useful when deep nested structs are being compared with cmp.Equal, and one wants to compare errors in that context. But for the simple case of checking errors, it feels like using cmp is not justified.

I hope this feedback is helpful, let me know if not. I might be missing some context.

[neild]

Instead of an EquateErrors function, can we have a general facility for smart comparisons of interface values?

• When comparing values a and b
• If a or b has a CmpEqual(x T) bool method
• Where b or a is assignable to T
• The comparison succeeds if CmpEqual returns true.

Now CheckError is:

type CheckError func(err error) bool

func (c CheckError) Error() string {
	return fmt.Sprintf("%#v", c)
}

func (c CheckError) CmpEqual(err error) bool {
	return c(err)
}

This general approach can then be applied to other cases where we may want to embed comparison information in the object being compared.

[dsnet]

On hold for golang/go#29934

Presumably we would add:

func EquateErrors() cmp.Option {
    return cmp.Comparerer(func (x, y error) bool {
        return errors.Is(x, y) || errors.Is(y, x) 
    })
}

[mccurdyc]

@dsnet is this still on hold?

Wasn't errors.Is in the most recent Go release? Looks like it is part of errors now

I wouldn't mind picking this one up.