cmd/compile/internal/types2: address some TODOs (cleanup)

- Address some easy TODOs.
- Remove some TODOs that are not correct anymore or are unimportent.
- Simplify some code on the way.

Change-Id: I4d20de3725b3a735022afe022cbc002b2798936d
Reviewed-on: https://go-review.googlesource.com/c/go/+/345176
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

13 files changed, 27 insertions, 67 deletions

diff --git a/src/cmd/compile/internal/types2/check_test.go b/src/cmd/compile/internal/types2/check_test.go
index 41b0c54702..bc68e76407 100644
--- a/src/cmd/compile/internal/types2/check_test.go
+++ b/src/cmd/compile/internal/types2/check_test.go
@@ -20,9 +20,6 @@
 //		_ = x /* ERROR "not declared" */ + 1
 //	}
 
-// TODO(gri) Also collect strict mode errors of the form /* STRICT ... */
-//           and test against strict mode.
-
 package types2_test
 
 import (
diff --git a/src/cmd/compile/internal/types2/expr.go b/src/cmd/compile/internal/types2/expr.go
index d108093dac..86a8444ee2 100644
--- a/src/cmd/compile/internal/types2/expr.go
+++ b/src/cmd/compile/internal/types2/expr.go
@@ -127,9 +127,7 @@ func (check *Checker) overflow(x *operand) {
 }
 
 // opName returns the name of an operation, or the empty string.
-// For now, only operations that might overflow are handled.
-// TODO(gri) Expand this to a general mechanism giving names to
-//           nodes?
+// Only operations that might overflow are handled.
 func opName(e *syntax.Operation) string {
 	op := int(e.Op)
 	if e.Y == nil {
diff --git a/src/cmd/compile/internal/types2/hilbert_test.go b/src/cmd/compile/internal/types2/hilbert_test.go
index 9f9dad6b64..03fea4fe7c 100644
--- a/src/cmd/compile/internal/types2/hilbert_test.go
+++ b/src/cmd/compile/internal/types2/hilbert_test.go
@@ -29,8 +29,7 @@ func TestHilbert(t *testing.T) {
 	}
 
 	// parse source
-	// TODO(gri) get rid of []bytes to string conversion below
-	f, err := parseSrc("hilbert.go", string(src))
+	f, err := syntax.Parse(syntax.NewFileBase("hilbert.go"), bytes.NewReader(src), nil, nil, 0)
 	if err != nil {
 		t.Fatal(err)
 	}
diff --git a/src/cmd/compile/internal/types2/instantiate.go b/src/cmd/compile/internal/types2/instantiate.go
index 8bea63ec86..b78ac3bea3 100644
--- a/src/cmd/compile/internal/types2/instantiate.go
+++ b/src/cmd/compile/internal/types2/instantiate.go
@@ -122,19 +122,17 @@ func (check *Checker) instantiate(pos syntax.Pos, typ Type, targs []Type, posLis
 // instance creates a type or function instance using the given original type
 // typ and arguments targs. For Named types the resulting instance will be
 // unexpanded.
-func (check *Checker) instance(pos syntax.Pos, typ Type, targs []Type) (res Type) {
-	// TODO(gri) What is better here: work with TypeParams, or work with TypeNames?
+func (check *Checker) instance(pos syntax.Pos, typ Type, targs []Type) Type {
 	switch t := typ.(type) {
 	case *Named:
 		h := instantiatedHash(t, targs)
 		if check != nil {
-			// typ may already have been instantiated with identical type arguments. In
-			// that case, re-use the existing instance.
+			// typ may already have been instantiated with identical type arguments.
+			// In that case, re-use the existing instance.
 			if named := check.typMap[h]; named != nil {
 				return named
 			}
 		}
-
 		tname := NewTypeName(pos, t.obj.pkg, t.obj.name, nil)
 		named := check.newNamed(tname, t, nil, nil, nil) // methods and tparams are set when named is loaded
 		named.targs = NewTypeList(targs)
@@ -142,7 +140,8 @@ func (check *Checker) instance(pos syntax.Pos, typ Type, targs []Type) (res Type
 		if check != nil {
 			check.typMap[h] = named
 		}
-		res = named
+		return named
+
 	case *Signature:
 		tparams := t.TParams()
 		if !check.validateTArgLen(pos, tparams.Len(), len(targs)) {
@@ -151,30 +150,22 @@ func (check *Checker) instance(pos syntax.Pos, typ Type, targs []Type) (res Type
 		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))
+		sig := check.subst(pos, typ, makeSubstMap(tparams.list(), targs), nil).(*Signature)
+		// 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 sig == t {
+			copy := *sig
+			sig = &copy
+		}
+		// After instantiating a generic signature, it is not generic
+		// anymore; we need to set tparams to nil.
+		sig.tparams = nil
+		return sig
 	}
-	return res
+
+	// only types and functions can be generic
+	panic(fmt.Sprintf("%v: cannot instantiate %v", pos, typ))
 }
 
 // validateTArgLen verifies that the length of targs and tparams matches,
diff --git a/src/cmd/compile/internal/types2/named.go b/src/cmd/compile/internal/types2/named.go
index a3a2595a22..ccb1f265be 100644
--- a/src/cmd/compile/internal/types2/named.go
+++ b/src/cmd/compile/internal/types2/named.go
@@ -9,8 +9,6 @@ import (
 	"sync"
 )
 
-// TODO(gri) Clean up Named struct below; specifically the fromRHS field (can we use underlying?).
-
 // A Named represents a named (defined) type.
 type Named struct {
 	check      *Checker
diff --git a/src/cmd/compile/internal/types2/self_test.go b/src/cmd/compile/internal/types2/self_test.go
index 4722fec988..e0d2e1b07a 100644
--- a/src/cmd/compile/internal/types2/self_test.go
+++ b/src/cmd/compile/internal/types2/self_test.go
@@ -24,12 +24,7 @@ func TestSelf(t *testing.T) {
 	conf := Config{Importer: defaultImporter()}
 	_, err = conf.Check("cmd/compile/internal/types2", files, nil)
 	if err != nil {
-		// Importing go/constant doesn't work in the
-		// build dashboard environment. Don't report an error
-		// for now so that the build remains green.
-		// TODO(gri) fix this
-		t.Log(err) // replace w/ t.Fatal eventually
-		return
+		t.Fatal(err)
 	}
 }
 
diff --git a/src/cmd/compile/internal/types2/signature.go b/src/cmd/compile/internal/types2/signature.go
index f1bf60ae8e..d28e7b8944 100644
--- a/src/cmd/compile/internal/types2/signature.go
+++ b/src/cmd/compile/internal/types2/signature.go
@@ -150,12 +150,7 @@ func (check *Checker) funcType(sig *Signature, recvPar *syntax.Field, tparams []
 					// bound is (possibly) parameterized in the context of the
 					// receiver type declaration. Substitute parameters for the
 					// current context.
-					// TODO(gri) should we assume now that bounds always exist?
-					//           (no bound == empty interface)
-					if bound != nil {
-						bound = check.subst(tpar.obj.pos, bound, smap, nil)
-						tpar.bound = bound
-					}
+					tpar.bound = check.subst(tpar.obj.pos, bound, smap, nil)
 				}
 			}
 		}
diff --git a/src/cmd/compile/internal/types2/stmt.go b/src/cmd/compile/internal/types2/stmt.go
index 7865c2d4f4..8cfdf92e67 100644
--- a/src/cmd/compile/internal/types2/stmt.go
+++ b/src/cmd/compile/internal/types2/stmt.go
@@ -52,11 +52,6 @@ func (check *Checker) funcBody(decl *declInfo, name string, sig *Signature, body
 		check.error(body.Rbrace, "missing return")
 	}
 
-	// TODO(gri) Should we make it an error to declare generic functions
-	//           where the type parameters are not used?
-	// 12/19/2018: Probably not - it can make sense to have an API with
-	//           all functions uniformly sharing the same type parameters.
-
 	// spec: "Implementation restriction: A compiler may make it illegal to
 	// declare a variable inside a function body if the variable is never used."
 	check.usage(sig.scope)
@@ -422,7 +417,6 @@ func (check *Checker) stmt(ctxt stmtContext, s syntax.Stmt) {
 		check.assignVar(lhs[0], &x)
 
 	case *syntax.CallStmt:
-		// TODO(gri) get rid of this conversion to string
 		kind := "go"
 		if s.Tok == syntax.Defer {
 			kind = "defer"
diff --git a/src/cmd/compile/internal/types2/testdata/check/tinference.go2 b/src/cmd/compile/internal/types2/testdata/check/tinference.go2
index 0afb77c1e4..2409fef4ae 100644
--- a/src/cmd/compile/internal/types2/testdata/check/tinference.go2
+++ b/src/cmd/compile/internal/types2/testdata/check/tinference.go2
@@ -15,7 +15,7 @@ type any interface{}
 // 	f("a", "b", "c", "d")
 // 	f0("a", "b", "c", "d")
 // }
-// 
+//
 // func f1[A any, B interface{~A}](A, B)
 // func _() {
 // 	f := f1[int]
@@ -60,9 +60,7 @@ func _() {
 	var _ string = x
 }
 
-// TODO(gri) Need to flag invalid recursive constraints. At the
-// moment these cause infinite recursions and stack overflow.
-// func f7[A interface{type B}, B interface{~A}]()
+func f7[A interface{*B}, B interface{~*A}]() {}
 
 // More realistic examples
 
diff --git a/src/cmd/compile/internal/types2/tuple.go b/src/cmd/compile/internal/types2/tuple.go
index a3946beab5..1356aae0b0 100644
--- a/src/cmd/compile/internal/types2/tuple.go
+++ b/src/cmd/compile/internal/types2/tuple.go
@@ -16,8 +16,6 @@ func NewTuple(x ...*Var) *Tuple {
 	if len(x) > 0 {
 		return &Tuple{vars: x}
 	}
-	// TODO(gri) Don't represent empty tuples with a (*Tuple)(nil) pointer;
-	//           it's too subtle and causes problems.
 	return nil
 }
 
diff --git a/src/cmd/compile/internal/types2/type.go b/src/cmd/compile/internal/types2/type.go
index 4b8642aa96..ca5ecdc434 100644
--- a/src/cmd/compile/internal/types2/type.go
+++ b/src/cmd/compile/internal/types2/type.go
@@ -34,7 +34,6 @@ func (t *top) String() string   { return TypeString(t, nil) }
 // under must only be called when a type is known
 // to be fully set up.
 func under(t Type) Type {
-	// TODO(gri) is this correct for *Union?
 	if n := asNamed(t); n != nil {
 		return n.under()
 	}
diff --git a/src/cmd/compile/internal/types2/typestring.go b/src/cmd/compile/internal/types2/typestring.go
index 2c34d036db..9980408593 100644
--- a/src/cmd/compile/internal/types2/typestring.go
+++ b/src/cmd/compile/internal/types2/typestring.go
@@ -279,6 +279,7 @@ func writeTParamList(buf *bytes.Buffer, list []*TypeParam, qf Qualifier, visited
 
 func writeTypeName(buf *bytes.Buffer, obj *TypeName, qf Qualifier) {
 	if obj == nil {
+		assert(instanceHashing == 0) // we need an object for instance hashing
 		buf.WriteString("<Named w/o object>")
 		return
 	}
diff --git a/src/cmd/compile/internal/types2/unify.go b/src/cmd/compile/internal/types2/unify.go
index d4fbebc11b..72542e7d2e 100644
--- a/src/cmd/compile/internal/types2/unify.go
+++ b/src/cmd/compile/internal/types2/unify.go
@@ -433,9 +433,6 @@ func (u *unifier) nify(x, y Type, p *ifacePair) bool {
 			xargs := x.targs.list()
 			yargs := y.targs.list()
 
-			// TODO(gri) This is not always correct: two types may have the same names
-			//           in the same package if one of them is nested in a function.
-			//           Extremely unlikely but we need an always correct solution.
 			if x.obj.pkg == y.obj.pkg && x.obj.name == y.obj.name {
 				assert(len(xargs) == len(yargs))
 				for i, x := range xargs {