Method Declarations & Trailing Exprs & Constructors #2337
Merged
+1,132
−499
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
antoniosarosi
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
antoniosarosi
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
antoniosarosi
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
🌿 Preview your docs: https://boundary-preview-4579a768-77ff-4ae6-85e8-602e5cc97af0.docs.buildwithfern.com |
![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 78384be in 2 minutes and 4 seconds. Click for details.
- Reviewed
499lines of code in15files - 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/tests/validation_files/expr/method_decl.baml:1
- Draft comment:
Test file covers basic method decl; consider adding tests with complex bodies and error cases. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. engine/baml-compiler/src/codegen.rs:380
- Draft comment:
Typographical error: The TODO comment says "Hanlde field & array accessors". Please change "Hanlde" to "Handle". - 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 rules say to only keep comments that require code changes and are clearly correct. While this is technically a code change, it's an extremely minor typo fix in a TODO comment. TODO comments are temporary by nature and will be removed when the actual functionality is implemented. The rules don't explicitly address typos in comments, but the spirit seems to be focused on substantive code issues rather than cosmetic fixes. The typo is real and the fix is correct. One could argue that maintaining clean, professional code includes fixing even minor typos when found. While the fix is correct, this is an extremely low-value change that doesn't impact functionality or readability in any meaningful way. The TODO comment is temporary and will be removed when field/array accessors are implemented. The comment should be deleted. While technically correct, fixing a typo in a temporary TODO comment is too minor to warrant a PR comment and doesn't align with the focus on substantive code issues.
3. engine/baml-lib/ast/src/parser/datamodel.pest:334
- Draft comment:
Typo: In the comment, "Needs to supports things likeobject.getter().field = value..." should be "Needs to support things like...". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the comment is technically correct about the grammar, it's an extremely minor documentation fix that doesn't affect functionality. The meaning is perfectly clear either way. Our rules state we should not make purely informative comments or obvious/unimportant suggestions. The comment is accurate and would improve the code quality slightly. Documentation quality is important for maintainability. While documentation quality matters, this is such a minor grammatical issue that it creates more noise than value. The meaning is not impacted at all. Delete this comment as it's too minor and doesn't affect functionality or clarity in any meaningful way.
Workflow ID: wflow_KVnFVChRPMLq1Vh6
You can customize 
Review Summary🏷️ Draft Comments (2)
🔍 Comments beyond diff scope (1)
|
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
engine/baml-compiler/src/codegen.rs
Outdated
Comment on lines
377
to
387
| hir::Statement::Assign { left, value, .. } => { | ||
| self.compile_expression(value); | ||
|
|
||
| // TODO: Hanlde field & array accessors. | ||
| let name = match left { | ||
| hir::Expression::Identifier(name, _) => name, | ||
| _ => panic!("left side of assignment is not an identifier: {:?}", left), | ||
| }; | ||
|
|
||
| self.emit(Instruction::StoreVar(self.locals[name])); | ||
| } |
There was a problem hiding this comment.
correctness: Assign and AssignOp statement handling assumes left side is always an identifier, panicking at runtime for field or array assignments, causing crashes for valid code like a[0] = 1.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In engine/baml-compiler/src/codegen.rs, lines 377-387, the handling of `hir::Statement::Assign` panics if the left side is not an identifier, which will crash for valid assignments like `a[0] = 1` or `obj.field = 2`. Update this match arm to support array and field assignments by compiling the left-hand side as an lvalue and emitting the appropriate `StoreArrayElement` or `StoreField` instructions. Implement field index lookup as needed. Do not panic for these valid cases.
Comment on lines
391
to
394
| let name = match left { | ||
| hir::Expression::Identifier(name, _) => name, | ||
| _ => panic!("left side of assignment is not an identifier: {:?}", left), | ||
| }; |
There was a problem hiding this comment.
correctness: Assign and AssignOp statements only support identifiers on the left side; using a field or array access will panic at runtime.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In engine/baml-compiler/src/thir/typecheck.rs, lines 391-394, the code panics if the left side of an assignment is not an identifier. This will cause a runtime crash if a user writes code like `obj.field = 1` or `arr[0] = 2`. Instead, report a typechecking error and return None for unsupported assignment targets. Replace the panic with a diagnostics error and early return.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| let name = match left { | |
| hir::Expression::Identifier(name, _) => name, | |
| _ => panic!("left side of assignment is not an identifier: {:?}", left), | |
| }; | |
| let name = match left { | |
| hir::Expression::Identifier(name, _) => name, | |
| // TODO: Support field and array assignment (e.g., obj.field = ... or arr[0] = ...) | |
| _ => { | |
| diagnostics.push_error(DatamodelError::new_validation_error( | |
| "Assignment target must be a variable identifier (field and array assignments are not yet supported)", | |
| left.span(), | |
| )); | |
| return None; | |
| } | |
| }; |
engine/baml-lib/ast/src/ast/stmt.rs
Outdated
Comment on lines
179
to
197
| Stmt::Break(_) => panic!("break statements don't have identifiers"), | ||
| Stmt::Continue(_) => panic!("continue statements don't have identifiers"), | ||
| Stmt::CForLoop(_) => panic!("c-like for loops don't have identifiers"), | ||
| Stmt::Assign(stmt) => &stmt.identifier, | ||
| Stmt::AssignOp(stmt) => &stmt.identifier, | ||
| Stmt::Assign(stmt) => match &stmt.left { | ||
| Expression::Identifier(id) => id, | ||
| _ => panic!( | ||
| "left side of assignment is not an identifier: {:?}", | ||
| stmt.left | ||
| ), | ||
| }, | ||
| Stmt::AssignOp(stmt) => match &stmt.left { | ||
| Expression::Identifier(id) => id, | ||
| _ => panic!( | ||
| "left side of assignment is not an identifier: {:?}", | ||
| stmt.left | ||
| ), | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
correctness: Stmt::identifier() panics if the left side of assignment is not an identifier, which can cause runtime crashes for valid assignment targets like destructuring or member access.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In engine/baml-lib/ast/src/ast/stmt.rs, lines 173-197, the `Stmt::identifier()` method panics if the left side of an assignment is not an identifier, which can cause runtime crashes for valid assignment targets like destructuring or member access. Update the method so that it only allows identifier left-hand sides and provides a clear panic message indicating that this method is only valid for simple assignments to identifiers.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| Stmt::Break(_) => panic!("break statements don't have identifiers"), | |
| Stmt::Continue(_) => panic!("continue statements don't have identifiers"), | |
| Stmt::CForLoop(_) => panic!("c-like for loops don't have identifiers"), | |
| Stmt::Assign(stmt) => &stmt.identifier, | |
| Stmt::AssignOp(stmt) => &stmt.identifier, | |
| Stmt::Assign(stmt) => match &stmt.left { | |
| Expression::Identifier(id) => id, | |
| _ => panic!( | |
| "left side of assignment is not an identifier: {:?}", | |
| stmt.left | |
| ), | |
| }, | |
| Stmt::AssignOp(stmt) => match &stmt.left { | |
| Expression::Identifier(id) => id, | |
| _ => panic!( | |
| "left side of assignment is not an identifier: {:?}", | |
| stmt.left | |
| ), | |
| }, | |
| } | |
| } | |
| pub fn identifier(&self) -> &Identifier { | |
| match self { | |
| Stmt::Let(stmt) => &stmt.identifier, | |
| Stmt::ForLoop(stmt) => &stmt.identifier, | |
| Stmt::Expression(_) => panic!("expressions don't have identifiers"), | |
| Stmt::WhileLoop(_) => panic!("while loops don't have identifiers"), | |
| Stmt::Break(_) => panic!("break statements don't have identifiers"), | |
| Stmt::Continue(_) => panic!("continue statements don't have identifiers"), | |
| Stmt::CForLoop(_) => panic!("c-like for loops don't have identifiers"), | |
| Stmt::Assign(stmt) => { | |
| if let Expression::Identifier(id) = &stmt.left { | |
| id | |
| } else { | |
| panic!("identifier() called on assignment with non-identifier left-hand side; this method is only valid for simple assignments to identifiers") | |
| } | |
| }, | |
| Stmt::AssignOp(stmt) => { | |
| if let Expression::Identifier(id) = &stmt.left { | |
| id | |
| } else { | |
| panic!("identifier() called on assignment-op with non-identifier left-hand side; this method is only valid for simple assignments to identifiers") | |
| } | |
| }, | |
| } | |
| } |
Comment on lines
+336
to
+337
| assign_stmt = { expression ~ "=" ~ expression } | ||
| assign_op_stmt = { expression ~ assign_op ~ expression } |
There was a problem hiding this comment.
correctness: assign_stmt and assign_op_stmt now allow any expression on the left side, which permits invalid assignments to non-assignable expressions (e.g., literals, function calls), breaking assignment semantics.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In engine/baml-lib/ast/src/parser/datamodel.pest, lines 336-337, the assignment rules now allow any expression on the left-hand side, which permits assignments to non-assignable expressions (like literals or function calls). Restrict the left-hand side to only valid assignable expressions (identifiers, field accessors, array accessors, or method calls). Update the grammar to use an `assignable_expression` rule for the left side of assignments.
Comment on lines
65
to
82
| } | ||
| } | ||
|
|
||
| if is_self { | ||
| r#type = Some(BlockArg { | ||
| is_mutable, | ||
| is_self, | ||
| span: name | ||
| .as_ref() | ||
| .map(|ident| ident.span().to_owned()) | ||
| .unwrap_or(Span::fake()), | ||
| field_type: FieldType::Symbol( | ||
| FieldArity::Required, | ||
| Identifier::Local("Self".to_string(), Span::fake()), | ||
| None, | ||
| ), | ||
| }); | ||
| } |
There was a problem hiding this comment.
correctness: is_self is set to true if the identifier is 'self', but the code does not prevent a user from naming a non-self argument 'self', which could cause incorrect type assignment.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In engine/baml-lib/ast/src/parser/parse_named_args_list.rs, lines 50-82: The current logic sets `is_self` to true if the identifier is 'self', but does not prevent a user from naming a non-self argument 'self', which could cause incorrect type assignment. Update the code so that 'self' is only allowed as the first argument, and if 'self' appears elsewhere, push a validation error. Ensure that only the first argument named 'self' is treated as the special self parameter.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| } | |
| } | |
| if is_self { | |
| r#type = Some(BlockArg { | |
| is_mutable, | |
| is_self, | |
| span: name | |
| .as_ref() | |
| .map(|ident| ident.span().to_owned()) | |
| .unwrap_or(Span::fake()), | |
| field_type: FieldType::Symbol( | |
| FieldArity::Required, | |
| Identifier::Local("Self".to_string(), Span::fake()), | |
| None, | |
| ), | |
| }); | |
| } | |
| if ident.name() == "self" { | |
| is_self = true; | |
| name = Some(ident); | |
| } else { | |
| name = Some(ident); | |
| } | |
| ... | |
| if is_self { | |
| // Only allow 'self' as the first argument | |
| if args.is_empty() { | |
| r#type = Some(BlockArg { | |
| is_mutable, | |
| is_self, | |
| span: name | |
| .as_ref() | |
| .map(|ident| ident.span().to_owned()) | |
| .unwrap_or(Span::fake()), | |
| field_type: FieldType::Symbol( | |
| FieldArity::Required, | |
| Identifier::Local("Self".to_string(), Span::fake()), | |
| None, | |
| ), | |
| }); | |
| } else { | |
| diagnostics.push_error(DatamodelError::new_validation_error( | |
| "'self' can only be used as the first argument in a method.", | |
| name.as_ref().map(|n| n.span().clone()).unwrap_or(Span::fake()), | |
| )); | |
| } | |
| } |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ff7fcc8 in 2 minutes and 0 seconds. Click for details.
- Reviewed
111lines of code in2files - 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/parser/datamodel.pest:268
- Draft comment:
Added lookahead (~ !"=") to the arithmetic operators to avoid misinterpreting compound assignments. Verify that this pattern is uniformly applied. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. engine/baml-lib/ast/src/parser/datamodel.pest:275
- Draft comment:
Similarly, bitwise operators now include the same lookahead to prevent incorrect parsing of assignments. Confirm consistency for all operator rules. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. engine/baml-lib/ast/src/parser/datamodel.pest:337
- Draft comment:
The comment preceding 'assign_stmt' has been updated to clarify support for complex LHS expressions (like field access or array expressions). Ensure the grammar covers all intended use cases. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. engine/baml-vm/src/vm.rs:647
- Draft comment:
A block of debug print code has been commented out. Consider using #[cfg(debug_assertions)] for conditional compilation or removing the legacy debug code to reduce clutter. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. engine/baml-lib/ast/src/parser/datamodel.pest:337
- Draft comment:
Typo: 'supports' should be 'support' in the comment. - Reason this comment was not posted:
Comment was on unchanged code.
6. engine/baml-lib/ast/src/parser/datamodel.pest:341
- Draft comment:
Typo: 'treaded' should be 'treated' in the comment. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the comment is technically correct - there is a typo - fixing typos in comments is not a critical code change. The meaning is still clear despite the typo. The rules state we should only keep comments that require clear code changes, not purely informative or trivial fixes. The typo could theoretically make the code slightly harder to understand for non-native English speakers. Documentation quality does matter. While documentation quality matters, this typo is minor and the meaning remains clear. The rules explicitly state to avoid trivial comments. Delete this comment as it points out a trivial typo in a comment that doesn't impact code functionality.
Workflow ID: wflow_GMNS7KRXJ82sSjpY
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 493bddf in 1 minute and 35 seconds. Click for details.
- Reviewed
100lines 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/ast/src/parser/datamodel.pest:318
- Draft comment:
Stmt rule now accepts any expression; update outdated comment to reflect this change. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. engine/baml-lib/ast/src/parser/parse_expr.rs:346
- Draft comment:
Using 'Rule::expression' in statement parsing simplifies expression handling; verify that method-call constructs are parsed as intended. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. engine/baml-vm/tests/vm.rs:862
- Draft comment:
New tests for method binding and class method declarations improve coverage; consider adding tests for error cases as well. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_JF0fUNhzGHg587hK
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 1d15aaa in 1 minute and 56 seconds. Click for details.
- Reviewed
321lines of code in2files - 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/thir/typecheck.rs:1187
- Draft comment:
In the function subtype check for Arrow types, the parameters are checked using a_arrow.inputs.zip(e_arrow.inputs).all(|(a, e)| a.is_subtype(e)). For function types the parameters should be checked contravariantly (i.e. expected input should be a subtype of the actual input). - 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-compiler/src/thir/typecheck.rs:104
- Draft comment:
The context (TypeContext) is cloned (e.g. in the loop over expr functions) multiple times which may impact performance for large HIRs. Consider using shared references (rc/arc) or otherwise avoiding deep clones if possible. - 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-compiler/src/thir/typecheck.rs:1104
- Draft comment:
Binary and unary operation expressions are currently typechecked but the resulting meta type is set to None. It might be beneficial to infer and attach an appropriate result type for these operations. - 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-compiler/src/thir/typecheck.rs:829
- Draft comment:
The method call branch currently creates a FreeVar for the method name without proper type resolution. This is marked as a TODO, but be sure to handle method dispatch and type checking more robustly later. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. engine/baml-compiler/src/thir/typecheck.rs:672
- Draft comment:
When inferring the array type, only the first element’s type is used. In heterogeneous arrays this may be insufficient—consider validating all elements or enforcing homogeneous arrays. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_85ZCbmjUiYTsFZHH
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 0b4803b in 2 minutes and 44 seconds. Click for details.
- Reviewed
106lines of code in3files - 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-vm/tests/vm.rs:1155
- Draft comment:
The test for basic method declaration has a potential inconsistency – the source code returns a literal1, while the comment suggests returninga.value(which should be updated to 3 aftera.add(b)). Confirm the intended behavior and update the test accordingly. - 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/tests/vm.rs:271
- Draft comment:
Using the newlet ... elsepattern for matching object variants (e.g. in array and instance tests) is concise. Ensure that the minimum Rust version supported is documented, as this syntax requires a recent compiler version. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. engine/baml-vm/tests/vm.rs:398
- Draft comment:
Consider using assertion helpers (likeassert_matches!) for pattern matching on enum variants in tests to increase clarity and provide more informative error messages. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. engine/baml-compiler/src/codegen.rs:422
- Draft comment:
Typo: The comment says 'funcion body' instead of 'function body'. Please correct 'funcion' to 'function'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The rules state to only keep comments that require code changes. This is a typo in a comment, not in code. Comments about typos in comments are not useful enough to warrant a PR comment. The author can fix it if they notice it, but it doesn't impact functionality. Could fixing typos in comments help improve code readability and maintainability in the long run? Could this be considered a valid code quality improvement? While clear comments are good, fixing typos in comments is too minor to warrant a PR comment. The rules specifically say not to make purely informative comments or comments about obvious/unimportant things. A typo in a TODO comment is very low priority. Delete this comment. Typos in comments are too minor to warrant a PR comment and don't require code changes.
Workflow ID: wflow_knLniVNyngy7QWSk
You can customize
antoniosarosi
force-pushed
the
antonio/method-decls
branch
from
0b4803b to
493bddf
Compare
Co-authored-by: Luke Ramsden <hello@lukeramsden.com> Co-authored-by: aaronvg <aaron@boundaryml.com> Co-authored-by: hellovai <vbv@boundaryml.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> Co-authored-by: Jesús Lapastora <jesus@boundaryml.com> Co-authored-by: Samuel Lijin <sam@boundaryml.com> Co-authored-by: Ayush Goyal <ayushg1214@gmail.com> Co-authored-by: Chris Watts <chris@boundaryml.com>
antoniosarosi
had a problem deploying
to
boundary-tools-dev
GitHub Actions
Error
antoniosarosi
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
antoniosarosi
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
antoniosarosi
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
antoniosarosi
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 3572949 in 1 minute and 10 seconds. Click for details.
- Reviewed
30lines 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-vm/benches/fib.rs:25
- Draft comment:
Using StackIndex::from_raw(0) improves type safety over a raw integer for locals_offset. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. engine/baml-vm/benches/fib.rs:27
- Draft comment:
Wrapping the stack initialization in EvalStack::from_vec and converting runtime_allocs_offset with ObjectIndex::from_raw enforces consistent type safety. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_Fo29uTGFBPsmfsAc
You can customize
|
🌿 Preview your docs: https://boundary-preview-84459bcf-3c9d-4fcd-b7b0-2f10451a8ebf.docs.buildwithfern.com |
antoniosarosi
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
antoniosarosi
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
antoniosarosi
had a problem deploying
to
boundary-tools-dev
GitHub Actions
Error
|
🌿 Preview your docs: https://boundary-preview-a1675b43-2bed-4495-9806-4f48a5008296.docs.buildwithfern.com |
antoniosarosi
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
antoniosarosi
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
antoniosarosi
had a problem deploying
to
boundary-tools-dev
GitHub Actions
Error
|
🌿 Preview your docs: https://boundary-preview-83f1e8f3-a2d6-4f30-8e5d-51f67d7d5698.docs.buildwithfern.com |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 5f00df0 in 2 minutes and 16 seconds. Click for details.
- Reviewed
681lines of code in32files - Skipped
0files when reviewing. - Skipped posting
13draft 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/hir/dump.rs:291
- Draft comment:
Good update for handling trailing expressions in function bodies. Consider refactoring the re-assignment of 'body_doc' (lines 291–297) into a helper for clarity. - 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 code is not complex enough to warrant extraction into a helper function. It's a single logical operation (appending a trailing expression) that fits naturally in the to_doc() method. Creating a helper would add unnecessary abstraction and could make the code harder to follow by splitting related logic across multiple functions. The current implementation is clear and maintainable. Perhaps I'm underestimating the value of consistency - if other similar operations are extracted into helpers, this should be too. The nesting of append() calls could be seen as complex enough to warrant extraction. Looking at the rest of the file, similar operations are not extracted into helpers. The nesting pattern is common throughout the codebase and developers would be familiar with it. The suggested refactor would not provide meaningful benefits. The comment should be deleted as the suggested refactor would add unnecessary complexity without providing meaningful benefits to code clarity or maintainability.
2. engine/baml-compiler/src/hir/dump.rs:335
- Draft comment:
The Block::to_doc method now uses a local variable 'doc' and conditionally appends the trailing expression. Verify that the nesting and line breaks remain consistent with those in ExprFunction::to_doc. - 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 consistency in nesting and line breaks, which falls under asking the author to double-check things. This violates the rules.
3. engine/baml-compiler/src/thir.rs:129
- Draft comment:
The modifications in Block::dump_str to include the trailing expression (lines 128–134) look correct. Ensure that the formatting of dumped THIR remains consistent with HIR output. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure consistency in formatting, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue with the code.
4. engine/baml-lib/ast/src/parser/datamodel.pest:317
- Draft comment:
The reordering in the 'stmt' rule (lines 317–334) removes duplicate alternatives for for_loop, if_expression, and while_loop. This improves grammar clarity. Please verify that no intended edge cases for nested statements have been affected. - 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 no intended edge cases for nested statements have been affected. This falls under the rule of not asking the author to confirm their intention or to ensure behavior is intended. Therefore, this comment should be removed.
5. Multiple test files:1
- Draft comment:
Renaming built-in function references from 'len' to 'std.Array.len' across test files is consistent and clarifies its namespace. Ensure that all expected outputs and bytecode comments reflect the updated name. - 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.
6. engine/baml-lib/baml/tests/validation_files/expr/method_decl.baml:13
- Draft comment:
The commented-out SetName method indicates a known limitation with assignment to field accesses. Document the plan for supporting left-hand side FieldAccess in assignments. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%The comment is asking for documentation of a plan, which is not a specific code suggestion or a request for a test. It seems to be more about project management or documentation rather than code review.
7. engine/baml-lib/baml/tests/validation_files/loops/c_for.baml:18
- Draft comment:
Error cases in the c_for tests (e.g. unknown variable 'i' or 'x' in loop headers) appear to be intentional to assert proper scope handling. Confirm that these error messages are correctly enforced during validation. - 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.
8. engine/baml-lib/baml/tests/bytecode_files/array_access.baml:48
- Draft comment:
It looks like you changed the function comment from "len" to "std.Array.len". Note that the class reference on line 47 uses the double colon (std::Request), so for consistency, consider using "std::Array.len" instead of "std.Array.len". - 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% This is a test file containing bytecode examples. The comment is about a purely cosmetic change to a documentation/comment line. The actual functionality or behavior of the code is not affected. The suggestion is about maintaining consistent notation in comments, which is not a critical issue that requires a code change. The comment does point out an actual inconsistency in notation style. Could inconsistent documentation style cause confusion for future developers? While consistency is good, this is a test file and the comment is about documentation style rather than actual code. The meaning is clear either way, and this doesn't meet our bar for "clearly requires a code change". Delete the comment. It's about documentation style in a test file rather than a substantive code issue that needs fixing.
9. engine/baml-lib/baml/tests/bytecode_files/function_calls.baml:56
- Draft comment:
Typographical error: Inconsistent namespace delimiter. In the previous comment, the class is referenced asstd::Requestwith a double colon. Consider updatingstd.Array.lentostd::Array.lenfor 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% This is a test file showing bytecode output. The presence of both "::" and "." could be intentional to test different namespace delimiter styles. Without knowing the full context of the BAML language specification or test intentions, we can't be certain this is actually an error. The comment is also somewhat pedantic about a formatting detail in a test file. I might be underestimating the importance of consistent namespace delimiters in test files. Perhaps this inconsistency could confuse future developers. While consistency is good, this is a test file that may be intentionally testing different delimiter styles. Without strong evidence that this is actually incorrect, we should not keep the comment. Delete the comment as it makes assumptions about required consistency in a test file without strong evidence that the inconsistency is actually a problem.
10. engine/baml-lib/baml/tests/bytecode_files/loops/break.baml:93
- Draft comment:
Typo alert: The comment now reads "Function: std.Array.len" but earlier the namespace is referenced as "std::Request". Should "std.Array.len" use "std::" for consistency? - 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% This is a test file showing bytecode output. The two notations (:: vs .) appear in different contexts - one for a class name, one for a function name. There's no evidence that these need to be consistent. In fact, they may intentionally use different notation. Without knowing the codebase's conventions, we can't assume this is a real issue. Maybe there is a style guide or codebase convention that requires consistent namespace notation that we're unaware of? Even if such a convention exists, we don't have strong evidence of it here. Making assumptions about style conventions without context violates our core principle of needing strong evidence. Delete this comment. We don't have strong evidence that the different notations are actually a problem, and this appears to be in test/documentation content rather than actual code.
11. engine/baml-lib/baml/tests/bytecode_files/loops/continue.baml:96
- Draft comment:
Typo: The current comment uses "std.Array.len" but elsewhere (line 95) the standard namespace is referenced using double colons as in "std::Request". Please confirm if this should be "std::Array.len" for consistency. - 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% This is a test file containing bytecode output comments. The comment is about notation consistency in comments, not actual code. The comments appear to be machine-generated output from a bytecode generator. Making the notation consistent in generated test output is not important and could even be incorrect if it's reflecting the actual output of some tool. Maybe the notation inconsistency reflects an actual bug in the bytecode generator that should be fixed? Maybe this consistency is important for documentation purposes? Since this is in a test file and appears to be machine-generated output, the notation should reflect the actual output of the tool being tested, not be manually "fixed" for consistency. Delete the comment. It's suggesting to change machine-generated test output for cosmetic reasons, which could actually hide real behavior of the system being tested.
12. engine/baml-lib/baml/tests/bytecode_files/mut_variables.baml:32
- Draft comment:
Inconsistent use of namespace separators: the comment above uses 'std::Request' while here it's written as 'std.Array.len'. It might be a typographical error and should be 'std::Array.len' for consistency. - 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% This is a test file that appears to be showing bytecode output. The inconsistency in separators might be intentional as these could be representing different things - one being a class name (std::Request) and another being a function reference (std.Array.len). Without more context about the bytecode format and conventions, we can't be certain this is actually an issue. I might be missing some internal convention about how namespaces should be represented in bytecode output. The different separators could have semantic meaning I'm unaware of. Since this is a test file showing bytecode output, we should be very cautious about assuming formatting conventions. The different separators might be intentionally different to represent different types of references. The comment should be deleted as we don't have strong evidence that this is actually an issue, and it's in a test file showing bytecode output where the different separators might be intentional.
13. engine/baml-lib/baml/tests/bytecode_files/simple_function.baml:11
- Draft comment:
Typographical consistency issue: The comment usesstd.Array.lenwhereas the class on line 10 is referenced asstd::Request. Should the namespace delimiter for the Array be::as well (i.e.,std::Array.len)? - 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% This is a test file showing bytecode output. The namespace delimiters might be intentionally different as they could be representing different things - one being a class reference and another being a function reference. Without knowing the bytecode format specifications or the larger codebase context, we can't be certain this is actually an issue. The comment is also somewhat speculative, using "Should" language. I might be missing some internal standard for namespace delimiters in this codebase. The inconsistency could actually be a real issue that needs fixing. However, this is a test file showing bytecode output, and the different delimiter styles might be intentional. Without strong evidence that this is actually wrong, we should err on the side of removing the comment. Delete the comment as it's speculative and we don't have strong evidence that the different delimiter styles are actually incorrect in this context.
Workflow ID: wflow_BdqSpcw4T3BLUrPG
You can customize
antoniosarosi
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
antoniosarosi
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
antoniosarosi
temporarily deployed
to
boundary-tools-dev
GitHub Actions
Inactive
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 7b80c60 in 1 minute and 35 seconds. Click for details.
- Reviewed
44lines of code in2files - 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-vm/src/vm.rs:878
- Draft comment:
JumpIfFalse does not pop the condition value (uses ensure_stack_top) which may leave an extra boolean on the stack. Consider consuming the condition for stack balance. - 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:856
- Draft comment:
Repeated reassignment of 'function' to re-read the current function (via as_function) suggests a pattern that could be refactored into a helper to reduce boilerplate and potential errors. - 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:710
- Draft comment:
The GC logic using runtime_allocs_offset (objects.drain(runtime_allocs_offset..)) depends on correctly tracking compile‐time vs runtime objects. Ensure this contract is well documented and remains valid under future changes. - 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_Ig145AA9F04ey9XU
You can customize
|
🌿 Preview your docs: https://boundary-preview-a9bfe62e-2b7f-483e-87e0-ebff06ad90fa.docs.buildwithfern.com |
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.
Method declarations and a bunch of other AST / HIR / THIR / Codegen fixes.
Important
Enhance BAML compiler and VM with method declarations, trailing expression handling, and improved constructor logic, including comprehensive test updates.
selfparameter handling inparse_named_args_list.rsandparse_type_expression_block.rs.produces_final_value()withtrailing_exprinhir.rsandthir.rs.compile_thir_to_bytecode()incodegen.rsto handle method bytecode generation.Vm::len()innative.rsto usestd.Array.len.parse_expr.rsandparse_named_args_list.rsto support new syntax for method declarations and assignments.parse_type_expression_block.rsto parse methods within classes.validate_expr_fns()inexpr_fns.rsto check for method name uniqueness and argument validity.bytecode_tests.rsandvm.rsto cover new method declaration and execution scenarios.This description was created by
for 7b80c60. You can customize this summary. It will automatically update as commits are pushed.