Field numbers for multi-key leafs pointing to the same field should be unique. (#610)

* Field numbers for multi-key leafs pointing to the same field should be unique.

This OpenConfig snippet is causing duplicate field numbers to be generated due to the list keys both pointing to the same path:
```yang
list inter-area-propagation-policy {
  key "src-area dst-area";
  description
    "A list of connections between pairs of areas - routes are
    propagated from the source (src) area to the destination (dst)
    area according to the policy specified";

  leaf src-area {
    type leafref {
      path "../config/src-area";
    }
    description
      "Reference to the source area";
  }

  leaf dst-area {
    type leafref {
      path "../config/dst-area";
    }
    description
      "Reference to the destination area";
  }
```

This change makes `genListKeyProto` use the non-resolved entry path when
determining the field number of its oneof fields. The new
`proto-test-g.yang` file has the following diff after this change:

--- want
+++ got
@@ -28,12 +28,12 @@
   }
   message ElistKey {
     oneof one {
-      sint64 one_sint64 = 380657501;
-      string one_string = 434013286;
+      sint64 one_sint64 = 278603474;
+      string one_string = 515705169;
     }
     oneof two {
-      sint64 two_sint64 = 380657501;
-      string two_string = 434013286;
+      sint64 two_sint64 = 322077556;
+      string two_string = 157954079;
     }
     Elist elist = 3;
   }

I've verified that two fields in a container that both point to the same
leaf doesn't have this problem.

* Only change tag numbers when collision occurs to preserve backwards compatibility

* Update integration test files

* Add comment on what path we're using to generate proto tag numbers

Author: wenovus

ygen/codegen_test.go

@@ -2152,6 +2152,18 @@ func TestGenerateProto3(t *testing.T) {
 			"openconfig.proto_test_f.a":   filepath.Join(TestRoot, "testdata", "proto", "proto_test_f.uncompressed.proto_test_f.a.formatted-txt"),
 			"openconfig.proto_test_f.a.c": filepath.Join(TestRoot, "testdata", "proto", "proto_test_f.uncompressed.proto_test_f.a.c.formatted-txt"),
 		},
