fix(hashset): make copy independent of the original - #4130
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a correctness bug in HashSet::copy where the copy could share mutable Entry objects with the original set, allowing removals/insertions in one set to silently corrupt the other’s probe sequences. This aligns HashSet copy semantics with other containers that rebuild entries rather than blitting references.
Changes:
- Rebuild
HashSetentries duringcopy()to avoid sharing mutableEntryobjects. - Add regression tests ensuring mutations on either the original or the copy do not affect the other.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hashset/hashset.mbt | Replaces reference blit in HashSet::copy with per-slot entry reconstruction to prevent shared mutable state. |
| hashset/copy_test.mbt | Adds regression coverage for copy independence (removals and later insertions/removals). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ///| | ||
| /// `copy` used to blit the entry references, which left both sets sharing | ||
| /// `Entry` objects. Because `Entry::psl` is mutable and `shift_back` | ||
| /// decrements it, a removal on either set silently corrupted the other's | ||
| /// probe sequences. The keys here are chosen to collide so that removal | ||
| /// actually triggers `shift_back`. | ||
| test "HashSet::copy is independent of the original" { | ||
| let original = @hashset.HashSet([]) | ||
| for i in 0..<32 { | ||
| original.add(i * 8) | ||
| } | ||
| let duplicate = original.copy() | ||
| for i in 0..<32 { | ||
| duplicate.remove(i * 8) | ||
| } | ||
| inspect(duplicate.length(), content="0") | ||
| inspect(original.length(), content="32") | ||
| let mut still_present = 0 | ||
| for i in 0..<32 { | ||
| if original.contains(i * 8) { | ||
| still_present += 1 | ||
| } | ||
| } | ||
| inspect(still_present, content="32") | ||
| } |
Coverage Report for CI Build 6295Coverage increased (+0.001%) to 90.877%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
03d8043 to
e92b1fa
Compare
Copilot review addressed
Correct, and the fix makes the test strictly better. The tests now use a It also made the failure far sharper. Against the unfixed code:
Self-reviewI tried to break this rather than confirm it. What I checked, and what it turned up: Only one of the three tests is a regression witness. I verified each against the unfixed code: the removal test fails (2 of 32), but the insertion test and the empty-set test pass. Insertion-direction corruption is real — The fix is complete for the type. Nothing depended on the sharing. No caller of No sibling has the same defect. I read all three: It costs something, and the PR should say so. Where I would still expect trouble: nothing in this diff, but the same class of defect — a container sharing mutable internals through a shallow copy — is worth checking for elsewhere in core. Outside this PR's scope, and I have not swept for it.
|
`HashSet::copy` allocated a fresh `entries` array and then blitted the old one into it. `FixedArray[Entry[K]?]` holds references, so both sets came away sharing the same `Entry` objects -- and `Entry::psl` is mutable, decremented by `shift_back` whenever a removal shifts a run back. So a removal on either set silently corrupted the other's probe sequences. Emptying a copy of a 32-element set leaves all but two of the original's keys unreachable by `contains`, while `length` still reports 32, because `size` is a separate field on each set. Rebuild each occupied slot instead, which is what `HashMap::copy`, `Map::copy` and `Set::copy` already do -- `HashSet` was the only container blitting. Slots are copied position for position, so the copy's iteration order still matches the original's. `copy` gets slower, because it now actually copies: 241 us -> 455 us on native and 281 us -> 430 us on js for 50000 elements. That is the cost of the operation being correct, and it brings `HashSet` in line with what the other three containers already pay. The regression test uses a key type whose hash maps into eight buckets, so the collisions that make removal reach `shift_back` are deterministic rather than dependent on how `Int` hashing happens to mix -- the same `Collide` pattern `hashmap` already uses for its coverage tests. Found while benchmarking removal for the struct-of-arrays work: a copy-then-remove benchmark gave results that could not be explained until the shallow copy came to light. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e92b1fa to
a558dc6
Compare
HashSet::copyallocates a freshentriesarray and then blits the old one into it.FixedArray[Entry[K]?]holds references, so both sets come away sharing the sameEntryobjects — andEntry::pslis mutable, decremented byshift_backwhenever a removal shifts a run back.So a removal on either set silently corrupts the other:
Insertions can disturb the original the same way, once one displaces a shared entry.
HashSetwas the only container doing this.HashMap::copy,Map::copyandSet::copyall rebuild their entries already; this makesHashSetmatch them.How it surfaced
While benchmarking removal for the struct-of-arrays work (#4127), I added a copy-then-remove benchmark and the numbers made no sense —
main's copy was implausibly cheap and the removal figures were unstable. Codex CLI, reviewing that PR atultraeffort, worked out why: the benchmark's timed closure was mutating the shared template, so every iteration after the first ran against progressively corrupted state. That invalidated my benchmark, and the reason it was invalid turned out to be a genuine bug underneath.The struct-of-arrays branch fixes this incidentally, because it rebuilds slots rather than blitting. This commit fixes it on
mainon its own, so the fix is not gated on that larger and still-contested change.Test
hashset/copy_test.mbtcovers both directions — mutating the copy must not disturb the original, and vice versa — using colliding keys so removal actually reachesshift_back. Without the fix the first test fails with 26 of 32 keys still reachable.moon testpasses 7543/7543 (7484/7484 on js).pkg.generated.mbtiunchanged.