From ae8016313d8e5881557b3db3cba3cbb5aed5f80b Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Fri, 26 Jul 2013 21:49:27 -0400 Subject: [PATCH] go.tools/ssa: tests of method promotion and of interface conversion + bugfixes. methprom.go covers method promotion. Found bug: receiver() requires a following load under some circumstances. ifaceconv.go covers interface conversion. Found bug: confusion about infallible and fallible conversions led to use of TypeAssert in emitConv, which should never fail. Changed semantics of ChangeInterface to make it infallible and made some simplifications. Also in this CL: - SelectState.Pos now records the position of the the '<-' operator for sends/receives done by a Select. R=gri CC=golang-dev https://golang.org/cl/11931044 --- ssa/builder.go | 23 ++++---- ssa/emit.go | 24 ++------- ssa/interp/interp.go | 6 +-- ssa/interp/interp_test.go | 8 +-- ssa/interp/ops.go | 8 ++- ssa/interp/testdata/ifaceconv.go | 83 ++++++++++++++++++++++++++++ ssa/interp/testdata/methprom.go | 93 ++++++++++++++++++++++++++++++++ ssa/ssa.go | 11 ++-- 8 files changed, 211 insertions(+), 45 deletions(-) create mode 100644 ssa/interp/testdata/ifaceconv.go create mode 100644 ssa/interp/testdata/methprom.go diff --git a/ssa/builder.go b/ssa/builder.go index 8fc5de6a..bf52f21c 100644 --- a/ssa/builder.go +++ b/ssa/builder.go @@ -523,6 +523,8 @@ func (b *builder) expr(fn *Function, e ast.Expr) Value { switch y := y.(type) { case *Convert: y.pos = e.Lparen + case *ChangeInterface: + y.pos = e.Lparen case *ChangeType: y.pos = e.Lparen case *MakeInterface: @@ -662,8 +664,6 @@ func (b *builder) expr(fn *Function, e ast.Expr) Value { wantAddr := isPointer(recvType(obj)) escaping := true v := b.receiver(fn, e.X, wantAddr, escaping, indices, isIndirect) - // TODO(adonovan): test the case where - // *struct{I} inherits a method from interface I. c := &MakeClosure{ Fn: boundMethodWrapper(fn.Prog, obj), Bindings: []Value{v}, @@ -761,7 +761,11 @@ func (b *builder) receiver(fn *Function, e ast.Expr, wantAddr, escaping bool, in } last := len(indices) - 1 - return emitImplicitSelections(fn, v, indices[:last]) + v = emitImplicitSelections(fn, v, indices[:last]) + if !wantAddr && isPointer(v.Type()) { + v = emitLoad(fn, v) + } + return v } // setCallFunc populates the function parts of a CallCommon structure @@ -836,10 +840,6 @@ func (b *builder) setCallFunc(fn *Function, e *ast.CallExpr, c *CallCommon) { escaping := true v := b.receiver(fn, sel.X, wantAddr, escaping, indices, isIndirect) if _, ok := deref(v.Type()).Underlying().(*types.Interface); ok { - if isPointer(v.Type()) { - // *struct{I} inherits methods from I. - v = emitLoad(fn, v) - } // Invoke-mode call. c.Value = v c.Method = obj @@ -1540,18 +1540,23 @@ func (b *builder) selectStmt(fn *Function, s *ast.SelectStmt, label *lblock) { Chan: ch, Send: emitConv(fn, b.expr(fn, comm.Value), ch.Type().Underlying().(*types.Chan).Elem()), + Pos: comm.Arrow, }) case *ast.AssignStmt: // x := <-ch + unary := unparen(comm.Rhs[0]).(*ast.UnaryExpr) states = append(states, SelectState{ Dir: ast.RECV, - Chan: b.expr(fn, unparen(comm.Rhs[0]).(*ast.UnaryExpr).X), + Chan: b.expr(fn, unary.X), + Pos: unary.OpPos, }) case *ast.ExprStmt: // <-ch + unary := unparen(comm.X).(*ast.UnaryExpr) states = append(states, SelectState{ Dir: ast.RECV, - Chan: b.expr(fn, unparen(comm.X).(*ast.UnaryExpr).X), + Chan: b.expr(fn, unary.X), + Pos: unary.OpPos, }) } } diff --git a/ssa/emit.go b/ssa/emit.go index 1e09eeb1..130721d0 100644 --- a/ssa/emit.go +++ b/ssa/emit.go @@ -160,7 +160,7 @@ func isValuePreserving(ut_src, ut_dst types.Type) bool { // emitConv emits to f code to convert Value val to exactly type typ, // and returns the converted value. Implicit conversions are required // by language assignability rules in assignments, parameter passing, -// etc. +// etc. Conversions cannot fail dynamically. // func emitConv(f *Function, val Value, typ types.Type) Value { t_src := val.Type() @@ -185,7 +185,9 @@ func emitConv(f *Function, val Value, typ types.Type) Value { // Assignment from one interface type to another? if _, ok := ut_src.(*types.Interface); ok { - return emitTypeAssert(f, val, typ, token.NoPos) + c := &ChangeInterface{X: val} + c.setType(typ) + return f.emit(c) } // Untyped nil constant? Return interface-typed nil constant. @@ -268,20 +270,6 @@ func emitExtract(f *Function, tuple Value, index int, typ types.Type) Value { // returns the value. x.Type() must be an interface. // func emitTypeAssert(f *Function, x Value, t types.Type, pos token.Pos) Value { - // Simplify infallible assertions. - txi := x.Type().Underlying().(*types.Interface) - if ti, ok := t.Underlying().(*types.Interface); ok { - // Even when ti==txi, we still need ChangeInterface - // since it performs a nil-check. - // TODO(adonovan): needs more tests. - if types.IsAssignableTo(ti, txi) { - c := &ChangeInterface{X: x} - c.setPos(pos) - c.setType(t) - return f.emit(c) - } - } - a := &TypeAssert{X: x, AssertedType: t} a.setPos(pos) a.setType(t) @@ -292,10 +280,6 @@ func emitTypeAssert(f *Function, x Value, t types.Type, pos token.Pos) Value { // a (value, ok) tuple. x.Type() must be an interface. // func emitTypeTest(f *Function, x Value, t types.Type, pos token.Pos) Value { - // TODO(adonovan): opt: simplify infallible tests as per - // emitTypeAssert, and return (x, vTrue). - // (Requires that exprN returns a slice of extracted values, - // not a single Value of type *types.Tuple.) a := &TypeAssert{ X: x, AssertedType: t, diff --git a/ssa/interp/interp.go b/ssa/interp/interp.go index b1aee540..e7e891cc 100644 --- a/ssa/interp/interp.go +++ b/ssa/interp/interp.go @@ -164,11 +164,7 @@ func visitInstr(fr *frame, instr ssa.Instruction) continuation { fr.env[instr] = call(fr.i, fr, instr.Pos(), fn, args) case *ssa.ChangeInterface: - x := fr.get(instr.X) - if x.(iface).t == nil { - panic(fmt.Sprintf("interface conversion: interface is nil, not %s", instr.Type())) - } - fr.env[instr] = x + fr.env[instr] = fr.get(instr.X) case *ssa.ChangeType: fr.env[instr] = fr.get(instr.X) // (can't fail) diff --git a/ssa/interp/interp_test.go b/ssa/interp/interp_test.go index 553ae679..06845a39 100644 --- a/ssa/interp/interp_test.go +++ b/ssa/interp/interp_test.go @@ -129,11 +129,13 @@ var gorootTests = []string{ // These are files in go.tools/ssa/interp/testdata/. var testdataTests = []string{ - "coverage.go", - "mrvchain.go", "boundmeth.go", - "ifaceprom.go", + "coverage.go", "fieldprom.go", + "ifaceconv.go", + "ifaceprom.go", + "methprom.go", + "mrvchain.go", } func run(t *testing.T, dir, input string) bool { diff --git a/ssa/interp/ops.go b/ssa/interp/ops.go index fbab4286..a2b260db 100644 --- a/ssa/interp/ops.go +++ b/ssa/interp/ops.go @@ -839,8 +839,12 @@ func typeAssert(i *interpreter, instr *ssa.TypeAssert, itf iface) value { var v value err := "" if idst, ok := instr.AssertedType.Underlying().(*types.Interface); ok { - v = itf - err = checkInterface(i, idst, itf) + if itf.t == nil { + err = fmt.Sprintf("interface conversion: interface is nil, not %s", instr.AssertedType) + } else { + v = itf + err = checkInterface(i, idst, itf) + } } else if types.IsIdentical(itf.t, instr.AssertedType) { v = copyVal(itf.v) // extract value diff --git a/ssa/interp/testdata/ifaceconv.go b/ssa/interp/testdata/ifaceconv.go new file mode 100644 index 00000000..96fc1058 --- /dev/null +++ b/ssa/interp/testdata/ifaceconv.go @@ -0,0 +1,83 @@ +package main + +// Tests of interface conversions and type assertions. + +type I0 interface { +} +type I1 interface { + f() +} +type I2 interface { + f() + g() +} + +type C0 struct{} +type C1 struct{} + +func (C1) f() {} + +type C2 struct{} + +func (C2) f() {} +func (C2) g() {} + +func main() { + var i0 I0 + var i1 I1 + var i2 I2 + + // Nil always causes a type assertion to fail, even to the + // same type. + if _, ok := i0.(I0); ok { + panic("nil i0.(I0) succeeded") + } + if _, ok := i1.(I1); ok { + panic("nil i1.(I1) succeeded") + } + if _, ok := i2.(I2); ok { + panic("nil i2.(I2) succeeded") + } + + // Conversions can't fail, even with nil. + _ = I0(i0) + + _ = I0(i1) + _ = I1(i1) + + _ = I0(i2) + _ = I1(i2) + _ = I2(i2) + + // Non-nil type assertions pass or fail based on the concrete type. + i1 = C1{} + if _, ok := i1.(I0); !ok { + panic("C1 i1.(I0) failed") + } + if _, ok := i1.(I1); !ok { + panic("C1 i1.(I1) failed") + } + if _, ok := i1.(I2); ok { + panic("C1 i1.(I2) succeeded") + } + + i1 = C2{} + if _, ok := i1.(I0); !ok { + panic("C2 i1.(I0) failed") + } + if _, ok := i1.(I1); !ok { + panic("C2 i1.(I1) failed") + } + if _, ok := i1.(I2); !ok { + panic("C2 i1.(I2) failed") + } + + // Conversions can't fail. + i1 = C1{} + if I0(i1) == nil { + panic("C1 I0(i1) was nil") + } + if I1(i1) == nil { + panic("C1 I1(i1) was nil") + } +} diff --git a/ssa/interp/testdata/methprom.go b/ssa/interp/testdata/methprom.go new file mode 100644 index 00000000..e8e384c3 --- /dev/null +++ b/ssa/interp/testdata/methprom.go @@ -0,0 +1,93 @@ +package main + +// Tests of method promotion logic. + +type A struct{ magic int } + +func (a A) x() { + if a.magic != 1 { + panic(a.magic) + } +} +func (a *A) y() *A { + return a +} + +type B struct{ magic int } + +func (b B) p() { + if b.magic != 2 { + panic(b.magic) + } +} +func (b *B) q() { + if b != theC.B { + panic("oops") + } +} + +type I interface { + f() +} + +type impl struct{ magic int } + +func (i impl) f() { + if i.magic != 3 { + panic("oops") + } +} + +type C struct { + A + *B + I +} + +func assert(cond bool) { + if !cond { + panic("failed") + } +} + +var theC = C{ + A: A{1}, + B: &B{2}, + I: impl{3}, +} + +func addr() *C { + return &theC +} + +func value() C { + return theC +} + +func main() { + // address + addr().x() + if addr().y() != &theC.A { + panic("oops") + } + addr().p() + addr().q() + addr().f() + + // addressable value + var c C = value() + c.x() + if c.y() != &c.A { + panic("oops") + } + c.p() + c.q() + c.f() + + // non-addressable value + value().x() + // value().y() // not in method set + value().p() + value().q() + value().f() +} diff --git a/ssa/ssa.go b/ssa/ssa.go index 4e2bfc22..29a5d645 100644 --- a/ssa/ssa.go +++ b/ssa/ssa.go @@ -582,10 +582,7 @@ type Convert struct { // ChangeInterface constructs a value of one interface type from a // value of another interface type known to be assignable to it. -// -// This operation fails if the operand is nil. -// For all other operands, well-typedness ensures success. -// Use TypeAssert for interface conversions that are uncertain. +// This operation cannot fail. // // Pos() returns the ast.CallExpr.Lparen if the instruction arose from // an explicit T(e) conversion; the ast.TypeAssertExpr.Lparen if the @@ -813,6 +810,7 @@ type SelectState struct { Dir ast.ChanDir // direction of case Chan Value // channel to use (for send or receive) Send Value // value to send (for send) + Pos token.Pos // position of token.ARROW } // The Select instruction tests whether (or blocks until) one or more @@ -915,8 +913,9 @@ type Next struct { // If AssertedType is an interface, TypeAssert checks whether the // dynamic type of the interface is assignable to it, and if so, the // result of the conversion is a copy of the interface value X. -// If AssertedType is a superinterface of X.Type(), the operation -// cannot fail; ChangeInterface is preferred in this case. +// If AssertedType is a superinterface of X.Type(), the operation will +// fail iff the operand is nil. (Contrast with ChangeInterface, which +// performs no nil-check.) // // Type() reflects the actual type of the result, possibly a // 2-types.Tuple; AssertedType is the asserted type.