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>

diff --git a/src/cmd/compile/internal/types2/unify.go b/src/cmd/compile/internal/types2/unify.go
index 9eb1f63090986112db3856013ca8aef6ecabc36f..a1e5b3679bff17a47037d1a2abf1dad295ef0d1e 100644 (file)
--- a/src/cmd/compile/internal/types2/unify.go
+++ b/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 {

diff --git a/src/go/types/check_test.go b/src/go/types/check_test.go
index 8c8452c9c683d26c478fcef3b6808560c87f978e..e9df90c4ea3c4a55eef229d081e4789a11dc867e 100644 (file)
--- a/src/go/types/check_test.go
+++ b/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 (

diff --git a/src/go/types/expr.go b/src/go/types/expr.go
index 61d57cc4fa78906fd48a1182394dd9d2f73ca9ad..2a204cf5f688a4f54521090f4f2f9812db2385af 100644 (file)
--- a/src/go/types/expr.go
+++ b/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.
-// TODO(gri) Expand this to a general mechanism giving names to
-//           nodes?
+// Only operations that might overflow are handled.
 func opName(e ast.Expr) string {
        switch e := e.(type) {
        case *ast.BinaryExpr:

diff --git a/src/go/types/instantiate.go b/src/go/types/instantiate.go
index 5f691d5246b3f515e61ea958c5f1f0ff4a9e2d7d..09c2ecf8b4cd8aaca9389de20b31303a72165364 100644 (file)
--- a/src/go/types/instantiate.go
+++ b/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) {
-       // TODO(gri) What is better here: work with TypeParams, or work with TypeNames?
+func (check *Checker) instance(pos token.Pos, typ Type, targs []Type) Type {
        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() {
-                       // 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/go/types/named.go b/src/go/types/named.go
index 6bc33b95389762df98334d360fd12f98c24a1b7b..6f89922a4140980e8c28a164ca39319ca5a5a04b 100644 (file)
--- a/src/go/types/named.go
+++ b/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

diff --git a/src/go/types/self_test.go b/src/go/types/self_test.go
index 262dc7b97abe931c49dc916681ed98b62befd745..55436d3b6225a0e709a82ed43dc5f155e3afeaa5 100644 (file)
--- a/src/go/types/self_test.go
+++ b/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
-               // 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/go/types/signature.go b/src/go/types/signature.go
index d1d50b38c4c9863ef6b6d17bd9a9fc2c04b0f1f7..2e6ab4d88a48b52689d70aa268146ce9c29b56d9 100644 (file)
--- a/src/go/types/signature.go
+++ b/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?
-                                       //           (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/go/types/stmt.go b/src/go/types/stmt.go
index 056b21e3d2bc3dd62a748a5c94e54767050fd3e7..5ba57041bd75413f4d4e5e40bc81ba1c2e5fa2d3 100644 (file)
--- a/src/go/types/stmt.go
+++ b/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)

diff --git a/src/go/types/testdata/check/tinference.go2 b/src/go/types/testdata/check/tinference.go2
index 5bd2ba74e79e33004b65bd7708b19b413ac0ae55..28516ef6399ced97cc9897eff0ff8480e4e0737c 100644 (file)
--- a/src/go/types/testdata/check/tinference.go2
+++ b/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
-// 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/go/types/tuple.go b/src/go/types/tuple.go
index 16d28bc9a6fc222a4dee0a1de842b318df81cc9d..e85c5aa81bef566c2af58fbb0d1b144dce537147 100644 (file)
--- a/src/go/types/tuple.go
+++ b/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
 }
 

diff --git a/src/go/types/type.go b/src/go/types/type.go
index 3be42a15846e19a954bc0cc4da3016083ed5fe5a..b9634cf6f663606d0011e31da2b88d9171ffd07a 100644 (file)
--- a/src/go/types/type.go
+++ b/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()
        }