Conversation
Assert span lists at runtime and document ordering requirement.
Reviewer's GuideThis PR replaces debug‐only span assertions with runtime validation that aggregates and panics on unsorted or overlapping spans, introduces a dedicated error type for diagnostics, updates the parser plan docs to note the new requirements, and adds comprehensive tests for these checks. Entity relationship diagram for span validation functions and error reportingerDiagram
SPAN {
int start
int end
}
SPAN_ORDER_ERROR {
Span prev
Span next
}
VALIDATE_SPANS_SORTED ||--o{ SPAN : checks
ENSURE_SPAN_LISTS_SORTED ||--o{ SPAN : checks
VALIDATE_SPANS_SORTED }o--|| SPAN_ORDER_ERROR : returns
ENSURE_SPAN_LISTS_SORTED }o--|| SPAN_ORDER_ERROR : aggregates
Class diagram for updated build_green_tree span validationclassDiagram
class parser {
+build_green_tree(tokens, src, imports, typedefs, relations, indexes, functions, rules) GreenNode
}
parser ..> ensure_span_lists_sorted : calls
parser ..> validate_spans_sorted : calls
parser ..> SpanOrderError : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThe documentation for the parser plan and the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant build_green_tree
participant ensure_span_lists_sorted
participant validate_spans_sorted
Caller->>build_green_tree: call with statement spans
build_green_tree->>ensure_span_lists_sorted: check all span lists
ensure_span_lists_sorted->>validate_spans_sorted: validate each span list
alt All spans valid
validate_spans_sorted-->>ensure_span_lists_sorted: success
ensure_span_lists_sorted-->>build_green_tree: success
build_green_tree-->>Caller: proceed with CST construction
else Some spans invalid (overlap/unsorted)
validate_spans_sorted-->>ensure_span_lists_sorted: error details
ensure_span_lists_sorted-->>Caller: panic with aggregated error messages
end
Possibly related issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Clippy (1.86.0)warning: failed to write cache, path: /usr/local/registry/index/index.crates.io-1949cf8c6b5b557f/.cache/ch/um/chumsky, error: Permission denied (os error 13) Caused by: 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (5)`docs/**/*.md`: Documentation must use en-GB-oxendict spelling and grammar (with the exception of 'license' which is to be left unchanged for community consistency).
📄 Source: CodeRabbit Inference Engine (AGENTS.md) List of files the instruction was applied to:
`**/*.md`: Markdown paragraphs and bullet points must be wrapped at 80 columns. Code blocks must be wrapped at 120 columns. Tables and headings must not be wrapped.
📄 Source: CodeRabbit Inference Engine (AGENTS.md) List of files the instruction was applied to:
`**/*.md`: * Avoid 2nd person or 1st person pronouns ("I", "you", "we") * Use en...
⚙️ Source: CodeRabbit Configuration File List of files the instruction was applied to:
`**/*.rs`: Comment why, not what. Explain assumptions, edge cases, trade-offs, o...
📄 Source: CodeRabbit Inference Engine (AGENTS.md) List of files the instruction was applied to:
`**/*.rs`: * Seek to keep the cyclomatic complexity of functions no more than 12...
⚙️ Source: CodeRabbit Configuration File List of files the instruction was applied to:
🔇 Additional comments (6)
✨ 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 (
|
![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 - here's some feedback:
- Consider adding test cases for empty, single-element, and correctly sorted spans to ensure
assert_spans_sorteddoes not panic on valid input. - If you want more flexibility, consider changing
assert_spans_sortedto return aResult<(), Error>rather than unconditionally panicking, so callers can handle invalid span orders gracefully.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding test cases for empty, single-element, and correctly sorted spans to ensure `assert_spans_sorted` does _not_ panic on valid input.
- If you want more flexibility, consider changing `assert_spans_sorted` to return a `Result<(), Error>` rather than unconditionally panicking, so callers can handle invalid span orders gracefully.
## Individual Comments
### Comment 1
<location> `docs/parser-plan.md:84` </location>
<code_context>
```
+`build_green_tree` expects every list of statement spans to be sorted by start
+offset and free from overlaps. If any span is mis-ordered, the builder panics to
+prevent mismatched nodes in the resulting CST.
+
</code_context>
<issue_to_address>
Consider changing 'mis-ordered' to 'misordered' for standard spelling.
'Misordered' is the standard spelling and preferred for consistency.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
offset and free from overlaps. If any span is mis-ordered, the builder panics to
prevent mismatched nodes in the resulting CST.
=======
offset and free from overlaps. If any span is misordered, the builder panics to
prevent mismatched nodes in the resulting CST.
>>>>>>> REPLACE
</suggested_fix>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}
|
@sourcery-ai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Refactor the repeated
if let Err(e)span checks into a helper or macro to reduce boilerplate and keep the code DRY. - Consider aggregating all span violations and reporting them together instead of panicking on the first one for more comprehensive diagnostics.
- Add tests that invoke
build_green_treewith misordered spans to assert it panics with the expected error message and spans shown.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Refactor the repeated `if let Err(e)` span checks into a helper or macro to reduce boilerplate and keep the code DRY.
- Consider aggregating all span violations and reporting them together instead of panicking on the first one for more comprehensive diagnostics.
- Add tests that invoke `build_green_tree` with misordered spans to assert it panics with the expected error message and spans shown.
## Individual Comments
### Comment 1
<location> `src/parser/mod.rs:999` </location>
<code_context>
+
+impl std::error::Error for SpanOrderError {}
+
+fn assert_spans_sorted(spans: &[Span]) -> Result<(), SpanOrderError> {
for pair in spans.windows(2) {
let [first, second] = pair else { continue };
</code_context>
<issue_to_address>
Function name 'assert_spans_sorted' may be misleading now that it returns a Result.
Consider renaming the function to 'check_spans_sorted' or 'validate_spans_sorted' to accurately reflect that it returns a Result instead of asserting.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
fn assert_spans_sorted(spans: &[Span]) -> Result<(), SpanOrderError> {
=======
fn validate_spans_sorted(spans: &[Span]) -> Result<(), SpanOrderError> {
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/parser/mod.rs:1010` </location>
<code_context>
+ errors.push(format!("{name} not sorted: {e}"));
+ }
+ }
+ assert!(errors.is_empty(), "{}", errors.join("\n"));
}
</code_context>
<issue_to_address>
Using assert! for error reporting may not be ideal for all build configurations.
Consider replacing assert! with panic! to ensure errors are reported in all build modes, including release.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
assert!(errors.is_empty(), "{}", errors.join("\n"));
=======
if !errors.is_empty() {
panic!("{}", errors.join("\n"));
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| errors.push(format!("{name} not sorted: {e}")); | ||
| } | ||
| } | ||
| assert!(errors.is_empty(), "{}", errors.join("\n")); |
There was a problem hiding this comment.
suggestion (bug_risk): Using assert! for error reporting may not be ideal for all build configurations.
Consider replacing assert! with panic! to ensure errors are reported in all build modes, including release.
| assert!(errors.is_empty(), "{}", errors.join("\n")); | |
| if !errors.is_empty() { | |
| panic!("{}", errors.join("\n")); | |
| } |
|
@coderabbitai have the following concerns all been addressed?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos Yes, all five concerns have been thoroughly addressed in this PR: ✅ Concern 1: Refactor repeated span checksThe implementation now uses ✅ Concern 2: Aggregate all violationsThe ✅ Concern 3: Tests for build_green_tree panicsMultiple test functions verify that
✅ Concern 4: Tests for valid inputsComprehensive test coverage for
✅ Concern 5: Return Result instead of panickingThe The implementation includes a well-designed 🏁 Script executed: Length of output: 6136 🏁 Script executed: Length of output: 712 🏁 Script executed: Length of output: 3862 🏁 Script executed: Length of output: 335 @leynos Yes, all five concerns have been thoroughly addressed in this PR: ✅ Concern 1: Refactor repeated span checksThe implementation now uses ✅ Concern 2: Aggregate all violationsThe ✅ Concern 3: Tests for build_green_tree panicsMultiple test functions verify that
✅ Concern 4: Tests for valid inputsComprehensive test coverage for
✅ Concern 5: Return Result instead of panickingThe The implementation includes a well-designed |
leynos
deleted the
codex/improve-span-ordering-validation-in-build_green_tree
branch
1 participant
Summary
Testing
make fmtmake lintmake testmake markdownlinthttps://chatgpt.com/codex/tasks/task_e_68672c7943f083228335ff6f3d5022cd
Summary by Sourcery
Add comprehensive runtime span ordering checks to parser tree construction, with detailed error reporting and updated documentation.
New Features:
Enhancements:
Documentation:
Tests: