go/types: address some TODOs (cleanup)

This is a port of CL 345176 to go/types, though not all TODOs were
present in go/types.

A TODO that still needs to be resolved was added back to types2.

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

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

@@ -434,6 +434,9 @@ 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 {

src/go/types/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 types_test
 
 import (

src/go/types/expr.go

@@ -114,9 +114,7 @@ func (check *Checker) overflow(x *operand, op token.Token, opPos token.Pos) {
 }
 
 // opName returns the name of an operation, or the empty string.
-// For now, only operations that might overflow are handled.
+// Only operations that might overflow are handled.
-// TODO(gri) Expand this to a general mechanism giving names to
-//           nodes?
 func opName(e ast.Expr) string {
 	switch e := e.(type) {
 	case *ast.BinaryExpr:

src/go/types/instantiate.go

@@ -116,8 +116,7 @@ func (check *Checker) instantiate(pos token.Pos, typ Type, targs []Type, posList
 // 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 token.Pos, typ Type, targs []Type) (res Type) {
+func (check *Checker) instance(pos token.Pos, typ Type, targs []Type) Type {
-	// TODO(gri) What is better here: work with TypeParams, or work with TypeNames?
 	switch t := typ.(type) {
 	case *Named:
 		h := instantiatedHash(t, targs)
@@ -128,7 +127,6 @@ func (check *Checker) instance(pos token.Pos, typ Type, targs []Type) (res Type)
 				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)
@@ -136,7 +134,7 @@ func (check *Checker) instance(pos token.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)) {
@@ -145,30 +143,21 @@ func (check *Checker) instance(pos token.Pos, typ Type, targs []Type) (res Type)
 		if tparams.Len() == 0 {
 			return typ // nothing to do (minor optimization)
 		}
-		defer func() {
+		sig := check.subst(pos, typ, makeSubstMap(tparams.list(), targs), nil).(*Signature)
-			// 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 {
+		if sig == t {
-				copy := *t
+			copy := *sig
-				res = &copy
+			sig = &copy
 		}
 		// After instantiating a generic signature, it is not generic
 		// anymore; we need to set tparams to nil.
-			res.(*Signature).tparams = nil
+		sig.tparams = nil
-		}()
+		return sig
-		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
 }
 
 // validateTArgLen verifies that the length of targs and tparams matches,

src/go/types/named.go

@@ -9,8 +9,6 @@ import (
 	"sync"
 )
 
-// TODO(rfindley) 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

src/go/types/self_test.go

@@ -27,12 +27,7 @@ func TestSelf(t *testing.T) {
 	conf := Config{Importer: importer.Default()}
 	_, err = conf.Check("go/types", fset, files, nil)
 	if err != nil {
-		// Importing go/constant doesn't work in the
+		t.Fatal(err)
-		// 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
 	}
 }
 

src/go/types/signature.go

@@ -145,12 +145,7 @@ func (check *Checker) funcType(sig *Signature, recvPar *ast.FieldList, ftyp *ast
 					// 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?
+					tpar.bound = check.subst(tpar.obj.pos, bound, smap, nil)
-					//           (no bound == empty interface)
-					if bound != nil {
-						bound = check.subst(tpar.obj.pos, bound, smap, nil)
-						tpar.bound = bound
-					}
 				}
 			}
 		}

src/go/types/stmt.go

@@ -53,11 +53,6 @@ func (check *Checker) funcBody(decl *declInfo, name string, sig *Signature, body
 		check.error(atPos(body.Rbrace), _MissingReturn, "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)

src/go/types/testdata/check/tinference.go2

@@ -63,9 +63,7 @@ func _() {
 	var _ string = x
 }
 
-// TODO(gri) Need to flag invalid recursive constraints. At the
+func f7[A interface{*B}, B interface{~*A}]() {}
-// moment these cause infinite recursions and stack overflow.
-// func f7[A interface{type B}, B interface{~A}]()
 
 // More realistic examples
 

src/go/types/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
 }
 

src/go/types/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()
 	}