Skip to content

fix(gnovm): embedded type identity — struct field name + interface method-set flatten#5739

Open
ltzmaxwell wants to merge 42 commits into
gnolang:masterfrom
ltzmaxwell:fix/alias_2
Open

fix(gnovm): embedded type identity — struct field name + interface method-set flatten#5739
ltzmaxwell wants to merge 42 commits into
gnolang:masterfrom
ltzmaxwell:fix/alias_2

Conversation

@ltzmaxwell

@ltzmaxwell ltzmaxwell commented May 28, 2026

Copy link
Copy Markdown
Contributor
  • Embedded type-alias fields take the field name from the alias as written (type Int = int; struct{ Int }Int, not int), derived from the source AST expr across all three construction paths; covers qualified, pointer, interface, rune/byte, and alias-to-defined-type embeds.
  • Embedded interfaces flatten into their method set at construction (flattenInterfaceMethods), so interface identity is the method set, not the embed/alias spelling: interface{ SAlias } == interface{ Stringer } == interface{ S() string }, matching Go.
  • Cross-package unexported methods keep their origin-package identity via a stamped FieldType.PkgPath (new wire field): sealed interfaces stay sealed through flattening, and unexported selection/satisfaction gate on the origin package, not the enclosing interface's.
  • Consensus-breaking (coordinated upgrade / fresh genesis, same release class as fix(gnovm): match Go's call-time dispatch for interface-bound method values #5737): TypeIDs of alias-embedding structs and embed-carrying interfaces change, and the runtime now assumes flattened interfaces — decoding pre-flattening persisted state is a hard error; regenesis (tx replay) or a re-flatten migration is required (see gnovm/adr/pr5739_interface_method_set_flattening.md).
  • Tests: alias2alias6 + iface_embed_* filetests (identity, cross-package sealing, access gate, conflict rejection), flatten/TypeID/hard-error unit tests, codec parity for the new wire field; all verified against Go.

@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

❌ Patch coverage is 74.28571% with 9 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
gnovm/pkg/gnolang/types.go 70.96% 6 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

@thehowl thehowl added the a/vm GnoVM, Security, Runtime team label Jun 11, 2026
@ltzmaxwell ltzmaxwell changed the title WIP: fix(gnovm): preserve embedded alias name as struct field name fix(gnovm): preserve embedded alias name as struct field name Jun 12, 2026
@ltzmaxwell ltzmaxwell changed the title fix(gnovm): preserve embedded alias name as struct field name WIP fix(gnovm): preserve embedded alias name as struct field name Jun 12, 2026
@ltzmaxwell ltzmaxwell changed the title WIP fix(gnovm): preserve embedded alias name as struct field name fix(gnovm): preserve embedded alias name as struct field name Jun 17, 2026
@ltzmaxwell ltzmaxwell requested review from omarsy and thehowl June 17, 2026 13:24
@davd-gzl davd-gzl self-requested a review June 17, 2026 20:38

@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.

Verified on 155f1a7 against a side-by-side Go run and current master:

                 struct{ Int }     interface{ SAlias }
                 vs struct{ int }   vs interface{ Stringer }
  Go             distinct  ✓        identical ✓
  master (pre)   identical ✗        identical ✓
  PR 155f1a7     distinct  ✓        distinct  ✗  <- new divergence

Full review: https://github.com/samouraiworld/gno-agent-workspace/blob/main/reviews/pr/5xxx/5739-preserve-embedded-alias-name/1-155f1a7/review_claude-opus-4-8_davd-gzl.md

Comment thread gnovm/pkg/gnolang/types.go
…elling

Interface identity is the method set, so interface{ SAlias } must equal interface{ Stringer }; only struct embeds take the written name. Adds alias5 regression test. interface{ Stringer } != interface{ Str()string } stays (pre-existing; needs method-set flattening, see TODO).
Replaces gnolang#5739's minimal interface-embed fix: instead of naming embeds from the resolved type, flatten embedded interfaces into their method set at construction, so the embed name leaves the TypeID entirely. interface{Stringer} == interface{Str()string} now holds too, not just alias-vs-target. Consensus-breaking (anonymous-interface TypeIDs change); land with a chain upgrade.
…tening

The name fillEmbeddedName set on embeds was discarded by flattenInterfaceMethods anyway; embed=false drops the dead work and matches the runtime path.
…branches

The embedded-interface branches in FindEmbeddedFieldType/VerifyImplementedBy only fire for interfaces decoded from persisted state, since new ones are flattened.
…attening interfaces

Flattening hoisted an embedded interface's unexported method into the enclosing interface, which re-qualified it with the wrong package — over-collapsing distinct interface types and letting a local type satisfy another package's sealed interface. Record each method's origin package (new FieldType.PkgPath) and use it for TypeID qualification and satisfaction gating. Adds ADR + cross-package regression filetests. FieldType gains an amino field (empty-guarded in binary, so non-flattened state is unchanged); land with a chain upgrade.
Covers exported vs unexported pkgpath stamping, cross-package origin preservation, diamond dedup, same-name-different-package coexistence, and the conflict panic.
… assume flattened

Records the sequencing: keep the FindEmbeddedFieldType/VerifyImplementedBy recursion until a chain-upgrade migration re-flattens persisted interface types, then drop it (assume-flattened); never assume-flattened before the migration. Also notes the conflict-panic follow-up.
… deferred

All interfaces are flattened unless decoded from pre-flattening bytes. Fresh chain or post-migration => assume-flattened, drop the recursion; in-place upgrade without migration => keep it. Which applies depends on the rollout model (TBD); PR ships with the recursion retained, safe under either.
…s OK, embed-vs-explicit diverges)

