Skip to content

Draft: TriHashMap Entry API#290

Draft
SG-devel wants to merge 13 commits into
oxidecomputer:mainfrom
SG-devel:tri-entry-api
Draft

Draft: TriHashMap Entry API#290
SG-devel wants to merge 13 commits into
oxidecomputer:mainfrom
SG-devel:tri-entry-api

Conversation

@SG-devel

@SG-devel SG-devel commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Draft: I am opening this for maintainer feedback and discussion before converting it to a ready-for-review PR. The implementation follows the proposal in issue #7, but I would appreciate feedback on the API shape, scope, and semantics before continuing work and asking for formal review.

Summary

Implements the TriHashMap Entry API.

This adds a Tri-specific public Entry API while keeping the shared correctness-critical lookup-state logic internal. The API mirrors the existing BiHashMap Entry style where appropriate, but uses accessor-backed non-unique views for TriHashMap so overlapping key matches such as A / A / B and A / B / A preserve per-key mapping without exposing aliased mutable references.

This implements the proposal from issue #7, with the concrete API shape shown below.

Commit structure

This PR is intentionally split into reviewable commits:

  1. Prepare TriHashMap internals for Entry support

    • make TriHashMap::tables visibility match BiHashMap
    • normalize Tri hash helper naming
  2. Add shared internal Entry lookup state

    • add support::entry
    • classify vacant / unique / non-unique states
    • preserve first-key-hit distinct ordering and key-to-slot mapping
  3. Migrate BiHashMap internals to shared lookup state

    • preserve BiHashMap public API and behavior
  4. Add initial TriHashMap Entry API

    • add Entry, VacantEntry, OccupiedEntry, OccupiedEntryRef, NonUniqueEntryRef
    • add TriHashMap::entry
    • add vacant insertion and shared/read-only access
  5. Add mutable occupied Entry views

    • add OccupiedEntryMut and NonUniqueEntryMut
    • use accessor-backed mutable non-unique access
    • add and_modify
  6. Add occupied Entry mutation

    • add remove
    • add insert
    • add or_insert and or_insert_with
    • validate replacement state before mutation
    • preserve first-key-hit return ordering
  7. Document and harden the API

    • add rustdoc and doctests
    • document partial/mixed occupied states
    • document non-unique access and mutation semantics
    • harden tests
  8. Clean up TriHashMap entry debug output

    • remove stale internal dead-code helper
    • make non-unique entry debug output use the public per-key view
  9. Test repeated non-unique key mapping

    • cover A / B / A mapping
    • assert per-key access maps key1/key3 to A and key2 to B
    • assert for_each visits distinct items once in first-key-hit order
  10. Add model-based TriHashMap Entry mutation coverage

    • add NaiveMap oracle support for TRI entry remove and replacement
    • add property-based TRI entry remove and compatible replacement operations
    • generate vacant, unique, partial, and mixed occupied entry states
    • preserve and check TRI's first-key-hit return order
    • add missing insert_entry mismatch, unique occupied, and or_insert_with laziness tests
    • clarify hash-validation docs and improve entry-related panic messages
  11. Merge remote-tracking branch 'origin/main' into tri-entry-api after switch to Hegel as PBT framework

  12. Consolidate tri entry shared-access coverage

    • replace the narrow repeated-key regression test with broader table-driven shared-access coverage for non-unique TriHashMap entries.
    • new cases keep the A/B/A repeated-key mapping behavior while also checking partial hits, mixed hits, per-key accessor mapping, and first-key-hit iteration order in one place.
  13. Restore unrelated files from main

    • Restore pathological.rs and panic_safety.rs to the main versions. These files had unrelated lint/style changes that are not needed for the entry API implementation or tests.

Important semantics

For TriHashMap::entry(key1, key2, key3):

  • None / None / None is vacant.
  • A / A / A is occupied unique.
  • Partial hits such as A / A / None, A / None / A, and None / A / A are occupied non-unique.
  • Mixed hits such as A / A / B, A / B / A, and A / B / C are occupied non-unique.

Non-unique access preserves per-key mapping. For example:

  • in A / A / B, key1 and key2 both map to A, while key3 maps to B;
  • in A / B / A, key1 and key3 both map to A, while key2 maps to B.

Mutable non-unique access is accessor-backed and stores one mutable reference per distinct matched item, preventing the public API from exposing aliased mutable references.

Removal, replacement, and visitor-style APIs process distinct matched items in deterministic first-key-hit order.

Testing notes

The entry mutation behavior is covered with model-based property tests.

The real TriHashMap is tested against NaiveMap, a simple vector-backed oracle. For each generated operation, the test applies the operation to both implementations, compares the externally visible result, and then validates the real map's internal invariants.

For TRI entry coverage, the property tests include:

  • EntryRemove(UniqueKeysOp), which resolves entry keys from the current oracle state to generate vacant, partial, mixed, and unique occupied states.
  • EntryInsertOverwrite(EntryInsertOverwriteOp), which resolves entry keys the same way, then constructs a replacement item using exactly those keys so the property test exercises compatible occupied replacement rather than mismatch panics.

The oracle preserves TRI's documented first-key-hit return order while removing from its backing vector in descending physical index order to avoid shifted-index artifacts.

This is intended to give TRI entry mutation coverage comparable to the existing BI entry API property coverage while preserving TRI-specific ordering semantics.

Validation

I ran locally:

cargo fmt --check
cargo xfmt --check

cargo check
cargo check -p iddqd --no-default-features

cargo clippy --all-targets --all-features -- -D warnings

cargo test -p iddqd --doc
cargo nextest run -p iddqd

PROPTEST_CASES=10000 cargo nextest run -p iddqd
bi_hash_map::proptest_ops
tri_hash_map::proptest_ops

PROPTEST_CASES=10000 cargo nextest run -p iddqd
bi_hash_map::proptest_panic_safety::proptest_panic_ops
tri_hash_map::proptest_panic_safety::proptest_panic_ops

PROPTEST_CASES=10000 cargo nextest run -p iddqd bi_hash_map tri_hash_map

Notes

The implementation keeps the public API arity-specific. It does not introduce a generic public Entry framework.

The shared lookup-state primitive remains crate-internal and uses the existing internal item-index representation.

Contribution

I submit this contribution under the repository’s existing license terms, MIT OR Apache-2.0, without any warranty.

AI assistance disclosure: I used ChatGPT (GPT-5.5 Thinking, Codex) throughout development of this patch, including reasoning about the issue, writing code and tests, reviewing the implementation, and writing PR text. I guided the work and ran the listed checks.

SG-devel added 10 commits June 20, 2026 12:30
Remove a stale dead-code helper from support::entry and make TriHashMap non-unique entry Debug output use the public per-key view instead of internal slot-mapping fields.
Add model-based property tests for TriHashMap entry insertion and
removal, matching the BiHashMap entry API coverage while preserving
TRI's first-key-hit removal order.

Extend the naive test oracle with TRI entry insert/remove behavior,
including partial and mixed occupied entries. Also cover vacant
insert_entry hash mismatch panics, exact unique occupied entry
insert/remove cases, and or_insert_with laziness for unique and repeated
mixed entries.

Tighten TRI entry documentation around hash validation and improve
entry-related panic messages.
@SG-devel

Copy link
Copy Markdown
Contributor Author

Force-pushed to normalize commit author/committer metadata to my GitHub noreply address. This was only a history/metadata cleanup; no code changes were intended as part of the rewrite.

@sunshowers

Copy link
Copy Markdown
Collaborator

Thanks for doing this. FYI it's going to take me a few more days to review this, since I have higher-priority work on my plate.

Apologies for the merge conflicts -- you're welcome to fix them (it's a switch to Hegel as our PBT framework) or I can.

SG-devel added 3 commits June 28, 2026 11:48
Replace the narrow repeated-key regression test with broader table-driven
shared-access coverage for non-unique TriHashMap entries.

The new cases keep the A/B/A repeated-key mapping behavior while also
checking partial hits, mixed hits, per-key accessor mapping, and first-key-hit
iteration order in one place.
Restore pathological.rs and panic_safety.rs to the upstream main
versions. These files had unrelated lint/style changes that are
not needed for the entry API implementation or tests.
@SG-devel

Copy link
Copy Markdown
Contributor Author

Thanks, no problem. I’ve merged current main, ported the property tests to Hegel, and restored and consolidated the entry API coverage. I also cleaned up two unrelated file diffs so the PR stays focused. No rush on the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants