From 53a8d66b5b7f6957ccf3d797cb4087578525de12 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Fri, 20 Dec 2019 22:15:53 -0800 Subject: [PATCH] go/types: temporary fix for subtle signature instantiation bug A signature that's instantiated but doesn't have any incoming or result (value) parameters doesn't get a copy automatically. This leads to bugs because the instantiated signature doesn't lose its type parameters when it should. Make a copy outside for now, this fixes some (but not all cases) and added test cases. Also, factored out printing of type parameters in type printing. Change-Id: I0ec3a4226c7473cddfb16704a2218992dd411593 --- src/go/types/NOTES | 5 ++++ src/go/types/call.go | 14 ++++++++--- src/go/types/contracts.go | 7 +++--- src/go/types/subst.go | 18 +++++++++------ src/go/types/testdata/contracts.go2 | 24 +++++++++++++++++++ src/go/types/testdata/tmp.go2 | 10 ++++++++ src/go/types/typestring.go | 36 ++++++++++++++++++----------- 7 files changed, 86 insertions(+), 28 deletions(-) diff --git a/src/go/types/NOTES b/src/go/types/NOTES index 6d521d61cc..a5cf7a6cc7 100644 --- a/src/go/types/NOTES +++ b/src/go/types/NOTES @@ -11,6 +11,8 @@ TODO - use []*TypeParam for tparams in subst? (unclear) OPEN ISSUES +- instantiating a parameterized function type w/o value or result parameters may have unexpected side-effects + (we don't make a copy of the signature in some cases) - investigate - using a contract and enumerating type arguments currently leads to an error (e.g. func f(type T C(T)) (x T) ... ) DESIGN/IMPLEMENTATION @@ -39,3 +41,6 @@ DESIGN/IMPLEMENTATION - 12/20/2019: Decided to start moving type parameters to types (from TypeName to Named), need to do the same for Func. This make more sense as in general any type (conceptually even literal types) could have type parameters. It's a property of the type, not the type name. It also simplified the code. + +- 12/20/2019: Type parameters may be part of type lists in contracts/interfaces. It just falls out + naturally. Added test cases. diff --git a/src/go/types/call.go b/src/go/types/call.go index 056cef8337..e6ed5773a3 100644 --- a/src/go/types/call.go +++ b/src/go/types/call.go @@ -106,9 +106,17 @@ func (check *Checker) call(x *operand, e *ast.CallExpr) exprKind { } // instantiate function signature - sig = check.instantiate(x.pos(), sig, targs, poslist).(*Signature) - sig.tparams = nil // signature is not generic anymore - x.typ = sig + res := check.instantiate(x.pos(), sig, targs, poslist).(*Signature) + // TODO(gri) The code below should not be necessary. The subst function + // does not correctly create a copy for signatures that have no value + // parameters but are instantiated. Documented bug. + // Look also into the situation for methods. + if res == sig { + copy := *sig + res = © + } + res.tparams = nil // signature is not generic anymore + x.typ = res x.mode = value x.expr = e return expression diff --git a/src/go/types/contracts.go b/src/go/types/contracts.go index 5a767e5cad..a1632bca75 100644 --- a/src/go/types/contracts.go +++ b/src/go/types/contracts.go @@ -13,14 +13,14 @@ import ( type contractType struct{} -func (contractType) String() string { return "" } +func (contractType) String() string { return "" } func (contractType) Underlying() Type { panic("unreachable") } func (check *Checker) contractDecl(contr *Contract, e *ast.ContractSpec) { assert(contr.typ == nil) // contracts don't have types, but we need to set a type to - // detect recursive declrations and satisfy various assertions + // detect recursive declarations and satisfy various assertions contr.typ = new(contractType) check.openScope(e, "contract") @@ -224,8 +224,7 @@ func (check *Checker) typeConstraint(typ Type, why *string) bool { *why = check.sprintf("%s is not a type literal", t) return false case *TypeParam: - // TODO(gri) should this be ok? need a good use case - // ok for now + // ok, e.g.: func f (type T interface { type T }) () default: unreachable() } diff --git a/src/go/types/subst.go b/src/go/types/subst.go index e297ce8a84..22bab1d521 100644 --- a/src/go/types/subst.go +++ b/src/go/types/subst.go @@ -24,7 +24,7 @@ func (check *Checker) instantiate(pos token.Pos, typ Type, targs []Type, poslist if res != nil { under = res.Underlying() } - check.trace(pos, "=> %s %s", res, under) + check.trace(pos, "=> %s (under = %s)", res, under) }() } @@ -32,11 +32,11 @@ func (check *Checker) instantiate(pos token.Pos, typ Type, targs []Type, poslist // TODO(gri) What is better here: work with TypeParams, or work with TypeNames? var tparams []*TypeName - switch typ := typ.(type) { + switch t := typ.(type) { case *Named: - tparams = typ.tparams + tparams = t.tparams case *Signature: - tparams = typ.tparams + tparams = t.tparams default: check.dump(">>> trying to instantiate %s", typ) unreachable() // only defined types and (defined) functions can be generic @@ -220,16 +220,20 @@ func (subst *subster) typ(typ Type) Type { return subst.tuple(t) case *Signature: + // TODO(gri) BUG: If we instantiate this signature but it has no value params, we don't get a copy! + // We need to look at the actual type parameters of the signature as well. // TODO(gri) rethink the recv situation with respect to methods on parameterized types //recv := s.var_(t.recv) // not strictly needed (receivers cannot be parameterized) (?) recv := t.recv params := subst.tuple(t.params) results := subst.tuple(t.results) if recv != t.recv || params != t.params || results != t.results { + // TODO(gri) what do we need to do with t.scope, if anything? copy := *t - // note: we leave (copy.)tparams alone - if a caller instantiated a function - // via instantiate (calling subst) it is the caller's responsibility - // to nil out this field + // TODO(gri) if we instantiate this signature, we need to set + // tparams to nil (the signature may be a field type of a struct) + // otherwise the signature remains parameterized which would be + // wrong. Investigate the correct approach. copy.recv = recv copy.params = params copy.results = results diff --git a/src/go/types/testdata/contracts.go2 b/src/go/types/testdata/contracts.go2 index 9614c8577f..6b4195fc32 100644 --- a/src/go/types/testdata/contracts.go2 +++ b/src/go/types/testdata/contracts.go2 @@ -185,6 +185,30 @@ func adderSum(type T Adder(T))(data []T) T { func _(type T Adder /* ERROR cannot use generic type Adder */)(data []T) T +// -------------------------------------------------------------------------------------- +// Type lists may contain type parameters... :-) + +contract G1(T) { T T } +contract G2(T, T2) { T T, int; T2 T, int } + +func g1 (type T G1) () +func g2 (type T, T2 G2) () + +func h1 (type T interface{ type T }) () +func h2 (type T, T2 interface{ type T, int }) () + +func _() { + g1(int)() + g1(string)() + g2(int, int)() + g2(int, string /* ERROR string does not satisfy */ )() + + h1(int)() + h1(string)() + h2(int, int)() + h2(int, string /* ERROR string does not satisfy */ )() +} + // -------------------------------------------------------------------------------------- // Instantiations require bounds to be satisfied diff --git a/src/go/types/testdata/tmp.go2 b/src/go/types/testdata/tmp.go2 index cbb358b144..ef13afa0d6 100644 --- a/src/go/types/testdata/tmp.go2 +++ b/src/go/types/testdata/tmp.go2 @@ -3,3 +3,13 @@ // license that can be found in the LICENSE file. package p + +func f1 (type T interface{ type T }) () +func f2 (type T, T2 interface{ type T, int }) () + +func _() { + f1(int)() + f1(string)() + f2(int, int)() + f2(int, string /* ERROR string does not satisfy */ )() +} \ No newline at end of file diff --git a/src/go/types/typestring.go b/src/go/types/typestring.go index c2b1026344..15ba361d55 100644 --- a/src/go/types/typestring.go +++ b/src/go/types/typestring.go @@ -260,20 +260,7 @@ func writeType(buf *bytes.Buffer, typ Type, qf Qualifier, visited []Type) { case *Named: writeTypeName(buf, t.obj, qf) if t.tparams != nil { - buf.WriteString("(type ") - for i, p := range t.tparams { - if i > 0 { - buf.WriteString(", ") - } - buf.WriteString(p.name) - if ptyp, _ := p.typ.(*TypeParam); ptyp != nil && ptyp.bound != nil { - buf.WriteByte(' ') - writeType(buf, ptyp.bound, qf, visited) - // TODO(gri) if this is a generic type bound, we should print - // the type parameters - } - } - buf.WriteByte(')') + writeTParamList(buf, t.tparams, qf, visited) } case *TypeParam: @@ -300,6 +287,23 @@ func writeTypeList(buf *bytes.Buffer, list []Type, qf Qualifier, visited []Type) } } +func writeTParamList(buf *bytes.Buffer, list []*TypeName, qf Qualifier, visited []Type) { + buf.WriteString("(type ") + for i, p := range list { + if i > 0 { + buf.WriteString(", ") + } + buf.WriteString(p.name) + if ptyp, _ := p.typ.(*TypeParam); ptyp != nil && ptyp.bound != nil { + buf.WriteByte(' ') + writeType(buf, ptyp.bound, qf, visited) + // TODO(gri) if this is a generic type bound, we should print + // the type parameters + } + } + buf.WriteByte(')') +} + func writeTypeName(buf *bytes.Buffer, obj *TypeName, qf Qualifier) { s := "" if obj != nil { @@ -361,6 +365,10 @@ func WriteSignature(buf *bytes.Buffer, sig *Signature, qf Qualifier) { } func writeSignature(buf *bytes.Buffer, sig *Signature, qf Qualifier, visited []Type) { + if sig.tparams != nil { + writeTParamList(buf, sig.tparams, qf, visited) + } + writeTuple(buf, sig.params, sig.variadic, qf, visited) n := sig.results.Len()