From 51dc972ba2626dadd9b2cb1a56e0f5edeb2b2115 Mon Sep 17 00:00:00 2001 From: Erik Miller Date: Fri, 11 Sep 2026 19:58:39 -0700 Subject: [PATCH 1/3] fix: skip accessor generation when it collides with a sibling field writeStructAccessors only checked generated GetX/SetX names against methods oapi-codegen had already emitted, not against other field names on the same struct. Terraform schemas can legitimately have two unrelated fields where one field's name is the accessor name the other would generate, e.g. aws_instance's get_password_data (bool) and password_data (string) become sibling fields GetPasswordData and PasswordData. Generating GetPasswordData() for the PasswordData field then collides with the GetPasswordData field itself, which Go rejects outright ("field and method with the same name"), breaking the build for provider-aws-ec2's InstanceStatusAtProvider and SpotInstanceRequestStatusAtProvider. Skip emitting an accessor whenever its name matches any field name on the struct, the same way we already skip when it matches an existing method. Signed-off-by: Erik Miller --- internal/schemas/generator/accessors.go | 17 +++++++-- internal/schemas/generator/accessors_test.go | 38 ++++++++++++++++++++ 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/internal/schemas/generator/accessors.go b/internal/schemas/generator/accessors.go index 9ae4c9ba..7881d761 100644 --- a/internal/schemas/generator/accessors.go +++ b/internal/schemas/generator/accessors.go @@ -148,8 +148,19 @@ func isNilable(e ast.Expr) bool { // writeStructAccessors appends a getter and setter for each named field of the // given struct to b. Any accessor whose name already exists in skip is omitted -// to avoid colliding with methods oapi-codegen already generated. +// to avoid colliding with methods oapi-codegen already generated. An accessor +// is also omitted if its name collides with another field of the same struct +// (e.g. a field named PasswordData alongside a sibling field named +// GetPasswordData): Go forbids a method and a field sharing a name on the same +// type, and Terraform schemas occasionally produce exactly that pair. func writeStructAccessors(b *strings.Builder, fset *token.FileSet, typeName string, st *ast.StructType, skip map[string]bool) { + fieldNames := map[string]bool{} + for _, field := range st.Fields.List { + for _, name := range field.Names { + fieldNames[name.Name] = true + } + } + for _, field := range st.Fields.List { // Skip embedded/anonymous fields; generated models don't use them. if len(field.Names) == 0 { @@ -176,7 +187,7 @@ func writeStructAccessors(b *strings.Builder, fset *token.FileSet, typeName stri // Getter. It tolerates a nil receiver so that chained getters are // safe on partially-populated resources. - if !skip["Get"+fieldName] { + if !skip["Get"+fieldName] && !fieldNames["Get"+fieldName] { b.WriteString("\n// Get" + fieldName + " returns the " + fieldName + " field.\n") b.WriteString("// It returns the zero value if the receiver is nil.\n") b.WriteString("func (" + accessorReceiver + " *" + typeName + ") Get" + fieldName + "() " + fieldType + " {\n") @@ -193,7 +204,7 @@ func writeStructAccessors(b *strings.Builder, fset *token.FileSet, typeName stri } // Setter. - if !skip["Set"+fieldName] { + if !skip["Set"+fieldName] && !fieldNames["Set"+fieldName] { b.WriteString("\n// Set" + fieldName + " sets the " + fieldName + " field.\n") b.WriteString("func (" + accessorReceiver + " *" + typeName + ") Set" + fieldName + "(v " + fieldType + ") {\n") b.WriteString("\t" + accessorReceiver + "." + fieldName + " = v\n") diff --git a/internal/schemas/generator/accessors_test.go b/internal/schemas/generator/accessors_test.go index e2a08d9e..72acded8 100644 --- a/internal/schemas/generator/accessors_test.go +++ b/internal/schemas/generator/accessors_test.go @@ -489,3 +489,41 @@ func (o *Foo) GetAdditionalProperties() *map[string]string { t.Errorf("expected SetAdditionalProperties to be generated, got %d", n) } } + +// TestAddAccessorsSkipsFieldNameCollisions guards against a Terraform schema +// that legitimately has both a field (e.g. PasswordData) and a sibling field +// whose name matches the accessor the first field would generate (e.g. +// GetPasswordData). Go forbids a method and a field sharing a name on the same +// type, so emitting GetPasswordData() here would make the package fail to +// compile, as happened for provider-aws-ec2's InstanceStatusAtProvider. +func TestAddAccessorsSkipsFieldNameCollisions(t *testing.T) { + input := `package v1alpha1 + +type Foo struct { + PasswordData *string ` + "`json:\"passwordData,omitempty\"`" + ` + GetPasswordData *bool ` + "`json:\"getPasswordData,omitempty\"`" + ` +} +` + + got, err := addAccessors(input) + if err != nil { + t.Fatalf("addAccessors returned error: %v", err) + } + + // The colliding getter must not be generated at all: the struct's own + // GetPasswordData field is the only thing named GetPasswordData. + if n := countMethods(t, got, "Foo", "GetPasswordData"); n != 0 { + t.Errorf("expected no GetPasswordData method (field of that name already exists), got %d", n) + } + // SetPasswordData doesn't collide with anything and should still be generated. + if n := countMethods(t, got, "Foo", "SetPasswordData"); n != 1 { + t.Errorf("expected SetPasswordData to be generated, got %d", n) + } + // The other field's own accessors are unaffected. + if n := countMethods(t, got, "Foo", "GetGetPasswordData"); n != 1 { + t.Errorf("expected GetGetPasswordData to be generated for the GetPasswordData field, got %d", n) + } + if n := countMethods(t, got, "Foo", "SetGetPasswordData"); n != 1 { + t.Errorf("expected SetGetPasswordData to be generated for the GetPasswordData field, got %d", n) + } +} From d5b6e827e6eaf84bece7028f38ed5f89799f55ae Mon Sep 17 00:00:00 2001 From: Erik Miller Date: Sat, 12 Sep 2026 08:09:21 -0700 Subject: [PATCH 2/3] fix: also treat embedded field names as collision candidates; table-drive the collision test Addresses CodeRabbit review on crossplane/cli#366: - addAccessors skipped anonymous/embedded fields entirely when collecting fieldNames, but Go promotes an embedded type's own name into the struct's namespace, so an embedded field can still collide with a generated accessor even though go/ast reports it with an empty field.Names. Add embeddedFieldName to derive the promoted name (stripping pointer indirection and package qualification) and record it alongside named fields. - Rewrite TestAddAccessorsSkipsFieldNameCollisions as table-driven cases with args/want/reason, matching this repo's test conventions, and add coverage for the setter collision path and the embedded-field case alongside the original getter collision, diffing the full generated method set with cmp.Diff rather than spot-checking individual methods. Signed-off-by: Erik Miller --- internal/schemas/generator/accessors.go | 31 ++++++ internal/schemas/generator/accessors_test.go | 102 ++++++++++++++----- 2 files changed, 108 insertions(+), 25 deletions(-) diff --git a/internal/schemas/generator/accessors.go b/internal/schemas/generator/accessors.go index 7881d761..2df2a90d 100644 --- a/internal/schemas/generator/accessors.go +++ b/internal/schemas/generator/accessors.go @@ -130,6 +130,26 @@ func receiverTypeName(e ast.Expr) string { return "" } +// embeddedFieldName returns the name Go promotes into the struct's namespace +// for an anonymous field, mirroring the Go spec: it's the embedded type's own +// name, ignoring any pointer indirection or package qualification (e.g. +// embedding `*Bar` or `pkg.Bar` both promote the name `Bar`). Returns "" for +// type shapes generated models don't use (generics, etc.), which simply +// aren't tracked as potential collisions. +func embeddedFieldName(e ast.Expr) string { + if star, ok := e.(*ast.StarExpr); ok { + e = star.X + } + switch t := e.(type) { + case *ast.Ident: + return t.Name + case *ast.SelectorExpr: + return t.Sel.Name + default: + return "" + } +} + // isNilable reports whether a zero value of the given type is spelled `nil`, // letting the generated getter return nil directly instead of declaring a zero // variable. Generated models use pointers throughout, so this is the common @@ -156,6 +176,17 @@ func isNilable(e ast.Expr) bool { func writeStructAccessors(b *strings.Builder, fset *token.FileSet, typeName string, st *ast.StructType, skip map[string]bool) { fieldNames := map[string]bool{} for _, field := range st.Fields.List { + if len(field.Names) == 0 { + // Anonymous/embedded field: Go promotes the embedded type's own + // name into the struct's namespace (e.g. embedding GetFoo gives + // the struct a field effectively named GetFoo), so it can still + // collide with a generated accessor even though it has no + // explicit field.Names entry of its own. + if n := embeddedFieldName(field.Type); n != "" { + fieldNames[n] = true + } + continue + } for _, name := range field.Names { fieldNames[name.Name] = true } diff --git a/internal/schemas/generator/accessors_test.go b/internal/schemas/generator/accessors_test.go index 72acded8..88330160 100644 --- a/internal/schemas/generator/accessors_test.go +++ b/internal/schemas/generator/accessors_test.go @@ -491,39 +491,91 @@ func (o *Foo) GetAdditionalProperties() *map[string]string { } // TestAddAccessorsSkipsFieldNameCollisions guards against a Terraform schema -// that legitimately has both a field (e.g. PasswordData) and a sibling field -// whose name matches the accessor the first field would generate (e.g. -// GetPasswordData). Go forbids a method and a field sharing a name on the same -// type, so emitting GetPasswordData() here would make the package fail to -// compile, as happened for provider-aws-ec2's InstanceStatusAtProvider. +// that legitimately has two fields where one's name matches the accessor the +// other would generate (e.g. a field named PasswordData alongside a sibling +// field named GetPasswordData). Go forbids a method and a field sharing a name +// on the same type, so emitting the colliding accessor would make the package +// fail to compile, as happened for provider-aws-ec2's InstanceStatusAtProvider. +// Table-driven so a regression in either the getter or the setter collision +// check — or in the embedded-field case, which go/ast reports via an empty +// field.Names rather than a plain name — is caught, and cmp.Diff over the full +// generated method set confirms nothing else was skipped along the way. func TestAddAccessorsSkipsFieldNameCollisions(t *testing.T) { - input := `package v1alpha1 + cases := []struct { + name string + args string + want map[string]string + reason string + }{ + { + name: "GetterCollidesWithSiblingField", + args: `package v1alpha1 type Foo struct { PasswordData *string ` + "`json:\"passwordData,omitempty\"`" + ` GetPasswordData *bool ` + "`json:\"getPasswordData,omitempty\"`" + ` } -` +`, + want: map[string]string{ + // No Foo.GetPasswordData: it would collide with the sibling + // field of that exact name. + "Foo.SetPasswordData": "*string", + "Foo.GetGetPasswordData": "*bool", + "Foo.SetGetPasswordData": "*bool", + }, + reason: "a field named PasswordData must not get a GetPasswordData() method when a sibling field is itself named GetPasswordData", + }, + { + name: "SetterCollidesWithSiblingField", + args: `package v1alpha1 - got, err := addAccessors(input) - if err != nil { - t.Fatalf("addAccessors returned error: %v", err) - } +type Foo struct { + Data *string ` + "`json:\"data,omitempty\"`" + ` + SetData *bool ` + "`json:\"setData,omitempty\"`" + ` +} +`, + want: map[string]string{ + "Foo.GetData": "*string", + // No Foo.SetData: it would collide with the sibling field of + // that exact name. + "Foo.GetSetData": "*bool", + "Foo.SetSetData": "*bool", + }, + reason: "a field named Data must not get a SetData() method when a sibling field is itself named SetData", + }, + { + name: "GetterCollidesWithEmbeddedFieldName", + args: `package v1alpha1 - // The colliding getter must not be generated at all: the struct's own - // GetPasswordData field is the only thing named GetPasswordData. - if n := countMethods(t, got, "Foo", "GetPasswordData"); n != 0 { - t.Errorf("expected no GetPasswordData method (field of that name already exists), got %d", n) - } - // SetPasswordData doesn't collide with anything and should still be generated. - if n := countMethods(t, got, "Foo", "SetPasswordData"); n != 1 { - t.Errorf("expected SetPasswordData to be generated, got %d", n) - } - // The other field's own accessors are unaffected. - if n := countMethods(t, got, "Foo", "GetGetPasswordData"); n != 1 { - t.Errorf("expected GetGetPasswordData to be generated for the GetPasswordData field, got %d", n) +type GetPasswordData struct { + Enabled *bool ` + "`json:\"enabled,omitempty\"`" + ` +} + +type Foo struct { + PasswordData *string ` + "`json:\"passwordData,omitempty\"`" + ` + GetPasswordData +} +`, + want: map[string]string{ + // No Foo.GetPasswordData: Go promotes the embedded + // GetPasswordData field under that same name. + "Foo.SetPasswordData": "*string", + "GetPasswordData.GetEnabled": "*bool", + "GetPasswordData.SetEnabled": "*bool", + }, + reason: "an anonymous/embedded field promotes its type name into the struct's namespace just like a named field would, so it must be treated as a collision candidate too", + }, } - if n := countMethods(t, got, "Foo", "SetGetPasswordData"); n != 1 { - t.Errorf("expected SetGetPasswordData to be generated for the GetPasswordData field, got %d", n) + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := addAccessors(tc.args) + if err != nil { + t.Fatalf("addAccessors returned error: %v", err) + } + if diff := cmp.Diff(tc.want, collectMethods(t, got)); diff != "" { + t.Errorf("%s\ngenerated accessors (-want +got):\n%s", tc.reason, diff) + } + }) } } From 19d16805b9b8a73138ee854d00f42b6eca9129ec Mon Sep 17 00:00:00 2001 From: Erik Miller Date: Wed, 16 Sep 2026 18:27:52 -0700 Subject: [PATCH 3/3] fix: reduce writeStructAccessors cognitive complexity Extract collectFieldNames, writeGetter, and writeSetter helpers so writeStructAccessors clears the gocognit threshold (31 -> 18) that the collision-detection changes in d5b6e82 pushed it past. No behavior change. --- internal/schemas/generator/accessors.go | 90 ++++++++++++++----------- 1 file changed, 51 insertions(+), 39 deletions(-) diff --git a/internal/schemas/generator/accessors.go b/internal/schemas/generator/accessors.go index 2df2a90d..b3d671e3 100644 --- a/internal/schemas/generator/accessors.go +++ b/internal/schemas/generator/accessors.go @@ -174,23 +174,7 @@ func isNilable(e ast.Expr) bool { // GetPasswordData): Go forbids a method and a field sharing a name on the same // type, and Terraform schemas occasionally produce exactly that pair. func writeStructAccessors(b *strings.Builder, fset *token.FileSet, typeName string, st *ast.StructType, skip map[string]bool) { - fieldNames := map[string]bool{} - for _, field := range st.Fields.List { - if len(field.Names) == 0 { - // Anonymous/embedded field: Go promotes the embedded type's own - // name into the struct's namespace (e.g. embedding GetFoo gives - // the struct a field effectively named GetFoo), so it can still - // collide with a generated accessor even though it has no - // explicit field.Names entry of its own. - if n := embeddedFieldName(field.Type); n != "" { - fieldNames[n] = true - } - continue - } - for _, name := range field.Names { - fieldNames[name.Name] = true - } - } + fieldNames := collectFieldNames(st) for _, field := range st.Fields.List { // Skip embedded/anonymous fields; generated models don't use them. @@ -215,32 +199,60 @@ func writeStructAccessors(b *strings.Builder, fset *token.FileSet, typeName stri } fieldName := name.Name - - // Getter. It tolerates a nil receiver so that chained getters are - // safe on partially-populated resources. if !skip["Get"+fieldName] && !fieldNames["Get"+fieldName] { - b.WriteString("\n// Get" + fieldName + " returns the " + fieldName + " field.\n") - b.WriteString("// It returns the zero value if the receiver is nil.\n") - b.WriteString("func (" + accessorReceiver + " *" + typeName + ") Get" + fieldName + "() " + fieldType + " {\n") - b.WriteString("\tif " + accessorReceiver + " == nil {\n") - if isNilable(field.Type) { - b.WriteString("\t\treturn nil\n") - } else { - b.WriteString("\t\tvar zero " + fieldType + "\n") - b.WriteString("\t\treturn zero\n") - } - b.WriteString("\t}\n") - b.WriteString("\treturn " + accessorReceiver + "." + fieldName + "\n") - b.WriteString("}\n") + writeGetter(b, typeName, fieldName, fieldType, isNilable(field.Type)) } - - // Setter. if !skip["Set"+fieldName] && !fieldNames["Set"+fieldName] { - b.WriteString("\n// Set" + fieldName + " sets the " + fieldName + " field.\n") - b.WriteString("func (" + accessorReceiver + " *" + typeName + ") Set" + fieldName + "(v " + fieldType + ") {\n") - b.WriteString("\t" + accessorReceiver + "." + fieldName + " = v\n") - b.WriteString("}\n") + writeSetter(b, typeName, fieldName, fieldType) } } } } + +// collectFieldNames returns every name that occupies the struct's field +// namespace, including names promoted by anonymous/embedded fields. Go +// promotes an embedded type's own name into the struct's namespace (e.g. +// embedding GetFoo gives the struct a field effectively named GetFoo), so +// those still count as potential collisions even though they have no +// explicit field.Names entry of their own. +func collectFieldNames(st *ast.StructType) map[string]bool { + fieldNames := map[string]bool{} + for _, field := range st.Fields.List { + if len(field.Names) == 0 { + if n := embeddedFieldName(field.Type); n != "" { + fieldNames[n] = true + } + continue + } + for _, name := range field.Names { + fieldNames[name.Name] = true + } + } + return fieldNames +} + +// writeGetter appends a getter for fieldName to b. It tolerates a nil +// receiver so that chained getters are safe on partially-populated resources. +func writeGetter(b *strings.Builder, typeName, fieldName, fieldType string, nilable bool) { + b.WriteString("\n// Get" + fieldName + " returns the " + fieldName + " field.\n") + b.WriteString("// It returns the zero value if the receiver is nil.\n") + b.WriteString("func (" + accessorReceiver + " *" + typeName + ") Get" + fieldName + "() " + fieldType + " {\n") + b.WriteString("\tif " + accessorReceiver + " == nil {\n") + if nilable { + b.WriteString("\t\treturn nil\n") + } else { + b.WriteString("\t\tvar zero " + fieldType + "\n") + b.WriteString("\t\treturn zero\n") + } + b.WriteString("\t}\n") + b.WriteString("\treturn " + accessorReceiver + "." + fieldName + "\n") + b.WriteString("}\n") +} + +// writeSetter appends a setter for fieldName to b. +func writeSetter(b *strings.Builder, typeName, fieldName, fieldType string) { + b.WriteString("\n// Set" + fieldName + " sets the " + fieldName + " field.\n") + b.WriteString("func (" + accessorReceiver + " *" + typeName + ") Set" + fieldName + "(v " + fieldType + ") {\n") + b.WriteString("\t" + accessorReceiver + "." + fieldName + " = v\n") + b.WriteString("}\n") +}