A Zero-Sized Bug Hunt in golang.org/x/sync

… continued from the previous post.

While researching the usage of zero-sized types in Go I wrote zerolint¹ and the accompanying cmplint² and examined over 500 popular Go projects.

I want to look into some examples why I think those linters are useful. Let’s start with golang.org/x/sync/singleflight:

git clone --branch v0.15.0 https://go.googlesource.com/sync golang-sync
cd golang-sync
zerolint ./singleflight

# .../golang-sync/singleflight/singleflight_test.go:24:11: error interface implemented on pointer to zero-sized type "golang.org/x/sync/singleflight.errValue" (zl:err)
# .../golang-sync/singleflight/singleflight_test.go:78:8: comparison of pointer to zero-size type "golang.org/x/sync/singleflight.errValue" with error interface (zl:cme)

Okay, interesting. Let’s see:

18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83

package singleflight

import ...

type errValue struct{}

func (err *errValue) Error() string {
	return "error value"
}

func TestPanicErrorUnwrap(t *testing.T) {
	t.Parallel()

	testCases := []struct {
		name             string
		panicValue       interface{}
		wrappedErrorType bool
	}{
		{
			name:             "panicError wraps non-error type",
			panicValue:       &panicError{value: "string value"},
			wrappedErrorType: false,
		},
		{
			name:             "panicError wraps error type",
			panicValue:       &panicError{value: new(errValue)},
			wrappedErrorType: false,
		},
	}

	for _, tc := range testCases {
		tc := tc

		t.Run(tc.name, func(t *testing.T) {
			t.Parallel()

			var recovered interface{}

			group := &Group{}

			func() {
				defer func() {
					recovered = recover()
					t.Logf("after panic(%#v) in group.Do, recovered %#v", tc.panicValue, recovered)
				}()

				_, _, _ = group.Do(tc.name, func() (interface{}, error) {
					panic(tc.panicValue)
				})
			}()

			if recovered == nil {
				t.Fatal("expected a non-nil panic value")
			}

			err, ok := recovered.(error)
			if !ok {
				t.Fatalf("recovered non-error type: %T", recovered)
			}

			if !errors.Is(err, new(errValue)) && tc.wrappedErrorType {
				t.Errorf("unexpected wrapped error type %T; want %T", err, new(errValue))
			}
		})
	}
}

This test should verify that

1. A panic in a Group.Do is caught and re-thrown in the enclosing scope.

2. The value returned from recover is always an error type.

3. If the value passed to panic is also an error, the result from recover wraps the value.

The last point is why this test is named TestPanicErrorUnwrap.

Let us clean up a little before we come to the point:

1. The test cases are erroneous. wrappedErrorType is always false (should be true in the second case) and the values are already wrapped in the private error (they shouldn’t):

   36 37 38 39 40 41 42 43 44 45

   { name: "panicError wraps non-error type", panicValue: "string value", wrappedErrorType: false, }, { name: "panicError wraps error type", panicValue: new(errValue), wrappedErrorType: true, },

2. We should probably change the package to singleflight_test and import . "golang.org/x/sync/singleflight" to avoid using internal types. Skip this when you believe dot imports are evil.

3. Line 65 tc := tc is not needed since Go 1.22³

4. Also, line 79 should be something like t.Errorf("unexpected wrapped error \"%v\"; want \"%v\"", err, new(errValue)), since the error type we get is internal, so we can’t directly expect that - it should only wrap our error.

Let’s run the modified test (Go Playground):

=== RUN   TestPanicErrorUnwrap
=== RUN   TestPanicErrorUnwrap/panicError_wraps_non-error_type
=== RUN   TestPanicErrorUnwrap/panicError_wraps_error_type
--- PASS: TestPanicErrorUnwrap (0.00s)
    --- PASS: TestPanicErrorUnwrap/panicError_wraps_non-error_type (0.00s)
    --- PASS: TestPanicErrorUnwrap/panicError_wraps_error_type (0.00s)
PASS

Fine, we are done? Just for the fun of it, let’s modify line 10 to type errValue struct{ _ int } and run it again (Go Playground):

=== RUN   TestPanicErrorUnwrap
=== RUN   TestPanicErrorUnwrap/panicError_wraps_non-error_type
=== RUN   TestPanicErrorUnwrap/panicError_wraps_error_type
--- FAIL: TestPanicErrorUnwrap (0.00s)
    --- PASS: TestPanicErrorUnwrap/panicError_wraps_non-error_type (0.00s)
    --- FAIL: TestPanicErrorUnwrap/panicError_wraps_error_type (0.00s)
FAIL

So, when you read the last article you’ll know that errors.Is(err, new(T)) is undefined for zero-sized Ts. Let us first fix the test:

1. Use a (local) sentinel value errValue := errors.New("error value") instead of the type.

2. Replace all new(errValue) by references to the sentinel errValue.

The fixed test⁴ runs (Go Playground), is correct, not flagged by zerolint and easier to understand.

As a closing remark: zerolint (and in this case cmplint) find real errors in popular Go programs that are not necessarily detectable by testing.

… to be continued.