From 4af3eb217ef4234a7bfed485d513d93276b153b3 Mon Sep 17 00:00:00 2001 From: Morgan Date: Thu, 5 Oct 2023 19:32:18 +0200 Subject: [PATCH] fix(gnolang): allow comparisons using uninitialized string values (#1132) The provided additional test, without the patch, fails with the following error: ``` === RUN TestFiles/comp3.gno Machine.RunMain() panic: interface conversion: gnolang.Value is nil, not gnolang.StringValue [...] goroutine 180 [running]: runtime/debug.Stack() /usr/lib/go/src/runtime/debug/stack.go:24 +0x65 [...] panic({0xb91680, 0xc001fa7bf0}) /usr/lib/go/src/runtime/panic.go:884 +0x213 github.com/gnolang/gno/gnovm/pkg/gnolang.isLss(0xc0054b4050, 0xc0054b4078) /home/howl/oc/gno2/gnovm/pkg/gnolang/op_binary.go:492 +0x431 ``` This seems to be because internally, the string value is "uninitialized" (hopefully right word here). This is in opposition to an initialised string, as would happen for the statement `x := ""`. This PR changes the behaviour for comparisons inside of `op_binary` (<, >, <=, >=) to use `GetString` instead of a type assertion of `TypedValue.V` to a `StringValue`. This is in line with what is already done inside of `isEql`, introduced in this commit: https://github.com/gnolang/gno/commit/da6520f3de0fe5d70602b8652528b96a17dc15f0#diff-7cc5c6bc5496b6ad9ed55e04e1cdf2f0d1d5954af21be5bc38ef3c46389149a9L358 `git blame` points instead this part of code inside of the comparisons functions to go back to the initial commit. Additionally, I looked for other cases where we are currently doing type assertions directly into a `StringValue`, and there was one in the implementation of `append`. Since this is a special case and requires having a native value as the first argument, I haven't written a test for it, but the change should be safe as `GetString()` internally just does the type assertion, but checks for `tv.V == nil` first.
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).
--------- Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com> --- gnovm/pkg/gnolang/op_binary.go | 8 ++++---- gnovm/pkg/gnolang/uverse.go | 2 +- gnovm/tests/files/append5.gno | 10 ++++++++++ gnovm/tests/files/comp3.gno | 21 +++++++++++++++++++++ 4 files changed, 36 insertions(+), 5 deletions(-) create mode 100644 gnovm/tests/files/append5.gno create mode 100644 gnovm/tests/files/comp3.gno diff --git a/gnovm/pkg/gnolang/op_binary.go b/gnovm/pkg/gnolang/op_binary.go index 13156133e38..ddab8302d2d 100644 --- a/gnovm/pkg/gnolang/op_binary.go +++ b/gnovm/pkg/gnolang/op_binary.go @@ -489,7 +489,7 @@ func isEql(store Store, lv, rv *TypedValue) bool { func isLss(lv, rv *TypedValue) bool { switch lv.T.Kind() { case StringKind: - return (lv.V.(StringValue) < rv.V.(StringValue)) + return (lv.GetString() < rv.GetString()) case IntKind: return (lv.GetInt() < rv.GetInt()) case Int8Kind: @@ -533,7 +533,7 @@ func isLss(lv, rv *TypedValue) bool { func isLeq(lv, rv *TypedValue) bool { switch lv.T.Kind() { case StringKind: - return (lv.V.(StringValue) <= rv.V.(StringValue)) + return (lv.GetString() <= rv.GetString()) case IntKind: return (lv.GetInt() <= rv.GetInt()) case Int8Kind: @@ -577,7 +577,7 @@ func isLeq(lv, rv *TypedValue) bool { func isGtr(lv, rv *TypedValue) bool { switch lv.T.Kind() { case StringKind: - return (lv.V.(StringValue) > rv.V.(StringValue)) + return (lv.GetString() > rv.GetString()) case IntKind: return (lv.GetInt() > rv.GetInt()) case Int8Kind: @@ -621,7 +621,7 @@ func isGtr(lv, rv *TypedValue) bool { func isGeq(lv, rv *TypedValue) bool { switch lv.T.Kind() { case StringKind: - return (lv.V.(StringValue) >= rv.V.(StringValue)) + return (lv.GetString() >= rv.GetString()) case IntKind: return (lv.GetInt() >= rv.GetInt()) case Int8Kind: diff --git a/gnovm/pkg/gnolang/uverse.go b/gnovm/pkg/gnolang/uverse.go index 80f8a751e57..57f8f6d393d 100644 --- a/gnovm/pkg/gnolang/uverse.go +++ b/gnovm/pkg/gnolang/uverse.go @@ -550,7 +550,7 @@ func UverseNode() *PackageNode { if xt.Elem().Kind() == Uint8Kind { // TODO this might be faster if reflect supports // appending this way without first converting to a slice. - argrv := reflect.ValueOf([]byte(arg1.TV.V.(StringValue))) + argrv := reflect.ValueOf([]byte(arg1.TV.GetString())) resrv := reflect.AppendSlice(sv, argrv) m.PushValue(TypedValue{ T: xt, diff --git a/gnovm/tests/files/append5.gno b/gnovm/tests/files/append5.gno new file mode 100644 index 00000000000..b1fdae852b1 --- /dev/null +++ b/gnovm/tests/files/append5.gno @@ -0,0 +1,10 @@ +package main + +func main() { + var x string + y := append([]byte{'X'}, x...) + println(string(y)) +} + +// Output: +// X diff --git a/gnovm/tests/files/comp3.gno b/gnovm/tests/files/comp3.gno new file mode 100644 index 00000000000..d412e8f0e48 --- /dev/null +++ b/gnovm/tests/files/comp3.gno @@ -0,0 +1,21 @@ +package main + +func main() { + // test against uninitialized value: https://github.com/gnolang/gno/pull/1132 + var x string + y := "Hello" + results := [...]bool{ + x < y, + x <= y, + x >= y, + x > y, + + x == y, + x == "", + y == "", + } + println(results) +} + +// Output: +// array[(true bool),(true bool),(false bool),(false bool),(false bool),(true bool),(false bool)]