go/types: don't update the underlying type of an imported type

Updating the underlying type of an imported type (even though
is was set to the same type again) leads to a race condition
if the imported package is imported by separate, concurrently
type-checked packages.

Fixes #31749.

Change-Id: Iabb8e8593eb067eb4816c1df81e545ff52d32c6c
Reviewed-on: https://go-review.googlesource.com/c/go/+/201838
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

src/go/types/decl.go

@@ -489,27 +489,34 @@ func (check *Checker) underlying(typ Type) Type {
 	}
 
 	// Otherwise, follow the forward chain.
-	seen := map[*Named]int{n0: 0, n: 1}
-	path := []Object{n0.obj, n.obj}
+	seen := map[*Named]int{n0: 0}
+	path := []Object{n0.obj}
 	for {
 		typ = n.underlying
-		n, _ = typ.(*Named)
-		if n == nil {
+		n1, _ := typ.(*Named)
+		if n1 == nil {
 			break // end of chain
 		}
 
+		seen[n] = len(seen)
+		path = append(path, n.obj)
+		n = n1
+
 		if i, ok := seen[n]; ok {
 			// cycle
 			check.cycleError(path[i:])
 			typ = Typ[Invalid]
 			break
 		}
-
-		seen[n] = len(seen)
-		path = append(path, n.obj)
 	}
 
 	for n := range seen {
+		// We should never have to update the underlying type of an imported type;
+		// those underlying types should have been resolved during the import.
+		// Also, doing so would lead to a race condition (was issue #31749).
+		if n.obj.pkg != check.pkg {
+			panic("internal error: imported type with unresolved underlying type")
+		}
 		n.underlying = typ
 	}
 

src/go/types/issues_test.go

@@ -493,3 +493,33 @@ func (h importHelper) Import(path string) (*Package, error) {
 	}
 	return h.pkg, nil
 }
+
+// TestIssue34921 verifies that we don't update an imported type's underlying
+// type when resolving an underlying type. Specifically, when determining the
+// underlying type of b.T (which is the underlying type of a.T, which is int)
+// we must not set the underlying type of a.T again since that would lead to
+// a race condition if package b is imported elsewhere, in a package that is
+// concurrently type-checked.
+func TestIssue34921(t *testing.T) {
+	defer func() {
+		if r := recover(); r != nil {
+			t.Error(r)
+		}
+	}()
+
+	var sources = []string{
+		`package a; type T int`,
+		`package b; import "a"; type T a.T`,
+	}
+
+	var pkg *Package
+	for _, src := range sources {
+		f := mustParse(t, src)
+		conf := Config{Importer: importHelper{pkg}}
+		res, err := conf.Check(f.Name.Name, fset, []*ast.File{f}, nil)
+		if err != nil {
+			t.Errorf("%q failed to typecheck: %v", src, err)
+		}
+		pkg = res // res is imported by the next package in this test
+	}
+}