Skip to content

Validate usage of features in editions#264

Merged
jhump merged 4 commits intomainfrom
jh/validate-feature-usage
Apr 5, 2024
Merged

Validate usage of features in editions#264
jhump merged 4 commits intomainfrom
jh/validate-feature-usage

Conversation

@jhump
Copy link
Member

@jhump jhump commented Apr 3, 2024

This adds lots of validation of feature values, to mirror the logic performed in protoc.

This also adds some other validation checks that are implemented in protoc but were not previously implemented here in protocompile.

This would have been the last change needed to support editions in protocompile, except it looks like we'll also need to support extra validation of which features are allowed in which editions (see this protoc commit that just landed this week: protocolbuffers/protobuf@b09b3e4).

Since that change includes new stuff in descriptor.proto which isn't likely to make it into protobuf-go's descriptorpb package until v27 is released (or at least until the first release candidate is made available), we'll need to provide a stop-gap here to opt into editions support. I was hoping to just enable editions support in this repo once the work was complete; but now it won't be complete until after the above happens, yet we need to start working with the support that has been implemented so far in order to update logic in the buf CLI. So we'll have to expose the current internal flag to allow importing code (like the buf CLI) to opt into working with editions so we can implement and test editions-related things there.

Base automatically changed from jh/extension-declarations to main April 4, 2024 13:08
@jhump jhump marked this pull request as ready for review April 4, 2024 19:46
@jhump jhump requested a review from emcfarlane April 4, 2024 19:47
@jhump
Copy link
Member Author

jhump commented Apr 4, 2024

For context, here are the checks in the protoc C++ code base:

  1. Validate file options: https://github.com/protocolbuffers/protobuf/blob/d4525cb9edb196e3c52e75c0e4e88f14891b0c6f/src/google/protobuf/descriptor.cc#L7546
  2. Validate file-level features: https://github.com/protocolbuffers/protobuf/blob/d4525cb9edb196e3c52e75c0e4e88f14891b0c6f/src/google/protobuf/descriptor.cc#L7836
  3. Validate field optoins: https://github.com/protocolbuffers/protobuf/blob/d4525cb9edb196e3c52e75c0e4e88f14891b0c6f/src/google/protobuf/descriptor.cc#L7650
  4. Validate field-level features: https://github.com/protocolbuffers/protobuf/blob/d4525cb9edb196e3c52e75c0e4e88f14891b0c6f/src/google/protobuf/descriptor.cc#L7855

Notably different is how this ValidateOptions for files also does the proto3 syntax checks. But those aren't present here because those don't require interpreting options to perform, so protocompile does those much earlier, at the point of producing descriptor protos from the AST in the parser sub-package. The checks in this PR are all after options are interpreted.

One exception: this PR resurrects a check that is done in the parser, related to proto3 and enum values starting at zero. In a prior PR (#260), that check was removed in favor of the later check, which can also work with editions (which can't be done until after options are interpreted since features in editions are options). But in this PR, I've restored the check (very first commit) so that we can provide a more precise error message as well as detect the problem earlier.

Copy link
Contributor

@emcfarlane emcfarlane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@jhump jhump merged commit 1adabbd into main Apr 5, 2024
@jhump jhump deleted the jh/validate-feature-usage branch April 5, 2024 01:31
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