Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove incorrect type assertion when reading values from storage #1606

Merged
merged 6 commits into from
Feb 22, 2024

Conversation

deelawn
Copy link
Contributor

@deelawn deelawn commented Jan 30, 2024

Addresses #1588.

The following explanation refers to the txtar test linked to in the issue. Here it is 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.

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Jan 30, 2024
@deelawn deelawn marked this pull request as draft January 30, 2024 23:14
Copy link

codecov bot commented Jan 30, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (d1ead00) 55.80% compared to head (0b78d9d) 56.94%.
Report is 8 commits behind head on master.

Files Patch % Lines
gnovm/pkg/gnolang/values.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1606      +/-   ##
==========================================
+ Coverage   55.80%   56.94%   +1.14%     
==========================================
  Files         435      436       +1     
  Lines       66055    68245    +2190     
==========================================
+ Hits        36862    38863    +2001     
- Misses      26320    26386      +66     
- Partials     2873     2996     +123     
Flag Coverage Δ
go-1.21.x ∅ <ø> (∅)
misc ∅ <ø> (∅)
misc-_test.genstd ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Jan 31, 2024
@deelawn deelawn linked an issue Jan 31, 2024 that may be closed by this pull request
@deelawn deelawn marked this pull request as ready for review January 31, 2024 00:42
@thehowl
Copy link
Member

thehowl commented Jan 31, 2024

Thanks for the rich description!

@thehowl
Copy link
Member

thehowl commented Feb 8, 2024

good to merge after @piux2 review :)

@jaekwon
Copy link
Contributor

jaekwon commented Feb 21, 2024

looking at this now, finally.

@thehowl thehowl merged commit 0009c44 into gnolang:master Feb 22, 2024
42 checks passed
@deelawn deelawn deleted the bug/1588 branch February 23, 2024 01:34
leohhhn pushed a commit to leohhhn/gno that referenced this pull request Feb 29, 2024
…gnolang#1606)

<!-- please provide a detailed description of the changes made in this
pull request. -->
Addresses gnolang#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

gnovm(bug): time.Time rendering causes Machine Panic
5 participants