gno icon indicating copy to clipboard operation
gno copied to clipboard

fix: adjust and clarify behavior of pointers and values shared between realms

Open deelawn opened this issue 1 year ago • 5 comments

Closes #974.

The first part of this PR addresses the issue linked above. The issue details a scenario in which pointer ownership was able to be stolen by passing the pointer value to another realm that saved the value to it's own realm during initialization. The solution for this is to persist the mem package before running any init functions. This should be fine because realm finalization happens the same number of times.

The realm initialization behavior before (still the behavior for packages):

  1. Finalize realm after each init function
  2. Finalize the mem package

New behavior for realms:

  1. Finalize the mem package
  2. Finalize the realm after each init function

The second part of this PR attempts to clarify what the expected behavior should be when sharing pointers to values amongst realms. It achieves this by updating the effective gno docs to detail the expected behavior and to generally discourage users from doing things like this unless there is a good reason to do so and they have a strong understanding of how this is expected to work. There are also clarifying txtar tests that have been added to test the expected behavior.

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
  • [x] Added new benchmarks to generated graphs, if any. More info here.

deelawn avatar Mar 29 '24 20:03 deelawn

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 48.27%. Comparing base (b2f12a9) to head (4c59eb3). Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1861      +/-   ##
==========================================
+ Coverage   48.25%   48.27%   +0.02%     
==========================================
  Files         408      409       +1     
  Lines       62338    62360      +22     
==========================================
+ Hits        30081    30106      +25     
+ Misses      29749    29737      -12     
- Partials     2508     2517       +9     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 29 '24 20:03 codecov[bot]

Marking as draft until the failing tests are fixed. I took a brief look and I see two categories of failing tests -- I may be completely wrong about these and will need to spend more time looking at them.

  1. ~~Tests are failing that are relying on state to be saved in packages. Tests previously allowed package state because the mem package was persisted after running init. Now that the mem package is saved before running init, state variables initialized within the init function won't get persisted (maybe) source~~ addressed by 77032fc and 5342c67
  2. Saving the mem package before init is causing an issue when trying to assign a value to a realm interface type variable from within the init. It's not just the assign, actually the further mutations on the object (reassignments via star expr) that are causing some issues when finalizing the realm. source

I think this change exposes a known issue with trying to assign a value to a nil realm pointer variable that is of a declared type -- https://github.com/gnolang/gno/issues/1326

deelawn avatar Mar 29 '24 21:03 deelawn

Relevant, older PR by @n0izn0iz : https://github.com/gnolang/gno/pull/1074

zivkovicmilos avatar Apr 17 '24 13:04 zivkovicmilos

back on this.

jaekwon avatar Apr 17 '24 17:04 jaekwon

@mvertes @petar-dambovaliev please look into this

Kouteki avatar May 23 '24 09:05 Kouteki

Closing; #2255 also fixed this issue.

deelawn avatar Jun 27 '24 17:06 deelawn