[release-branch.go1.18] go/types,types2: delay the check for conflicting struct field names

In #52529, we observed that checking types for duplicate fields and
methods during method collection can result in incorrect early expansion
of the base type. Fix this by delaying the check for duplicate fields.
Notably, we can't delay the check for duplicate methods as we must
preserve the invariant that added method names are unique.

After this change, it may be possible in the presence of errors to have
a type-checked type containing a method name that conflicts with a field
name. With the previous logic conflicting methods would have been
skipped. This is a change in behavior, but only for invalid code.
Preserving the existing behavior would likely require delaying method
collection, which could have more significant consequences.

As a result of this change, the compiler test fixedbugs/issue28268.go
started passing with types2, being previously marked as broken. The fix
was not actually related to the duplicate method error, but rather the
fact that we stopped reporting redundant errors on the calls to x.b()
and x.E(), because they are now (valid!) methods.

Updates #52529
Fixes #52558

Change-Id: I850ce85c6ba76d79544f46bfd3deb8538d8c7d00
Reviewed-on: https://go-review.googlesource.com/c/go/+/403455
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit b75e492b35746ca3b327f7b353f4912e705a3125)
Reviewed-on: https://go-review.googlesource.com/c/go/+/403754

5 files changed, 107 insertions, 38 deletions

