NamedInterfaceOf/InterfaceOf/NewInterfaceType/SetInterfaceType for llgo

[visualfc]

No description provided.

[gemini-code-assist]

Code Review

This pull request refactors interface method handling and cache lookups in reflectx by extracting common logic into helper functions and implementing llgo-specific runtime stubs. The review feedback suggests that setMethodSet on llgo should return an unimplemented error or panic instead of silently returning nil to prevent silent failures. Additionally, the newInterface implementations in both methodof.go and rtype_llgo.go should be optimized to check the cache before performing heap allocations, utilizing strings.Builder for efficient signature string construction.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

The setMethodSet function currently silently returns nil on llgo. This can lead to hard-to-debug runtime issues where methods are expected to be set but are silently ignored. Since setMethodSet returns an error, it should return an unimplemented error (or panic like newMethodSet does) to make the lack of implementation explicit.

func (ctx *Context) setMethodSet(typ reflect.Type, methods []Method, sortMethods bool) error {
	return fmt.Errorf("setMethodSet: not implemented on llgo")
}

[gemini-code-assist]

The newInterface function allocates rt and populates st.Methods before checking the cache. This results in unnecessary heap allocations on every cache hit. We can optimize this by constructing the interface signature string first, checking the cache, and only performing the allocations on a cache miss. Additionally, we can use strings.Builder to construct the signature string efficiently without allocating an intermediate slice of strings.

func (ctx *Context) newInterface(methods []reflect.Method) reflect.Type {
	var str string
	var sb strings.Builder
	var lastname string
	first := true
	for _, m := range methods {
		if m.Name == lastname {
			continue
		}
		lastname = m.Name
		if first {
			sb.WriteString("*interface { ")
			first = false
		} else {
			sb.WriteString("; ")
		}
		sb.WriteString(methodStr(m.Name, m.Type))
	}
	if first {
		str = "*interface {}"
	} else {
		sb.WriteString(" }")
		str = sb.String()
	}

	if t, ok := ctx.interfceLookupCache[str]; ok {
		return t
	}

	rt, _ := newType("", "", tyEmptyInterface, 0, 0)
	st := (*interfaceType)(toKindType(rt))
	st.Methods = nil
	lastname = ""
	for _, m := range methods {
		if m.Name == lastname {
			continue
		}
		lastname = m.Name
		isexport := methodIsExported(m.Name)
		var mname nameOff
		nm := newNameEx(m.Name, "", isexport, !isexport)
		mname = resolveReflectName(nm)
		if !isexport {
			setPkgPath(nm, m.PkgPath)
		}
		st.Methods = append(st.Methods, imethod{
			Name: mname,
			Typ:  resolveReflectType(totype(m.Type)),
		})
	}
	if len(st.Methods) > 0 {
		rt.Equal = interequal
	}
	rt.Str = resolveReflectName(newName(str, "", false))
	typ := toType(rt)
	ctx.interfceLookupCache[str] = typ
	return typ
}

[gemini-code-assist]

Similar to the non-llgo implementation, newInterface in rtype_llgo.go allocates rt and populates st.Methods before checking the cache. We can optimize this by constructing the interface signature string first using strings.Builder, checking the cache, and only performing the allocations on a cache miss.

func (ctx *Context) newInterface(methods []reflect.Method) reflect.Type {
	var str string
	var sb strings.Builder
	var lastname string
	first := true
	for _, m := range methods {
		if m.Name == lastname {
			continue
		}
		lastname = m.Name
		if first {
			sb.WriteString("*interface { ")
			first = false
		} else {
			sb.WriteString("; ")
		}
		sb.WriteString(methodStr(m.Name, m.Type))
	}
	if first {
		str = "*interface {}"
	} else {
		sb.WriteString(" }")
		str = sb.String()
	}

	if t, ok := ctx.interfceLookupCache[str]; ok {
		return t
	}

	rt, _ := newType("", "", tyEmptyInterface, 0, 0)
	st := (*interfaceType)(toKindType(rt))
	st.Methods = nil
	lastname = ""
	var mname string
	for _, m := range methods {
		if m.Name == lastname {
			continue
		}
		lastname = m.Name
		if !methodIsExported(m.Name) {
			mname = m.PkgPath + "." + m.Name
		} else {
			mname = m.Name
		}
		st.Methods = append(st.Methods, imethod{
			Name_: mname,
			Typ_:  totype(m.Type).FuncType(),
		})
	}
	if len(st.Methods) > 0 {
		rt.Equal = interequal
	}
	rt.Str_ = str
	typ := toType(rt)
	ctx.interfceLookupCache[str] = typ
	return typ
}

[fennoai]

Code Review Summary

