Fix overload collision checks by ignoring type param names (#718)

checker/checker_test.go

@@ -2312,3 +2312,53 @@ func TestCheck(t *testing.T) {
 		})
 	}
 }
+
+func TestAddDuplicateDeclarations(t *testing.T) {
+	reg, err := types.NewRegistry(&proto2pb.TestAllTypes{}, &proto3pb.TestAllTypes{})
+	if err != nil {
+		t.Fatalf("types.NewRegistry() failed: %v", err)
+	}
+	env, err := NewEnv(containers.DefaultContainer, reg, CrossTypeNumericComparisons(true))
+	if err != nil {
+		t.Fatalf("NewEnv() failed: %v", err)
+	}
+	err = env.Add(StandardDeclarations()...)
+	if err != nil {
+		t.Fatalf("env.Add() failed: %v", err)
+	}
+	err = env.Add(StandardDeclarations()...)
+	if err != nil {
+		t.Errorf("env.Add() failed with duplicate declarations: %v", err)
+	}
+}
+
+func TestAddEquivalentDeclarations(t *testing.T) {
+	reg, err := types.NewRegistry(&proto2pb.TestAllTypes{}, &proto3pb.TestAllTypes{})
+	if err != nil {
+		t.Fatalf("types.NewRegistry() failed: %v", err)
+	}
+	env, err := NewEnv(containers.DefaultContainer, reg, CrossTypeNumericComparisons(true))
+	if err != nil {
+		t.Fatalf("NewEnv() failed: %v", err)
+	}
+	optIndex := decls.NewFunction("optional_index",
+		decls.NewParameterizedOverload("optional_map_key_value",
+			[]*exprpb.Type{
+				decls.NewMapType(decls.NewTypeParamType("K"), decls.NewTypeParamType("V")),
+				decls.NewTypeParamType("K")},
+			decls.NewOptionalType(decls.NewTypeParamType("V")), []string{"K", "V"}))
+	optIndexEquiv := decls.NewFunction("optional_index",
+		decls.NewParameterizedOverload("optional_map_key_value",
+			[]*exprpb.Type{
+				decls.NewMapType(decls.NewTypeParamType("K"), decls.NewTypeParamType("V")),
+				decls.NewTypeParamType("K")},
+			decls.NewOptionalType(decls.NewTypeParamType("V")), []string{"V", "K"}))
+	err = env.Add(optIndex)
+	if err != nil {
+		t.Fatalf("env.Add(optIndex) failed: %v", err)
+	}
+	err = env.Add(optIndexEquiv)
+	if err != nil {
+		t.Errorf("env.Add(optIndexEquiv) failed: %v", err)
+	}
+}

checker/env.go

@@ -226,7 +226,7 @@ func (e *Env) setFunction(decl *exprpb.Decl) []errorMsg {
 		newOverloads := []*exprpb.Decl_FunctionDecl_Overload{}
 		for _, overload := range overloads {
 			existing, found := existingOverloads[overload.GetOverloadId()]
-			if !found || !proto.Equal(existing, overload) {
+			if !found || !overloadsEqual(existing, overload) {
 				newOverloads = append(newOverloads, overload)
 			}
 		}
@@ -264,6 +264,31 @@ func (e *Env) isOverloadDisabled(overloadID string) bool {
 	return found
 }
 
+// overloadsEqual returns whether two overloads have identical signatures.
+//
+// type parameter names are ignored as they may be specified in any order and have no bearing on overload
+// equivalence
+func overloadsEqual(o1, o2 *exprpb.Decl_FunctionDecl_Overload) bool {
+	return o1.GetOverloadId() == o2.GetOverloadId() &&
+		o1.GetIsInstanceFunction() == o2.GetIsInstanceFunction() &&
+		paramsEqual(o1.GetParams(), o2.GetParams()) &&
+		proto.Equal(o1.GetResultType(), o2.GetResultType())
+}
+
+// paramsEqual returns whether two lists have equal length and all types are equal
+func paramsEqual(p1, p2 []*exprpb.Type) bool {
+	if len(p1) != len(p2) {
+		return false
+	}
+	for i, a := range p1 {
+		b := p2[i]
+		if !proto.Equal(a, b) {
+			return false
+		}
+	}
+	return true
+}
+
 // sanitizeFunction replaces well-known types referenced by message name with their equivalent
 // CEL built-in type instances.
 func sanitizeFunction(decl *exprpb.Decl) *exprpb.Decl {