Expose experimental edition features in descriptor sets

[timostamm]

This change exposes edition feature sets in a DescriptorSet, the wrapper around protobuf file descriptor messages provided by createDescriptorSet from @bufbuild/protobuf.

Each type (DescFile, DescMessage, DescField, etc.) receives a new method with this PR:

  /**
   * Get the edition features for this protobuf element.
   */
  getFeatures(): MergedFeatureSet;

The returned type is a protobuf message google.protobuf.FeatureSet, but with a modified type that marks all properties as required.

The value is resolved from the edition feature defaults (obtained at build time from protoc in #619), merged with the features of all parent elements and the current element. This implements the logic specified in the design documents for editions here and here.

Users can provide their own edition feature defaults via a new optional argument to createDescriptorSet. This can be useful for language-specific features, although usability is currently limited because we do not support extensions yet (tracked in #86), which makes it difficult to obtain the features at runtime.

This PR also updates the properties packed and packedByDefault of DescField (and DescExtension) to provide sensible values for editions that respect features. Test coverage for proto2 and proto3 has been added prior to this PR in #625 to guard against regressions.

[jhump]

I have some questions and comments, but mostly LGTM.

[jhump]

Perhaps it doesn't matter since it seems to have been this way before, too, but I think this and the function above are missing a check that proto.label == LABEL_REPEATED.

Also, this default case seems a bit more verbose than I would have expected. I would instead have expected simply:

return proto.options?.packed ?? isPackedFieldByDefault(file, proto, resolveFeatures)

[timostamm]

Agree that it would make sense to check for LABEL_REPEATED. Didn't want to change behavior in this PR.

Also, this default case seems a bit more verbose than I would have expected.

Yes, I wonder how this will play out for other implementations. But the field packed of FieldOptions is indeed not populated at all with editions.

[jhump]

This will cause a compile issue if a new feature is added and the resulting generated code is paired with an older version of the runtime, right (since the code may be populating a MergedFeatureSet without setting all required fields)? I guess that's probably desirable.

[timostamm]

If a new feature is added, I expect this to happen with the release of a new edition. For an upstream release, we generate the WKT, and also seed the edition defaults (see #619) at build time. The function createDescriptorSet uses the seeded defaults. As a result, createDescriptorSet can now parse descriptors including the new edition. The message FeatureSet will be in sync, since it is part of the WKT.

createDescriptorSet may have to be updated to provide a good user experience around a new feature. Then the runtime will have implement this feature. Then we can release full support for the new edition. It is possible to cut a release before we support the new future in the runtime, and only make it available in createDescriptorSet, but not in protoc-gen-es. We can sort out the exact details if we get there.

If createDescriptorSet is called with descriptors using an edition from the future, createFeatureResolver will raise an error because the edition is not supported yet. createDescriptorSet takes an optional argument to bring your own FeatureSetDefaults, allowing to use editions other than the ones supported by the @bufbuild/protobuf release. The getFeatures() methods will return FeatureSet classes (and still be valid MergedFeatureSet because it seems very unlikely that a feature will ever be removed from descriptors).

The optional argument to bring your own FeatureSetDefaults can also be used to provide language-specific extensions to the protobuf message FeatureSet. It would be a nice addition to @bufbuild/protoplugin to support this use case, but we really ought to support extensions first.

It's unfortunate that feature resolution is effectively split into build-time and runtime concerns. The logic could certainly be made completely dynamic (and #628 partially gets us there). But since we currently don't support extensions and don't embed descriptors in generated code, implementation is complex and DX is awkward. Since it will also affect bundle size, I think we're better off punting on this now, and reconsider the feature later on.

[jhump]

Won't the ?? 0 cause comparison issues below? IIUC, I think a zero for max will cause this to always generate an error like Edition <edition> is later than the maximum supported edition EDITION_UNKNOWN, which could be confusing. Though I suppose it is also possible that the call to Edition[edition] to build the error message could also raise an exception.

Maybe a more explicit check with a better error message about the feature set defaults being incomplete/corrupt or something?

[timostamm]

I kept this pretty close to the cc implementation, but had the same thoughts. Pushed up 3bc7274 with a little more validation.

For a google.protobuf.FeatureSetDefaults from the future, our generated Editions enumeration may be missing values. In this case, the error messages will be "Edition undefined is earlier ...". Not ideal, but the alternatives seem barely better, or prohibitively complex.

[jhump]

So if we pair this set of fixed properties with a newer FeatureSet definition, we'll end up violating the type instead of this being an error, right?

Perhaps better would be to a constant literal of type MergedFeatureSet, so the compiler can verify it is correct (e.g. has all required properties), and then here loop over the keys of the constant.

[timostamm]

Thanks for raising this question, I was meaning to take a closer look at forward compatibility. In 169354c, I'm adding the following comments to this function:

// When protoc generates google.protobuf.FeatureSetDefaults, it ensures that
// fields are not repeated or required, do not use oneof, and have a default
// value.
//
// When features for an element are resolved, features of the element and its
// parents are merged into the default FeatureSet for the edition. Because unset
// fields in the FeatureSet of an element do not unset the default FeatureSet
// values, a resolved FeatureSet is guaranteed to have all fields set. This is
// also the case for extensions to FeatureSet that a user might provide, and for
// features from the future.
//
// We cannot exhaustively validate correctness of FeatureSetDefaults at runtime
// without knowing the schema: If no value for a feature is provided, we do not
// know that it exists at all.
//
// As a sanity check, we validate that all fields known to our version of
// FeatureSet are set.

The gist is that I don't see an alternative to trusting the FeatureSetDefaults to be valid. If the defaults are valid, feature resolution will always yield a message with all fields set.

In ad1965d, I've updated the logic to use the field information embedded in the generated code to check fields. So this sanity check will always include all known fields.

[jhump]

(I'm still ooo, but happened to see the message in Slack so figured I could do a quick scan.)