Conversation
Reviewer's GuideEncapsulates cycle detection traversal into a dedicated CycleDetector struct to improve state management and readability, refactors path handling to use borrowed &Path references, updates find_cycle to leverage the new API, documents the helper in design docs, and adds regression tests for cycle canonicalization. Class diagram for CycleDetector and related cycle detection changesclassDiagram
class CycleDetector {
- targets: &HashMap<PathBuf, BuildEdge>
- stack: Vec<&Path>
- states: HashMap<&Path, VisitState>
+ new(targets: &HashMap<PathBuf, BuildEdge>)
+ is_visited(node: &Path) bool
+ visit_node(node: &Path) Option<Vec<PathBuf>>
+ visit_dependencies(deps: &[PathBuf]) Option<Vec<PathBuf>>
}
class VisitState {
<<enum>>
Visiting
Visited
}
CycleDetector --> VisitState
CycleDetector --> BuildEdge
BuildEdge --> PathBuf
CycleDetector --> Path
CycleDetector --> PathBuf
CycleDetector --> HashMap
CycleDetector --> Vec
CycleDetector : -targets
CycleDetector : -stack
CycleDetector : -states
CycleDetector : +new()
CycleDetector : +is_visited()
CycleDetector : +visit_node()
CycleDetector : +visit_dependencies()
Class diagram for updated find_cycle functionclassDiagram
class find_cycle {
+find_cycle(targets: &HashMap<PathBuf, BuildEdge>) Option<Vec<PathBuf>>
}
find_cycle --> CycleDetector
find_cycle --> BuildEdge
find_cycle --> PathBuf
find_cycle --> HashMap
find_cycle : +find_cycle()
Class diagram for canonicalize_cycle regression testsclassDiagram
class canonicalize_cycle {
+canonicalize_cycle(cycle: Vec<PathBuf>) Vec<PathBuf>
}
canonicalize_cycle --> PathBuf
canonicalize_cycle --> Vec
canonicalize_cycle : +canonicalize_cycle()
class tests {
+canonicalize_cycle_rotates_smallest_node()
+canonicalize_cycle_handles_reverse_direction()
}
tests --> canonicalize_cycle
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughRefactor cycle detection in src/ir.rs by introducing a CycleDetector struct using borrowed Path references, updating traversal and state management, and integrating with find_cycle. Adjust documentation in docs/netsuke-design.md to describe the new helper and apply formatting and typographic tweaks. Add/adjust tests for cycle canonicalisation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant IR as find_cycle
participant CD as CycleDetector
participant CC as canonicalize_cycle
Caller->>IR: find_cycle(targets)
IR->>CD: new(targets)
loop for each node
IR->>CD: is_visited(node)?
alt not visited
IR->>CD: visit_node(node)
CD->>CD: visit_dependencies(node)
alt cycle found
CD-->>IR: Err(cycle stack)
IR->>CC: canonicalize(cycle)
CC-->>IR: canonical_cycle
IR-->>Caller: Err(canonical_cycle)
else no cycle
CD-->>IR: Ok
end
else already visited
IR-->>IR: skip
end
end
IR-->>Caller: Ok (no cycles)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Comment Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/netsuke-design.md(8 hunks)src/ir.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/netsuke-design.md
**/*.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/ir.rs
🔇 Additional comments (9)
docs/netsuke-design.md (5)
1084-1087: Good addition: document the traversal helper explicitly.The explanation of
CycleDetectorclarifies ownership and responsibilities in the DFS stage and aligns with the refactor insrc/ir.rs.
1188-1191: CLI–Ninja bridge details read cleanly and follow en‑GB spelling.The pass‑through of flags, directory canonicalisation, and I/O capture are described precisely and respect the style guide.
Also applies to: 1193-1197, 1199-1203
1286-1289: Error‑reporting bullets improved.The tightened wording of “Where” and “Why” keeps the guidance actionable and matches Section 7’s goals.
Also applies to: 1290-1293
1537-1538: Roadmap tasks are now unambiguous.The phased items are clear and consistent with the current implementation order.
Also applies to: 1541-1542, 1544-1544, 1546-1546
1659-1659: Footnote metadata and style are compliant.Date format and citation style match en‑GB and the repo’s Markdown rules.
src/ir.rs (4)
28-28: Path import is correct and required.Importing
PathalongsidePathBufenables borrowed‑key traversal without cloning.
495-501: Encapsulate traversal state (CQRS and SRP).The
CycleDetectorstruct reduces function complexity in line with the PR’s objective.
562-575: Top‑level driver is clean and deterministic.Iterating nodes once and using
is_visited()avoids redundant work. Canonicalisation downstream ensures consistent diagnostics.
515-546: Avoid MSRV‑brittlelet‑chains and improve panic messageReplace the chained
if let … && let …with nestedif letto avoid raising the MSRV; update theexpect("slice")to a descriptive message. Verify the project's MSRV (rust-version in Cargo.toml or rust-toolchain) before keeping let-chains.Apply:
- if let Some(edge) = self.targets.get(node) - && let Some(cycle) = self.visit_dependencies(&edge.inputs) - { - return Some(cycle); - } + if let Some(edge) = self.targets.get(node) { + if let Some(cycle) = self.visit_dependencies(&edge.inputs) { + return Some(cycle); + } + }- .get(idx..) - .expect("slice") + .get(idx..) // idx is from position(), always <= len + .expect("cycle slice must exist for previously seen node")Verify by running:
#!/bin/bash rg -n --type=rust -C2 $'if\\s+let\\b[^{\\n]*&&\\s*let\\b' src || true rg -n '^rust-version\\s*=\\s*".*"' Cargo.toml || true fd -HI 'rust-toolchain*' -t f -x sed -n '1,200p' {} || true
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainBased on my analysis of the current code, I can confirm that both issues have been resolved: ✅ Encapsulation Issue Resolved: The code no longer directly accesses ✅ Performance Issue Resolved: The implementation now uses
The refactoring successfully addresses both concerns while maintaining the same public API and functionality. The 🏁 Script executed: Length of output: 20341 @leynos: Yes, both issues have been fully resolved! Here's the evidence: ✅ Encapsulation Issue Resolved:
✅ Performance Issue Resolved:
The refactoring successfully addresses both performance and encapsulation concerns while maintaining the same public API and functionality. The |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- Consider consolidating the self‐edge cycle check into your core visiting logic (e.g. in
should_visit_node) instead of special-casing it invisit_dependencies, so you don’t duplicate handling. - The chained
if let Some(edge) = … && let Some(cycle) = …invisit_nodeis concise but a bit cryptic—splitting it into nestedif letor amatchcould improve readability. - Since any detected cycle always has length ≥2, you could simplify
canonicalize_cycleby removing the conditional guard aroundrotate_leftand directly rotating the prefix slice, making the intent clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider consolidating the self‐edge cycle check into your core visiting logic (e.g. in `should_visit_node`) instead of special-casing it in `visit_dependencies`, so you don’t duplicate handling.
- The chained `if let Some(edge) = … && let Some(cycle) = …` in `visit_node` is concise but a bit cryptic—splitting it into nested `if let` or a `match` could improve readability.
- Since any detected cycle always has length ≥2, you could simplify `canonicalize_cycle` by removing the conditional guard around `rotate_left` and directly rotating the prefix slice, making the intent clearer.
## Individual Comments
### Comment 1
<location> `src/ir.rs:596` </location>
<code_context>
.min_by(|(_, a), (_, b)| a.cmp(b))
.map_or(0, |(idx, _)| idx);
- cycle.rotate_left(start);
+ if let Some(slice) = cycle.get_mut(..len) {
+ slice.rotate_left(start);
+ }
</code_context>
<issue_to_address>
**question:** Using get_mut(..len) for rotation may be less clear than rotating the whole vector.
Rotating only a slice may behave differently if len < cycle.len(). Please verify this matches the intended logic for all cases.
</issue_to_address>
### Comment 2
<location> `src/ir.rs:496` </location>
<code_context>
- ) -> Option<Vec<PathBuf>> {
- match should_visit_node(states, node) {
+/// Detects cycles in a dependency graph by tracking traversal state.
+struct CycleDetector<'a> {
+ targets: &'a HashMap<PathBuf, BuildEdge>,
+ stack: Vec<&'a Path>,
</code_context>
<issue_to_address>
**issue (complexity):** Consider using owned PathBufs instead of borrowed &Path references to simplify the cycle detection logic and remove lifetime annotations.
Consider dropping the `&Path`‐based state entirely and just operate on owned `PathBuf`s. This lets you remove all those lifetimes and simplify both `CycleDetector` and `find_cycle`. For example:
```rust
enum VisitState { Visiting, Visited }
struct CycleDetector<'a> {
targets: &'a HashMap<PathBuf, BuildEdge>,
stack: Vec<PathBuf>,
states: HashMap<PathBuf, VisitState>,
}
impl<'a> CycleDetector<'a> {
fn find_cycle(targets: &'a HashMap<PathBuf, BuildEdge>) -> Option<Vec<PathBuf>> {
let mut det = CycleDetector { targets, stack: Vec::new(), states: Default::default() };
for node in targets.keys() {
if det.states.contains_key(node) { continue }
if let Some(cycle) = det.visit(node.clone()) {
return Some(cycle);
}
}
None
}
fn visit(&mut self, node: PathBuf) -> Option<Vec<PathBuf>> {
match self.states.get(&node) {
Some(VisitState::Visited) => return None,
Some(VisitState::Visiting) => {
let idx = self.stack.iter().position(|n| n == &node).unwrap();
let mut cycle = self.stack[idx..].to_vec();
cycle.push(node.clone());
return Some(canonicalize_cycle(cycle));
}
None => { self.states.insert(node.clone(), VisitState::Visiting); }
}
self.stack.push(node.clone());
if let Some(edge) = self.targets.get(&node) {
for dep in &edge.inputs {
if self.targets.contains_key(dep) {
if let Some(cycle) = self.visit(dep.clone()) {
return Some(cycle);
}
}
}
}
self.stack.pop();
self.states.insert(node, VisitState::Visited);
None
}
}
// usage:
fn find_cycle(targets: &HashMap<PathBuf, BuildEdge>) -> Option<Vec<PathBuf>> {
CycleDetector::find_cycle(targets)
}
```
This:
- Eliminates all `&'a Path` keys and the related lifetimes.
- Collapses `visit_node`/`visit_dependencies` into one `visit`.
- Keeps the same behavior and cycle‐canonicalization.
</issue_to_address>
### Comment 3
<location> `src/ir.rs:496` </location>
<code_context>
- ) -> Option<Vec<PathBuf>> {
- match should_visit_node(states, node) {
+/// Detects cycles in a dependency graph by tracking traversal state.
+struct CycleDetector<'a> {
+ targets: &'a HashMap<PathBuf, BuildEdge>,
+ stack: Vec<&'a Path>,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural tests for the new CycleDetector struct to demonstrate its correct operation in cycle detection scenarios.
While unit tests for canonicalize_cycle have been added, there are no direct behavioural tests for CycleDetector itself. Add tests that exercise CycleDetector's public methods, verifying correct cycle detection and traversal state management.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 4
<location> `src/ir.rs:495` </location>
<code_context>
}
}
-fn find_cycle(targets: &HashMap<PathBuf, BuildEdge>) -> Option<Vec<PathBuf>> {
- fn visit(
- targets: &HashMap<PathBuf, BuildEdge>,
</code_context>
<issue_to_address>
**issue (review_instructions):** The module does not begin with a `//!` comment as required by the instructions.
Please add a `//!` module-level comment at the top of this file to describe its purpose, as per the review instructions.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Every module **must** begin with a `//!` comment.
</details>
</issue_to_address>
### Comment 5
<location> `docs/netsuke-design.md:1103` </location>
<code_context>
+
+ Traversal state is managed by a small `CycleDetector` helper struct. This
+ type holds the recursion stack and visitation map, allowing the traversal
+ functions to remain focused and easily testable. The detector borrows path
+ references from the `targets` map, so `targets` must remain unchanged during
+ detection.
</code_context>
<issue_to_address>
**issue (review_instructions):** This paragraph line exceeds the 80 column wrapping limit for paragraphs and bullets.
Please wrap this paragraph so that no line exceeds 80 columns, in accordance with the documentation style guide.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 6
<location> `docs/netsuke-design.md:1104` </location>
<code_context>
+ Traversal state is managed by a small `CycleDetector` helper struct. This
+ type holds the recursion stack and visitation map, allowing the traversal
+ functions to remain focused and easily testable. The detector borrows path
+ references from the `targets` map, so `targets` must remain unchanged during
+ detection.
</code_context>
<issue_to_address>
**issue (review_instructions):** This paragraph line exceeds the 80 column wrapping limit for paragraphs and bullets.
Please wrap this paragraph so that no line exceeds 80 columns, in accordance with the documentation style guide.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 7
<location> `docs/netsuke-design.md:1105` </location>
<code_context>
+ type holds the recursion stack and visitation map, allowing the traversal
+ functions to remain focused and easily testable. The detector borrows path
+ references from the `targets` map, so `targets` must remain unchanged during
+ detection.
### 5.4 Ninja File Synthesis (`ninja_gen.rs`)
</code_context>
<issue_to_address>
**issue (review_instructions):** This paragraph line exceeds the 80 column wrapping limit for paragraphs and bullets.
Please wrap this paragraph so that no line exceeds 80 columns, in accordance with the documentation style guide.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
leynos
deleted the
codex/refactor-find_cycle-to-reduce-nesting-complexity
branch
1 participant
Summary
CycleDetectorfor clearer state managementcloses #74
Testing
make fmtmake check-fmtmake lintmake testmake markdownlintmake nixiehttps://chatgpt.com/codex/tasks/task_e_68c615de9a808322b7eca4a379fe4f9b
Summary by Sourcery
Refactor cycle detection into a dedicated CycleDetector struct, improve canonicalize_cycle safety, expand tests, and update documentation.
Bug Fixes:
Enhancements:
Documentation:
Tests: