Support compiling multi-module packages#161
Merged
warunalakshitha merged 8 commits intoballerina-platform:mainfrom Feb 24, 2026
Merged
Support compiling multi-module packages#161warunalakshitha merged 8 commits intoballerina-platform:mainfrom
warunalakshitha merged 8 commits intoballerina-platform:mainfrom
Conversation
📝 WalkthroughWalkthroughReplaces ad-hoc module ordering with a graph-based resolver, adds DependencyGraph and module resolver, splits compilation into two phases (sequential parse/type-resolve, concurrent CFG/analysis/desugaring), updates module import extraction and backend iteration source, and adds multi-module/cyclic test fixtures and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant PkgRes as PackageResolution
participant Resolver as ModuleResolver
participant Graph as DependencyGraph
participant PkgComp as PackageCompilation
participant ModCtx as ModuleContext
PkgRes->>PkgRes: collectModuleDescriptors()
PkgRes->>Resolver: newModuleResolver(descriptors)
PkgRes->>Resolver: resolveModuleLoadRequests()
Resolver-->>PkgRes: importModuleResponse (resolved/unresolved)
PkgRes->>Graph: add nodes & dependencies
PkgRes->>Graph: ToTopologicallySortedList()
Graph-->>PkgRes: sorted ModuleDescriptor list
PkgRes->>PkgComp: set topologicallySortedModuleList
sequenceDiagram
participant Backend as BallerinaBackend
participant PkgComp as PackageCompilation
participant ModCtx as ModuleContext
participant Worker as Phase2Worker
Backend->>PkgComp: iterate topologicallySortedModuleList
loop Phase 1 (sequential per module)
PkgComp->>ModCtx: resolveTypesAndSymbols() — parse, AST, symbol/type resolution
ModCtx->>ModCtx: parseDocumentsParallel()
end
PkgComp->>Worker: spawn goroutines for analyzeAndDesugar()
loop Phase 2 (concurrent)
Worker->>ModCtx: create CFG
Worker->>ModCtx: semantic analysis & CFG analysis
Worker->>ModCtx: desugar
end
Worker-->>PkgComp: signal completion
PkgComp->>PkgComp: collect diagnostics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
projects/dependency_graph.go (2)
83-120: Nondeterministic node iteration order affects topological sort stability.The
computeTopologicalSortmethod iterates overg.nodes()which usesmaps.Keys- this returns keys in an unspecified order. While the topological ordering will still be valid (dependencies before dependents), the specific order of nodes at the same dependency level will vary between runs.This could cause:
- Non-reproducible builds
- Flaky tests that depend on module order
- Harder debugging when order-sensitive issues occur
🔧 Suggested fix: sort nodes for deterministic traversal
func (g *DependencyGraph[T]) computeTopologicalSort() ([]T, [][]T) { nodes := g.nodes() + // Sort nodes for deterministic traversal order + slices.SortFunc(nodes, func(a, b T) int { + // Requires T to have a String() method or comparable ordering + // Alternatively, pass a comparator during graph construction + return strings.Compare(fmt.Sprint(a), fmt.Sprint(b)) + }) visited := make(map[T]bool, len(nodes))Alternatively, track insertion order in the builder and use that for iteration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/dependency_graph.go` around lines 83 - 120, computeTopologicalSort iterates over g.nodes() (which calls maps.Keys) producing a nondeterministic traversal; sort the node list before DFS so the visit order is stable: call a deterministic sort on the slice returned by g.nodes() (or use a deterministic ordering source) prior to the for _, node := range nodes loop so visit, inStack and cycles are processed reproducibly by computeTopologicalSort/visit; ensure any dependency iteration (for dep := range g.dependencies[vertex]) is also deterministic (sort the dependency keys) to fully stabilize the topological output.
29-35: Consider documenting therootNodefield usage.The
DependencyGraphstruct has arootNodefield that is copied inbuild()but never used in the current implementation. If this is intended for future use, a brief comment would help. If unused, consider removing it to reduce confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/dependency_graph.go` around lines 29 - 35, The rootNode field on struct DependencyGraph[T comparable] is currently copied in build() but never used, which is confusing; either remove the rootNode field and any assignments to it in build() (to eliminate dead state), or add a short doc comment above rootNode explaining its intended future purpose (e.g., "stored copy of the entry/root node for potential future traversal or reporting; currently unused") so callers/maintainers understand why it exists—update build() accordingly to stop assigning if removed or keep the assignment if you choose to retain the documented field.projects/module_context.go (3)
307-322: Test documents are parsed but results are discarded.The test document parsing loop spawns goroutines that parse documents but don't collect the results. If test documents don't need syntax trees for the current workflow, consider whether parsing them here is necessary at all, or document why the side effects of parsing are needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/module_context.go` around lines 307 - 322, The loop that iterates testDocIDs and calls documentContext.parse() in goroutines (using testDocContextMap, wg, sem) runs parses but never uses or stores any results; either remove the loop if parsing test documents is unnecessary, or modify it to collect and persist parse outputs (e.g., have documentContext.parse return or populate a field with the syntax tree that you then store back into testDocContextMap or a results slice) and ensure synchronization around writes; if parse is only kept for side effects, add a clear comment above the loop explaining those required side effects so future readers know why results are discarded.
228-233: Phase 2 silently skips modules without logging.When
bLangPkgorcompilerCtxis nil, Phase 2 returns silently. This could mask issues where Phase 1 failed unexpectedly. Consider adding a debug log or ensuring the compilation state reflects the skip.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/module_context.go` around lines 228 - 233, compilePhase2 currently returns silently when moduleCtx.bLangPkg or moduleCtx.compilerCtx is nil; add a diagnostic to avoid masking Phase 1 failures by logging the skip and marking the module state. Update compilePhase2 to check moduleCtx.bLangPkg and moduleCtx.compilerCtx and when either is nil emit a debug/error log that includes the module identifier (e.g., moduleCtx.name or moduleCtx.package path) and reason, and also set or update a module-level flag/state (e.g., phase2Skipped, errored, or similar on moduleContext) so the rest of the pipeline can detect the skip.
279-304: Nondeterministic syntax tree ordering may cause subtle issues.The parallel parsing collects syntax trees into a shared slice under a mutex, but the order depends on goroutine scheduling. While
buildBLangPackagemerges declarations, the order ofTopLevelNodes,Functions, etc. in the resultingBLangPackagewill be nondeterministic across runs.If downstream processing (symbol resolution, error reporting, code generation) is sensitive to declaration order, this could cause flaky behavior or non-reproducible builds.
🔧 Suggested fix: preserve deterministic ordering
- var ( - mu sync.Mutex - wg sync.WaitGroup - syntaxTrees []*tree.SyntaxTree - ) + var wg sync.WaitGroup + // Pre-allocate slice to preserve document order + syntaxTrees := make([]*tree.SyntaxTree, len(srcDocIDs)) // Parse source documents - collect syntax trees - for _, docID := range srcDocIDs { + for i, docID := range srcDocIDs { docCtx := srcDocContextMap[docID] if docCtx == nil { continue } wg.Add(1) - go func(dc *documentContext) { + go func(idx int, dc *documentContext) { defer wg.Done() sem <- struct{}{} // acquire semaphore defer func() { <-sem }() // release semaphore st := dc.parse() - if st != nil { - mu.Lock() - syntaxTrees = append(syntaxTrees, st) - mu.Unlock() - } - }(docCtx) + syntaxTrees[idx] = st // nil is fine, filtered below + }(i, docCtx) }Then filter nils after
wg.Wait():wg.Wait() // Filter nil entries while preserving order result := syntaxTrees[:0] for _, st := range syntaxTrees { if st != nil { result = append(result, st) } } return result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/module_context.go` around lines 279 - 304, The parallel parse loop appends SyntaxTree results in goroutines which yields nondeterministic ordering; fix by making collection deterministic: preallocate syntaxTrees with length len(srcDocIDs) and in the for loop capture the loop index (e.g., i) and write the parsed result into syntaxTrees[i] inside the goroutine after calling dc.parse(); after wg.Wait(), build the final ordered slice by iterating syntaxTrees and appending non-nil entries (preserving original srcDocIDs order) before passing them to buildBLangPackage or other downstream code that reads TopLevelNodes/Functions.projects/module_resolver.go (1)
88-104: Redundant check inresolveRequest.The method checks
isRootPackageModulewhich already verifies the module exists inmoduleNames, then immediately checksmoduleNames[request.moduleName]again. This is safe but slightly redundant.♻️ Minor simplification
func (r *moduleResolver) resolveRequest(request *moduleLoadRequest) *importModuleResponse { - // Check if this is a root package module - if r.isRootPackageModule(request.orgName, request.moduleName) { - modDesc, ok := r.moduleNames[request.moduleName] - if ok { - return &importModuleResponse{ - moduleDesc: modDesc, - resolved: true, - } - } + // Check org matches (or is nil) before looking up module + if request.orgName != nil && !request.orgName.Equals(r.rootPkgDesc.Org()) { + return &importModuleResponse{resolved: false} + } + + if modDesc, ok := r.moduleNames[request.moduleName]; ok { + return &importModuleResponse{ + moduleDesc: modDesc, + resolved: true, + } } // Module not found in root package return &importModuleResponse{ resolved: false, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/module_resolver.go` around lines 88 - 104, In resolveRequest, avoid the redundant double lookup of r.moduleNames by first fetching modDesc with "modDesc, ok := r.moduleNames[request.moduleName]" and then checking ok && r.isRootPackageModule(request.orgName, request.moduleName); if true return an importModuleResponse using the already-fetched modDesc; otherwise return the existing unresolved response. This keeps the same behavior but eliminates the repeated map access while still using the isRootPackageModule, moduleNames map, resolveRequest method, and importModuleResponse struct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/build_project_test.go`:
- Around line 616-618: The test fixture path is incorrect: update the
occurrences where projectPath is set (currently filepath.Join("testdata",
"multimoduleproject")) and any subsequent filepath.Abs calls that derive absPath
in projects/build_project_test.go to use the correct fixture directory name
"testdata/multi-module-project" (matching the usage around the later block at
lines ~728-730); ensure both places (the block setting projectPath/absPath near
the 616–618 range and the similar block near 667–669) are changed so the
projectPath variable and resulting absPath reference "multi-module-project".
In `@projects/document_context.go`:
- Around line 178-185: The org name path creates a PackageOrg with the raw text
from orgNameNode.OrgName().Text(), but quoted identifiers are not normalized
like module parts are; update the code in the branch that constructs org (where
NewPackageOrg is called) to strip a leading '\'' from
orgNameNode.OrgName().Text() (e.g., use strings.TrimPrefix(text, "'") or
equivalent) before passing it to NewPackageOrg so quoted org names match the
module-name normalization used elsewhere.
In `@projects/package_resolution.go`:
- Around line 125-131: Replace the blank assignment for
r.moduleDependencyGraph.FindCycles() with a real variable (e.g., cycles :=
r.moduleDependencyGraph.FindCycles()), check if len(cycles) > 0, build
Diagnostic entries describing each detected cycle (use your project's Diagnostic
type and a suitable diagnostic code such as a cycle/DependencyCycle code), set
r.diagnosticResult = NewDiagnosticResult(diags) with those diagnostics, and
prevent continuing with an invalid topological order (e.g., do not set
r.topologicallySortedModuleList or return/propagate an error so compilation
stops). Ensure you reference the existing symbols:
r.moduleDependencyGraph.FindCycles(), r.topologicallySortedModuleList, and
r.diagnosticResult = NewDiagnosticResult(nil) when replacing the nil with the
actual diagnostics.
- Around line 109-123: The map currently keys moduleContext entries by
modCtx.getDescriptor().String(), which is fragile and inefficient; change
descToCtx to type map[ModuleDescriptor]*moduleContext and use the descriptor
value itself as the key (assign descToCtx[modCtx.getDescriptor()] = modCtx in
the loop), then when building sorted use if modCtx, ok := descToCtx[desc]; ok {
sorted = append(sorted, modCtx) } so lookups use ModuleDescriptor directly
instead of string conversions; update the descToCtx make(...) call and both
places where you index/assign to descToCtx accordingly.
In `@projects/single_file_project_test.go`:
- Line 102: Update the incorrect testdata path references from
"testdata/single_file/main.bal" to "testdata/single-file/main.bal" inside the
tests; specifically edit the occurrences used by TestLoadSingleFile and
TestSingleFileTargetDirectory (where the projectPath or similar path variable is
assigned) so they match the hyphenated directory used elsewhere (e.g., where
projectPath is set to filepath.Join("testdata", "single-file", "main.bal")).
---
Nitpick comments:
In `@projects/dependency_graph.go`:
- Around line 83-120: computeTopologicalSort iterates over g.nodes() (which
calls maps.Keys) producing a nondeterministic traversal; sort the node list
before DFS so the visit order is stable: call a deterministic sort on the slice
returned by g.nodes() (or use a deterministic ordering source) prior to the for
_, node := range nodes loop so visit, inStack and cycles are processed
reproducibly by computeTopologicalSort/visit; ensure any dependency iteration
(for dep := range g.dependencies[vertex]) is also deterministic (sort the
dependency keys) to fully stabilize the topological output.
- Around line 29-35: The rootNode field on struct DependencyGraph[T comparable]
is currently copied in build() but never used, which is confusing; either remove
the rootNode field and any assignments to it in build() (to eliminate dead
state), or add a short doc comment above rootNode explaining its intended future
purpose (e.g., "stored copy of the entry/root node for potential future
traversal or reporting; currently unused") so callers/maintainers understand why
it exists—update build() accordingly to stop assigning if removed or keep the
assignment if you choose to retain the documented field.
In `@projects/module_context.go`:
- Around line 307-322: The loop that iterates testDocIDs and calls
documentContext.parse() in goroutines (using testDocContextMap, wg, sem) runs
parses but never uses or stores any results; either remove the loop if parsing
test documents is unnecessary, or modify it to collect and persist parse outputs
(e.g., have documentContext.parse return or populate a field with the syntax
tree that you then store back into testDocContextMap or a results slice) and
ensure synchronization around writes; if parse is only kept for side effects,
add a clear comment above the loop explaining those required side effects so
future readers know why results are discarded.
- Around line 228-233: compilePhase2 currently returns silently when
moduleCtx.bLangPkg or moduleCtx.compilerCtx is nil; add a diagnostic to avoid
masking Phase 1 failures by logging the skip and marking the module state.
Update compilePhase2 to check moduleCtx.bLangPkg and moduleCtx.compilerCtx and
when either is nil emit a debug/error log that includes the module identifier
(e.g., moduleCtx.name or moduleCtx.package path) and reason, and also set or
update a module-level flag/state (e.g., phase2Skipped, errored, or similar on
moduleContext) so the rest of the pipeline can detect the skip.
- Around line 279-304: The parallel parse loop appends SyntaxTree results in
goroutines which yields nondeterministic ordering; fix by making collection
deterministic: preallocate syntaxTrees with length len(srcDocIDs) and in the for
loop capture the loop index (e.g., i) and write the parsed result into
syntaxTrees[i] inside the goroutine after calling dc.parse(); after wg.Wait(),
build the final ordered slice by iterating syntaxTrees and appending non-nil
entries (preserving original srcDocIDs order) before passing them to
buildBLangPackage or other downstream code that reads TopLevelNodes/Functions.
In `@projects/module_resolver.go`:
- Around line 88-104: In resolveRequest, avoid the redundant double lookup of
r.moduleNames by first fetching modDesc with "modDesc, ok :=
r.moduleNames[request.moduleName]" and then checking ok &&
r.isRootPackageModule(request.orgName, request.moduleName); if true return an
importModuleResponse using the already-fetched modDesc; otherwise return the
existing unresolved response. This keeps the same behavior but eliminates the
repeated map access while still using the isRootPackageModule, moduleNames map,
resolveRequest method, and importModuleResponse struct.
azinneera
force-pushed
the
prj_api-m3
branch
from
8edd635 to
465a572
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
projects/testdata/multi-module-project/main.bal (1)
17-18: Import style inconsistency.The imports use two different styles:
multimoduleproject.services(without org) andtestorg/multimoduleproject.storage(with org). If this is intentional to test both import formats, consider adding a comment. Otherwise, consider using a consistent format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/testdata/multi-module-project/main.bal` around lines 17 - 18, The two import statements use inconsistent styles: multimoduleproject.services (no org) and testorg/multimoduleproject.storage (with org); either make them consistent by using the same import format for both modules (e.g., change multimoduleproject.services to testorg/multimoduleproject.services or change testorg/multimoduleproject.storage to multimoduleproject.storage) or add a brief inline comment explaining that the mixed styles are intentional for testing; update the imports referencing multimoduleproject.services and testorg/multimoduleproject.storage accordingly.projects/dependency_graph.go (1)
83-120: Topological sort iteration order is non-deterministic.The
nodes()method returns map keys in non-deterministic order (line 84, 113). While the resulting topological sort is still valid, different runs may produce different orderings for nodes at the same dependency level. This is typically acceptable but worth noting if deterministic output is needed for debugging or testing.If deterministic ordering is desired, consider sorting
nodesbefore iteration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/dependency_graph.go` around lines 83 - 120, The topological traversal uses g.nodes() which iterates map keys non-deterministically causing varying order across runs; to make results deterministic, sort the slice returned by nodes() before using it in computeTopologicalSort (so sort the local variable nodes prior to the for _, node := range nodes loop), ensuring visit/visit-related logic (in computeTopologicalSort and its inner visit closure) operates over the sorted order; pick a stable comparator for T (or require/convert to a comparable string/int key) and apply that sort step to guarantee consistent sorted and cycles output.projects/module_context.go (1)
266-326: Non-deterministic syntax tree ordering may cause inconsistent compilation results.The parallel parsing collects syntax trees in completion order, which is non-deterministic. While
buildBLangPackagemerges all compilation units, the order could affect:
- The order of top-level nodes in the resulting
BLangPackage- Potentially, which
PackageIDis used (first syntax tree's ID is taken)Consider sorting the syntax trees by document ID after collection to ensure deterministic ordering.
♻️ Proposed fix to ensure deterministic ordering
wg.Wait() + + // Sort by document path/name to ensure deterministic ordering + slices.SortFunc(syntaxTrees, func(a, b *tree.SyntaxTree) int { + // Assuming SyntaxTree has a method to get its source path/name + return strings.Compare(a.FilePath(), b.FilePath()) + }) + return syntaxTreesNote: Adjust the comparison function based on the actual
SyntaxTreeAPI for obtaining the file path or document identifier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/module_context.go` around lines 266 - 326, The parsing goroutines append syntax trees in completion order causing non-deterministic ordering; to fix, change parseDocumentsParallel so you store results into a map keyed by DocumentID (e.g., map[DocumentID]*tree.SyntaxTree) instead of appending directly to the shared slice, and after wg.Wait() build the returned slice in a deterministic order by iterating srcDocIDs (or another stable ordering) and appending map[docID] when present; update references to the local variable syntaxTrees accordingly and ensure the goroutines set the map entry (protected by mu) using the input docID or the dc identity rather than relying on parse completion order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@projects/package_resolution.go`:
- Around line 125-131: The call to r.moduleDependencyGraph.FindCycles()
currently discards the result, so when cycles exist the compiler continues;
update the code to check the returned cycles slice and, if non-empty, populate
r.diagnosticResult with a Diagnostic (or Diagnostics) describing each cycle (use
DiagnosticCode for cycle errors), and prevent or flag further processing (do not
silently set r.topologicallySortedModuleList to sorted without handling cycles).
Specifically, use the result of r.moduleDependencyGraph.FindCycles(), convert
each cycle into a diagnostic entry (with clear message and location
information), add them to the value returned by NewDiagnosticResult(...) or
create the result from those diagnostics, and ensure subsequent logic respects
r.diagnosticResult when cycles are present.
- Around line 109-123: Replace the string-key map with a ModuleDescriptor-key
map to avoid using String(): change descToCtx to type
map[ModuleDescriptor]*moduleContext (e.g., descToCtx :=
make(map[ModuleDescriptor]*moduleContext, len(pkgCtx.moduleIDs))), use
modCtx.getDescriptor() (the ModuleDescriptor) as the key when populating
descToCtx, and when building sorted iterate over sortedDescs (which are
ModuleDescriptor values) and look up descToCtx[desc] instead of
descToCtx[desc.String()]. Ensure all key lookups and assignments use the
ModuleDescriptor type consistently (symbols: descToCtx, moduleContext,
pkgCtx.moduleIDs, pkgCtx.moduleContextMap, sortedDescs, modCtx.getDescriptor()).
---
Nitpick comments:
In `@projects/dependency_graph.go`:
- Around line 83-120: The topological traversal uses g.nodes() which iterates
map keys non-deterministically causing varying order across runs; to make
results deterministic, sort the slice returned by nodes() before using it in
computeTopologicalSort (so sort the local variable nodes prior to the for _,
node := range nodes loop), ensuring visit/visit-related logic (in
computeTopologicalSort and its inner visit closure) operates over the sorted
order; pick a stable comparator for T (or require/convert to a comparable
string/int key) and apply that sort step to guarantee consistent sorted and
cycles output.
In `@projects/module_context.go`:
- Around line 266-326: The parsing goroutines append syntax trees in completion
order causing non-deterministic ordering; to fix, change parseDocumentsParallel
so you store results into a map keyed by DocumentID (e.g.,
map[DocumentID]*tree.SyntaxTree) instead of appending directly to the shared
slice, and after wg.Wait() build the returned slice in a deterministic order by
iterating srcDocIDs (or another stable ordering) and appending map[docID] when
present; update references to the local variable syntaxTrees accordingly and
ensure the goroutines set the map entry (protected by mu) using the input docID
or the dc identity rather than relying on parse completion order.
In `@projects/testdata/multi-module-project/main.bal`:
- Around line 17-18: The two import statements use inconsistent styles:
multimoduleproject.services (no org) and testorg/multimoduleproject.storage
(with org); either make them consistent by using the same import format for both
modules (e.g., change multimoduleproject.services to
testorg/multimoduleproject.services or change testorg/multimoduleproject.storage
to multimoduleproject.storage) or add a brief inline comment explaining that the
mixed styles are intentional for testing; update the imports referencing
multimoduleproject.services and testorg/multimoduleproject.storage accordingly.
2625cb2 to
27f8045
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #161 +/- ##
==========================================
+ Coverage 29.13% 29.48% +0.34%
==========================================
Files 254 257 +3
Lines 54309 54514 +205
==========================================
+ Hits 15825 16074 +249
+ Misses 37508 37451 -57
- Partials 976 989 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
azinneera
force-pushed
the
prj_api-m3
branch
from
27f8045 to
eccf879
Compare
heshanpadmasiri
previously approved these changes
Feb 20, 2026
Member
heshanpadmasiri
left a comment
heshanpadmasiri
left a comment
There was a problem hiding this comment.
Minor nit other than that LGTM
azinneera
force-pushed
the
prj_api-m3
branch
from
eccf879 to
790a54e
Compare
Contributor
|
Please sync with latest changes in main branch |
azinneera
force-pushed
the
prj_api-m3
branch
from
790a54e to
18b4a36
Compare
azinneera
force-pushed
the
prj_api-m3
branch
from
ed11293 to
23fc644
Compare
azinneera
force-pushed
the
prj_api-m3
branch
from
23fc644 to
eba75f5
Compare
warunalakshitha
approved these changes
Feb 24, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
projects/package_resolution.go (1)
125-131: Cycle detection result is discarded — already tracked for a later phase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/package_resolution.go` around lines 125 - 131, The code calls r.moduleDependencyGraph.FindCycles() and stores the result in cycles but discards it (assigned to _), losing cycle diagnostics; update the block in projects/package_resolution.go to capture and process the cycles instead of ignoring them: use the returned value from FindCycles() (cycles) and convert it into diagnostics to include in r.diagnosticResult (or pass into NewDiagnosticResult) so cycle information is preserved for later phases, referencing r.moduleDependencyGraph.FindCycles, the cycles variable, r.diagnosticResult, and NewDiagnosticResult when implementing the fix.
🧹 Nitpick comments (3)
projects/module_context.go (1)
272-321: Preserve source-document order for deterministic outputs.
parseDocumentsParallelcurrently appends in completion order, which can shuffle compilation unit order and diagnostic ordering. Consider capturing by index and compacting after join.♻️ Suggested deterministic collection
func parseDocumentsParallel( srcDocIDs []DocumentID, srcDocContextMap map[DocumentID]*documentContext, testDocIDs []DocumentID, testDocContextMap map[DocumentID]*documentContext, ) []*tree.SyntaxTree { var ( mu sync.Mutex wg sync.WaitGroup - syntaxTrees []*tree.SyntaxTree + syntaxTrees = make([]*tree.SyntaxTree, len(srcDocIDs)) ) // Parse source documents - collect syntax trees - for _, docID := range srcDocIDs { + for i, docID := range srcDocIDs { docCtx := srcDocContextMap[docID] if docCtx == nil { continue } wg.Add(1) - go func(dc *documentContext) { + go func(idx int, dc *documentContext) { defer wg.Done() st := dc.parse() if st != nil { - mu.Lock() - syntaxTrees = append(syntaxTrees, st) - mu.Unlock() + mu.Lock() + syntaxTrees[idx] = st + mu.Unlock() } - }(docCtx) + }(i, docCtx) } @@ wg.Wait() - return syntaxTrees + ordered := make([]*tree.SyntaxTree, 0, len(syntaxTrees)) + for _, st := range syntaxTrees { + if st != nil { + ordered = append(ordered, st) + } + } + return ordered }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/module_context.go` around lines 272 - 321, parseDocumentsParallel appends syntax trees in goroutine completion order causing non-deterministic ordering; instead allocate a temp slice with length len(srcDocIDs), spawn each source parse goroutine passing its index (use go func(i int, dc *documentContext) { ... }(i, docCtx)) and write the returned *tree.SyntaxTree into tempTrees[i], then after wg.Wait() compact tempTrees into syntaxTrees by iterating in index order and appending non-nil entries; reference parseDocumentsParallel, documentContext.parse, and the syntaxTrees variable when making these changes.projects/package_resolution.go (2)
37-44: Remove block-level comments — extract the described blocks to named functions instead.The file has repeated block-level comments (
// Add all modules as nodes first,// Get all module load requests for this module,// Map descriptors to contexts for lookup,// Build sorted module list from sorted descriptors, and the three innewPackageResolution) that all violate the project guideline. When you need a comment to describe a block, the block should become a function with that name instead.In
newPackageResolution(Lines 37–44) the comments are doubly redundant — the three blocks are already extracted to self-describing named functions (collectModuleDescriptors,buildModuleDependencyGraph,resolveDependencies).Inside
buildModuleDependencyGraph, the inner loops can be extracted:♻️ Suggested extraction for
buildModuleDependencyGraphfunc (r *PackageResolution) buildModuleDependencyGraph() { pkgCtx := r.rootPackageContext builder := newDependencyGraphBuilder[ModuleDescriptor]() - - // Add all modules as nodes first - for _, modID := range pkgCtx.moduleIDs { - modCtx := pkgCtx.moduleContextMap[modID] - if modCtx != nil { - builder.addNode(modCtx.getDescriptor()) - } - } - - // Process each module's imports and add edges - for _, modID := range pkgCtx.moduleIDs { - modCtx := pkgCtx.moduleContextMap[modID] - if modCtx == nil { - continue - } - fromDesc := modCtx.getDescriptor() - // Get all module load requests for this module - requests := modCtx.populateModuleLoadRequests() - requests = append(requests, modCtx.populateTestModuleLoadRequests()...) - // Resolve requests and add edges - responses := r.moduleResolver.resolveModuleLoadRequests(requests) - for _, resp := range responses { - if resp.resolved { - toDesc := resp.moduleDesc - // Only add edge if the dependency is a different module - if !fromDesc.Equals(toDesc) { - builder.addDependency(fromDesc, toDesc) - } - } - } - } + addModuleNodes(builder, pkgCtx) + r.addModuleEdges(builder, pkgCtx) r.moduleDependencyGraph = builder.build() } + +func addModuleNodes(builder *dependencyGraphBuilder[ModuleDescriptor], pkgCtx *packageContext) { + for _, modID := range pkgCtx.moduleIDs { + if modCtx := pkgCtx.moduleContextMap[modID]; modCtx != nil { + builder.addNode(modCtx.getDescriptor()) + } + } +} + +func (r *PackageResolution) addModuleEdges(builder *dependencyGraphBuilder[ModuleDescriptor], pkgCtx *packageContext) { + for _, modID := range pkgCtx.moduleIDs { + modCtx := pkgCtx.moduleContextMap[modID] + if modCtx == nil { + continue + } + fromDesc := modCtx.getDescriptor() + requests := modCtx.populateModuleLoadRequests() + requests = append(requests, modCtx.populateTestModuleLoadRequests()...) + for _, resp := range r.moduleResolver.resolveModuleLoadRequests(requests) { + if resp.resolved && !fromDesc.Equals(resp.moduleDesc) { + builder.addDependency(fromDesc, resp.moduleDesc) + } + } + } +}As per coding guidelines: "Don't add comments explaining each line of code. If you need to add comments to describe a block of statements then extract them to a function with meaningful name."
Also applies to: 61-96, 102-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/package_resolution.go` around lines 37 - 44, Remove the block-level comments by extracting each commented block into a clearly named function and calling it from the existing places; specifically, in newPackageResolution replace the three commented blocks with calls to collectModuleDescriptors, buildModuleDependencyGraph, and resolveDependencies (they already exist) and delete the redundant comments, and inside buildModuleDependencyGraph break the inner loops into small helper functions such as addModuleNodes (for "Add all modules as nodes first"), getModuleLoadRequests (for "Get all module load requests for this module"), mapDescriptorsToContexts, and addDependencyEdges (or similar names) so each block has a self-describing function name rather than a comment; update callers to use these new helpers and remove the original block comments in the regions around functions referenced (collectModuleDescriptors, buildModuleDependencyGraph, resolveDependencies and the blocks around lines 61-96 and 102-131).
26-27:moduleResolvershould be a local variable, not a struct field.
r.moduleResolveris written once at line 39 and read once at line 87 (insidebuildModuleDependencyGraph, which is called only from the constructor). The field unnecessarily extends the resolver's lifetime for the entire duration ofPackageResolution. Pass it as a parameter tobuildModuleDependencyGraphinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/package_resolution.go` around lines 26 - 27, The field r.moduleResolver on PackageResolution should be removed and made a local variable in the constructor: stop storing moduleResolver as a struct field, create it locally where it is currently assigned, and pass it into buildModuleDependencyGraph as an explicit parameter instead of reading r.moduleResolver; update buildModuleDependencyGraph's signature to accept the *moduleResolver argument and update the call site in the constructor (where r.moduleResolver is currently set and buildModuleDependencyGraph is invoked). Remove any references to r.moduleResolver elsewhere (e.g., in PackageResolution) so the resolver's lifetime is limited to the constructor-only usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/dependency_graph.go`:
- Around line 27-72: Rename the exported type DependencyGraph to an unexported
dependencyGraph and update all code references accordingly; specifically change
the type declaration "type DependencyGraph[T comparable]" to "type
dependencyGraph[T comparable]" and update any constructors/usages, plus adjust
method receivers on nodes(), DirectDependencies(), ToTopologicallySortedList(),
and FindCycles() to use the new receiver type name; also make these methods
unexported if they are only package-internal (e.g., DirectDependencies ->
directDependencies, ToTopologicallySortedList -> toTopologicallySortedList,
FindCycles -> findCycles) and update any callers/tests in the package to use the
new names.
In `@projects/module_context.go`:
- Around line 56-61: The struct field importedSymbols is left nil and should be
initialized to an empty map in all constructors that create this module/context
structs; update the constructor(s) that build the object containing fields
bLangPkg, bPackageSymbol, compilerCtx, importedSymbols, birPkg to set
importedSymbols = make(map[string]model.ExportedSymbolSpace) (and likewise
initialize the same-named map fields in the other related constructors
referenced around lines 87-98 and 122-133) so callers never encounter nil-map
panics.
---
Duplicate comments:
In `@projects/package_resolution.go`:
- Around line 125-131: The code calls r.moduleDependencyGraph.FindCycles() and
stores the result in cycles but discards it (assigned to _), losing cycle
diagnostics; update the block in projects/package_resolution.go to capture and
process the cycles instead of ignoring them: use the returned value from
FindCycles() (cycles) and convert it into diagnostics to include in
r.diagnosticResult (or pass into NewDiagnosticResult) so cycle information is
preserved for later phases, referencing r.moduleDependencyGraph.FindCycles, the
cycles variable, r.diagnosticResult, and NewDiagnosticResult when implementing
the fix.
---
Nitpick comments:
In `@projects/module_context.go`:
- Around line 272-321: parseDocumentsParallel appends syntax trees in goroutine
completion order causing non-deterministic ordering; instead allocate a temp
slice with length len(srcDocIDs), spawn each source parse goroutine passing its
index (use go func(i int, dc *documentContext) { ... }(i, docCtx)) and write the
returned *tree.SyntaxTree into tempTrees[i], then after wg.Wait() compact
tempTrees into syntaxTrees by iterating in index order and appending non-nil
entries; reference parseDocumentsParallel, documentContext.parse, and the
syntaxTrees variable when making these changes.
In `@projects/package_resolution.go`:
- Around line 37-44: Remove the block-level comments by extracting each
commented block into a clearly named function and calling it from the existing
places; specifically, in newPackageResolution replace the three commented blocks
with calls to collectModuleDescriptors, buildModuleDependencyGraph, and
resolveDependencies (they already exist) and delete the redundant comments, and
inside buildModuleDependencyGraph break the inner loops into small helper
functions such as addModuleNodes (for "Add all modules as nodes first"),
getModuleLoadRequests (for "Get all module load requests for this module"),
mapDescriptorsToContexts, and addDependencyEdges (or similar names) so each
block has a self-describing function name rather than a comment; update callers
to use these new helpers and remove the original block comments in the regions
around functions referenced (collectModuleDescriptors,
buildModuleDependencyGraph, resolveDependencies and the blocks around lines
61-96 and 102-131).
- Around line 26-27: The field r.moduleResolver on PackageResolution should be
removed and made a local variable in the constructor: stop storing
moduleResolver as a struct field, create it locally where it is currently
assigned, and pass it into buildModuleDependencyGraph as an explicit parameter
instead of reading r.moduleResolver; update buildModuleDependencyGraph's
signature to accept the *moduleResolver argument and update the call site in the
constructor (where r.moduleResolver is currently set and
buildModuleDependencyGraph is invoked). Remove any references to
r.moduleResolver elsewhere (e.g., in PackageResolution) so the resolver's
lifetime is limited to the constructor-only usage.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.gitignoreprojects/ballerina_backend.goprojects/build_project_test.goprojects/dependency_graph.goprojects/document_context.goprojects/export_test.goprojects/module_context.goprojects/module_load_request.goprojects/module_resolver.goprojects/package_compilation.goprojects/package_resolution.goprojects/single_file_project_test.goprojects/testdata/cyclicproject/Ballerina.tomlprojects/testdata/cyclicproject/main.balprojects/testdata/cyclicproject/modules/moduleA/a.balprojects/testdata/cyclicproject/modules/moduleB/b.balprojects/testdata/multi-module-project/Ballerina.tomlprojects/testdata/multi-module-project/main.balprojects/testdata/multi-module-project/modules/services/svc.balprojects/testdata/multi-module-project/modules/storage/db.balprojects/testdata/multi-module-project/util/file-util.balprojects/testdata/multi-module-project/utils.balprojects/testdata/myproject/main.balprojects/testdata/single-file/main-with-error.balprojects/testdata/single-file/main.bal
🚧 Files skipped from review as they are similar to previous changes (12)
- projects/testdata/multi-module-project/modules/services/svc.bal
- projects/build_project_test.go
- projects/testdata/cyclicproject/modules/moduleB/b.bal
- projects/testdata/multi-module-project/main.bal
- projects/document_context.go
- projects/testdata/multi-module-project/util/file-util.bal
- projects/export_test.go
- projects/testdata/multi-module-project/utils.bal
- .gitignore
- projects/package_compilation.go
- projects/single_file_project_test.go
- projects/module_load_request.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Approach
Phase 1 (Sequential): Parse documents (parallel) → AST building → Import Resolution → Symbol Resolution → Type Resolution
Phase 2 (Parallel): CFG creation → Semantic Analysis → CFG Analysis
Summary by CodeRabbit
New Features
Improvements
Tests