Master names the embed from the resolved type and never flattens, so alias-vs-target was already identical on master; only embed-vs-explicit diverged. The alias case broke transiently at 155f1a7 (spelling-based naming). No naming policy fixes embed-vs-explicit — only flattening does.
@ltzmaxwell

ltzmaxwell commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Why flatten is the correct fix — verified on master

Master names an embedded-interface entry from the resolved type (fillEmbeddedName: case *DeclaredType: ft.Name = ct.Name) and never flattens, so the embed's name is part of the interface TypeID. Verified on origin/master (ad09cb7):

comparison master Go
interface{ Stringer } vs interface{ Str() string } distinct identical
interface{ SAlias } vs interface{ Stringer } (type SAlias = Stringer) identical identical

No naming policy — resolved or spelled — can make interface{ Stringer } equal interface{ Str() string }: the embed is still a single named entry (Stringer) rather than its method (Str). Go defines interface identity as the flattened method set — embedding contributes methods, not a name. The only fix that matches Go is to flatten the embed into its methods, removing the embed name from identity. That's why this PR takes the flatten path. (Recorded in gnovm/adr/pr5739_interface_method_set_flattening.md.)

Fixes the make-generate / gno fmt CI check, which requires a blank line between the top-level func and const declarations.
…egory)

alias5 (interface{SAlias}==interface{Stringer}) was an interface-flatten test misfiled in the struct embed-name alias* series; it duplicates iface_embed_id's alias-vs-target case. Remove it and fix alias6's reference. Now alias* = struct embed-name rule, iface_embed_* = interface flattening.
@ltzmaxwell ltzmaxwell requested a review from thehowl June 30, 2026 02:40
@ltzmaxwell ltzmaxwell changed the title fix(gnovm): preserve embedded alias name as struct field name fix(gnovm): flatten embedded interface method sets for type identity Jun 30, 2026
@ltzmaxwell ltzmaxwell changed the title fix(gnovm): flatten embedded interface method sets for type identity fix(gnovm): embedded type identity — struct field name + interface method-set flatten Jun 30, 2026
… coexist

interface{ ifaceext.Sec; ifaceext2.Sec } keeps both p.sec and q.sec as distinct methods (verified against Go), stable identity, distinct from embedding one — pins the (pkgpath,name) dedup-coexistence.
iface_embed_conflict.gno: embedding two interfaces with method M of different signatures is rejected like Go's duplicate-method error — a positioned // Error: from the VM (flattenInterfaceMethods panic wrapped as PreprocessError) and a // TypeCheckError: from go/types. ADR: the conflict path needs no follow-up (already a matchable positioned error).
…only cross-pkg

