Skip to content

Commit

Permalink
fix(gnolang): allow comparisons using uninitialized string values (#1132
Browse files Browse the repository at this point in the history
)

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:


da6520f#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.

<details><summary>Contributors' checklist...</summary>

- [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).
</details>

---------

Co-authored-by: Manfred Touron <[email protected]>
  • Loading branch information
thehowl and moul authored Oct 5, 2023
1 parent 740e7f7 commit 4af3eb2
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 5 deletions.
8 changes: 4 additions & 4 deletions gnovm/pkg/gnolang/op_binary.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion gnovm/pkg/gnolang/uverse.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
10 changes: 10 additions & 0 deletions gnovm/tests/files/append5.gno
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package main

func main() {
var x string
y := append([]byte{'X'}, x...)
println(string(y))
}

// Output:
// X
21 changes: 21 additions & 0 deletions gnovm/tests/files/comp3.gno
Original file line number Diff line number Diff line change
@@ -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)]

0 comments on commit 4af3eb2

Please sign in to comment.