diff --git a/htmx.go b/htmx.go index c1b4702..fd87bfe 100644 --- a/htmx.go +++ b/htmx.go @@ -248,10 +248,7 @@ func (t *htmxUnmatchedTarget) Is(component any) bool { // pageNodeHasType reports whether pn's value has the receiver type info // names. func pageNodeHasType(pn *PageNode, info *methodInfo) bool { - if info.isBound { - return sameTypeName(pn.Value.Type(), info.receiverTypeName) - } - return pointerType(pn.Value.Type()) == pointerType(info.receiverType) + return info.receiverIs(pn.Value.Type()) } func (t *htmxUnmatchedTarget) isFunction(info *methodInfo) bool { diff --git a/id_bound_cross_package_test.go b/id_bound_cross_package_test.go new file mode 100644 index 0000000..852fd83 --- /dev/null +++ b/id_bound_cross_package_test.go @@ -0,0 +1,137 @@ +package structpages + +import ( + "context" + "reflect" + "testing" + + fnfmt "github.com/jackielii/structpages/internal/fn-fmt" + "github.com/jackielii/structpages/internal/idconflicta" + "github.com/jackielii/structpages/internal/idconflictb" +) + +// widgetNodes returns the idConflictRoot tree and its two Widget nodes, +// one per package. +func widgetNodes(t *testing.T) (pc *parseContext, a, b *PageNode) { + t.Helper() + pc, err := parsePageTree("/", &idConflictRoot{}) + if err != nil { + t.Fatalf("parsePageTree: %v", err) + } + for n := range pc.root.All() { + switch pointerType(n.Value.Type()) { + case reflect.TypeFor[*idconflicta.Widget](): + a = n + case reflect.TypeFor[*idconflictb.Widget](): + b = n + } + } + if a == nil || b == nil { + t.Fatal("Widget nodes not found") + } + return pc, a, b +} + +// A bound method value (b.List) names its receiver's package as well as +// its type, so it resolves as exactly as the method expression: never to +// a same-named type from another package (#39). +func TestID_boundMethodValueCrossPackage(t *testing.T) { + pc, aNode, bNode := widgetNodes(t) + base := pcCtx.WithValue(context.Background(), pc) + var a idconflicta.Widget + var b idconflictb.Widget + + for _, fn := range []struct { + name string + id func(context.Context, any) (string, error) + }{ + {"ID", ID}, + {"IDTarget", IDTarget}, + } { + wantA, err := fn.id(base, idconflicta.Widget.List) + if err != nil { + t.Fatalf("%s(idconflicta.Widget.List): %v", fn.name, err) + } + wantB, err := fn.id(base, idconflictb.Widget.List) + if err != nil { + t.Fatalf("%s(idconflictb.Widget.List): %v", fn.name, err) + } + for _, cur := range []struct { + name string + page *PageNode + }{ + {"no current page", nil}, + {"current page idconflicta.Widget", aNode}, + {"current page idconflictb.Widget", bNode}, + } { + ctx := base + if cur.page != nil { + ctx = currentPageCtx.WithValue(base, cur.page) + } + if got, err := fn.id(ctx, b.List); err != nil || got != wantB { + t.Errorf("%s(b.List), %s: got %q, %v; want %q", fn.name, cur.name, got, err, wantB) + } + if got, err := fn.id(ctx, a.List); err != nil || got != wantA { + t.Errorf("%s(a.List), %s: got %q, %v; want %q", fn.name, cur.name, got, err, wantA) + } + } + } +} + +// A request for a page's own component Is that component's bound value, +// and not the bound value of a same-named type from another package. +func TestMethodRenderTarget_boundMethodValueCrossPackage(t *testing.T) { + m, ok := reflect.TypeFor[idconflicta.Widget]().MethodByName("List") + if !ok { + t.Fatal("idconflicta.Widget.List not found") + } + target := newMethodRenderTarget("List", &m) + var a idconflicta.Widget + var b idconflictb.Widget + if !target.Is(a.List) { + t.Error("Is(a.List) = false, want true") + } + if target.Is(b.List) { + t.Error("Is(b.List) = true, want false: b is idconflictb.Widget") + } +} + +// With htmx, a request served by one Widget page that targets the other +// package's Widget.List region Is that region's bound value, and not the +// served page's own. +func TestHTMXUnmatchedTarget_boundMethodValueCrossPackage(t *testing.T) { + pc, aNode, bNode := widgetNodes(t) + var a idconflicta.Widget + var b idconflictb.Widget + for _, tt := range []struct { + name string + served, want *PageNode + is, isNot any + }{ + {"served a, target b", aNode, bNode, b.List, a.List}, + {"served b, target a", bNode, aNode, a.List, b.List}, + } { + t.Run(tt.name, func(t *testing.T) { + target := newHTMXUnmatchedTarget(pc.componentID(tt.want, "List", true), tt.served, pc) + if !target.Is(tt.is) { + t.Error("Is(other package's Widget.List) = false, want true") + } + if target.Is(tt.isNot) { + t.Error("Is(served page's own Widget.List) = true, want false") + } + }) + } +} + +// A bound method value's function name ends in "-fm"; a standalone +// function in a package whose path merely contains "-fm" is not one. +func TestID_functionInPackagePathWithFm(t *testing.T) { + pc, err := parsePageTree("/", &idConflictRoot{}) + if err != nil { + t.Fatalf("parsePageTree: %v", err) + } + ctx := pcCtx.WithValue(context.Background(), pc) + if got, err := ID(ctx, fnfmt.Badge); err != nil || got != "fn-fmt-badge" { + t.Errorf("ID(fnfmt.Badge) = %q, %v; want %q", got, err, "fn-fmt-badge") + } +} diff --git a/id_coverage_test.go b/id_coverage_test.go index eb3924f..2876223 100644 --- a/id_coverage_test.go +++ b/id_coverage_test.go @@ -4,8 +4,6 @@ import ( "context" "strings" "testing" - - "github.com/jackielii/structpages/internal/idconflicta" ) // A singly-mounted tree for exercising bound-method-value resolution and @@ -97,23 +95,23 @@ func TestID_ChainEdgeErrors(t *testing.T) { } } -// TestID_BoundMethodAmbiguous covers the isBound name-matching path when the -// same type name is mounted twice (the two cross-package Widget types from -// idConflictRoot in id_cross_package_test.go): a bound method value resolves -// by type name, matches both, and errors with both distinct ids listed. +// TestID_BoundMethodAmbiguous covers the isBound path when one type is +// mounted under two parents (ambBRoot in id_ambiguity_test.go): a bound +// method value matches both mounts, and errors with both distinct ids +// listed, as the method expression does. func TestID_BoundMethodAmbiguous(t *testing.T) { - pc, err := parsePageTree("/", &idConflictRoot{}) + pc, err := parsePageTree("/", &ambBRoot{}) if err != nil { t.Fatalf("parsePageTree: %v", err) } ctx := pcCtx.WithValue(context.Background(), pc) - var inst idconflicta.Widget // bound value; List is a bound method value - _, err = ID(ctx, inst.List) + var inst ambSameName // bound value; Overlays is a bound method value + _, err = ID(ctx, inst.Overlays) if err == nil { - t.Fatal("expected ambiguity error for bound method on a doubly-mounted type name") + t.Fatal("expected ambiguity error for bound method on a doubly-mounted type") } - for _, want := range []string{"a-widget-list", "b-widget-list"} { + for _, want := range []string{"foundations-entry-detail-overlays", "components-entry-detail-overlays"} { if !strings.Contains(err.Error(), want) { t.Errorf("error %q does not contain %q", err.Error(), want) } diff --git a/id_for.go b/id_for.go index 66dba92..1794841 100644 --- a/id_for.go +++ b/id_for.go @@ -268,10 +268,7 @@ func resolvePageForMethod(pc *parseContext, currentPage *PageNode, info *methodI matches := pc.collectPageNodesForMethod(info) switch len(matches) { case 0: - if info.isBound { - return nil, fmt.Errorf("no page node found with type name %q", info.receiverTypeName) - } - return nil, fmt.Errorf("no page node found for type %s", pointerType(info.receiverType).String()) + return nil, fmt.Errorf("no page node found for type %s", info.receiverString()) case 1: return matches[0], nil } @@ -326,15 +323,13 @@ func (p *parseContext) collectPageNodesForMethod(info *methodInfo) []*PageNode { var out []*PageNode for node := range p.root.All() { nodeType := node.Value.Type() + if !info.receiverIs(nodeType) { + continue + } if info.isBound { - if !sameTypeName(nodeType, info.receiverTypeName) { - continue - } if _, found := nodeType.MethodByName(info.methodName); !found { continue } - } else if pointerType(nodeType) != pointerType(info.receiverType) { - continue } out = append(out, node) } @@ -342,22 +337,16 @@ func (p *parseContext) collectPageNodesForMethod(info *methodInfo) []*PageNode { } // pageNodeMatchesMethod reports whether pn's value type is the -// receiver type for info (either by reflect.Type or by type name -// for bound method values). +// receiver type for info and has its method. func pageNodeMatchesMethod(pn *PageNode, info *methodInfo) bool { if pn == nil { return false } nodeType := pn.Value.Type() - if info.isBound { - if !sameTypeName(nodeType, info.receiverTypeName) { - return false - } - } else if pointerType(nodeType) != pointerType(info.receiverType) { + if !info.receiverIs(nodeType) { return false } - // Confirm the method exists on this type — guards against - // matching a same-named type that doesn't have the method. + // Confirm the method exists on this type. _, found := pointerType(nodeType).MethodByName(info.methodName) return found } diff --git a/internal/fn-fmt/badge.go b/internal/fn-fmt/badge.go new file mode 100644 index 0000000..dbeb293 --- /dev/null +++ b/internal/fn-fmt/badge.go @@ -0,0 +1,19 @@ +// Package fnfmt provides a standalone function component in a package +// whose import path contains "-fm", the suffix the Go compiler gives bound +// method values, to test that its function is not taken for one. +package fnfmt + +import ( + "context" + "io" +) + +type comp struct{ s string } + +func (c comp) Render(_ context.Context, w io.Writer) error { + _, err := io.WriteString(w, c.s) + return err +} + +// Badge is a standalone function component. +func Badge() comp { return comp{s: "fnfmt.Badge"} } diff --git a/internal/fn-fmt/badge_test.go b/internal/fn-fmt/badge_test.go new file mode 100644 index 0000000..51ebdbc --- /dev/null +++ b/internal/fn-fmt/badge_test.go @@ -0,0 +1,17 @@ +package fnfmt + +import ( + "context" + "strings" + "testing" +) + +func TestBadgeRenders(t *testing.T) { + var b strings.Builder + if err := Badge().Render(context.Background(), &b); err != nil { + t.Fatalf("Render: %v", err) + } + if got := b.String(); got != "fnfmt.Badge" { + t.Errorf("rendered %q, want %q", got, "fnfmt.Badge") + } +} diff --git a/reflect.go b/reflect.go index 90a2daf..838248a 100644 --- a/reflect.go +++ b/reflect.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "net/url" "reflect" "runtime" "strings" @@ -15,7 +16,8 @@ import ( type methodInfo struct { methodName string receiverType reflect.Type // nil for bound methods and standalone functions - receiverTypeName string // for bound methods + receiverPkgPath string // for bound methods: the receiver type's package path + receiverTypeName string // for bound methods: the receiver type's name, "T[...]" if generic packageName string // short package name, for standalone functions isBound bool isFunction bool // true for standalone functions (not methods) @@ -60,21 +62,24 @@ func extractMethodInfo(methodExpr any) (*methodInfo, error) { funcType := v.Type() // Check if this is a bound method (instance.Method) - // Bound methods have "-fm" suffix in the name (receiver already bound) + // Bound methods have "-fm" suffix in the name (receiver already bound); + // a package path may contain "-fm" too, so only the suffix counts // The number of input parameters varies based on the method's arguments // Method pattern in the name: "package.(*Type).Method-fm" - isBound := strings.Contains(fullName, "-fm") + isBound := strings.HasSuffix(fullName, "-fm") var info *methodInfo if isBound { - // Extract receiver type name from function name - typeName := extractReceiverTypeNameFromFuncName(fullName) + // A bound method value's func is a closure over its receiver, which + // reflect can't reach, so the receiver type comes from its name + pkgPath, typeName := boundReceiverFromFuncName(fullName) if typeName == "" { return nil, fmt.Errorf("cannot extract receiver type from bound method: %s", fullName) } info = &methodInfo{ methodName: methodName, + receiverPkgPath: pkgPath, receiverTypeName: typeName, isBound: true, isFunction: false, @@ -202,43 +207,80 @@ func isMethodPattern(fullName string) bool { return strings.Contains(remainder, ".") } -// extractReceiverTypeNameFromFuncName extracts the receiver type name from a function name. +// boundReceiverFromFuncName extracts the receiver type's package path and +// name from a bound method value's function name. // Examples: -// - "github.com/user/pkg.(*TypeName).Method-fm" -> "TypeName" -// - "github.com/user/pkg.TypeName.Method-fm" -> "TypeName" -// - "main.(*MyType).Method-fm" -> "MyType" -func extractReceiverTypeNameFromFuncName(fullName string) string { - // Find the last package separator +// - "github.com/user/pkg.(*TypeName).Method-fm" -> "github.com/user/pkg", "TypeName" +// - "github.com/user/pkg.TypeName.Method-fm" -> "github.com/user/pkg", "TypeName" +// - "github.com/user/pkg.Gen[...].Method-fm" -> "github.com/user/pkg", "Gen[...]" +// - "main.(*MyType).Method-fm" -> "main", "MyType" +// +// Symbol names escape the package path's last element: a "." there is +// written "%2e" ("gopkg.in/yaml%2ev3"), so the path is unescaped to +// compare with reflect.Type.PkgPath. +func boundReceiverFromFuncName(fullName string) (pkgPath, typeName string) { + // The package path ends at the first dot after its last slash lastSlash := strings.LastIndex(fullName, "/") - remainder := fullName - if lastSlash != -1 { - remainder = fullName[lastSlash+1:] - } - - // Look for the pattern: "package.(*Type).Method" or "package.Type.Method" - // First, find the package name and skip it - dotIdx := strings.Index(remainder, ".") + dotIdx := strings.Index(fullName[lastSlash+1:], ".") if dotIdx == -1 { - return "" + return "", "" + } + dotIdx += lastSlash + 1 + pkgPath, err := url.PathUnescape(fullName[:dotIdx]) + if err != nil { + return "", "" } - remainder = remainder[dotIdx+1:] + remainder := fullName[dotIdx+1:] - // Check for pointer receiver: (*Type) + // Pointer receiver: (*Type).Method if strings.HasPrefix(remainder, "(*") { closeParenIdx := strings.Index(remainder, ")") if closeParenIdx == -1 { - return "" + return "", "" } - return remainder[2:closeParenIdx] + return pkgPath, remainder[2:closeParenIdx] } - // Value receiver: Type.Method - dotIdx = strings.Index(remainder, ".") + // Value receiver: Type.Method, or Type[...].Method for a generic type, + // whose type arguments may hold dots + end := 0 + if i := strings.IndexAny(remainder, ".["); i != -1 && remainder[i] == '[' { + closeIdx := strings.Index(remainder, "]") + if closeIdx == -1 { + return "", "" + } + end = closeIdx + 1 + } + dotIdx = strings.Index(remainder[end:], ".") if dotIdx == -1 { - return "" + return "", "" } + return pkgPath, remainder[:end+dotIdx] +} - return remainder[:dotIdx] +// receiverIs reports whether t, or the type t points to, is the receiver +// type of the method info describes. A bound method value names its +// receiver by package path and type name, ignoring type arguments: its +// name says "T[...]" where reflect says "T[int]". A promoted method's +// receiver is the embedded type that declares it, not the embedding one. +func (info *methodInfo) receiverIs(t reflect.Type) bool { + if t.Kind() == reflect.Pointer { + t = t.Elem() + } + if info.isBound { + return t.PkgPath() == info.receiverPkgPath && + genericBase(t.Name()) == genericBase(info.receiverTypeName) + } + return pointerType(t) == pointerType(info.receiverType) +} + +// receiverString names the receiver type by package path for error +// messages, the same way for bound values and method expressions. +func (info *methodInfo) receiverString() string { + if info.isBound { + return info.receiverPkgPath + "." + info.receiverTypeName + } + return info.receiverType.PkgPath() + "." + info.receiverType.Name() } // pointerType normalizes a type to its pointer variant. @@ -302,16 +344,6 @@ func formatMethod(method *reflect.Method) string { return fmt.Sprintf("%s.%s", receiver.String(), method.Name) } -// sameTypeName reports whether t's name is name, ignoring type arguments: -// a bound method value of a generic type names its receiver "T[...]", -// while reflect names it "T[int]". -func sameTypeName(t reflect.Type, name string) bool { - if t.Kind() == reflect.Pointer { - t = t.Elem() - } - return genericBase(t.Name()) == genericBase(name) -} - func genericBase(name string) string { if i := strings.IndexByte(name, '['); i >= 0 { return name[:i] diff --git a/reflect_test.go b/reflect_test.go index 7701ded..00b9ff6 100644 --- a/reflect_test.go +++ b/reflect_test.go @@ -5,61 +5,88 @@ import ( "testing" ) -// Test extractReceiverTypeNameFromFuncName with various edge cases -func TestExtractReceiverTypeNameFromFuncName(t *testing.T) { +// Test boundReceiverFromFuncName with various edge cases +func TestBoundReceiverFromFuncName(t *testing.T) { tests := []struct { name string funcName string - expected string + pkgPath string + typeName string }{ { name: "pointer receiver with full path", funcName: "github.com/user/pkg.(*TypeName).Method-fm", - expected: "TypeName", + pkgPath: "github.com/user/pkg", + typeName: "TypeName", }, { name: "value receiver with full path", funcName: "github.com/user/pkg.TypeName.Method-fm", - expected: "TypeName", + pkgPath: "github.com/user/pkg", + typeName: "TypeName", + }, + { + name: "generic receiver", + funcName: "github.com/user/pkg.Gen[...].Method-fm", + pkgPath: "github.com/user/pkg", + typeName: "Gen[...]", + }, + { + name: "dot in the last path element is escaped", + funcName: "gopkg.in/yaml%2ev3.(*Node).Method-fm", + pkgPath: "gopkg.in/yaml.v3", + typeName: "Node", + }, + { + name: "dot in an earlier path element", + funcName: "example.com/a.b/pkg.T.Method-fm", + pkgPath: "example.com/a.b/pkg", + typeName: "T", }, { name: "pointer receiver in main", funcName: "main.(*MyType).Method-fm", - expected: "MyType", + pkgPath: "main", + typeName: "MyType", }, { name: "value receiver in main", funcName: "main.MyType.Method-fm", - expected: "MyType", + pkgPath: "main", + typeName: "MyType", }, { name: "no dots in name", funcName: "NoDotsAtAll", - expected: "", + }, + { + name: "no dot after the last slash", + funcName: "example.com/nodot", }, { name: "pointer receiver without closing paren", funcName: "main.(*BrokenType.Method", - expected: "", }, { name: "value receiver without method", funcName: "main.TypeNameOnly", - expected: "", }, { name: "only package name", funcName: "main.", - expected: "", + }, + { + name: "malformed escape", + funcName: "example.com/bad%zz.T.Method-fm", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := extractReceiverTypeNameFromFuncName(tt.funcName) - if result != tt.expected { - t.Errorf("extractReceiverTypeNameFromFuncName(%q) = %q, want %q", - tt.funcName, result, tt.expected) + pkgPath, typeName := boundReceiverFromFuncName(tt.funcName) + if typeName != tt.typeName || (typeName != "" && pkgPath != tt.pkgPath) { + t.Errorf("boundReceiverFromFuncName(%q) = %q, %q, want %q, %q", + tt.funcName, pkgPath, typeName, tt.pkgPath, tt.typeName) } }) } diff --git a/render_target.go b/render_target.go index 5d5ca0c..91888e9 100644 --- a/render_target.go +++ b/render_target.go @@ -95,31 +95,10 @@ func (mrt *methodRenderTarget) Is(method any) bool { } // For methods, compare both name and receiver type - var selectedReceiverType reflect.Type - if mrt.method.Type.NumIn() > 0 { - selectedReceiverType = mrt.method.Type.In(0) - if selectedReceiverType.Kind() == reflect.Pointer { - selectedReceiverType = selectedReceiverType.Elem() - } - } - - // For bound methods, compare by receiver type name - if info.isBound { - if selectedReceiverType == nil { - return false - } - return mrt.method.Name == info.methodName && - sameTypeName(selectedReceiverType, info.receiverTypeName) - } - - // For unbound methods, compare by receiver type - actualType := info.receiverType - if actualType.Kind() == reflect.Pointer { - actualType = actualType.Elem() + if mrt.method.Type.NumIn() == 0 { + return false } - - return mrt.method.Name == info.methodName && - selectedReceiverType == actualType + return mrt.method.Name == info.methodName && info.receiverIs(mrt.method.Type.In(0)) } // newMethodRenderTarget creates a RenderTarget for a method component.