From ece5c870284964be6f781f140e61b593d7810653 Mon Sep 17 00:00:00 2001 From: Jackie Li Date: Mon, 26 Oct 2026 07:41:27 +0000 Subject: [PATCH] fix: match bound method values by package path, not bare type name ID, IDTarget and RenderTarget.Is matched a bound method value (p.List) to its page by the receiver's bare type name, dropping the package. With same-named page types in two packages, as with a Page type per package, a bound value errored as ambiguous with no current page, and silently took the current page's id when that was the other package's type. The htmx and method render targets had the same confusion in Is. boundReceiverFromFuncName now keeps the package path from the function name, unescaping its last element ("yaml%2ev3"), and methodInfo.receiverIs matches bound values on package path and type name, so they resolve as exactly as method expressions. A generic value receiver "Gen[...].M-fm" now parses to "Gen[...]" rather than "Gen[". Not-found errors name the type by package path for both forms. A function counted as a bound method value whenever its name contained "-fm", so a standalone function in a package path such as "go-fmt..." failed. Only the "-fm" suffix counts now. Fixes #39 Claude-Session: https://claude.ai/code/session_01NpjJeP32ycvAHNFXRgTviG --- htmx.go | 5 +- id_bound_cross_package_test.go | 137 +++++++++++++++++++++++++++++++++ id_coverage_test.go | 20 +++-- id_for.go | 25 ++---- internal/fn-fmt/badge.go | 19 +++++ internal/fn-fmt/badge_test.go | 17 ++++ reflect.go | 108 +++++++++++++++++--------- reflect_test.go | 57 ++++++++++---- render_target.go | 27 +------ 9 files changed, 305 insertions(+), 110 deletions(-) create mode 100644 id_bound_cross_package_test.go create mode 100644 internal/fn-fmt/badge.go create mode 100644 internal/fn-fmt/badge_test.go 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.