Skip to content

Commit

Permalink
fix: remove incorrect type assertion when reading values from storage (
Browse files Browse the repository at this point in the history
…#1606)

<!-- please provide a detailed description of the changes made in this
pull request. -->
Addresses #1588.

The following explanation refers to the txtar test linked to in the
issue. [Here it
is](https://gist.github.com/thehowl/f0ca7715705998116c2c88a6811a692f)
for convenience.

### Why the panic?
This line in the txtar test's `Read()` function caused the panic: `out
+= post.CreatedAt.Format(time.RFC3339)`. The `Format` method results in
invoking the `Time.locabs` method, and this is the line inside that
method that caused the panic:
https://github.com/gnolang/gno/blob/master/gnovm/stdlibs/time/time.gno#L468

The panic occurs when trying to resolve the value of `l.cacheZone` as a
part of the selector operation. This is a panic caused by an invalid
type assertion. The invalid type assertion happens on the line that was
modified for this PR:
https://github.com/gnolang/gno/blob/master/gnovm/stdlibs/time/time.gno#L468

In the case of this particular txtar test, the target value's base value
is a struct value, not an array value. We can look at how the original
`cacheZone` value was persisted to understand why the base type is
different from what we'd expect.

### Original value persistence
The original time value is built as a result of this line in the txtar
test's `Store()` function: `parsedTime, err = time.Parse(time.RFC3339,
date)`.

The `cacheZone` value that is persisted either has a struct base value
or is nil for the majority of this operation. However, the time value's
`Location` field gets updated near the end of the `parse` operation and
assumes the value of the result of the `FixedZone` function:
https://github.com/gnolang/gno/blob/master/gnovm/stdlibs/time/format.gno#L1306

Taking a look at what `FixedZone` actually does, we can see that the
location's `cacheZone` is now a reference to a zone that exists as a
part of an array -- a struct with a base type of an array value:
https://github.com/gnolang/gno/blob/master/gnovm/stdlibs/time/timezoneinfo.gno#L114

This explains why the value we were seeing when trying to resolve the
`cacheZone` selector had an array base.

### The fix
It doesn't matter if the source value's parent is an array or struct; we
need the underlying value's type and value. That being said, I don't
think the array type assertion needs to happen since the type value is
accessible via the `Elem()` method defined as part of the `Type`
interface. Now the value is obtained without causing a panic.

<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
- [x] 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>
  • Loading branch information
deelawn authored Feb 22, 2024
1 parent 989e6d6 commit 0009c44
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 1 deletion.
48 changes: 48 additions & 0 deletions gno.land/cmd/gnoland/testdata/issue-1588.txtar
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# Reproducible Test for https://github.com/gnolang/gno/issues/1588

gnoland start

# add contract
gnokey maketx addpkg -pkgdir $WORK -pkgpath gno.land/r/demo/xx -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!

gnokey maketx call -pkgpath gno.land/r/demo/xx -func DefineFamily -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!

gnokey maketx call -pkgpath gno.land/r/demo/xx -func GetOutcastChildAge -gas-fee 1000000ugnot -gas-wanted 2000000 -broadcast -chainid=tendermint_test test1
stdout OK!
stdout '(10 int)'

-- gno.mod --
module gno.land/r/demo/xx

-- realm.gno --
package xx

type Parent struct {
children []Child
outcastChild *Child
}

type Child struct {
age int
}

var parent Parent

func DefineFamily() {
parent = Parent{
children: []Child{
{age: 10},
{age: 20},
},
}
parent.outcastChild = &parent.children[0]
}

func GetOutcastChildAge() int {
if parent.outcastChild == nil {
return -1
}
return parent.outcastChild.age
}
2 changes: 1 addition & 1 deletion gnovm/pkg/gnolang/values.go
Original file line number Diff line number Diff line change
Expand Up @@ -2533,7 +2533,7 @@ func fillValueTV(store Store, tv *TypedValue) *TypedValue {
cv.Base = base
switch cb := base.(type) {
case *ArrayValue:
et := baseOf(tv.T).(*ArrayType).Elt
et := baseOf(tv.T).(*PointerType).Elt
epv := cb.GetPointerAtIndexInt2(store, cv.Index, et)
cv.TV = epv.TV // TODO optimize? (epv.* ignored)
case *StructValue:
Expand Down

0 comments on commit 0009c44

Please sign in to comment.