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")