This PR refactors SetInterfaceType/InterfaceOf/NewInterfaceType to share common helpers and adds llgo-specific implementations. The interface-building path looks correct for llgo. Several issues need attention before merging.

Critical / High

1. newMethodSet panics unconditionally for llgo builds (runtime crash)

rtype_llgo.go adds newMethodSet that always panics. method.go previously had //go:build !llgo removed in this PR, so NewMethodSet and StructToMethodSet will now call newMethodSet under llgo and crash unconditionally. The commented-out implementation above the panic suggests the intent is known — but the PR removes the build guard without adding a replacement guard or t.Skip in tests.

2. method_test.go: removing //go:build !llgo breaks llgo test runs

Tests like TestIntMethodOf, TestSliceMethodOf, TestStructMethodOf all call reflectx.NewMethodSet, which (see above) panics under llgo. These tests should be split into a separate file or individually guarded with t.Skip if llgo support for NewMethodSet is not yet ready.

3. setMethodSet for llgo is a silent no-op returning nil error

rtype_llgo.go's setMethodSet sorts the slice but never applies anything, then returns nil. Callers get a false success signal. At minimum, a // TODO: not yet implemented for llgo comment is needed so callers understand the semantics differ.

4. Cross-module //go:linkname to an internal package

//go:linkname interequal github.com/goplus/llgo/runtime/internal/runtime.interequal binds to an internal/ package of a separate module. That symbol has no stability contract — llgo can rename or change its signature without it being a breaking change. If the ABI signature differs from func(unsafe.Pointer, unsafe.Pointer) bool, assigning it to rtype.Equal produces undefined behaviour. Consider copying the implementation locally (as done in x/reflect/reflect.go) or exposing a stable shim in llgo's non-internal package.

Medium

5. //go:linkname haveIdenticalType reflect.haveIdenticalType has no Go-version guard on the llgo path

The non-llgo path has a version split (linkname_go121.go vs linkname_go123.go) specifically because reflect.haveIdenticalType is not directly linkname-accessible in Go 1.23+. The llgo path unconditionally uses reflect.haveIdenticalType regardless of Go version. If llgo can run against Go 1.23+, this will produce a link error.

6. unnamed parameter silently ignored in llgo's setInterfaceMethods

The caller SetInterfaceType passes typ.Name() == "" as the unnamed argument expecting it to affect behaviour (as it does in the non-llgo path). The llgo implementation accepts the parameter but never reads it. This is likely intentional due to llgo's different type representation, but it should be documented with a comment to prevent future maintainers from suspecting a bug.

7. ctx.newInterface allocates before checking the cache

Both implementations call newType(...) (heap allocation) and build the full method slice before consulting interfceLookupCache. On a cache hit the allocation is immediately discarded as garbage. Building the string key first and doing an early-return cache check would avoid unnecessary allocation on the hot path.

[fennoai]

//go:linkname interequal github.com/goplus/llgo/runtime/internal/runtime.interequal links to an internal/ package of an external module (llgo). This symbol carries no stability contract: llgo can rename or change its ABI without it being a breaking change. If the signature differs from func(unsafe.Pointer, unsafe.Pointer) bool, assigning it to rtype.Equal is undefined behaviour. Consider copying the implementation inline (as done in x/reflect/reflect.go) or linking to a stable exported shim.

[fennoai]

This unconditionally links to reflect.haveIdenticalType regardless of Go version. The non-llgo path splits this across linkname_go121.go / linkname_go123.go specifically because the symbol is not directly accessible in Go 1.23+. If llgo can run on Go 1.23+, this will produce a link error. Please add an equivalent version guard.

[fennoai]

panic("TODO newMethodSet") — method.go no longer has //go:build !llgo, so NewMethodSet and StructToMethodSet are now active for llgo builds and will reach this panic. Either restore a build guard on method.go for these functions, or add t.Skip in tests, until the implementation is ready.

[fennoai]

setMethodSet sorts the input but never applies any methods and returns nil. Callers receive a false success signal. Please add a // TODO: not yet implemented for llgo comment (or equivalent) so callers understand the semantics differ from the non-llgo path.

[fennoai]

The unnamed parameter is accepted but never read — the llgo path always encodes names the same way regardless. This is likely intentional (different internal type representation), but a short comment would prevent future confusion since the non-llgo setInterfaceMethods uses it to control export/unexport handling.

[fennoai]

Removing //go:build !llgo means tests calling reflectx.NewMethodSet (e.g. TestIntMethodOf, TestSliceMethodOf, TestStructMethodOf) will panic under llgo because newMethodSet always panics. Consider splitting llgo-compatible tests into a separate file or adding t.Skip guards for the NewMethodSet-dependent tests until that path is implemented.