From fe2287d0a299457517e4e1aae82a60fca0fd10d2 Mon Sep 17 00:00:00 2001
From: Wojciech Tyczynski <wojtekt@google.com>
Date: Mon, 14 Mar 2016 14:22:48 +0100
Subject: [PATCH] Small cleanups in protobuf generator

---
 .../go-to-protobuf/protobuf/generator.go      | 58 ++++++++++---------
 .../go2idl/go-to-protobuf/protobuf/namer.go   |  8 ++-
 2 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/cmd/libs/go2idl/go-to-protobuf/protobuf/generator.go b/cmd/libs/go2idl/go-to-protobuf/protobuf/generator.go
index 92a7879d814..e81c8140021 100644
--- a/cmd/libs/go2idl/go-to-protobuf/protobuf/generator.go
+++ b/cmd/libs/go2idl/go-to-protobuf/protobuf/generator.go
@@ -271,6 +271,7 @@ func (b bodyGen) doStruct(sw *generator.SnippetWriter) error {
 		}
 	}
 
+	// If we don't explicitly embed anything, generate fields by traversing fields.
 	if fields == nil {
 		memberFields, err := membersToFields(b.locator, b.t, b.localPackage, b.omitFieldTypes)
 		if err != nil {
@@ -399,8 +400,10 @@ func memberTypeToProtobufField(locator ProtobufLocator, field *protoField, t *ty
 		if err := memberTypeToProtobufField(locator, keyField, t.Key); err != nil {
 			return err
 		}
+		// All other protobuf types has kind types.Protobuf, so setting types.Map
+		// here would be very misleading.
 		field.Type = &types.Type{
-			Kind: types.Map,
+			Kind: types.Protobuf,
 			Key:  keyField.Type,
 			Elem: valueField.Type,
 		}
@@ -450,7 +453,6 @@ func memberTypeToProtobufField(locator ProtobufLocator, field *protoField, t *ty
 }
 
 // protobufTagToField extracts information from an existing protobuf tag
-// TODO: take a current package
 func protobufTagToField(tag string, field *protoField, m types.Member, t *types.Type, localPackage types.Name) error {
 	if len(tag) == 0 {
 		return nil
@@ -466,31 +468,36 @@ func protobufTagToField(tag string, field *protoField, m types.Member, t *types.
 		return fmt.Errorf("member %q of %q malformed 'protobuf' tag, field ID is %q which is not an integer: %v\n", m.Name, t.Name, parts[1], err)
 	}
 	field.Tag = protoTag
-	// TODO: we are converting a Protobuf type back into an internal type, which is questionable
-	// TODO; Allow a way to unambiguously represent a type into two systems at the same time, like Go and Protobuf.
-	if last := strings.LastIndex(parts[0], "."); last != -1 {
-		prefix := parts[0][:last]
-		field.Type = &types.Type{
-			Name: types.Name{
+
+	// In general there is doesn't make sense to parse the protobuf tags to get the type,
+	// as all auto-generated once will have wire type "bytes", "varint" or "fixed64".
+	// However, sometimes we explicitly set them to have a custom serialization, e.g.:
+	//   type Time struct {
+	//     time.Time `protobuf:"Timestamp,1,req,name=time"`
+	//   }
+	// to force the generator to use a given type (that we manually wrote serialization &
+	// deserialization methods for).
+	switch parts[0] {
+	case "varint", "fixed32", "fixed64", "bytes", "group":
+	default:
+		name := types.Name{}
+		if last := strings.LastIndex(parts[0], "."); last != -1 {
+			prefix := parts[0][:last]
+			name = types.Name{
 				Name:    parts[0][last+1:],
 				Package: prefix,
-				// TODO: this probably needs to be a lookup into a namer
-				Path: strings.Replace(prefix, ".", "/", -1),
-			},
-			Kind: types.Protobuf,
-		}
-	} else {
-		switch parts[0] {
-		case "varint", "bytes", "fixed64":
-		default:
-			field.Type = &types.Type{
-				Name: types.Name{
-					Name:    parts[0],
-					Package: localPackage.Package,
-					Path:    localPackage.Path,
-				},
-				Kind: types.Protobuf,
+				Path:    strings.Replace(prefix, ".", "/", -1),
 			}
+		} else {
+			name = types.Name{
+				Name:    parts[0],
+				Package: localPackage.Package,
+				Path:    localPackage.Path,
+			}
+		}
+		field.Type = &types.Type{
+			Name: name,
+			Kind: types.Protobuf,
 		}
 	}
 
@@ -501,11 +508,10 @@ func protobufTagToField(tag string, field *protoField, m types.Member, t *types.
 			return fmt.Errorf("member %q of %q malformed 'protobuf' tag, tag %d should be key=value, got %q\n", m.Name, t.Name, i+4, extra)
 		}
 		switch parts[0] {
-		case "casttype":
+		case "casttype", "castkey", "castvalue":
 			parts[0] = fmt.Sprintf("(gogoproto.%s)", parts[0])
 			protoExtra[parts[0]] = parts[1]
 		}
-		// TODO: Should we parse castkey and castvalue too?
 	}
 
 	field.Extras = protoExtra
diff --git a/cmd/libs/go2idl/go-to-protobuf/protobuf/namer.go b/cmd/libs/go2idl/go-to-protobuf/protobuf/namer.go
index ea80094cb03..662206c0e5c 100644
--- a/cmd/libs/go2idl/go-to-protobuf/protobuf/namer.go
+++ b/cmd/libs/go2idl/go-to-protobuf/protobuf/namer.go
@@ -31,7 +31,7 @@ type localNamer struct {
 }
 
 func (n localNamer) Name(t *types.Type) string {
-	if t.Kind == types.Map {
+	if t.Key != nil && t.Elem != nil {
 		return fmt.Sprintf("map<%s, %s>", n.Name(t.Key), n.Name(t.Elem))
 	}
 	if len(n.localPackage.Package) != 0 && n.localPackage.Package == t.Name.Package {
@@ -67,8 +67,10 @@ func (n *protobufNamer) List() []generator.Package {
 }
 
 func (n *protobufNamer) Add(p *protobufPackage) {
-	n.packagesByPath[p.PackagePath] = p
-	n.packages = append(n.packages, p)
+	if _, ok := n.packagesByPath[p.PackagePath]; !ok {
+		n.packagesByPath[p.PackagePath] = p
+		n.packages = append(n.packages, p)
+	}
 }
 
 func (n *protobufNamer) GoNameToProtoName(name types.Name) types.Name {