go/types: minor cleanup of instantiation

This CL addresses a couple TODOs related to instantiation:
 - factor out resolving the best environment
 - don't eagerly resolve substituted instances

Change-Id: I4a5de7ea7939b6f272991071f591d622dec04b53
Reviewed-on: https://go-review.googlesource.com/c/go/+/349429
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/go/types/named.go

@@ -219,6 +219,21 @@ func (n *Named) setUnderlying(typ Type) {
 	}
 }
 
+// bestEnv returns the best available environment. In order of preference:
+// - the given env, if non-nil
+// - the Checker env, if check is non-nil
+// - a new environment
+func (check *Checker) bestEnv(env *Environment) *Environment {
+	if env != nil {
+		return env
+	}
+	if check != nil {
+		assert(check.conf.Environment != nil)
+		return check.conf.Environment
+	}
+	return NewEnvironment()
+}
+
 // expandNamed ensures that the underlying type of n is instantiated.
 // The underlying type will be Typ[Invalid] if there was an error.
 func expandNamed(env *Environment, n *Named, instPos token.Pos) (tparams *TypeParamList, underlying Type, methods []*Func) {
@@ -227,24 +242,15 @@ func expandNamed(env *Environment, n *Named, instPos token.Pos) (tparams *TypePa
 	check := n.check
 
 	if check.validateTArgLen(instPos, n.orig.tparams.Len(), n.targs.Len()) {
-		// TODO(rfindley): handling an optional Checker and Environment here (and
+		// We must always have an env, to avoid infinite recursion.
-		// in subst) feels overly complicated. Can we simplify?
+		env = check.bestEnv(env)
-		if env == nil {
+		h := env.typeHash(n.orig, n.targs.list())
-			if check != nil {
+		// ensure that an instance is recorded for h to avoid infinite recursion.
-				env = check.conf.Environment
+		env.typeForHash(h, n)
-			} else {
+
-				// If we're instantiating lazily, we might be outside the scope of a
-				// type-checking pass. In that case we won't have a pre-existing
-				// environment, but don't want to create a duplicate of the current
-				// instance in the process of expansion.
-				env = NewEnvironment()
-			}
-			h := env.typeHash(n.orig, n.targs.list())
-			// ensure that an instance is recorded for h to avoid infinite recursion.
-			env.typeForHash(h, n)
-		}
 		smap := makeSubstMap(n.orig.tparams.list(), n.targs.list())
 		underlying = n.check.subst(instPos, n.orig.underlying, smap, env)
+
 		for i := 0; i < n.orig.NumMethods(); i++ {
 			origm := n.orig.Method(i)
 

src/go/types/subst.go

@@ -52,24 +52,12 @@ func (check *Checker) subst(pos token.Pos, typ Type, smap substMap, env *Environ
 	}
 
 	// general case
-	var subst subster
+	subst := subster{
-	subst.pos = pos
+		pos:   pos,
-	subst.smap = smap
+		smap:  smap,
-
+		check: check,
-	if check != nil {
+		env:   check.bestEnv(env),
-		subst.check = check
-		if env == nil {
-			env = check.conf.Environment
-		}
 	}
-	if env == nil {
-		// If we don't have a *Checker and its global type map,
-		// use a local version. Besides avoiding duplicate work,
-		// the type map prevents infinite recursive substitution
-		// for recursive types (example: type T[P any] *T[P]).
-		env = NewEnvironment()
-	}
-	subst.env = env
 	return subst.typ(typ)
 }
 
@@ -227,11 +215,8 @@ func (subst *subster) typ(typ Type) Type {
 		// recursion. The position used here is irrelevant because validation only
 		// occurs on t (we don't call validType on named), but we use subst.pos to
 		// help with debugging.
-		named := subst.check.instance(subst.pos, t.orig, newTArgs, subst.env).(*Named)
+		t.orig.resolve(subst.env)
-		// TODO(rfindley): we probably don't need to resolve here. Investigate if
+		return subst.check.instance(subst.pos, t.orig, newTArgs, subst.env)
-		// this can be removed.
-		named.resolve(subst.env)
-		assert(named.underlying != nil)
 
 		// Note that if we were to expose substitution more generally (not just in
 		// the context of a declaration), we'd have to substitute in
@@ -239,8 +224,6 @@ func (subst *subster) typ(typ Type) Type {
 		//
 		// But this is unnecessary for now.
 
-		return named
-
 	case *TypeParam:
 		return subst.smap.lookup(t)