From 7d939a051dd004ae5fdb44de60068a9368458146 Mon Sep 17 00:00:00 2001 From: deelawn Date: Tue, 21 May 2024 08:47:00 +0100 Subject: [PATCH] fix: achieve type assertion parity with go (#1689) Fixes #1650 This PR was originally intended to fix the case described in issue #1650, but it soon became apparent that there were other subtle inconsistencies between type assertions in go vs gno. The changes outlined here attempt to fix the issues and the new file tests ensure correctness. The summary below provides details as to the cases that were fixed for type assertions, both 1 and 2 -- 1 and 2 referring to the type assertion op codes, `OpTypeAssert1` and `OpTypeAssert2`. The former handles type assertions with one LHS argument that panic on assertion failure and the latter handles those with two LHS arguments that assigns a boolean value dependent on success or failure. ### Summary of type assertion changes - fail when the value being asserted has a nil type (i.e. the result of `recover()` when no panic has occurred). This applies for both cases where the type being asserted to is an interface or concrete type. - fail when asserting to an interface type and the underlying type of the value being asserted is also an interface type (non-concrete type) Also, is this an accurate assumption? https://github.com/gnolang/gno/blob/7d4e8e645ca1d2e89f5a8f2e85470e66b3253b33/gnovm/pkg/gnolang/op_expressions.go#L261
Contributors' checklist... - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [x] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
--- gnovm/pkg/gnolang/op_expressions.go | 107 ++++++++++++++++++---- gnovm/pkg/gnolang/preprocess.go | 27 ++++++ gnovm/tests/file.go | 1 + gnovm/tests/files/typeassert1.gno | 13 +++ gnovm/tests/files/typeassert2.gno | 32 +++++++ gnovm/tests/files/typeassert2a.gno | 15 +++ gnovm/tests/files/typeassert3.gno | 15 +++ gnovm/tests/files/typeassert4.gno | 58 ++++++++++++ gnovm/tests/files/typeassert4a.gno | 62 +++++++++++++ gnovm/tests/files/typeassert5.gno | 30 ++++++ gnovm/tests/files/typeassert5a.gno | 29 ++++++ gnovm/tests/files/typeassert6.gno | 25 +++++ gnovm/tests/files/typeassert6a.gno | 40 ++++++++ gnovm/tests/files/typeassert7_native.gno | 44 +++++++++ gnovm/tests/files/typeassert7a_native.gno | 43 +++++++++ gnovm/tests/files/typeassert8.gno | 18 ++++ gnovm/tests/files/typeassert8a.gno | 18 ++++ gnovm/tests/files/typeassert9.gno | 20 ++++ 18 files changed, 579 insertions(+), 18 deletions(-) create mode 100644 gnovm/tests/files/typeassert1.gno create mode 100644 gnovm/tests/files/typeassert2.gno create mode 100644 gnovm/tests/files/typeassert2a.gno create mode 100644 gnovm/tests/files/typeassert3.gno create mode 100644 gnovm/tests/files/typeassert4.gno create mode 100644 gnovm/tests/files/typeassert4a.gno create mode 100644 gnovm/tests/files/typeassert5.gno create mode 100644 gnovm/tests/files/typeassert5a.gno create mode 100644 gnovm/tests/files/typeassert6.gno create mode 100644 gnovm/tests/files/typeassert6a.gno create mode 100644 gnovm/tests/files/typeassert7_native.gno create mode 100644 gnovm/tests/files/typeassert7a_native.gno create mode 100644 gnovm/tests/files/typeassert8.gno create mode 100644 gnovm/tests/files/typeassert8a.gno create mode 100644 gnovm/tests/files/typeassert9.gno diff --git a/gnovm/pkg/gnolang/op_expressions.go b/gnovm/pkg/gnolang/op_expressions.go index 2efcc41fdce..4bbeef2dace 100644 --- a/gnovm/pkg/gnolang/op_expressions.go +++ b/gnovm/pkg/gnolang/op_expressions.go @@ -204,16 +204,41 @@ func (m *Machine) doOpRef() { func (m *Machine) doOpTypeAssert1() { m.PopExpr() // pop type - t := m.PopValue().GetType() + t := m.PopValue().GetType() // type being asserted + // peek x for re-use - xv := m.PeekValue(1) - xt := xv.T + xv := m.PeekValue(1) // value result / value to assert + xt := xv.T // underlying value's type + + // xt may be nil, but we need to wait to return because the value of xt that is set + // will depend on whether we are trying to assert to an interface or concrete type. + // xt can be nil in the case where recover can't find a panic to recover from and + // returns a bare TypedValue{}. if t.Kind() == InterfaceKind { // is interface assert + if xt == nil || xv.IsNilInterface() { + // TODO: default panic type? + ex := fmt.Sprintf("interface conversion: interface is nil, not %s", t.String()) + m.Panic(typedString(ex)) + return + } + if it, ok := baseOf(t).(*InterfaceType); ok { + // An interface type assertion on a value that doesn't have a concrete base + // type should always fail. + if _, ok := baseOf(xt).(*InterfaceType); ok { + // TODO: default panic type? + ex := fmt.Sprintf( + "non-concrete %s doesn't implement %s", + xt.String(), + it.String()) + m.Panic(typedString(ex)) + return + } + // t is Gno interface. // assert that x implements type. - impl := false + var impl bool impl = it.IsImplementedBy(xt) if !impl { // TODO: default panic type? @@ -230,16 +255,22 @@ func (m *Machine) doOpTypeAssert1() { } else if nt, ok := baseOf(t).(*NativeType); ok { // t is Go interface. // assert that x implements type. - impl := false + errPrefix := "non-concrete " + var impl bool if nxt, ok := xt.(*NativeType); ok { - impl = nxt.Type.Implements(nt.Type) - } else { - impl = false + // If the underlying native type is reflect.Interface kind, then this has no + // concrete value and should fail. + if nxt.Type.Kind() != reflect.Interface { + impl = nxt.Type.Implements(nt.Type) + errPrefix = "" + } } + if !impl { // TODO: default panic type? ex := fmt.Sprintf( - "%s doesn't implement %s", + "%s%s doesn't implement %s", + errPrefix, xt.String(), nt.String()) m.Panic(typedString(ex)) @@ -251,6 +282,12 @@ func (m *Machine) doOpTypeAssert1() { panic("should not happen") } } else { // is concrete assert + if xt == nil { + ex := fmt.Sprintf("nil is not of type %s", t.String()) + m.Panic(typedString(ex)) + return + } + tid := t.TypeID() xtid := xt.TypeID() // assert that x is of type. @@ -273,17 +310,37 @@ func (m *Machine) doOpTypeAssert1() { func (m *Machine) doOpTypeAssert2() { m.PopExpr() // peek type for re-use - tv := m.PeekValue(1) - t := tv.GetType() + tv := m.PeekValue(1) // boolean result + t := tv.GetType() // type being asserted + // peek x for re-use - xv := m.PeekValue(2) - xt := xv.T + xv := m.PeekValue(2) // value result / value to assert + xt := xv.T // underlying value's type + + // xt may be nil, but we need to wait to return because the value of xt that is set + // will depend on whether we are trying to assert to an interface or concrete type. + // xt can be nil in the case where recover can't find a panic to recover from and + // returns a bare TypedValue{}. if t.Kind() == InterfaceKind { // is interface assert + if xt == nil { + *xv = TypedValue{} + *tv = untypedBool(false) + return + } + if it, ok := baseOf(t).(*InterfaceType); ok { + // An interface type assertion on a value that doesn't have a concrete base + // type should always fail. + if _, ok := baseOf(xt).(*InterfaceType); ok { + *xv = TypedValue{} + *tv = untypedBool(false) + return + } + // t is Gno interface. // assert that x implements type. - impl := false + var impl bool impl = it.IsImplementedBy(xt) if impl { // *xv = *xv @@ -295,14 +352,18 @@ func (m *Machine) doOpTypeAssert2() { *tv = untypedBool(false) } } else if nt, ok := baseOf(t).(*NativeType); ok { + // If the value being asserted on is nil, it can't implement an interface. // t is Go interface. // assert that x implements type. - impl := false + var impl bool if nxt, ok := xt.(*NativeType); ok { - impl = nxt.Type.Implements(nt.Type) - } else { - impl = false + // If the underlying native type is reflect.Interface kind, then this has no + // concrete value and should fail. + if nxt.Type.Kind() != reflect.Interface { + impl = nxt.Type.Implements(nt.Type) + } } + if impl { // *xv = *xv *tv = untypedBool(true) @@ -314,10 +375,20 @@ func (m *Machine) doOpTypeAssert2() { panic("should not happen") } } else { // is concrete assert + if xt == nil { + *xv = TypedValue{ + T: t, + V: defaultValue(m.Alloc, t), + } + *tv = untypedBool(false) + return + } + tid := t.TypeID() xtid := xt.TypeID() // assert that x is of type. same := tid == xtid + if same { // *xv = *xv *tv = untypedBool(true) diff --git a/gnovm/pkg/gnolang/preprocess.go b/gnovm/pkg/gnolang/preprocess.go index 12cf78cb7fa..ddc72c3a048 100644 --- a/gnovm/pkg/gnolang/preprocess.go +++ b/gnovm/pkg/gnolang/preprocess.go @@ -9,6 +9,8 @@ import ( "github.com/gnolang/gno/tm2/pkg/errors" ) +const blankIdentifer string = "_" + // In the case of a *FileSet, some declaration steps have to happen // in a restricted parallel way across all the files. // Anything predefined or preprocessed here get skipped during the Preprocess @@ -1237,8 +1239,33 @@ func Preprocess(store Store, ctx BlockNode, n Node) Node { if n.Type == nil { panic("should not happen") } + + // Type assertions on the blank identifier are illegal. + if nx, ok := n.X.(*NameExpr); ok && string(nx.Name) == blankIdentifer { + panic("cannot use _ as value or type") + } + // ExprStmt of form `x.()`, // or special case form `c, ok := x.()`. + t := evalStaticTypeOf(store, last, n.X) + baseType := baseOf(t) // The base type of the asserted value must be an interface. + switch bt := baseType.(type) { + case *InterfaceType: + break + case *NativeType: + if bt.Type.Kind() == reflect.Interface { + break + } + default: + panic( + fmt.Sprintf( + "invalid operation: %s (variable of type %s) is not an interface", + n.X.String(), + t.String(), + ), + ) + } + evalStaticType(store, last, n.Type) // TRANS_LEAVE ----------------------- diff --git a/gnovm/tests/file.go b/gnovm/tests/file.go index 8db662e53fb..328d9e09299 100644 --- a/gnovm/tests/file.go +++ b/gnovm/tests/file.go @@ -258,6 +258,7 @@ func RunFileTest(rootDir string, path string, opts ...RunFileTestOption) error { default: errstr = strings.TrimSpace(fmt.Sprintf("%v", pnc)) } + if errstr != errWanted { panic(fmt.Sprintf("fail on %s: got %q, want: %q", path, errstr, errWanted)) } diff --git a/gnovm/tests/files/typeassert1.gno b/gnovm/tests/files/typeassert1.gno new file mode 100644 index 00000000000..041034e4bd0 --- /dev/null +++ b/gnovm/tests/files/typeassert1.gno @@ -0,0 +1,13 @@ +package main + +type A interface { + Do(s string) +} + +func main() { + var a A + _ = a.(A) +} + +// Error: +// interface conversion: interface is nil, not main.A diff --git a/gnovm/tests/files/typeassert2.gno b/gnovm/tests/files/typeassert2.gno new file mode 100644 index 00000000000..a9791fa0038 --- /dev/null +++ b/gnovm/tests/files/typeassert2.gno @@ -0,0 +1,32 @@ +package main + +type ex int + +func (ex) Error() string { return "" } + +type A interface { + Do(s string) +} + +func main() { + defer func() { + e := recover() + if _, ok := e.(ex); ok { + println("wat") + } else { + println("ok") + } + }() + defer func() { + e := recover() + if _, ok := e.(A); ok { + println("wat") + } else { + println("ok") + } + }() +} + +// Output: +// ok +// ok diff --git a/gnovm/tests/files/typeassert2a.gno b/gnovm/tests/files/typeassert2a.gno new file mode 100644 index 00000000000..0441bf83437 --- /dev/null +++ b/gnovm/tests/files/typeassert2a.gno @@ -0,0 +1,15 @@ +package main + +type A interface { + Do(s string) +} + +func main() { + defer func() { + e := recover() + _ = e.(A) + }() +} + +// Error: +// interface conversion: interface is nil, not main.A diff --git a/gnovm/tests/files/typeassert3.gno b/gnovm/tests/files/typeassert3.gno new file mode 100644 index 00000000000..83bedd503f7 --- /dev/null +++ b/gnovm/tests/files/typeassert3.gno @@ -0,0 +1,15 @@ +package main + +type ex int + +func (ex) Error() string { return "" } + +func main() { + defer func() { + r := _.(ex) + println(r) + }() +} + +// Error: +// main/files/typeassert3.gno:9: cannot use _ as value or type diff --git a/gnovm/tests/files/typeassert4.gno b/gnovm/tests/files/typeassert4.gno new file mode 100644 index 00000000000..a137d3833ee --- /dev/null +++ b/gnovm/tests/files/typeassert4.gno @@ -0,0 +1,58 @@ +package main + +type Setter interface { + Set(string) +} + +type SetterClone interface { + Set(string) +} + +type ValueSetter struct { + value string +} + +func (s *ValueSetter) Set(value string) { + s.value = value +} + +func cmpSetter(i interface{}) { + if _, ok := i.(Setter); ok { + println("ok") + } else { + println("not ok") + } +} + +func main() { + var ( + i interface{} + setter Setter + setterClone SetterClone + valueSetter ValueSetter + valueSetterPtr *ValueSetter + ) + + cmpSetter(i) + + i = setter + cmpSetter(i) + + setterClone = valueSetterPtr + setter = setterClone + i = setter + cmpSetter(i) + + i = valueSetter + cmpSetter(i) + + i = valueSetterPtr + cmpSetter(i) +} + +// Output: +// not ok +// not ok +// ok +// not ok +// ok diff --git a/gnovm/tests/files/typeassert4a.gno b/gnovm/tests/files/typeassert4a.gno new file mode 100644 index 00000000000..46d3728de1f --- /dev/null +++ b/gnovm/tests/files/typeassert4a.gno @@ -0,0 +1,62 @@ +package main + +type Setter interface { + Set(string) +} + +type SetterClone interface { + Set(string) +} + +type ValueSetter struct { + value string +} + +func (s *ValueSetter) Set(value string) { + s.value = value +} + +func cmpSetter(i interface{}) { + defer func() { + if r := recover(); r != nil { + println(r) + } else { + println("ok") + } + }() + + _ = i.(Setter) +} + +func main() { + var ( + i interface{} + setter Setter + setterClone SetterClone + valueSetter ValueSetter + valueSetterPtr *ValueSetter + ) + + cmpSetter(i) + + i = setter + cmpSetter(i) + + setterClone = valueSetterPtr + setter = setterClone + i = setter + cmpSetter(i) + + i = valueSetter + cmpSetter(i) + + i = valueSetterPtr + cmpSetter(i) +} + +// Output: +// interface conversion: interface is nil, not main.Setter +// interface conversion: interface is nil, not main.Setter +// ok +// main.ValueSetter doesn't implement interface{Set func(string)()} +// ok diff --git a/gnovm/tests/files/typeassert5.gno b/gnovm/tests/files/typeassert5.gno new file mode 100644 index 00000000000..f548de6c0e1 --- /dev/null +++ b/gnovm/tests/files/typeassert5.gno @@ -0,0 +1,30 @@ +package main + +type A interface { + Do(i int) +} + +type B interface { + Do(i int) +} + +type C struct{} + +func (c C) Do(i int) {} + +func AcceptA(a A) { + AcceptB(a) +} + +func AcceptB(b B) { + if _, ok := b.(A); ok { + println("ok") + } +} + +func main() { + AcceptA(C{}) +} + +// Output: +// ok diff --git a/gnovm/tests/files/typeassert5a.gno b/gnovm/tests/files/typeassert5a.gno new file mode 100644 index 00000000000..a4f720d65c6 --- /dev/null +++ b/gnovm/tests/files/typeassert5a.gno @@ -0,0 +1,29 @@ +package main + +type A interface { + Do(i int) +} + +type B interface { + Do(i int) +} + +type C struct{} + +func (c C) Do(i int) {} + +func AcceptA(a A) { + AcceptB(a) +} + +func AcceptB(b B) { + _ = b.(A) + println("ok") +} + +func main() { + AcceptA(C{}) +} + +// Output: +// ok diff --git a/gnovm/tests/files/typeassert6.gno b/gnovm/tests/files/typeassert6.gno new file mode 100644 index 00000000000..5faa392a703 --- /dev/null +++ b/gnovm/tests/files/typeassert6.gno @@ -0,0 +1,25 @@ +package main + +type A interface { + Do(s string) +} + +func main() { + var v interface{} + v = 9 + + if _, ok := v.(A); !ok { + println(v) + } + + vp := new(int) + *vp = 99 + v = vp + if _, ok := v.(A); !ok { + println(*(v.(*int))) + } +} + +// Output: +// 9 +// 99 diff --git a/gnovm/tests/files/typeassert6a.gno b/gnovm/tests/files/typeassert6a.gno new file mode 100644 index 00000000000..f4b925cfeee --- /dev/null +++ b/gnovm/tests/files/typeassert6a.gno @@ -0,0 +1,40 @@ +package main + +type A interface { + Do(s string) +} + +func test1() { + defer func() { + if r := recover(); r != nil { + println(r) + } + }() + + var v interface{} + v = 9 + _ = v.(A) +} + +func test2() { + defer func() { + if r := recover(); r != nil { + println(r) + } + }() + + var v interface{} + vp := new(int) + *vp = 99 + v = vp + _ = v.(A) +} + +func main() { + test1() + test2() +} + +// Output: +// int doesn't implement interface{Do func(string)()} +// *int doesn't implement interface{Do func(string)()} diff --git a/gnovm/tests/files/typeassert7_native.gno b/gnovm/tests/files/typeassert7_native.gno new file mode 100644 index 00000000000..ed79ecf2e2e --- /dev/null +++ b/gnovm/tests/files/typeassert7_native.gno @@ -0,0 +1,44 @@ +package main + +import ( + "bytes" + "io" +) + +func main() { + { + var v interface{} + var r io.Reader + r = bytes.NewBuffer([]byte("hello")) + v = r + if _, ok := v.(io.Reader); ok { + println("ok") + } else { + println("not ok") + } + } + { + var v interface{} + var r io.Reader + v = r + if _, ok := v.(io.Reader); ok { + println("ok") + } else { + println("not ok") + } + } + { + var v interface{} + v = bytes.NewBuffer([]byte("hello")) + if _, ok := v.(io.Reader); ok { + println("ok") + } else { + println("not ok") + } + } +} + +// Output: +// ok +// not ok +// ok diff --git a/gnovm/tests/files/typeassert7a_native.gno b/gnovm/tests/files/typeassert7a_native.gno new file mode 100644 index 00000000000..cafb27b6a6b --- /dev/null +++ b/gnovm/tests/files/typeassert7a_native.gno @@ -0,0 +1,43 @@ +package main + +import ( + "bytes" + "io" +) + +func testImpl(v interface{}) { + defer func() { + if r := recover(); r != nil { + println(r) + } + }() + + _ = v.(io.Reader) + println("ok") +} + +func main() { + { + var v interface{} + var r io.Reader + r = bytes.NewBuffer([]byte("hello")) + v = r + testImpl(v) + } + { + var v interface{} + var r io.Reader + v = r + testImpl(v) + } + { + var v interface{} + v = bytes.NewBuffer([]byte("hello")) + testImpl(v) + } +} + +// Output: +// ok +// interface conversion: interface is nil, not gonative{io.Reader} +// ok diff --git a/gnovm/tests/files/typeassert8.gno b/gnovm/tests/files/typeassert8.gno new file mode 100644 index 00000000000..b744afc8703 --- /dev/null +++ b/gnovm/tests/files/typeassert8.gno @@ -0,0 +1,18 @@ +package main + +type ex int + +func (ex) Error() string { return "" } + +type i interface { + Error() string +} + +func main() { + r := []int(nil) + e := r.(ex) + println(e) +} + +// Error: +// main/files/typeassert8.gno:13: invalid operation: r (variable of type []int) is not an interface diff --git a/gnovm/tests/files/typeassert8a.gno b/gnovm/tests/files/typeassert8a.gno new file mode 100644 index 00000000000..a32aadc7679 --- /dev/null +++ b/gnovm/tests/files/typeassert8a.gno @@ -0,0 +1,18 @@ +package main + +type ex int + +func (ex) Error() string { return "" } + +type i interface { + Error() string +} + +func main() { + r := []int(nil) + e, ok := r.(ex) + println(e, ok) +} + +// Error: +// main/files/typeassert8a.gno:13: invalid operation: r (variable of type []int) is not an interface diff --git a/gnovm/tests/files/typeassert9.gno b/gnovm/tests/files/typeassert9.gno new file mode 100644 index 00000000000..d9d5bad55af --- /dev/null +++ b/gnovm/tests/files/typeassert9.gno @@ -0,0 +1,20 @@ +package main + +// First interface +type Reader interface { + Read() string +} + +// Second interface +type Writer interface { + Write() string +} + +func main() { + var reader Reader + + _ = reader.(Writer) +} + +// Error: +// interface conversion: interface is nil, not main.Writer