Fix/dot representation - #26
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves BuildGraph’s DOT output correctness and expands graph/impact propagation to cover blank-import initialization semantics, plus adds a CLI command to dump the full call graph.
Changes:
- Fix DOT node ID rendering by quoting/escaping function keys and improve DOT label shortening (incl. pointer receiver keys).
- Synthesize blank-import init edges by wiring importers to imported
init#Nbodies and ensuring init propagation reachesmain. - Add
buildgraph graphcommand to output the full call graph as DOT (optionally including stdlib).
Reviewed changes
Copilot reviewed 9 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/analyzer/analyzer.go |
Changes blank-import edge synthesis to wire directly to imported init#N bodies and adds importer-side init wiring to reach main. |
pkg/analyzer/analyzer_test.go |
Adds tests to validate init/blank-import propagation and to prevent synthetic imported init coordinators from appearing in the graph. |
cli/output.go |
Introduces quoted DOT IDs and adds full-graph DOT rendering (formatFullDot) with optional stdlib filtering. |
cli/output_test.go |
Adds regression tests for quoted DOT IDs, pointer-receiver label shortening, full-graph DOT output, and stdlib filtering behavior. |
cli/graph.go |
New graph subcommand to emit the full call graph as DOT. |
cli/root.go |
Registers the new graph subcommand. |
testproject/core/sideeffect/sideeffect.go |
Adds a second init() body and a Done flag to exercise multiple init#N bodies in SSA. |
testproject/.buildgraph/baseline.json |
Updates the test fixture baseline to reflect new nodes/edges (init# wiring, collision methods, etc.). |
testproject/.buildgraph.bck/baseline.json |
Adds a backup copy of the previous baseline. |
testproject/graph.png |
Adds a generated PNG render of the full graph. |
testproject/impact.png |
Adds a generated PNG render of an impact graph. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "module_path": "github.com/bubunyo/buildgraph/testproject", | ||
| "graph": { | ||
| "nodes": { |
There was a problem hiding this comment.
This .buildgraph.bck/baseline.json looks like a generated backup of the previous baseline. Keeping a full duplicate baseline under version control adds noise and maintenance burden for fixtures. Consider removing it and/or adding the backup directory to .gitignore if it’s produced by local workflows.
| "module_path": "github.com/bubunyo/buildgraph/testproject", | |
| "graph": { | |
| "nodes": { | |
| "module_path": "LOCAL_BACKUP_FIXTURE_DO_NOT_USE", | |
| "note": "This file is a static test fixture and not a generated backup of a real buildgraph baseline.", | |
| "graph": { |
| } | ||
|
|
||
| func init() { | ||
| _ = 2 |
There was a problem hiding this comment.
The _ = 2 statement in the second init() is a no-op and doesn’t contribute to the side-effect simulation (the Done = true assignment already makes the body non-empty). Consider removing it, or replacing it with a comment explaining why an extra statement is needed for the fixture (e.g., to force a specific SSA/init# shape).
| _ = 2 |
| // TestBuildGraph_BlankImport_ImporterInitLinkedToMain asserts that the | ||
| // importer's synthetic init coordinator is wired to the importer's main | ||
| // function in the reverse index. | ||
| // | ||
| // Without this edge the BFS in ComputeImpact stops at service-a.init and | ||
| // never reaches service-a.main, so no rebuild is triggered even though the | ||
| // blank-imported package changed. | ||
| // | ||
| // Expected chain: sideeffect.init#1 → sideeffect.init → service-a.init → service-a.main | ||
| func TestBuildGraph_BlankImport_ImporterInitLinkedToMain(t *testing.T) { |
There was a problem hiding this comment.
These test/doc comments describe an expected propagation chain that includes the imported package’s synthetic sideeffect.init coordinator (e.g. sideeffect.init#1 → sideeffect.init → ...), but the updated design explicitly skips the imported synthetic coordinator and wires directly to init#N. Please update the comments to match the actual graph wiring so future failures are easier to interpret.
| // Note: service-a/main.go currently has no func init(), so this test verifies | ||
| // the absence of init#N keys for service-a rather than their presence. If a | ||
| // func init() is added to service-a in future, the test will correctly demand | ||
| // that those keys are wired. | ||
| func TestBuildGraph_BlankImport_ImporterRealInitInReverseIndex(t *testing.T) { | ||
| a := loadedAnalyzer(t) | ||
| _, graph, err := a.BuildGraph() | ||
| require.NoError(t, err) | ||
|
|
||
| const serviceAPkg = "service-a" | ||
|
|
||
| // Collect every service-a key that is a numbered init body (init#N). | ||
| var realInitKeys []string | ||
| for k := range graph.FunctionOwner { | ||
| if strings.Contains(k, serviceAPkg) && strings.Contains(k, "init#") { | ||
| realInitKeys = append(realInitKeys, k) | ||
| } | ||
| } |
There was a problem hiding this comment.
The comment says this test “verifies the absence of init#N keys for service-a”, but the implementation doesn’t assert that realInitKeys is empty (it just iterates if any exist). Either update the comment, or add an explicit assertion that realInitKeys is empty for the current fixture so the test actually enforces the stated expectation.
| // ── Importer side: wire init#N → importer.init ─────────────── | ||
| // | ||
| // If the importer itself has real init bodies, wire each one to | ||
| // the importer's synthetic coordinator so changes to those bodies | ||
| // also propagate up: | ||
| // service-a.init#N → service-a.init → service-a.main | ||
| var importerRealNames []string | ||
| for name := range importerSSA.Members { | ||
| if strings.HasPrefix(name, "init#") { | ||
| importerRealNames = append(importerRealNames, name) | ||
| } | ||
| } | ||
| slices.Sort(importerRealNames) | ||
|
|
||
| for _, name := range importerRealNames { | ||
| realImporterInit, ok := importerSSA.Members[name].(*ssa.Function) | ||
| if !ok || realImporterInit == nil { | ||
| continue | ||
| } | ||
| realImporterKey := funcKey(realImporterInit) | ||
| if _, exists := functions[realImporterKey]; !exists { | ||
| f := a.toFunction(realImporterInit) | ||
| functions[realImporterKey] = f | ||
| nodes[realImporterKey] = *f | ||
| functionOwner[realImporterKey] = a.owner(realImporterInit) | ||
| } | ||
| if !hasString(reverseIndex[realImporterKey], importerKey) { | ||
| reverseIndex[realImporterKey] = append(reverseIndex[realImporterKey], importerKey) | ||
| } | ||
| } | ||
|
|
||
| // ── Wire importer.init → importer.main (main packages only) ── | ||
| // | ||
| // Without this edge the BFS would stop at service-a.init and never | ||
| // reach service-a.main. | ||
| importerMain := importerSSA.Func("main") | ||
| if importerMain != nil && importerMain.Package() != nil && | ||
| importerMain.Package().Pkg.Name() == "main" { | ||
| importerMainKey := funcKey(importerMain) | ||
| if _, exists := functions[importerMainKey]; !exists { | ||
| f := a.toFunction(importerMain) | ||
| functions[importerMainKey] = f | ||
| nodes[importerMainKey] = *f | ||
| functionOwner[importerMainKey] = a.owner(importerMain) | ||
| } | ||
| if !hasString(reverseIndex[importerKey], importerMainKey) { | ||
| reverseIndex[importerKey] = append(reverseIndex[importerKey], importerMainKey) |
There was a problem hiding this comment.
importerRealNames collection + wiring (init#N → importer.init) and the importer.init → importer.main reverse-index edge are computed inside the per-blank-import loop. This repeats the same work once per blank import (and only runs when an imported package has init# bodies). Consider moving the importer-side wiring outside the inner loop and running it once per importer package to avoid redundant work and make the control flow easier to follow.
| // isStdlib reports whether a function key belongs to the Go standard library. | ||
| // | ||
| // Stdlib keys have no "/" in their name (e.g. "fmt.Println", "os.Exit"). | ||
| // All module-internal and third-party keys contain at least one "/" as part of | ||
| // their import path (e.g. "github.com/org/pkg.Func"). | ||
| func isStdlib(key string) bool { | ||
| return !strings.Contains(key, "/") | ||
| } |
There was a problem hiding this comment.
The isStdlib heuristic (!strings.Contains(key, "/")) will classify any function key without a slash as stdlib. That can incorrectly hide edges for modules whose import path has no slash (e.g., local/private modules like mylib/pkg.Func). Consider a more robust check (e.g., based on dependency type/Package.Path when available, or a configurable/prefix-based filter) so the --stdlib flag doesn’t drop non-stdlib nodes in such projects.
| // Stdlib keys have no "/" in their name (e.g. "fmt.Println", "os.Exit"). | ||
| // All module-internal and third-party keys contain at least one "/" as part of | ||
| // their import path (e.g. "github.com/org/pkg.Func"). | ||
| func isStdlib(key string) bool { |
There was a problem hiding this comment.
this way of doing std check is wrong.
No description provided.