feat: Milestone 8b — Conditionals & Logical Operators - #57
Merged
Conversation
Add logical operator variants to AST enums and IfStmt support: - BinaryOperator: Add `And` and `Or` variants with Display impls - UnaryOperator: Add `Not` variant with Display impl - StmtKind: Add `IfStmt` variant with supporting `IfStmt` and `ElifBranch` structs - Statement: Add IfStmt handling to `pretty_print_inline` and `pretty_print_children` - Add dead_code allows on new variants (will be used in subsequent parser/lexer tasks) - Add Display tests for new logical operators - Add minimal todo!() stubs in astgen.rs to maintain compilation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add Elif token to the lexer and implement if/elif/else parsing with recursive body_statement_parser for nested if support. Extract shared indented_block() helper used by both function_def_parser and if_stmt_parser, replacing duplicated block-parsing logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tests Complete M8b by adding the missing sema pass for And/Or/Not expressions and IfStmt statements, plus 7 end-to-end integration tests covering classify, in_range, not, simple if/else, if-without-else, nested if, and combined logical+conditional programs. Sema changes: merge And/Or into check_binary_op (reusing the existing binary-op pipeline), add Not arm to analyze_expr, add IfStmt arm to analyze_stmt with extracted analyze_block and check_condition_bool helpers to avoid duplication. Includes cargo fmt reformatting of parser.rs, tir.rs, and uir.rs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove resolved issues: - I-003 (no control flow) — if/elif/else and and/or/not now implemented - I-009 (FunctionContext rebuilt per statement) — now built once per function Update stale references: - I-005: HirStmt → UIR/TIR terminology - I-020: rewritten — now TirRef-keyed, multi-block works via block params, narrowed to future expression-level control flow concern Add new issues from M8b: - I-031: no return-flow analysis for if/elif/else - I-032: IfStmt is statement-only, no expression-level conditional - I-033: branch-scoped variables not promoted after if statement Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update README.md to reflect current compiler capabilities (M8b) instead of stale M4 state. Remove milestone2/ and milestone3/ example directories (covered by integration tests). Add classify.ryo and fizzbuzz.ryo as representative working examples. Move 12 aspirational examples to examples/future/. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add assert milestone design to implementation roadmap covering motivation, implementation approach, and pipeline placement. Fix "nightly" → "dev" wording in release workflow step name. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughImplements conditional statements ( Changes
Sequence DiagramsequenceDiagram
actor User
participant Lexer
participant Parser
participant ASTGEN
participant Sema
participant TIR
participant UIR
participant Codegen
User->>Lexer: Source with if/elif/else & and/or/not
Lexer->>Lexer: Recognize keywords (if, elif, and, or, not)
Lexer->>Parser: Token stream
Parser->>Parser: Parse if_stmt_parser (condition, blocks)
Parser->>Parser: Parse binary/unary operators (and/or/not)
Parser->>ASTGEN: AST with IfStmt & BinaryOperator
ASTGEN->>ASTGEN: Lower if statement to InstTag::IfStmt
ASTGEN->>ASTGEN: Lower and/or/not to InstTag::{And,Or,Not}
ASTGEN->>Sema: Instruction stream
Sema->>Sema: check_condition_bool: verify cond is bool
Sema->>Sema: Type-check and/or as bool→bool
Sema->>Sema: Type-check not as bool→bool
Sema->>TIR: Typed instructions with TirTag variants
TIR->>UIR: TirIfStmtView decoded & re-encoded
UIR->>Codegen: IfStmtView with elif branches
Codegen->>Codegen: Construct CFG blocks per branch
Codegen->>Codegen: Short-circuit and/or with block params
Codegen->>Codegen: Emit BoolNot as XOR with 1
Codegen->>User: Machine code (executable)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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. Review rate limit: 0/1 reviews remaining, refill in 29 minutes and 9 seconds.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: 7
🧹 Nitpick comments (3)
src/lexer.rs (1)
45-56: ⚡ Quick winAdd a focused lexer test for
elif/and/or/notkeywords.The token plumbing looks correct, but a targeted regression test would lock behavior if keyword/identifier ordering changes later.
✅ Suggested test addition
#[test] fn lex_keywords() { let (toks, _) = lex_strings("fn if else return mut struct enum match"); @@ assert_eq!(toks[7], Token::Match); } + +#[test] +fn lex_conditional_and_logical_keywords() { + let (toks, _) = lex_strings("if elif else and or not"); + assert_eq!( + toks, + vec![Token::If, Token::Elif, Token::Else, Token::And, Token::Or, Token::Not] + ); +}Also applies to: 164-187, 379-390
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lexer.rs` around lines 45 - 56, Add a focused unit test that verifies the lexer emits the Elif, And, Or, and Not token variants (and surrounding Identifier/Keyword tokens) for inputs containing "elif", "and", "or", "not" to prevent regressions if keyword/identifier ordering changes; implement the test next to the lexer tests (e.g., in the tests module of src/lexer.rs or tests/lexer.rs), construct a small input string containing those words (with spacing/punctuation variations), run it through the existing lexer entry point (e.g., Lexer::new(...)->collect() or tokenize(...) depending on your code), and assert the resulting token sequence contains the Elif, And, Or, Not variants in the expected positions relative to other tokens (use the Token enum variants Elif, And, Or, Not for comparisons).tests/integration_tests.rs (1)
949-1073: ⚡ Quick winAssert the new control-flow behavior, not just successful execution.
These cases only check
status.success()and"[Result] => 0", so they would still pass ififpicked the wrong branch ornot/andproduced the wrong value as long as the program kept exiting normally. Please assert distinct stdout markers from the selected branch/result, and make theandcase observe RHS evaluation so it actually exercises short-circuiting.Based on learnings, use integration tests in
tests/integration_tests.rsfor end-to-end compilation and execution; use inline unit tests inmod testsfor isolated module behavior.src/tir.rs (1)
904-926: ⚡ Quick winAdd one round-trip test with a non-empty
elifbranch.Current coverage validates
if + elsewith emptyelif; adding a case with at least oneelifwould directly exercise the newelif_count/body decode path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tir.rs` around lines 904 - 926, Add a new round-trip unit test similar to if_stmt_round_trips_through_extra but include at least one non-empty elif branch to exercise the elif_count/body decode path: create an additional elif condition and its statements (e.g., an elif_cond via TirBuilder::bool_const and an elif_stmt like a Return or other unary), call TirBuilder::if_stmt with the elifs provided (the function TirBuilder::if_stmt and the resulting if_ref are the key symbols), finish the TIR and use tir.if_stmt_view(if_ref) to assert that view.elif_branches contains the expected elif condition and statements as well as verifying cond, then_stmts, and else_stmts as in the existing test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/dev/implementation_roadmap.md`:
- Around line 766-775: Update the Ryo snippet so it uses current M8a syntax:
change the function signature from "fn main() -> int" to "fn main():", remove
the "let" declarations (use "x = 10" and "y = 3" instead of "let x = 10"/"let y
= 3"), drop the "return 0" line, and ensure the fenced code block is marked as
"```ryo" and the assert calls remain intact (refer to fn main, assert, x, y in
the snippet).
- Around line 709-713: Fix the malformed markdown in the M8b2 section: correct
the bolding around "Goal: Implement `assert`" so it uses proper Markdown (e.g.,
"Goal: Implement `assert`"), ensure all fenced code blocks include language tags
(use ryo for ryo examples and text for plain output) such as the blocks showing
AssertStmt, the ryo snippet for the assert runtime, and the sample assertion
failure line, and remove or move the stray '---' setext underline so it doesn't
form an accidental heading (replace it with a blank line or move it outside the
paragraph). Reference the AssertStmt struct name and the assert
function/signature (fn assert(condition: bool, message: str)) when updating the
three affected code blocks noted (around lines referenced in the comment) to
ensure consistent language tags and correct formatting.
In `@README.md`:
- Around line 199-203: The fenced code block showing the program output (the
block containing "Hello, Ryo!" and "[Result] => 0") is missing a language tag;
update that fenced block in README.md to include an appropriate language
identifier (for example add "text", "console", or the project-specific "ryo") so
the fence becomes ```text (or ```console/```ryo) to satisfy markdownlint rule
MD040 and the project's documentation guidelines.
In `@src/ast.rs`:
- Around line 114-126: The IfStmt and ElifBranch AST structs are missing a span
field; add pub span: SimpleSpan to both IfStmt and ElifBranch definitions so
control-flow nodes carry source spans, then update all places that construct or
pattern-match these nodes (parser functions/constructors that create IfStmt and
ElifBranch, any StmtKind::If creation sites, and any code that destructures
them) to provide and propagate the SimpleSpan value. Ensure you also update any
imports to bring SimpleSpan into scope and adjust tests/uses expecting the old
shape.
In `@src/codegen.rs`:
- Around line 428-470: The code emits each branch (then/elif/else) into the
shared ctx.locals so declarations inside a branch leak past the merge; fix by
saving a copy of ctx.locals (e.g., let saved_locals = ctx.locals.clone()) before
calling Self::emit_body for each branch (then, each elif body and else body) and
restoring ctx.locals = saved_locals immediately after emitting that branch
(regardless of whether the branch returns), so branch-local Variable bindings do
not survive past the block merge; update the blocks handled in emit_body calls
(references: Self::emit_body, Self::eval_inst, ctx.locals, view.then_stmts,
view.elif_branches, view.else_stmts, else_or_merge) to perform this save/restore
around each branch emission.
In `@src/parser.rs`:
- Around line 72-82: The indented-block parser currently allows back-to-back
statements because it uses stmt.then_ignore(skip_newlines()) when repeating
statements; change the repetition to require an actual newline token between
statements by replacing the separator with just(Token::Newline) (i.e., use
stmt.then_ignore(just(Token::Newline)).repeated().at_least(1).collect()) and
keep the surrounding
skip_newlines()/delimited_by(skip_newlines().ignore_then(just(Token::Indent)),
just(Token::Dedent)) logic so a trailing newline before Dedent is still
accepted.
In `@src/tir.rs`:
- Around line 605-647: The if_stmt_view decoder performs unchecked indexing and
usize arithmetic (direct slice[pos], range.as_range(), read_ref_list usage)
which can panic on malformed input; update if_stmt_view to validate the extra
range before slicing, use get(...) or checked indexing for every slice access
(including reading cond, elif_count, has_else), replace unchecked increments
with checked_add/checked_add_opt and bounds checks on pos before each read,
propagate errors by changing the signature to return a Result<TirIfStmtView,
DecodeError> (or use Option) and adjust calls, and similarly harden any adjacent
decoding code that uses read_ref_list or direct indexing (e.g., the nearby block
referenced at lines ~650-659) so all reads use read_ref_list safely and
bounds-checked operations instead of unchecked indexing and arithmetic.
---
Nitpick comments:
In `@src/lexer.rs`:
- Around line 45-56: Add a focused unit test that verifies the lexer emits the
Elif, And, Or, and Not token variants (and surrounding Identifier/Keyword
tokens) for inputs containing "elif", "and", "or", "not" to prevent regressions
if keyword/identifier ordering changes; implement the test next to the lexer
tests (e.g., in the tests module of src/lexer.rs or tests/lexer.rs), construct a
small input string containing those words (with spacing/punctuation variations),
run it through the existing lexer entry point (e.g., Lexer::new(...)->collect()
or tokenize(...) depending on your code), and assert the resulting token
sequence contains the Elif, And, Or, Not variants in the expected positions
relative to other tokens (use the Token enum variants Elif, And, Or, Not for
comparisons).
In `@src/tir.rs`:
- Around line 904-926: Add a new round-trip unit test similar to
if_stmt_round_trips_through_extra but include at least one non-empty elif branch
to exercise the elif_count/body decode path: create an additional elif condition
and its statements (e.g., an elif_cond via TirBuilder::bool_const and an
elif_stmt like a Return or other unary), call TirBuilder::if_stmt with the elifs
provided (the function TirBuilder::if_stmt and the resulting if_ref are the key
symbols), finish the TIR and use tir.if_stmt_view(if_ref) to assert that
view.elif_branches contains the expected elif condition and statements as well
as verifying cond, then_stmts, and else_stmts as in the existing test.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b32a8a70-ceb2-4665-a369-656cb9ac504f
📒 Files selected for processing (50)
.github/workflows/release.ymlISSUES.mdREADME.mddocs/dev/implementation_roadmap.mdexamples/classify.ryoexamples/fizzbuzz.ryoexamples/future/channel_communication.ryoexamples/future/closure_fib.ryoexamples/future/example.ryoexamples/future/mandelbrot.ryoexamples/future/mem.ryoexamples/future/memory.ryoexamples/future/panic.ryoexamples/future/recursive_fib.ryoexamples/future/select_example.ryoexamples/future/simple.ryoexamples/future/task_join.ryoexamples/future/task_spawn_run.ryoexamples/hello.ryoexamples/milestone2/README.mdexamples/milestone2/complete.ryoexamples/milestone2/error_invalid_syntax.ryoexamples/milestone2/error_missing_assign.ryoexamples/milestone2/error_missing_initializer.ryoexamples/milestone2/error_multiple.ryoexamples/milestone2/error_type_annotation.ryoexamples/milestone2/error_unexpected_token.ryoexamples/milestone2/expressions.ryoexamples/milestone2/mutable.ryoexamples/milestone2/simple.ryoexamples/milestone2/typed.ryoexamples/milestone3/README.mdexamples/milestone3/arithmetic.ryoexamples/milestone3/exit_code_future.ryoexamples/milestone3/exit_zero.ryoexamples/milestone3/multiple.ryoexamples/milestone3/parenthesized.ryoexamples/milestone3/simple.ryoexamples/square.ryosrc/ast.rssrc/astgen.rssrc/codegen.rssrc/diag.rssrc/lexer.rssrc/parser.rssrc/pipeline.rssrc/sema.rssrc/tir.rssrc/uir.rstests/integration_tests.rs
💤 Files with no reviewable changes (20)
- examples/milestone2/simple.ryo
- examples/milestone2/typed.ryo
- examples/milestone3/exit_zero.ryo
- examples/hello.ryo
- examples/milestone3/arithmetic.ryo
- examples/milestone2/error_unexpected_token.ryo
- examples/milestone2/error_multiple.ryo
- examples/milestone2/mutable.ryo
- examples/milestone2/error_missing_assign.ryo
- examples/milestone3/multiple.ryo
- examples/milestone2/error_missing_initializer.ryo
- examples/milestone2/README.md
- examples/milestone2/complete.ryo
- examples/milestone3/README.md
- examples/milestone2/error_type_annotation.ryo
- examples/milestone2/expressions.ryo
- examples/milestone3/parenthesized.ryo
- examples/milestone2/error_invalid_syntax.ryo
- examples/milestone3/exit_code_future.ryo
- examples/milestone3/simple.ryo
Comment on lines
+709
to
+713
| **Goal:*** Implement `assert` | ||
| - Add `assert` function: | ||
| ```ryo | ||
| fn assert(condition: bool, message: str) | ||
| ``` |
There was a problem hiding this comment.
Fix markdownlint issues in the new M8b2 section.
There are a few formatting issues: malformed bold on Line 709, fenced blocks without language tags, and an accidental setext heading caused by --- immediately after text.
🧹 Proposed markdown cleanup
-**Goal:*** Implement `assert`
+**Goal:** Implement `assert`
-```
+```text
AssertStmt { cond: Expr, msg: StringLit, span: Span }- +ryo
if not cond:
__ryo_assert_failed("", , )
-```
+```text
assertion failed at tests/arith.ryo:3: arithmetic is broken
That single primitive unlocks the rest of your milestone validation work — every future feature gets tested with the same tool.
+
</details>
As per coding guidelines: Code examples in documentation must use fenced code blocks with language tag.
Also applies to: 738-740, 748-751, 795-808
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/dev/implementation_roadmap.md around lines 709 - 713, Fix the malformed
markdown in the M8b2 section: correct the bolding around "Goal: Implement
assert" so it uses proper Markdown (e.g., "Goal: Implement assert"), ensure
all fenced code blocks include language tags (use ryo for ryo examples and text
for plain output) such as the blocks showing AssertStmt, the ryo snippet for the
assert runtime, and the sample assertion failure line, and remove or move the
stray '---' setext underline so it doesn't form an accidental heading (replace
it with a blank line or move it outside the paragraph). Reference the AssertStmt
struct name and the assert function/signature (fn assert(condition: bool,
message: str)) when updating the three affected code blocks noted (around lines
referenced in the comment) to ensure consistent language tags and correct
formatting.
</details>
<!-- fingerprinting:phantom:poseidon:hawk:23973eb4-e14e-4d1b-88b5-2b80a4da8343 -->
<!-- d98c2f50 -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+766
to
+775
| fn main() -> int: | ||
| assert(2 + 2 == 4, "arithmetic is broken") | ||
| assert(not false, "negation is broken") | ||
|
|
||
| let x = 10 | ||
| let y = 3 | ||
| assert(x % y == 1, "modulo wrong") | ||
|
|
||
| return 0 | ||
| ``` |
There was a problem hiding this comment.
Update the assert usage snippet to current Ryo syntax.
This example currently uses fn main() -> int, return 0, and let, which conflicts with the M8a rules documented in this same roadmap (fn main(): and assignment without let). As written, this snippet is likely to fail if copied into current tests.
💡 Proposed doc fix
-```ryo
-fn main() -> int:
- assert(2 + 2 == 4, "arithmetic is broken")
- assert(not false, "negation is broken")
-
- let x = 10
- let y = 3
- assert(x % y == 1, "modulo wrong")
-
- return 0
-```
+```ryo
+fn main():
+ assert(2 + 2 == 4, "arithmetic is broken")
+ assert(not false, "negation is broken")
+
+ x = 10
+ y = 3
+ assert(x % y == 1, "modulo wrong")
+```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/dev/implementation_roadmap.md` around lines 766 - 775, Update the Ryo
snippet so it uses current M8a syntax: change the function signature from "fn
main() -> int" to "fn main():", remove the "let" declarations (use "x = 10" and
"y = 3" instead of "let x = 10"/"let y = 3"), drop the "return 0" line, and
ensure the fenced code block is marked as "```ryo" and the assert calls remain
intact (refer to fn main, assert, x, y in the snippet).
Comment on lines
+199
to
203
| You should see the AST, codegen output, then: | ||
| ``` | ||
| [Input Source] | ||
| x = 42 | ||
|
|
||
| [AST] | ||
| Program (0..6) | ||
| └── Statement [VarDecl] (0..6) | ||
| VarDecl | ||
| ├── name: x (0..1) | ||
| └── initializer: | ||
| Literal(Int(42)) (4..6) | ||
|
|
||
| [Codegen] | ||
| Hello, Ryo! | ||
| [Result] => 0 | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to this fenced block.
This fence is missing a language identifier, which is why markdownlint is flagging MD040. text or console would both work here.
Minimal fix
-```
+```text
Hello, Ryo!
[Result] => 0</details>
As per coding guidelines, code examples in documentation must use fenced code blocks with language tag (e.g., ````ryo`).
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
You should see the AST, codegen output, then:
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 200-200: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 199 - 203, The fenced code block showing the program
output (the block containing "Hello, Ryo!" and "[Result] => 0") is missing a
language tag; update that fenced block in README.md to include an appropriate
language identifier (for example add "text", "console", or the project-specific
"ryo") so the fence becomes ```text (or ```console/```ryo) to satisfy
markdownlint rule MD040 and the project's documentation guidelines.
Comment on lines
+114
to
126
| #[derive(Debug, Clone, PartialEq)] | ||
| pub struct IfStmt { | ||
| pub cond: Expression, | ||
| pub then_block: Vec<Statement>, | ||
| pub elif_branches: Vec<ElifBranch>, | ||
| pub else_block: Option<Vec<Statement>>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub struct ElifBranch { | ||
| pub cond: Expression, | ||
| pub block: Vec<Statement>, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add SimpleSpan to the new control-flow AST nodes.
IfStmt and ElifBranch are AST nodes, but unlike the rest of this layer they don't carry their own span. That makes it impossible to point diagnostics at a specific branch header later and breaks the AST shape expected by the repo guidelines.
Suggested shape
pub struct IfStmt {
+ pub span: SimpleSpan,
pub cond: Expression,
pub then_block: Vec<Statement>,
pub elif_branches: Vec<ElifBranch>,
pub else_block: Option<Vec<Statement>>,
}
pub struct ElifBranch {
+ pub span: SimpleSpan,
pub cond: Expression,
pub block: Vec<Statement>,
}As per coding guidelines, src/ast.rs: AST nodes must include span: SimpleSpan for error reporting and define variants in StmtKind or ExprKind.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[derive(Debug, Clone, PartialEq)] | |
| pub struct IfStmt { | |
| pub cond: Expression, | |
| pub then_block: Vec<Statement>, | |
| pub elif_branches: Vec<ElifBranch>, | |
| pub else_block: Option<Vec<Statement>>, | |
| } | |
| #[derive(Debug, Clone, PartialEq)] | |
| pub struct ElifBranch { | |
| pub cond: Expression, | |
| pub block: Vec<Statement>, | |
| } | |
| #[derive(Debug, Clone, PartialEq)] | |
| pub struct IfStmt { | |
| pub span: SimpleSpan, | |
| pub cond: Expression, | |
| pub then_block: Vec<Statement>, | |
| pub elif_branches: Vec<ElifBranch>, | |
| pub else_block: Option<Vec<Statement>>, | |
| } | |
| #[derive(Debug, Clone, PartialEq)] | |
| pub struct ElifBranch { | |
| pub span: SimpleSpan, | |
| pub cond: Expression, | |
| pub block: Vec<Statement>, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ast.rs` around lines 114 - 126, The IfStmt and ElifBranch AST structs are
missing a span field; add pub span: SimpleSpan to both IfStmt and ElifBranch
definitions so control-flow nodes carry source spans, then update all places
that construct or pattern-match these nodes (parser functions/constructors that
create IfStmt and ElifBranch, any StmtKind::If creation sites, and any code that
destructures them) to provide and propagate the SimpleSpan value. Ensure you
also update any imports to bring SimpleSpan into scope and adjust tests/uses
expecting the old shape.
Comment on lines
+605
to
+647
| pub fn if_stmt_view(&self, r: TirRef) -> TirIfStmtView { | ||
| let inst = self.inst(r); | ||
| debug_assert!(matches!(inst.tag, TirTag::IfStmt)); | ||
| let range = match inst.data { | ||
| TirData::Extra(rng) => rng, | ||
| _ => unreachable!("IfStmt must carry TirData::Extra"), | ||
| }; | ||
| let slice = &self.extra[range.as_range()]; | ||
| let mut pos = 0; | ||
|
|
||
| let cond = TirRef::from_raw(slice[pos]); | ||
| pos += 1; | ||
|
|
||
| let then_stmts = read_ref_list(slice, &mut pos); | ||
|
|
||
| let elif_count = slice[pos] as usize; | ||
| pos += 1; | ||
| let mut elif_branches = Vec::with_capacity(elif_count); | ||
| for _ in 0..elif_count { | ||
| let elif_cond = TirRef::from_raw(slice[pos]); | ||
| pos += 1; | ||
| let body = read_ref_list(slice, &mut pos); | ||
| elif_branches.push(TirElifView { | ||
| cond: elif_cond, | ||
| body, | ||
| }); | ||
| } | ||
|
|
||
| let has_else = slice[pos] != 0; | ||
| pos += 1; | ||
| let else_stmts = if has_else { | ||
| Some(read_ref_list(slice, &mut pos)) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| TirIfStmtView { | ||
| cond, | ||
| then_stmts, | ||
| elif_branches, | ||
| else_stmts, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Harden if_stmt_view decoding with checked position/range math.
read_ref_list currently advances/slices with unchecked usize arithmetic and direct indexing, so malformed payloads can panic in non-local ways. Add checked adds and bounds-checked access for clearer failure boundaries and no silent index overflow.
Suggested defensive decode update
pub fn if_stmt_view(&self, r: TirRef) -> TirIfStmtView {
@@
- TirIfStmtView {
+ let out = TirIfStmtView {
cond,
then_stmts,
elif_branches,
else_stmts,
- }
+ };
+ debug_assert_eq!(pos, slice.len(), "Malformed IfStmt payload: trailing data");
+ out
}
}
fn read_ref_list(slice: &[u32], pos: &mut usize) -> Vec<TirRef> {
- let count = slice[*pos] as usize;
- *pos += 1;
- let refs = slice[*pos..*pos + count]
+ let count = *slice
+ .get(*pos)
+ .expect("Malformed IfStmt payload: missing list length") as usize;
+ *pos = pos.checked_add(1).expect("IfStmt decode position overflow");
+ let end = (*pos)
+ .checked_add(count)
+ .expect("IfStmt decode position overflow");
+ let refs = slice
+ .get(*pos..end)
+ .expect("Malformed IfStmt payload: list exceeds payload")
.iter()
.copied()
.map(TirRef::from_raw)
.collect();
- *pos += count;
+ *pos = end;
refs
}Also applies to: 650-659
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tir.rs` around lines 605 - 647, The if_stmt_view decoder performs
unchecked indexing and usize arithmetic (direct slice[pos], range.as_range(),
read_ref_list usage) which can panic on malformed input; update if_stmt_view to
validate the extra range before slicing, use get(...) or checked indexing for
every slice access (including reading cond, elif_count, has_else), replace
unchecked increments with checked_add/checked_add_opt and bounds checks on pos
before each read, propagate errors by changing the signature to return a
Result<TirIfStmtView, DecodeError> (or use Option) and adjust calls, and
similarly harden any adjacent decoding code that uses read_ref_list or direct
indexing (e.g., the nearby block referenced at lines ~650-659) so all reads use
read_ref_list safely and bounds-checked operations instead of unchecked indexing
and arithmetic.
…blocks Prevent variable declarations inside if/elif/else branches from leaking past the merge block by saving and restoring ctx.locals via a new emit_scoped_body helper. Fix indented_block parser to require newlines between statements, matching top-level behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Aug 3, 2026
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
1 participant
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.
Implements conditionals (
if/elif/else) and logical operators (and,or,not) across the full compiler pipeline.Summary by CodeRabbit
Release Notes
New Features
if/elif/elsesupportand,or,notDocumentation
assertstatements to development roadmap