Remove unused FieldTypeList.Len (Less/Swap already gone). Extract FieldType.originPkg(fallback), shared by idName and VerifyImplementedBy. flattenInterfaceMethods now stamps PkgPath only for cross-package unexported methods; same-package ones stay unstamped (idName fallback), so slow and fast paths produce identical FieldTypes.
Comment thread gnovm/pkg/gnolang/types.go Outdated
Comment thread gnovm/pkg/gnolang/types.go Outdated
@ltzmaxwell

Copy link
Copy Markdown
Contributor Author

880040d (+ c8c7329) resolves the ADR's deferred rollout decision: the runtime now assumes flattened interfaces — the legacy embedded-interface branches in FindEmbeddedFieldType/VerifyImplementedBy are dropped.

  • Invariant: every construction path flattens (flattenInterfaceMethods), so Methods holds only concrete methods (FuncType entries). An InterfaceKind entry is only possible by decoding pre-flattening persisted bytes.
  • Compatibility: tolerating those bytes was a half-measure — flattening already moved interface identity (TypeIDs), so resolving against a legacy type would "work" while equality, type-keyed store entries, and hashes stay silently split from post-change construction. Loud-fail beats silent-wrong.
  • Persistence: pre-flattening state is unsupported. A network carrying it needs fresh genesis (tx replay rebuilds all state flattened) or a re-flatten migration (flatten every persisted InterfaceType, remap old→new TypeIDs). Same release class as fix(gnovm): match Go's call-time dispatch for interface-bound method values #5737: consensus-breaking, coordinated upgrade.
  • Enforcement: panicUnflattened — ungated at TypeID() (once per instance; the production choke), under -tags debugAssert at the hot per-lookup sites.

Details: gnovm/adr/pr5739_interface_method_set_flattening.md.

// (unsupported state); reject at this decode boundary, which
// sees every stored type. See panicUnflattened.
if ct.Methods[i].Type.Kind() == InterfaceKind {
ct.panicUnflattened(ct.Methods[i])

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the one ungated legacy-state check, placed here deliberately: store bytes are external input, and fillType sees every stored type — type entries via GetTypeSafe, and object-carried anonymous types via fillTypesOfValue (only DeclaredType collapses to RefType on persist; unnamed types nest inline, so a GetTypeSafe-only check would miss them). An embed entry can only come from bytes persisted before flattening, so it fails at decode, before any semantics run.

The interior sites (FindEmbeddedFieldType / VerifyImplementedBy / TypeID) assume the invariant on a validated store and only assert under -tags debugAssert.

@ltzmaxwell

Copy link
Copy Markdown
Contributor Author

@thehowl — several changes landed after your approval; a quick second look would be appreciated. Summary:

  • Your two comments applied: the unexported gate now keys on im.originPkg(...) (0c7c632, with iface_embed_xpkg_access.gno pinning the compile-time reject + the origin-package positive direction), and slices.ContainsFunc (7302974).
  • ADR rollout question resolved — runtime now assumes flattened interfaces (880040da43ef22): the legacy embedded-interface branches in FindEmbeddedFieldType/VerifyImplementedBy are dropped. Enforcement is split by concern: one ungated check at the decode boundary (fillType — store bytes are external input; see the inline comment there), while interior sites (FindEmbeddedFieldType/VerifyImplementedBy/TypeID) assert under -tags debugAssert only. Rationale in the updated ADR; PR body now declares the release class (consensus-breaking, coordinated upgrade / fresh genesis, same as fix(gnovm): match Go's call-time dispatch for interface-bound method values #5737).
  • Coverage gaps closed: alias5.gno (rune/byte builtin-alias embeds), and TestDoOpStructType_EmbedNames + alias7.gno for the runtime struct-op path — previously unreached by any filetest (verified by instrumentation).

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 📦 🤖 gnovm Issues or PRs gnovm related

Projects

Development

Successfully merging this pull request may close these issues.

4 participants