Skip to content

fix(gnovm): match Go's call-time dispatch for interface-bound method values#5737

Open
ltzmaxwell wants to merge 13 commits into
gnolang:masterfrom
ltzmaxwell:fix/defer12
Open

fix(gnovm): match Go's call-time dispatch for interface-bound method values#5737
ltzmaxwell wants to merge 13 commits into
gnolang:masterfrom
ltzmaxwell:fix/defer12

Conversation

@ltzmaxwell

@ltzmaxwell ltzmaxwell commented May 28, 2026

Copy link
Copy Markdown
Contributor
  • Interface-bound method values (g := i.M, defer i.M()) now resolve their concrete method and receiver at call time, matching Go: nil-panic timing, value snapshot, embedded promotion (value/pointer/interface fields, any depth), field re-read through a boxed pointer, and dynamic re-dispatch. Also fixes two VM crashes: defer i.M() on a non-nil interface, and a nil embedded pointer-receiver (c5).
  • Mechanism: BoundMethodValue gains a Method field; Func == nil marks a lazy bind resolved at the call (resolveLazyBound); deferred callables are unified onto Defer.Callable (type-switched like doOpPrecall). Concrete/pointer binds (pt.M, t.M) are unchanged.
  • Hard fork: Method is persisted (proto field 4; pb3_gen regenerated), _allocBoundMethodValue 200→216 → wire/merkle/gas change; coordinated-upgrade / fresh-genesis only (like ADR-5544), old state still decodes.
  • Gas: the dispatch walk moved bind→call, so both consts were re-fit — OpCPUSelectorInterface 751→276 (the lazy bind no longer walks) and new OpCPULazyBoundResolve=529 (the call-time walk); net ≈ +54/interface call. Ratio-scaled (reference HW unavailable) — calibration TODO before the fork.
  • Tests: classed filetests (timing / nil / embedded / operand-capture / dynamic / defer) + persist and boxed-cur realm txtars; rationale in gnovm/adr/pr5737_nil_value_method_panic_timing.md. Pre-existing orthogonal divergences filed separately: method expressions don't support interface, promoted, or mixed-receiver forms #5787, GnoVM: calling a method on a nil interface panics uncatchably instead of a recoverable nil-pointer panic #5850.

@Gno2D2

Gno2D2 commented May 28, 2026

Copy link
Copy Markdown
Collaborator

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):
  • IGNORE the bot requirements for this PR (force green CI check)
Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 The pull request was created from a fork (head branch repo: ltzmaxwell/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission

@codecov

codecov Bot commented May 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@davd-gzl davd-gzl self-requested a review May 29, 2026 11:02
@thehowl thehowl added the a/vm GnoVM, Security, Runtime team label Jun 11, 2026

@davd-gzl davd-gzl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a design problem, not a tweak. The root cause sits a level deeper than the single flag site, so as written the change fixes the interface case but regresses the concrete one. Go has two distinct timings: a concrete *T(nil) derefs *pt when the method value is formed, so it panics eagerly at bind, while an interface-boxed *T only derefs inside the call, so its panic lands at call time. The nilReceiverPanic flag is read at the single VPDerefValMethod site, but that site runs after the interface is unwrapped to a bare *T, so it can't tell the two apart and defers both, when only the interface case should defer. To hold both, the decision needs to live where the receiver kind is still known (interface dispatch, or marked in the preprocessor) rather than at that one late site.

receiver form        Go            master         this PR
-------------------  ------------  -------------  --------------
defer pt.M() concrete eager(=0)    eager(=0) ✓    call-time(=1) ✗   <- regressed
defer i.M()  iface    call-time(=1) eager(=0) ✗   call-time(=1) ✓   <- fixed
G=i.M; persist; G()   panic         (binds eager)  no panic ✗        <- new hole

The current filetest covers the interface case; the concrete case and the persistence hole below are both unexercised today, so they'd be worth adding. An ADR pinning the intended semantics would also help anchor the fix.

Full review: https://github.com/samouraiworld/gno-agent-workspace/blob/main/reviews/pr/5xxx/5737-defer-nil-receiver-panic/1-4c57c37e4/claude-opus-4-8_davd-gzl.md

Repros run at 4c57c37.

Comment thread gnovm/pkg/gnolang/values.go
Comment thread gnovm/pkg/gnolang/values.go Outdated
@ltzmaxwell ltzmaxwell force-pushed the fix/defer12 branch 11 times, most recently from 47b512e to db352a1 Compare June 18, 2026 13:38
@ltzmaxwell ltzmaxwell changed the title fix(gnovm): defer nil-pointer panic for value method bound to nil receiver fix(gnovm): match Go's call-time semantics for interface value-method values Jun 18, 2026
@ltzmaxwell ltzmaxwell changed the title fix(gnovm): match Go's call-time semantics for interface value-method values fix(gnovm): match Go's call-time semantics for interface value-method receivers Jun 18, 2026
@ltzmaxwell ltzmaxwell force-pushed the fix/defer12 branch 2 times, most recently from 0b89756 to 581b58c Compare June 22, 2026 06:57
@ltzmaxwell ltzmaxwell changed the title fix(gnovm): match Go's call-time semantics for interface value-method receivers fix(gnovm): match Go's call-time dispatch for interface-bound method values Jun 22, 2026
@ltzmaxwell ltzmaxwell force-pushed the fix/defer12 branch 2 times, most recently from d1bdb5a to 48dae0c Compare June 22, 2026 07:58
@ltzmaxwell ltzmaxwell force-pushed the fix/defer12 branch 5 times, most recently from 14a0c36 to 0a1d788 Compare June 24, 2026 03:28
Comment thread gnovm/pkg/gnolang/alloc.go
…values

Interface-bound method values now resolve their concrete method and receiver
at call time (deref, value snapshot, nil panic, embedded-field re-read and
dynamic re-dispatch all match Go), instead of binding eagerly. The call-time
dispatch is charged via OpCPULazyBoundResolve. Hard-fork: BoundMethodValue
gains a persisted Method field (wire/merkle/gas change).
Comment thread gnovm/pkg/gnolang/values.go
Comment thread gnovm/pkg/gnolang/op_call.go Outdated
…patch

Raise a fatal, gno-unrecoverable panic when the call-time dispatch loop revisits an operand, instead of hanging. Covers direct/indirect/embedded/deferred/persisted cycles plus negative guards.
Thread the machine into resolveLazyBound and charge OpCPULazyBoundResolve each iteration so cost scales with embedded-interface depth. Gas-neutral for the common single-hop case; complements (does not replace) cycle detection.
Comment thread gnovm/pkg/gnolang/values.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a/vm GnoVM, Security, Runtime team 📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related

Projects

Development

Successfully merging this pull request may close these issues.

5 participants