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.

Fixes #52529

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>

diff --git a/src/cmd/compile/internal/types2/decl.go b/src/cmd/compile/internal/types2/decl.go
index 4f28c362c76ec817d5770c818909763b61c9d949..9176358dd5f6aa6dea72d5e080051581f6297282 100644 (file)
--- 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.go b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue52529.go
new file mode 100644 (file)
index 0000000..de7b296
--- /dev/null
+++ b/src/cmd/compile/internal/types2/testdata/fixedbugs/issue52529.go
@@ -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 a20b56c9502f6334ea476b3a1f37c5013302262f..7229104190278562fb67e41ca10bc0c559b3ddb5 100644 (file)
--- 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.go b/src/go/types/testdata/fixedbugs/issue52529.go
new file mode 100644 (file)
index 0000000..de7b296
--- /dev/null
+++ b/src/go/types/testdata/fixedbugs/issue52529.go
@@ -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 45cd086fc43191f0cd02dfa89e06540acc9bf312..27e16f689235ffb01049893f1314a68a32c80ffc 100644 (file)
--- a/test/run.go
+++ b/test/run.go
@@ -1950,7 +1950,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 (preference: 1.17 compiler)
        "fixedbugs/issue20245.go", // types2 reports two instead of one error (preference: 1.17 compiler)
-       "fixedbugs/issue28268.go", // types2 reports follow-on errors (preference: 1.17 compiler)
        "fixedbugs/issue31053.go", // types2 reports "unknown field" instead of "cannot refer to unexported field"
 )
 
@@ -2022,11 +2021,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) {