descriptor: reject missing reserved range bounds

[fallintoplace]

What changed

• Reject message and enum reserved ranges with a missing start or end bound.
• Keep complete ranges and the existing range normalization behavior unchanged.
• Return the original optional bounds in PoolError::InvalidReservedRange.

Tests

• cargo test --workspace
• cargo clippy --workspace --all-targets -- -D warnings
• cargo fmt --all -- --check
• task lint-md

[github-actions]

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

[iainmcgin]

[claude code]

Thanks for this one — the transactional tests and the borrow-only helper are good, but the presence check itself rejects descriptors that protoc and protobuf-go both accept, so it needs a different rule before it can go in.

Neither reference tests whether a bound is set; both read the proto2 default of 0. For enums, DescriptorBuilder::BuildReservedRange does result->start = proto.start(); result->end = proto.end(); and the only error is start > end (descriptor.cc, the EnumDescriptorProto::EnumReservedRange overload); protobuf-go builds the range from rr.GetStart()/rr.GetEnd() and rejects only !(Start() <= End()) (internal/filedesc/desc_list.go). Enum reserved values may legitimately be zero or negative, so a closed enum with values 10 and 11 and EnumReservedRange { end: Some(8) } (start unset → 0, i.e. reserved 0 to 8) builds cleanly upstream and is rejected here at pool.rs:1255; pool_e2e.rs:1357 enshrines the missing-start and missing-both enum cases as expected rejections, which are false positives. The message side happens to be a true positive, but for protoc's reasons rather than presence: unset start → 0 → "Reserved numbers must be positive integers."; unset end → 0 → start >= end → "Reserved range end number must be greater than start number."

That also points at the real gap, which the fragment claims but the diff does not cover: ReservedRanges::from_half_open (pool.rs:107 on main) silently drops any message range with start >= end or start <= 0, so {start: 9, end: 5}, {start: 0, end: 5} and {start: -3, end: 5} still pass validation and reserve nothing — all three are protoc errors.

Suggested rework, which stays small:

• Default a missing bound to 0, as both references do, then enforce protoc's per-kind rules: message start > 0 && start < end (end exclusive); enum start <= end (end inclusive).
• Split the variant by owner kind, matching descriptor: reject duplicate reserved names #419's DuplicateMessageReservedName / DuplicateEnumReservedName: InvalidMessageReservedRange { message, start, end } and InvalidEnumReservedRange { enum_name, start, end }. owner: String gives a matcher no way to tell which kind it names, and the two kinds have different end semantics. Print plain numbers in Display (the current arm renders start: None, end: Some(8), which leaks Rust syntax; neighbours print {start}..{end}).
• Update the comment at pool.rs:91 ("An unset bound cannot be honoured; protoc requires both") and the unset_or_empty_ranges_are_ignored unit test, both of which encode the old premise.
• Invert the two enum test cases into accept cases, add message cases for inverted / zero / negative start, and add (#418) to the fragment.

Ordering note for the eventual merge: #421 inserts at the same line in link_enum; protoc reports the empty-enum error before reserved-range errors, so EmptyEnum should stay above this check.

One forward note since you're working through protoc's validation list: protobuf-go rejects allow_alias = true with no actual alias, but protoc does not, so that one would be a false positive against protoc in the same way.

[iainmcgin]

[claude code] I've done the rework described above so this can ride 0.10.0 with its siblings (7dda356): ReservedRanges::for_message/for_enum now validate as they index, reading an unset bound as 0 — message ranges must satisfy 0 < start < end (InvalidMessageReservedRange), enum ranges start <= end (InvalidEnumReservedRange) — so the enum false positives are gone and the previously-dropped zero/negative/empty/reversed message ranges are now errors. Display prints plain bounds (unset for a missing one), the unit test and comment that encoded the old premise are replaced, the e2e tests cover both the accept and reject directions (including that ..8 on an enum really reserves 0–8), and the fragment cites (#418). Merged main; error-summary paragraphs keep main's wording. CI approved and running.