Skip to content

Commit ff29aba

Browse files
committed
fix(fieldorder): recursively detect pointer data in struct/array fields
Copilot review on PR #723 flagged that IsPointerish only checked reflect.Kind, so a struct field whose own Kind is reflect.Struct (or reflect.Array) but which itself wraps a pointer-bearing field (e.g. a nested struct holding a []byte) would be misclassified as scalar -- letting a real GC-ptrdata regression slip past these layout tests while the package doc comment claims to check "pointer-bearing fields". None of the four structs this package currently checks happen to have such a field today, but the gap is real for future callers. IsPointerish now takes a reflect.Type and recurses into struct fields and array element types. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
1 parent 44e4ee3 commit ff29aba

2 files changed

Lines changed: 65 additions & 6 deletions

File tree

pkg/goldmark/internal/fieldorder/fieldorder.go

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,27 @@ import (
1212
"testing"
1313
)
1414

15-
// IsPointerish reports whether a field's kind carries at least one
16-
// pointer word that the GC must scan (a pointer, interface, slice,
17-
// map, chan, func, string header, or unsafe.Pointer).
18-
func IsPointerish(k reflect.Kind) bool {
19-
switch k {
15+
// IsPointerish reports whether t carries at least one pointer word
16+
// that the GC must scan: directly (a pointer, interface, slice, map,
17+
// chan, func, string header, or unsafe.Pointer), or indirectly
18+
// through a struct field or array element of such a type, checked
19+
// recursively. A struct's reflect.Kind is always reflect.Struct
20+
// regardless of what its fields hold, so a Kind-only check would
21+
// misclassify e.g. a struct wrapping a []byte as scalar.
22+
func IsPointerish(t reflect.Type) bool {
23+
switch t.Kind() {
2024
case reflect.Ptr, reflect.Interface, reflect.Slice, reflect.Map,
2125
reflect.Chan, reflect.Func, reflect.String, reflect.UnsafePointer:
2226
return true
27+
case reflect.Struct:
28+
for i := 0; i < t.NumField(); i++ {
29+
if IsPointerish(t.Field(i).Type) {
30+
return true
31+
}
32+
}
33+
return false
34+
case reflect.Array:
35+
return IsPointerish(t.Elem())
2336
}
2437
return false
2538
}
@@ -34,7 +47,7 @@ func AssertPointersBeforeScalars(t testing.TB, typ reflect.Type, skip int) {
3447
seenScalar := false
3548
for i := skip; i < typ.NumField(); i++ {
3649
f := typ.Field(i)
37-
if IsPointerish(f.Type.Kind()) {
50+
if IsPointerish(f.Type) {
3851
if seenScalar {
3952
t.Errorf("field %s (%s, pointer-ish) is declared after a scalar field; "+
4053
"pointer fields should precede scalars to shrink GC ptrdata",
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
package fieldorder
2+
3+
import (
4+
"reflect"
5+
"testing"
6+
7+
"github.com/stretchr/testify/assert"
8+
)
9+
10+
// nestedPointerish is a struct whose Kind is reflect.Struct but which
11+
// carries a pointer word (Data) via a nested field — the shape
12+
// IsPointerish must recognise even though the field itself is not
13+
// directly a pointer/interface/slice/map/chan/func/string/
14+
// unsafe.Pointer kind.
15+
type nestedPointerish struct {
16+
Data []byte
17+
}
18+
19+
func TestIsPointerish_DetectsNestedStructField(t *testing.T) {
20+
typ := reflect.TypeOf(nestedPointerish{})
21+
assert.True(t, IsPointerish(typ),
22+
"a struct containing a pointer-bearing field (here, a []byte) must itself be pointer-ish")
23+
}
24+
25+
func TestIsPointerish_DetectsArrayOfPointerish(t *testing.T) {
26+
typ := reflect.TypeOf([2]nestedPointerish{})
27+
assert.True(t, IsPointerish(typ),
28+
"an array of pointer-bearing elements must itself be pointer-ish")
29+
}
30+
31+
func TestIsPointerish_ScalarStructIsNotPointerish(t *testing.T) {
32+
type allScalar struct {
33+
A int
34+
B bool
35+
C byte
36+
}
37+
assert.False(t, IsPointerish(reflect.TypeOf(allScalar{})))
38+
}
39+
40+
func TestIsPointerish_DirectKinds(t *testing.T) {
41+
assert.True(t, IsPointerish(reflect.TypeOf((*int)(nil))))
42+
assert.True(t, IsPointerish(reflect.TypeOf("")))
43+
assert.True(t, IsPointerish(reflect.TypeOf([]int(nil))))
44+
assert.False(t, IsPointerish(reflect.TypeOf(0)))
45+
assert.False(t, IsPointerish(reflect.TypeOf(byte(0))))
46+
}

0 commit comments

Comments
 (0)