Skip to content

descriptor: reject empty enums - #421

Open
fallintoplace wants to merge 3 commits into
anthropics:mainfrom
fallintoplace:fix/reject-empty-enums
Open

descriptor: reject empty enums#421
fallintoplace wants to merge 3 commits into
anthropics:mainfrom
fallintoplace:fix/reject-empty-enums

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

What changed

  • Rejects empty enum declarations in DescriptorPool with PoolError::EmptyEnum.
  • Keeps failed pool additions transactional.
  • Keeps one-value closed enums with nonzero numbers valid.

Testing

  • cargo test -p buffa-descriptor --all-features --all-targets
  • cargo test --workspace
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo fmt --all --check

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

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

@iainmcgin

Copy link
Copy Markdown
Collaborator

[claude code]

Matches protoc ("Enums must contain at least one value.", checked before the first-value rule, which protoc guards with value_count() > 0) and protobuf-go, keeps a one-value closed enum with a nonzero number valid, and is transactional and nested-covered. CI is green. Two fixups:

  • open_enum_with_no_values_is_not_rejected_by_the_first_value_rule (pool_e2e.rs:436) still passes but is now vacuous — an empty enum can no longer reach the rule it guards, and its comment ("protoc rejects an empty enum for a different reason") is stale. Either fold it into the new test or change it to assert Err(PoolError::EmptyEnum { .. }), keeping the comment about why the first-value rule must not be the one that fires.
  • Add (#421) to the fragment.

And as on the sibling PRs: please drop the edit to the # Errors paragraphs on DescriptorPool::new / decode; we'll rewrite that sentence once after the series lands rather than merge eight competing versions of it. When #418 is reworked it inserts at the same line in link_enumEmptyEnum should stay first, which is protoc's order.

# Conflicts:
#	buffa-descriptor/src/pool.rs
… in the fragment

The test guarding the first-value rule against an empty enum became
vacuous once the pool rejects empty enums outright; it now asserts the
EmptyEnum error (and still that the first-value rule is not what fires).
Merged main; the DescriptorPool::new/decode error summaries keep main's
wording pending a single rewrite after this series.
@iainmcgin

Copy link
Copy Markdown
Collaborator

[claude code] No push since the review, so I've applied the two items myself so this can ride 0.10.0: c036353 merges main, turns the now-vacuous open_enum_with_no_values... test into an assertion that EmptyEnum (not the first-value rule) is what fires, adds (#421) to the fragment, and keeps main's wording for the DescriptorPool::new/decode error summaries (one rewrite after the series). CI approved and running.

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