Add intermediate representation structures#32
Conversation
Reviewer's GuideThis PR introduces the intermediate representation layer by defining BuildGraph, Action, and BuildEdge structs in a new IR module, exposes the module publicly, adds documentation and tests (unit and Cucumber) for the IR, and updates related design docs and roadmap. Class diagram for new IR structures (BuildGraph, Action, BuildEdge)classDiagram
class BuildGraph {
+HashMap<String, Action> actions
+HashMap<PathBuf, BuildEdge> targets
+Vec<PathBuf> default_targets
}
class Action {
+Recipe recipe
+Option<String> description
+Option<String> depfile
+Option<String> deps_format
+Option<String> pool
+bool restat
}
class BuildEdge {
+String action_id
+Vec<PathBuf> inputs
+Vec<PathBuf> explicit_outputs
+Vec<PathBuf> implicit_outputs
+Vec<PathBuf> order_only_deps
+bool phony
+bool always
}
BuildGraph "1" o-- "*" Action : contains
BuildGraph "1" o-- "*" BuildEdge : contains
BuildEdge "1" --> "1" Action : references by action_id
File-Level Changes
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 2 minutes and 41 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 (1)
Summary by CodeRabbit
WalkthroughIntroduce a new intermediate representation (IR) module defining Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant BuildGraph
participant Action
participant BuildEdge
Tester->>BuildGraph: Create new BuildGraph
Tester->>Action: Create Action (with Recipe, metadata)
Tester->>BuildGraph: Insert Action
Tester->>BuildEdge: Create BuildEdge (linking inputs/outputs, flags)
Tester->>BuildGraph: Insert BuildEdge as Target
Tester->>BuildGraph: Set default targets
Tester->>BuildGraph: Query actions, targets, defaults
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/ir_tests.rs:9` </location>
<code_context>
+use std::path::PathBuf;
+
+#[rstest]
+fn build_graph_default_is_empty() {
+ let graph = BuildGraph::default();
+ assert!(graph.actions.is_empty());
+ assert!(graph.targets.is_empty());
+ assert!(graph.default_targets.is_empty());
+}
+
</code_context>
<issue_to_address>
Consider adding tests for edge cases such as duplicate action IDs or targets.
Adding tests for duplicate action IDs or targets will help confirm correct handling of these edge cases, such as whether duplicates are overwritten, cause errors, or are accepted.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
#[rstest]
fn build_graph_default_is_empty() {
let graph = BuildGraph::default();
assert!(graph.actions.is_empty());
assert!(graph.targets.is_empty());
assert!(graph.default_targets.is_empty());
}
=======
#[rstest]
fn build_graph_default_is_empty() {
let graph = BuildGraph::default();
assert!(graph.actions.is_empty());
assert!(graph.targets.is_empty());
assert!(graph.default_targets.is_empty());
}
#[test]
fn build_graph_duplicate_action_ids() {
let mut graph = BuildGraph::default();
let action1 = Action { id: "a".to_string(), ..Default::default() };
let action2 = Action { id: "a".to_string(), ..Default::default() };
// Insert first action
graph.actions.insert(action1.id.clone(), action1.clone());
// Insert duplicate action with same ID
let prev = graph.actions.insert(action2.id.clone(), action2.clone());
// Check if the duplicate overwrites the previous one
assert_eq!(graph.actions.len(), 1);
assert_eq!(graph.actions.get("a").unwrap(), &action2);
assert_eq!(prev.unwrap(), action1);
}
#[test]
fn build_graph_duplicate_targets() {
let mut graph = BuildGraph::default();
let target1 = PathBuf::from("foo.txt");
let target2 = PathBuf::from("foo.txt");
// Insert first target
graph.targets.insert(target1.clone(), BuildEdge::default());
// Insert duplicate target
let prev = graph.targets.insert(target2.clone(), BuildEdge::default());
// Check if the duplicate overwrites the previous one
assert_eq!(graph.targets.len(), 1);
assert!(graph.targets.contains_key(&target1));
assert!(prev.is_some());
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `docs/netsuke-design.md:1050` </location>
<code_context>
+### 5.5 Design Decisions
+
+The IR structures defined in `src/ir.rs` are minimal containers that mirror
+Ninja's conceptual model while remaining backend agnostic. `BuildGraph`
+collects all `Action`s and `BuildEdge`s in hash maps keyed by stable strings and
+`PathBuf`s so the graph can be deterministically traversed for snapshot tests.
</code_context>
<issue_to_address>
'backend agnostic' should be hyphenated as 'backend-agnostic' for correct grammar.
Compound adjectives before a noun should be hyphenated. Please use 'backend-agnostic'.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 (9)
docs/netsuke-design.md(1 hunks)docs/roadmap.md(1 hunks)src/ir.rs(1 hunks)src/lib.rs(1 hunks)tests/cucumber.rs(1 hunks)tests/features/ir.feature(1 hunks)tests/ir_tests.rs(1 hunks)tests/steps/ir_steps.rs(1 hunks)tests/steps/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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.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.
Files:
tests/cucumber.rstests/steps/mod.rssrc/lib.rstests/steps/ir_steps.rssrc/ir.rstests/ir_tests.rs
**/*.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- 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/roadmap.mddocs/netsuke-design.md
🔇 Additional comments (13)
src/lib.rs (1)
8-8: LGTM: IR module correctly exposed.The public module declaration properly exposes the new intermediate representation functionality.
src/ir.rs (1)
1-26: Excellent module documentation with practical example.The module doc comment properly explains the purpose and includes a helpful usage example that demonstrates the key structs.
tests/steps/mod.rs (1)
2-2: LGTM: Module declaration follows existing pattern.The ir_steps module is correctly added in alphabetical order, maintaining consistency with the existing structure.
tests/features/ir.feature (1)
1-8: Well-structured feature test for BuildGraph initialisation.The Cucumber feature properly tests the initial empty state of BuildGraph with clear, readable steps that verify all key collections.
tests/cucumber.rs (1)
9-9: LGTM: BuildGraph correctly integrated into test world.The build_graph field properly extends CliWorld with optional BuildGraph state, following the established pattern for test context fields.
docs/roadmap.md (1)
40-41: LGTM: Roadmap accurately reflects completed IR implementation.The checklist update correctly marks the IR data structures task as complete, aligning with the implementation in
src/ir.rs.docs/netsuke-design.md (1)
1047-1057: Excellent documentation of IR design rationale.The new subsection clearly explains the design decisions behind the IR structures, particularly the choice of hash map keys for deterministic traversal and the backend-agnostic approach. This documentation will be valuable for future maintainers.
tests/ir_tests.rs (4)
8-14: Clean test verifying default BuildGraph state.The test correctly validates that a default BuildGraph has empty collections for actions, targets, and default targets.
16-42: Comprehensive test of Action and BuildEdge creation.The test effectively validates the creation and insertion of IR structures, covering key fields like recipe, description, depfile, and flags. The assertions properly verify the graph state after insertions.
44-77: Robust duplicate action handling test.The test correctly verifies that inserting a duplicate action ID overwrites the previous entry and returns the replaced value. The Recipe pattern matching ensures the correct action is stored.
79-112: Effective duplicate target testing.The test properly validates duplicate target handling, confirming that the second BuildEdge overwrites the first and that the
alwaysflag is preserved correctly.tests/steps/ir_steps.rs (2)
7-10: Clean BuildGraph creation step.The step properly creates a default BuildGraph and stores it in the test world for subsequent assertions.
12-28: Well-implemented assertion steps for graph collections.The steps correctly use
.expect()for error handling and verify the lengths of actions, targets, and default targets collections. The parameterised expressions allow flexible count verification.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/ir.rs (3)
32-32: Past review comment addressed: Clone derive added.The
BuildGraphstruct now includes theClonederive as suggested in previous reviews, enabling cloning for testing and transformation scenarios.
43-43: Past review comment addressed: Essential derives added to Action.The
Actionstruct now includesCloneandPartialEqderives alongsideDebug, addressing the previous suggestion for improved usability and testing capabilities.
54-54: Past review comment addressed: Essential derives added to BuildEdge.The
BuildEdgestruct now includesCloneandPartialEqderives alongsideDebug, enabling usage in collections and testing scenarios as previously suggested.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
AGENTS.md(4 hunks)src/ast.rs(2 hunks)src/ir.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Prefer .expect() over .unwrap().
Files:
src/ast.rssrc/ir.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.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.
Files:
src/ast.rssrc/ir.rs
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Validate Markdown files using make markdownlint.
Run make fmt after any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by running make nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references.
Files:
AGENTS.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- 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:
AGENTS.md
🪛 LanguageTool
AGENTS.md
[locale-violation] ~46-~46: LICENSE must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...grammar. (EXCEPTION: the naming of the "LICENSE" file, which is to be left unchanged ...
(LICENCE_LICENSE_NOUN_SINGULAR)
[uncategorized] ~160-~160: Possible missing comma found.
Context: ...(>=) version requirements is strictly forbidden as they introduce unacceptable risk a...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (7)
AGENTS.md (2)
12-13: Approve formatting improvements for 80-column wrapping.The line wrapping changes correctly adhere to the markdown guideline of wrapping paragraphs and bullet points at 80 columns whilst preserving semantic content.
Also applies to: 28-34, 46-47, 157-161, 167-169
46-47: Ignore the LICENSE spelling suggestion.The static analysis tool suggests changing "LICENSE" to "licence" for British English, but the text explicitly states this is an exception for community consistency. The current spelling is correct.
src/ast.rs (1)
109-109: Approve derive additions for IR compatibility.Adding
CloneandPartialEqderives toRecipeandStringOrListenums is appropriate and necessary for their usage in the new intermediate representation structures. These standard derives enable cloning and equality comparison without introducing complexity.Also applies to: 175-175
src/ir.rs (4)
1-25: Approve comprehensive module documentation.The module documentation effectively explains the IR's purpose as a backend-agnostic representation and provides a clear, practical example demonstrating typical usage patterns.
31-40: Approve BuildGraph design with deterministic key strategy.The
BuildGraphstructure usesHashMap<String, Action>andHashMap<PathBuf, BuildEdge>with stable keys, enabling deterministic graph traversal essential for snapshot testing. The separation of actions and targets with default targets list provides clear organisation.
42-51: Approve Action struct design for build commands.The
Actionstruct appropriately mirrors Ninja rule concepts whilst remaining backend-agnostic. Fields likedepfile,deps_format, andpoolprovide necessary build system flexibility without coupling to specific implementations.
53-70: Approve BuildEdge design for input-output relationships.The
BuildEdgestruct effectively models build statements with clear separation of explicit inputs, outputs, and dependencies. Thephonyandalwaysflags directly reflect manifest requirements, maintaining traceability from source to IR.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary
Testing
make fmtmake lintmake testmake markdownlintmake nixie(fails: too many arguments)https://chatgpt.com/codex/tasks/task_e_6884e7980bf48322972c4e1fb67d08d9
Summary by Sourcery
Introduce a backend-agnostic intermediate representation (IR) for the build system and accompany it with documentation and tests
New Features:
Documentation:
Tests: