diff --git a/blast_radius.go b/blast_radius.go index 7c88616..fb75b4e 100644 --- a/blast_radius.go +++ b/blast_radius.go @@ -1377,10 +1377,16 @@ func renderImportersReportCLI(w io.Writer, report scanner.ImportersReport) { renderCoverage(w, report.CoverageStatus, report.CoverageNotes) return } - fmt.Fprintf(w, "No files import %s.\n", report.File) - if strings.EqualFold(filepath.Ext(report.File), ".go") { - fmt.Fprintln(w, " Note: files in the same package never import each other (Go resolves") - fmt.Fprintln(w, " imports at package level), so only cross-package importers appear here.") + if report.Package != "" && report.PackageSiblings > 0 { + // A Go file's importers are the files that import its package, so + // a zero here says nothing about the package's own files: those + // references are not edges in this graph, and the reader has to + // check them by hand. Say so, with the count, instead of printing + // a bare zero that reads as "nothing depends on this". + fmt.Fprintf(w, "No cross-package importers of %s. Same-package references are not modeled (%s in package %s).\n", + report.File, pluralFiles(report.PackageSiblings, "other file"), report.Package) + } else { + fmt.Fprintf(w, "No files import %s.\n", report.File) } // Show scan provenance even for an empty answer, so a partial scan // isn't read as a confident negative. @@ -1390,10 +1396,28 @@ func renderImportersReportCLI(w io.Writer, report scanner.ImportersReport) { renderImportersReport(w, report) } +// importersGranularity names the unit an importer count was measured at when +// it is not the file: Go imports name packages, so "21 files import +// watch/events.go" means 21 files import package watch, and a reader must not +// take the number as evidence that any of them references events.go itself. +func importersGranularity(report scanner.ImportersReport) string { + if report.Package == "" { + return "" + } + return fmt.Sprintf(" (package %s: Go imports name packages, not files)", report.Package) +} + +func pluralFiles(n int, noun string) string { + if n == 1 { + return fmt.Sprintf("%d %s", n, noun) + } + return fmt.Sprintf("%d %ss", n, noun) +} + func renderImportersReport(w io.Writer, report scanner.ImportersReport) { if scanner.CountHubImporters(report.Importers) >= scanner.HubThreshold { fmt.Fprintf(w, "โš ๏ธ HUB FILE: %s\n", report.File) - fmt.Fprintf(w, " Imported by %d files - changes have wide impact!\n", len(report.Importers)) + fmt.Fprintf(w, " Imported by %d files%s - changes have wide impact!\n", len(report.Importers), importersGranularity(report)) fmt.Fprintln(w) fmt.Fprintln(w, " Dependents:") for i, imp := range report.Importers { @@ -1405,7 +1429,7 @@ func renderImportersReport(w io.Writer, report scanner.ImportersReport) { } } else if len(report.Importers) > 0 { fmt.Fprintf(w, "๐Ÿ“ File: %s\n", report.File) - fmt.Fprintf(w, " Imported by %d file(s)\n", len(report.Importers)) + fmt.Fprintf(w, " Imported by %d file(s)%s\n", len(report.Importers), importersGranularity(report)) for _, imp := range report.Importers { fmt.Fprintf(w, " โ€ข %s\n", imp) } @@ -1415,7 +1439,9 @@ func renderImportersReport(w io.Writer, report scanner.ImportersReport) { if len(report.Importers) == 0 { fmt.Fprintf(w, "๐Ÿ“ File: %s\n", report.File) } - fmt.Fprintf(w, " Imports %d hub(s): %s\n", len(report.HubImports), strings.Join(report.HubImports, ", ")) + // Every non-test file of a hub Go package is a hub, so list the + // package once with its file count rather than each file. + fmt.Fprintf(w, " Imports %d hub(s): %s\n", len(report.HubImports), strings.Join(scanner.CollapseGoPackageFiles(report.HubImports), ", ")) } renderCoverage(w, report.CoverageStatus, report.CoverageNotes) @@ -1480,6 +1506,16 @@ func buildImportersReportFromGraph(root, file string, fg *scanner.FileGraph) (sc CoverageNotes: append([]string(nil), fg.Coverage.Notes...), } + if pkg, pkgFiles, ok := fg.GoPackage(file); ok { + report.Package = pkg + report.PackageSiblings = len(pkgFiles) + if !scanner.IsTestFile(file) { + // pkgFiles holds this file too; a _test.go file is in the package + // but never in the index, so every indexed file is a sibling of it. + report.PackageSiblings-- + } + } + for _, imp := range imports { if fg.IsHub(imp) { report.HubImports = append(report.HubImports, imp) diff --git a/cmd/hooks.go b/cmd/hooks.go index 13d8c86..b05c618 100644 --- a/cmd/hooks.go +++ b/cmd/hooks.go @@ -2153,7 +2153,7 @@ func checkFileImportersWithPhase(root, filePath, phase string) error { } } if len(hubImports) > 0 { - fmt.Printf(" Imports %d hub(s): %s\n", len(hubImports), strings.Join(hubImports, ", ")) + fmt.Printf(" Imports %d hub(s): %s\n", len(hubImports), strings.Join(scanner.CollapseGoPackageFiles(hubImports), ", ")) fmt.Println() } diff --git a/collide.go b/collide.go index d1d58ea..e536b13 100644 --- a/collide.go +++ b/collide.go @@ -364,13 +364,11 @@ func collidePackageOf(fg *scanner.FileGraph, path string) (string, bool) { // collidePackageImporterCounts counts, per package, the files that import it. // -// This cannot be read off FileGraph.Importers. BuildFileGraph deliberately -// drops a Go import that resolves to more than one file rather than fanning one -// import into an edge per file (it would inflate every hub count in the -// repository), so a multi-file package has no file-level edges at all and the -// graph's own answer for "who imports scanner/filegraph.go" is a structural -// zero. The raw import strings do carry it, which is why the scan outcome is -// kept. +// It is read off the raw import strings rather than FileGraph.Importers so +// the count is per package, not per file: BuildFileGraph fans one Go import +// out into an edge per file of the imported package, which is right for +// "who imports scanner/filegraph.go" but would make a package's weight here +// depend on how many files it happens to be split into. // // A file importing its own package is not counted: a package is not a second // party to a collision inside itself. Its siblings are counted separately. diff --git a/importers_go_package_test.go b/importers_go_package_test.go new file mode 100644 index 0000000..b9d16e3 --- /dev/null +++ b/importers_go_package_test.go @@ -0,0 +1,81 @@ +package main + +import ( + "context" + "path/filepath" + "strings" + "testing" + + "codemap/scanner" +) + +// The printed lines for a Go file inside a multi-file package, on the #191 +// fixture: the count is labelled with its unit, and an empty answer says what +// the graph does not model instead of a bare zero. +func TestImportersCLIReportsGoPackageGranularity(t *testing.T) { + root, err := filepath.Abs(filepath.Join("testdata", "go-multi-file-package")) + if err != nil { + t.Fatal(err) + } + fg, err := scanner.BuildFileGraph(context.Background(), root, scanner.Filters{}) + if err != nil { + t.Fatalf("BuildFileGraph: %v", err) + } + + render := func(file string) (scanner.ImportersReport, string) { + t.Helper() + report, err := buildImportersReportFromGraph(root, file, fg) + if err != nil { + t.Fatalf("buildImportersReportFromGraph(%s): %v", file, err) + } + var buf strings.Builder + renderImportersReportCLI(&buf, report) + return report, buf.String() + } + + report, out := render("multi/a.go") + if got, want := report.Importers, []string{"app/main.go"}; strings.Join(got, ",") != strings.Join(want, ",") { + t.Fatalf("importers of multi/a.go = %v, want %v", got, want) + } + if report.Package != "example.com/demo/multi" || report.PackageSiblings != 1 { + t.Fatalf("report package = %q siblings = %d, want example.com/demo/multi with 1 sibling", report.Package, report.PackageSiblings) + } + want := " Imported by 1 file(s) (package example.com/demo/multi: Go imports name packages, not files)\n โ€ข app/main.go\n" + if !strings.Contains(out, want) { + t.Fatalf("--importers multi/a.go output:\n%q\nwant it to contain:\n%q", out, want) + } + + _, out = render("multi/a_test.go") + want = "No cross-package importers of multi/a_test.go. Same-package references are not modeled (2 other files in package example.com/demo/multi).\n" + if !strings.Contains(out, want) { + t.Fatalf("--importers multi/a_test.go output:\n%q\nwant it to contain:\n%q", out, want) + } + + // A single-file package with no importers is a true zero, and gets no + // same-package caveat because there is nothing unmodeled. + report, out = render("orphan/orphan.go") + if len(report.Importers) != 0 || report.PackageSiblings != 0 { + t.Fatalf("orphan report = %+v, want zero importers and zero siblings", report) + } + if !strings.Contains(out, "No files import orphan/orphan.go.\n") || strings.Contains(out, "Same-package") { + t.Fatalf("--importers orphan/orphan.go output:\n%q\nwant a plain zero with no same-package caveat", out) + } +} + +// End to end through the binary, as the issue reproduced it. +func TestImportersCommandListsMultiFilePackageImporter(t *testing.T) { + root, err := filepath.Abs(filepath.Join("testdata", "go-multi-file-package")) + if err != nil { + t.Fatal(err) + } + out, err := runCodemap("--importers", "multi/a.go", root) + if err != nil { + t.Fatalf("codemap --importers multi/a.go: %v\n%s", err, out) + } + if !strings.Contains(out, "โ€ข app/main.go") { + t.Fatalf("--importers multi/a.go must list app/main.go:\n%s", out) + } + if strings.Contains(out, "No files import") { + t.Fatalf("--importers multi/a.go reported a zero:\n%s", out) + } +} diff --git a/main_cli_polish_test.go b/main_cli_polish_test.go index 29574d3..264f4c3 100644 --- a/main_cli_polish_test.go +++ b/main_cli_polish_test.go @@ -9,19 +9,25 @@ import ( func TestRenderImportersReportExplainsEmptyResult(t *testing.T) { report := scanner.ImportersReport{ - Root: "/repo", - Mode: "importers", - File: "watch/events.go", + Root: "/repo", + Mode: "importers", + File: "watch/events.go", + Package: "codemap/watch", + PackageSiblings: 10, } var buf strings.Builder renderImportersReportCLI(&buf, report) out := buf.String() - if !strings.Contains(out, "No files import watch/events.go") { - t.Fatalf("empty importers result must say so instead of printing nothing:\n%q", out) - } - if !strings.Contains(out, "same package") { - t.Fatalf("empty result should explain the same-package limitation for Go:\n%q", out) + // A Go file inside a multi-file package: the zero is a cross-package + // zero, and the line must say what the graph does not model, with the + // count, instead of a bare "No files import" (#138 part 1). + want := "No cross-package importers of watch/events.go. Same-package references are not modeled (10 other files in package codemap/watch).\n" + if !strings.Contains(out, want) { + t.Fatalf("empty Go importers result must name the unmodeled siblings:\n got %q\nwant %q", out, want) + } + if strings.Contains(out, "No files import") { + t.Fatalf("a Go file with package siblings must not print a bare zero:\n%q", out) } // The token-budgeted blast-radius renderer must keep omitting empty diff --git a/mcp/main.go b/mcp/main.go index ea7f301..81519d3 100644 --- a/mcp/main.go +++ b/mcp/main.go @@ -908,8 +908,28 @@ func handleGetImporters(ctx context.Context, req *mcp.CallToolRequest, input Imp importers := append([]string(nil), fg.Importers[file]...) sort.Strings(importers) structured := newImportersOutput(absRoot, file, fg) + // Go imports name packages, so a Go file's importers are its package's + // importers and its own package's files are never among them. Label the + // count with that unit, and turn an empty answer into the statement of + // what is not modeled instead of a bare zero. + granularity := "" + empty := "No files import '" + file + "'" + if pkg, pkgFiles, ok := fg.GoPackage(file); ok { + granularity = fmt.Sprintf(" (package %s: Go imports name packages, not files)", pkg) + siblings := len(pkgFiles) + if !scanner.IsTestFile(file) { + siblings-- + } + if siblings > 0 { + noun := "other files" + if siblings == 1 { + noun = "other file" + } + empty = fmt.Sprintf("No cross-package importers of '%s'. Same-package references are not modeled (%d %s in package %s)", file, siblings, noun, pkg) + } + } if len(importers) == 0 { - return textResult("No files import '" + file + "'" + mcpCoverageText(fg)), structured, nil + return textResult(empty + mcpCoverageText(fg)), structured, nil } isHub := scanner.CountHubImporters(importers) >= scanner.HubThreshold @@ -918,7 +938,7 @@ func handleGetImporters(ctx context.Context, req *mcp.CallToolRequest, input Imp hubNote = " โš ๏ธ HUB FILE" } - return textResult(fmt.Sprintf("%d files import '%s':%s\n%s%s", len(importers), file, hubNote, strings.Join(importers, "\n"), mcpCoverageText(fg))), structured, nil + return textResult(fmt.Sprintf("%d files import '%s'%s:%s\n%s%s", len(importers), file, granularity, hubNote, strings.Join(importers, "\n"), mcpCoverageText(fg))), structured, nil } func handleFind(ctx context.Context, req *mcp.CallToolRequest, input FindQueryInput) (*mcp.CallToolResult, any, error) { diff --git a/render/depgraph.go b/render/depgraph.go index b67e297..5d8c3dd 100644 --- a/render/depgraph.go +++ b/render/depgraph.go @@ -51,6 +51,8 @@ func getSystemName(dirPath string) string { return "Root" } +var depgraphExtPattern = regexp.MustCompile(`\.[^.]+$`) + // Depgraph renders the dependency flow visualization. The file graph is built // from the caller's analyses (BuildFileGraphFromAnalyses) rather than by // re-running the ast-grep scan, and ctx propagates cancellation through that @@ -68,7 +70,7 @@ func Depgraph(ctx context.Context, w io.Writer, project scanner.DepsProject) { // Build internal names lookup internalNames := make(map[string]bool) - extPattern := regexp.MustCompile(`\.[^.]+$`) + extPattern := depgraphExtPattern for _, f := range files { basename := filepath.Base(f.Path) name := strings.ToLower(extPattern.ReplaceAllString(basename, "")) @@ -84,6 +86,7 @@ func Depgraph(ctx context.Context, w io.Writer, project scanner.DepsProject) { fg, err := scanner.BuildFileGraphFromAnalyses(ctx, project.Root, files, graphFilters) var internalDeps map[string][]string var depCounts map[string]int + depImporters := make(map[string][]string) if err == nil && fg != nil { // Build set of files we're displaying (may be filtered by --diff) displayedFiles := make(map[string]bool) @@ -114,14 +117,15 @@ func Depgraph(ctx context.Context, w io.Writer, project scanner.DepsProject) { if !displayedFiles[file] { continue } - count := 0 + var shown []string for _, imp := range importers { if displayedFiles[imp] { - count++ + shown = append(shown, imp) } } - if count > 0 { - depCounts[file] = count + if len(shown) > 0 { + depCounts[file] = len(shown) + depImporters[file] = shown } } } else { @@ -269,6 +273,12 @@ func Depgraph(ctx context.Context, w io.Writer, project scanner.DepsProject) { continue } + // Go imports name packages, so one import of a multi-file package + // is one edge per file in the graph (and in --json). The text view + // draws it once, as "dir/ (N files)", so the output grows with the + // number of imports rather than with how packages are split up. + targetStrs := depgraphTargetNames(targets) + if len(targets) == 1 { t := targets[0] tName := extPattern.ReplaceAllString(t, "") @@ -299,13 +309,10 @@ func Depgraph(ctx context.Context, w io.Writer, project scanner.DepsProject) { } else { fmt.Fprintf(w, " %s โ”€โ”€โ”€โ–ถ %s\n", nameNoExt, tName) } + } else if len(targetStrs) == 1 { + fmt.Fprintf(w, " %s โ”€โ”€โ”€โ–ถ %s\n", nameNoExt, targetStrs[0]) } else { - var targetStrs []string - for _, t := range targets { - targetStrs = append(targetStrs, extPattern.ReplaceAllString(t, "")) - } - - if len(targets) <= 4 { + if len(targetStrs) <= 4 { fmt.Fprintf(w, " %s โ”€โ”€โ”€โ–ถ %s\n", nameNoExt, strings.Join(targetStrs, ", ")) } else { fmt.Fprintf(w, " %s โ”€โ”€โ”ฌโ”€โ”€โ–ถ %s\n", nameNoExt, targetStrs[0]) @@ -337,19 +344,7 @@ func Depgraph(ctx context.Context, w io.Writer, project scanner.DepsProject) { // HUBS section if len(depCounts) > 0 { - type hub struct { - name string - count int - } - var hubs []hub - for name, count := range depCounts { - if count >= 2 { - hubs = append(hubs, hub{name, count}) - } - } - sort.Slice(hubs, func(i, j int) bool { - return hubs[i].count > hubs[j].count - }) + hubs := depgraphHubs(depCounts, depImporters) if len(hubs) > 6 { hubs = hubs[:6] } @@ -358,7 +353,7 @@ func Depgraph(ctx context.Context, w io.Writer, project scanner.DepsProject) { fmt.Fprintln(w, strings.Repeat("โ”€", 61)) var hubStrs []string for _, h := range hubs { - hubStrs = append(hubStrs, fmt.Sprintf("%s (%dโ†)", extPattern.ReplaceAllString(h.name, ""), h.count)) + hubStrs = append(hubStrs, h.String()) } fmt.Fprintf(w, "HUBS: %s\n", strings.Join(hubStrs, ", ")) } @@ -371,13 +366,97 @@ func Depgraph(ctx context.Context, w io.Writer, project scanner.DepsProject) { } internalCount := 0 for _, targets := range internalDeps { - internalCount += len(targets) + internalCount += len(depgraphTargetNames(targets)) } fmt.Fprintf(w, "%d files ยท %d functions ยท %d deps\n", len(files), totalFuncs, internalCount) renderCoverageLine(w, project.Coverage) fmt.Fprintln(w) } +// depgraphTargetNames is the text form of one file's resolved imports: the +// files of a multi-file Go package collapse to one "dir/ (N files)" entry and +// every other path loses its extension. The graph itself (and --json) keeps +// the per-file edges. +func depgraphTargetNames(targets []string) []string { + names := make([]string, 0, len(targets)) + for _, t := range scanner.CollapseGoPackageFiles(targets) { + if strings.HasSuffix(t, " files)") { + names = append(names, t) + continue + } + names = append(names, depgraphExtPattern.ReplaceAllString(t, "")) + } + return names +} + +type depgraphHub struct { + name string + count int + files int // non-zero when name is a multi-file Go package +} + +func (h depgraphHub) String() string { + if h.files > 0 { + return fmt.Sprintf("%s (%dโ†, %d files)", h.name, h.count, h.files) + } + return fmt.Sprintf("%s (%dโ†)", h.name, h.count) +} + +// depgraphHubs ranks hubs by displayed importer count. The files of one +// multi-file Go package all carry the same importers (an import names the +// package), so they are reported once, as the package, with the number of +// distinct files importing it; a single-file package stays a file. Ties break +// on name so the head of the list is stable across runs. +func depgraphHubs(depCounts map[string]int, depImporters map[string][]string) []depgraphHub { + goFilesPerDir := make(map[string]int) + for name := range depCounts { + if strings.EqualFold(filepath.Ext(name), ".go") { + goFilesPerDir[pathDir(name)]++ + } + } + packageImporters := make(map[string]map[string]bool) + var hubs []depgraphHub + for name, count := range depCounts { + if strings.EqualFold(filepath.Ext(name), ".go") && goFilesPerDir[pathDir(name)] >= 2 { + dir := pathDir(name) + if packageImporters[dir] == nil { + packageImporters[dir] = make(map[string]bool) + } + for _, imp := range depImporters[name] { + packageImporters[dir][imp] = true + } + continue + } + if count >= 2 { + hubs = append(hubs, depgraphHub{name: depgraphExtPattern.ReplaceAllString(name, ""), count: count}) + } + } + for dir, importers := range packageImporters { + if len(importers) >= 2 { + label := dir + if label == "." { + label = "" + } + hubs = append(hubs, depgraphHub{name: label + "/", count: len(importers), files: goFilesPerDir[dir]}) + } + } + sort.Slice(hubs, func(i, j int) bool { + if hubs[i].count != hubs[j].count { + return hubs[i].count > hubs[j].count + } + return hubs[i].name < hubs[j].name + }) + return hubs +} + +func pathDir(path string) string { + dir := filepath.ToSlash(filepath.Dir(filepath.FromSlash(path))) + if dir == "" { + return "." + } + return dir +} + // renderCoverageLine surfaces degraded scan coverage in text output so a // fail-closed or partial scan never reads as a complete, empty graph. func renderCoverageLine(w io.Writer, coverage analysis.Coverage) { diff --git a/render/depgraph_test.go b/render/depgraph_test.go index 6806625..1891dd2 100644 --- a/render/depgraph_test.go +++ b/render/depgraph_test.go @@ -336,3 +336,45 @@ func TestDepgraphRendersSharedCoverageDetailOnce(t *testing.T) { t.Fatalf("expected partial coverage line, got:\n%s", output) } } + +// A Go import of a multi-file package is one edge per file in the graph; the +// text view draws it once at package granularity so output grows with imports +// rather than with how a package is split, and the hub line reports the +// package with its distinct importer count rather than every file in it. +func TestDepgraphRendersGoPackageImportsAtPackageGranularity(t *testing.T) { + root := t.TempDir() + writeDepgraphFile(t, root, "go.mod", "module example.com/demo\n\ngo 1.22\n") + writeDepgraphFile(t, root, "app/main.go", "package main\n") + writeDepgraphFile(t, root, "cli/run.go", "package cli\n") + writeDepgraphFile(t, root, "multi/a.go", "package multi\n") + writeDepgraphFile(t, root, "multi/b.go", "package multi\n") + writeDepgraphFile(t, root, "single/single.go", "package single\n") + + project := scanner.DepsProject{ + Root: root, + Files: []scanner.FileAnalysis{ + {Path: "app/main.go", Language: "go", Functions: []string{"main"}, Imports: []string{"example.com/demo/multi", "example.com/demo/single"}}, + {Path: "cli/run.go", Language: "go", Functions: []string{"Run"}, Imports: []string{"example.com/demo/multi"}}, + {Path: "multi/a.go", Language: "go", Functions: []string{"A"}}, + {Path: "multi/b.go", Language: "go", Functions: []string{"B"}}, + {Path: "single/single.go", Language: "go", Functions: []string{"One"}}, + }, + } + + var buf bytes.Buffer + Depgraph(context.Background(), &buf, project) + output := buf.String() + for _, want := range []string{ + " main โ”€โ”€โ”€โ–ถ multi/ (2 files), single/single\n", + " run โ”€โ”€โ”€โ–ถ multi/ (2 files)\n", + "HUBS: multi/ (2โ†, 2 files)\n", + "5 files ยท 5 functions ยท 3 deps\n", + } { + if !strings.Contains(output, want) { + t.Errorf("deps text missing %q:\n%s", want, output) + } + } + if strings.Contains(output, "multi/a") { + t.Errorf("deps text lists a package member file instead of the package:\n%s", output) + } +} diff --git a/scanner/filegraph.go b/scanner/filegraph.go index 334d32f..a30ad7f 100644 --- a/scanner/filegraph.go +++ b/scanner/filegraph.go @@ -4,6 +4,7 @@ import ( "bufio" "context" "encoding/json" + "fmt" "os" pathpkg "path" "path/filepath" @@ -249,7 +250,6 @@ func buildFileGraphFromAnalysesWithCargoMetadataAndFilters(ctx context.Context, if a.Language == "rust" { resolvedImports = resolveRustReferences(absRoot, a, idx, rustWorkspace) } else { - sourceLanguage := DetectLanguage(a.Path) for _, imp := range a.Imports { if err := ctx.Err(); err != nil { return nil, err @@ -257,15 +257,14 @@ func buildFileGraphFromAnalysesWithCargoMetadataAndFilters(ctx context.Context, resolved := fuzzyResolveWithWorkspace( imp, a.Path, idx, fg.Module, fg.PathAliases, fg.BaseURL, jsResolver, dartResolver, ) - // Exclude multi-file Go package imports to avoid inflating hub counts. - // Go package imports start with the module prefix and resolve to all - // files in that package. For all other imports (e.g., C# namespace - // imports that resolve via directory matching), allow multi-file - // resolution so inter-namespace dependencies are tracked. - isGoPkg := sourceLanguage == "go" && isLocalGoImport(imp, fg.Module) && len(resolved) > 1 - if !isGoPkg && len(resolved) > 0 { - resolvedImports = append(resolvedImports, resolved...) - } + // A Go import names a package, and a package is every non-test + // .go file in its directory, so one import fans out to one edge + // per file. Dropping the multi-file case (as this once did, to + // keep hub counts small) made every importer of a package with + // two or more files vanish from --deps and --importers: see #191. + // Text renderers that need package granularity collapse these + // edges themselves; the graph keeps the file edges. + resolvedImports = append(resolvedImports, resolved...) } } @@ -1064,6 +1063,68 @@ func (fg *FileGraph) IsHub(path string) bool { return CountHubImporters(fg.Importers[path]) >= HubThreshold } +// GoPackage reports the Go package that path belongs to when this graph +// resolved Go imports at package granularity: the package's import path and +// its non-test files, which include path itself unless path is a _test.go +// file (one lives in the package but is never an import target, so it is +// absent from Packages). ok is false for a non-Go file, for a graph with no +// go.mod, and for a directory the index never saw. +func (fg *FileGraph) GoPackage(path string) (importPath string, files []string, ok bool) { + if fg == nil || fg.Module == "" || len(fg.Packages) == 0 { + return "", nil, false + } + if !strings.EqualFold(filepath.Ext(path), ".go") { + return "", nil, false + } + importPath = fg.Module + if dir := pathpkg.Dir(filepath.ToSlash(path)); dir != "" && dir != "." { + importPath += "/" + dir + } + files, ok = fg.Packages[importPath] + if !ok { + return "", nil, false + } + return importPath, files, true +} + +// CollapseGoPackageFiles rewrites a list of file paths for text output so Go +// files that were linked as one package read as one entry. Go imports name +// packages, so every non-test .go file of a directory arrives in an edge list +// together, and printing each one inflates the output without adding an edge +// the reader can act on. Two or more .go paths in one directory become +// "dir/ (N files)" with N the number of paths collapsed into it, placed at the +// first of their positions; every other path is returned unchanged. +func CollapseGoPackageFiles(paths []string) []string { + perDir := make(map[string]int, len(paths)) + for _, path := range paths { + if strings.EqualFold(filepath.Ext(path), ".go") { + perDir[pathpkg.Dir(filepath.ToSlash(path))]++ + } + } + out := make([]string, 0, len(paths)) + emitted := make(map[string]bool, len(perDir)) + for _, path := range paths { + if !strings.EqualFold(filepath.Ext(path), ".go") { + out = append(out, path) + continue + } + dir := pathpkg.Dir(filepath.ToSlash(path)) + if perDir[dir] < 2 { + out = append(out, path) + continue + } + if emitted[dir] { + continue + } + emitted[dir] = true + if dir == "." { + dir = "" + } + out = append(out, fmt.Sprintf("%s/ (%d files)", dir, perDir[pathpkg.Dir(filepath.ToSlash(path))])) + } + return out +} + // HubFiles returns all files that qualify as hubs under IsHub, ordered by // non-test importer count descending and then by path. Map iteration order is // random, so callers that truncate or display the head of this list would diff --git a/scanner/filegraph_test.go b/scanner/filegraph_test.go index cc044ba..eed175a 100644 --- a/scanner/filegraph_test.go +++ b/scanner/filegraph_test.go @@ -608,7 +608,14 @@ func TestBuildFileGraphGoPackageTargets(t *testing.T) { t.Fatal(err) } - want := []string{filepath.FromSlash("pkg/only/only.go")} + // A package import links the importer to every non-test file of the + // package, however many there are (#191); _test.go files and packages + // with only test files are never targets. + want := []string{ + filepath.FromSlash("pkg/only/only.go"), + filepath.FromSlash("pkg/multi/a.go"), + filepath.FromSlash("pkg/multi/b.go"), + } for _, caller := range []string{"cmd/caller.go", "cmd/caller_test.go"} { caller = filepath.FromSlash(caller) if got := graph.Imports[caller]; !reflect.DeepEqual(got, want) { @@ -766,7 +773,7 @@ func TestBuildFileGraphImportResolutionBoundaries(t *testing.T) { assertImports("python/main.py", []string{"python/services/__init__.py"}) assertImports("MyApp/Program.cs", []string{"MyApp/Models/User.cs"}) assertImports("cmd/main.go", []string{"pkg/single/util.go"}) - assertImports("cmd/multi.go", nil) + assertImports("cmd/multi.go", []string{"pkg/multi/a.go", "pkg/multi/b.go"}) assertImports("README.md", nil) } diff --git a/scanner/gopackage_test.go b/scanner/gopackage_test.go new file mode 100644 index 0000000..1e80898 --- /dev/null +++ b/scanner/gopackage_test.go @@ -0,0 +1,232 @@ +package scanner + +import ( + "bytes" + "context" + "encoding/json" + "io" + "os" + "os/exec" + "path/filepath" + "reflect" + "sort" + "strings" + "testing" +) + +// goMultiFilePackageFixture is the repro from #191: one module whose packages +// differ only in how many non-test files they hold. +const goMultiFilePackageFixture = "../testdata/go-multi-file-package" + +// An import of a Go package links the importer to every non-test file of the +// package. On main before this test, a package with two or more files produced +// no edge at all, so --importers multi/a.go answered "No files import" with +// complete coverage (#191). +func TestGoMultiFilePackageImportsResolve(t *testing.T) { + fg, err := BuildFileGraph(context.Background(), goMultiFilePackageFixture, Filters{}) + if err != nil { + t.Fatalf("BuildFileGraph: %v", err) + } + + wantImporters := map[string][]string{ + "multi/a.go": {"app/main.go"}, + "multi/b.go": {"app/main.go"}, + "named/named.go": {"app/main.go"}, + "named/other.go": {"app/main.go"}, + "single/single.go": {"app/main.go"}, + "single2/impl.go": {"app/main.go"}, + // Negative cases: nothing imports package orphan or package main, and + // a _test.go file is never an import target. + "orphan/orphan.go": nil, + "app/main.go": nil, + "multi/a_test.go": nil, + } + for file, want := range wantImporters { + if got := fg.Importers[file]; !reflect.DeepEqual(got, want) { + t.Errorf("Importers[%q] = %v, want %v", file, got, want) + } + } + + wantImports := []string{ + "multi/a.go", "multi/b.go", "named/named.go", "named/other.go", + "single/single.go", "single2/impl.go", + } + gotImports := append([]string(nil), fg.Imports["app/main.go"]...) + sort.Strings(gotImports) + if !reflect.DeepEqual(gotImports, wantImports) { + t.Errorf("Imports[app/main.go] = %v, want %v", gotImports, wantImports) + } + if fg.Coverage.Status != "" && fg.Coverage.Status != "complete" { + t.Errorf("coverage = %q, want complete", fg.Coverage.Status) + } + + // Hub status follows from package importers: one importer is below the + // threshold either way, and the package attribution is what --importers + // prints next to the count. + pkg, files, ok := fg.GoPackage("multi/a.go") + if !ok || pkg != "example.com/demo/multi" || len(files) != 2 { + t.Errorf("GoPackage(multi/a.go) = %q, %v, %v; want example.com/demo/multi with 2 files", pkg, files, ok) + } + if pkg, files, ok := fg.GoPackage("multi/a_test.go"); !ok || pkg != "example.com/demo/multi" || len(files) != 2 { + t.Errorf("GoPackage(multi/a_test.go) = %q, %v, %v; want the package it lives in", pkg, files, ok) + } + if _, _, ok := fg.GoPackage("go.mod"); ok { + t.Errorf("GoPackage(go.mod) reported a package for a non-Go file") + } +} + +func TestCollapseGoPackageFiles(t *testing.T) { + got := CollapseGoPackageFiles([]string{ + "analysis/contracts.go", + "scanner/filegraph.go", + "web/app.ts", + "scanner/types.go", + "scanner/walker.go", + "config/config.go", + "main.go", + "root_options.go", + }) + want := []string{ + "analysis/contracts.go", + "scanner/ (3 files)", + "web/app.ts", + "config/config.go", + "/ (2 files)", + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("CollapseGoPackageFiles = %v, want %v", got, want) + } + if got := CollapseGoPackageFiles(nil); len(got) != 0 { + t.Fatalf("CollapseGoPackageFiles(nil) = %v, want empty", got) + } +} + +// goListPackage is the subset of `go list -json` this oracle reads. +type goListPackage struct { + ImportPath string + Dir string + GoFiles []string + Imports []string + Module *struct{ Path string } +} + +func runGoList(t *testing.T, root string) []goListPackage { + t.Helper() + cmd := exec.Command("go", "list", "-json", "./...") + cmd.Dir = root + cmd.Env = append(os.Environ(), "GOWORK=off", "GOFLAGS=-mod=mod") + var stderr bytes.Buffer + cmd.Stderr = &stderr + out, err := cmd.Output() + if err != nil { + t.Fatalf("go list -json ./... in %s: %v\n%s", root, err, stderr.String()) + } + var packages []goListPackage + decoder := json.NewDecoder(bytes.NewReader(out)) + for { + var pkg goListPackage + if err := decoder.Decode(&pkg); err == io.EOF { + break + } else if err != nil { + t.Fatalf("decode go list output: %v", err) + } + packages = append(packages, pkg) + } + return packages +} + +// assertGraphCoversGoList checks that every cross-package import edge the Go +// toolchain reports between packages of this module has a file edge in fg: +// for each importing package P and imported package I, every non-test file of +// I is imported by at least one non-test file of P. Returns the number of +// package edges checked so a caller can refuse a vacuous pass. +func assertGraphCoversGoList(t *testing.T, root string, fg *FileGraph, packages []goListPackage) int { + t.Helper() + absRoot, err := filepath.Abs(root) + if err != nil { + t.Fatal(err) + } + byImportPath := make(map[string]goListPackage, len(packages)) + for _, pkg := range packages { + byImportPath[pkg.ImportPath] = pkg + } + relFiles := func(pkg goListPackage) []string { + rel, err := filepath.Rel(absRoot, pkg.Dir) + if err != nil { + t.Fatalf("package %s dir %q is outside %q: %v", pkg.ImportPath, pkg.Dir, absRoot, err) + } + files := make([]string, 0, len(pkg.GoFiles)) + for _, f := range pkg.GoFiles { + files = append(files, filepath.ToSlash(filepath.Join(rel, f))) + } + return files + } + edges := 0 + for _, pkg := range packages { + if pkg.Module == nil { + continue + } + module := pkg.Module.Path + imported := make(map[string]bool) + for _, f := range relFiles(pkg) { + for _, target := range fg.Imports[f] { + imported[filepath.ToSlash(target)] = true + } + } + for _, imp := range pkg.Imports { + if imp != module && !strings.HasPrefix(imp, module+"/") { + continue + } + target, ok := byImportPath[imp] + if !ok { + continue + } + edges++ + for _, f := range relFiles(target) { + if !imported[f] { + t.Errorf("go list: package %s imports %s, but no file of %s imports %s in codemap's graph (its files import %d targets)", + pkg.ImportPath, imp, pkg.ImportPath, f, len(imported)) + } + } + } + } + return edges +} + +// TestGoGraphMatchesGoList is the truth oracle for Go resolution: the compiler +// knows every cross-package edge, and codemap's graph must contain each one. +// This is the check that would have caught #191 on day one. +func TestGoGraphMatchesGoList(t *testing.T) { + if _, err := exec.LookPath("go"); err != nil { + t.Skip("go not on PATH") + } + + t.Run("fixture", func(t *testing.T) { + fg, err := BuildFileGraph(context.Background(), goMultiFilePackageFixture, Filters{}) + if err != nil { + t.Fatalf("BuildFileGraph: %v", err) + } + edges := assertGraphCoversGoList(t, goMultiFilePackageFixture, fg, runGoList(t, goMultiFilePackageFixture)) + if edges != 4 { + t.Fatalf("checked %d package edges, want 4 (app/main.go imports four packages)", edges) + } + }) + + t.Run("codemap", func(t *testing.T) { + if testing.Short() { + t.Skip("skipping whole-repository oracle in -short mode") + } + root := ".." + if _, err := os.Stat(filepath.Join(root, "go.mod")); err != nil { + t.Skipf("repository root not available: %v", err) + } + fg, err := BuildFileGraph(context.Background(), root, Filters{}) + if err != nil { + t.Fatalf("BuildFileGraph: %v", err) + } + edges := assertGraphCoversGoList(t, root, fg, runGoList(t, root)) + if edges == 0 { + t.Fatal("go list reported no in-module package edges for codemap; the oracle checked nothing") + } + }) +} diff --git a/scanner/types.go b/scanner/types.go index 3871f2f..28223b5 100644 --- a/scanner/types.go +++ b/scanner/types.go @@ -183,6 +183,13 @@ type ImportersReport struct { HubImports []string `json:"hub_imports,omitempty"` ImporterCount int `json:"importer_count"` IsHub bool `json:"is_hub"` + // Package is the Go import path of the package File belongs to, set only + // when the graph resolved File's language at package granularity (Go with + // a go.mod). Importers of a Go file are the files that import its package, + // so PackageSiblings โ€” the other non-test files in that package โ€” is the + // set a reader must check by hand: same-package references are not edges. + Package string `json:"package,omitempty"` + PackageSiblings int `json:"package_siblings,omitempty"` // NotIndexed is true when the queried file was never part of the scanned // file set at all โ€” e.g. it lives under a hidden or excluded directory, // or the path doesn't exist โ€” as opposed to being scanned and genuinely diff --git a/testdata/go-multi-file-package/app/main.go b/testdata/go-multi-file-package/app/main.go new file mode 100644 index 0000000..e617281 --- /dev/null +++ b/testdata/go-multi-file-package/app/main.go @@ -0,0 +1,14 @@ +package main + +import ( + "fmt" + + "example.com/demo/multi" + "example.com/demo/named" + "example.com/demo/single" + "example.com/demo/single2" +) + +func main() { + fmt.Println(single.One(), single2.Two(), multi.A(), multi.B(), named.Named(), named.Other()) +} diff --git a/testdata/go-multi-file-package/go.mod b/testdata/go-multi-file-package/go.mod new file mode 100644 index 0000000..007be7e --- /dev/null +++ b/testdata/go-multi-file-package/go.mod @@ -0,0 +1,3 @@ +module example.com/demo + +go 1.22 diff --git a/testdata/go-multi-file-package/multi/a.go b/testdata/go-multi-file-package/multi/a.go new file mode 100644 index 0000000..325090a --- /dev/null +++ b/testdata/go-multi-file-package/multi/a.go @@ -0,0 +1,4 @@ +package multi + +// A is defined in the first of two files in package multi. +func A() int { return helper() } diff --git a/testdata/go-multi-file-package/multi/a_test.go b/testdata/go-multi-file-package/multi/a_test.go new file mode 100644 index 0000000..eca2f26 --- /dev/null +++ b/testdata/go-multi-file-package/multi/a_test.go @@ -0,0 +1,10 @@ +package multi + +import "testing" + +// A test file lives in the package but is never an import target. +func TestA(t *testing.T) { + if A() != 1 { + t.Fatal("A() != 1") + } +} diff --git a/testdata/go-multi-file-package/multi/b.go b/testdata/go-multi-file-package/multi/b.go new file mode 100644 index 0000000..3e8f357 --- /dev/null +++ b/testdata/go-multi-file-package/multi/b.go @@ -0,0 +1,7 @@ +package multi + +// B is defined in the second file; helper is referenced from a.go without an +// import, which is the same-package reference codemap does not model. +func B() int { return 2 } + +func helper() int { return 1 } diff --git a/testdata/go-multi-file-package/named/named.go b/testdata/go-multi-file-package/named/named.go new file mode 100644 index 0000000..2b1b2a6 --- /dev/null +++ b/testdata/go-multi-file-package/named/named.go @@ -0,0 +1,4 @@ +package named + +// Named is in the file that shares the package's name. +func Named() int { return 3 } diff --git a/testdata/go-multi-file-package/named/other.go b/testdata/go-multi-file-package/named/other.go new file mode 100644 index 0000000..fced2f4 --- /dev/null +++ b/testdata/go-multi-file-package/named/other.go @@ -0,0 +1,4 @@ +package named + +// Other is in the file that does not. +func Other() int { return 4 } diff --git a/testdata/go-multi-file-package/orphan/orphan.go b/testdata/go-multi-file-package/orphan/orphan.go new file mode 100644 index 0000000..b83db34 --- /dev/null +++ b/testdata/go-multi-file-package/orphan/orphan.go @@ -0,0 +1,5 @@ +package orphan + +// Nothing in this module imports package orphan: its importer count is a +// true zero, not a dropped edge. +func Unused() int { return 0 } diff --git a/testdata/go-multi-file-package/single/single.go b/testdata/go-multi-file-package/single/single.go new file mode 100644 index 0000000..898ed0a --- /dev/null +++ b/testdata/go-multi-file-package/single/single.go @@ -0,0 +1,4 @@ +package single + +// One is the only non-test file in package single. +func One() int { return 1 } diff --git a/testdata/go-multi-file-package/single2/impl.go b/testdata/go-multi-file-package/single2/impl.go new file mode 100644 index 0000000..56f9cc6 --- /dev/null +++ b/testdata/go-multi-file-package/single2/impl.go @@ -0,0 +1,4 @@ +package single2 + +// Two lives in a file whose name differs from its package. +func Two() int { return 2 }