From d6bdae33e918f779e9e50c020d32042e569368e2 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Wed, 25 Aug 2021 18:13:28 -0700 Subject: 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 Reviewed-by: Matthew Dempsky --- src/cmd/compile/internal/types2/check_test.go | 3 -- src/cmd/compile/internal/types2/expr.go | 4 +- src/cmd/compile/internal/types2/hilbert_test.go | 3 +- src/cmd/compile/internal/types2/instantiate.go | 49 +++++++++------------- src/cmd/compile/internal/types2/named.go | 2 - src/cmd/compile/internal/types2/self_test.go | 7 +--- src/cmd/compile/internal/types2/signature.go | 7 +--- src/cmd/compile/internal/types2/stmt.go | 6 --- .../internal/types2/testdata/check/tinference.go2 | 6 +-- src/cmd/compile/internal/types2/tuple.go | 2 - src/cmd/compile/internal/types2/type.go | 1 - src/cmd/compile/internal/types2/typestring.go | 1 + src/cmd/compile/internal/types2/unify.go | 3 -- 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 = © - } - // 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 = © + } + // 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("") 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 { -- cgit v1.2.3-54-g00ecf