go/types: postpone interface method type comparison to the end

Introduce a new list of final actions that is executed at the
end of type checking and use it to collect method type comparisons
and also map key checks.

Fixes #33656.

Change-Id: Ia77a35a45a9d7eaa7fc3e9e19f41f32dcd6ef9d9
Reviewed-on: https://go-review.googlesource.com/c/go/+/191418
Reviewed-by: Alan Donovan <adonovan@google.com>

src/go/types/check.go

@@ -89,7 +89,8 @@ type Checker struct {
 	firstErr error                 // first error encountered
 	methods  map[*TypeName][]*Func // maps package scope type names to associated non-blank (non-interface) methods
 	untyped  map[ast.Expr]exprInfo // map of expressions without final type
-	delayed  []func()              // stack of delayed actions
+	delayed  []func()              // stack of delayed action segments; segments are processed in FIFO order
+	finals   []func()              // list of final actions; processed at the end of type-checking the current set of files
 	objPath  []Object              // path of object dependencies during type inference (for cycle reporting)
 
 	// context within which the current object is type-checked
@@ -145,6 +146,14 @@ func (check *Checker) later(f func()) {
 	check.delayed = append(check.delayed, f)
 }
 
+// atEnd adds f to the list of actions processed at the end
+// of type-checking, before initialization order computation.
+// Actions added by atEnd are processed after any actions
+// added by later.
+func (check *Checker) atEnd(f func()) {
+	check.finals = append(check.finals, f)
+}
+
 // push pushes obj onto the object path and returns its index in the path.
 func (check *Checker) push(obj Object) int {
 	check.objPath = append(check.objPath, obj)
@@ -195,6 +204,7 @@ func (check *Checker) initFiles(files []*ast.File) {
 	check.methods = nil
 	check.untyped = nil
 	check.delayed = nil
+	check.finals = nil
 
 	// determine package name and collect valid files
 	pkg := check.pkg
@@ -245,6 +255,7 @@ func (check *Checker) checkFiles(files []*ast.File) (err error) {
 	check.packageObjects()
 
 	check.processDelayed(0) // incl. all functions
+	check.processFinals()
 
 	check.initOrder()
 
@@ -258,6 +269,16 @@ func (check *Checker) checkFiles(files []*ast.File) (err error) {
 	return
 }
 
+func (check *Checker) processFinals() {
+	n := len(check.finals)
+	for _, f := range check.finals {
+		f() // must not append to check.finals
+	}
+	if len(check.finals) != n {
+		panic("internal error: final action list grew")
+	}
+}
+
 func (check *Checker) recordUntyped() {
 	if !debug && check.Types == nil {
 		return // nothing to do

src/go/types/testdata/cycles2.src

@@ -45,13 +45,11 @@ type B interface {
 type AB interface {
 	a() interface {
 		A
-		// TODO(gri) there shouldn't be an error here. See issue #33656.
-		B // ERROR duplicate method a
+		B
 	}
 	b() interface {
 		A
-		// TODO(gri) there shouldn't be an error here. See issue #33656.
-		B // ERROR duplicate method a
+		B
 	}
 }
 

src/go/types/type.go

@@ -351,16 +351,18 @@ func (t *Interface) Complete() *Interface {
 
 	t.allMethods = markComplete // avoid infinite recursion
 
+	var todo []*Func
 	var methods []*Func
 	var seen objset
 	addMethod := func(m *Func, explicit bool) {
-		switch alt := seen.insert(m); {
-		case alt == nil:
+		switch other := seen.insert(m); {
+		case other == nil:
 			methods = append(methods, m)
-		case explicit || !Identical(m.Type(), alt.Type()):
+		case explicit:
 			panic("duplicate method " + m.name)
 		default:
-			// silently drop method m
+			// check method signatures after all locally embedded interfaces are computed
+			todo = append(todo, m, other.(*Func))
 		}
 	}
 
@@ -376,6 +378,14 @@ func (t *Interface) Complete() *Interface {
 		}
 	}
 
+	for i := 0; i < len(todo); i += 2 {
+		m := todo[i]
+		other := todo[i+1]
+		if !Identical(m.typ, other.typ) {
+			panic("duplicate method " + m.name)
+		}
+	}
+
 	if methods != nil {
 		sort.Sort(byUniqueMethodName(methods))
 		t.allMethods = methods

src/go/types/typexpr.go

@@ -314,7 +314,7 @@ func (check *Checker) typInternal(e ast.Expr, def *Named) Type {
 		//
 		// Delay this check because it requires fully setup types;
 		// it is safe to continue in any case (was issue 6667).
-		check.later(func() {
+		check.atEnd(func() {
 			if !Comparable(typ.key) {
 				check.errorf(e.Key.Pos(), "invalid map key type %s", typ.key)
 			}
@@ -560,17 +560,20 @@ func (check *Checker) completeInterface(ityp *Interface) {
 	var methods []*Func
 	var seen objset
 	addMethod := func(m *Func, explicit bool) {
-		switch alt := seen.insert(m); {
-		case alt == nil:
+		switch other := seen.insert(m); {
+		case other == nil:
 			methods = append(methods, m)
-		case explicit || !Identical(m.Type(), alt.Type()):
+		case explicit:
 			check.errorf(m.pos, "duplicate method %s", m.name)
-			// We use "other" rather than "previous" here because
-			// the first declaration seen may not be textually
-			// earlier in the source.
-			check.errorf(alt.Pos(), "\tother declaration of %s", m) // secondary error, \t indented
+			check.reportAltDecl(other)
 		default:
-			// silently drop method m
+			// check method signatures after all types are computed (issue #33656)
+			check.atEnd(func() {
+				if !Identical(m.typ, other.Type()) {
+					check.errorf(m.pos, "duplicate method %s", m.name)
+					check.reportAltDecl(other)
+				}
+			})
 		}
 	}
 
@@ -581,7 +584,7 @@ func (check *Checker) completeInterface(ityp *Interface) {
 	posList := check.posMap[ityp]
 	for i, typ := range ityp.embeddeds {
 		pos := posList[i] // embedding position
-		typ := typ.Underlying().(*Interface)
+		typ := underlying(typ).(*Interface)
 		check.completeInterface(typ)
 		for _, m := range typ.allMethods {
 			copy := *m