for loops implementation #2117
Merged
for loops implementation #2117
Conversation
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 9eb1e89 in 2 minutes and 15 seconds. Click for details.
- Reviewed
1019lines of code in17files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-lib/ast/src/ast/stmt.rs:7
- Draft comment:
The new ForLoop variant looks clear. Remember that nested for loops may need careful handling of shadowing and scope cleanup – verify that the loop variable is removed correctly when exiting nested loops. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is informative and suggests verifying the handling of loop variables in nested for loops. It doesn't provide a specific suggestion or point out a specific issue with the code. It also asks the author to verify something, which is against the rules.
2. engine/baml-lib/ast/src/parser/parse_expr.rs:67
- Draft comment:
The parse_for_loop function correctly extracts the identifier, iterator, and body. However, since nested for loops are known to have issues, please add tests for nested loops and review how local scopes are handled to avoid variable clobbering. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is speculative - it warns about potential issues with nested loops without evidence that there are actual problems. The parse_for_loop function just handles the syntax parsing into an AST node. Scope handling would be handled by the interpreter/evaluator, not the parser. The comment goes beyond the scope of the parsing changes being made. Maybe there are actual scope handling issues that the reviewer is aware of from experience with the codebase. Maybe the AST structure itself needs to handle scopes differently. Without concrete evidence of scope issues in the current implementation, this is speculative. The parser's job is just to build the AST correctly, which this code appears to do. The comment should be deleted as it makes speculative suggestions about potential issues without evidence, and discusses concerns beyond the scope of the parsing changes.
3. engine/baml-lib/src/validate/validation_pipeline/validations/expr_fns.rs:105
- Draft comment:
In the validation of a for loop, a new scope is created by cloning the parent's scope and adding the loop variable. For deeply nested loops, ensure that inner-loop variables do not improperly shadow or persist. Consider testing edge cases. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. engine/baml-vm/src/vm.rs:757
- Draft comment:
The ITER_NEXT instruction pops an iterator and pushes the next element and has_next flag. Verify that nested loop iterations do not share or overwrite the iterator state. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify that nested loop iterations do not share or overwrite the iterator state. This falls under asking the author to double-check or ensure behavior, which is against the rules. Therefore, this comment should be removed.
5. engine/baml-vm/tests/vm.rs:250
- Draft comment:
While tests for for loops (for_loop_simple and for_loop_with_expressions) exist, there is no explicit test for nested for loops. Adding a nested loop test would help identify and fix the known issue. - Reason this comment was not posted:
Comment was on unchanged code.
6. engine/baml-vm/src/bytecode.rs:137
- Draft comment:
Typo in the TODO comment: the phrase "if this insn is complex than needed" seems off. Consider rephrasing it to "if this insn is more complex than needed." - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the grammar suggestion is technically correct, TODO comments are temporary by nature and will be removed when the actual work is done. The meaning is clear despite the minor grammatical error. This kind of nitpicking on grammar in temporary comments doesn't add value and could be seen as noise. The grammar is indeed incorrect, and clear communication is important in code comments. Maybe improving clarity in comments helps future developers. While clear communication is important, this is a temporary TODO comment that will be removed soon. The meaning is already clear enough, and focusing on grammar in TODOs is excessive. Delete this comment as it's an unnecessary nitpick about grammar in a temporary TODO comment that will be removed anyway.
Workflow ID: wflow_pOwmbeKTJFZXc6kA
You can customize 
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 47630b8 in 10 minutes and 34 seconds. Click for details.
- Reviewed
330lines of code in9files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-lib/validate/validation_pipeline/validations/expr_fns.rs:106
- Draft comment:
When validating a ForLoop statement, consider also checking that the iterator expression returns an iterable type. This additional check would catch cases where a non‐iterable is used and improve feedback to the user. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. engine/baml-lib/validate/validation_pipeline/validations/expr_fns.rs:79
- Draft comment:
In a ForLoop, after validating a statement in the loop body, the identifier is inserted into the loop scope. Verify that this push is correct if multiple statements define the same variable (i.e. handling of shadowing) and consider reporting a duplicate definition error if necessary. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. engine/baml-lib/ast/src/parser/parse_expr.rs:67
- Draft comment:
The parse_for_loop function cleanly parses a for‐loop by gathering the identifier, iterator, and block body. Confirm that the resulting AST correctly distinguishes loop constructs from let bindings in downstream processing. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. engine/baml-vm/tests/vm.rs:260
- Draft comment:
The tests for for loops (simple, nested, and with expressions) look good. Consider later adding tests that inspect side‑effects or the actual loop variable values to ensure the iteration semantics work as intended, especially for nested loops. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is purely informative and suggests adding tests in the future, which violates the rule against asking the PR author to ensure behavior is intended or to make sure their change is tested. It doesn't provide a specific suggestion or point out a specific issue with the current code.
5. engine/baml-lib/baml-core/src/ir/repr.rs:309
- Draft comment:
The IR conversion in convert_function_body folds over statements using stmt.expr(). For ForLoop statements this uses the loop body expression. Verify that this lowering captures the intended control flow of for loops—i.e. that the iterative behavior is correctly implemented and not merely treated as a simple let binding. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_VI1GIk2zGBto0LAI
You can customize
engine/baml-lib/ast/src/ast/stmt.rs
Outdated
| @@ -69,10 +95,31 @@ impl Stmt { | |||
| } | |||
| } | |||
|
|
|||
| pub fn body(&self) -> &Expression { | |||
| pub fn body(&self) -> &ExpressionBlock { | |||
There was a problem hiding this comment.
The helper methods (identifier, iterator, expr, statements) for ForLoop panic when called on Let statements. While acceptable in a controlled setting, consider a more graceful error reporting mechanism to avoid unexpected panics in production.
There was a problem hiding this comment.
Skipped PR review on 85db7c2 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
engine/baml-compiler/src/lib.rs
Outdated
| globals, | ||
| }; | ||
|
|
||
| let result = vm.exec()?; |
There was a problem hiding this comment.
This is the compiler crate, tests here should only check that the compiler emits the correct instructions. Tests that actually need to run the VM itself should be in the VM crate.
engine/baml-compiler/src/lib.rs
Outdated
Comment on lines
866
to
880
| if let Value::Object(index) = result { | ||
| if let Object::Iterator { | ||
| array, | ||
| index: iter_index, | ||
| } = &vm.objects[index] | ||
| { | ||
| assert_eq!(*iter_index, 0); // Should start at index 0 | ||
| // Array should be at index 1 (after function at index 0) | ||
| assert_eq!(*array, 1); | ||
| } else { | ||
| panic!("Expected Iterator object"); | ||
| } | ||
| } else { | ||
| panic!("Expected Object value"); | ||
| } |
There was a problem hiding this comment.
When you have this big if blocks with a tiny else statement at the end, use Rust's let-else construct:
let Value::Object(index) = result else {
panic!("Expected Object value");
};
let Object::Iterator { array, index: iter_index } = &vm.objects[index] else {
panic!("Expected Iterator object");
};
assert_eq(*iter_index, 0);
assert_eq(*array, 1);That way it's much easier to read each case and you don't have a lot of nesting. It's similar to early returns in functions.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed b848a5d in 1 minute and 32 seconds. Click for details.
- Reviewed
410lines of code in4files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-compiler/src/lib.rs:219
- Draft comment:
The for-loop compilation branch has complex stack manipulation (e.g. computing the loop variable’s slot as last_local + 2, multiple pops, and jump patching). Consider refactoring or adding more detailed comments to clarify the expected stack state, especially for nested loops. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. engine/baml-vm/src/vm.rs:809
- Draft comment:
In the IterNext instruction, the iterator index is incremented even when the array is exhausted. Confirm if this behavior is intended; otherwise, consider not advancing the index when no element is yielded. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. engine/baml-vm/src/vm.rs:612
- Draft comment:
The repeated pattern of restoring the function reference after operations (e.g. after AllocArray, AllocInstance, CreateIterator, IterNext) suggests a candidate for refactoring into a helper. This would reduce boilerplate and mitigate potential issues from vector reallocation. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. engine/baml-vm/tests/vm.rs:390
- Draft comment:
Consider adding tests for error conditions in for-loop constructs, such as passing a non-array to a for-loop, to ensure robust error handling. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
5. engine/baml-compiler/src/lib.rs:242
- Draft comment:
The local variable assignment in the for-loop (using 'last_local_index + 2') appears ad hoc. Verify that this approach consistently reserves the correct slot for the loop variable, especially when loops are nested or multiple locals are declared. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_IXAubmbMulErjeuy
You can customize
engine/baml-lib/ast/src/ast/stmt.rs
Outdated
Comment on lines
98
to
124
| pub fn body(&self) -> &ExpressionBlock { | ||
| match self { | ||
| Stmt::Let(_) => panic!("Let statement does not have a body"), | ||
| Stmt::ForLoop(stmt) => &stmt.body, | ||
| } | ||
| } | ||
|
|
||
| pub fn expr(&self) -> &Expression { | ||
| match self { | ||
| Stmt::Let(stmt) => &stmt.expr, | ||
| Stmt::ForLoop(stmt) => &stmt.body.expr, | ||
| } | ||
| } | ||
|
|
||
| pub fn iterator(&self) -> &Expression { | ||
| match self { | ||
| Stmt::Let(_) => panic!("Let statement does not have an iterator"), | ||
| Stmt::ForLoop(stmt) => &stmt.iterator, | ||
| } | ||
| } | ||
|
|
||
| pub fn statements(&self) -> &Vec<Stmt> { | ||
| match self { | ||
| Stmt::Let(_) => panic!("Let statement does not have statements"), | ||
| Stmt::ForLoop(stmt) => &stmt.body.stmts, | ||
| } | ||
| } |
There was a problem hiding this comment.
You don't need any of these functions, notice how all of them panic! if called with the wrong variant of Stmt. Usually you'll match over a Stmt and at that point the variant is narrowed down, so you'll never check that a let statement has an iterator. You'll do:
match statement {
Stmt::ForLoop(stmt) => {
stmt.iterator // This exists here because we know the type.
}
Stmt::Let(stmt) => {
// Again we know the type of stmt so we can access fields.
}
}Plus, if you match over a statement and then need to match over the same statement again to grab a field though a getter, it's just redundant code and redundant checks at runtime.
// Only one variant check
match statement {
Stmt::ForLoop(stmt) => stmt.iterator
}
// This is 2 checks for no reason
match statement {
Stmt::ForLoop(_) => statement.iterator()
}
// Because iterator is defined as:
fn iterator(&self) {
match self {
Stmt::Let(_) => panic!("Let statemetn does not have an iterator"
Stmt::ForLoop(_) => &stmt.iterator
}
}Don't be afraid of public fields in structs. You don't need getters everywhere a la Java. Private fields with getters are nice if you're exposing some user facing API in a library. Other than that, in our internal code, we control how fields are accessed so no need for boilerplate @ba11b0y .
There was a problem hiding this comment.
ah makes sense. My bad, I come from a world guarding access patterns.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 45674a2 in 2 minutes and 17 seconds. Click for details.
- Reviewed
85lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-compiler/src/lib.rs:205
- Draft comment:
In the Let branch, you pattern match to bindstmtbut still callstatement.identifier(). For consistency, consider usingstmt.identifierinstead. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. engine/baml-compiler/src/lib.rs:218
- Draft comment:
The ForLoop branch now usesstmt, which is good; however, the TODO regarding nested for loops remains. The local-index calculation (using max() + 2) may not work when loops are nested. Consider revising local tracking and adding tests for nested for loops. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. engine/baml-lib/ast/src/ast/stmt.rs:7
- Draft comment:
The fields in LetStmt and ForLoopStmt are now public. Consider whether accessor methods might be preferable in order to preserve encapsulation. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. engine/baml-compiler/src/lib.rs:208
- Draft comment:
Typographical Note: In the 'Let' arm, you destructure the statement as 'stmt', yet on line 208 you use 'statement.identifier()' instead of 'stmt.identifier()'. For clarity and consistency, consider using 'stmt.identifier().to_string()' here. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_dLGuRNMumSuA1j8x
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 58fe2bb in 1 minute and 36 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-compiler/src/lib.rs:247
- Draft comment:
Pass a reference to the loop body using &stmt.body to avoid consuming/copying the body. This fix helps nested for loops work correctly. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. engine/baml-compiler/src/lib.rs:247
- Draft comment:
Consider using consistent variable naming (e.g. use 'stmt' throughout) in this ForLoop arm for clarity. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_2ej9e4io5HCXVbWG
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed b534017 in 1 minute and 38 seconds. Click for details.
- Reviewed
146lines of code in7files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-lib/parser-database/src/walkers/expr_fn.rs:51
- Draft comment:
In walk_tests, self.name() is called repeatedly inside the filter. Consider caching it in a local variable to avoid redundant lookups and improve clarity. For example: let fn_name = self.name(); let mut tests = self.db.walk_test_cases().filter(|w| w.test_case().functions.iter().any(|f| f.0 == fn_name)).collect::<Vec<_>>(); - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. engine/baml-lib/parser-database/src/walkers/expr_fn.rs:12
- Draft comment:
The top-level assignment walker's name() method directly indexes into the AST using self.id. Ensure that id stability is enforced elsewhere, as any inconsistency could lead to panics. - Reason this comment was not posted:
Comment was on unchanged code.
3. engine/baml-lib/ast/src/ast/top.rs:117
- Draft comment:
Typographical note: In the TopLevelAssignment arm, you changedx.stmt.identifier()to&x.stmt.identifier. This may be an inadvertent omission of the parentheses. Ifidentifieris meant to be a method call (as in the other variants), consider restoring the()to ensure consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The code shows a consistent pattern where some types use method calls (like Enum, Class, etc.) while others use direct field access (ExprFn). The change to TopLevelAssignment makes it consistent with ExprFn's style. The comment assumes all variants should use method calls, but that's not necessarily true - there appear to be two valid patterns in use. I could be wrong about the intentionality of these two patterns - maybe the field access is actually a bug that should be fixed everywhere. However, since both patterns exist in the code and the change makes TopLevelAssignment consistent with at least one other case (ExprFn), I cannot be confident that this is a mistake that needs fixing. The comment should be deleted because it assumes a consistency that doesn't actually exist in the codebase, and the change could be intentionally aligning with an alternate valid pattern.
Workflow ID: wflow_D8NuWDJ0EZCnYVwR
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed f860090 in 1 minute and 23 seconds. Click for details.
- Reviewed
116lines of code in3files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-lib/ast/src/ast/stmt.rs:22
- Draft comment:
Removed ForLoopStmt::new and related accessor methods. Ensure all call sites now use direct field access consistently and consider adding documentation for this API change. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. engine/baml-lib/ast/src/ast/stmt.rs:80
- Draft comment:
The removal of accessor methods (body(), iterator(), statements()) implies that client code must now directly access struct fields. Verify that all usages are updated accordingly. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. engine/baml-lib/ast/src/parser/parse_expr.rs:83
- Draft comment:
Using a direct struct literal for ForLoopStmt improves clarity. Confirm that this construction is consistent across the codebase. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. engine/baml-lib/baml-core/src/validate/validation_pipeline/validations/expr_fns.rs:101
- Draft comment:
Field accesses for ForLoopStmt have been updated in validate_stmt. Verify that the updated scope handling (inserting the loop variable and each statement’s identifier) meets the intended semantics, especially for nested for loops. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. engine/baml-lib/baml-core/src/validate/validation_pipeline/validations/expr_fns.rs:110
- Draft comment:
Check the scoping logic in the for-loop validation: after validating each statement in the loop body, its identifier is added to the scope. Ensure this matches the desired variable shadowing behavior for nested for loops. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_LgDJh0XKKLevTMGP
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed e5bd0bb in 1 minute and 47 seconds. Click for details.
- Reviewed
71lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/baml-lib/baml-core/src/ir/repr.rs:2229
- Draft comment:
ForLoop match arm: confirm that the 'item' field intentionally isn’t annotated (i.e. doesn’t need recursive processing). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. engine/baml-lib/baml-core/src/ir/repr.rs:2747
- Draft comment:
In specialize_generics, verify whether generics on the loop variable ('item') should be processed similar to lambda parameters. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. engine/baml-lib/baml-core/src/ir/repr.rs:2920
- Draft comment:
Consider adding dedicated unit tests for the new ForLoop IR variant to ensure its behavior in various scenarios. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_FIEf2smCVSX1k40y
You can customize
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
TODO: nested for loops don't work right now.
Important
Implement for loop support in Baml compiler and VM, adding iterator instructions and tests.
lib.rs, handling iterator creation and loop execution.CreateIteratorandIterNextinstructions inbytecode.rsfor iterator operations.vm.rsto handle new instructions and manage iterators.vm.rsandtests/vm.rs.debug.rsto display new instructions.This description was created by
for e5bd0bb. You can customize this summary. It will automatically update as commits are pushed.