internal/gcimporter: another workaround for race to interface type set

When importing, it is also possible to produce an incomplete interface
instance, which can be racy (as reproduced in the included test).

Partially avoid this by completing interface underlyings of instances.

For golang/go#61561

Change-Id: I8ca2bdc2d03fa1a46179bbd8596d7ef9baf51600
Reviewed-on: https://go-review.googlesource.com/c/tools/+/512955
gopls-CI: kokoro <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: findleyr

internal/gcimporter/gcimporter_test.go

@@ -25,6 +25,7 @@ import (
 	"runtime"
 	"sort"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -769,6 +770,72 @@ func TestIssue51836(t *testing.T) {
 	_ = importPkg(t, "./testdata/aa", tmpdir)
 }
 
+func TestIssue61561(t *testing.T) {
+	testenv.NeedsGo1Point(t, 18) // requires generics
+
+	const src = `package p
+
+type I[P any] interface {
+	m(P)
+	n() P
+}
+
+type J = I[int]
+
+type StillBad[P any] *interface{b(P)}
+
+type K = StillBad[string]
+`
+	fset := token.NewFileSet()
+	f, err := goparser.ParseFile(fset, "p.go", src, 0)
+	if f == nil {
+		// Some test cases may have parse errors, but we must always have a
+		// file.
+		t.Fatalf("ParseFile returned nil file. Err: %v", err)
+	}
+
+	config := &types.Config{}
+	pkg1, err := config.Check("p", fset, []*ast.File{f}, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Export it. (Shallowness isn't important here.)
+	data, err := IExportShallow(fset, pkg1, nil)
+	if err != nil {
+		t.Fatalf("export: %v", err) // any failure to export is a bug
+	}
+
+	// Re-import it.
+	imports := make(map[string]*types.Package)
+	pkg2, err := IImportShallow(fset, GetPackagesFromMap(imports), data, "p", nil)
+	if err != nil {
+		t.Fatalf("import: %v", err) // any failure of IExport+IImport is a bug.
+	}
+
+	insts := []types.Type{
+		pkg2.Scope().Lookup("J").Type(),
+		// This test is still racy, because the incomplete interface is contained
+		// within a nested type expression.
+		//
+		// Uncomment this once golang/go#61561 is fixed.
+		// pkg2.Scope().Lookup("K").Type().Underlying().(*types.Pointer).Elem(),
+	}
+
+	// Use the interface instances concurrently.
+	for _, inst := range insts {
+		var wg sync.WaitGroup
+		for i := 0; i < 2; i++ {
+			wg.Add(1)
+			go func() {
+				defer wg.Done()
+				_ = types.NewMethodSet(inst)
+			}()
+		}
+		wg.Wait()
+	}
+}
+
 func TestIssue57015(t *testing.T) {
 	testenv.NeedsGo1Point(t, 18) // requires generics
 

internal/gcimporter/iimport.go

@@ -328,6 +328,13 @@ func iimportCommon(fset *token.FileSet, getPackages GetPackagesFunc, data []byte
 		typ.Complete()
 	}
 
+	// Workaround for golang/go#61561. See the doc for instanceList for details.
+	for _, typ := range p.instanceList {
+		if iface, _ := typ.Underlying().(*types.Interface); iface != nil {
+			iface.Complete()
+		}
+	}
+
 	return pkgs, nil
 }
 
@@ -358,6 +365,12 @@ type iimporter struct {
 	fake          fakeFileSet
 	interfaceList []*types.Interface
 
+	// Workaround for the go/types bug golang/go#61561: instances produced during
+	// instantiation may contain incomplete interfaces. Here we only complete the
+	// underlying type of the instance, which is the most common case but doesn't
+	// handle parameterized interface literals defined deeper in the type.
+	instanceList []types.Type // instances for later completion (see golang/go#61561)
+
 	// Arguments for calls to SetConstraint that are deferred due to recursive types
 	later []setConstraintArgs
 
@@ -954,6 +967,9 @@ func (r *importReader) doType(base *types.Named) (res types.Type) {
 		// we must always use the methods of the base (orig) type.
 		// TODO provide a non-nil *Environment
 		t, _ := typeparams.Instantiate(nil, baseType, targs, false)
+
+		// Workaround for golang/go#61561. See the doc for instanceList for details.
+		r.p.instanceList = append(r.p.instanceList, t)
 		return t
 
 	case unionType: