From 272033e8832726105b8e19737d6a0df3c91ee170 Mon Sep 17 00:00:00 2001 From: Florian Kinder Date: Tue, 7 Oct 2025 00:18:44 +0900 Subject: [PATCH] Fix AST parsing issues with function scope, generics, and test packages This commit addresses several parsing issues that caused errors when analyzing large codebases: 1. Function-scoped declarations: The visitor now stops descending into function bodies after emitting FuncDecl nodes. This prevents function-local variables from being incorrectly tracked as file-level declarations, which was causing "duplicate declaration" errors. 2. File emission order: Fixed the AST visitor to properly track package context when emitting File nodes. Previously, all files were emitted at once when processing a Package node, causing ImportSpec nodes to be assigned to the wrong file. Now File nodes are emitted as they're encountered during the walk, using the stored package context to look up filenames. 3. Blank identifier: Added logic to skip tracking "_" declarations since the blank identifier can legitimately appear multiple times in a file (e.g., for compile-time interface checks). 4. External test packages: Added filtering to skip packages with names ending in "_test" since these are external test packages that cannot be imported and won't cause import cycles. 5. Generic type receivers: Added support for extracting receiver names from generic types like Foo[T] and *Foo[T]. The new extractTypeName function handles ast.IndexExpr and ast.IndexListExpr for generic types with one or multiple type parameters. 6. Error messages: Enhanced error messages to include file paths for easier debugging when duplicate declarations are detected. These fixes allow the tool to successfully analyze complex codebases with generics, extensive test suites, and varied declaration patterns. --- cmd/goimportcycle/main.go | 4 ++ internal/ast/dependency_visitor.go | 99 ++++++++++++++++++++++-------- internal/ast/primitive_builder.go | 12 ++-- 3 files changed, 87 insertions(+), 28 deletions(-) diff --git a/cmd/goimportcycle/main.go b/cmd/goimportcycle/main.go index 244f628..2d24e69 100644 --- a/cmd/goimportcycle/main.go +++ b/cmd/goimportcycle/main.go @@ -79,6 +79,10 @@ func parseFiles( } for _, pkg := range pkgs { + // Skip external test packages (they can't be imported, so won't cause cycles) + if strings.HasSuffix(pkg.Name, "_test") { + continue + } ast.Walk(depVis, pkg) } diff --git a/internal/ast/dependency_visitor.go b/internal/ast/dependency_visitor.go index c202b5f..07fe15e 100644 --- a/internal/ast/dependency_visitor.go +++ b/internal/ast/dependency_visitor.go @@ -48,6 +48,10 @@ type DependencyVisitor struct { out chan<- ast.Node fileImports map[string]struct{} + + // Track current package context for filename lookup + currentPackage *ast.Package + currentDirName string } func NewDependencyVisitor() (*DependencyVisitor, <-chan ast.Node) { @@ -66,12 +70,15 @@ func (v *DependencyVisitor) Visit(node ast.Node) ast.Visitor { case *ast.File: v.fileImports = make(map[string]struct{}) + v.emitFile(node) case *ast.ImportSpec: v.emitImportSpec(node) case *ast.FuncDecl: v.emitFuncDecl(node) + // Don't descend into function bodies to avoid collecting function-scoped declarations + return nil case *ast.GenDecl: switch node.Tok { @@ -109,29 +116,56 @@ func (v *DependencyVisitor) Visit(node ast.Node) ast.Visitor { } func (v *DependencyVisitor) emitPackageAndFiles(node *ast.Package) { - var setImportPathAndEmitPackage bool + // Get directory name from first file var dirName string - for filename, astFile := range node.Files { + for filename := range node.Files { absPath, err := filepath.Abs(filename) if err != nil { continue } - if !setImportPathAndEmitPackage { - dirName, _ = filepath.Split(absPath) - dirName = strings.TrimRight(dirName, "/") - v.out <- &Package{ - Package: node, - DirName: dirName, - } - setImportPathAndEmitPackage = true - } + dirName, _ = filepath.Split(absPath) + dirName = strings.TrimRight(dirName, "/") + break + } + + // Store package context for later file emission + v.currentPackage = node + v.currentDirName = dirName + + v.out <- &Package{ + Package: node, + DirName: dirName, + } +} + +func (v *DependencyVisitor) emitFile(node *ast.File) { + if v.currentPackage == nil { + return + } - v.out <- &File{ - File: astFile, - AbsPath: absPath, - DirName: dirName, + // Find the filename for this ast.File in the package's Files map + var filename string + for fn, astFile := range v.currentPackage.Files { + if astFile == node { + filename = fn + break } } + + if filename == "" { + return + } + + absPath, err := filepath.Abs(filename) + if err != nil { + return + } + + v.out <- &File{ + File: node, + AbsPath: absPath, + DirName: v.currentDirName, + } } func (v *DependencyVisitor) emitImportSpec(node *ast.ImportSpec) { @@ -169,18 +203,18 @@ func (v *DependencyVisitor) emitFuncDecl(node *ast.FuncDecl) { if node.Recv != nil { // TODO: don't emit receiver functions/methods? we don't need them var typName string - switch expr := node.Recv.List[0].Type.(type) { + recvType := node.Recv.List[0].Type + switch expr := recvType.(type) { case *ast.Ident: typName = expr.String() case *ast.StarExpr: - if expr.X == nil { - // panic error, invalid receiver method - } - ident, ok := expr.X.(*ast.Ident) - if !ok { - // panic error, invalid receiver method - } - typName = ident.String() + typName = extractTypeName(expr.X) + case *ast.IndexExpr: + // Generic type with one parameter: Foo[T] + typName = extractTypeName(expr) + case *ast.IndexListExpr: + // Generic type with multiple parameters: Foo[T, U] + typName = extractTypeName(expr) default: // panic error, invalid receiver method } @@ -198,3 +232,20 @@ func (v *DependencyVisitor) emitFuncDecl(node *ast.FuncDecl) { func (v *DependencyVisitor) Close() { close(v.out) } + +// extractTypeName extracts the type name from an expression, handling generics +func extractTypeName(expr ast.Expr) string { + switch e := expr.(type) { + case *ast.Ident: + // Simple type: Foo + return e.String() + case *ast.IndexExpr: + // Generic type with one parameter: Foo[T] + return extractTypeName(e.X) + case *ast.IndexListExpr: + // Generic type with multiple parameters: Foo[T, U] + return extractTypeName(e.X) + default: + return "" + } +} diff --git a/internal/ast/primitive_builder.go b/internal/ast/primitive_builder.go index e6213d0..926b839 100644 --- a/internal/ast/primitive_builder.go +++ b/internal/ast/primitive_builder.go @@ -217,7 +217,7 @@ func (builder *PrimitiveBuilder) addImport(node *ImportSpec) error { } impUID := imp.UID() if _, ok := builder.curFile.Imports[impUID]; ok { - return fmt.Errorf("add import: duplicate import: %s \"%s\"", node.Name.String(), node.Path.Value) + return fmt.Errorf("add import: duplicate import: %s \"%s\" in file %s", node.Name.String(), node.Path.Value, builder.curFile.AbsPath) } // if the package exists, use it, otherwise use a stub @@ -250,7 +250,7 @@ func (builder *PrimitiveBuilder) addFuncDecl(node *FuncDecl) error { } declUID := node.QualifiedName if _, ok := builder.curFile.Decls[declUID]; ok { - return fmt.Errorf("add func decl: duplicate declaration: %s", node.QualifiedName) + return fmt.Errorf("add func decl: duplicate declaration: %s in file %s", node.QualifiedName, builder.curFile.AbsPath) } // TODO: receiver methods should never be received and should be skipped var receiverDecl *internal.Decl @@ -285,7 +285,7 @@ func (builder *PrimitiveBuilder) addGenDecl(node *ast.GenDecl) error { return errors.New("add gen decl: invalid declaration") } if _, ok := builder.curFile.Decls[spec.Name.String()]; ok { - return errors.New("add gen decl: duplicate declaration") + return fmt.Errorf("add gen decl: duplicate declaration '%s' in file %s", spec.Name.String(), builder.curFile.AbsPath) } if spec.Name.String() == "" { return errors.New("add gen decl: invalid name") @@ -303,8 +303,12 @@ func (builder *PrimitiveBuilder) addGenDecl(node *ast.GenDecl) error { return errors.New("add gen decl: invalid declaration") } for _, name := range spec.Names { + // Skip blank identifier - it can be used multiple times + if name.String() == "_" { + continue + } if _, ok := builder.curFile.Decls[name.String()]; ok { - return errors.New("add gen decl: duplicate declaration") + return fmt.Errorf("add gen decl: duplicate declaration '%s' in file %s", name.String(), builder.curFile.AbsPath) } if name.String() == "" { return errors.New("add gen decl: invalid constant or variable name")