Skip to content

Commit

Permalink
internal/filedesc: refactor editions feature resolution
Browse files Browse the repository at this point in the history 
Before this CL we only used features where they were applicable, e.g.
file features were only set on the file descriptor and field features
only on the fields descriptor. After this CL all descriptors (file,
message and field) contain always all features and when initializing
either of them we merge the features specified by options with the
features of the parent descriptor and use the result as the effective
feature. This way we initialize all the descriptors the same way and the
proto spec can change on which level features can be specified without
us having to change the code (at the moment most features can only be
specified on file or field level, in the future message might also be
possible).

Change-Id: I822202bf6a2bba5167bf9622c91a65c4523e78f9
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/555975
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
  • Loading branch information
@lfolger
lfolger committed Jan 16, 2024
1 parent 26a52f3 commit 9e454d6
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 90 deletions.
27 changes: 17 additions & 10 deletions internal/filedesc/desc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,15 @@ type (
Extensions Extensions
Services Services

EditionFeatures FileEditionFeatures
EditionFeatures EditionFeatures
}
FileL2 struct {
Options func() protoreflect.ProtoMessage
Imports FileImports
Locations SourceLocations
}

FileEditionFeatures struct {
EditionFeatures struct {
// IsFieldPresence is true if field_presence is EXPLICIT
// https://protobuf.dev/editions/features/#field_presence
IsFieldPresence bool
Expand Down Expand Up @@ -156,6 +156,8 @@ type (
}
EnumL1 struct {
eagerValues bool // controls whether EnumL2.Values is already populated

EditionFeatures EditionFeatures
}
EnumL2 struct {
Options func() protoreflect.ProtoMessage
Expand Down Expand Up @@ -217,6 +219,8 @@ type (
Extensions Extensions
IsMapEntry bool // promoted from google.protobuf.MessageOptions
IsMessageSet bool // promoted from google.protobuf.MessageOptions

EditionFeatures EditionFeatures
}
MessageL2 struct {
Options func() protoreflect.ProtoMessage
Expand Down Expand Up @@ -250,8 +254,7 @@ type (
Enum protoreflect.EnumDescriptor
Message protoreflect.MessageDescriptor

// Edition features.
Presence bool
EditionFeatures EditionFeatures
}

Oneof struct {
Expand All @@ -261,6 +264,8 @@ type (
OneofL1 struct {
Options func() protoreflect.ProtoMessage
Fields OneofFields // must be consistent with Message.Fields.ContainingOneof

EditionFeatures EditionFeatures
}
)

Expand Down Expand Up @@ -310,13 +315,15 @@ func (fd *Field) Options() protoreflect.ProtoMessage {
}
func (fd *Field) Number() protoreflect.FieldNumber { return fd.L1.Number }
func (fd *Field) Cardinality() protoreflect.Cardinality { return fd.L1.Cardinality }
func (fd *Field) Kind() protoreflect.Kind { return fd.L1.Kind }
func (fd *Field) HasJSONName() bool { return fd.L1.StringName.hasJSON }
func (fd *Field) JSONName() string { return fd.L1.StringName.getJSON(fd) }
func (fd *Field) TextName() string { return fd.L1.StringName.getText(fd) }
func (fd *Field) Kind() protoreflect.Kind {
return fd.L1.Kind
}
func (fd *Field) HasJSONName() bool { return fd.L1.StringName.hasJSON }
func (fd *Field) JSONName() string { return fd.L1.StringName.getJSON(fd) }
func (fd *Field) TextName() string { return fd.L1.StringName.getText(fd) }
func (fd *Field) HasPresence() bool {
if fd.L0.ParentFile.L1.Syntax == protoreflect.Editions {
return fd.L1.Presence || fd.L1.Message != nil || fd.L1.ContainingOneof != nil
if fd.Syntax() == protoreflect.Editions {
return fd.L1.EditionFeatures.IsFieldPresence || fd.L1.Message != nil || fd.L1.ContainingOneof != nil
}
return fd.L1.Cardinality != protoreflect.Repeated && (fd.L0.ParentFile.L1.Syntax == protoreflect.Proto2 || fd.L1.Message != nil || fd.L1.ContainingOneof != nil)
}
Expand Down
18 changes: 14 additions & 4 deletions reflect/protodesc/desc_init.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ func (r descsByName) initEnumDeclarations(eds []*descriptorpb.EnumDescriptorProt
protoreflect.EnumNumber(rr.GetEnd()),
})
}
if e.Base.L0.ParentFile.Syntax() == protoreflect.Editions {
e.L1.EditionFeatures = mergeEditionFeatures(parent, ed.GetOptions().GetFeatures())
}
if e.L2.Values.List, err = r.initEnumValuesFromDescriptorProto(ed.GetValue(), e, sb); err != nil {
return nil, err
}
Expand Down Expand Up @@ -68,6 +71,9 @@ func (r descsByName) initMessagesDeclarations(mds []*descriptorpb.DescriptorProt
if m.L0, err = r.makeBase(m, parent, md.GetName(), i, sb); err != nil {
return nil, err
}
if m.Base.L0.ParentFile.Syntax() == protoreflect.Editions {
m.L1.EditionFeatures = mergeEditionFeatures(parent, md.GetOptions().GetFeatures())
}
if opts := md.GetOptions(); opts != nil {
opts = proto.Clone(opts).(*descriptorpb.MessageOptions)
m.L2.Options = func() protoreflect.ProtoMessage { return opts }
Expand Down Expand Up @@ -160,12 +166,13 @@ func (r descsByName) initFieldsFromDescriptorProto(fds []*descriptorpb.FieldDesc
}

if f.Base.L0.ParentFile.Syntax() == protoreflect.Editions {
f.L1.Presence = resolveFeatureHasFieldPresence(f.Base.L0.ParentFile, fd)
f.L1.EditionFeatures = mergeEditionFeatures(parent, fd.GetOptions().GetFeatures())

// We reuse the existing field because the old option `[packed =
// true]` is mutually exclusive with the editions feature.
if canBePacked(fd) {
f.L1.HasPacked = true
f.L1.IsPacked = resolveFeatureRepeatedFieldEncodingPacked(f.Base.L0.ParentFile, fd)
f.L1.IsPacked = f.L1.EditionFeatures.IsPacked
}

// We pretend this option is always explicitly set because the only
Expand All @@ -176,9 +183,9 @@ func (r descsByName) initFieldsFromDescriptorProto(fds []*descriptorpb.FieldDesc
// requested from the descriptor).
// In proto2/proto3 syntax HasEnforceUTF8 might be false.
f.L1.HasEnforceUTF8 = true
f.L1.EnforceUTF8 = resolveFeatureEnforceUTF8(f.Base.L0.ParentFile, fd)
f.L1.EnforceUTF8 = f.L1.EditionFeatures.IsUTF8Validated

if f.L1.Kind == protoreflect.MessageKind && resolveFeatureDelimitedEncoding(f.Base.L0.ParentFile, fd) {
if f.L1.Kind == protoreflect.MessageKind && f.L1.EditionFeatures.IsDelimitedEncoded {
f.L1.Kind = protoreflect.GroupKind
}
}
Expand All @@ -196,6 +203,9 @@ func (r descsByName) initOneofsFromDescriptorProto(ods []*descriptorpb.OneofDesc
if opts := od.GetOptions(); opts != nil {
opts = proto.Clone(opts).(*descriptorpb.OneofOptions)
o.L1.Options = func() protoreflect.ProtoMessage { return opts }
if parent.Syntax() == protoreflect.Editions {
o.L1.EditionFeatures = mergeEditionFeatures(parent, opts.GetFeatures())
}
}
}
return os, nil
Expand Down
117 changes: 41 additions & 76 deletions reflect/protodesc/editions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

"google.golang.org/protobuf/internal/filedesc"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"
"google.golang.org/protobuf/types/descriptorpb"
)

Expand Down Expand Up @@ -83,37 +84,49 @@ func getFeatureSetFor(ed filedesc.Edition) *descriptorpb.FeatureSet {
return fs
}

func resolveFeatureHasFieldPresence(fileDesc *filedesc.File, fieldDesc *descriptorpb.FieldDescriptorProto) bool {
fs := fieldDesc.GetOptions().GetFeatures()
if fs == nil || fs.FieldPresence == nil {
return fileDesc.L1.EditionFeatures.IsFieldPresence
// mergeEditionFeatures merges the parent and child feature sets. This function
// should be used when initializing Go descriptors from descriptor protos which
// is why the parent is a filedesc.EditionsFeatures (Go representation) while
// the child is a descriptorproto.FeatureSet (protoc representation).
// Any feature set by the child overwrites what is set by the parent.
func mergeEditionFeatures(parentDesc protoreflect.Descriptor, child *descriptorpb.FeatureSet) filedesc.EditionFeatures {
var parentFS filedesc.EditionFeatures
switch p := parentDesc.(type) {
case *filedesc.File:
parentFS = p.L1.EditionFeatures
case *filedesc.Message:
parentFS = p.L1.EditionFeatures
default:
panic(fmt.Sprintf("unknown parent type %T", parentDesc))
}
if child == nil {
return parentFS
}
if fp := child.FieldPresence; fp != nil {
parentFS.IsFieldPresence = *fp == descriptorpb.FeatureSet_LEGACY_REQUIRED ||
*fp == descriptorpb.FeatureSet_EXPLICIT
}
if et := child.EnumType; et != nil {
parentFS.IsOpenEnum = *et == descriptorpb.FeatureSet_OPEN
}
return fs.GetFieldPresence() == descriptorpb.FeatureSet_LEGACY_REQUIRED ||
fs.GetFieldPresence() == descriptorpb.FeatureSet_EXPLICIT
}

func resolveFeatureRepeatedFieldEncodingPacked(fileDesc *filedesc.File, fieldDesc *descriptorpb.FieldDescriptorProto) bool {
fs := fieldDesc.GetOptions().GetFeatures()
if fs == nil || fs.RepeatedFieldEncoding == nil {
return fileDesc.L1.EditionFeatures.IsPacked
if rfe := child.RepeatedFieldEncoding; rfe != nil {
parentFS.IsPacked = *rfe == descriptorpb.FeatureSet_PACKED
}
return fs.GetRepeatedFieldEncoding() == descriptorpb.FeatureSet_PACKED
}

func resolveFeatureEnforceUTF8(fileDesc *filedesc.File, fieldDesc *descriptorpb.FieldDescriptorProto) bool {
fs := fieldDesc.GetOptions().GetFeatures()
if fs == nil || fs.Utf8Validation == nil {
return fileDesc.L1.EditionFeatures.IsUTF8Validated
if utf8val := child.Utf8Validation; utf8val != nil {
parentFS.IsUTF8Validated = *utf8val == descriptorpb.FeatureSet_VERIFY
}

if me := child.MessageEncoding; me != nil {
parentFS.IsDelimitedEncoded = *me == descriptorpb.FeatureSet_DELIMITED
}
return fs.GetUtf8Validation() == descriptorpb.FeatureSet_VERIFY
}

func resolveFeatureDelimitedEncoding(fileDesc *filedesc.File, fieldDesc *descriptorpb.FieldDescriptorProto) bool {
fs := fieldDesc.GetOptions().GetFeatures()
if fs == nil || fs.MessageEncoding == nil {
return fileDesc.L1.EditionFeatures.IsDelimitedEncoded
if jf := child.JsonFormat; jf != nil {
parentFS.IsJSONCompliant = *jf == descriptorpb.FeatureSet_ALLOW
}
return fs.GetMessageEncoding() == descriptorpb.FeatureSet_DELIMITED

return parentFS
}

// initFileDescFromFeatureSet initializes editions related fields in fd based
Expand All @@ -122,56 +135,8 @@ func resolveFeatureDelimitedEncoding(fileDesc *filedesc.File, fieldDesc *descrip
// before calling this function.
func initFileDescFromFeatureSet(fd *filedesc.File, fs *descriptorpb.FeatureSet) {
dfs := getFeatureSetFor(fd.L1.Edition)
if fs == nil {
fs = &descriptorpb.FeatureSet{}
}

var fieldPresence descriptorpb.FeatureSet_FieldPresence
if fp := fs.FieldPresence; fp != nil {
fieldPresence = *fp
} else {
fieldPresence = *dfs.FieldPresence
}
fd.L1.EditionFeatures.IsFieldPresence = fieldPresence == descriptorpb.FeatureSet_LEGACY_REQUIRED ||
fieldPresence == descriptorpb.FeatureSet_EXPLICIT

var enumType descriptorpb.FeatureSet_EnumType
if et := fs.EnumType; et != nil {
enumType = *et
} else {
enumType = *dfs.EnumType
}
fd.L1.EditionFeatures.IsOpenEnum = enumType == descriptorpb.FeatureSet_OPEN

var respeatedFieldEncoding descriptorpb.FeatureSet_RepeatedFieldEncoding
if rfe := fs.RepeatedFieldEncoding; rfe != nil {
respeatedFieldEncoding = *rfe
} else {
respeatedFieldEncoding = *dfs.RepeatedFieldEncoding
}
fd.L1.EditionFeatures.IsPacked = respeatedFieldEncoding == descriptorpb.FeatureSet_PACKED

var isUTF8Validated descriptorpb.FeatureSet_Utf8Validation
if utf8val := fs.Utf8Validation; utf8val != nil {
isUTF8Validated = *utf8val
} else {
isUTF8Validated = *dfs.Utf8Validation
}
fd.L1.EditionFeatures.IsUTF8Validated = isUTF8Validated == descriptorpb.FeatureSet_VERIFY

var messageEncoding descriptorpb.FeatureSet_MessageEncoding
if me := fs.MessageEncoding; me != nil {
messageEncoding = *me
} else {
messageEncoding = *dfs.MessageEncoding
}
fd.L1.EditionFeatures.IsDelimitedEncoded = messageEncoding == descriptorpb.FeatureSet_DELIMITED

var jsonFormat descriptorpb.FeatureSet_JsonFormat
if jf := fs.JsonFormat; jf != nil {
jsonFormat = *jf
} else {
jsonFormat = *dfs.JsonFormat
}
fd.L1.EditionFeatures.IsJSONCompliant = jsonFormat == descriptorpb.FeatureSet_ALLOW
// initialize the featureset with the defaults
fd.L1.EditionFeatures = mergeEditionFeatures(fd, dfs)
// overwrite any options explicitly specified
fd.L1.EditionFeatures = mergeEditionFeatures(fd, fs)
}

0 comments on commit 9e454d6

Please sign in to comment.