chore: Add tests documenting error invariants (#62992)

Based on the [discussion in Slack](https://sourcegraph.slack.com/archives/C02UC4WUX1Q/p1717067410264679),
 thought it would be useful to solidify the invariants that hold
between errors.As, errors.Is and errors.HasType
(and provide counter-examples for things which you think
ought to hold but don't actually hold) in code itself, instead of 
just via docs. So this PR adds property-based tests for
quickly generating different kinds of error shapes, and check
which invariants hold and which ones don't.

It also adds better documentation for errors.Is and errors.As

deps.bzl

@@ -7958,6 +7958,13 @@ def go_dependencies():
         sum = "h1:7uVkIFmeBqHfdjD+gZwtXXI+RODJ2Wc4O7MPEh/QiW4=",
         version = "v1.3.0",
     )
+    go_repository(
+        name = "net_pgregory_rapid",
+        build_file_proto_mode = "disable_global",
+        importpath = "pgregory.net/rapid",
+        sum = "h1:CMa0sjHSru3puNx+J0MIAuiiEV4N0qj8/cMWGBBCsjw=",
+        version = "v1.1.0",
+    )
     go_repository(
         name = "net_starlark_go",
         build_file_proto_mode = "disable_global",

go.sum

@@ -2702,6 +2702,8 @@ oss.terrastruct.com/d2 v0.6.5 h1:VgZgiwtMhh3uVR2mm7e0bdh25f1px3ZCPM/la5GKfMc=
 oss.terrastruct.com/d2 v0.6.5/go.mod h1:WUTwQN18MM0MWbDgFo7pmm2ousV6N6jUwg8MgVdT6I0=
 oss.terrastruct.com/util-go v0.0.0-20231101220827-55b3812542c2 h1:n6y6RoZCgZDchN4gLGlzNRO1Jdf9xOGGqohDBph5BG8=
 oss.terrastruct.com/util-go v0.0.0-20231101220827-55b3812542c2/go.mod h1:eMWv0sOtD9T2RUl90DLWfuShZCYp4NrsqNpI8eqO6U4=
+pgregory.net/rapid v1.1.0 h1:CMa0sjHSru3puNx+J0MIAuiiEV4N0qj8/cMWGBBCsjw=
+pgregory.net/rapid v1.1.0/go.mod h1:PY5XlDGj0+V1FCq0o192FdRhpKHGTRIWBgqjDBTrq04=
 rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8=
 rsc.io/pdf v0.1.1 h1:k1MczvYDUvJBe93bYd7wrZLLUEcLZAuF824/I4e5Xr4=
 rsc.io/pdf v0.1.1/go.mod h1:n8OzWcQ6Sp37PL01nO98y4iUCRdTGarVfzxY20ICaU4=

lib/errors/.gitignore

@@ -0,0 +1,10 @@
+# Generated by rapid after finding counter-examples.
+#
+# When re-running tests locally during development,
+# this enables faster failure finding.
+# https://github.com/flyingmutant/rapid/commit/4db076b4aa4d41036416da11abd5bbfc228a324e
+#
+# During development, if a check fails, it makes sense
+# to disable or refine the check, and add specific,
+# minimal tests using the counter-examples found by rapid.
+testdata/rapid/

lib/errors/BUILD.bazel

@@ -27,6 +27,7 @@ go_test(
     srcs = [
         "errors_test.go",
         "filter_test.go",
+        "invariants_test.go",
         "warning_test.go",
     ],
     embed = [":errors"],
@@ -34,5 +35,6 @@ go_test(
     deps = [
         "@com_github_stretchr_testify//assert",
         "@com_github_stretchr_testify//require",
+        "@net_pgregory_rapid//:rapid",
     ],
 )

lib/errors/cockroach.go

@@ -52,8 +52,31 @@ var (
 	// - in Sentry reports.
 	WithSafeDetails = errors.WithSafeDetails
 
-	Is        = errors.Is
-	IsAny     = errors.IsAny
+	// Is checks if the error tree err is equal to the value target.
+	//
+	// For error types which do not contain any data, Is is equivalent to As.
+	//
+	// For error types which contain data, it's possible that Is
+	// returns true for a value other than the one returned by As,
+	// since an error tree can contain multiple errors of the same
+	// concrete type but with different data.
+	Is    = errors.Is
+	IsAny = errors.IsAny
+	// As checks if the error tree err is of type target, and if so,
+	// sets target to the value of the error. This can be used in two ways:
+	//
+	// 1. If looking for an error of concrete type T, then the second
+	//    argument must be a non-nil pointer of type *T. This implies that
+	//    if the error interface is implemented with a pointer receiver,
+	//    then target must be of type **MyConcreteType.
+	// 2. If looking for an error satisfying an interface I (with a value
+	//    or pointer receiver), then the second argument must be of type I.
+	//
+	// For error types which do not contain any data, As is equivalent to Is.
+	//
+	// For error types which contain data, As will return an arbitrary
+	// error of the target type, in case there are multiple errors of the
+	// same concrete type in the error tree.
 	As        = errors.As
 	HasType   = errors.HasType
 	Cause     = errors.Cause

lib/errors/invariants_test.go

@@ -0,0 +1,297 @@
+package errors
+
+// This file contains tests which check the relationships between
+// the various error-checking functions such as Is, As and HasType.
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"pgregory.net/rapid"
+)
+
+// errorTree generates a tree of errors, where the leaves
+// are generated using leafError.
+func errorTree(leafError *rapid.Generator[error]) *rapid.Generator[error] {
+	errorSliceGen := rapid.SliceOfN(rapid.Deferred(func() *rapid.Generator[error] {
+		return errorTree(leafError)
+	}), 1, 3)
+	return rapid.OneOf(
+		leafError,
+		rapid.Map[[]error, error](errorSliceGen, func(errs []error) error {
+			return Append(nil, errs...)
+		}),
+	)
+}
+
+func TestInvariants(t *testing.T) {
+	// Check the behavior of the various error-checking
+	// functions for different kinds of error types -
+	// value receiver vs pointer receiver,
+	// and errors without data vs with data.
+
+	t.Run("payloadLessStruct", func(t *testing.T) {
+		rapid.Check(t, func(t *rapid.T) {
+			err := errorTree(rapid.OneOf(
+				rapid.Just(error(&notTheErrorOfInterest{})),
+				rapid.Just(error(payloadLessStructError{})),
+			)).Draw(t, "err")
+			// Is implies As for errors without data
+			if Is(err, payloadLessStructError{}) {
+				// This can be false, see Counter-example 1
+				//require.True(t, HasType(err, payloadLessStructError{}))
+				var check payloadLessStructError
+				require.True(t, As(err, &check))
+			}
+			// HasType implies Is and As for errors without data
+			if HasType(err, payloadLessStructError{}) {
+				require.True(t, Is(err, payloadLessStructError{}))
+				var check payloadLessStructError
+				require.True(t, As(err, &check))
+			}
+			var check payloadLessStructError
+			// As implies Is for errors without data
+			if As(err, &check) {
+				require.True(t, Is(err, payloadLessStructError{}))
+				// This can be false, see Counter-example 2
+				//require.True(t, HasType(err, payloadLessStructError{}))
+			}
+		})
+	})
+
+	t.Run("withPayloadStructError", func(t *testing.T) {
+		rapid.Check(t, func(t *rapid.T) {
+			errorOfInterest := withPayloadStructError{data: 11}
+			errorWithOtherData := withPayloadStructError{data: 24}
+			err := errorTree(rapid.OneOf(
+				rapid.Just(error(&notTheErrorOfInterest{})),
+				rapid.Just(error(errorOfInterest)),
+				rapid.Just(error(errorWithOtherData)),
+			)).Draw(t, "err")
+
+			// Is implies As for errors with data
+			if Is(err, errorOfInterest) {
+				// This is false, see Counter-example 5
+				//require.False(t, Is(err, errorWithOtherData))
+				require.False(t, Is(err, withPayloadStructError{}))
+				// These can be false, see Counter-example 1
+				//require.True(t, HasType(err, errorOfInterest))
+				//require.True(t, HasType(err, errorWithOtherData))
+				//require.True(t, HasType(err, withPayloadStructError{}))
+				var check withPayloadStructError
+				require.True(t, As(err, &check))
+				// This can be false, see Counter-example 6
+				//require.Equal(t, errorOfInterest, check)
+			}
+
+			// HasType implies As for errors with data
+			if HasType(err, errorOfInterest) {
+				require.True(t, HasType(err, errorWithOtherData))
+				require.True(t, HasType(err, withPayloadStructError{}))
+				// This can be false, see Counter-example 3
+				//require.True(t, Is(err, errorOfInterest))
+				var check withPayloadStructError
+				require.True(t, As(err, &check))
+				// This can be false, see Counter-example 4
+				//require.Equal(t, errorOfInterest, check)
+			}
+
+			// As implies a limited form of Is for errors with data
+			var check withPayloadStructError
+			if As(err, &check) {
+				require.True(t, check == errorOfInterest || check == errorWithOtherData)
+				require.True(t, Is(err, errorOfInterest) || Is(err, errorWithOtherData))
+				// These can be false, see Counter-example 2
+				//require.True(t, HasType(err, errorOfInterest))
+				//require.True(t, HasType(err, errorWithOtherData))
+				//require.True(t, HasType(err, withPayloadStructError{}))
+			}
+		})
+	})
+
+	t.Run("payloadLessPtrError", func(t *testing.T) {
+		rapid.Check(t, func(t *rapid.T) {
+			err := errorTree(rapid.OneOf(
+				rapid.Just(error(&notTheErrorOfInterest{})),
+				rapid.Just(error(&payloadLessPtrError{})),
+			)).Draw(t, "err")
+			// Is implies As for errors without data
+			if Is(err, &payloadLessPtrError{}) {
+				// This can be false, see Counter-example 1
+				//require.True(t, HasType(err, &payloadLessPtrError{}))
+				require.Panics(t, func() {
+					var check payloadLessPtrError
+					require.True(t, As(err, &check))
+				})
+				var check *payloadLessPtrError
+				require.True(t, As(err, &check))
+			}
+			// HasType implies Is and As for errors without data
+			if HasType(err, &payloadLessPtrError{}) {
+				require.True(t, Is(err, &payloadLessPtrError{}))
+				require.Panics(t, func() {
+					var check payloadLessPtrError
+					require.True(t, As(err, &check))
+				})
+				var check *payloadLessPtrError
+				require.True(t, As(err, &check))
+			}
+			var check *payloadLessPtrError
+			// As implies Is for errors without data
+			if As(err, &check) {
+				require.True(t, Is(err, &payloadLessPtrError{}))
+				// This can be false, see Counter-example 2
+				//require.True(t, errors.HasType(err, &payloadLessPtrError{}))
+			}
+		})
+	})
+
+	t.Run("withPayloadPtrError", func(t *testing.T) {
+		rapid.Check(t, func(t *rapid.T) {
+			errorOfInterest := &withPayloadPtrError{data: 11}
+			errorWithOtherData := &withPayloadPtrError{data: 24}
+			err := errorTree(rapid.OneOf(
+				rapid.Just(error(&notTheErrorOfInterest{})),
+				rapid.Just(error(errorOfInterest)),
+				rapid.Just(error(errorWithOtherData)),
+			)).Draw(t, "err")
+
+			// Is implies As for errors with data
+			if Is(err, errorOfInterest) {
+				// This is false, see Counter-example 5
+				//require.False(t, Is(err, errorWithOtherData))
+				require.False(t, Is(err, &withPayloadPtrError{}))
+				// These can be false, see Counter-example 1
+				//require.True(t, HasType(err, errorOfInterest))
+				//require.True(t, HasType(err, errorWithOtherData))
+				//require.True(t, HasType(err, withPayloadStructError{}))
+				require.Panics(t, func() {
+					var check withPayloadPtrError
+					_ = As(err, &check)
+				})
+				var check *withPayloadPtrError
+				require.True(t, As(err, &check))
+				// This can be false, see Counter-example 6
+				//require.Equal(t, *errorOfInterest, *check)
+			}
+
+			// HasType implies As for errors with data
+			if HasType(err, errorOfInterest) {
+				require.True(t, HasType(err, errorWithOtherData))
+				require.True(t, HasType(err, &withPayloadPtrError{}))
+				//This can be false, see Counter-example 3
+				//require.True(t, Is(err, errorOfInterest))
+				require.Panics(t, func() {
+					var check withPayloadPtrError
+					_ = As(err, &check)
+				})
+				var check *withPayloadPtrError
+				require.True(t, As(err, &check))
+				require.True(t, *check == *errorOfInterest || *check == *errorWithOtherData)
+			}
+
+			// As implies a limited form of Is for errors with data
+			var check *withPayloadPtrError
+			if As(err, &check) {
+				require.True(t, *check == *errorOfInterest || *check == *errorWithOtherData)
+				require.True(t, Is(err, errorOfInterest) || Is(err, errorWithOtherData))
+				// These can be false, see Counter-example 2
+				//require.True(t, HasType(err, errorOfInterest))
+				//require.True(t, HasType(err, errorWithOtherData))
+			}
+		})
+	})
+
+	t.Run("Counter-examples", func(t *testing.T) {
+		// Counter-example 1. Is does not imply HasType
+		{
+			err := Append(payloadLessStructError{}, &notTheErrorOfInterest{})
+			check := payloadLessStructError{}
+			require.True(t, Is(err, check))
+			require.False(t, HasType(err, check))
+		}
+		// Counter-example 2. As does not imply HasType
+		{
+			err := Append(payloadLessStructError{}, &notTheErrorOfInterest{})
+			check := payloadLessStructError{}
+			require.True(t, As(err, &check))
+			require.False(t, HasType(err, payloadLessStructError{}))
+		}
+		// Counter-example 3. HasType does not imply Is
+		{
+			err := error(withPayloadStructError{data: 3})
+			require.True(t, HasType(err, withPayloadStructError{}))
+			require.False(t, Is(err, withPayloadStructError{data: 1}))
+		}
+		// Counter-example 4. HasType does not imply As
+		{
+			err := error(withPayloadStructError{data: 3})
+			hasTypeCheck := withPayloadStructError{data: 1}
+			require.True(t, HasType(err, hasTypeCheck))
+			var valueFromAs withPayloadStructError
+			require.True(t, As(err, &valueFromAs))
+			require.NotEqual(t, hasTypeCheck, valueFromAs)
+		}
+		// Counter-example 5. Is can return true for distinct values
+		{
+			err := Append(withPayloadStructError{data: 3}, withPayloadStructError{data: 1})
+			require.True(t, Is(err, withPayloadStructError{data: 3}))
+			require.True(t, Is(err, withPayloadStructError{data: 1}))
+		}
+		// Counter-example 6. As can return a different value than the one passed to Is
+		{
+			err := Append(withPayloadStructError{data: 3}, withPayloadStructError{data: 1})
+			var check withPayloadStructError
+			require.True(t, Is(err, withPayloadStructError{data: 1}))
+			require.True(t, As(err, &check))
+			// 'As' picks the first value it finds.
+			require.Equal(t, check, withPayloadStructError{data: 3})
+			require.NotEqual(t, check, withPayloadStructError{data: 1})
+		}
+	})
+}
+
+type payloadLessStructError struct{}
+
+func (p payloadLessStructError) Error() string {
+	return "payloadLessStructError{}"
+}
+
+var _ error = payloadLessStructError{}
+
+type withPayloadStructError struct {
+	data int
+}
+
+var _ error = withPayloadStructError{}
+
+func (p withPayloadStructError) Error() string {
+	return fmt.Sprintf("withPayloadStructError{data: %v}", p.data)
+}
+
+type payloadLessPtrError struct{}
+
+var _ error = &payloadLessPtrError{}
+
+func (p *payloadLessPtrError) Error() string {
+	return "payloadLessPtrError{}"
+}
+
+type withPayloadPtrError struct {
+	data int
+}
+
+var _ error = &withPayloadPtrError{}
+
+func (p *withPayloadPtrError) Error() string {
+	return fmt.Sprintf("withPayloadPtrError{data: %d}", p.data)
+}
+
+type notTheErrorOfInterest struct{}
+
+var _ error = &notTheErrorOfInterest{}
+
+func (p *notTheErrorOfInterest) Error() string {
+	return "notTheErrorOfInterest{}"
+}

lib/go.mod

@@ -41,6 +41,7 @@ require (
 	google.golang.org/protobuf v1.33.0
 	gopkg.in/yaml.v2 v2.4.0
 	gopkg.in/yaml.v3 v3.0.1
+	pgregory.net/rapid v1.1.0
 )
 
 require (

lib/go.sum

@@ -555,3 +555,5 @@ honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWh
 honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
 mvdan.cc/gofumpt v0.4.0 h1:JVf4NN1mIpHogBj7ABpgOyZc65/UUOkKQFkoURsz4MM=
 mvdan.cc/gofumpt v0.4.0/go.mod h1:PljLOHDeZqgS8opHRKLzp2It2VBuSdteAgqUfzMTxlQ=
+pgregory.net/rapid v1.1.0 h1:CMa0sjHSru3puNx+J0MIAuiiEV4N0qj8/cMWGBBCsjw=
+pgregory.net/rapid v1.1.0/go.mod h1:PY5XlDGj0+V1FCq0o192FdRhpKHGTRIWBgqjDBTrq04=

lib/managedservicesplatform/go.sum

@@ -542,3 +542,5 @@ honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWh
 mvdan.cc/gofumpt v0.4.0/go.mod h1:PljLOHDeZqgS8opHRKLzp2It2VBuSdteAgqUfzMTxlQ=
 mvdan.cc/gofumpt v0.5.0 h1:0EQ+Z56k8tXjj/6TQD25BFNKQXpCvT0rnansIc7Ug5E=
 mvdan.cc/gofumpt v0.5.0/go.mod h1:HBeVDtMKRZpXyxFciAirzdKklDlGu8aAy1wEbH5Y9js=
+pgregory.net/rapid v1.1.0 h1:CMa0sjHSru3puNx+J0MIAuiiEV4N0qj8/cMWGBBCsjw=
+pgregory.net/rapid v1.1.0/go.mod h1:PY5XlDGj0+V1FCq0o192FdRhpKHGTRIWBgqjDBTrq04=

monitoring/go.sum

@@ -716,6 +716,8 @@ honnef.co/go/tools v0.0.1-2020.1.3/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9
 honnef.co/go/tools v0.0.1-2020.1.4/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k=
 mvdan.cc/gofumpt v0.4.0 h1:JVf4NN1mIpHogBj7ABpgOyZc65/UUOkKQFkoURsz4MM=
 mvdan.cc/gofumpt v0.4.0/go.mod h1:PljLOHDeZqgS8opHRKLzp2It2VBuSdteAgqUfzMTxlQ=
+pgregory.net/rapid v1.1.0 h1:CMa0sjHSru3puNx+J0MIAuiiEV4N0qj8/cMWGBBCsjw=
+pgregory.net/rapid v1.1.0/go.mod h1:PY5XlDGj0+V1FCq0o192FdRhpKHGTRIWBgqjDBTrq04=
 rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8=
 rsc.io/quote/v3 v3.1.0/go.mod h1:yEA65RcK8LyAZtP9Kv3t0HmxON59tX3rD+tICJqUlj0=
 rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA=