[0pcom]

A type can hold two unexported methods of one name, its own and one
promoted from an embedded type in another package, since Go names an
unexported method by its declaring package. RelString spells both the
same, so they shared one symbol: a call through the other package's
interface ran the type's own method, and gvisor's tcp.Endpoint, which
has beforeSave this way, was reported as redeclared. The promoted one
now carries its package.

Fixes #5845

[deadprogram]

Thanks for this fix @0pcom. I ran the tests on my local machine and took a good look. The following is edited from an automated review:

1. The new suffix uses ·, which would be the only non-ASCII character in TinyGo symbol names. The other synthetic names use $, such as $bound, $thunk and $methodset. Should it be $ plus the package path to match?

2. The comment on methodPackageQualifier is a bit hard to read. Perhaps something like:

   // methodPackageQualifier adds the declaring package to an unexported method
   // promoted from another package, since RelString names it like the type's own.

3. The test only covers a value receiver. Perhaps add a pointer receiver case too, since inner.Ptr(&Outer{}) hit the same bug before this change?

What do you think of this?

[0pcom]

@deadprogram Thanks for the review. I agree with all three and pushed 4b23652.

1. The suffix is now $ plus the package path, so the promoted method is (main.Outer).value$github.com/tinygo-org/tinygo/testdata/promoted/inner.
2. I used your comment.
3. The test now has a pointer receiver case. With the qualifier removed, both the value and the pointer case print the outer method's result.

I ran the test on the host and on js/wasm under Node, plus go test ./compiler, with LLVM 23.

[0pcom]

@deadprogram 4b23652 addresses all three points from your review. Could you take another look when you have a moment?

[dgryski]

Thanks for the fixes, 4b23652 looks good to me.

One suggestion in methodPackageQualifier. The check named.Obj().Pkg() == obj.Pkg() compares *types.Package pointers. That works as long as every package is type checked with a shared importer. Comparing Path() avoids relying on that:

if !ok || named.Obj().Pkg() == nil || named.Obj().Pkg().Path() == obj.Pkg().Path() {
    return ""
}

I also wrote some extra test cases and they all pass on the host with the current fix. They cover:

• embedding through a pointer (*inner.Inner)
• two levels of embedding
• two packages with the same unexported method name embedded in one struct
• a generic outer type

The patch is attached below and applies on top of 4b23652.

promoted-extra-tests.patch

diff --git a/testdata/promoted.go b/testdata/promoted.go
index 5023dc18..63680b60 100644
--- a/testdata/promoted.go
+++ b/testdata/promoted.go
@@ -1,6 +1,9 @@
 package main
 
-import "github.com/tinygo-org/tinygo/testdata/promoted/inner"
+import (
+	"github.com/tinygo-org/tinygo/testdata/promoted/inner"
+	"github.com/tinygo-org/tinygo/testdata/promoted/inner2"
+)
 
 // Outer has its own unexported methods and also the ones promoted from
 // inner.Inner, which Go treats as different methods.
@@ -12,9 +15,51 @@ func (Outer) value() int { return 2 }
 
 func (*Outer) ptr() int { return 2 }
 
+type PtrOuter struct {
+	*inner.Inner
+}
+
+func (PtrOuter) value() int { return 2 }
+
+func (PtrOuter) ptr() int { return 2 }
+
+type Outer2 struct {
+	Outer
+}
+
+type Both struct {
+	inner.Inner
+	inner2.Inner2
+}
+
+type G[T any] struct {
+	inner.Inner
+	v T
+}
+
+func (G[T]) value() int { return 2 }
+
+type mainValuer interface{ value() int }
+
 func main() {
 	println("own:", Outer{}.value())
 	println("promoted:", inner.Value(Outer{}))
 	println("own ptr:", (&Outer{}).ptr())
 	println("promoted ptr:", inner.Ptr(&Outer{}))
+
+	po := PtrOuter{&inner.Inner{}}
+	println("embedded ptr own:", po.value())
+	println("embedded ptr promoted:", inner.Value(po))
+	println("embedded ptr promoted ptr:", inner.Ptr(po))
+
+	println("two levels main:", any(Outer2{}).(mainValuer).value())
+	println("two levels inner:", inner.Value(Outer2{}))
+	println("two levels inner ptr:", inner.Ptr(&Outer2{}))
+
+	println("both inner:", inner.Value(Both{}))
+	println("both inner2:", inner2.Value(Both{}))
+
+	println("generic own:", G[int]{}.value())
+	println("generic promoted:", inner.Value(G[int]{}))
+	println("generic string promoted:", inner.Value(G[string]{}))
 }
diff --git a/testdata/promoted.txt b/testdata/promoted.txt
index 78e2c74d..5c368cc3 100644
--- a/testdata/promoted.txt
+++ b/testdata/promoted.txt
@@ -2,3 +2,14 @@ own: 2
 promoted: 1
 own ptr: 2
 promoted ptr: 1
+embedded ptr own: 2
+embedded ptr promoted: 1
+embedded ptr promoted ptr: 1
+two levels main: 2
+two levels inner: 1
+two levels inner ptr: 1
+both inner: 1
+both inner2: 3
+generic own: 2
+generic promoted: 1
+generic string promoted: 1
diff --git a/testdata/promoted/inner2/inner2.go b/testdata/promoted/inner2/inner2.go
new file mode 100644
index 00000000..b80af036
--- /dev/null
+++ b/testdata/promoted/inner2/inner2.go
@@ -0,0 +1,9 @@
+package inner2
+
+type Inner2 struct{}
+
+func (Inner2) value() int { return 3 }
+
+type valuer interface{ value() int }
+
+func Value(v any) int { return v.(valuer).value() }

[0pcom]

Thanks @dgryski. f90c748 switches to comparing Path() and adds your test cases. They pass with LLVM 23 on top of current dev, together with calls, generics and interface.

[dgryski]

LGTM

[dgryski]

Please rebase and fix the conflict in binary-size.txt.

[0pcom]

Merged dev in. The test list moved to build_test.go, so promoted.go is added there now.

[dgryski]

This branch needs to have a linear history so we can merge it. No merge commits, please.

[0pcom]

Rebased onto dev with no merge commits.