Skip to content

Fix AST parsing issues with function scope, generics, and test packages #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fank wants to merge 1 commit into from
Closed

Fix AST parsing issues with function scope, generics, and test packages #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cmd/goimportcycle/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
99 changes: 75 additions & 24 deletions internal/ast/dependency_visitor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
}
Expand All @@ -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 ""
}
}
12 changes: 8 additions & 4 deletions internal/ast/primitive_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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")
Expand All @@ -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")
Expand Down