Skip to content

descriptor: expose the pool ordinal of MessageIndex, EnumIndex and ExtensionIndex - #438

Open
harshitwandhare wants to merge 1 commit into
anthropics:mainfrom
harshitwandhare:feat/expose-index-ordinals
Open

harshitwandhare wants to merge 1 commit into
anthropics:mainfrom
harshitwandhare:feat/expose-index-ordinals

Conversation

@harshitwandhare

Copy link
Copy Markdown
Contributor

Closes #433.

MessageIndex, EnumIndex and ExtensionIndex each gain index(self) -> usize, returning the ordinal within the pool that issued them. This is the first of the two options in the issue, which it calls the smaller one and enough on its own.

Why the ordinal is safe to document as dense. The pool stores each kind in a Vec keyed by exactly this newtype (messages, enums, extensions in pool.rs:610-620), and the existing messages() / enums() / extensions() accessors already document themselves as returning them "in pool index order". So the ordinal is the slice position, and exposing it publishes a property the pool already holds rather than adding a constraint it now has to maintain. The doc comments say it is meaningless across pools, matching the caveat the types already carry for comparison.

The count half of the ask turned out to be unnecessary. The issue also proposes DescriptorPool::message_count() "and friends so a side table can be sized up front". messages(), enums() and extensions() are already public and return slices, so pool.messages().len() sizes the table today. I left those out rather than adding three accessors that duplicate a len() call. Say the word if you would still rather have them.

Scope. ServiceIndex has the same shape and the same backing Vec, but the issue names three types, so I did not touch it. Happy to add it in this PR if you want the set complete.

Verification. Four tests in buffa-descriptor/tests/pool_e2e.rs, against the real protoc-built fixture pool (14 messages, 3 enums, 7 extensions, so none of the loops is vacuous). Three walk every descriptor of each kind, resolve its index back by full name, and assert index() equals the enumerate position. The fourth fills a Vec sized from messages().len() by ordinal and asserts every slot is occupied, which is the dense-over-0..len claim the docs make and the thing a future change to index assignment would break.

These cannot fail on main, since the method does not exist there; they fail to compile. What they guard is the density property going forward.

Gates run locally on d02c80f:

cargo fmt --all -- --check                              exit 0
cargo clippy --workspace --all-targets -- -D warnings   no warnings
cargo test --workspace                                  2853 passed, 0 failed
RUSTDOCFLAGS="-D warnings" cargo doc --no-deps -p buffa-descriptor --all-features   clean
RUSTUP_TOOLCHAIN=1.75 cargo check --workspace --all-targets --locked                exit 0

main at 053fcf3 is 2849 passed on the same command, so the delta is exactly the four new tests. The MSRV run used the workflow's own lockfile regeneration step (CARGO_RESOLVER_INCOMPATIBLE_RUST_VERSIONS=fallback, cargo generate-lockfile) and the checked-in Cargo.lock was restored afterwards, so it is unchanged here.

Changelog fragment added under .changes/unreleased/ rather than editing CHANGELOG.md. I do not have go-task installed, so the fragment was written by hand to match the existing files and changie merge was not run; that is the one gate I could not reproduce locally.

@github-actions

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

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.

Expose the ordinal of MessageIndex / EnumIndex / ExtensionIndex for dense side tables

1 participant