diff --git a/internal/engine_test.go b/internal/engine_test.go index 7d72493..b84b003 100644 --- a/internal/engine_test.go +++ b/internal/engine_test.go @@ -304,7 +304,7 @@ func TestRulesFireOnTestdata(t *testing.T) { }{ {"simplify-slice-range", "slice0.gno"}, {"unnecessary-type-conversion", "coversion/conv0.gno"}, - {"cycle-detection", "cycle/types.gno"}, + {"cycle-detection", "cycle0.gno"}, {"emit-format", "emit/emit1.gno"}, {"useless-break", "break/break1.gno"}, {"early-return-opportunity", "early_return/a0.gno"}, diff --git a/internal/lints/detect_cycle_test.go b/internal/lints/detect_cycle_test.go index a066f9c..96217a3 100644 --- a/internal/lints/detect_cycle_test.go +++ b/internal/lints/detect_cycle_test.go @@ -2,8 +2,6 @@ package lints import ( "go/ast" - "go/parser" - "go/token" "testing" ) @@ -30,19 +28,6 @@ func TestDetectCycle(t *testing.T) { src := ` package main -type A struct { - B *B -} - -type B struct { - A *A -} - -var ( - x = &y - y = &x -) - func a() { b() } @@ -52,27 +37,183 @@ func b() { } func outer() { - var inner func() - inner = func() { - outer() - } - inner() + var inner func() + inner = func() { + outer() + } + inner() }` - fset := token.NewFileSet() - f, err := parser.ParseFile(fset, "", src, 0) - if err != nil { - t.Fatalf("failed to parse source: %v", err) + cycles := detectCyclesInSource(t, src) + // Expected: [a b a] and [outer outer$anon... outer]. + if len(cycles) != 2 { + t.Errorf("expected 2 cycles, got %d: %v", len(cycles), cycles) } +} - cycle := newCycle() - result := cycle.detectCycles(f) - if len(result) != 4 { - // [B A B] - // [B A B] - // [x y x] - // [b a b] - // [outer outer$anon
outer] - t.Errorf("unexpected result: %v", result) +// TestDetectCycle_NoFalsePositives covers patterns that the rule used +// to flag (type-level cycles via indirection, pointer init cycles) +// but that are valid Go and must NOT be reported. The gno-ibc Field/ +// Schema reproducer is the lead case. +func TestDetectCycle_NoFalsePositives(t *testing.T) { + t.Parallel() + cases := []struct { + name string + src string + }{ + { + name: "gno-ibc Field/Schema reproducer", + src: `package main +type Type int +type Field struct { + Type Type + Sub *Schema + Elem *Field +} +type Schema struct { + Fields []Field +}`, + }, + { + name: "self-pointer linked list", + src: `package main +type Node struct { Next *Node }`, + }, + { + name: "recursive map values", + src: `package main +type Tree struct { Children map[string]*Tree }`, + }, + { + name: "self-typed interface method", + src: `package main +type I interface { F() I }`, + }, + { + name: "function-typed field", + src: `package main +type Lazy struct { Eval func() Lazy }`, + }, + { + name: "embedded mutual", + src: `package main +type Outer struct { *Inner } +type Inner struct { *Outer }`, + }, + { + name: "pointer var cycle", + src: `package main +var x = &y +var y = &x`, + }, + { + name: "slice of pointer to self", + src: `package main +type T []*T`, + }, + { + name: "type alias chain", + src: `package main +type A = B +type B = int`, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + cycles := detectCyclesInSource(t, tc.src) + if len(cycles) != 0 { + t.Errorf("expected no cycles, got %d: %v", len(cycles), cycles) + } + }) + } +} + +// TestDetectCycle_MutualFunctions guards the latent-bug fix: before +// the fix, analyzeFuncDecl filtered call-site idents to self-only, +// so `a()` calling `b()` and back never registered. +func TestDetectCycle_MutualFunctions(t *testing.T) { + t.Parallel() + src := `package main +func a() { b() } +func b() { a() }` + cycles := detectCyclesInSource(t, src) + if len(cycles) != 1 { + t.Fatalf("expected 1 cycle, got %d: %v", len(cycles), cycles) + } +} + +// TestDetectCycle_Methods covers SelectorExpr-based recursion. +func TestDetectCycle_Methods(t *testing.T) { + t.Parallel() + cases := []struct { + name string + src string + wantCount int + }{ + { + name: "same-type mutual via pointer receiver", + src: `package main +type T struct{} +func (r *T) foo() { r.bar() } +func (r *T) bar() { r.foo() }`, + wantCount: 1, + }, + { + name: "same-type three-method chain", + src: `package main +type T struct{} +func (r *T) a() { r.b() } +func (r *T) b() { r.c() } +func (r *T) c() { r.a() }`, + wantCount: 1, + }, + { + // Direct self-recursion is not flagged — matches the + // existing free-function behavior. + name: "same-type self-recursion via receiver", + src: `package main +type T struct{} +func (r *T) foo() { r.foo() }`, + wantCount: 0, + }, + { + name: "cross-type method recursion (known limitation)", + src: `package main +type A struct{ b *B } +type B struct{ a *A } +func (a *A) f() { a.b.g() } +func (b *B) g() { b.a.f() }`, + // Resolving b.a.f() requires go/types; out of scope. + wantCount: 0, + }, + { + name: "anonymous receiver (known limitation)", + src: `package main +type T struct{} +func (T) foo() { /* can't resolve calls without param name */ } +func (T) bar() {}`, + wantCount: 0, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + cycles := detectCyclesInSource(t, tc.src) + if len(cycles) != tc.wantCount { + t.Errorf("expected %d cycles, got %d: %v", tc.wantCount, len(cycles), cycles) + } + }) + } +} + +func detectCyclesInSource(t *testing.T, src string) []string { + t.Helper() + f, _, err := ParseFile("", []byte(src)) + if err != nil { + t.Fatalf("failed to parse source: %v", err) } + return newCycle().detectCycles(f) } diff --git a/internal/lints/detect_cycles.go b/internal/lints/detect_cycles.go index 65d06c0..0fc1593 100644 --- a/internal/lints/detect_cycles.go +++ b/internal/lints/detect_cycles.go @@ -3,6 +3,7 @@ package lints import ( "fmt" "go/ast" + "slices" "github.com/gnolang/tlin/internal/rule" tt "github.com/gnolang/tlin/internal/types" @@ -48,46 +49,97 @@ func newCycle() *cycle { } } +// receiverInfo carries an enclosing method's receiver type and param +// name so SelectorExpr callees of the form `r.method()` can be +// resolved to "TypeName.method". Empty fields mean "no receiver" or +// "anonymous receiver" — in either case selector callees won't be +// resolved without go/types. +type receiverInfo struct { + typeName string + paramName string +} + func (c *cycle) analyzeFuncDecl(fn *ast.FuncDecl) { - name := fn.Name.Name + name, recv := funcDeclNode(fn) c.dependencies[name] = []string{} - - // ignore bodyless function - if fn.Body != nil { - ast.Inspect(fn.Body, func(n ast.Node) bool { - switch x := n.(type) { - case *ast.CallExpr: - if ident, ok := x.Fun.(*ast.Ident); ok { - if ident.Name == name { - c.dependencies[name] = append(c.dependencies[name], ident.Name) - } - } - case *ast.FuncLit: - c.analyzeFuncLit(x, name) - } - return true - }) + if fn.Body == nil { + return } + c.collectCallees(fn.Body, name, recv) } -func (c *cycle) analyzeFuncLit(fn *ast.FuncLit, parentName string) { +func (c *cycle) analyzeFuncLit(fn *ast.FuncLit, parentName string, recv receiverInfo) { anonName := fmt.Sprintf("%s$anon%p", parentName, fn) c.dependencies[anonName] = []string{} + c.collectCallees(fn.Body, anonName, recv) + c.dependencies[parentName] = append(c.dependencies[parentName], anonName) +} - ast.Inspect(fn.Body, func(n ast.Node) bool { +func (c *cycle) collectCallees(body *ast.BlockStmt, owner string, recv receiverInfo) { + ast.Inspect(body, func(n ast.Node) bool { switch x := n.(type) { case *ast.CallExpr: - if ident, ok := x.Fun.(*ast.Ident); ok { - c.dependencies[anonName] = append(c.dependencies[anonName], ident.Name) + if callee := resolveCallee(x.Fun, recv); callee != "" { + c.dependencies[owner] = append(c.dependencies[owner], callee) } case *ast.FuncLit: - c.analyzeFuncLit(x, anonName) + c.analyzeFuncLit(x, owner, recv) + return false } return true }) +} - // add dependency from parent to anonymous function - c.dependencies[parentName] = append(c.dependencies[parentName], anonName) +func resolveCallee(fn ast.Expr, recv receiverInfo) string { + switch e := fn.(type) { + case *ast.Ident: + return e.Name + case *ast.SelectorExpr: + if recv.paramName == "" || recv.typeName == "" { + return "" + } + x, ok := e.X.(*ast.Ident) + if !ok || x.Name != recv.paramName { + return "" + } + return recv.typeName + "." + e.Sel.Name + } + return "" +} + +func funcDeclNode(fn *ast.FuncDecl) (string, receiverInfo) { + name := fn.Name.Name + if fn.Recv == nil || len(fn.Recv.List) == 0 { + return name, receiverInfo{} + } + typeName := receiverTypeName(fn.Recv.List[0].Type) + if typeName == "" { + return name, receiverInfo{} + } + paramName := "" + if names := fn.Recv.List[0].Names; len(names) > 0 { + paramName = names[0].Name + } + return typeName + "." + name, receiverInfo{typeName: typeName, paramName: paramName} +} + +// receiverTypeName extracts T from receiver type expressions like +// T, *T, T[U], *T[U, V]. Returns "" for unsupported shapes. +func receiverTypeName(expr ast.Expr) string { + for { + switch e := expr.(type) { + case *ast.StarExpr: + expr = e.X + case *ast.IndexExpr: + expr = e.X + case *ast.IndexListExpr: + expr = e.X + case *ast.Ident: + return e.Name + default: + return "" + } + } } func (c *cycle) detectCycles(node ast.Node) []string { @@ -95,13 +147,10 @@ func (c *cycle) detectCycles(node ast.Node) []string { switch x := n.(type) { case *ast.FuncDecl: c.analyzeFuncDecl(x) - case *ast.TypeSpec: - c.analyzeTypeSpec(x) - case *ast.ValueSpec: - c.analyzeValueSpec(x) + return false case *ast.FuncLit: - // handle top-level anonymous functions - c.analyzeFuncLit(x, "topLevel") + c.analyzeFuncLit(x, "topLevel", receiverInfo{}) + return false } return true }) @@ -115,32 +164,6 @@ func (c *cycle) detectCycles(node ast.Node) []string { return c.cycles } -func (c *cycle) analyzeTypeSpec(ts *ast.TypeSpec) { - name := ts.Name.Name - c.dependencies[name] = []string{} - - ast.Inspect(ts.Type, func(n ast.Node) bool { - if ident, ok := n.(*ast.Ident); ok { - c.dependencies[name] = append(c.dependencies[name], ident.Name) - } - return true - }) -} - -func (c *cycle) analyzeValueSpec(vs *ast.ValueSpec) { - for i, name := range vs.Names { - c.dependencies[name.Name] = []string{} - if vs.Values != nil && i < len(vs.Values) { - ast.Inspect(vs.Values[i], func(n ast.Node) bool { - if ident, ok := n.(*ast.Ident); ok { - c.dependencies[name.Name] = append(c.dependencies[name.Name], ident.Name) - } - return true - }) - } - } -} - func (c *cycle) dfs(name string) { c.visited[name] = true c.stack = append(c.stack, name) @@ -148,30 +171,13 @@ func (c *cycle) dfs(name string) { for _, dep := range c.dependencies[name] { if !c.visited[dep] { c.dfs(dep) - } else if contains(c.stack, dep) && dep != name { - cycle := append(c.stack[indexOf(c.stack, dep):], dep) - res := fmt.Sprintf("%v", cycle) - c.cycles = append(c.cycles, res) + } else if dep != name { + if i := slices.Index(c.stack, dep); i >= 0 { + cycle := append(c.stack[i:], dep) + c.cycles = append(c.cycles, fmt.Sprintf("%v", cycle)) + } } } c.stack = c.stack[:len(c.stack)-1] } - -func contains(slice []string, item string) bool { - for _, v := range slice { - if v == item { - return true - } - } - return false -} - -func indexOf(slice []string, item string) int { - for i, v := range slice { - if v == item { - return i - } - } - return -1 -} diff --git a/testdata/cycle/types.gno b/testdata/cycle/types.gno deleted file mode 100644 index 5cb1933..0000000 --- a/testdata/cycle/types.gno +++ /dev/null @@ -1,9 +0,0 @@ -package main - -type A struct { - b *B -} - -type B struct { - a *A -}