-
Notifications
You must be signed in to change notification settings - Fork 200
for loops implementation #2117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ba11b0y
wants to merge
5
commits into
antonio/vm
Choose a base branch
from
rahul/for-loops
base: antonio/vm
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
for loops implementation #2117
+482
−165
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 ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 9eb1e89 in 2 minutes and 15 seconds. Click for details.
- Reviewed
1019
lines of code in17
files - Skipped
0
files when reviewing. - Skipped posting
6
draft 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 by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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, including parsing, AST, bytecode, and VM execution, with tests.for
loop support in BAML, including parsing, AST representation, bytecode generation, and VM execution.for
loops iterate over arrays, pushing elements to the stack.for
loops are not yet supported.ForLoop
variant toStmt
enum instmt.rs
.parse.rs
andparse_expr.rs
to handlefor
loops.datamodel.pest
to includefor_loop
rule.CreateIterator
andIterNext
instructions tobytecode.rs
.CreateIterator
andIterNext
invm.rs
to handle array iteration.for
loop execution invm.rs
.This description was created by
for 9eb1e89. You can customize this summary. It will automatically update as commits are pushed.