fix(gnovm): depth-based shadowing for promoted struct fields and methods#5820
Draft
omarsy wants to merge 2 commits into
Draft
fix(gnovm): depth-based shadowing for promoted struct fields and methods#5820omarsy wants to merge 2 commits into
omarsy wants to merge 2 commits into
Conversation
StructType.FindEmbeddedFieldType treated any second embedded match as a
conflict, with no notion of promotion depth. A method or field promoted
from a shallower embedded field therefore did NOT shadow a same-named one
reachable deeper, so gno reported a spurious conflict and rejected valid
programs:
type Deep struct{}; func (Deep) M() {}
type Mid struct{ Deep } // M at depth 2
type Shallow struct{}; func (Shallow) M() {}
type T struct { Shallow; Mid } // Go: T.M resolves to Shallow.M
gno rejected `var i I = T{}` at preprocess with "T does not implement I
(missing method M)". On-chain addpkg type-checks with go/types first
(which accepts these), so such a package passed the deployment gate and
then panicked at VM preprocess.
Search direct fields first (a depth-0 field shadows promotion), then keep
the shallowest embedded candidate (trail length = depth); two at the same
shallowest depth are ambiguous. The recursion-guard set and the return
contract are unchanged, so runtime selector navigation is unaffected.
Diamond ambiguity (the same embedded type reached via two paths at equal
depth) remains accepted as on upstream; it is gated on-chain by go/types
and is left as a follow-up (see ADR). Confirmed against the Go toolchain
across shadowing, multi-level, pointer-embedding, same-depth ambiguity,
internally-ambiguous siblings, self-cycle, and embedded-interface cases.
Collaborator
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
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:
☑️ Reviewer Actions:
📚 Resources:Debug
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
StructType.FindEmbeddedFieldTypetreated any second embedded match as a conflict, with no notion of promotion depth. So a method or field promoted from a shallower embedded field did not shadow a same-named one reachable deeper, and gno reported a spurious conflict and rejected valid programs:gno rejected
var i I = T{}at preprocess withT does not implement I (missing method M). On-chainaddpkgtype-checks with go/types first (which accepts these), so such a package passed the deployment gate and then panicked at VM preprocess.Change
Search direct (depth-0) fields first (a direct field shadows promotion), then keep the shallowest embedded candidate (trail length = depth); two at the same shallowest depth are ambiguous. The recursion-guard set and the 5-value return contract are unchanged, so runtime selector navigation and all callers are unaffected.
Scope (important)
This closes the common disjoint-subtree shadowing case. It is a minimal, zero-regression step (verified by differential fuzzing vs. the Go toolchain: 0 regressions over 400 random shapes). It does not fix the broader pre-existing promotion bug where the shared recursion-guard set prunes legitimate paths when the same type is reachable via multiple embedded paths — tracked in #5819, which also covers the diamond false-accept. The ADR documents both out-of-scope cases.
Tests
embed_method_shadow0/1/2pin the fix (they panic at preprocess on the old code);embed_method_shadow_ambiguous,embed_method_shadow_fieldmethod, andembed_method_shadow_internal_ambiguousare parity/regression guards (pass on old + new), with go/types parity viaTypeCheckErrordirectives. Confirmed against the Go toolchain across shadowing, multi-level, pointer-embedding, same-depth ambiguity, internally-ambiguous siblings, self-cycle, and embedded-interface cases.Note
Draft. Local full-
TestFilessuite passes (0 failures); CI will also run it. This PR is AI-assisted; ADR included per AGENTS.md (gnovm/adr/prxxxx_embedded_method_depth_shadowing.md, to be renamed with the PR number).Related spec-compliance fixes from the same effort: #5784, #5785, #5807, #5808.
🤖 Generated with Claude Code