Add zipper-type property tests: same_trie, seeded ring algebra, long-path serialization

[MesTTo]

Summary

Three additive test/API commits, no internals changed:

• PathMap::same_trie — an O(1) copy-on-write root-identity staleness
  check (compares the root node pointer, no traversal).
• Seeded ring algebra property tests — implements the fuzz test the
  trailing TODO in ring.rs described: seeded pseudorandom HashSet and
  nested-HashMap lattices checked against plain set oracles across
  pjoin/pmeet/psubtract/join_into, asserting both results and the
  identity/element status masks. Also covers the empty-subset-collapse case
  and the HashMap psubtract case the inline notes flagged as untested. Only
  the now-resolved TODO notes are removed; the still-open ones (Vec
SetLattice, LatticeCounter/LatticeBitfield traits) are left in place.
• Long-path serialization coverage — answers the note at the top of
  paths_serialization.rs: round-trips deterministic paths of 7 / 2049 /
  4097 / 24000 bytes, so the input spans the internal 4096-byte compression
  chunk, checked in both directions.

Test plan

• cargo test --release --lib (664/0, up from the 660/0/1 baseline)
• cargo test --release --lib same_trie / seeded / paths_serialization individually

[Adam-Vandervorst]

This comment is deleted but is_bottom check is not introduced?

[luketpeterson]

The is_bottom method is also deleted.

[Adam-Vandervorst]

This seems like a verbose and at the same time unspecific explanation.

[Adam-Vandervorst]

@luketpeterson good public interface addition? To me, "same trie" could also be logical, so I'd change this to "ptr eq".

[Adam-Vandervorst]

Should also use node_id?

[luketpeterson]

I ended up implementing ZipperConcrete instead. It provides more useful API capability and I think it's more consistent.

@MesTTo WDYT?

[MesTTo]

pushed a fix. #4 was the important one, it's a real bug.

renamed same_trie to root_ptr_eq. your read on the name was right, and it's wrong the other way too, it's false for separately-built identical tries. rewrote the doc against shared_node_id's shape.

the bug #4 points at: it compared only the root node, but root values live in root_val outside the node, and set_val_at on an empty key goes through set_root_val without touching root. so two maps differing only by a root value compared equal, i.e. true for different contents. added root_ptr_eq_is_false_when_a_root_value_differs, which fails on the old code. fixed by returning false when either map has a root value, so a true result means same contents. same reason shared_node_id returns None at a node with a value.

on the ring.rs one: the is_bottom that TODO wanted was a Lattice method against bottom(), and bottom() was deleted from the trait (04732ef), so it never landed. the empty-collapse it wanted tested is real though, it's result_set.len() == 0 -> AlgebraicResult::None at ring.rs:1070, which is what the test exercises.

[luketpeterson]

The is_bottom method is also deleted.

[MesTTo]

on ZipperConcrete: agreed, it should replace root_ptr_eq. it gives callers something
root_ptr_eq could not, and the thing i actually cared about survived the swap:
self.root_val().is_some() in PathMap::shared_node_id is the bug my test was pinning. root values
live outside the root node, so a node-level identity check cannot see them, and without that guard
two maps differing only by a root value report identical.

i went looking for whether i could have got there through a zipper instead, and you could not have:
on master the zipper route returns None at every root, because ReadZipperCore::is_shared
hardcodes false there (// root), TrieRefBorrowed/TrieRefOwned::is_shared return false
unconditionally, and read_zipper_shared_node_id short-circuits on !is_shared(). so putting it on
PathMap was the only place it could go, and before your commit i do not think there was any call
sequence that answered this question.

two small asks, neither blocking:

1. the new impl has no doc comment, and the contract is not obvious. Some(x) == Some(y) means same
   root node and no root value on either side, therefore same contents. None means unknown, not
   different. that asymmetry is the whole reason the root_val guard is there, and the trait docs
   say the opposite thing about is_shared ("must never rely on the return value for correctness"),
   which the new impl does rely on. worth writing down for whoever touches it next.
2. the new test asserts with_root_value.shared_node_id() == None on one map. the regression it
   replaces asserted the predicate on a pair differing only by a root value. one line restores that
   shape: assert_ne!(m.shared_node_id(), with_root_value.shared_node_id()) and the reverse.

one observation rather than a request, which i noticed while checking the above: the two impls now
disagree about the same location. for a map whose root is shared with a clone, PathMap::is_shared()
is true and gives Some(id), while read_zipper().is_shared() is false and gives None. i
think yours is the useful answer and the zipper's false // root is the artifact, so i read it as a
question about whether the zipper root case should be fixed to match, not as anything this PR needs
to resolve.

i am also giving up some precision by taking your version, all of it in the safe direction. i now get
None where root_ptr_eq returned true, for two empty maps, for an unshared map against itself,
and for two identical maps that both carry a root value. none of that touches the cow-staleness case
i wrote it for, since a snapshot clone always drives the refcount to 2, so i am fine losing it. the
one new hazard i would watch is that shared_node_id hands back a storable u64, so caching it
across a drop can collide with a later allocation, where root_ptr_eq took two live references and
could not. the trait doc already warns about it.

on is_bottom: right, both are gone. i grepped and the only occurrence left in the tree is the
TODO comment itself. bottom() went in 04732ef ("Cleaning up a bunch of dead code"), and it was
already commented out before that, together with the note proposing is_bottom. so the check that
TODO wanted was never implemented and the trait method it would have compared against is gone too.

the behaviour it wanted tested is real though, and i traced it to
set_lattice_integrate_into_result: result_set.len() == 0 returns AlgebraicResult::None, and the
caller drops it so the key never enters the outer set. the assertion five lines below the TODO,
assert!(joined.get(&"C").is_none()), was vacuous on master, since neither operand had "C". this
PR inserts "C" as an empty sub-map into both, so the inner pjoin of two empty maps actually runs
and only the collapse keeps it out.

i checked that rather than assuming it. i patched if result_len == 0 to if false and re-ran:
set_lattice_join_test2 still passes on master, and fails on this branch with left: 3, right: 2.
so the test does exercise the path, and it is the insert that makes it do so.

i will also fix the unused import: AlgebraicStatus in ring.rs while i am in here.

[luketpeterson]

on master the zipper route returns None at every root, because ReadZipperCore::is_shared hardcodes false there

The reason it was hardcoded to false was a leftover from a limitation that no longer exists. Pushed the fix for that. But leaving the ZipperConcrete impl on PathMap because it saves the overhead of creating a zipper.