go/types: type-check embedded methods in correct scope (regression)

Change https://go-review.googlesource.com/79575 fixed the computation
of recursive method sets by separating the method set computation from
type computation. However, it didn't track an embedded method's scope
and as a result, some methods' signatures were typed in the wrong
context.

This change tracks embedded methods together with their scope and
uses that scope for the correct context setup when typing those
method signatures.

Fixes #23914.

Change-Id: If3677dceddb43e9db2f9fb3c7a4a87d2531fbc2a
Reviewed-on: https://go-review.googlesource.com/96376
Reviewed-by: Alan Donovan <[email protected]>

Author: griesemer

src/go/types/interfaces.go

@@ -53,12 +53,14 @@ func (info *ifaceInfo) String() string {
 
 // methodInfo represents an interface method.
 // At least one of src or fun must be non-nil.
-// (Methods declared in the current package have a non-nil src,
-// and eventually a non-nil fun field; imported and predeclared
-// methods have a nil src, and only a non-nil fun field.)
+// (Methods declared in the current package have a non-nil scope
+// and src, and eventually a non-nil fun field; imported and pre-
+// declared methods have a nil scope and src, and only a non-nil
+// fun field.)
 type methodInfo struct {
-	src *ast.Field // syntax tree representation of interface method; or nil
-	fun *Func      // corresponding fully type-checked method type; or nil
+	scope *Scope     // scope of interface method; or nil
+	src   *ast.Field // syntax tree representation of interface method; or nil
+	fun   *Func      // corresponding fully type-checked method type; or nil
 }
 
 func (info *methodInfo) String() string {
@@ -124,15 +126,16 @@ func (check *Checker) reportAltMethod(m *methodInfo) {
 	}
 }
 
-// infoFromTypeLit computes the method set for the given interface iface.
+// infoFromTypeLit computes the method set for the given interface iface
+// declared in scope.
 // If a corresponding type name exists (tname != nil), it is used for
 // cycle detection and to cache the method set.
 // The result is the method set, or nil if there is a cycle via embedded
 // interfaces. A non-nil result doesn't mean that there were no errors,
 // but they were either reported (e.g., blank methods), or will be found
 // (again) when computing the interface's type.
 // If tname is not nil it must be the last element in path.
-func (check *Checker) infoFromTypeLit(iface *ast.InterfaceType, tname *TypeName, path []*TypeName) (info *ifaceInfo) {
+func (check *Checker) infoFromTypeLit(scope *Scope, iface *ast.InterfaceType, tname *TypeName, path []*TypeName) (info *ifaceInfo) {
 	assert(iface != nil)
 
 	// lazy-allocate interfaces map
@@ -207,7 +210,7 @@ func (check *Checker) infoFromTypeLit(iface *ast.InterfaceType, tname *TypeName,
 					continue // ignore
 				}
 
-				m := &methodInfo{src: f}
+				m := &methodInfo{scope: scope, src: f}
 				if check.declareInMethodSet(&mset, f.Pos(), m) {
 					info.methods = append(info.methods, m)
 				}
@@ -333,7 +336,7 @@ typenameLoop:
 			return check.infoFromQualifiedTypeName(typ)
 		case *ast.InterfaceType:
 			// type tname interface{...}
-			return check.infoFromTypeLit(typ, tname, path)
+			return check.infoFromTypeLit(decl.file, typ, tname, path)
 		}
 		// type tname X // and X is not an interface type
 		return nil

src/go/types/testdata/importdecl1a.src

@@ -6,6 +6,17 @@
 
 package importdecl1
 
+import "go/ast"
 import . "unsafe"
 
 var _ Pointer // use dot-imported package unsafe
+
+// Test cases for issue 23914.
+
+type A interface {
+	// Methods m1, m2 must be type-checked in this file scope
+	// even when embedded in an interface in a different
+	// file of the same package.
+	m1() ast.Node
+	m2() Pointer
+}

src/go/types/testdata/importdecl1b.src

@@ -5,3 +5,7 @@
 package importdecl1
 
 import . /* ERROR "imported but not used" */ "unsafe"
+
+type B interface {
+	A
+}

src/go/types/typexpr.go

@@ -481,9 +481,9 @@ func (check *Checker) interfaceType(ityp *Interface, iface *ast.InterfaceType, d
 	// collect embedded interfaces
 	// Only needed for printing and API. Delay collection
 	// to end of type-checking when all types are complete.
-	interfaceScope := check.scope // capture for use in closure below
+	interfaceContext := check.context // capture for use in closure below
 	check.later(func() {
-		check.scope = interfaceScope
+		check.context = interfaceContext
 		if trace {
 			check.trace(iface.Pos(), "-- delayed checking embedded interfaces of %s", iface)
 			check.indent++
@@ -495,7 +495,7 @@ func (check *Checker) interfaceType(ityp *Interface, iface *ast.InterfaceType, d
 			if len(f.Names) == 0 {
 				typ := check.typ(f.Type)
 				// typ should be a named type denoting an interface
-				// (the parser will make sure it's a name type but
+				// (the parser will make sure it's a named type but
 				// constructed ASTs may be wrong).
 				if typ == Typ[Invalid] {
 					continue // error reported before
@@ -531,7 +531,7 @@ func (check *Checker) interfaceType(ityp *Interface, iface *ast.InterfaceType, d
 	if def != nil {
 		tname = def.obj
 	}
-	info := check.infoFromTypeLit(iface, tname, path)
+	info := check.infoFromTypeLit(check.scope, iface, tname, path)
 	if info == nil || info == &emptyIfaceInfo {
 		// error or empty interface - exit early
 		ityp.allMethods = markComplete
@@ -574,7 +574,11 @@ func (check *Checker) interfaceType(ityp *Interface, iface *ast.InterfaceType, d
 	}
 
 	// fix signatures now that we have collected all methods
+	savedContext := check.context
 	for _, minfo := range sigfix {
+		// (possibly embedded) methods must be type-checked within their scope and
+		// type-checking them must not affect the current context (was issue #23914)
+		check.context = context{scope: minfo.scope}
 		typ := check.typ(minfo.src.Type)
 		sig, _ := typ.(*Signature)
 		if sig == nil {
@@ -588,6 +592,7 @@ func (check *Checker) interfaceType(ityp *Interface, iface *ast.InterfaceType, d
 		sig.recv = old.recv
 		*old = *sig // update signature (don't replace pointer!)
 	}
+	check.context = savedContext
 
 	// sort to match NewInterface
 	// TODO(gri) we may be able to switch to source order