feat: add toTopologicallySortedLevels() to DependencyGraph#44564
feat: add toTopologicallySortedLevels() to DependencyGraph#44564singhvishalkr wants to merge 5 commits into
Conversation
Implement BFS-based (Kahn's algorithm) level computation for the dependency graph. Nodes are grouped into levels where level 0 contains nodes with no dependencies, level 1 contains nodes that only depend on level-0 nodes, etc. Includes cycle detection that throws IllegalStateException with the set of unsorted nodes when cyclic dependencies exist.
Covers diamond graph, linear chain, disconnected components, single node, all-independent, cyclic (IllegalStateException), partial cycle, and unmodifiable return guarantees.
singhvishalkr
requested review from
MaryamZi,
gimantha,
hasithaa and
sameerajayasoma
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Assert all mutation ops (add, remove, set) on the outer List and (add, remove, clear) on every inner Set throw UnsupportedOperationException.
|
Addressed CodeRabbit's review — strengthened
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
compiler/ballerina-lang/src/main/java/io/ballerina/projects/DependencyGraph.java (1)
175-183: Document the public API guarantees.The implementation intentionally returns deeply unmodifiable levels and throws on cycles, but the Javadoc only describes the happy path. Please add the immutability guarantee and
@throws IllegalStateExceptionso callers can rely on the contract.📝 Proposed documentation update
* `@return` an ordered list of sets, where the index represents the level - * and each set contains nodes at that level + * and each set contains nodes at that level; both the returned list + * and each contained set are unmodifiable + * `@throws` IllegalStateException if the graph contains cyclic dependencies */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compiler/ballerina-lang/src/main/java/io/ballerina/projects/DependencyGraph.java` around lines 175 - 183, Update the Javadoc for the public method in DependencyGraph that returns nodes grouped by dependency levels to state the API guarantees: document that each returned level (each Set and the enclosing List) is deeply unmodifiable (immutable) and that the method will throw IllegalStateException when a cycle is detected. Add an explicit `@throws` IllegalStateException clause and a brief sentence describing the immutability guarantee so callers (of the DependencyGraph method that returns dependency levels) can rely on the contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@compiler/ballerina-lang/src/main/java/io/ballerina/projects/DependencyGraph.java`:
- Around line 185-205: The depCount map is only populated for keys of
dependencies, so nodes that appear only as dependency values never get an entry
and are missing from the initial zero-in-degree queue; update the population
logic so every node that appears either as a key or as a dependency value gets
an entry in depCount (e.g., before or while iterating dependencies.entrySet(),
ensure depCount.putIfAbsent(dep, 0) for each dep), keep the existing
depCount.put(dependent, entry.getValue().size()) for dependents, and continue
building reverseDeps as before so nodes like "B" in
DependencyGraph.from(Map.of("A", Set.of("B"))) will be queued correctly.
In
`@compiler/ballerina-lang/src/test/java/io/ballerina/projects/TopologicallySortedLevelsTest.java`:
- Around line 34-158: Add a new test in TopologicallySortedLevelsTest that
builds a graph via DependencyGraph.from(Map.of("A", Set.of("B"))) (import
java.util.Map) and asserts that toTopologicallySortedLevels() does not throw and
returns two levels with level 0 == Set.of("B") and level 1 == Set.of("A"); name
it something like testFromMapHandlesDependencyOnlyLeaves to mirror existing
tests and ensure dependency-only leaves are treated as level 0 rather than
causing a cyclic-detection error.
---
Nitpick comments:
In
`@compiler/ballerina-lang/src/main/java/io/ballerina/projects/DependencyGraph.java`:
- Around line 175-183: Update the Javadoc for the public method in
DependencyGraph that returns nodes grouped by dependency levels to state the API
guarantees: document that each returned level (each Set and the enclosing List)
is deeply unmodifiable (immutable) and that the method will throw
IllegalStateException when a cycle is detected. Add an explicit `@throws`
IllegalStateException clause and a brief sentence describing the immutability
guarantee so callers (of the DependencyGraph method that returns dependency
levels) can rely on the contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74decea4-d8fc-422b-a3d3-43b44f37cbf7
📒 Files selected for processing (2)
compiler/ballerina-lang/src/main/java/io/ballerina/projects/DependencyGraph.javacompiler/ballerina-lang/src/test/java/io/ballerina/projects/TopologicallySortedLevelsTest.java
Nodes appearing only as dependency values (not map keys) were missing from depCount, causing them to be excluded from the initial zero-in-degree queue. This made DependencyGraph.from() graphs incorrectly throw IllegalStateException for acyclic graphs. Also documents @throws and unmodifiable return guarantees.
|
All three points addressed:
|
1 participant
Summary
Adds a new public method
toTopologicallySortedLevels()toDependencyGraph<T>that groups nodes by their dependency depth. This enables parallel compilation of independent modules within each level.Resolves #43027
Changes
DependencyGraph.javaList<Set<T>> toTopologicallySortedLevels()using BFS-based Kahn's algorithmIllegalStateExceptionwith unsorted node set if cycles existCollections.unmodifiableListofCollections.unmodifiableSet)TopologicallySortedLevelsTest.java8 unit tests covering:
[D] [B,C] [A]IllegalStateExceptionIllegalStateExceptionExample
Summary
This pull request adds a new toTopologicallySortedLevels() method to DependencyGraph that groups nodes into dependency levels to enable parallel processing of independent modules.
Changes
New public API: DependencyGraph.toTopologicallySortedLevels()
Bug fix
Javadoc
Tests
Business outcome
Provides a reliable way for the compiler driver (and other consumers) to obtain dependency levels so independent modules can be processed or compiled in parallel, improving compilation parallelism and correctness for graphs that include dependency-only nodes.