diff --git a/src/cmd/compile/internal/types2/decl.go b/src/cmd/compile/internal/types2/decl.go
index 17453f31fa..fc03155e69 100644
--- a/src/cmd/compile/internal/types2/decl.go
+++ b/src/cmd/compile/internal/types2/decl.go
@@ -636,14 +636,12 @@ func (check *Checker) collectMethods(obj *TypeName) {
 	base, _ := obj.typ.(*Named) // shouldn't fail but be conservative
 	if base != nil {
 		assert(base.targs.Len() == 0) // collectMethods should not be called on an instantiated type
-		u := base.under()
-		if t, _ := u.(*Struct); t != nil {
-			for _, fld := range t.fields {
-				if fld.name != "_" {
-					assert(mset.insert(fld) == nil)
-				}
-			}
-		}
+
+		// See issue #52529: we must delay the expansion of underlying here, as
+		// base may not be fully set-up.
+		check.later(func() {
+			check.checkFieldUniqueness(base)
+		}).describef(obj, "verifying field uniqueness for %v", base)
 
 		// Checker.Files may be called multiple times; additional package files
 		// may add methods to already type-checked types. Add pre-existing methods
@@ -662,17 +660,10 @@ func (check *Checker) collectMethods(obj *TypeName) {
 		assert(m.name != "_")
 		if alt := mset.insert(m); alt != nil {
 			var err error_
-			switch alt.(type) {
-			case *Var:
-				err.errorf(m.pos, "field and method with the same name %s", m.name)
-			case *Func:
-				if check.conf.CompilerErrorMessages {
-					err.errorf(m.pos, "%s.%s redeclared in this block", obj.Name(), m.name)
-				} else {
-					err.errorf(m.pos, "method %s already declared for %s", m.name, obj)
-				}
-			default:
-				unreachable()
+			if check.conf.CompilerErrorMessages {
+				err.errorf(m.pos, "%s.%s redeclared in this block", obj.Name(), m.name)
+			} else {
+				err.errorf(m.pos, "method %s already declared for %s", m.name, obj)
 			}
 			err.recordAltDecl(alt)
 			check.report(&err)
@@ -686,6 +677,36 @@ func (check *Checker) collectMethods(obj *TypeName) {
 	}
 }
 
+func (check *Checker) checkFieldUniqueness(base *Named) {
+	if t, _ := base.under().(*Struct); t != nil {
+		var mset objset
+		for i := 0; i < base.methods.Len(); i++ {
+			m := base.methods.At(i, nil)
+			assert(m.name != "_")
+			assert(mset.insert(m) == nil)
+		}
+
+		// Check that any non-blank field names of base are distinct from its
+		// method names.
+		for _, fld := range t.fields {
+			if fld.name != "_" {
+				if alt := mset.insert(fld); alt != nil {
+					// Struct fields should already be unique, so we should only
+					// encounter an alternate via collision with a method name.
+					_ = alt.(*Func)
+
+					// For historical consistency, we report the primary error on the
+					// method, and the alt decl on the field.
+					var err error_
+					err.errorf(alt, "field and method with the same name %s", fld.name)
+					err.recordAltDecl(fld)
+					check.report(&err)
+				}
+			}
+		}
+	}
+}
+
 func (check *Checker) funcDecl(obj *Func, decl *declInfo) {
 	assert(obj.typ == nil)
 
diff --git a/src/cmd/compile/internal/types2/testdata/fixedbugs/issue52529.go2 b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue52529.go2
new file mode 100644
index 0000000000..de7b2964b0
--- /dev/null
+++ b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue52529.go2
@@ -0,0 +1,15 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package p
+
+type Foo[P any] struct {
+	_ *Bar[P]
+}
+
+type Bar[Q any] Foo[Q]
+
+func (v *Bar[R]) M() {
+	_ = (*Foo[R])(v)
+}
diff --git a/src/go/types/decl.go b/src/go/types/decl.go
index 93a37d76ce..581e711e30 100644
--- a/src/go/types/decl.go
+++ b/src/go/types/decl.go
@@ -712,14 +712,12 @@ func (check *Checker) collectMethods(obj *TypeName) {
 	base, _ := obj.typ.(*Named) // shouldn't fail but be conservative
 	if base != nil {
 		assert(base.targs.Len() == 0) // collectMethods should not be called on an instantiated type
-		u := base.under()
-		if t, _ := u.(*Struct); t != nil {
-			for _, fld := range t.fields {
-				if fld.name != "_" {
-					assert(mset.insert(fld) == nil)
-				}
-			}
-		}
+
+		// See issue #52529: we must delay the expansion of underlying here, as
+		// base may not be fully set-up.
+		check.later(func() {
+			check.checkFieldUniqueness(base)
+		}).describef(obj, "verifying field uniqueness for %v", base)
 
 		// Checker.Files may be called multiple times; additional package files
 		// may add methods to already type-checked types. Add pre-existing methods
@@ -737,14 +735,7 @@ func (check *Checker) collectMethods(obj *TypeName) {
 		// to it must be unique."
 		assert(m.name != "_")
 		if alt := mset.insert(m); alt != nil {
-			switch alt.(type) {
-			case *Var:
-				check.errorf(m, _DuplicateFieldAndMethod, "field and method with the same name %s", m.name)
-			case *Func:
-				check.errorf(m, _DuplicateMethod, "method %s already declared for %s", m.name, obj)
-			default:
-				unreachable()
-			}
+			check.errorf(m, _DuplicateMethod, "method %s already declared for %s", m.name, obj)
 			check.reportAltDecl(alt)
 			continue
 		}
@@ -756,6 +747,34 @@ func (check *Checker) collectMethods(obj *TypeName) {
 	}
 }
 
+func (check *Checker) checkFieldUniqueness(base *Named) {
+	if t, _ := base.under().(*Struct); t != nil {
+		var mset objset
+		for i := 0; i < base.methods.Len(); i++ {
+			m := base.methods.At(i, nil)
+			assert(m.name != "_")
+			assert(mset.insert(m) == nil)
+		}
+
+		// Check that any non-blank field names of base are distinct from its
+		// method names.
+		for _, fld := range t.fields {
+			if fld.name != "_" {
+				if alt := mset.insert(fld); alt != nil {
+					// Struct fields should already be unique, so we should only
+					// encounter an alternate via collision with a method name.
+					_ = alt.(*Func)
+
+					// For historical consistency, we report the primary error on the
+					// method, and the alt decl on the field.
+					check.errorf(alt, _DuplicateFieldAndMethod, "field and method with the same name %s", fld.name)
+					check.reportAltDecl(fld)
+				}
+			}
+		}
+	}
+}
+
 func (check *Checker) funcDecl(obj *Func, decl *declInfo) {
 	assert(obj.typ == nil)
 
diff --git a/src/go/types/testdata/fixedbugs/issue52529.go2 b/src/go/types/testdata/fixedbugs/issue52529.go2
new file mode 100644
index 0000000000..de7b2964b0
--- /dev/null
+++ b/src/go/types/testdata/fixedbugs/issue52529.go2
@@ -0,0 +1,15 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package p
+
+type Foo[P any] struct {
+	_ *Bar[P]
+}
+
+type Bar[Q any] Foo[Q]
+
+func (v *Bar[R]) M() {
+	_ = (*Foo[R])(v)
+}
diff --git a/test/run.go b/test/run.go
index ae5afc751d..3f0232d7a4 100644
--- a/test/run.go
+++ b/test/run.go
@@ -2136,7 +2136,6 @@ var types2Failures = setOf(
 	"fixedbugs/issue18419.go", // types2 reports no field or method member, but should say unexported
 	"fixedbugs/issue20233.go", // types2 reports two instead of one error (pref: -G=0)
 	"fixedbugs/issue20245.go", // types2 reports two instead of one error (pref: -G=0)
-	"fixedbugs/issue28268.go", // types2 reports follow-on errors (pref: -G=0)
 	"fixedbugs/issue31053.go", // types2 reports "unknown field" instead of "cannot refer to unexported field"
 )
 
@@ -2212,11 +2211,11 @@ func setOf(keys ...string) map[string]bool {
 //
 // For example, the following string:
 //
-//     a b:"c d" 'e''f'  "g\""
+//	a b:"c d" 'e''f'  "g\""
 //
 // Would be parsed as:
 //
-//     []string{"a", "b:c d", "ef", `g"`}
+//	[]string{"a", "b:c d", "ef", `g"`}
 //
 // [copied from src/go/build/build.go]
 func splitQuoted(s string) (r []string, err error) {