refactor(bloom): adopt fastbloom for BloomFilter implementation

[paulnsorensen]

Summary

NIH audit finding — drop ~105 LOC of hand-rolled bloom-filter math in src/index/bloom.rs (double_hash, combined_hash, hash_with_seed, optimal-bit/hash sizing) in favor of fastbloom, the SIMD-optimized de facto Rust bloom crate.

The extract_identifiers byte state machine — the unique-to-tilth piece — stays. BloomFilterCache now wraps fastbloom::BloomFilter, constructed via BloomFilter::with_false_pos(0.01).expected_items(n).

Net: +5 / -133 in src/index/bloom.rs; one new dependency (fastbloom = "0.17").

This is one of three split PRs extracted from the bundled NIH audit refactor (see closed #81 / paulnsorensen#9). The other two — thiserror (#86) and strsim — are independent and land separately.

Dependency health

fastbloom verified at time of writing (2026-05-08):

Crate	Latest	Last release	Repo last push	Stars	Recent downloads
fastbloom	0.17.0	2026-03-01	2026-03-01	348	5.0M

Small project (348 stars), single maintainer, but very active: 0.15 → 0.16 → 0.17 across three weeks in Feb–Mar 2026. Only 4 open issues. The de facto fast Rust bloom crate; alternatives (bloomfilter, growable-bloom-filter) are unmaintained or niche. Non-archived, MIT/Apache-2.0.

Test plan

• cargo build clean
• cargo clippy -- -D warnings clean
• cargo fmt --check clean
• cargo test — 342 passed (3 suites, all bloom tests including identifier extraction, FPR, and large-scale insertion)
• Branch is off latest origin/main (post-v0.8.0)

Notes

• Drop test_bloom_filter_sizing — it asserted private fields (num_bits, num_hashes) of the removed hand-rolled BloomFilter type. Equivalent guarantees are part of fastbloom's own test suite.
• BloomFilter::new(expected_items, target_fpr) (old, two-positional-arg constructor) becomes BloomFilter::with_false_pos(0.01).expected_items(n) (fastbloom builder pattern).

Why split from #81

#81 bundled three independent NIH-audit changes. Reviewing them separately is easier — fastbloom is the largest and most controversial of the three because of single-maintainer dependency risk, so it deserves its own review thread.

The three split PRs touch disjoint files (error.rs / index/bloom.rs / diff/matching.rs) and disjoint Cargo.toml lines, so they merge in any order without conflict.

[jahala]

Reviewed by Opus 4.7:

Behavior parity confirmed — FPR target 0.01 preserved, builder-pattern equivalent to the old constructor, per-process random seed is fine for an in-memory cache keyed on path+mtime. The math is right and the swap is clean.

Test count discrepancy — confirm before merge

Your PR description says "342 pass (down from 344)". Running cargo test on the checked-out branch shows 338. Diff has two explicit deletions visible (test_bloom_filter_sizing and the constructor test), but the actual count is down 6, not 2.

Most likely explanation: the old hand-rolled BloomFilter struct had #[cfg(test)] mod tests { ... } content that disappeared along with the module, so the count delta includes those internal tests as well as the two visible top-level ones. That's correct behavior — those tests asserted on num_bits / num_hashes private fields that no longer exist on the opaque fastbloom::BloomFilter, and the equivalent guarantees are part of fastbloom's own test suite.

Could you confirm and update the PR description? A quick git diff main -- src/index/bloom.rs | grep -c "^+.*#\[test\]" vs the same for ^- would settle it. Just want to make sure the count drop isn't masking a regression somewhere unrelated.

Nit: lost context comment on filter sizing

build_filter now does:

let idents: Vec<&str> = extract_identifiers(content).collect();
let expected = idents.len().max(1);
let mut filter = BloomFilter::with_false_pos(0.01).expected_items(expected);
for ident in idents {
    filter.insert(ident);
}

idents contains many duplicates — same identifier (fn, let, use, i, j) appears repeatedly in any source file. So expected_items(idents.len()) over-sizes the filter relative to actual unique-identifier load, which means achieved FPR is much better than 1% in practice. That's fine (the filter is just slightly bigger than necessary), but the deleted comment "Count identifiers first to size the filter appropriately" was load-bearing context for a future reader. Restore something like // Sized for total token count, which over-allocates given duplicates — achieved FPR is < 0.01 in practice.

Once the test count is verified and the comment is restored, this is good to merge.

[jahala]

Reviewed by Opus 4.7:

Behavior parity confirmed — FPR target 0.01 preserved, builder-pattern equivalent to the old constructor, per-process random seed is fine for an in-memory cache keyed on path+mtime. The math is right and the swap is clean.

Test count discrepancy — confirm before merge

Your PR description says "342 pass (down from 344)". Running cargo test on the checked-out branch shows 338. Diff has two explicit deletions visible (test_bloom_filter_sizing and the constructor test), but the actual count is down 6, not 2.

Most likely explanation: the old hand-rolled BloomFilter struct had #[cfg(test)] mod tests { ... } content that disappeared along with the module, so the count delta includes those internal tests as well as the two visible top-level ones. That's correct behavior — those tests asserted on num_bits / num_hashes private fields that no longer exist on the opaque fastbloom::BloomFilter, and the equivalent guarantees are part of fastbloom's own test suite.

Could you confirm and update the PR description? A quick git diff main -- src/index/bloom.rs | grep -c "^+.*#\[test\]" vs the same for ^- would settle it. Just want to make sure the count drop isn't masking a regression somewhere unrelated.

Nit: lost context comment on filter sizing

build_filter now does:

let idents: Vec<&str> = extract_identifiers(content).collect();
let expected = idents.len().max(1);
let mut filter = BloomFilter::with_false_pos(0.01).expected_items(expected);
for ident in idents {
    filter.insert(ident);
}

idents contains many duplicates — same identifier (fn, let, use, i, j) appears repeatedly in any source file. So expected_items(idents.len()) over-sizes the filter relative to actual unique-identifier load, which means achieved FPR is much better than 1% in practice. That's fine (the filter is just slightly bigger than necessary), but the deleted comment "Count identifiers first to size the filter appropriately" was load-bearing context for a future reader. Restore something like // Sized for total token count, which over-allocates given duplicates — achieved FPR is < 0.01 in practice.

Once the test count is verified and the comment is restored, this is good to merge.

[paulnsorensen]

Verified before pushing — the 342 in the PR description is the total across all three suites; the 338 you ran into is the unit-tests-only line:

Suite	main	This PR	Δ
Lib unit	339	338	−1
Integration	4	4	0
Doctests	1	0	−1
Total	344	342	−2

So the −2 is exactly: −1 test_bloom_filter_sizing (asserted on the now-gone num_bits / num_hashes private fields) and −1 the BloomFilter::new doctest (removed with the constructor it documented). No regression hiding in the delta — PR description's "342 (down from 344)" stands.

Restored the filter-sizing comment in build_filter:

// Sized for total token count, not unique identifiers -- duplicates over-allocate
// the filter, so the achieved FPR is well below the 0.01 target in practice.

Also rebased onto latest main to clear the conflict with #86's thiserror addition. Single commit, force-pushed: 3ff87ca.