cmd/compile/internal/types2: return an error from Instantiate

Change Instantiate to be a function (not a method) and return an error.
Introduce an ArgumentError type to report information about which type
argument led to an error during verification.

This resolves a few concerns with the current API:
 - The Checker method set was previously just Files. It is somewhat odd
   to add an additional method for instantiation. Passing the checker as
   an argument seems cleaner.
 - pos, posList, and verify were bound together. In cases where no
   verification is required, the call site was somewhat cluttered.
 - Callers will likely want to access structured information about why
   type information is invalid, and also may not have access to position
   information. Returning an argument index solves both these problems;
   if callers want to associate errors with an argument position, they
   can do this via the resulting index.

We may want to make the first argument an opaque environment rather than
a Checker.

Change-Id: I3bc56d205c13d832b538401a4c91d3917c041225
Reviewed-on: https://go-review.googlesource.com/c/go/+/342152
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>

src/cmd/compile/internal/importer/iimport.go

@@ -652,7 +652,9 @@ func (r *importReader) doType(base *types2.Named) types2.Type {
 		if r.p.exportVersion < iexportVersionGenerics {
 			errorf("unexpected instantiation type")
 		}
-		pos := r.pos()
+		// pos does not matter for instances: they are positioned on the original
+		// type.
+		_ = r.pos()
 		len := r.uint64()
 		targs := make([]types2.Type, len)
 		for i := range targs {
@@ -661,8 +663,8 @@ func (r *importReader) doType(base *types2.Named) types2.Type {
 		baseType := r.typ()
 		// The imported instantiated type doesn't include any methods, so
 		// we must always use the methods of the base (orig) type.
-		var check *types2.Checker // TODO provide a non-nil *Checker
+		// TODO provide a non-nil *Checker
-		t := check.Instantiate(pos, baseType, targs, nil, false)
+		t, _ := types2.Instantiate(nil, baseType, targs, false)
 		return t
 
 	case unionType:

src/cmd/compile/internal/noder/reader2.go

@@ -229,7 +229,8 @@ func (r *reader2) doTyp() (res types2.Type) {
 		obj, targs := r.obj()
 		name := obj.(*types2.TypeName)
 		if len(targs) != 0 {
-			return r.p.check.Instantiate(syntax.Pos{}, name.Type(), targs, nil, false)
+			t, _ := types2.Instantiate(r.p.check, name.Type(), targs, false)
+			return t
 		}
 		return name.Type()
 

src/cmd/compile/internal/types2/api.go

@@ -55,6 +55,18 @@ func (err Error) FullError() string {
 	return fmt.Sprintf("%s: %s", err.Pos, err.Full)
 }
 
+// An ArgumentError holds an error that is associated with an argument.
+type ArgumentError struct {
+	index int
+	error
+}
+
+// Index returns the positional index of the argument associated with the
+// error.
+func (e ArgumentError) Index() int {
+	return e.index
+}
+
 // An Importer resolves import paths to Packages.
 //
 // CAUTION: This interface does not support the import of locally

src/cmd/compile/internal/types2/api_test.go

@@ -1871,8 +1871,10 @@ func TestInstantiate(t *testing.T) {
 
 	// instantiation should succeed (no endless recursion)
 	// even with a nil *Checker
-	var check *Checker
+	res, err := Instantiate(nil, T, []Type{Typ[Int]}, false)
-	res := check.Instantiate(nopos, T, []Type{Typ[Int]}, nil, false)
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	// instantiated type should point to itself
 	if p := res.Underlying().(*Pointer).Elem(); p != res {
@@ -1880,6 +1882,39 @@ func TestInstantiate(t *testing.T) {
 	}
 }
 
+func TestInstantiateErrors(t *testing.T) {
+	tests := []struct {
+		src    string // by convention, T must be the type being instantiated
+		targs  []Type
+		wantAt int // -1 indicates no error
+	}{
+		{"type T[P interface{~string}] int", []Type{Typ[Int]}, 0},
+		{"type T[P1 interface{int}, P2 interface{~string}] int", []Type{Typ[Int], Typ[Int]}, 1},
+		{"type T[P1 any, P2 interface{~[]P1}] int", []Type{Typ[Int], NewSlice(Typ[String])}, 1},
+		{"type T[P1 interface{~[]P2}, P2 any] int", []Type{NewSlice(Typ[String]), Typ[Int]}, 0},
+	}
+
+	for _, test := range tests {
+		src := genericPkg + "p; " + test.src
+		pkg, err := pkgFor(".", src, nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		T := pkg.Scope().Lookup("T").Type().(*Named)
+
+		_, err = Instantiate(nil, T, test.targs, true)
+		if err == nil {
+			t.Fatalf("Instantiate(%v, %v) returned nil error, want non-nil", T, test.targs)
+		}
+
+		gotAt := err.(ArgumentError).Index()
+		if gotAt != test.wantAt {
+			t.Errorf("Instantate(%v, %v): error at index %d, want index %d", T, test.targs, gotAt, test.wantAt)
+		}
+	}
+}
+
 func TestInstanceIdentity(t *testing.T) {
 	imports := make(testImporter)
 	conf := Config{Importer: imports}

src/cmd/compile/internal/types2/call.go

@@ -56,7 +56,7 @@ func (check *Checker) funcInst(x *operand, inst *syntax.IndexExpr) {
 	}
 
 	// instantiate function signature
-	res := check.Instantiate(x.Pos(), sig, targs, poslist, true).(*Signature)
+	res := check.instantiate(x.Pos(), sig, targs, poslist).(*Signature)
 	assert(res.TParams().Len() == 0) // signature is not generic anymore
 	if inferred {
 		check.recordInferred(inst, targs, res)
@@ -326,7 +326,7 @@ func (check *Checker) arguments(call *syntax.CallExpr, sig *Signature, targs []T
 		}
 
 		// compute result signature
-		rsig = check.Instantiate(call.Pos(), sig, targs, nil, true).(*Signature)
+		rsig = check.instantiate(call.Pos(), sig, targs, nil).(*Signature)
 		assert(rsig.TParams().Len() == 0) // signature is not generic anymore
 		check.recordInferred(call, targs, rsig)
 

src/cmd/compile/internal/types2/instantiate.go

@@ -13,54 +13,65 @@ import (
 	"fmt"
 )
 
-// Instantiate instantiates the type typ with the given type arguments
+// Instantiate instantiates the type typ with the given type arguments targs.
-// targs. To check type constraint satisfaction, verify must be set.
+// typ must be a *Named or a *Signature type, and its number of type parameters
-// pos and posList correspond to the instantiation and type argument
+// must match the number of provided type arguments. The result is a new,
-// positions respectively; posList may be nil or shorter than the number
+// instantiated (not parameterized) type of the same kind (either a *Named or a
-// of type arguments provided.
+// *Signature). Any methods attached to a *Named are simply copied; they are
-// typ must be a *Named or a *Signature type, and its number of type
+// not instantiated.
-// parameters must match the number of provided type arguments.
+//
-// The receiver (check) may be nil if and only if verify is not set.
+// If check is non-nil, it will be used to de-dupe the instance against
-// The result is a new, instantiated (not generic) type of the same kind
+// previous instances with the same identity.
-// (either a *Named or a *Signature).
+//
-// Any methods attached to a *Named are simply copied; they are not
+// If verify is set and constraint satisfaction fails, the returned error may
-// instantiated.
+// be of dynamic type ArgumentError indicating which type argument did not
-func (check *Checker) Instantiate(pos syntax.Pos, typ Type, targs []Type, posList []syntax.Pos, verify bool) (res Type) {
+// satisfy its corresponding type parameter constraint, and why.
-	// TODO(gri) What is better here: work with TypeParams, or work with TypeNames?
+//
-	var inst Type
+// TODO(rfindley): change this function to also return an error if lengths of
+// tparams and targs do not match.
+func Instantiate(check *Checker, typ Type, targs []Type, validate bool) (Type, error) {
+	inst := check.instance(nopos, typ, targs)
+
+	var err error
+	if validate {
+		var tparams []*TypeName
 		switch t := typ.(type) {
 		case *Named:
-		inst = check.instantiateLazy(pos, t, targs)
+			tparams = t.TParams().list()
 		case *Signature:
-		tparams := t.TParams().list()
+			tparams = t.TParams().list()
-		defer func() {
-			// If we had an unexpected failure somewhere don't panic below when
-			// asserting res.(*Signature). Check for *Signature in case Typ[Invalid]
-			// is returned.
-			if _, ok := res.(*Signature); !ok {
-				return
 		}
-			// If the signature doesn't use its type parameters, subst
+		if i, err := check.verify(nopos, tparams, targs); err != nil {
-			// will not make a copy. In that case, make a copy now (so
+			return inst, ArgumentError{i, err}
-			// we can set tparams to nil w/o causing side-effects).
-			if t == res {
-				copy := *t
-				res = &copy
 		}
-			// After instantiating a generic signature, it is not generic
-			// anymore; we need to set tparams to nil.
-			res.(*Signature).tparams = nil
-		}()
-		inst = check.instantiate(pos, typ, tparams, targs, nil)
-	default:
-		// only types and functions can be generic
-		panic(fmt.Sprintf("%v: cannot instantiate %v", pos, typ))
 	}
 
-	if verify {
+	return inst, err
-		if check == nil {
+}
-			panic("cannot have nil Checker if verifying constraints")
+
+// instantiate creates an instance and defers verification of constraints to
+// later in the type checking pass. For Named types the resulting instance will
+// be unexpanded.
+func (check *Checker) instantiate(pos syntax.Pos, typ Type, targs []Type, posList []syntax.Pos) (res Type) {
+	if check != nil && check.conf.Trace {
+		check.trace(pos, "-- instantiating %s with %s", typ, typeListString(targs))
+		check.indent++
+		defer func() {
+			check.indent--
+			var under Type
+			if res != nil {
+				// Calling under() here may lead to endless instantiations.
+				// Test case: type T[P any] T[P]
+				// TODO(gri) investigate if that's a bug or to be expected.
+				under = safeUnderlying(res)
 			}
+			check.trace(pos, "=> %s (under = %s)", res, under)
+		}()
+	}
+
+	assert(check != nil)
+	inst := check.instance(pos, typ, targs)
+
 	assert(len(posList) <= len(targs))
 	check.later(func() {
 		// Collect tparams again because lazily loaded *Named types may not have
@@ -85,49 +96,17 @@ func (check *Checker) Instantiate(pos syntax.Pos, typ Type, targs []Type, posLis
 			}
 		}
 	})
-	}
-
 	return inst
 }
 
-func (check *Checker) instantiate(pos syntax.Pos, typ Type, tparams []*TypeName, targs []Type, typMap map[string]*Named) (res Type) {
+// instance creates a type or function instance using the given original type
-	// the number of supplied types must match the number of type parameters
+// typ and arguments targs. For Named types the resulting instance will be
-	if len(targs) != len(tparams) {
+// unexpanded.
-		// TODO(gri) provide better error message
+func (check *Checker) instance(pos syntax.Pos, typ Type, targs []Type) (res Type) {
-		if check != nil {
+	// TODO(gri) What is better here: work with TypeParams, or work with TypeNames?
-			check.errorf(pos, "got %d arguments but %d type parameters", len(targs), len(tparams))
+	switch t := typ.(type) {
-			return Typ[Invalid]
+	case *Named:
-		}
+		h := instantiatedHash(t, targs)
-		panic(fmt.Sprintf("%v: got %d arguments but %d type parameters", pos, len(targs), len(tparams)))
-	}
-
-	if check != nil && check.conf.Trace {
-		check.trace(pos, "-- instantiating %s with %s", typ, typeListString(targs))
-		check.indent++
-		defer func() {
-			check.indent--
-			var under Type
-			if res != nil {
-				// Calling under() here may lead to endless instantiations.
-				// Test case: type T[P any] T[P]
-				// TODO(gri) investigate if that's a bug or to be expected.
-				under = safeUnderlying(res)
-			}
-			check.trace(pos, "=> %s (under = %s)", res, under)
-		}()
-	}
-
-	if len(tparams) == 0 {
-		return typ // nothing to do (minor optimization)
-	}
-
-	return check.subst(pos, typ, makeSubstMap(tparams, targs), typMap)
-}
-
-// instantiateLazy avoids actually instantiating the type until needed. typ
-// must be a *Named type.
-func (check *Checker) instantiateLazy(pos syntax.Pos, orig *Named, targs []Type) Type {
-	h := instantiatedHash(orig, targs)
 		if check != nil {
 			// typ may already have been instantiated with identical type arguments. In
 			// that case, re-use the existing instance.
@@ -136,15 +115,61 @@ func (check *Checker) instantiateLazy(pos syntax.Pos, orig *Named, targs []Type)
 			}
 		}
 
-	tname := NewTypeName(pos, orig.obj.pkg, orig.obj.name, nil)
+		tname := NewTypeName(pos, t.obj.pkg, t.obj.name, nil)
-	named := check.newNamed(tname, orig, nil, nil, nil) // methods and tparams are set when named is loaded
+		named := check.newNamed(tname, t, nil, nil, nil) // methods and tparams are set when named is loaded
 		named.targs = targs
 		named.instance = &instance{pos}
 		if check != nil {
 			check.typMap[h] = named
 		}
+		res = named
+	case *Signature:
+		tparams := t.TParams()
+		if !check.validateTArgLen(pos, tparams, targs) {
+			return Typ[Invalid]
+		}
+		if tparams.Len() == 0 {
+			return typ // nothing to do (minor optimization)
+		}
+		defer func() {
+			// If we had an unexpected failure somewhere don't panic below when
+			// asserting res.(*Signature). Check for *Signature in case Typ[Invalid]
+			// is returned.
+			if _, ok := res.(*Signature); !ok {
+				return
+			}
+			// If the signature doesn't use its type parameters, subst
+			// will not make a copy. In that case, make a copy now (so
+			// we can set tparams to nil w/o causing side-effects).
+			if t == res {
+				copy := *t
+				res = &copy
+			}
+			// After instantiating a generic signature, it is not generic
+			// anymore; we need to set tparams to nil.
+			res.(*Signature).tparams = nil
+		}()
+		res = check.subst(pos, typ, makeSubstMap(tparams.list(), targs), nil)
+	default:
+		// only types and functions can be generic
+		panic(fmt.Sprintf("%v: cannot instantiate %v", pos, typ))
+	}
+	return res
+}
 
-	return named
+// validateTArgLen verifies that the length of targs and tparams matches,
+// reporting an error if not. If validation fails and check is nil,
+// validateTArgLen panics.
+func (check *Checker) validateTArgLen(pos syntax.Pos, tparams *TParamList, targs []Type) bool {
+	if len(targs) != tparams.Len() {
+		// TODO(gri) provide better error message
+		if check != nil {
+			check.errorf(pos, "got %d arguments but %d type parameters", len(targs), tparams.Len())
+			return false
+		}
+		panic(fmt.Sprintf("%v: got %d arguments but %d type parameters", pos, len(targs), tparams.Len()))
+	}
+	return true
 }
 
 func (check *Checker) verify(pos syntax.Pos, tparams []*TypeName, targs []Type) (int, error) {

src/cmd/compile/internal/types2/named.go

@@ -258,6 +258,8 @@ func (n *Named) expand(typMap map[string]*Named) *Named {
 		// tparams. This is done implicitly by the call to n.TParams, but making it
 		// explicit is harmless: load is idempotent.
 		n.load()
+		var u Type
+		if n.check.validateTArgLen(n.instance.pos, n.tparams, n.targs) {
 			if typMap == nil {
 				if n.check != nil {
 					typMap = n.check.typMap
@@ -270,10 +272,12 @@ func (n *Named) expand(typMap map[string]*Named) *Named {
 					typMap = map[string]*Named{h: n}
 				}
 			}
-
+			u = n.check.subst(n.instance.pos, n.orig.underlying, makeSubstMap(n.TParams().list(), n.targs), typMap)
-		inst := n.check.instantiate(n.instance.pos, n.orig.underlying, n.TParams().list(), n.targs, typMap)
+		} else {
-		n.underlying = inst
+			u = Typ[Invalid]
-		n.fromRHS = inst
+		}
+		n.underlying = u
+		n.fromRHS = u
 		n.instance = nil
 	}
 	return n

src/cmd/compile/internal/types2/subst.go

@@ -35,13 +35,12 @@ func (m substMap) lookup(tpar *TypeParam) Type {
 	return tpar
 }
 
-// subst returns the type typ with its type parameters tpars replaced by
+// subst returns the type typ with its type parameters tpars replaced by the
-// the corresponding type arguments targs, recursively.
+// corresponding type arguments targs, recursively. subst doesn't modify the
-// subst is functional in the sense that it doesn't modify the incoming
+// incoming type. If a substitution took place, the result type is different
-// type. If a substitution took place, the result type is different from
+// from from the incoming type.
-// from the incoming type.
 //
-// If the given typMap is nil and check is non-nil, check.typMap is used.
+// If the given typMap is non-nil, it is used in lieu of check.typMap.
 func (check *Checker) subst(pos syntax.Pos, typ Type, smap substMap, typMap map[string]*Named) Type {
 	if smap.empty() {
 		return typ

src/cmd/compile/internal/types2/typexpr.go

@@ -444,7 +444,7 @@ func (check *Checker) instantiatedType(x syntax.Expr, targsx []syntax.Expr, def
 		posList[i] = syntax.StartPos(arg)
 	}
 
-	typ := check.Instantiate(x.Pos(), base, targs, posList, true)
+	typ := check.instantiate(x.Pos(), base, targs, posList)
 	def.setUnderlying(typ)
 
 	// make sure we check instantiation works at least once