Skip to content

Commit

Permalink
all: implement proto2/proto3 as editions [1/2]
Browse files Browse the repository at this point in the history 
This change removes most usages of Syntax() from the repository and uses
edition features for instead. The appropriate edition feature defaults are
loaded for proto2/proto3 when the initialization of the descriptors
start.

All of these changes were tested on the Google corpus.

Change-Id: Ieca076a2b38ca8e50e084cd32e725b7b3dcb4171
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/572435
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Michael Stapelberg <[email protected]>
  • Loading branch information
@lfolger
lfolger committed Mar 19, 2024
1 parent f563685 commit 7259b46
Show file tree
Hide file tree
Showing 16 changed files with 129 additions and 114 deletions.
8 changes: 4 additions & 4 deletions cmd/protoc-gen-go/internal_gengo/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,11 +289,11 @@ func genEnum(g *protogen.GeneratedFile, f *fileInfo, e *enumInfo) {
genEnumReflectMethods(g, f, e)

// UnmarshalJSON method.
needsUnmarshalJSONMethod := e.genJSONMethod && e.Desc.Syntax() == protoreflect.Proto2
if fde, ok := e.Desc.(*filedesc.Enum); ok && fde.L1.EditionFeatures.GenerateLegacyUnmarshalJSON {
needsUnmarshalJSONMethod = true
needsUnmarshalJSONMethod := false
if fde, ok := e.Desc.(*filedesc.Enum); ok {
needsUnmarshalJSONMethod = fde.L1.EditionFeatures.GenerateLegacyUnmarshalJSON
}
if needsUnmarshalJSONMethod {
if e.genJSONMethod && needsUnmarshalJSONMethod {
g.P("// Deprecated: Do not use.")
g.P("func (x *", e.GoIdent, ") UnmarshalJSON(b []byte) error {")
g.P("num, err := ", protoimplPackage.Ident("X"), ".UnmarshalJSONEnum(x.Descriptor(), b)")
Expand Down
5 changes: 4 additions & 1 deletion internal/cmd/generate-protos/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ func main() {

func generateEditionsDefaults() {
dest := filepath.Join(repoRoot, "internal", "editiondefaults", "editions_defaults.binpb")
srcDescriptorProto := filepath.Join(protoRoot, "src", "google", "protobuf", "descriptor.proto")
srcGoFeatures := filepath.Join(repoRoot, "types", "gofeaturespb", "go_features.proto")
// The enum in Go string formats to "EDITION_${EDITION}" but protoc expects
// the flag in the form "${EDITION}". To work around this, we trim the
// "EDITION_" prefix.
Expand All @@ -113,7 +115,8 @@ func generateEditionsDefaults() {
"--experimental_edition_defaults_out", dest,
"--experimental_edition_defaults_minimum", minEdition,
"--experimental_edition_defaults_maximum", maxEdition,
"--proto_path", protoRoot, "src/google/protobuf/descriptor.proto",
"-I"+filepath.Join(protoRoot, "src"),
"--proto_path", repoRoot, srcDescriptorProto, srcGoFeatures,
)
out, err := cmd.CombinedOutput()
if err != nil {
Expand Down
Binary file modified internal/editiondefaults/editions_defaults.binpb
View file
Binary file not shown.
4 changes: 2 additions & 2 deletions internal/encoding/tag/tag.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ var byteType = reflect.TypeOf(byte(0))
func Unmarshal(tag string, goType reflect.Type, evs protoreflect.EnumValueDescriptors) protoreflect.FieldDescriptor {
f := new(filedesc.Field)
f.L0.ParentFile = filedesc.SurrogateProto2
f.L1.EditionFeatures = f.L0.ParentFile.L1.EditionFeatures
for len(tag) > 0 {
i := strings.IndexByte(tag, ',')
if i < 0 {
Expand Down Expand Up @@ -107,8 +108,7 @@ func Unmarshal(tag string, goType reflect.Type, evs protoreflect.EnumValueDescri
f.L1.StringName.InitJSON(jsonName)
}
case s == "packed":
f.L1.HasPacked = true
f.L1.IsPacked = true
f.L1.EditionFeatures.IsPacked = true
case strings.HasPrefix(s, "weak="):
f.L1.IsWeak = true
f.L1.Message = filedesc.PlaceholderMessage(protoreflect.FullName(s[len("weak="):]))
Expand Down
32 changes: 11 additions & 21 deletions internal/filedesc/desc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,10 +251,6 @@ type (
StringName stringName
IsProto3Optional bool // promoted from google.protobuf.FieldDescriptorProto
IsWeak bool // promoted from google.protobuf.FieldOptions
HasPacked bool // promoted from google.protobuf.FieldOptions
IsPacked bool // promoted from google.protobuf.FieldOptions
HasEnforceUTF8 bool // promoted from google.protobuf.FieldOptions
EnforceUTF8 bool // promoted from google.protobuf.FieldOptions
Default defaultValue
ContainingOneof protoreflect.OneofDescriptor // must be consistent with Message.Oneofs.Fields
Enum protoreflect.EnumDescriptor
Expand Down Expand Up @@ -345,14 +341,7 @@ func (fd *Field) IsPacked() bool {
case protoreflect.StringKind, protoreflect.BytesKind, protoreflect.MessageKind, protoreflect.GroupKind:
return false
}
if fd.L0.ParentFile.L1.Syntax == protoreflect.Editions {
return fd.L1.EditionFeatures.IsPacked
}
if fd.L0.ParentFile.L1.Syntax == protoreflect.Proto3 {
// proto3 repeated fields are packed by default.
return !fd.L1.HasPacked || fd.L1.IsPacked
}
return fd.L1.IsPacked
return fd.L1.EditionFeatures.IsPacked
}
func (fd *Field) IsExtension() bool { return false }
func (fd *Field) IsWeak() bool { return fd.L1.IsWeak }
Expand Down Expand Up @@ -399,13 +388,7 @@ func (fd *Field) ProtoType(protoreflect.FieldDescriptor) {}
// WARNING: This method is exempt from the compatibility promise and may be
// removed in the future without warning.
func (fd *Field) EnforceUTF8() bool {
if fd.L0.ParentFile.L1.Syntax == protoreflect.Editions {
return fd.L1.EditionFeatures.IsUTF8Validated
}
if fd.L1.HasEnforceUTF8 {
return fd.L1.EnforceUTF8
}
return fd.L0.ParentFile.L1.Syntax == protoreflect.Proto3
return fd.L1.EditionFeatures.IsUTF8Validated
}

func (od *Oneof) IsSynthetic() bool {
Expand Down Expand Up @@ -433,12 +416,19 @@ type (
Cardinality protoreflect.Cardinality
Kind protoreflect.Kind
EditionFeatures EditionFeatures
// To resolve the EditionFeatures we need to resolve the Extendee which
// happens at the end of the initialization of L1. Thus, we need to buffer
// the unresolved features (which are parsed when starting to initialize
// L1). We cannot move this to L2 because it is required to initialize
// Kind properly. Because some of the options (i.e. packed) affect the
// EditionFeatures we need to unmarshal the full options after resolving
// the Extendee.
rawOptions []byte
}
ExtensionL2 struct {
Options func() protoreflect.ProtoMessage
StringName stringName
IsProto3Optional bool // promoted from google.protobuf.FieldDescriptorProto
IsPacked bool // promoted from google.protobuf.FieldOptions
Default defaultValue
Enum protoreflect.EnumDescriptor
Message protoreflect.MessageDescriptor
Expand All @@ -461,7 +451,7 @@ func (xd *Extension) HasPresence() bool { return xd.L1.Cardi
func (xd *Extension) HasOptionalKeyword() bool {
return (xd.L0.ParentFile.L1.Syntax == protoreflect.Proto2 && xd.L1.Cardinality == protoreflect.Optional) || xd.lazyInit().IsProto3Optional
}
func (xd *Extension) IsPacked() bool { return xd.lazyInit().IsPacked }
func (xd *Extension) IsPacked() bool { return xd.L1.EditionFeatures.IsPacked }
func (xd *Extension) IsExtension() bool { return true }
func (xd *Extension) IsWeak() bool { return false }
func (xd *Extension) IsList() bool { return xd.Cardinality() == protoreflect.Repeated }
Expand Down
50 changes: 47 additions & 3 deletions internal/filedesc/desc_init.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,20 @@ func newRawFile(db Builder) *File {
for i := range fd.allExtensions {
xd := &fd.allExtensions[i]
xd.L1.Extendee = fd.resolveMessageDependency(xd.L1.Extendee, listExtTargets, int32(i))

// If the Extendee is not resolved, we cannot resolve the edition
// features of the parent. This should (to my knowledge) only happen
// for v1 messages for which we don't support editions. In v2 the
// Extendee should be resolved at binary start up time.
if _, ok := xd.L1.Extendee.(PlaceholderMessage); !ok {
xd.L1.EditionFeatures = featuresFromParentDesc(xd.L1.Extendee)
}
if xd.L1.rawOptions != nil {
xd.unmarshalOptions(xd.L1.rawOptions)
}
if xd.L1.Kind == protoreflect.MessageKind && xd.L1.EditionFeatures.IsDelimitedEncoded {
xd.L1.Kind = protoreflect.GroupKind
}
}

fd.checkDecls()
Expand Down Expand Up @@ -113,8 +127,10 @@ func (fd *File) unmarshalSeed(b []byte) {
switch string(v) {
case "proto2":
fd.L1.Syntax = protoreflect.Proto2
fd.L1.Edition = EditionProto2
case "proto3":
fd.L1.Syntax = protoreflect.Proto3
fd.L1.Edition = EditionProto3
case "editions":
fd.L1.Syntax = protoreflect.Editions
default:
Expand Down Expand Up @@ -177,11 +193,10 @@ func (fd *File) unmarshalSeed(b []byte) {
// If syntax is missing, it is assumed to be proto2.
if fd.L1.Syntax == 0 {
fd.L1.Syntax = protoreflect.Proto2
fd.L1.Edition = EditionProto2
}

if fd.L1.Syntax == protoreflect.Editions {
fd.L1.EditionFeatures = getFeaturesFor(fd.L1.Edition)
}
fd.L1.EditionFeatures = getFeaturesFor(fd.L1.Edition)

// Parse editions features from options if any
if options != nil {
Expand Down Expand Up @@ -267,6 +282,7 @@ func (ed *Enum) unmarshalSeed(b []byte, sb *strs.Builder, pf *File, pd protorefl
ed.L0.ParentFile = pf
ed.L0.Parent = pd
ed.L0.Index = i
ed.L1.EditionFeatures = featuresFromParentDesc(ed.Parent())

var numValues int
for b := b; len(b) > 0; {
Expand Down Expand Up @@ -467,6 +483,34 @@ func (xd *Extension) unmarshalSeed(b []byte, sb *strs.Builder, pf *File, pd prot
xd.L0.FullName = appendFullName(sb, pd.FullName(), v)
case genid.FieldDescriptorProto_Extendee_field_number:
xd.L1.Extendee = PlaceholderMessage(makeFullName(sb, v))
case genid.FieldDescriptorProto_Options_field_number:
xd.L1.rawOptions = v
}
default:
m := protowire.ConsumeFieldValue(num, typ, b)
b = b[m:]
}
}
}

func (xd *Extension) unmarshalOptions(b []byte) {
for len(b) > 0 {
num, typ, n := protowire.ConsumeTag(b)
b = b[n:]
switch typ {
case protowire.VarintType:
v, m := protowire.ConsumeVarint(b)
b = b[m:]
switch num {
case genid.FieldOptions_Packed_field_number:
xd.L1.EditionFeatures.IsPacked = protowire.DecodeBool(v)
}
case protowire.BytesType:
v, m := protowire.ConsumeBytes(b)
b = b[m:]
switch num {
case genid.FieldOptions_Features_field_number:
xd.L1.EditionFeatures = unmarshalFeatureSet(v, xd.L1.EditionFeatures)
}
default:
m := protowire.ConsumeFieldValue(num, typ, b)
Expand Down
44 changes: 4 additions & 40 deletions internal/filedesc/desc_lazy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -466,10 +466,10 @@ func (fd *Field) unmarshalFull(b []byte, sb *strs.Builder, pf *File, pd protoref
b = b[m:]
}
}
if fd.Syntax() == protoreflect.Editions && fd.L1.Kind == protoreflect.MessageKind && fd.L1.EditionFeatures.IsDelimitedEncoded {
if fd.L1.Kind == protoreflect.MessageKind && fd.L1.EditionFeatures.IsDelimitedEncoded {
fd.L1.Kind = protoreflect.GroupKind
}
if fd.Syntax() == protoreflect.Editions && fd.L1.EditionFeatures.IsLegacyRequired {
if fd.L1.EditionFeatures.IsLegacyRequired {
fd.L1.Cardinality = protoreflect.Required
}
if rawTypeName != nil {
Expand All @@ -496,13 +496,11 @@ func (fd *Field) unmarshalOptions(b []byte) {
b = b[m:]
switch num {
case genid.FieldOptions_Packed_field_number:
fd.L1.HasPacked = true
fd.L1.IsPacked = protowire.DecodeBool(v)
fd.L1.EditionFeatures.IsPacked = protowire.DecodeBool(v)
case genid.FieldOptions_Weak_field_number:
fd.L1.IsWeak = protowire.DecodeBool(v)
case FieldOptions_EnforceUTF8:
fd.L1.HasEnforceUTF8 = true
fd.L1.EnforceUTF8 = protowire.DecodeBool(v)
fd.L1.EditionFeatures.IsUTF8Validated = protowire.DecodeBool(v)
}
case protowire.BytesType:
v, m := protowire.ConsumeBytes(b)
Expand Down Expand Up @@ -548,7 +546,6 @@ func (od *Oneof) unmarshalFull(b []byte, sb *strs.Builder, pf *File, pd protoref
func (xd *Extension) unmarshalFull(b []byte, sb *strs.Builder) {
var rawTypeName []byte
var rawOptions []byte
xd.L1.EditionFeatures = featuresFromParentDesc(xd.L1.Extendee)
xd.L2 = new(ExtensionL2)
for len(b) > 0 {
num, typ, n := protowire.ConsumeTag(b)
Expand All @@ -572,20 +569,13 @@ func (xd *Extension) unmarshalFull(b []byte, sb *strs.Builder) {
case genid.FieldDescriptorProto_TypeName_field_number:
rawTypeName = v
case genid.FieldDescriptorProto_Options_field_number:
xd.unmarshalOptions(v)
rawOptions = appendOptions(rawOptions, v)
}
default:
m := protowire.ConsumeFieldValue(num, typ, b)
b = b[m:]
}
}
if xd.Syntax() == protoreflect.Editions && xd.L1.Kind == protoreflect.MessageKind && xd.L1.EditionFeatures.IsDelimitedEncoded {
xd.L1.Kind = protoreflect.GroupKind
}
if xd.Syntax() == protoreflect.Editions && xd.L1.EditionFeatures.IsLegacyRequired {
xd.L1.Cardinality = protoreflect.Required
}
if rawTypeName != nil {
name := makeFullName(sb, rawTypeName)
switch xd.L1.Kind {
Expand All @@ -598,32 +588,6 @@ func (xd *Extension) unmarshalFull(b []byte, sb *strs.Builder) {
xd.L2.Options = xd.L0.ParentFile.builder.optionsUnmarshaler(&descopts.Field, rawOptions)
}

func (xd *Extension) unmarshalOptions(b []byte) {
for len(b) > 0 {
num, typ, n := protowire.ConsumeTag(b)
b = b[n:]
switch typ {
case protowire.VarintType:
v, m := protowire.ConsumeVarint(b)
b = b[m:]
switch num {
case genid.FieldOptions_Packed_field_number:
xd.L2.IsPacked = protowire.DecodeBool(v)
}
case protowire.BytesType:
v, m := protowire.ConsumeBytes(b)
b = b[m:]
switch num {
case genid.FieldOptions_Features_field_number:
xd.L1.EditionFeatures = unmarshalFeatureSet(v, xd.L1.EditionFeatures)
}
default:
m := protowire.ConsumeFieldValue(num, typ, b)
b = b[m:]
}
}
}

func (sd *Service) unmarshalFull(b []byte, sb *strs.Builder) {
var rawMethods [][]byte
var rawOptions []byte
Expand Down
2 changes: 2 additions & 0 deletions internal/filedesc/editions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ var defaultsCache = make(map[Edition]EditionFeatures)

func init() {
unmarshalEditionDefaults(editiondefaults.Defaults)
SurrogateProto2.L1.EditionFeatures = getFeaturesFor(EditionProto2)
SurrogateProto3.L1.EditionFeatures = getFeaturesFor(EditionProto3)
}

func unmarshalGoFeature(b []byte, parent EditionFeatures) EditionFeatures {
Expand Down
1 change: 1 addition & 0 deletions internal/impl/legacy_enum.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ func aberrantLoadEnumDesc(t reflect.Type) protoreflect.EnumDescriptor {
ed := &filedesc.Enum{L2: new(filedesc.EnumL2)}
ed.L0.FullName = AberrantDeriveFullName(t) // e.g., github_com.user.repo.MyEnum
ed.L0.ParentFile = filedesc.SurrogateProto3
ed.L1.EditionFeatures = ed.L0.ParentFile.L1.EditionFeatures
ed.L2.Values.List = append(ed.L2.Values.List, filedesc.EnumValue{})

// TODO: Use the presence of a UnmarshalJSON method to determine proto2?
Expand Down
2 changes: 1 addition & 1 deletion internal/impl/legacy_extension.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ func (xi *ExtensionInfo) initFromLegacy() {
xd.L1.Number = protoreflect.FieldNumber(xi.Field)
xd.L1.Cardinality = fd.L1.Cardinality
xd.L1.Kind = fd.L1.Kind
xd.L2.IsPacked = fd.L1.IsPacked
xd.L1.EditionFeatures = fd.L1.EditionFeatures
xd.L2.Default = fd.L1.Default
xd.L1.Extendee = Export{}.MessageDescriptorOf(xi.ExtendedType)
xd.L2.Enum = ed
Expand Down
10 changes: 7 additions & 3 deletions internal/impl/legacy_message.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ func aberrantLoadMessageDescReentrant(t reflect.Type, name protoreflect.FullName
}
}

md.L1.EditionFeatures = md.L0.ParentFile.L1.EditionFeatures
// Obtain a list of oneof wrapper types.
var oneofWrappers []reflect.Type
methods := make([]reflect.Method, 0, 2)
Expand Down Expand Up @@ -250,6 +251,7 @@ func aberrantLoadMessageDescReentrant(t reflect.Type, name protoreflect.FullName
od := &md.L2.Oneofs.List[n]
od.L0.FullName = md.FullName().Append(protoreflect.Name(tag))
od.L0.ParentFile = md.L0.ParentFile
od.L1.EditionFeatures = md.L1.EditionFeatures
od.L0.Parent = md
od.L0.Index = n

Expand All @@ -260,6 +262,7 @@ func aberrantLoadMessageDescReentrant(t reflect.Type, name protoreflect.FullName
aberrantAppendField(md, f.Type, tag, "", "")
fd := &md.L2.Fields.List[len(md.L2.Fields.List)-1]
fd.L1.ContainingOneof = od
fd.L1.EditionFeatures = od.L1.EditionFeatures
od.L1.Fields.List = append(od.L1.Fields.List, fd)
}
}
Expand Down Expand Up @@ -307,14 +310,14 @@ func aberrantAppendField(md *filedesc.Message, goType reflect.Type, tag, tagKey,
fd.L0.Parent = md
fd.L0.Index = n

if fd.L1.IsWeak || fd.L1.HasPacked {
if fd.L1.IsWeak || fd.L1.EditionFeatures.IsPacked {
fd.L1.Options = func() protoreflect.ProtoMessage {
opts := descopts.Field.ProtoReflect().New()
if fd.L1.IsWeak {
opts.Set(opts.Descriptor().Fields().ByName("weak"), protoreflect.ValueOfBool(true))
}
if fd.L1.HasPacked {
opts.Set(opts.Descriptor().Fields().ByName("packed"), protoreflect.ValueOfBool(fd.L1.IsPacked))
if fd.L1.EditionFeatures.IsPacked {
opts.Set(opts.Descriptor().Fields().ByName("packed"), protoreflect.ValueOfBool(fd.L1.EditionFeatures.IsPacked))
}
return opts.Interface()
}
Expand Down Expand Up @@ -344,6 +347,7 @@ func aberrantAppendField(md *filedesc.Message, goType reflect.Type, tag, tagKey,
md2.L0.ParentFile = md.L0.ParentFile
md2.L0.Parent = md
md2.L0.Index = n
md2.L1.EditionFeatures = md.L1.EditionFeatures

md2.L1.IsMapEntry = true
md2.L2.Options = func() protoreflect.ProtoMessage {
Expand Down
0 internal/testprotos/race/race_test.go → ...nal/race_test/file_desc_init/race_test.go
View file
File renamed without changes.
Loading

0 comments on commit 7259b46

Please sign in to comment.