+	}, {
+		name:    "yang schema with leafrefs that point to the same path",
+		inFiles: []string{filepath.Join(TestRoot, "testdata", "proto", "proto-test-g.yang")},
+		inConfig: GeneratorConfig{
+			ProtoOptions: ProtoOpts{
+				GoPackageBase:  "github.com/foo/baz",
+				NestedMessages: true,
+			},
+		},
+		wantOutputFiles: map[string]string{
+			"openconfig.proto_test_g": filepath.Join(TestRoot, "testdata", "proto", "proto-test-g.proto-test-g.formatted-txt"),
+		},
 	}, {
 		name:    "yang schema with fake root, path compression and union list key",
 		inFiles: []string{filepath.Join(TestRoot, "testdata", "proto", "proto-union-list-key.yang")},

ygen/protogen.go

@@ -1038,7 +1038,7 @@ func protoLeafDefinition(leafName string, args *protoDefinitionArgs, useDefining
 	case util.IsEnumeratedType(args.field.Type):
 		d.globalEnum = true
 	case protoType.UnionTypes != nil:
-		u, err := unionFieldToOneOf(leafName, args.field, protoType, args.cfg.annotateEnumNames, useDefiningModuleForTypedefEnumNames, useConsistentNamesForProtoUnionEnums)
+		u, err := unionFieldToOneOf(leafName, args.field, args.field.Path(), protoType, args.cfg.annotateEnumNames, useDefiningModuleForTypedefEnumNames, useConsistentNamesForProtoUnionEnums)
 		if err != nil {
 			return nil, err
 		}
@@ -1128,6 +1128,10 @@ func genListKeyProto(listPackage string, listName string, args *protoDefinitionA
 
 	definedFieldNames := map[string]bool{}
 	ctag := uint32(1)
+	// unionEntries keeps track of union keys such that if two keys point
+	// to the same union entry, such a conflict when creating field tags
+	// for them can be detected to avoid a tag collision.
+	unionEntries := map[*yang.Entry]bool{}
 	for _, k := range strings.Fields(args.field.Key) {
 		kf, ok := args.directory.Fields[k]
 		if !ok {
@@ -1209,7 +1213,18 @@ func genListKeyProto(listPackage string, listName string, args *protoDefinitionA
 			km.Enums[tn] = enum
 		case unionEntry != nil:
 			fd.IsOneOf = true
-			u, err := unionFieldToOneOf(fd.Name, unionEntry, scalarType, args.cfg.annotateEnumNames, useDefiningModuleForTypedefEnumNames, useConsistentNamesForProtoUnionEnums)
+			path := unionEntry.Path()
+			if unionEntries[unionEntry] {
+				// It is possible for two keys to point to the same resolved unionEntry.
+				// In this case, the path we use to generate the proto tag numbers needs
+				// to be different to avoid a collision, and here we use the path of the
+				// (leafref) key field. The reason the first instance uses the resolved
+				// unionEntry is for backwards compatibility
+				// (https://github.com/openconfig/ygot/pull/610#discussion_r781510037).
+				path = kf.Path()
+			}
+			unionEntries[unionEntry] = true
+			u, err := unionFieldToOneOf(fd.Name, unionEntry, path, scalarType, args.cfg.annotateEnumNames, useDefiningModuleForTypedefEnumNames, useConsistentNamesForProtoUnionEnums)
 			if err != nil {
 				return nil, fmt.Errorf("error generating type for union list key %s in list %s", k, args.field.Path())
 			}
@@ -1312,11 +1327,12 @@ type protoUnionField struct {
 	hadGlobalEnums bool                     // hadGlobalEnums determines whether there was a global scope enum (typedef, identityref) in the message.
 }
 
-// unionFieldToOneOf takes an input name, a yang.Entry containing a field definition and a MappedType
+// unionFieldToOneOf takes an input name, a yang.Entry containing a field
+// definition, a path argument used to compute the field tag numbers, and a MappedType
 // containing the proto type that the entry has been mapped to, and returns a definition of a union
 // field within the protobuf message. If the annotateEnumNames boolean is set, then any enumerated types
 // within the union have their original names within the YANG schema appended.
-func unionFieldToOneOf(fieldName string, e *yang.Entry, mtype *MappedType, annotateEnumNames, useDefiningModuleForTypedefEnumNames, useConsistentNamesForProtoUnionEnums bool) (*protoUnionField, error) {
+func unionFieldToOneOf(fieldName string, e *yang.Entry, path string, mtype *MappedType, annotateEnumNames, useDefiningModuleForTypedefEnumNames, useConsistentNamesForProtoUnionEnums bool) (*protoUnionField, error) {
 	enums, err := enumInProtoUnionField(fieldName, resolveTypeArgs{yangType: e.Type, contextEntry: e}, annotateEnumNames, useDefiningModuleForTypedefEnumNames, useConsistentNamesForProtoUnionEnums)
 	if err != nil {
 		return nil, err
@@ -1344,7 +1360,7 @@ func unionFieldToOneOf(fieldName string, e *yang.Entry, mtype *MappedType, annot
 		// such that we have unique inputs for each option. We make the name lower-case
 		// as it is conventional that protobuf field names are lowercase separated by
 		// underscores.
-		ft, err := fieldTag(fmt.Sprintf("%s_%s", e.Path(), strings.ToLower(tn)))
+		ft, err := fieldTag(fmt.Sprintf("%s_%s", path, strings.ToLower(tn)))
 		if err != nil {
 			return nil, fmt.Errorf("could not calculate tag number for %s, type %s in oneof", e.Path(), tn)
 		}

ygen/protogen_test.go

@@ -1822,9 +1822,11 @@ func TestUnionFieldToOneOf(t *testing.T) {
 	}
 
 	tests := []struct {
-		name                string
-		inName              string
-		inEntry             *yang.Entry
+		name    string
+		inName  string
+		inEntry *yang.Entry
+		// inPath is populated with in.Entry.Path() if not set.
+		inPath              string
 		inMappedType        *MappedType
 		inAnnotateEnumNames bool
 		wantFields          []*protoMsgField
@@ -1859,6 +1861,35 @@ func TestUnionFieldToOneOf(t *testing.T) {
 			Type: "string",
 		}},
 		wantEnums: map[string]*protoMsgEnum{},
+	}, {
+		name:   "simple string union with a non-empty path argument",
+		inName: "FieldName",
+		inEntry: &yang.Entry{
+			Name: "field-name",
+			Type: &yang.YangType{
+				Type: []*yang.YangType{
+					{Kind: yang.Ystring},
+					{Kind: yang.Yint8},
+				},
+			},
+		},
+		inMappedType: &MappedType{
+			UnionTypes: map[string]int{
+				"string": 0,
+				"sint64": 0,
+			},
+		},
+		inPath: "a/b/c/d",
+		wantFields: []*protoMsgField{{
+			Tag:  352411621,
+			Name: "FieldName_sint64",
+			Type: "sint64",
+		}, {
+			Tag:  156680110,
+			Name: "FieldName_string",
+			Type: "string",
+		}},
+		wantEnums: map[string]*protoMsgEnum{},
 	}, {
 		name:   "decimal64 union",
 		inName: "FieldName",
@@ -2001,7 +2032,10 @@ func TestUnionFieldToOneOf(t *testing.T) {
 	}}
 
 	for _, tt := range tests {
-		got, err := unionFieldToOneOf(tt.inName, tt.inEntry, tt.inMappedType, tt.inAnnotateEnumNames, true, true)
+		if tt.inPath == "" {
+			tt.inPath = tt.inEntry.Path()
+		}
+		got, err := unionFieldToOneOf(tt.inName, tt.inEntry, tt.inPath, tt.inMappedType, tt.inAnnotateEnumNames, true, true)
 		if (err != nil) != tt.wantErr {
 			t.Errorf("%s: unionFieldToOneOf(%s, %v, %v, %v): did not get expected error, got: %v, wanted err: %v", tt.name, tt.inName, tt.inEntry, tt.inMappedType, tt.inAnnotateEnumNames, err, tt.wantErr)
 		}

ygen/testdata/proto/proto-test-g.proto-test-g.formatted-txt

@@ -0,0 +1,41 @@
+// openconfig.proto_test_g is generated by codegen-tests as a protobuf
+// representation of a YANG schema.
+//
+// Input schema modules:
+//  - testdata/proto/proto-test-g.yang
+syntax = "proto3";
+
+package openconfig.proto_test_g;
+
+option go_package = "github.com/foo/baz/openconfig/proto_test_g";
+
+message Elists {
+  message Elist {
+    message Config {
+      oneof one {
+        sint64 one_sint64 = 380657501;
+        string one_string = 434013286;
+      }
+    }
+    message State {
+      oneof one {
+        sint64 one_sint64 = 134694452;
+        string one_string = 507493599;
+      }
+    }
+    Config config = 521512235;
+    State state = 30995436;
+  }
+  message ElistKey {
+    oneof one {
+      sint64 one_sint64 = 380657501;
+      string one_string = 434013286;
+    }
+    oneof two {
+      sint64 two_sint64 = 322077556;
+      string two_string = 157954079;
+    }
+    Elist elist = 3;
+  }
+  repeated ElistKey elist = 113229370;
+}

ygen/testdata/proto/proto-test-g.yang

@@ -0,0 +1,40 @@
+module proto-test-g {
+  prefix "proto-g";
+  namespace "urn:proto-g";
+
+  grouping grp {
+    leaf one {
+      type union {
+        type string;
+        type int32;
+      }
+    }
+  }
+
+  container elists {
+    list elist {
+      key "one two";
+
+      leaf one {
+        type leafref {
+          path "../config/one";
+        }
+      }
+
+      leaf two {
+        type leafref {
+          path "../config/one";
+        }
+      }
+
+      container config {
+        uses grp;
+      }
+
+      container state {
+        config false;
+        uses grp;
+      }
+    }
+  }
+}