descriptor: validate service and method symbols#302
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
The change is user-visible and belongs in the release notes. No-Verification-Needed: changelog fragment only
The field-identity validation landed first and rewrote the same two `# Errors` sentences this branch does, so neither wording survives alone: the pool now rejects both duplicate symbols and ambiguous field identities, and the docs say so. The two branches also grew different rejection helpers under one name — this one takes a whole FileDescriptorSet and checks the enum, service and extension counters as well; the other builds a file around a single message. Both sets of tests need theirs, and the signatures are ambiguous together, so keep both with this one renamed to match what it takes.
iainmcgin
left a comment
There was a problem hiding this comment.
[claude code] Approving. Every rule here is safe to tighten — each rejected construct is invalid per protoc's single-symbol-space rule, so protoc rejects them at compile time and no protoc-emitted descriptor set can trip them. I checked the two places this could plausibly have gone wrong and both are right:
- Enum-value scoping. Values register at the enclosing scope (
{parent_fqn}.{value_name}), not nested under the enum, which is protoc's C++ scoping rule — and for a nested enumlink_enumis called with the message FQN, so a nested value correctly lands atpkg.M.V. allow_aliasstill works. The check is keyed on value names, andallow_aliaspermits duplicate numbers, so numeric aliases remain permitted. Worth stating because getting that wrong would break a common proto2 pattern silently.
The real gain is one this PR fixes rather than merely validates: link_service previously only checked service_by_name, so message Foo + service Foo in one package was accepted and built a pool where one shadowed the other. protoc rejects it. That's a correctness fix, not just stricter validation.
Two commits from me:
5baf648adds the changelog fragment — user-visible behaviour for 0.9.0. (check-changelogonly verifies CHANGELOG.md is in sync with existing fragments, so its passing wasn't evidence one wasn't needed.)ca8e4b5merges current main and reconciles the collision with #300, which landed a few minutes ago. Two things needed hand-resolution:- Both PRs rewrote the same two
# Errorssentences onDescriptorPool::new/decode. Neither wording survives alone — the pool now rejects duplicate symbols and ambiguous field identities and invalid oneof indices, so the docs say all three. - Both grew a rejection helper under the name
assert_rejected_without_mutating_pool, with different signatures (yours takes a wholeFileDescriptorSetand also checks the enum/service/extension counters; #300's builds a file around oneDescriptorProto). Same arity, different types, so they're ambiguous together — yours is renamedassert_set_rejected_without_mutating_pooland both are kept, since each PR's tests need their own. All 24 tests pass together.
- Both PRs rewrote the same two
One thing worth knowing for a follow-up, not for this PR: protoc's single symbol space also covers field and oneof names — M.foo as a field cannot collide with M.foo as a nested type. This PR registers nested types but not fields, and #300 checks field names only among sibling fields, so that particular collision is caught by neither. Genuinely out of scope here; noting it so it isn't assumed covered.
Thanks — the transactional staging held up under review too: every new failure path returns before the *self = staged commit, and your tests assert the baseline counters directly rather than trusting it.
What this does
Registers message, enum, service, method, extension, and enum-value names through one shared symbol registry. Duplicate RPC method names and duplicate enum value names now fail during pool construction, while numeric enum aliases remain unchanged.
DescriptorPool already stages additions, so invalid descriptor sets leave an existing pool unchanged.
Coverage
Validation
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspacecargo test -p buffa-descriptor --features reflectcargo check --workspace --all-features