field: fix missing optional support (#95)

Author: snqk

Diff for: .travis.yml

@@ -4,8 +4,9 @@ go_import_path: github.com/lyft/protoc-gen-star
 
 env:
   matrix:
-    - PROTOC_VER="3.5.1"
-    - PROTOC_VER="3.6.1"
+    - PROTOC_VER="3.5.0"
+    - PROTOC_VER="3.6.0"
+    - PROTOC_VER="3.17.0"
 
 before_install:
   - mkdir -p $GOPATH/bin

Diff for: Makefile

@@ -1,5 +1,6 @@
 # the name of this package
 PKG  := $(shell go list .)
+PROTOC_VER := $(shell protoc --version | cut -d' ' -f2)
 
 .PHONY: bootstrap
 bootstrap: testdata # set up the project for development
@@ -16,15 +17,27 @@ lint: # lints the package for common code smells
 
 .PHONY: quick
 quick: testdata # runs all tests without the race detector or coverage
+ifeq ($(PROTOC_VER), 3.17.0)
+	go test $(PKGS) --tags=proto3_presence
+else
 	go test $(PKGS)
+endif
 
 .PHONY: tests
 tests: testdata # runs all tests against the package with race detection and coverage percentage
+ifeq ($(PROTOC_VER), 3.17.0)
+	go test -race -cover ./... --tags=proto3_presence
+else
 	go test -race -cover ./...
+endif
 
 .PHONY: cover
 cover: testdata # runs all tests against the package, generating a coverage report and opening it in the browser
+ifeq ($(PROTOC_VER), 3.17.0)
+	go test -race -covermode=atomic -coverprofile=cover.out ./... --tags=proto3_presence || true
+else
 	go test -race -covermode=atomic -coverprofile=cover.out ./... || true
+endif
 	go tool cover -html cover.out -o cover.html
 	open cover.html
 
@@ -76,6 +89,10 @@ testdata-go: protoc-gen-go bin/protoc-gen-debug # generate go-specific testdata
 		testdata-names \
 		testdata-packages \
 		testdata-outputs
+ifeq ($(PROTOC_VER), 3.17.0)
+	cd lang/go && $(MAKE) \
+		testdata-presence
+endif
 
 vendor: # install project dependencies
 	which glide || (curl https://glide.sh/get | sh)

Diff for: field.go

@@ -17,16 +17,29 @@ type Field interface {
 	Message() Message
 
 	// InOneOf returns true if the field is in a OneOf of the parent Message.
+	// This will return true for synthetic oneofs (proto3 field presence) as well.
 	InOneOf() bool
 
-	// OneOf returns the OneOf that this field is apart of. Nil is returned if
+	// InRealOneOf returns true if the field is in a OneOf of the parent Message.
+	// This will return false for synthetic oneofs, and will only include 'real' oneofs.
+	// See: https://github.com/protocolbuffers/protobuf/blob/v3.17.0/docs/field_presence.md
+	InRealOneOf() bool
+
+	// OneOf returns the OneOf that this field is a part of. Nil is returned if
 	// the field is not within a OneOf.
 	OneOf() OneOf
 
 	// Type returns the FieldType of this Field.
 	Type() FieldType
 
-	// Required returns whether or not the field is labeled as required. This
+	// HasPresence returns true for all fields that have explicit presence as defined by:
+	// See: https://github.com/protocolbuffers/protobuf/blob/v3.17.0/docs/field_presence.md
+	HasPresence() bool
+
+	// HasOptionalKeyword returns whether the field is labeled as optional.
+	HasOptionalKeyword() bool
+
+	// Required returns whether the field is labeled as required. This
 	// will only be true if the syntax is proto2.
 	Required() bool
 
@@ -61,6 +74,35 @@ func (f *field) Type() FieldType                              { return f.typ }
 func (f *field) setMessage(m Message)                         { f.msg = m }
 func (f *field) setOneOf(o OneOf)                             { f.oneof = o }
 
+func (f *field) InRealOneOf() bool {
+	return f.InOneOf() && !f.desc.GetProto3Optional()
+}
+
+func (f *field) HasPresence() bool {
+	if f.InOneOf() {
+		return true
+	}
+
+	if f.Type().IsEmbed() {
+		return true
+	}
+
+	if !f.Type().IsRepeated() && !f.Type().IsMap() {
+		if f.Syntax() == Proto2 {
+			return true
+		}
+		return f.HasOptionalKeyword()
+	}
+	return false
+}
+
+func (f *field) HasOptionalKeyword() bool {
+	if f.Syntax() == Proto3 {
+		return f.desc.GetProto3Optional()
+	}
+	return f.desc.GetLabel() == descriptor.FieldDescriptorProto_LABEL_OPTIONAL
+}
+
 func (f *field) Required() bool {
 	return f.Syntax().SupportsRequiredPrefix() &&
 		f.desc.GetLabel() == descriptor.FieldDescriptorProto_LABEL_REQUIRED

Diff for: field_test.go

@@ -99,6 +99,58 @@ func TestField_OneOf(t *testing.T) {
 	assert.True(t, f.InOneOf())
 }
 
+func TestField_InRealOneOf(t *testing.T) {
+	t.Parallel()
+
+	f := dummyField()
+	assert.False(t, f.InRealOneOf())
+
+	f = dummyOneOfField(false)
+	assert.True(t, f.InRealOneOf())
+
+	f = dummyOneOfField(true)
+	assert.False(t, f.InRealOneOf())
+}
+
+func TestField_HasPresence(t *testing.T) {
+	t.Parallel()
+
+	f := dummyField()
+	f.addType(&repT{scalarT: &scalarT{}})
+	assert.False(t, f.HasPresence())
+
+	f.addType(&mapT{repT: &repT{scalarT: &scalarT{}}})
+	assert.False(t, f.HasPresence())
+
+	f.addType(&scalarT{})
+	assert.False(t, f.HasPresence())
+
+	opt := true
+	f.desc = &descriptor.FieldDescriptorProto{Proto3Optional: &opt}
+	assert.True(t, f.HasPresence())
+}
+
+func TestField_HasOptionalKeyword(t *testing.T) {
+	t.Parallel()
+
+	optLabel := descriptor.FieldDescriptorProto_LABEL_OPTIONAL
+
+	f := &field{msg: &msg{parent: dummyFile()}}
+	assert.False(t, f.HasOptionalKeyword())
+
+	f.desc = &descriptor.FieldDescriptorProto{Label: &optLabel}
+	assert.False(t, f.HasOptionalKeyword())
+
+	f = dummyField()
+	assert.False(t, f.HasOptionalKeyword())
+
+	f = dummyOneOfField(false)
+	assert.False(t, f.HasOptionalKeyword())
+
+	f = dummyOneOfField(true)
+	assert.True(t, f.HasOptionalKeyword())
+}
+
 func TestField_Type(t *testing.T) {
 	t.Parallel()
 
@@ -194,3 +246,23 @@ func dummyField() *field {
 	f.addType(t)
 	return f
 }
+
+func dummyOneOfField(synthetic bool) *field {
+	m := dummyMsg()
+	o := dummyOneof()
+	str := descriptor.FieldDescriptorProto_TYPE_STRING
+	var oIndex int32
+	oIndex = 1
+	f := &field{desc: &descriptor.FieldDescriptorProto{
+		Name:           proto.String("field"),
+		Type:           &str,
+		OneofIndex:     &oIndex,
+		Proto3Optional: &synthetic,
+	}}
+	o.addField(f)
+	m.addField(f)
+	m.addOneOf(o)
+	t := &scalarT{}
+	f.addType(t)
+	return f
+}

Diff for: field_type.go

@@ -22,8 +22,7 @@ type FieldType interface {
 	// repeated fields containing embeds will still return false.
 	IsEmbed() bool
 
-	// IsOptional returns true if the message's syntax is not Proto2 or
-	// the field is prefixed as optional.
+	// IsOptional returns true if the field is prefixed as optional.
 	IsOptional() bool
 
 	// IsRequired returns true if and only if the field is prefixed as required.

Diff for: field_type_test.go

@@ -88,6 +88,27 @@ func TestScalarT_Key(t *testing.T) {
 func TestScalarT_IsOptional(t *testing.T) {
 	t.Parallel()
 
+	s := &scalarT{}
+	f := dummyOneOfField(true)
+	f.addType(s)
+
+	assert.True(t, s.IsOptional())
+
+	fl := dummyFile()
+	fl.desc.Syntax = nil
+	f.Message().setParent(fl)
+
+	assert.True(t, s.IsOptional())
+
+	req := descriptor.FieldDescriptorProto_LABEL_REQUIRED
+	f.desc.Label = &req
+
+	assert.False(t, s.IsOptional())
+}
+
+func TestScalarT_IsNotOptional(t *testing.T) {
+	t.Parallel()
+
 	s := &scalarT{}
 	f := dummyField()
 	f.addType(s)

Diff for: go.mod

@@ -6,4 +6,5 @@ require (
 	github.com/golang/protobuf v1.5.2
 	github.com/spf13/afero v1.3.3
 	github.com/stretchr/testify v1.6.1
+	google.golang.org/protobuf v1.26.0 // indirect
 )

Diff for: init_option.go

@@ -47,3 +47,11 @@ func FileSystem(fs afero.Fs) InitOption { return func(g *Generator) { g.persiste
 func BiDirectional() InitOption {
 	return func(g *Generator) { g.workflow = &onceWorkflow{workflow: &standardWorkflow{BiDi: true}} }
 }
+
+// SupportedFeatures allows defining protoc features to enable / disable.
+// See: https://github.com/protocolbuffers/protobuf/blob/v3.17.0/docs/implementing_proto3_presence.md#signaling-that-your-code-generator-supports-proto3-optional
+func SupportedFeatures(feat *uint64) InitOption {
+	return func(g *Generator) {
+		g.persister.SetSupportedFeatures(feat)
+	}
+}

Diff for: lang/go/Makefile

@@ -38,5 +38,18 @@ testdata-outputs: ../../bin/protoc-gen-debug
 		cd -; \
 	done
 
+testdata-presence: ../../bin/protoc-gen-debug
+	cd testdata/presence && \
+	set -e; for subdir in `find . -mindepth 1 -maxdepth 1 -type d`; do \
+		cd $$subdir; \
+		params=`cat params`; \
+		protoc -I . -I .. \
+			--plugin=protoc-gen-debug=../../../../../bin/protoc-gen-debug \
+			--debug_out=".:." \
+			--go_out="$$params:." \
+			`find . -name "*.proto"`; \
+		cd -; \
+	done
+
 ../../bin/protoc-gen-debug:
 	cd ../.. && $(MAKE) bin/protoc-gen-debug

Diff for: lang/go/testdata/presence/types/params

@@ -0,0 +1,67 @@
+syntax="proto3";
+package names.types;
+option go_package = "example.com/foo/bar";
+
+import "google/protobuf/duration.proto";
+import "google/protobuf/type.proto";
+
+message Proto3 {
+    double double = 1;
+    float float = 2;
+    int64 int64 = 3;
+    sfixed64 sfixed64 = 4;
+    sint64 sint64 = 5;
+    uint64 uint64 = 6;
+    fixed64 fixed64 = 7;
+    int32 int32 = 8;
+    sfixed32 sfixed32 = 9;
+    sint32 sint32 = 10;
+    uint32 uint32 = 11;
+    fixed32 fixed32 = 12;
+    bool bool = 13;
+    string string = 14;
+    bytes bytes = 15;
+
+    Enum enum = 16;
+    google.protobuf.Syntax ext_enum = 17;
+    Message msg = 18;
+    google.protobuf.Duration ext_msg = 19;
+
+    repeated double repeated_scalar = 20;
+    repeated Enum repeated_enum = 21;
+    repeated google.protobuf.Syntax repeated_ext_enum = 22;
+    repeated Message repeated_msg = 23;
+    repeated google.protobuf.Duration repeated_ext_msg = 24;
+
+    map<string, float> map_scalar = 25;
+    map<int32, Enum> map_enum = 26;
+    map<uint64, google.protobuf.Syntax> map_ext_enum = 27;
+    map<fixed32, Message> map_msg = 28;
+    map<sfixed64, google.protobuf.Duration> map_ext_msg = 29;
+
+    enum Enum {VALUE = 0;}
+
+    message Message {}
+
+    message Optional {
+        optional double double = 1;
+        optional float float = 2;
+        optional int64 int64 = 3;
+        optional sfixed64 sfixed64 = 4;
+        optional sint64 sint64 = 5;
+        optional uint64 uint64 = 6;
+        optional fixed64 fixed64 = 7;
+        optional int32 int32 = 8;
+        optional sfixed32 sfixed32 = 9;
+        optional sint32 sint32 = 10;
+        optional uint32 uint32 = 11;
+        optional fixed32 fixed32 = 12;
+        optional bool bool = 13;
+        optional string string = 14;
+        optional bytes bytes = 15;
+        optional Enum enum = 16;
+        optional google.protobuf.Syntax ext_enum = 17;
+        optional Optional msg = 18;
+        optional google.protobuf.Duration ext_msg = 19;
+    }
+}

Diff for: lang/go/testdata/presence/types/proto3.proto

@@ -25,7 +25,7 @@ func (c context) Type(f pgs.Field) TypeName {
 		t = scalarType(ft.ProtoType())
 	}
 
-	if f.Syntax() == pgs.Proto2 {
+	if f.HasPresence() {
 		return t.Pointer()
 	}