From f09f77ab06fc74b76a4dba908fb99458ae26c1ec Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 4 Jan 2026 03:11:48 +0000 Subject: [PATCH] Refactor unsafe pointer usage to use reflect.Type and pointers Remove internal/danger package and replace unsafe pointer arithmetic with direct pointer manipulation. Update AST node references to use pointers instead of integer offsets. This improves code safety and maintainability. Co-authored-by: thomas.pelletier --- errors.go | 3 +- internal/danger/danger.go | 65 ------------ internal/danger/danger_test.go | 176 --------------------------------- internal/danger/typeid.go | 23 ----- internal/tracker/seen_test.go | 6 +- strict.go | 3 +- unmarshaler.go | 11 +-- unstable/ast.go | 27 ++--- unstable/builder.go | 30 +++++- unstable/parser.go | 6 +- 10 files changed, 44 insertions(+), 306 deletions(-) delete mode 100644 internal/danger/danger.go delete mode 100644 internal/danger/danger_test.go delete mode 100644 internal/danger/typeid.go diff --git a/errors.go b/errors.go index e910a5c..446ddc9 100644 --- a/errors.go +++ b/errors.go @@ -5,7 +5,6 @@ import ( "strconv" "strings" - "github.com/pelletier/go-toml/v2/internal/danger" "github.com/pelletier/go-toml/v2/unstable" ) @@ -99,7 +98,7 @@ func (e *DecodeError) Key() Key { // //nolint:funlen func wrapDecodeError(document []byte, de *unstable.ParserError) *DecodeError { - offset := danger.SubsliceOffset(document, de.Highlight) + offset := cap(document) - cap(de.Highlight) errMessage := de.Error() errLine, errColumn := positionAtEnd(document[:offset]) diff --git a/internal/danger/danger.go b/internal/danger/danger.go deleted file mode 100644 index e38e113..0000000 --- a/internal/danger/danger.go +++ /dev/null @@ -1,65 +0,0 @@ -package danger - -import ( - "fmt" - "reflect" - "unsafe" -) - -const maxInt = uintptr(int(^uint(0) >> 1)) - -func SubsliceOffset(data []byte, subslice []byte) int { - datap := (*reflect.SliceHeader)(unsafe.Pointer(&data)) - hlp := (*reflect.SliceHeader)(unsafe.Pointer(&subslice)) - - if hlp.Data < datap.Data { - panic(fmt.Errorf("subslice address (%d) is before data address (%d)", hlp.Data, datap.Data)) - } - offset := hlp.Data - datap.Data - - if offset > maxInt { - panic(fmt.Errorf("slice offset larger than int (%d)", offset)) - } - - intoffset := int(offset) - - if intoffset > datap.Len { - panic(fmt.Errorf("slice offset (%d) is farther than data length (%d)", intoffset, datap.Len)) - } - - if intoffset+hlp.Len > datap.Len { - panic(fmt.Errorf("slice ends (%d+%d) is farther than data length (%d)", intoffset, hlp.Len, datap.Len)) - } - - return intoffset -} - -func BytesRange(start []byte, end []byte) []byte { - if start == nil || end == nil { - panic("cannot call BytesRange with nil") - } - startp := (*reflect.SliceHeader)(unsafe.Pointer(&start)) - endp := (*reflect.SliceHeader)(unsafe.Pointer(&end)) - - if startp.Data > endp.Data { - panic(fmt.Errorf("start pointer address (%d) is after end pointer address (%d)", startp.Data, endp.Data)) - } - - l := startp.Len - endLen := int(endp.Data-startp.Data) + endp.Len - if endLen > l { - l = endLen - } - - if l > startp.Cap { - panic(fmt.Errorf("range length is larger than capacity")) - } - - return start[:l] -} - -func Stride(ptr unsafe.Pointer, size uintptr, offset int) unsafe.Pointer { - // TODO: replace with unsafe.Add when Go 1.17 is released - // https://github.com/golang/go/issues/40481 - return unsafe.Pointer(uintptr(ptr) + uintptr(int(size)*offset)) -} diff --git a/internal/danger/danger_test.go b/internal/danger/danger_test.go deleted file mode 100644 index 6569cdb..0000000 --- a/internal/danger/danger_test.go +++ /dev/null @@ -1,176 +0,0 @@ -package danger_test - -import ( - "testing" - "unsafe" - - "github.com/pelletier/go-toml/v2/internal/assert" - "github.com/pelletier/go-toml/v2/internal/danger" -) - -func TestSubsliceOffsetValid(t *testing.T) { - examples := []struct { - desc string - test func() ([]byte, []byte) - offset int - }{ - { - desc: "simple", - test: func() ([]byte, []byte) { - data := []byte("hello") - return data, data[1:] - }, - offset: 1, - }, - } - - for _, e := range examples { - t.Run(e.desc, func(t *testing.T) { - d, s := e.test() - offset := danger.SubsliceOffset(d, s) - assert.Equal(t, e.offset, offset) - }) - } -} - -func TestSubsliceOffsetInvalid(t *testing.T) { - examples := []struct { - desc string - test func() ([]byte, []byte) - }{ - { - desc: "unrelated arrays", - test: func() ([]byte, []byte) { - return []byte("one"), []byte("two") - }, - }, - { - desc: "slice starts before data", - test: func() ([]byte, []byte) { - full := []byte("hello world") - return full[5:], full[1:] - }, - }, - { - desc: "slice starts after data", - test: func() ([]byte, []byte) { - full := []byte("hello world") - return full[:3], full[5:] - }, - }, - { - desc: "slice ends after data", - test: func() ([]byte, []byte) { - full := []byte("hello world") - return full[:5], full[3:8] - }, - }, - } - - for _, e := range examples { - t.Run(e.desc, func(t *testing.T) { - d, s := e.test() - assert.Panics(t, func() { - danger.SubsliceOffset(d, s) - }) - }) - } -} - -func TestStride(t *testing.T) { - a := []byte{1, 2, 3, 4} - x := &a[1] - n := (*byte)(danger.Stride(unsafe.Pointer(x), unsafe.Sizeof(byte(0)), 1)) - assert.Equal(t, &a[2], n) - n = (*byte)(danger.Stride(unsafe.Pointer(x), unsafe.Sizeof(byte(0)), -1)) - assert.Equal(t, &a[0], n) -} - -func TestBytesRange(t *testing.T) { - type fn = func() ([]byte, []byte) - examples := []struct { - desc string - test fn - expected []byte - }{ - { - desc: "simple", - test: func() ([]byte, []byte) { - full := []byte("hello world") - return full[1:3], full[6:8] - }, - expected: []byte("ello wo"), - }, - { - desc: "full", - test: func() ([]byte, []byte) { - full := []byte("hello world") - return full[0:1], full[len(full)-1:] - }, - expected: []byte("hello world"), - }, - { - desc: "end before start", - test: func() ([]byte, []byte) { - full := []byte("hello world") - return full[len(full)-1:], full[0:1] - }, - }, - { - desc: "nils", - test: func() ([]byte, []byte) { - return nil, nil - }, - }, - { - desc: "nils start", - test: func() ([]byte, []byte) { - return nil, []byte("foo") - }, - }, - { - desc: "nils end", - test: func() ([]byte, []byte) { - return []byte("foo"), nil - }, - }, - { - desc: "start is end", - test: func() ([]byte, []byte) { - full := []byte("hello world") - return full[1:3], full[1:3] - }, - expected: []byte("el"), - }, - { - desc: "end contained in start", - test: func() ([]byte, []byte) { - full := []byte("hello world") - return full[1:7], full[2:4] - }, - expected: []byte("ello w"), - }, - { - desc: "different backing arrays", - test: func() ([]byte, []byte) { - one := []byte("hello world") - two := []byte("hello world") - return one, two - }, - }, - } - - for _, e := range examples { - t.Run(e.desc, func(t *testing.T) { - start, end := e.test() - if e.expected == nil { - assert.Panics(t, func() { - danger.BytesRange(start, end) - }) - } else { - res := danger.BytesRange(start, end) - assert.Equal(t, e.expected, res) - } - }) - } -} diff --git a/internal/danger/typeid.go b/internal/danger/typeid.go deleted file mode 100644 index 9d41c28..0000000 --- a/internal/danger/typeid.go +++ /dev/null @@ -1,23 +0,0 @@ -package danger - -import ( - "reflect" - "unsafe" -) - -// typeID is used as key in encoder and decoder caches to enable using -// the optimize runtime.mapaccess2_fast64 function instead of the more -// expensive lookup if we were to use reflect.Type as map key. -// -// typeID holds the pointer to the reflect.Type value, which is unique -// in the program. -// -// https://github.com/segmentio/encoding/blob/master/json/codec.go#L59-L61 -type TypeID unsafe.Pointer - -func MakeTypeID(t reflect.Type) TypeID { - // reflect.Type has the fields: - // typ unsafe.Pointer - // ptr unsafe.Pointer - return TypeID((*[2]unsafe.Pointer)(unsafe.Pointer(&t))[1]) -} diff --git a/internal/tracker/seen_test.go b/internal/tracker/seen_test.go index 086b849..a8135bf 100644 --- a/internal/tracker/seen_test.go +++ b/internal/tracker/seen_test.go @@ -1,8 +1,8 @@ package tracker import ( + "reflect" "testing" - "unsafe" "github.com/pelletier/go-toml/v2/internal/assert" ) @@ -13,8 +13,8 @@ func TestEntrySize(t *testing.T) { // and a very good reason. maxExpectedEntrySize := 48 assert.True(t, - int(unsafe.Sizeof(entry{})) <= maxExpectedEntrySize, + int(reflect.TypeOf(entry{}).Size()) <= maxExpectedEntrySize, "Expected entry to be less than or equal to %d, got: %d", - maxExpectedEntrySize, int(unsafe.Sizeof(entry{})), + maxExpectedEntrySize, int(reflect.TypeOf(entry{}).Size()), ) } diff --git a/strict.go b/strict.go index 802e7e4..fb87d84 100644 --- a/strict.go +++ b/strict.go @@ -1,7 +1,6 @@ package toml import ( - "github.com/pelletier/go-toml/v2/internal/danger" "github.com/pelletier/go-toml/v2/internal/tracker" "github.com/pelletier/go-toml/v2/unstable" ) @@ -103,5 +102,5 @@ func keyLocation(node *unstable.Node) []byte { end = k.Node().Data } - return danger.BytesRange(start, end) + return start[:cap(start)-cap(end)+len(end)] } diff --git a/unmarshaler.go b/unmarshaler.go index 047c8b4..e8c028b 100644 --- a/unmarshaler.go +++ b/unmarshaler.go @@ -12,7 +12,6 @@ import ( "sync/atomic" "time" - "github.com/pelletier/go-toml/v2/internal/danger" "github.com/pelletier/go-toml/v2/internal/tracker" "github.com/pelletier/go-toml/v2/unstable" ) @@ -1294,13 +1293,13 @@ func fieldByIndex(v reflect.Value, path []int) reflect.Value { type fieldPathsMap = map[string][]int -var globalFieldPathsCache atomic.Value // map[danger.TypeID]fieldPathsMap +var globalFieldPathsCache atomic.Value // map[reflect.Type]fieldPathsMap func structFieldPath(v reflect.Value, name string) ([]int, bool) { t := v.Type() - cache, _ := globalFieldPathsCache.Load().(map[danger.TypeID]fieldPathsMap) - fieldPaths, ok := cache[danger.MakeTypeID(t)] + cache, _ := globalFieldPathsCache.Load().(map[reflect.Type]fieldPathsMap) + fieldPaths, ok := cache[t] if !ok { fieldPaths = map[string][]int{} @@ -1311,8 +1310,8 @@ func structFieldPath(v reflect.Value, name string) ([]int, bool) { fieldPaths[strings.ToLower(name)] = path }) - newCache := make(map[danger.TypeID]fieldPathsMap, len(cache)+1) - newCache[danger.MakeTypeID(t)] = fieldPaths + newCache := make(map[reflect.Type]fieldPathsMap, len(cache)+1) + newCache[t] = fieldPaths for k, v := range cache { newCache[k] = v } diff --git a/unstable/ast.go b/unstable/ast.go index 5f3acaf..1c1ec9b 100644 --- a/unstable/ast.go +++ b/unstable/ast.go @@ -2,9 +2,6 @@ package unstable import ( "fmt" - "unsafe" - - "github.com/pelletier/go-toml/v2/internal/danger" ) // Iterator over a sequence of nodes. @@ -37,7 +34,7 @@ func (c *Iterator) Next() bool { // IsLast returns true if the current node of the iterator is the last // one. Subsequent calls to Next() will return false. func (c *Iterator) IsLast() bool { - return c.node.next == 0 + return c.node.next == nil } // Node returns a pointer to the node pointed at by the iterator. @@ -65,11 +62,9 @@ type Node struct { Raw Range // Raw bytes from the input. Data []byte // Node value (either allocated or referencing the input). - // References to other nodes, as offsets in the backing array - // from this node. References can go backward, so those can be - // negative. - next int // 0 if last element - child int // 0 if no child + // References to other nodes. + next *Node // nil if last element + child *Node // nil if no child } // Range of bytes in the document. @@ -80,24 +75,14 @@ type Range struct { // Next returns a pointer to the next node, or nil if there is no next node. func (n *Node) Next() *Node { - if n.next == 0 { - return nil - } - ptr := unsafe.Pointer(n) - size := unsafe.Sizeof(Node{}) - return (*Node)(danger.Stride(ptr, size, n.next)) + return n.next } // Child returns a pointer to the first child node of this node. Other children // can be accessed calling Next on the first child. Returns nil if this Node // has no child. func (n *Node) Child() *Node { - if n.child == 0 { - return nil - } - ptr := unsafe.Pointer(n) - size := unsafe.Sizeof(Node{}) - return (*Node)(danger.Stride(ptr, size, n.child)) + return n.child } // Valid returns true if the node's kind is set (not to Invalid). diff --git a/unstable/builder.go b/unstable/builder.go index 9538e30..4348eb3 100644 --- a/unstable/builder.go +++ b/unstable/builder.go @@ -29,8 +29,10 @@ func (r reference) Valid() bool { } type builder struct { - tree root - lastIdx int + tree root + lastIdx int + nextOffsets []int + childOffsets []int } func (b *builder) Tree() *root { @@ -43,29 +45,47 @@ func (b *builder) NodeAt(ref reference) *Node { func (b *builder) Reset() { b.tree.nodes = b.tree.nodes[:0] + b.nextOffsets = b.nextOffsets[:0] + b.childOffsets = b.childOffsets[:0] b.lastIdx = 0 } func (b *builder) Push(n Node) reference { b.lastIdx = len(b.tree.nodes) b.tree.nodes = append(b.tree.nodes, n) + b.nextOffsets = append(b.nextOffsets, 0) + b.childOffsets = append(b.childOffsets, 0) return reference(b.lastIdx) } func (b *builder) PushAndChain(n Node) reference { newIdx := len(b.tree.nodes) b.tree.nodes = append(b.tree.nodes, n) + b.nextOffsets = append(b.nextOffsets, 0) + b.childOffsets = append(b.childOffsets, 0) + if b.lastIdx >= 0 { - b.tree.nodes[b.lastIdx].next = newIdx - b.lastIdx + b.nextOffsets[b.lastIdx] = newIdx - b.lastIdx } b.lastIdx = newIdx return reference(b.lastIdx) } func (b *builder) AttachChild(parent reference, child reference) { - b.tree.nodes[parent].child = int(child) - int(parent) + b.childOffsets[parent] = int(child) - int(parent) } func (b *builder) Chain(from reference, to reference) { - b.tree.nodes[from].next = int(to) - int(from) + b.nextOffsets[from] = int(to) - int(from) +} + +func (b *builder) Link() { + for i := range b.tree.nodes { + if next := b.nextOffsets[i]; next != 0 { + b.tree.nodes[i].next = &b.tree.nodes[i+next] + } + if child := b.childOffsets[i]; child != 0 { + b.tree.nodes[i].child = &b.tree.nodes[i+child] + } + } } diff --git a/unstable/parser.go b/unstable/parser.go index e29f5e1..aeeacb8 100644 --- a/unstable/parser.go +++ b/unstable/parser.go @@ -6,7 +6,6 @@ import ( "unicode" "github.com/pelletier/go-toml/v2/internal/characters" - "github.com/pelletier/go-toml/v2/internal/danger" ) // ParserError describes an error relative to the content of the document. @@ -70,7 +69,7 @@ func (p *Parser) Data() []byte { // panics. func (p *Parser) Range(b []byte) Range { return Range{ - Offset: uint32(danger.SubsliceOffset(p.data, b)), + Offset: uint32(cap(p.data) - cap(b)), Length: uint32(len(b)), } } @@ -126,6 +125,7 @@ func (p *Parser) NextExpression() bool { p.first = false if p.ref.Valid() { + p.builder.Link() return true } } @@ -159,7 +159,7 @@ type Shape struct { } func (p *Parser) position(b []byte) Position { - offset := danger.SubsliceOffset(p.data, b) + offset := cap(p.data) - cap(b) lead := p.data[:offset]