Repository navigation
descriptor: reject the feature settings protoc rejects in editions files - #800
Open
veljjanoski wants to merge 2 commits into
Open
veljjanoski wants to merge 2 commits into
veljjanoski wants to merge 2 commits into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #633 (all of it except the proto2/proto3
featuresrule; see below).DescriptorPoollinked editions files that protoc 33.5 rejects inValidateFieldFeaturesandValidateFileFeatures(descriptor.cc). Each case is now a newPoolErrorvariant that names the field or file:TYPE_GROUPfield or extensionGroupFieldInEditionspackedoption, either valuePackedOptionInEditionsfeatures.field_presenceon a oneof member, a repeated or map field, or an extension;IMPLICITon a message fieldInvalidFieldPresencefeatures.utf8_validationon a field that is not a string field or a map with a string key or valueInvalidUtf8Validationfeatures.message_encodingon a map field or a non-message fieldInvalidMessageEncodingfeatures.field_presence = LEGACY_REQUIREDLegacyRequiredFileDefaultThe four protoc presence messages share one variant, as the two
repeated_field_encodingmessages already shareInvalidRepeatedFieldEncoding. An extension set toLEGACY_REQUIREDstill reportsRequiredExtension.Details that follow protoc:
repeated_field_encodingcheck, so a file default passes over the fields it does not fit. They skip thekeyandvaluefields of a map entry: protoc copies the map field's features onto them, somap<string, int32> m = 1 [features.utf8_validation = NONE]gives theint32value field that feature too.editionsand its edition is 2023 or later. protoc does not apply them to an earlier edition: a hand-built file with syntaxeditionsandEDITION_PROTO2is accepted with apackedoption and a group. A new privateis_editions_filesetsLinkScope::editions; an edition this crate does not define counts as a later one. The pool's older rules (therequiredlabel, the proto3 rules) still go by the syntax string; I left them as they are.PackedOptionInEditionsis checked beforeInvalidPackedOption, sopacked = trueon an editions string field reports the editions rule.I checked every case against protoc 33.5 with
.protofiles, and the edition gate with a hand-built descriptor set passed throughprotoc --descriptor_set_in.Left out: protoc also rejects any
featuresin a proto2 or proto3 file ("Features are only valid under editions."). The pool can't take that rule as it stands:buffa-codegen'sopen_enums_inandoverride_feature_inwritefeatures.enum_type = OPENinto proto2 enum and field descriptors, and withgenerate_reflectionthe generateddescriptor_pool()loads them (for examplebuffa-test'sopen_enums.protoconfigs), so it would start to panic. That needs a decision on how the overrides should reach the embedded pool, so I left it for you. protoc'sValidateFileFeaturesalso rejects thejava_string_check_utf8file option in editions files; #633 does not list it, so it is not in this PR either.Tests: new
buffa-descriptor/tests/pool_editions_features.rs, with rejected and accepted cases for each rule. The accepted ones are the neighbours protoc accepts: aDELIMITEDmessage field, presence on a singular field,utf8_validationon string fields and string maps,message_encodingon singular and repeated message fields,EXPLICIT/IMPLICITfile defaults, file defaults for all three features over every kind of field, and files with syntaxeditionsandEDITION_PROTO2orEDITION_PROTO3. Group fields cover extensions, edition 2024 and an undefined edition number, and one test checks that a rejected file leaves the pool unchanged. Each new condition was mutation-checked: disabling it fails a test.Run locally on Windows (
x86_64-pc-windows-gnu, toolchain 1.95.0, protoc 33.5):cargo fmt --all --checkcargo clippy --workspace --all-targets -- -D warnings, andcargo clippy -p buffa-descriptor --all-targets --all-features -- -D warningscargo test --workspace --all-features: all pass except twobuffa-typestests that fail on Windows only, becauseSystemTimethere has 100 ns resolutiontask doc(rustdoc-D warnings)buffa-descriptorcommands oftask check-nostdtask conformance: all eight runs pass with 0 unexpected failures, the reflect run included (it loads the protoc-built conformance protos, editions ones too, into aDescriptorPool)AI-usage disclosure: Claude (Anthropic) was used as assistant.