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: handle assignments to dereferenced pointer values #1398

Closed
wants to merge 24 commits into from

Conversation

deelawn
Copy link
Contributor

@deelawn deelawn commented Nov 29, 2023

Addresses #1326

This is still a WIP and I need to do a lot more testing with different cases and tweak it accordingly. Before I spend the time to do that, I'm requesting feedback as to whether or not this is the correct approach to fixing this issue. I tried fixing it in a few different places prior to this PR; this was the only place I was successful in doing so. Perhaps the reference value should be resolved and the base value set earlier; I'm not sure.

cc @jaekwon

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 Nov 29, 2023
@deelawn deelawn requested a review from piux2 November 29, 2023 21:12
Copy link

codecov bot commented Nov 29, 2023

Codecov Report

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

Comparison is base (c6097cd) 55.84% compared to head (d03eec7) 55.84%.

Files Patch % Lines
gnovm/pkg/gnolang/values.go 44.73% 18 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1398      +/-   ##
==========================================
- Coverage   55.84%   55.84%   -0.01%     
==========================================
  Files         431      431              
  Lines       65747    65782      +35     
==========================================
+ Hits        36716    36734      +18     
- Misses      26158    26172      +14     
- Partials     2873     2876       +3     
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 Dec 7, 2023
@deelawn deelawn changed the title fix: handle assignments to dereference pointer values fix: handle assignments to dereferenced pointer values Dec 14, 2023
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate a second on these changes?

Were they not reproducing 1167 correctly?

@jaekwon
Copy link
Contributor

jaekwon commented Jan 3, 2024

taking a look...

@@ -273,7 +273,35 @@ func (pv PointerValue) Assign2(alloc *Allocator, store Store, rlm *Realm, tv2 Ty
// Special case of DataByte into (base=*SliceValue).Data.
pv.TV.SetDataByte(tv2.GetUint8())
return
} else if rlm != nil && pv.Base == nil && pv.Index == 0 {
// An index of zero indicates this is an assignment to a realm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An index of zero could mean it's the zero'th element of an array, or the zero'th element of a block,
or whatever the .Base type is. The Index doesn't say much but the .Base type says more but depends on what you mean by a realm object. Like a PackageValue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that an index of zero can mean more than one thing depending on the context, but in this case the realm is not nil and the base is nil, so I think this means that a value is being assigned to a realm object. I'm not sure what the difference is between realm object and PackageValue; they might be synonymous -- the values that are persisted from one block to the next; variables that are declared in a realm and are not local to any function or method.

Copy link
Contributor

@jaekwon jaekwon Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok at least the comment needs fixing, "A base of nil indicates"?
A base of nil indicates an assignment to an escaped object, or a new one that just hasn't been persisted yet.
An escaped object has no owner.
All persisted objects are realm objects.
Some are substructures (base == parent, ref==1), others are escaped.
All realms have an associated package value but not all packages are realms. Depends on the PkgPath name.

@jaekwon
Copy link
Contributor

jaekwon commented Jan 3, 2024

Sidenote: This depends on https://github.com/gnolang/gno/pull/1305/files DeepCopy but DeepCopy is a misnomer.

Base automatically changed from bug/slice-append to master January 4, 2024 23:19
@jaekwon
Copy link
Contributor

jaekwon commented Jan 7, 2024

I think this is the approach we should take: https://github.com/gnolang/gno/pull/1501/files generally speaking.

@deelawn
Copy link
Contributor Author

deelawn commented Mar 25, 2024

Closing in favor of #1501

@deelawn deelawn closed this Mar 25, 2024
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
Archived in project
Development

Successfully merging this pull request may close these issues.

unexpected unreal object when assigning a local variable to a global variable (pointer)
3 participants