Perform immut/{set, sparse_array}

[FlyCloudC]

This PR is the first part of the split #2205, with clearer git history.

1. clean doc in sparse_array
2. using Path to avoid passing depth
3. replace Collision(Bucket[A]) with Leaf(A, @list.T[A])
4. rename T to Node, remove Node::empty, define T as Node?
5. replace nested Branch({ data:[Branch(...)], .. }) by Flat(key, path)

Parts from #2205 not included in this PR:

1. adopt CPS style in add/remove to reuse old nodes in special cases (maybe we don't need this)
2. optimize set operations: union, intersection, difference
3. Improve test coverage by using MyInt, which only use the lower 8 bits for hash
4. apply same improvements to immut/map

Overview

-priv enum Bucket[T] {
-  JustOne(T) // must be non-empty
-  More(T, Bucket[T])
-}

-enum T[A] {
+priv enum Node[A] {
-  Empty

-  Leaf(A)
-  Collision(Bucket[A])
+  Leaf(A, @list.T[A])

+  Flat(A, Path)

-  Branch(@sparse_array.SparseArray[T[A]])
+  Branch(@sparse_array.SparseArray[Node[A]])
}

+ type T[A] Node[A]?

[peter-jerry-ye-code-review]

Path comparison in Node::contains might cause infinite loop

Category
Correctness
Code Snippet
fn[A : Eq] Node::contains(self : Node[A], key : A, path : Path) -> Bool {
loop (self, path) {
(Leaf(key1, bucket), _) => key == key1 || bucket.contains(key)
(Flat(key1, path1), path) => path == path1 && key == key1
(Branch(children), path) => {
let idx = path.idx()
if children[idx] is Some(child) {
continue (child, path.next())
}
false
}
}
}
Recommendation
Add a path length check to prevent infinite recursion:

(Branch(children), path) => {
  if path.is_last() { return false }
  let idx = path.idx()
  if children[idx] is Some(child) {
    continue (child, path.next())
  }
  false
}

Reasoning
The loop in Node::contains could potentially continue indefinitely if the path structure is malformed. Adding a check for path.is_last() ensures termination.

Unnecessary allocations in union operation

Category
Performance
Code Snippet
fn[K : Eq] T::union(self : T[K], other : T[K]) -> T[K] {
fn go(node1, node2) {
match (node1, node2) {
(node, Flat(key, path)) | (Flat(key, path), node) =>
node.add_with_path(key, path)
Recommendation
Consider reusing nodes when possible:

fn go(node1, node2) {
  match (node1, node2) {
    (node, Flat(key, path)) | (Flat(key, path), node) =>
      if node.contains(key, path) { node } else { node.add_with_path(key, path) }

Reasoning
The current implementation always creates a new node even when the key already exists in the target node. Checking containment first could avoid unnecessary allocations.

Path implementation details are not well documented

Category
Maintainability
Code Snippet
pub(all) type Path UInt derive(Eq)

const SEGMENT_LENGTH : Int = 5
const INDEX_MASK : UInt = (1 << SEGMENT_LENGTH) - 1
const SEGMENT_NUM : Int = 32 / SEGMENT_LENGTH
const HEAD_TAG : UInt = 0xffffffffU << (SEGMENT_LENGTH * SEGMENT_NUM)
Recommendation
Add detailed documentation explaining the Path type and its bit layout:

///| A Path represents a sequence of 5-bit segments in a 32-bit integer.
/// The format is: HEAD_TAG | segment_n | ... | segment_1 | segment_0
/// where each segment is 5 bits and HEAD_TAG ensures the upper bits are set.
pub(all) type Path UInt derive(Eq)

Reasoning
The Path type is central to the new implementation but its internal representation and constraints are not clearly documented. Better documentation would help maintainers understand the design.

[coveralls]

Pull Request Test Coverage Report for Build 94

Details

• 189 of 220 (85.91%) changed or added relevant lines in 5 files are covered.
• 2 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.1%) to 89.034%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
immut/internal/sparse_array/sparse_array.mbt	49	56	87.5%
immut/hashset/HAMT.mbt	117	141	82.98%

Files with Coverage Reduction	New Missed Lines	%
immut/hashset/HAMT.mbt	1	85.03%
immut/internal/sparse_array/sparse_array.mbt	1	89.04%

Totals
Change from base Build 91:	-0.1%
Covered Lines:	3475
Relevant Lines:	3903

---

💛 - Coveralls

[peter-jerry-ye]

I'll leave it to the experts for the implementation of the algorithm, but here's something I've observed.

[Guest0x0]

btw from #2205 the optimized set operations seem quite independent, I think we should include that part in this PR too, so that there would be no performance degeneration in those operations

[Guest0x0]

what's the purpose of this?

[Guest0x0]

So if I understand correctly, the new version only utilize the lower 30 bits of the hash value, and the highest 2 bits are used to identify end of hash value?

[Guest0x0]

just a note: maybe it's possible to also do path compression for Branch (i.e. add an extra path segment (Path + length) parameter) too?

[Guest0x0]

ys.add(y) will be run multiple times here

[FlyCloudC]

(using arrow function)