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

feat: Bound Method Realm support #1257

Merged
merged 8 commits into from
Jul 6, 2024
Merged

feat: Bound Method Realm support #1257

merged 8 commits into from
Jul 6, 2024

Conversation

jaekwon
Copy link
Contributor

@jaekwon jaekwon commented Oct 19, 2023

When a function is called, it's usually enough to get the pkgpath from where the function is declared, to find out whether we are switching to a new realm or not.

But when the function is a bound method, and the method and declared type are defined in a /p/ package, we need to look at the owning (persisting) pkgpath of the receiver, if any. When one is found, it uses that.

There are some edge cases to consider, I'm not sure if they are all fixed, but we will find out.
Here are some that are considered;

  1. if the receiver is an unpersisted new object with method, declared in a p module, it has no realm until it becomes persisted.
  2. nil receivers aren't objects, so even if it were stored in a realm, it would be stored by "reference" from a pointervalue which also is not an object, so there's no realm to consider even after persistence.
  3. if the method from 1 makes any modifications to the /p/ package's block state, it should result in an error and the tx should fail.
  4. if a /p/ declared thing with method gets stored in realm X, it should be possible for realm Y to call it, and have it modify realm X (e.g. say realm X attached a closure to the thing).

@jaekwon jaekwon requested a review from moul as a code owner October 19, 2023 13:46
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Oct 19, 2023
@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.75%. Comparing base (1169658) to head (a61ab41).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1257   +/-   ##
=======================================
  Coverage   54.74%   54.75%           
=======================================
  Files         592      592           
  Lines       79197    79145   -52     
=======================================
- Hits        43359    43333   -26     
+ Misses      32568    32547   -21     
+ Partials     3270     3265    -5     
Flag Coverage Δ
contribs/gnodev 23.53% <ø> (ø)
contribs/gnofaucet 15.31% <ø> (+0.85%) ⬆️
contribs/gnokeykc 0.00% <ø> (ø)
contribs/gnomd 0.00% <ø> (ø)
gno.land 62.89% <ø> (ø)

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.

tbruyelle pushed a commit to albttx/gno that referenced this pull request Oct 19, 2023
@jaekwon jaekwon requested a review from a team as a code owner October 19, 2023 21:10
gnovm/pkg/gnolang/machine.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Oct 19, 2023
@albttx
Copy link
Member

albttx commented Nov 2, 2023

Any update on this @jaekwon / @moul ?

@moul moul marked this pull request as draft January 8, 2024 17:20
@thehowl
Copy link
Member

thehowl commented Feb 14, 2024

txtar created by albttx which illustrates the kind of behaviour that should be fixed by this PR (or any continuation of this work) -- place in gnovm/cmd/gno/testadata/gno_test/:

# Set up GNOROOT in the current directory.
mkdir $WORK/gnovm
symlink $WORK/gnovm/stdlibs -> $GNOROOT/gnovm/stdlibs
env GNOROOT=$WORK

gno test -verbose ./examples/gno.land/r/demo/realm2

stderr '=== RUN   TestDo'
stderr '--- PASS: TestDo.*'

stderr '=== RUN   file/realm2_filetest.gno'
stderr '--- PASS: file/realm2_filetest.*'

-- examples/gno.land/p/demo/counter/gno.mod --
module gno.land/p/demo/counter

-- examples/gno.land/p/demo/counter/counter.gno --
package counter

type Counter struct {
	n int
}

func (c *Counter) Inc() {
	c.n++
}

-- examples/gno.land/r/demo/realm1/realm1.gno --
package realm1

import "gno.land/p/demo/counter"

var c = counter.Counter{}

func GetCounter() *counter.Counter {
	return &c
}

-- examples/gno.land/r/demo/realm2/realm2.gno --
package realm2

import (
	"gno.land/r/demo/realm1"
)

func Do() {
	realm1.GetCounter().Inc()
}

-- examples/gno.land/r/demo/realm2/realm2_filetest.gno --
// PKGPATH: gno.land/r/tests
package tests

import "gno.land/r/demo/realm2"

func main() {
	realm2.Do()
	println("OK")
}

// Output:
// OK

-- examples/gno.land/r/demo/realm2/realm2_test.gno --
package realm2

import "testing"

func TestDo(t *testing.T) {
	Do()
}

@jaekwon jaekwon force-pushed the dev/jae/boundmethodrealm branch from 75b1d10 to 368d14f Compare July 5, 2024 22:36
@jaekwon jaekwon changed the title WIP Bound Method Realm support feat: Bound Method Realm support Jul 5, 2024
@jaekwon jaekwon marked this pull request as ready for review July 5, 2024 22:37
@jaekwon jaekwon requested review from piux2, deelawn, thehowl, mvertes and a team as code owners July 5, 2024 22:37
@jaekwon jaekwon force-pushed the dev/jae/boundmethodrealm branch from 368d14f to 23f7173 Compare July 5, 2024 22:42
Signed-off-by: moul <[email protected]>
@moul moul requested a review from gfanton as a code owner July 6, 2024 14:26
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Jul 6, 2024
@moul moul merged commit 9ced20b into master Jul 6, 2024
89 checks passed
@moul moul deleted the dev/jae/boundmethodrealm branch July 6, 2024 14:56
gfanton pushed a commit to gfanton/gno that referenced this pull request Jul 23, 2024
When a function is called, it's usually enough to get the pkgpath from
where the function is declared, to find out whether we are switching to
a new realm or not.

But when the function is a bound method, and the method and declared
type are defined in a /p/ package, we need to look at the owning
(persisting) pkgpath of the receiver, if any. When one is found, it uses
that.

There are some edge cases to consider, I'm not sure if they are all
fixed, but we will find out.
Here are some that are considered;
1. if the receiver is an unpersisted new object with method, declared in
a p module, it has no realm until it becomes persisted.
2. nil receivers aren't objects, so even if it were stored in a realm,
it would be stored by "reference" from a pointervalue which also is not
an object, so there's no realm to consider even after persistence.
3. if the method from 1 makes any modifications to the /p/ package's
block state, it should result in an error and the tx should fail.
4. if a /p/ declared thing with method gets stored in realm X, it should
be possible for realm Y to call it, and have it modify realm X (e.g. say
realm X attached a closure to the thing).

---------

Signed-off-by: moul <[email protected]>
Co-authored-by: moul <[email protected]>
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 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: Done
Status: ✅ Done
Status: 🌟 Wanted for Launch
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants