fix: expand aliased slice views with differing lengths in TagExpand#214
Merged
Conversation
sameActiveNode identified an active slice node by its first-element pointer alone. Two slices viewing the same backing array at the same start (e.g. s[:2] and s[:3]) therefore compared equal, so a nested longer slice was mistaken for a cycle back to a shorter view and its later elements were silently dropped from the expansion. Give reflect.Slice its own case that also compares length: views of differing lengths are distinct traversal nodes, while a genuine self-referential slice re-enters with an identical pointer and length and is still detected as a cycle. Fixes #203
sameActiveNode compared slice nodes by first-element pointer and length. A named slice type implementing JawsGetTag can observe its capacity via cap, so two views sharing a pointer and length but differing in capacity are distinct getters that may yield different tags. Comparing pointer and length alone falsely reported a cycle and, because a non-comparable slice getter cannot itself be a tag key, returned ErrNotUsableAsTag instead of invoking the second getter. Compare pointer, length and capacity, which together fully identify a slice header. A genuine self-referential slice re-enters with an identical header and is still detected as a cycle.
linkdata
marked this pull request as ready for review
July 22, 2026 19:56
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #203.
Problem
TagExpand's cycle detector (sameActiveNode) identified an active[]anynode by its first-element pointer alone. For a slice,
reflect.Value.Pointer()is the address of the first element, so two distinct slice headers viewing the
same backing array at the same start compared equal even with different lengths.
Expanding a short view whose element references the longer backing slice then
mistook the longer slice for a cycle back to the short view, skipped its
expansion, and silently dropped every later tag:
Downstream
Dirty/broadcast/lookup on the dropped tag then silently miss thoseElements.
Fix
Give
reflect.Sliceits own case insameActiveNodethat also compares length:traversal nodes.
length and is still detected as a cycle.
Capacity is intentionally not compared — two views with the same start and
length iterate identical elements, so they are correctly the same node. The
change only makes detection more precise (removes a false positive); even a
hypothetical missed cycle stays bounded by
maxTagDepth/maxTagCount.Tests
TestTagExpand_AliasedSliceViews— the issue's exact reproduction.TestSameActiveNode_AliasedSliceViews— direct helper coverage of thediffering-length (distinct) and self (same-node) cases.
Both fail without the fix and pass with it. Existing cycle tests
(
TestTagExpand_SelfReferentialSliceStopsRecursing, the TagGetter self/mutualcycle tests) stay green.
Verified with
gofmt -l,go vet ./lib/tag/, andgo test -race ./...(all packages green, no races).
Known follow-up (out of scope)
FindTagGetterinlib/tag/errnotusableastag.gohas an analogousseenPtr{t, ptr}slice-aliasing collision, but it only powers a best-efforterror-message hint — over-detection there degrades a hint string, never drops
real tags. Left untouched to keep this change focused on the reported bug.