compiler: disambiguate function-local named types

[jakebailey]

Fixes #5180

This is an alternative to #5183 which handles more cases.

This is really tricky and I think only possible through the more complicated method in this PR, where we have to walk through more stuff to assign names where needed. As far as I can tell, this works even with build caching, but obviously comes at a cost.

I recommend looking just at the last commit, as that's the one that shows the change including how test golden files get updated.

[dgryski]

Nice. Handles generics (which my PR didn't). One more sign off from @niaow and/or @aykevl and can merge this.

[aykevl]

Didn't do a full review but here are some notes.

[aykevl]

Have you looked at typeutil.Map? It might do exactly what you want with much less complexity.

[jakebailey]

Yeah, I just didn't think it really added much, because all it does is store the type, which we then just .Obj() on, so doesn't really do anything?

[jakebailey]

For reference, this is what I mean:

diff --git a/compiler/compiler.go b/compiler/compiler.go
index 353a1799..55474033 100644
--- a/compiler/compiler.go
+++ b/compiler/compiler.go
@@ -92,7 +92,7 @@ type compilerContext struct {
 	pkg              *types.Package
 	packageDir       string // directory for this package
 	runtimePkg       *types.Package
-	localTypeNames   map[*types.TypeName]string
+	localTypeNames   typeutil.Map
 }
 
 // newCompilerContext returns a new compiler context ready for use, most
@@ -107,7 +107,6 @@ func newCompilerContext(moduleName string, machine llvm.TargetMachine, config *C
 		targetData:     machine.CreateTargetData(),
 		functionInfos:  map[*ssa.Function]functionInfo{},
 		astComments:    map[string]*ast.CommentGroup{},
-		localTypeNames: map[*types.TypeName]string{},
 	}
 
 	c.ctx = llvm.NewContext()
diff --git a/compiler/interface.go b/compiler/interface.go
index 83034ae5..741b5b94 100644
--- a/compiler/interface.go
+++ b/compiler/interface.go
@@ -596,11 +596,11 @@ func (c *compilerContext) getTypeCodeName(t types.Type) (name string, isLocal bo
 		// Function-local type. Both ordinary locals (Parent() != nil)
 		// and synthetic locals from generic instantiation
 		// (Parent() == nil) are pre-registered by scanLocalTypes.
-		n, ok := c.localTypeNames[tn]
-		if !ok {
+		v := c.localTypeNames.At(t)
+		if v == nil {
 			panic("compiler: local type " + tn.Name() + " was not registered by scanLocalTypes")
 		}
-		return "named:" + n, true
+		return "named:" + v.(string), true
 	case *types.Array:
 		s, isLocal := c.getTypeCodeName(t.Elem())
 		return "array:" + strconv.FormatInt(t.Len(), 10) + ":" + s, isLocal
@@ -851,7 +851,7 @@ func (c *compilerContext) scanLocalTypes(ssaPkg *ssa.Package) {
 // after sorting, before scanLocalTypes returns and any
 // getTypeCodeName lookups happen.
 func (c *compilerContext) registerLocalTypes(fn *ssa.Function, synthetic bool) {
-	var found []*types.TypeName
+	var found []*types.Named
 	seen := map[types.Type]bool{}
 	var visit func(t types.Type)
 	visit = func(t types.Type) {
@@ -865,9 +865,9 @@ func (c *compilerContext) registerLocalTypes(fn *ssa.Function, synthetic bool) {
 		case *types.Named:
 			tn := t.Obj()
 			if tn.Pkg() != nil && (tn.Parent() == nil) == synthetic {
-				if _, ok := c.localTypeNames[tn]; !ok {
-					c.localTypeNames[tn] = ""
-					found = append(found, tn)
+				if c.localTypeNames.At(t) == nil {
+					c.localTypeNames.Set(t, "")
+					found = append(found, t)
 				}
 			}
 			targs := t.TypeArgs()
@@ -946,11 +946,11 @@ func (c *compilerContext) registerLocalTypes(fn *ssa.Function, synthetic bool) {
 	// that is stable across builds and unaffected by //line directives
 	// (which only adjust the human-facing position from Fset.Position).
 	sort.Slice(found, func(i, j int) bool {
-		return found[i].Pos() < found[j].Pos()
+		return found[i].Obj().Pos() < found[j].Obj().Pos()
 	})
 	enclosing := fn.RelString(nil)
-	for i, tn := range found {
-		c.localTypeNames[tn] = enclosing + "." + tn.Name() + "$" + strconv.Itoa(i+1)
+	for i, named := range found {
+		c.localTypeNames.Set(named, enclosing+"."+named.Obj().Name()+"$"+strconv.Itoa(i+1))
 	}
 }

[aykevl]

Yes it stores the type, but from what I understand it also deduplicates types. Which it looks like you have reimplemented? So I think that by using typeutil.Map you can remove the deduplication logic.

[jakebailey]

I don't think that works in the case of build caching? We need to produce a string that is going to be identical between compiles. If I'm understanding correctly, you're suggesting using that map to autonumber types, but what I think would happen is that two different compilation units can see the same types in differing orders and give them different names. E.g. if package A has a generic func with an interior type, and then separate packages B and C depend on A and instantiate that generic the same, they would not agree on the autonumbered name.

package A
func F[T any]() any {
	type Inner struct{ x T }
	return Inner{}
}

package B
var x = F[int]     // Inner#1

package C
var a = F[string]  // Inner#1
var b = F[int]     // Inner#2

For regular situations, a Map would work I think, so I can see if there's a hybrid solution, but it seems tricky...

[aykevl]

Hmm that is a good point.

This does however give more weight to my idea of writing our own generics implementation, which would solve this and a few other issues. The idea would be to define all the possible implementations for a function in the package that is being compiled, so that packages that depend on it don't need to know about the implementation.

[jakebailey]

I'm not sure how that's possible; the possibilities are infinite.

I'm investigating alternative approaches to this PR, though; I'm still not happy with the complexity. Maybe something closer to @dgryski's PR was right after all...

[jakebailey]

Well, a lot of what I had was overkill, so the current state should be simpler still.

[aykevl]

To clarify: token.Pos is not used to uniquely identify a type, it is only used for sorting?
Asking since some things (like CGo) might not produce unique locations.

[jakebailey]

Correct, it is not, though I thought cgo was not supported in tinygo?

[deadprogram]

I thought cgo was not supported in tinygo?

CGo is for sure supported. We use this very heavily, for example see the recent work in https://github.com/tinygo-org/espradio

[jakebailey]

In any case, this is token.Pos, so is just the position inside FileSet, so I believe is safe to depend on even in the presense of cgo, but I don't quite know how I'd test it.

[aykevl]

See #3565 for what looks like a similar bug.