Paulo/optional function parameters#3476
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (10)
⛔ Files ignored due to path filters (20)
⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (20)
📒 Files selected for processing (10)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds optional/default parameters and named-argument binding end-to-end: parsing and CST → AST defaults and CallArg → HIR/PPIR signature metadata → TIR call binding/CallPlan → MIR prolog/adapters with OmittedArg → emitter/VM param_has_default and sentinel handling → engine bound-arg API → CLI/LSP/tools updates and tests. ChangesOptional parameters and named arguments
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Merging this PR will improve performance by 67.49%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | e2e_100_functions |
250.8 ms | 200.3 ms | +25.24% |
| ⚡ | WallTime | vm_closure_call_50k |
13.8 ms | 12 ms | +14.68% |
| ⚡ | WallTime | vm_nested_loop |
8.4 ms | 7.5 ms | +13.21% |
| ⚡ | WallTime | e2e_fib_20 |
161.7 ms | 128.1 ms | +26.15% |
| ⚡ | WallTime | compile_to_engine |
156.7 ms | 124 ms | +26.39% |
| ⚡ | WallTime | vm_loop_500k |
37.3 ms | 29.8 ms | +25.21% |
| ⚡ | WallTime | vm_call_chain_100_x_5k |
53.2 ms | 46.7 ms | +14.13% |
| ⚡ | WallTime | startup_empty_expression |
157.3 ms | 124.2 ms | +26.67% |
| ⚡ | WallTime | e2e_hello_world |
157.7 ms | 124.3 ms | +26.89% |
| ⚡ | WallTime | e2e_class_and_loop |
160.9 ms | 126.9 ms | +26.75% |
| ⚡ | WallTime | vm_field_access_50k |
10.8 ms | 9.1 ms | +18.22% |
| ⚡ | WallTime | vm_fib_20 |
8.8 ms | 7.7 ms | +15.25% |
| ⚡ | WallTime | vm_mixed_ops |
21.5 ms | 12.8 ms | +67.49% |
| ⚡ | WallTime | e2e_arithmetic |
157.6 ms | 124.5 ms | +26.61% |
Comparing paulo/optional-function-parameters (e5aa6e2) with canary (7a03331)1
Footnotes
Binary size checks failed❌ 1 violations · ✅ 6 passed
Details & how to fixViolations:
Add/update baselines:
[artifacts.bridge_cffi]
file_bytes = 17655096
stripped_bytes = 17655088
gzip_bytes = 6587451Generated by |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
baml_language/crates/baml_compiler2_visualization/src/control_flow/from_ast.rs (1)
784-791:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNamed call-argument labels are not rendered in compact call text.
render_expr_compact_astnow unwrapsCallArgviaa.expr, but this drops argument labels for named calls. That can makefoo(x: a)andfoo(a)render identically, which is misleading in CFG labels/debugging.Suggested fix
ast::Expr::Call { callee, args } => { let callee_str = render_expr_compact_ast(body, *callee); let args_str: Vec<_> = args .iter() - .map(|a| render_expr_compact_ast(body, a.expr)) + .map(|a| { + let expr = render_expr_compact_ast(body, a.expr); + match &a.label { + Some(label) => format!("{label}: {expr}"), + None => expr, + } + }) .collect(); format!("{}({})", callee_str, args_str.join(", ")) } ast::Expr::OptionalCall { callee, args } => { let callee_str = render_expr_compact_ast(body, *callee); let args_str: Vec<_> = args .iter() - .map(|a| render_expr_compact_ast(body, a.expr)) + .map(|a| { + let expr = render_expr_compact_ast(body, a.expr); + match &a.label { + Some(label) => format!("{label}: {expr}"), + None => expr, + } + }) .collect(); format!("{}?.({})", callee_str, args_str.join(", ")) }Also applies to: 792-799
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_compiler2_visualization/src/control_flow/from_ast.rs` around lines 784 - 791, The compact call rendering currently drops named argument labels by unwrapping CallArg via a.expr; update the ast::Expr::Call arm in render_expr_compact_ast (and the other similar call site) to inspect each CallArg (e.g., the iterator using args.iter().map(|a| ...)) and render a.label when present, producing "label: <expr>" for named args and "<expr>" for unnamed ones before joining them into the final "{}({})" string; ensure you reference the CallArg field names exactly (label and expr) and preserve existing compact rendering for the expr portion.baml_language/crates/tools_onionskin/src/compiler.rs (1)
489-494:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRender named argument labels here too.
These branches now descend into
arg.expr, but they still drop the argument label. After this PR,foo(x: 1)andfoo(1)will render the same in onionskin, which makes named/default-argument debugging misleading. Please include the label when present before renderingarg.expr.Also applies to: 566-571
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/tools_onionskin/src/compiler.rs` around lines 489 - 494, The loop over function arguments currently only renders arg.expr and drops any argument label, so named args like foo(x: 1) appear identical to foo(1); update the branches that iterate args (the block building spans with DetailSpan::Code(", ".into()) and calling expr_desc_spans(arg.expr, ...)) to first check for a label on the argument (e.g., arg.label or similar) and, when present, push a label span before the expression (for example push a DetailSpan::Code with "label: " or equivalent), then render the expr via expr_desc_spans as before; apply the same change to the similar block around lines 566-571 to ensure labels are rendered in both places.baml_language/languages/python/rust/codegen_python/src/translate_ty.rs (1)
74-82:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
typing.Callablecannot represent named/optional parameters—generated.pyisignatures will be unsound.This PR introduces callable types with optional and named parameters like
(x: int, b?: int) -> int, but the current translation at lines 74–82 emitstyping.Callable[[int, int], int], which:
- Erases parameter names (
x,b)- Loses optionality information (that
bis optional)- Causes type checkers to reject valid callables and accept signatures BAML would reject
Python's
typing.Callable[[...], R]only supports positional-only parameters; it has no syntax for keyword names or defaults. Type checkers (mypy, pyright) require aProtocolwith__call__to express full function signatures including keyword-only and optional parameters.To fix: either generate a
Protocolwith__call__for callables carrying names/defaults/optional modifiers, or fall back totyping.Callable[..., R](which accepts anything) when such metadata is present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/languages/python/rust/codegen_python/src/translate_ty.rs` around lines 74 - 82, The current translation in Ty::Callable (translate_ty) emits typing.Callable[[...], R], which loses parameter names and optionality; update translate_ty so it inspects params for named or optional parameters and when any are present emit a safer representation: either synthesize a Protocol class with a __call__(self, <full signature with names and optional defaults>) -> R and reference that Protocol, or if you prefer minimal change emit typing.Callable[..., R] to preserve soundness; keep using translate_ty and ctx for element translations and only use the positional typing.Callable[[...], R] when all params are plain positional required parameters.baml_language/crates/baml_lsp2_actions/src/annotations.rs (1)
226-252:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSuppress parameter-name hints when the call argument is already labeled.
With the new
CallArgshape supporting an optionallabel, calls likefoo(x = 42)would produce a redundant hint (x: x = 42). Whenarg.labelisSome, the user already named the argument explicitly, so the inlay hint adds visual noise.🛡️ Suggested guard
for (arg, param) in args.iter().zip(params.iter()) { + // If the user already provided a labeled argument, don't + // render a redundant parameter-name hint. + if arg.label.is_some() { + continue; + } // Only emit hints for named parameters. let Some(name) = ¶m.name else { continue; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_lsp2_actions/src/annotations.rs` around lines 226 - 252, The parameter-name inlay code iterates args and params and always emits an InlineAnnotation for each named parameter, causing redundant hints when the call argument already has an explicit label (CallArg.label); update the loop in annotations.rs to skip emitting a hint when arg.label.is_some() (i.e., if the CallArg already has a label) before computing arg_span and pushing InlineAnnotation, referencing the args iterator element (arg), the param.name check, and the InlineAnnotation/AnnotationKind::Parameter creation to locate where to add the guard.baml_language/crates/baml_compiler2_hir/src/builder.rs (1)
776-790:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack lambda defaults inside the lambda capture context.
References used by parameter defaults are walked before
lambda_stack.push(scope_id), sorecord_path_root_referencetags them withowner_lambda = None. That means outer names used in defaults are skipped byanalyze_lambda_captures. A lambda like(x = outer) -> xwill missouterin its capture set.Suggested fix
self.push_scope(ScopeKind::Lambda, None, source_map.expr_span(expr_id)); let scope_id = self.current_scope_id(); + self.lambda_stack.push(scope_id); for (idx, param) in func_def.params.iter().enumerate() { self.scope_bindings[scope_id.index() as usize] .params .push((param.name.clone(), idx)); } @@ - if let Some(ast::FunctionBodyDef::Expr(lambda_body, lambda_source_map)) = &func_def.body { - self.lambda_stack.push(scope_id); + if let Some(ast::FunctionBodyDef::Expr(lambda_body, lambda_source_map)) = &func_def.body { self.walk_expr_body(lambda_body, lambda_source_map); self.analyze_lambda_captures(scope_id, lambda_body, lambda_source_map); - self.lambda_stack.pop(); } + self.lambda_stack.pop(); self.pop_scope();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_compiler2_hir/src/builder.rs` around lines 776 - 790, Parameter default expressions are being walked before the lambda capture context is set, so references from defaults get recorded with owner_lambda = None and aren't captured; to fix, when func_def.body matches ast::FunctionBodyDef::Expr (a lambda) push the lambda scope (lambda_stack.push(scope_id)) before walking the parameter defaults and then proceed to walk_expr for each param.default, walk_expr_body, analyze_lambda_captures, and finally pop the lambda stack; this ensures record_path_root_reference sees owner_lambda = Some(scope_id) for references inside defaults and they are included in analyze_lambda_captures.baml_language/crates/baml_lsp2_actions/src/check.rs (1)
196-224:⚠️ Potential issue | 🔴 CriticalAdd validation for default parameter constraints in non-expression-body functions.
The check at lines 196-224 validates only type expressions via
lower_type_expr_in_ns. Builtin/LLM/Missing functions can have defaults (they are parsed and available viasig.params), but errors likeRequiredParamAfterDefault,SelfParamDefault, andDefaultParamForwardReferenceare only validated during TIR scope inference, which runs only for expression-body functions. Non-expression-body functions will silently skip default constraint validation in the LSP check path.To fix: Add validation logic in step 5 (after the type-expression checks) to verify default parameter constraints for non-expression-body functions, or invoke the builder for these function types as well.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_lsp2_actions/src/check.rs` around lines 196 - 224, The current loop over sig.params only calls lower_type_expr_in_ns and emits type-expression diagnostics, but it skips default-parameter constraint validation for non-expression-body functions; after the existing type-expression checks inside the loop (the block that fills type_errors and pushes Diagnostics using tir_type_error_to_diagnostic_id and diagnostics.push(...).with_primary_span(...).with_phase(DiagnosticPhase::Type)), add logic to validate default parameter constraints (RequiredParamAfterDefault, SelfParamDefault, DefaultParamForwardReference) for parameters that have defaults (from sig.params) when the function is not an expression-body; either invoke the same TIR scope inference/builder used for expression-body functions or replicate its constraint checks, map any resulting errors to diagnostics the same way (use the same file_id/range from func_data.params[..].type_expr or the param span) and push them onto diagnostics so builtins/LLM/Missing functions’ defaults are validated in the LSP path as well.
🧹 Nitpick comments (13)
baml_language/crates/baml_compiler2_visualization/src/control_flow/from_ast.rs (1)
1279-1301: ⚡ Quick winAdd unit coverage for named-argument rendering (call + optional call).
The updated tests only cover positional
CallArg::positional(...). Given this PR adds named arguments, add at least one test asserting label-preserving render output forCallandOptionalCall.As per coding guidelines "
**/*.rs: Prefer writing Rust unit tests over integration tests where possible`."Also applies to: 1442-1466
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_compiler2_visualization/src/control_flow/from_ast.rs` around lines 1279 - 1301, Add unit tests that cover named-argument calls for both Call and OptionalCall: in addition to the existing call_scope_has_source_expr test, create tests that build an AST using make_ast_body where the call uses ast::CallArg::named("label", arg) (and similarly an ast::Expr::OptionalCall if that variant exists), then run build_control_flow_graph_from_ast and assert you still get an OtherScope node with source_expr set (like call_scope_has_source_expr) and additionally assert the rendered/label-preserving output for that node preserves the argument label (use the same rendering path your suite uses for nodes). Reference ast::Expr::Call, ast::Expr::OptionalCall, ast::CallArg::named, make_ast_body, build_control_flow_graph_from_ast, and NodeType::OtherScope when locating where to add these tests.baml_language/languages/python/tests/test_sim_phase3.py (1)
76-85: ⚡ Quick winMirror the mutable-default regression check in the async path.
The sync test on Lines 71-72 proves omitted
itemsgets a fresh list on every call. The async client is a separate generated path, but heremutate_default_async()is only invoked once, so a shared-default bug there would still pass.Suggested addition
`@pytest.mark.asyncio` async def test_generated_client_defaulted_params_async(): from baml.baml_core import UNSET from baml_sdk.lorem import default_score_async, mutate_default_async assert await default_score_async("cats") == 10 assert await default_score_async("cats", max_results=5) == 5 assert await default_score_async("cats", filter=None) == 10 assert await default_score_async("cats", max_results=UNSET, filter="recent") == 110 assert await mutate_default_async() == 1 + assert await mutate_default_async() == 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/languages/python/tests/test_sim_phase3.py` around lines 76 - 85, The async test needs the same mutable-default regression check as the sync path: in test_generated_client_defaulted_params_async call mutate_default_async() a second time and assert the result is still the initial value to ensure defaults aren't shared; specifically add another assertion like "assert await mutate_default_async() == 1" after the existing mutate_default_async() call in the test_generated_client_defaulted_params_async test to mirror the sync-case protection for defaulted items.baml_language/crates/baml_fmt/src/ast/types.rs (1)
1074-1127: ⚡ Quick winAdd a focused unit test for
name?: Typefunction params.This change touches a pretty subtle parser/printer path. A local unit test covering at least
(<name>?: string) -> intand the paren-type disambiguation around optional types would make regressions much easier to catch. Please also runcargo test --liblocally for the Rust changes.As per coding guidelines, "
**/*.rs: Prefer writing Rust unit tests over integration tests where possible" and "Always run cargo test --lib if you changed any Rust code`."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_fmt/src/ast/types.rs` around lines 1074 - 1127, Add focused Rust unit tests to cover parsing and printing of function parameters with optional names and types (e.g. "(name?: string) -> int") and the paren-type disambiguation around optional types; specifically add tests that exercise FromCST::from_cst for FunctionTypeParam and the Printable::print path (FunctionTypeParam::print) to ensure the name/question/colon printing behavior and the leftmost_token logic are correct. Create cases for: a) a parameter with name and optional marker before the colon ("name?: string"), b) a parameter with an optional type that could be parsed as a parenthesized type to ensure disambiguation, and assert round-trip (parse -> print -> parse) or exact printed output; run cargo test --lib to verify. Ensure tests reference FunctionTypeParam parsing and printing behavior rather than other helpers so failures point to FromCST, print, or leftmost_token implementations.baml_language/crates/bex_vm_types/src/types.rs (1)
528-667: ⚡ Quick winAdd a unit test that keeps the omitted-arg paths in sync.
Value,ConstValue, and runtimeTypeall grewOmittedArghere. A small local test coveringConstValue::OmittedArg.to_value(...),Type::of(&Value::OmittedArg, ...), and the display string would catch future drift quickly.As per coding guidelines, "
**/*.rs: Prefer writing Rust unit tests over integration tests where possible`."Also applies to: 860-898
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/bex_vm_types/src/types.rs` around lines 528 - 667, Add a Rust unit test in this module (#[cfg(test)] mod tests) that ensures the omitted-arg variants stay in sync: call ConstValue::OmittedArg.to_value(...) with a simple resolver and assert it yields Value::OmittedArg; pass that Value::OmittedArg into Type::of(...) and assert the returned runtime Type is the OmittedArg variant; and assert format!("{}", Value::OmittedArg) == "<omitted>" (uses Display impl). Name the test clearly (e.g., omitted_arg_roundtrip_or_sync) and keep it local to this file so it runs as a unit test.baml_language/crates/bex_vm/src/vm.rs (1)
499-499: ⚡ Quick winPlease add/confirm a unit test for
OmittedArgtype-tagging and run lib tests.Since
value_type_tagnow classifiesValue::OmittedArgasUNKNOWN, add (or point to) a Rust unit test that exercisesTypeTag/IsTypebehavior for this sentinel, and confirmcargo test --libpassed for this PR.As per coding guidelines: "
**/*.rs: Prefer writing Rust unit tests over integration tests where possible" and "Always runcargo test --libif you changed any Rust code`".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/bex_vm/src/vm.rs` at line 499, Add a unit test exercising Value::OmittedArg through value_type_tag and the TypeTag/IsType APIs: create a Value::OmittedArg, assert value_type_tag(&Value::OmittedArg) returns type_tags::UNKNOWN (or the TypeTag variant equivalent), and assert the IsType checks behave as expected for UNKNOWN; place the test in the same Rust source file as the implementation (#[cfg(test)] mod tests) to follow unit-test guidance and run cargo test --lib to confirm all library tests pass.baml_language/crates/baml_tests/src/compiler2_emit/mod.rs (2)
29-35: 💤 Low valuePossible duplication of
mir_snapshot!macro.The
emit_snapshot!macro here is structurally identical tomir_snapshot!inbaml_language/crates/baml_tests/src/compiler2_mir/mod.rs. If this pattern proliferates to additional snapshot test modules, consider extracting a sharedsnapshot!helper macro (or a tiny utility module) within thebaml_testscrate to avoid divergence in snapshot settings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_tests/src/compiler2_emit/mod.rs` around lines 29 - 35, emit_snapshot! duplicates mir_snapshot! — extract a shared helper to avoid divergence: create a single macro (e.g. snapshot! or shared_snapshot!) or a small utility module in the baml_tests crate and move the common invocation (with_settings!({ snapshot_path => SNAPSHOT_PATH, omit_expression => true }, { assert_snapshot!($name, $output); })) there; then update both emit_snapshot! and mir_snapshot! (or replace them) to call the shared symbol, keeping references to with_settings!, SNAPSHOT_PATH, omit_expression, and assert_snapshot! so settings remain identical across modules.
114-177: 💤 Low valueConsider consolidating duplicated source string.
The two new tests
optional_param_metadata_and_omitted_sentinel_emitandoptional_defaults_emit_snapshotuse the exact same BAML source. Extracting a small&strconstant (or a helper that builds the DB) would keep them in sync if the source ever needs to change.♻️ Example refactor
+const OPTIONAL_DEFAULTS_SOURCE: &str = r#" + function add(base: int, amount: int = base + 2) -> int { + base + amount + } + + function main() -> int { + add(5) + } + "#; + #[test] fn optional_param_metadata_and_omitted_sentinel_emit() { let mut db = make_db(); - db.add_file( - "test.baml", - r#" - function add(base: int, amount: int = base + 2) -> int { - base + amount - } - - function main() -> int { - add(5) - } - "#, - ); + db.add_file("test.baml", OPTIONAL_DEFAULTS_SOURCE);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_tests/src/compiler2_emit/mod.rs` around lines 114 - 177, The two tests optional_param_metadata_and_omitted_sentinel_emit and optional_defaults_emit_snapshot duplicate the same BAML source string passed to db.add_file; extract that literal into a shared constant or helper to avoid drift (e.g., define a const TEST_BAML_SOURCE: &str = "..."; or add a helper fn add_optional_defaults_test_file(db: &mut Db) that calls db.add_file with the source) and update both tests to use the constant/helper so the source remains single-sourced.baml_language/crates/baml_compiler2_ast/src/lib.rs (1)
329-370: 💤 Low valueLGTM — good unit-test coverage of the new AST surface.
The test exercises both directions of the new feature (parameter defaults preserved on
FunctionDef.defaultsand labels preserved onCallArg), and it's a unit test in#[cfg(test)] mod testswhich aligns with the repo's preference for unit tests over integration tests.One small note:
function.defaults.exprs.exprs[default_id.0]reaches through three layers of internal structure. IfFunctionDefaultsis going to be a stable AST surface, exposing a small accessor (e.g.,defaults.expr(default_id) -> &Expr) would make tests and downstream consumers less coupled to the layout — but this is purely a follow-up nicety.As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_compiler2_ast/src/lib.rs` around lines 329 - 370, The test reaches through internal structure via function.defaults.exprs.exprs[default_id.0]; add a small public accessor on the FunctionDefaults (or the module that owns defaults) such as a method like FunctionDefaults::expr(&self, id: ExprId) -> &Expr (or similar name) and update the test (and any consumers) to call function.defaults.expr(default_id) instead of indexing into exprs.exprs, so callers don't depend on the nested storage layout; reference the FunctionDefaults type and the default_id/ExprId used in the test when implementing the accessor.baml_language/crates/baml_compiler2_emit/src/lib.rs (1)
916-979: ⚡ Quick winConsider a small unit test for the new
param_has_defaultextraction.The existing
testsmodule already coversparse_string_attr_valueandextract_schema_attrsin detail. A tiny test that constructs abaml_compiler2_hir::item_tree::Functionwith a mix of defaulted and non-defaulted params and asserts the returnedVec<bool>would lock in the new contract cheaply and is consistent with the file's test style. As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_compiler2_emit/src/lib.rs` around lines 916 - 979, Add a small unit test for compute_function_metadata_from_item_tree that constructs a baml_compiler2_hir::item_tree::Function with several params (some with default Some(...) and some with default None), then calls compute_function_metadata_from_item_tree (providing the minimal db/file/ResolvedAliases stubs consistent with existing tests in this file) and asserts the returned param_has_default Vec<bool> matches the expected pattern; place the test in the existing tests module and follow the same setup/style as parse_string_attr_value/extract_schema_attrs tests so it runs with cargo test.baml_language/crates/baml_compiler_parser/src/parser.rs (1)
2093-2100: ⚡ Quick winPlease add unit coverage for the new recovery/default branches.
The new tests miss three fresh paths in this PR:
self = ...recovery, lambda parameter defaults, and the explicit “default expressions are not allowed in function types” recovery branch. A small unit test per case would make these grammar changes much safer to evolve.As per coding guidelines, "Prefer writing Rust unit tests over integration tests where possible".
Also applies to: 2471-2473, 2744-2746, 7473-7538
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_compiler_parser/src/parser.rs` around lines 2093 - 2100, Add unit tests that exercise the three new recovery/default branches: (1) a parameter recovery where a lambda/func param named "self" is followed by "= ..." to trigger the self = ... recovery path, (2) a lambda parameter with a default value to hit the lambda parameter defaults path, and (3) a function-type parameter default that triggers the explicit "default expressions are not allowed in function types" branch where parser code calls p.eat(TokenKind::Equals), captures span via p.current(), calls p.error(..., span) and then p.parse_expr(). For each test call the parser entry (same function used by existing unit tests), assert that parsing completes with the expected error message(s) (including the exact string "default expressions are not allowed in function types") and that recovery continues (no panic), and add these as Rust unit tests (not integration tests) alongside other parser unit tests so they run with cargo test.baml_language/crates/baml_tests/src/compiler2_tir/mod.rs (1)
234-239: ⚡ Quick winRender default values in snapshots instead of collapsing them to
?.
p.default.is_some()tells us there is a concrete default expression, but these renderers only emitname?: T. That makesx = 1andx = 2snapshot-identical and hides part of the feature this PR is adding. Please include the default expression, or at least a distinct placeholder that preserves it in the snapshot output.Also applies to: 289-294, 1030-1033, 1674-1680
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_tests/src/compiler2_tir/mod.rs` around lines 234 - 239, The snapshot renderer currently collapses default values to "?" by checking p.default.is_some(); instead, render the actual default expression (or a distinct placeholder containing the default text) so different defaults are distinguishable. Update the formatting in the blocks that build parameter strings (the shown block and the other similar blocks at lines referenced) to include the default content from p.default (e.g., append " = <default_expr>" or embed the default string) when p.default.is_some(), using p.default's expression/text instead of only emitting "?"; apply the same change to the other occurrences mentioned (around the code that formats parameters at the other line ranges).baml_language/crates/baml_compiler2_ast/src/lower_expr_body.rs (2)
1558-1577: ⚡ Quick winExtract a helper for
label_spansrecording to remove duplication.The
label_spansderivation and the loop that inserts intosource_map.call_arg_label_spansare identical betweenlower_call_expr(Lines 1558-1577) andlower_optional_call_expr(Lines 1874-1889). Extracting a small helper keeps the two paths in lockstep and avoids drift if the source-map representation changes.♻️ Proposed helper
fn finalize_call_args( &mut self, lowered_args: Vec<(CallArg, Option<TextRange>)>, ) -> (Vec<CallArg>, Vec<Option<TextRange>>) { let label_spans: Vec<_> = lowered_args.iter().map(|(_, span)| *span).collect(); let args: Vec<_> = lowered_args.into_iter().map(|(arg, _)| arg).collect(); (args, label_spans) } fn record_call_arg_label_spans( &mut self, call_id: ExprId, label_spans: Vec<Option<TextRange>>, ) { for (idx, label_span) in label_spans.into_iter().enumerate() { if let Some(span) = label_span { self.source_map .call_arg_label_spans .insert((call_id, idx), span); } } }Then both call paths reduce to:
let lowered_args = node .children() .find(|n| n.kind() == SyntaxKind::CALL_ARGS) .map(|args_node| self.lower_call_args_node(&args_node)) .unwrap_or_default(); let (args, label_spans) = self.finalize_call_args(lowered_args); let id = self.alloc_expr(Expr::Call { callee, args }, node.text_range()); self.record_call_arg_label_spans(id, label_spans);Also applies to: 1874-1889
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_compiler2_ast/src/lower_expr_body.rs` around lines 1558 - 1577, The code duplicates building args and inserting label spans in lower_call_expr and lower_optional_call_expr; extract two helpers (e.g., finalize_call_args(&mut self, lowered_args: Vec<(CallArg, Option<TextRange>)>) -> (Vec<CallArg>, Vec<Option<TextRange>>) and record_call_arg_label_spans(&mut self, call_id: ExprId, label_spans: Vec<Option<TextRange>>)) and replace the duplicated logic: call self.finalize_call_args(lowered_args) to get (args, label_spans), alloc the call via self.alloc_expr(Expr::Call { callee, args }, ...), then call self.record_call_arg_label_spans(id, label_spans) so both lower_call_expr and lower_optional_call_expr reuse the same behavior around lower_call_args_node and source_map.call_arg_label_spans.
1591-1613: 💤 Low valueConsider making the bare-token fallback more defensive.
The
skip(if label.is_some() { 2 } else { 0 })relies on the label occupying exactly two non-trivia tokens (the WORD and EQUALS). This is currently correct, but fragile—if the grammar evolves (e.g.,name: T = expr), this would silently target the wrong token instead of failing loudly.Since
cst_arg.label()and the label token's range are already available, the fallback could skip past the label token's end position explicitly rather than assuming token count, making it resilient to future grammar changes. This is optional and low-effort, and only relevant if typed labels or similar features are anticipated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_compiler2_ast/src/lower_expr_body.rs` around lines 1591 - 1613, The fallback that finds a bare token currently uses skip(if label.is_some() { 2 } else { 0 }) which assumes the label occupies exactly two non-trivia tokens; update lower_call_arg_node to be defensive by using the existing label_token/text_range to skip tokens by position instead of count: if label_token.is_some(), compute its end position (label_token.text_range().end()) and change the token search to ignore any token whose text_range().start() <= label_end, then find the first non-trivia, non-comma token and pass it to lower_bare_token_expr/alloc_expr as before; this preserves behavior while making the code resilient to grammar changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/baml_compiler2_ast/src/ast.rs`:
- Around line 300-303: The FunctionTypeParam.optional flag is not being rendered
when printing function types, so update the formatter used for
TypeExpr::Function to emit the optional marker the same way parsing/lowering
expects (e.g., append "?" to the parameter name or before the type as your
surface syntax defines) when FunctionTypeParam.optional is true; locate the code
that iterates/prints params for TypeExpr::Function and change it to check each
FunctionTypeParam.optional and include the "?" marker in the output while
preserving name/ty formatting.
In `@baml_language/crates/baml_compiler2_ast/src/lower_cst.rs`:
- Around line 302-344: In lower_params_with_defaults the code records
default_nodes using the original enumerate index from pl.params(), but params is
built with filter_map so indices can shift when lower_param returns None; change
the collection logic to iterate pl.params() without enumerate, call
lower_param(&p, ...), and when it returns Some(param) push it onto params and,
if p.default_expr_syntax() exists, push (params.len() - 1, default_expr) into
default_nodes (preserving the defaults_allowed diagnostic behavior before/after
lowering as needed); this ensures the indices passed to
lower_expr_body::lower_default_expr_nodes and later lookups like
params.get_mut(idx) and the self-name check reference the post-filtered param
positions.
In `@baml_language/crates/baml_compiler2_emit/src/emit.rs`:
- Around line 658-660: Lambdas produced by compile_mir_function are inserted
with an empty param_has_default which causes runtime to treat defaultable lambda
params as required; update the lambda emission path to populate parameter
metadata the same way user functions do: call or reuse
compute_function_metadata_from_item_tree (or its extraction logic) for lambda
AST/mir nodes to fill Function.param_has_default, param_names and param_types,
or, if a full metadata pass is unavailable, explicitly initialize
Function.param_has_default to a bool vector matching Function.param_names length
(false for non-defaults, true where a default is present based on the lambda AST
in lower_cst.rs), so bex_engine.rs’s unwrap_or(false) no longer hides missing
metadata.
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 1434-1445: The default-expression lowering reuses
self.current_scope + AstExprId keys while swapping only
self.body/self.source_map in lower_default_expr, causing collisions between the
main function body and FunctionDefaults; fix by giving defaults their own
inference namespace: when entering lower_default_expr either (A) extend cache
keys to include body identity (e.g., include defaults.body_id or a pointer/tag)
so lookups for expr_types/resolutions/call_plans/function_coercions use
(body_id, current_scope, AstExprId), or (B) create a fresh LoweringContext
(clone/instantiate a new lowering state) for defaults so all inference caches
are isolated, then restore the original context after lowering; modify usages
around lower_default_expr, expr_types, resolutions, call_plans,
function_coercions and current_scope to use the new body-scoped key or separate
context.
- Around line 2923-2939: lower_call_arg_operands currently iterates
plan.bindings (callee param order) and lowers provided args in callee order,
which changes source evaluation order; fix by first evaluating each supplied
source arg in order and storing a map from AstExprId to its Operand (call
lower_to_operand for each item in args in order), then iterate plan.bindings and
for ParamBinding::Provided look up the previously stashed operand for that arg,
and for ParamBinding::OmittedDefault emit
Operand::Constant(Constant::OmittedArg); use the same call_plans lookup
(self.call_plans.get(&(self.current_scope, expr_id))) and keep function/variable
names unchanged (lower_call_arg_operands, lower_to_operand, plan.bindings,
ParamBinding::Provided/OmittedDefault) so only evaluation order changes.
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 3941-3951: The bug is that bindings are inferred by zipping
effective_params.iter().zip(args.iter()), which breaks for reordered named args;
change the loop to match each arg to its corresponding param by name (falling
back to positional index when the arg is unnamed) and then call
crate::generics::infer_bindings_allow_typevars(¶m.ty, &arg_ty, &mut
bindings) for that matched param; operate over the args collection (with their
names/indices) and lookup the param in effective_params (by param.name/ident or
by index) before inferring so generic/effect bindings and throws provenance
remain aligned.
- Around line 1289-1290: The forward-reference check in expr_references_name
misses Match arm bodies: when matching on Expr::Match it only inspects the
scrutinee and thus can miss references like `match x { _ => laterParam }`;
update expr_references_name to iterate over the match arms (the arm
expressions/blocks) and call the same reference-check logic on each arm
expression (and any guard expression) so DefaultParamForwardReference cannot be
bypassed by match arms; locate Expr::Match handling in expr_references_name and
add processing of arms/guards to check for the given name.
- Around line 1218-1275: The validation swaps self.expressions to a defaults
arena but leaves self.call_plans and self.function_coercions populated, which
lets entries keyed by default-arena ExprId leak into the main maps; in
check_function_parameter_defaults take (std::mem::take) both self.call_plans and
self.function_coercions into local saved variables before validating defaults
and replace them with empty maps, run the default checks, then restore the saved
call_plans and function_coercions after restoring self.expressions to prevent
cross-arena ExprId leakage (refer to the function
check_function_parameter_defaults and the fields self.expressions,
self.call_plans, self.function_coercions).
In `@baml_language/crates/baml_compiler2_tir/src/callable.rs`:
- Around line 284-292: The throws-binding loop uses
effective_params.iter().zip(args.iter()) which pairs parameters with raw,
non-canonicalized argument expression IDs (args) causing misalignment for
named/optional/out-of-order args; fix by canonicalizing args into parameter
order before zipping (reuse the same canonicalization used by type checking,
e.g. call bind_call_args or equivalent logic used in throws_analysis.rs to map
CallArg metadata to parameter positions) so that the loop in callable.rs that
calls crate::generics::infer_bindings_allow_typevars(¶m.ty, &arg_ty, &mut
bindings) receives the correctly ordered arg expression IDs; alternatively
change the API to accept CallArg metadata alongside expr IDs and perform
matching there.
In `@baml_language/crates/baml_compiler2_tir/src/normalize.rs`:
- Around line 289-297: The current required-parameter loop in normalize.rs only
compares types and ignores parameter names, so required params like `id` vs
`user_id` are treated as compatible; update the loop that iterates over
`sub_required` and `sup_required` to also compare their parameter names (e.g.,
require `sub.name == sup.name` or use whatever field holds the label) before
returning true, or explicitly document/perform a normalization step earlier that
strips/aligns names for positional-only required params; specifically change the
block using `for (sub, sup) in sub_required.iter().zip(sup_required.iter()) { if
!sup.ty.is_subtype_of(&sub.ty, assumptions) { ... } }` to additionally check the
names and fail if they differ (or call the normalization helper) so labeled
required parameters are not silently accepted.
In `@baml_language/crates/baml_compiler2_tir/src/package_interface.rs`:
- Around line 800-809: The implicit `self` parameter is being lowered via
lower_type_expr_in_ns (in the loop over sig.params in package_interface()) which
produces Ty::Unknown and diverges from the synthetic self created for
own-package methods; change the param lowering so that when encountering the
implicit/self parameter (the param from sig.params that represents `self`), you
call build_self_type_for_class(...) with the same context used for own-package
resolution instead of lower_type_expr_in_ns, and push that returned type into
params; keep other params unchanged and continue to collect diagnostics into
diags. Ensure you reference the same namespace/context (ns, all_generic_params,
self.own_items) used elsewhere so lookup_own_class_method() and
package_interface() use the same synthetic self representation.
In `@baml_language/crates/baml_fmt/src/ast/declarations.rs`:
- Around line 566-572: The branch that prints a synthesized " = " is currently
emitting only the trailing trivia from the equals token after the literal and
dropping the trailing trivia from the preceding name/type and the leading trivia
on the equals token; change the trivia handling around equals.span() so you
destructure printer.trivia.get_for_range_split(equals.span()) into
(prev_trailing, equals_leading) and print prev_trailing before printing " = "
and equals_leading immediately after the " = " (then continue to preserve the
default's leading trivia as you already do), updating uses of
printer.print_trivia_squished and the order of printer.print_str(" = ")
accordingly to preserve both the previous element's trailing trivia and the
equals token's leading trivia.
In `@baml_language/crates/baml_fmt/src/ast/expressions.rs`:
- Around line 1682-1700: The named-argument `=` handling drops trivia and
miscomputes width: in single_line_width (method single_line_width on CallArg)
include the full span length of the equals token and any trivia between label,
`=`, and the expression (use equals.span().len() or the split-trivia lengths
instead of the hard-coded " = ".len()) so width reflects emitted characters; in
Printable::print for CallArg stop emitting a hard-coded " = " and instead replay
the equals token and its full leading+trailing trivia (use
printer.print_raw_token(equals) or fetch both parts from
printer.trivia.get_for_range_split(equals.span()) and print them) so
comments/whitespace around `=` are preserved and not lost during formatting.
In `@baml_language/crates/baml_lsp2_actions/src/completions.rs`:
- Around line 57-63: The completion signature currently formats every parameter
as required using param.name and utils::display_type_expr(¶m.ty); update the
mapping in completions.rs to check param.default and render defaulted parameters
as optional by inserting a "?" after the parameter name (e.g., use param.name +
"?" when param.default.is_some()) so the completion shows optional params
correctly while still using utils::display_type_expr(¶m.ty) for the type
display.
In `@baml_language/languages/python/src/baml/baml_core/__init__.py`:
- Around line 225-234: The TypeError message in the block that computes
positional_limit (using required_positional_count, param_names, args) prints all
param_names instead of only the positional ones; update the error construction
in the raise TypeError inside that block to use a sliced list of param_names
limited to positional_limit (e.g., param_names[:positional_limit]) so the
message reports the correct count and only the relevant positional parameter
names.
---
Outside diff comments:
In `@baml_language/crates/baml_compiler2_hir/src/builder.rs`:
- Around line 776-790: Parameter default expressions are being walked before the
lambda capture context is set, so references from defaults get recorded with
owner_lambda = None and aren't captured; to fix, when func_def.body matches
ast::FunctionBodyDef::Expr (a lambda) push the lambda scope
(lambda_stack.push(scope_id)) before walking the parameter defaults and then
proceed to walk_expr for each param.default, walk_expr_body,
analyze_lambda_captures, and finally pop the lambda stack; this ensures
record_path_root_reference sees owner_lambda = Some(scope_id) for references
inside defaults and they are included in analyze_lambda_captures.
In
`@baml_language/crates/baml_compiler2_visualization/src/control_flow/from_ast.rs`:
- Around line 784-791: The compact call rendering currently drops named argument
labels by unwrapping CallArg via a.expr; update the ast::Expr::Call arm in
render_expr_compact_ast (and the other similar call site) to inspect each
CallArg (e.g., the iterator using args.iter().map(|a| ...)) and render a.label
when present, producing "label: <expr>" for named args and "<expr>" for unnamed
ones before joining them into the final "{}({})" string; ensure you reference
the CallArg field names exactly (label and expr) and preserve existing compact
rendering for the expr portion.
In `@baml_language/crates/baml_lsp2_actions/src/annotations.rs`:
- Around line 226-252: The parameter-name inlay code iterates args and params
and always emits an InlineAnnotation for each named parameter, causing redundant
hints when the call argument already has an explicit label (CallArg.label);
update the loop in annotations.rs to skip emitting a hint when
arg.label.is_some() (i.e., if the CallArg already has a label) before computing
arg_span and pushing InlineAnnotation, referencing the args iterator element
(arg), the param.name check, and the InlineAnnotation/AnnotationKind::Parameter
creation to locate where to add the guard.
In `@baml_language/crates/baml_lsp2_actions/src/check.rs`:
- Around line 196-224: The current loop over sig.params only calls
lower_type_expr_in_ns and emits type-expression diagnostics, but it skips
default-parameter constraint validation for non-expression-body functions; after
the existing type-expression checks inside the loop (the block that fills
type_errors and pushes Diagnostics using tir_type_error_to_diagnostic_id and
diagnostics.push(...).with_primary_span(...).with_phase(DiagnosticPhase::Type)),
add logic to validate default parameter constraints (RequiredParamAfterDefault,
SelfParamDefault, DefaultParamForwardReference) for parameters that have
defaults (from sig.params) when the function is not an expression-body; either
invoke the same TIR scope inference/builder used for expression-body functions
or replicate its constraint checks, map any resulting errors to diagnostics the
same way (use the same file_id/range from func_data.params[..].type_expr or the
param span) and push them onto diagnostics so builtins/LLM/Missing functions’
defaults are validated in the LSP path as well.
In `@baml_language/crates/tools_onionskin/src/compiler.rs`:
- Around line 489-494: The loop over function arguments currently only renders
arg.expr and drops any argument label, so named args like foo(x: 1) appear
identical to foo(1); update the branches that iterate args (the block building
spans with DetailSpan::Code(", ".into()) and calling expr_desc_spans(arg.expr,
...)) to first check for a label on the argument (e.g., arg.label or similar)
and, when present, push a label span before the expression (for example push a
DetailSpan::Code with "label: " or equivalent), then render the expr via
expr_desc_spans as before; apply the same change to the similar block around
lines 566-571 to ensure labels are rendered in both places.
In `@baml_language/languages/python/rust/codegen_python/src/translate_ty.rs`:
- Around line 74-82: The current translation in Ty::Callable (translate_ty)
emits typing.Callable[[...], R], which loses parameter names and optionality;
update translate_ty so it inspects params for named or optional parameters and
when any are present emit a safer representation: either synthesize a Protocol
class with a __call__(self, <full signature with names and optional defaults>)
-> R and reference that Protocol, or if you prefer minimal change emit
typing.Callable[..., R] to preserve soundness; keep using translate_ty and ctx
for element translations and only use the positional typing.Callable[[...], R]
when all params are plain positional required parameters.
---
Nitpick comments:
In `@baml_language/crates/baml_compiler_parser/src/parser.rs`:
- Around line 2093-2100: Add unit tests that exercise the three new
recovery/default branches: (1) a parameter recovery where a lambda/func param
named "self" is followed by "= ..." to trigger the self = ... recovery path, (2)
a lambda parameter with a default value to hit the lambda parameter defaults
path, and (3) a function-type parameter default that triggers the explicit
"default expressions are not allowed in function types" branch where parser code
calls p.eat(TokenKind::Equals), captures span via p.current(), calls
p.error(..., span) and then p.parse_expr(). For each test call the parser entry
(same function used by existing unit tests), assert that parsing completes with
the expected error message(s) (including the exact string "default expressions
are not allowed in function types") and that recovery continues (no panic), and
add these as Rust unit tests (not integration tests) alongside other parser unit
tests so they run with cargo test.
In `@baml_language/crates/baml_compiler2_ast/src/lib.rs`:
- Around line 329-370: The test reaches through internal structure via
function.defaults.exprs.exprs[default_id.0]; add a small public accessor on the
FunctionDefaults (or the module that owns defaults) such as a method like
FunctionDefaults::expr(&self, id: ExprId) -> &Expr (or similar name) and update
the test (and any consumers) to call function.defaults.expr(default_id) instead
of indexing into exprs.exprs, so callers don't depend on the nested storage
layout; reference the FunctionDefaults type and the default_id/ExprId used in
the test when implementing the accessor.
In `@baml_language/crates/baml_compiler2_ast/src/lower_expr_body.rs`:
- Around line 1558-1577: The code duplicates building args and inserting label
spans in lower_call_expr and lower_optional_call_expr; extract two helpers
(e.g., finalize_call_args(&mut self, lowered_args: Vec<(CallArg,
Option<TextRange>)>) -> (Vec<CallArg>, Vec<Option<TextRange>>) and
record_call_arg_label_spans(&mut self, call_id: ExprId, label_spans:
Vec<Option<TextRange>>)) and replace the duplicated logic: call
self.finalize_call_args(lowered_args) to get (args, label_spans), alloc the call
via self.alloc_expr(Expr::Call { callee, args }, ...), then call
self.record_call_arg_label_spans(id, label_spans) so both lower_call_expr and
lower_optional_call_expr reuse the same behavior around lower_call_args_node and
source_map.call_arg_label_spans.
- Around line 1591-1613: The fallback that finds a bare token currently uses
skip(if label.is_some() { 2 } else { 0 }) which assumes the label occupies
exactly two non-trivia tokens; update lower_call_arg_node to be defensive by
using the existing label_token/text_range to skip tokens by position instead of
count: if label_token.is_some(), compute its end position
(label_token.text_range().end()) and change the token search to ignore any token
whose text_range().start() <= label_end, then find the first non-trivia,
non-comma token and pass it to lower_bare_token_expr/alloc_expr as before; this
preserves behavior while making the code resilient to grammar changes.
In `@baml_language/crates/baml_compiler2_emit/src/lib.rs`:
- Around line 916-979: Add a small unit test for
compute_function_metadata_from_item_tree that constructs a
baml_compiler2_hir::item_tree::Function with several params (some with default
Some(...) and some with default None), then calls
compute_function_metadata_from_item_tree (providing the minimal
db/file/ResolvedAliases stubs consistent with existing tests in this file) and
asserts the returned param_has_default Vec<bool> matches the expected pattern;
place the test in the existing tests module and follow the same setup/style as
parse_string_attr_value/extract_schema_attrs tests so it runs with cargo test.
In
`@baml_language/crates/baml_compiler2_visualization/src/control_flow/from_ast.rs`:
- Around line 1279-1301: Add unit tests that cover named-argument calls for both
Call and OptionalCall: in addition to the existing call_scope_has_source_expr
test, create tests that build an AST using make_ast_body where the call uses
ast::CallArg::named("label", arg) (and similarly an ast::Expr::OptionalCall if
that variant exists), then run build_control_flow_graph_from_ast and assert you
still get an OtherScope node with source_expr set (like
call_scope_has_source_expr) and additionally assert the
rendered/label-preserving output for that node preserves the argument label (use
the same rendering path your suite uses for nodes). Reference ast::Expr::Call,
ast::Expr::OptionalCall, ast::CallArg::named, make_ast_body,
build_control_flow_graph_from_ast, and NodeType::OtherScope when locating where
to add these tests.
In `@baml_language/crates/baml_fmt/src/ast/types.rs`:
- Around line 1074-1127: Add focused Rust unit tests to cover parsing and
printing of function parameters with optional names and types (e.g. "(name?:
string) -> int") and the paren-type disambiguation around optional types;
specifically add tests that exercise FromCST::from_cst for FunctionTypeParam and
the Printable::print path (FunctionTypeParam::print) to ensure the
name/question/colon printing behavior and the leftmost_token logic are correct.
Create cases for: a) a parameter with name and optional marker before the colon
("name?: string"), b) a parameter with an optional type that could be parsed as
a parenthesized type to ensure disambiguation, and assert round-trip (parse ->
print -> parse) or exact printed output; run cargo test --lib to verify. Ensure
tests reference FunctionTypeParam parsing and printing behavior rather than
other helpers so failures point to FromCST, print, or leftmost_token
implementations.
In `@baml_language/crates/baml_tests/src/compiler2_emit/mod.rs`:
- Around line 29-35: emit_snapshot! duplicates mir_snapshot! — extract a shared
helper to avoid divergence: create a single macro (e.g. snapshot! or
shared_snapshot!) or a small utility module in the baml_tests crate and move the
common invocation (with_settings!({ snapshot_path => SNAPSHOT_PATH,
omit_expression => true }, { assert_snapshot!($name, $output); })) there; then
update both emit_snapshot! and mir_snapshot! (or replace them) to call the
shared symbol, keeping references to with_settings!, SNAPSHOT_PATH,
omit_expression, and assert_snapshot! so settings remain identical across
modules.
- Around line 114-177: The two tests
optional_param_metadata_and_omitted_sentinel_emit and
optional_defaults_emit_snapshot duplicate the same BAML source string passed to
db.add_file; extract that literal into a shared constant or helper to avoid
drift (e.g., define a const TEST_BAML_SOURCE: &str = "..."; or add a helper fn
add_optional_defaults_test_file(db: &mut Db) that calls db.add_file with the
source) and update both tests to use the constant/helper so the source remains
single-sourced.
In `@baml_language/crates/baml_tests/src/compiler2_tir/mod.rs`:
- Around line 234-239: The snapshot renderer currently collapses default values
to "?" by checking p.default.is_some(); instead, render the actual default
expression (or a distinct placeholder containing the default text) so different
defaults are distinguishable. Update the formatting in the blocks that build
parameter strings (the shown block and the other similar blocks at lines
referenced) to include the default content from p.default (e.g., append " =
<default_expr>" or embed the default string) when p.default.is_some(), using
p.default's expression/text instead of only emitting "?"; apply the same change
to the other occurrences mentioned (around the code that formats parameters at
the other line ranges).
In `@baml_language/crates/bex_vm_types/src/types.rs`:
- Around line 528-667: Add a Rust unit test in this module (#[cfg(test)] mod
tests) that ensures the omitted-arg variants stay in sync: call
ConstValue::OmittedArg.to_value(...) with a simple resolver and assert it yields
Value::OmittedArg; pass that Value::OmittedArg into Type::of(...) and assert the
returned runtime Type is the OmittedArg variant; and assert format!("{}",
Value::OmittedArg) == "<omitted>" (uses Display impl). Name the test clearly
(e.g., omitted_arg_roundtrip_or_sync) and keep it local to this file so it runs
as a unit test.
In `@baml_language/crates/bex_vm/src/vm.rs`:
- Line 499: Add a unit test exercising Value::OmittedArg through value_type_tag
and the TypeTag/IsType APIs: create a Value::OmittedArg, assert
value_type_tag(&Value::OmittedArg) returns type_tags::UNKNOWN (or the TypeTag
variant equivalent), and assert the IsType checks behave as expected for
UNKNOWN; place the test in the same Rust source file as the implementation
(#[cfg(test)] mod tests) to follow unit-test guidance and run cargo test --lib
to confirm all library tests pass.
In `@baml_language/languages/python/tests/test_sim_phase3.py`:
- Around line 76-85: The async test needs the same mutable-default regression
check as the sync path: in test_generated_client_defaulted_params_async call
mutate_default_async() a second time and assert the result is still the initial
value to ensure defaults aren't shared; specifically add another assertion like
"assert await mutate_default_async() == 1" after the existing
mutate_default_async() call in the test_generated_client_defaulted_params_async
test to mirror the sync-case protection for defaulted items.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
baml_language/languages/python/tests/test_sim_phase3.py (1)
76-87: ⚡ Quick winAsync twin is missing three cases from the sync suite.
The following cases present in
test_generated_client_defaulted_params_syncare absent from the async version:
- Named-only positional call:
await default_score_async(query="cats", filter="recent") == 110TypeErrorguard for invalid positional:pytest.raises(TypeError)withawait default_score_async("cats", 5)- Explicit override of mutable default:
await mutate_default_async(items=[1, 2]) == 3Without (3), the async path is never verified to produce anything other than
1, making it hard to distinguish a correct implementation from a stub.✅ Proposed additions to bring async in line with sync
`@pytest.mark.asyncio` async def test_generated_client_defaulted_params_async(): from baml.baml_core import UNSET from baml_sdk.lorem import default_score_async, mutate_default_async assert await default_score_async("cats") == 10 assert await default_score_async("cats", max_results=5) == 5 + assert await default_score_async(query="cats", filter="recent") == 110 assert await default_score_async("cats", filter=None) == 10 assert await default_score_async("cats", max_results=UNSET, filter="recent") == 110 + + with pytest.raises(TypeError): + await default_score_async("cats", 5) + assert await mutate_default_async() == 1 assert await mutate_default_async() == 1 + assert await mutate_default_async(items=[1, 2]) == 3🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/languages/python/tests/test_sim_phase3.py` around lines 76 - 87, Add the three missing async test cases to mirror the sync suite: call default_score_async with a named-only positional form using keyword args (await default_score_async(query="cats", filter="recent") and assert it equals 110), add a pytest.raises(TypeError) check for an invalid positional call (await default_score_async("cats", 5)), and add an explicit override of the mutable default for mutate_default_async (await mutate_default_async(items=[1,2]) and assert it equals 3); ensure imports (UNSET) and existing async calls to default_score_async and mutate_default_async remain unchanged so test_generated_client_defaulted_params_async covers the same cases as the sync version.baml_language/crates/tools_onionskin/src/compiler.rs (1)
2170-2191: 💤 Low valueSurface optional parameter defaults in TIR2 signature rendering.
Since this PR's main feature is optional function parameters with defaults, the rendered signatures at both locations could be more informative by displaying whether a param has a default (e.g.,
name: T = <default>orname?: T). Currently both required and defaulted params render identically asname: T, which loses information relevant when inspecting TIR2 output in this dev tool. TheSignatureParamstruct includes adefault: Option<DefaultExprRef>field that's available but currently unused.Low priority since this is a developer-visualization tool, not user-facing output.
Also applies to: 2748-2762
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/tools_onionskin/src/compiler.rs` around lines 2170 - 2191, The TIR2 signature rendering currently ignores SignatureParam.default and always prints params as "name: T"; update the param mapping in the function that calls baml_compiler2_ppir::function_signature (the block that builds params Vec<String> and sets sig_display) to inspect each SignatureParam.default: if default.is_some() render either "name?: <Type>" or "name: <Type> = <defaultExpr>" (choose your preferred style), using hir2_type_expr_to_string(¶m.ty) for the type and a helper to stringify the DefaultExprRef (or implement a small default_expr_to_string) to produce the default text; apply the same change to the other signature-rendering location that constructs params (the analogous params mapping later in the file) so optional parameters and their defaults are visible in both places.baml_language/crates/baml_project/src/client_codegen.rs (2)
50-58: 💤 Low valueNon-empty array/map literals silently fall through to
Expression; a comment would help future maintainers.The guards
if elements.is_empty()/if entries.is_empty()mean that[1, 2, 3]or{"a": 1}literal defaults are caught by the_arm and emitted asExpression { source }instead of any structured literal form. This is presumably intentional (Python uses_UNSETregardless; other backends read the raw source text), but it's invisible to anyone adding a new codegen target later that wants to distinguish concrete-list defaults from opaque call-expression defaults.💡 Suggested clarification
_ => Some(cg::FunctionArgumentDefault::Expression { + // Non-empty array/map literals and arbitrary call expressions all + // land here. Codegen backends that need structured literal support + // should add explicit match arms above before this catch-all. source: Some(defaults.exprs.display_expr(default_ref.expr.0)), }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_project/src/client_codegen.rs` around lines 50 - 58, Add a clarifying comment in the code surrounding the match that produces cg::FunctionArgumentDefault (the arm that returns Literal::EmptyList / EmptyMap and the fallback that returns Expression with defaults.exprs.display_expr(default_ref.expr.0)) explaining that non-empty array/map AST literals intentionally fall through to the Expression branch (so concrete non-empty literals are emitted as source expressions rather than structured literals), and note this is intentional for downstream backends that treat non-empty literals as opaque; reference cg::FunctionArgumentDefault::Literal, cg::DefaultLiteral::EmptyList, cg::DefaultLiteral::EmptyMap, and defaults.exprs.display_expr(DEFAULT_REF_EXPR) in the comment for future maintainers.
774-827: ⚡ Quick winMigrate to a direct unit test for
lower_codegen_default.The current integration test exercises all five branches of
lower_codegen_defaultthrough the full compiler pipeline, but per the Rust guideline, a lightweight unit test is preferred.FunctionDefaults::empty()is public and trivial (lines 1072–1077 inbaml_compiler2_ast/src/ast.rs), and patterns elsewhere in the codebase (e.g.,baml_compiler2_emit/src/lib.rs:1578–1579) show how to construct test fixtures directly by allocating expressions into theFunctionDefaults.exprsarena without spinning upProjectDatabase. This refactor would yield faster, more isolated, easier-to-maintain tests with minimal effort.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_project/src/client_codegen.rs` around lines 774 - 827, Replace the heavy integration test with a direct unit test that exercises lower_codegen_default by constructing a FunctionDefaults fixture using FunctionDefaults::empty(), allocating the necessary expressions into the FunctionDefaults.exprs arena (mirroring the pattern used in baml_compiler2_emit tests) and then calling lower_codegen_default directly; specifically, build distinct expr entries for the five branches (no default, literal int, expression via an allocated expr that calls default_filter(), empty list literal, and nullable null), pass that FunctionDefaults into lower_codegen_default and assert the resulting cg::FunctionArgumentDefault variants match the expectations previously checked in test_function_defaults_populate_codegen_metadata. Ensure you reference lower_codegen_default and FunctionDefaults::exprs/FunctionDefaults::empty when locating the code to modify.baml_language/crates/baml_compiler2_ast/src/lower_cst.rs (1)
302-359: ⚡ Quick winPast index-misalignment concern is correctly addressed.
lowered_param_idx = params.len()captured beforeparams.push(param)is the post-filter index, so subsequentparams.get_mut(idx)and theself-default check at lines 347–353 reference the right positions even when earlierlower_paramcalls returnedNone. Theusize::MAXsentinel in the failed-lower branch is effectively a no-op againstget_mut, which is fine becauseMissingParamNamealready surfaces the underlying issue.One small follow-up worth considering: pin the previously-broken case (
function f(: int, b: string = "x") {}) as a unit test so the post-filter indexing doesn't silently regress. As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_compiler2_ast/src/lower_cst.rs` around lines 302 - 359, Add a unit test to lock in the previously-broken indexing behavior: create a test that calls lower_params_with_defaults (via the public lowering entry that uses it) on a function signature like `fn f(: int, b: string = "x") {}` and asserts that the default for `b` is attached to the correct parameter (i.e., params list index for `b`) and that no default was attached to the missing name slot; place the test in the same crate (unit test module) so it exercises lower_params_with_defaults and will catch regressions of the post-filter indexing logic (refer to lower_params_with_defaults, lower_param, and lower_expr_body::lower_default_expr_nodes in your assertions).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/baml_compiler2_emit/src/lib.rs`:
- Around line 743-745: The synthesized function metadata for "$init_test" has
mismatched vectors—param_names has one entry and param_has_default has one entry
but param_types is empty; add a corresponding entry to the param_types Vec so
all three vectors remain parallel (e.g., push the appropriate placeholder/type
for the "registry" parameter) in the same block that constructs
param_names/param_types/param_has_default to keep the function parameter
metadata aligned.
---
Nitpick comments:
In `@baml_language/crates/baml_compiler2_ast/src/lower_cst.rs`:
- Around line 302-359: Add a unit test to lock in the previously-broken indexing
behavior: create a test that calls lower_params_with_defaults (via the public
lowering entry that uses it) on a function signature like `fn f(: int, b: string
= "x") {}` and asserts that the default for `b` is attached to the correct
parameter (i.e., params list index for `b`) and that no default was attached to
the missing name slot; place the test in the same crate (unit test module) so it
exercises lower_params_with_defaults and will catch regressions of the
post-filter indexing logic (refer to lower_params_with_defaults, lower_param,
and lower_expr_body::lower_default_expr_nodes in your assertions).
In `@baml_language/crates/baml_project/src/client_codegen.rs`:
- Around line 50-58: Add a clarifying comment in the code surrounding the match
that produces cg::FunctionArgumentDefault (the arm that returns
Literal::EmptyList / EmptyMap and the fallback that returns Expression with
defaults.exprs.display_expr(default_ref.expr.0)) explaining that non-empty
array/map AST literals intentionally fall through to the Expression branch (so
concrete non-empty literals are emitted as source expressions rather than
structured literals), and note this is intentional for downstream backends that
treat non-empty literals as opaque; reference
cg::FunctionArgumentDefault::Literal, cg::DefaultLiteral::EmptyList,
cg::DefaultLiteral::EmptyMap, and defaults.exprs.display_expr(DEFAULT_REF_EXPR)
in the comment for future maintainers.
- Around line 774-827: Replace the heavy integration test with a direct unit
test that exercises lower_codegen_default by constructing a FunctionDefaults
fixture using FunctionDefaults::empty(), allocating the necessary expressions
into the FunctionDefaults.exprs arena (mirroring the pattern used in
baml_compiler2_emit tests) and then calling lower_codegen_default directly;
specifically, build distinct expr entries for the five branches (no default,
literal int, expression via an allocated expr that calls default_filter(), empty
list literal, and nullable null), pass that FunctionDefaults into
lower_codegen_default and assert the resulting cg::FunctionArgumentDefault
variants match the expectations previously checked in
test_function_defaults_populate_codegen_metadata. Ensure you reference
lower_codegen_default and FunctionDefaults::exprs/FunctionDefaults::empty when
locating the code to modify.
In `@baml_language/crates/tools_onionskin/src/compiler.rs`:
- Around line 2170-2191: The TIR2 signature rendering currently ignores
SignatureParam.default and always prints params as "name: T"; update the param
mapping in the function that calls baml_compiler2_ppir::function_signature (the
block that builds params Vec<String> and sets sig_display) to inspect each
SignatureParam.default: if default.is_some() render either "name?: <Type>" or
"name: <Type> = <defaultExpr>" (choose your preferred style), using
hir2_type_expr_to_string(¶m.ty) for the type and a helper to stringify the
DefaultExprRef (or implement a small default_expr_to_string) to produce the
default text; apply the same change to the other signature-rendering location
that constructs params (the analogous params mapping later in the file) so
optional parameters and their defaults are visible in both places.
In `@baml_language/languages/python/tests/test_sim_phase3.py`:
- Around line 76-87: Add the three missing async test cases to mirror the sync
suite: call default_score_async with a named-only positional form using keyword
args (await default_score_async(query="cats", filter="recent") and assert it
equals 110), add a pytest.raises(TypeError) check for an invalid positional call
(await default_score_async("cats", 5)), and add an explicit override of the
mutable default for mutate_default_async (await
mutate_default_async(items=[1,2]) and assert it equals 3); ensure imports
(UNSET) and existing async calls to default_score_async and mutate_default_async
remain unchanged so test_generated_client_defaulted_params_async covers the same
cases as the sync version.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 51aca513-0b6b-49ed-8d63-8e113fb9f36a
⛔ Files ignored due to path filters (8)
baml_language/crates/baml_tests/snapshots/compiler2_mir/baml_tests__compiler2_mir__optional_named_reordered_args_evaluate_in_source_order.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/optional_function_parameters/baml_tests__compiles__optional_function_parameters__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/optional_function_parameters/baml_tests__compiles__optional_function_parameters__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/optional_parameter_defaults/baml_tests__diagnostic_errors__optional_parameter_defaults__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/optional_parameter_defaults/baml_tests__diagnostic_errors__optional_parameter_defaults__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase3a__optional_param_call_binding_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase3a__optional_param_default_declaration_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase3a__optional_params_accept_omission_and_named_override.snapis excluded by!**/*.snap
📒 Files selected for processing (39)
baml_language/crates/baml_cli/src/test_command.rsbaml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_ast/src/lib.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_ast/src/lower_expr_body.rsbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/callable.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/package_interface.rsbaml_language/crates/baml_compiler2_visualization/src/control_flow/from_ast.rsbaml_language/crates/baml_compiler_parser/src/parser.rsbaml_language/crates/baml_fmt/src/ast/declarations.rsbaml_language/crates/baml_fmt/src/ast/expressions.rsbaml_language/crates/baml_fmt/src/ast/types.rsbaml_language/crates/baml_lsp2_actions/src/annotations.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_lsp2_actions/src/completions.rsbaml_language/crates/baml_lsp2_actions/src/describe.rsbaml_language/crates/baml_lsp2_actions/src/utils.rsbaml_language/crates/baml_project/src/client_codegen.rsbaml_language/crates/baml_tests/src/compiler2_emit/mod.rsbaml_language/crates/baml_tests/src/compiler2_mir/mod.rsbaml_language/crates/baml_tests/src/compiler2_tir/inference.rsbaml_language/crates/baml_tests/src/compiler2_tir/mod.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase3a.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase8_exceptions.rsbaml_language/crates/baml_tests/src/engine.rsbaml_language/crates/baml_tests/src/lib.rsbaml_language/crates/baml_tests/tests/optional_function_parameters.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_project/src/bex.rsbaml_language/crates/bex_vm/src/vm.rsbaml_language/crates/bex_vm_types/src/types.rsbaml_language/crates/tools_onionskin/src/compiler.rsbaml_language/languages/python/rust/codegen_python/src/translate_ty.rsbaml_language/languages/python/src/baml/baml_core/__init__.pybaml_language/languages/python/tests/test_sim_phase3.py
✅ Files skipped from review due to trivial changes (1)
- baml_language/crates/baml_tests/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (21)
- baml_language/crates/baml_lsp2_actions/src/completions.rs
- baml_language/crates/baml_lsp2_actions/src/annotations.rs
- baml_language/crates/baml_tests/src/compiler2_emit/mod.rs
- baml_language/crates/baml_tests/src/compiler2_tir/phase3a.rs
- baml_language/languages/python/rust/codegen_python/src/translate_ty.rs
- baml_language/crates/baml_lsp2_actions/src/utils.rs
- baml_language/crates/baml_cli/src/test_command.rs
- baml_language/crates/baml_fmt/src/ast/declarations.rs
- baml_language/crates/bex_vm_types/src/types.rs
- baml_language/crates/baml_tests/src/compiler2_mir/mod.rs
- baml_language/crates/baml_tests/tests/optional_function_parameters.rs
- baml_language/crates/bex_engine/src/lib.rs
- baml_language/languages/python/src/baml/baml_core/init.py
- baml_language/crates/baml_lsp2_actions/src/check.rs
- baml_language/crates/baml_lsp2_actions/src/describe.rs
- baml_language/crates/baml_tests/src/engine.rs
- baml_language/crates/baml_fmt/src/ast/expressions.rs
- baml_language/crates/baml_compiler2_tir/src/inference.rs
- baml_language/crates/baml_compiler2_ast/src/ast.rs
- baml_language/crates/baml_fmt/src/ast/types.rs
- baml_language/crates/baml_compiler2_tir/src/builder.rs
👮 Files not reviewed due to content moderation or server errors (1)
- baml_language/crates/baml_compiler2_mir/src/lower.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/crates/tools_onionskin/src/compiler.rs (1)
511-514: 💤 Low valueDuplicated label-rendering block across
Expr::CallandExpr::OptionalCall.The four-line pattern that checks
arg.labeland pushes the"{label} = "span is copy-pasted verbatim in both match arms. Extracting it to a local closure eliminates the duplication:♻️ Suggested refactor
+ let push_arg = |spans: &mut Vec<DetailSpan>, arg: &baml_compiler2_ast::CallArg| { + if let Some(label) = &arg.label { + spans.push(DetailSpan::Code(format!("{label} = "))); + } + spans.extend(expr_desc_spans(arg.expr, body, inference)); + }; + Expr::Call { callee, args } => { spans.extend(expr_desc_spans(*callee, body, inference)); spans.push(DetailSpan::Code("(".into())); for (i, arg) in args.iter().enumerate() { if i > 0 { spans.push(DetailSpan::Code(", ".into())); } - if let Some(label) = &arg.label { - spans.push(DetailSpan::Code(format!("{label} = "))); - } - spans.extend(expr_desc_spans(arg.expr, body, inference)); + push_arg(&mut spans, arg); } spans.push(DetailSpan::Code(")".into())); } // … OptionalCall arm similarly uses push_arg …Also applies to: 591-594
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/tools_onionskin/src/compiler.rs` around lines 511 - 514, Duplicate label-rendering logic in the Expr::Call and Expr::OptionalCall arms is copy-pasted; extract the four-line pattern that checks arg.label and pushes DetailSpan::Code(format!("{label} = ")) followed by spans.extend(expr_desc_spans(arg.expr, body, inference)) into a small local closure (e.g., render_arg_label_and_spans) inside the function that builds expression spans; then call that closure from both Expr::Call and Expr::OptionalCall (and the other occurrence around lines 591-594) so Expr handling reuses the helper and avoids duplication while still using expr_desc_spans, DetailSpan::Code, and the arg.label field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@baml_language/crates/tools_onionskin/src/compiler.rs`:
- Around line 511-514: Duplicate label-rendering logic in the Expr::Call and
Expr::OptionalCall arms is copy-pasted; extract the four-line pattern that
checks arg.label and pushes DetailSpan::Code(format!("{label} = ")) followed by
spans.extend(expr_desc_spans(arg.expr, body, inference)) into a small local
closure (e.g., render_arg_label_and_spans) inside the function that builds
expression spans; then call that closure from both Expr::Call and
Expr::OptionalCall (and the other occurrence around lines 591-594) so Expr
handling reuses the helper and avoids duplication while still using
expr_desc_spans, DetailSpan::Code, and the arg.label field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 196cfeb5-54bb-4d70-8d7f-f487bca5eb92
📒 Files selected for processing (6)
baml_language/crates/baml_compiler2_ast/src/lib.rsbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_project/src/client_codegen.rsbaml_language/crates/baml_tests/src/compiler2_emit/mod.rsbaml_language/crates/tools_onionskin/src/compiler.rsbaml_language/languages/python/tests/test_sim_phase3.py
🚧 Files skipped from review as they are similar to previous changes (4)
- baml_language/crates/baml_compiler2_ast/src/lib.rs
- baml_language/languages/python/tests/test_sim_phase3.py
- baml_language/crates/baml_project/src/client_codegen.rs
- baml_language/crates/baml_compiler2_emit/src/lib.rs
rossirpaulo
force-pushed
the
paulo/optional-function-parameters
branch
from
May 7, 2026 14:50
4f064e5 to
b967395
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
baml_language/crates/baml_compiler2_tir/src/package_interface.rs (1)
261-281: 🏗️ Heavy liftSelf-Unknown fix looks good; consider extracting the shared method-lowering logic.
The implicit-
selfsubstitution viabuild_self_type_for_classnow correctly mirrorslookup_own_class_method, fixing the priorTy::Unknowndivergence.That said, the entire method-signature lowering block here (params loop with self-Unknown handling, return type, declared/callable throws, builtin_kind) is duplicated almost verbatim in
lookup_own_class_method(lines 805–890). Now that the self-handling has to stay in sync between the two, the duplication is a real maintenance hazard — a future tweak (e.g., new param metadata, default-expression lowering for methods) would need to be applied in both places or behavior will silently diverge. Worth extracting a shared helper that takes(db, class_loc, class_data, method_id, ns, &mut diags)and returns the lowered param/return/throws/builtin tuple, then have both call sites construct their respectiveExportedFunction/ResolvedFunctionfrom it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_compiler2_tir/src/package_interface.rs` around lines 261 - 281, The params/return/throws/builtin lowering logic (the loop using build_self_type_for_class, lower_type_expr_in_ns and exported_function_param) is duplicated between this block and lookup_own_class_method; extract it into a single helper (e.g., lower_method_signature or lower_class_method_signature) that accepts (db, class_loc/class_data, method_id or sig, ns_path, pkg_items, all_generic_params, &mut diags) and returns the normalized tuple of (Vec<ExportedFunctionParam>, return_type, declared_throws, callable_throws, builtin_kind) so both sites can call the helper and then construct their ExportedFunction or ResolvedFunction from the returned pieces. Update both call sites to use that helper and remove the duplicated logic, preserving use of build_self_type_for_class, lower_type_expr_in_ns, and exported_function_param inside the new function.baml_language/crates/baml_tests/src/compiler2_tir/phase3a.rs (1)
201-214: ⚡ Quick winMissing
insta::assert_snapshot!— secondary diagnostics in the lambda forward-reference test are uncovered
optional_param_default_forward_reference_checks_lambda_bodiesonly asserts that the one forward-reference diagnostic is present but doesn't snapshot the full TIR output. If the compiler emits additional diagnostics (e.g., unresolved namebinside the lambda body) or future changes accidentally suppress the forward-reference error while adding others, the singleassert!won't catch it.Add an
insta::assert_snapshot!call before the targeted assertion to lock in the complete output:💡 Suggested addition
let tir = render_tir(&db, file); + insta::assert_snapshot!("optional_param_default_forward_reference_checks_lambda_bodies", tir); assert!( tir.contains("default for parameter `a` cannot reference later parameter `b`"), "{tir}" );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_tests/src/compiler2_tir/phase3a.rs` around lines 201 - 214, Add an insta snapshot of the full TIR output in the test optional_param_default_forward_reference_checks_lambda_bodies: call insta::assert_snapshot! on the tir produced by render_tir(&db, file) (the tir variable) before the existing assert! so the entire compiler output (including any secondary diagnostics from lambda bodies) is locked in; keep the existing assert! that checks for the specific message after inserting the snapshot.baml_language/crates/baml_tests/src/compiler2_tir/mod.rs (1)
46-56: 💤 Low value
default_ref_suffix: thedefault.expr.expr()chain warrants a clarifying comment
DefaultExprRef.expris a field (of typeDefaultExprId) and.expr()is a method on that type returning anExprId. The double.expr.expr()is hard to parse at a glance. A brief comment or an intermediate binding would aid future readers, e.g.:💡 Suggested clarification
fn default_ref_suffix(default: Option<&DefaultExprRef>, defaults: &FunctionDefaults) -> String { default - .map(|default| format!(" = {}", defaults.exprs.display_expr(default.expr.expr()))) + .map(|default| { + // DefaultExprRef.expr: DefaultExprId → .expr(): ExprId + let expr_id = default.expr.expr(); + format!(" = {}", defaults.exprs.display_expr(expr_id)) + }) .unwrap_or_default() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_tests/src/compiler2_tir/mod.rs` around lines 46 - 56, In default_ref_suffix the expression chain default.expr.expr() is confusing because DefaultExprRef has a field named expr (a DefaultExprId) and that type exposes an .expr() method returning an ExprId; update default_ref_suffix (or the surrounding code) to either add a short clarifying comment explaining "default.expr is a DefaultExprId; .expr() returns an ExprId" or introduce a small intermediate binding (e.g., let default_expr_id = default.expr.expr(); then call defaults.exprs.display_expr(default_expr_id)) to make intent clear and improve readability for future maintainers.baml_language/crates/baml_cli/src/run_command.rs (1)
331-336: ⚡ Quick winSurface optional/defaulted parameters in
--helpoutput.Now that defaults are first-class in binding, target help still presents all params as if required. Threading
func_info.param_has_defaultinto help text would make invocation guidance accurate.Possible update in help rendering
- for (name, ty) in param_names.iter().zip(param_types.iter()) { + for ((name, ty), has_default) in param_names + .iter() + .zip(param_types.iter()) + .zip(func_info.param_has_default.iter()) + { let type_hint = match ty { @@ _ => String::new(), }; - println!(" --{name} <{ty}>{type_hint}"); + let req_hint = if *has_default { " (optional, has default)" } else { " (required)" }; + println!(" --{name} <{ty}>{type_hint}{req_hint}"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_cli/src/run_command.rs` around lines 331 - 336, The target help incorrectly shows all parameters as required because default info isn't used; update the help rendering path that calls build_args_from to pass and use func_info.param_has_default so optional/defaulted params are marked in help. Locate build_args_from and the code that formats help for target parameters (the call using &effective_target_args and &func_info.param_names/param_types) and thread param_has_default through into the help formatter so flags with a true param_has_default are annotated (e.g., “[optional]” or show “=DEFAULT”) and not shown as required. Ensure the help text generation logic checks func_info.param_has_default for each parameter when composing usage and flag descriptions.baml_language/crates/bex_engine/src/lib.rs (1)
861-917: ⚡ Quick winValidate bound-args arity/default compatibility before entering the VM.
call_function_bound_argsis public and currently accepts anyVec<BexCallArg>. Adding a preflight check (arity +OmittedDefaultonly whereparam_has_default=true) would prevent opaque VM/runtime failures and return a clearerEngineError::TypeMismatchearlier.Proposed guard
pub async fn call_function_bound_args( self: &Arc<Self>, function_name: &str, args: Vec<BexCallArg>, @@ ) -> Result<BexExternalValue, EngineError> { + let params = self.function_params(function_name)?; + if args.len() != params.len() { + return Err(EngineError::TypeMismatch { + message: format!( + "Function `{function_name}` expects {} argument(s), got {}", + params.len(), + args.len() + ), + }); + } + for (idx, (arg, (param_name, _ty, has_default))) in args.iter().zip(params.iter()).enumerate() { + if matches!(arg, BexCallArg::OmittedDefault) && !*has_default { + return Err(EngineError::TypeMismatch { + message: format!( + "Argument `{param_name}` at position {idx} cannot be omitted (no default value)" + ), + }); + } + } + // Fail fast if already cancelled — guarantees pre-cancelled tokens🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/bex_engine/src/lib.rs` around lines 861 - 917, call_function_bound_args should validate the provided args vector against the target function's parameter list before creating the VM: after computing function_index (via lookup_function) but before VM creation/use, fetch the target function's parameter metadata (parameter count and per-param has_default flags), ensure args.len() equals the number of parameters and that any BexCallArg::OmittedDefault only appears at positions where the corresponding parameter has_default == true; on violation return Err(EngineError::TypeMismatch) with a clear message. Use the existing function_name to obtain the signature/parameter info (the same place/function you use in function_return_type/function_throws_type) and perform this guard early in call_function_bound_args to prevent VM-time failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/baml_lsp2_actions/src/completions.rs`:
- Around line 445-478: The completion list currently only excludes named args
via provided_named_args(&args_node) and thus suggests parameters already
consumed positionally; fix by also counting positional arguments before the
cursor and treating the first N parameters as already provided. In
completions_for_value_position (or the function containing the shown loop)
compute positional_count by iterating args_node.children() (CALL_ARG nodes),
considering only args that appear before the current offset and that have no
label, then when building items skip the first positional_count entries from
params (or filter params by their index < positional_count). Update
provided_named_args usage or replace it with a helper that returns both a
HashSet of named labels and the positional count so the loop can exclude
parameters either present in the named set or already consumed positionally.
- Around line 231-233: The current early return using
find_call_args_ancestor(token) is too broad; restrict
CompletionContext::CallArguments to only when the token is directly in the
immediate arg-list/label position (not inside a nested expression, lambda, block
or other value subtree). Replace the unconditional is_some() check with a test
that the CALL_ARGS ancestor is the immediate container of the token (e.g., the
path-length/depth from the CALL_ARGS node to token is 1 or the token's direct
parent is the CALL_ARGS and not an expression node), or explicitly guard against
intermediate kinds like LAMBDA, BLOCK, CALL, or value-expr nodes before
returning CompletionContext::CallArguments; apply the same narrower logic where
find_call_args_ancestor is used in the other occurrence.
---
Nitpick comments:
In `@baml_language/crates/baml_cli/src/run_command.rs`:
- Around line 331-336: The target help incorrectly shows all parameters as
required because default info isn't used; update the help rendering path that
calls build_args_from to pass and use func_info.param_has_default so
optional/defaulted params are marked in help. Locate build_args_from and the
code that formats help for target parameters (the call using
&effective_target_args and &func_info.param_names/param_types) and thread
param_has_default through into the help formatter so flags with a true
param_has_default are annotated (e.g., “[optional]” or show “=DEFAULT”) and not
shown as required. Ensure the help text generation logic checks
func_info.param_has_default for each parameter when composing usage and flag
descriptions.
In `@baml_language/crates/baml_compiler2_tir/src/package_interface.rs`:
- Around line 261-281: The params/return/throws/builtin lowering logic (the loop
using build_self_type_for_class, lower_type_expr_in_ns and
exported_function_param) is duplicated between this block and
lookup_own_class_method; extract it into a single helper (e.g.,
lower_method_signature or lower_class_method_signature) that accepts (db,
class_loc/class_data, method_id or sig, ns_path, pkg_items, all_generic_params,
&mut diags) and returns the normalized tuple of (Vec<ExportedFunctionParam>,
return_type, declared_throws, callable_throws, builtin_kind) so both sites can
call the helper and then construct their ExportedFunction or ResolvedFunction
from the returned pieces. Update both call sites to use that helper and remove
the duplicated logic, preserving use of build_self_type_for_class,
lower_type_expr_in_ns, and exported_function_param inside the new function.
In `@baml_language/crates/baml_tests/src/compiler2_tir/mod.rs`:
- Around line 46-56: In default_ref_suffix the expression chain
default.expr.expr() is confusing because DefaultExprRef has a field named expr
(a DefaultExprId) and that type exposes an .expr() method returning an ExprId;
update default_ref_suffix (or the surrounding code) to either add a short
clarifying comment explaining "default.expr is a DefaultExprId; .expr() returns
an ExprId" or introduce a small intermediate binding (e.g., let default_expr_id
= default.expr.expr(); then call defaults.exprs.display_expr(default_expr_id))
to make intent clear and improve readability for future maintainers.
In `@baml_language/crates/baml_tests/src/compiler2_tir/phase3a.rs`:
- Around line 201-214: Add an insta snapshot of the full TIR output in the test
optional_param_default_forward_reference_checks_lambda_bodies: call
insta::assert_snapshot! on the tir produced by render_tir(&db, file) (the tir
variable) before the existing assert! so the entire compiler output (including
any secondary diagnostics from lambda bodies) is locked in; keep the existing
assert! that checks for the specific message after inserting the snapshot.
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 861-917: call_function_bound_args should validate the provided
args vector against the target function's parameter list before creating the VM:
after computing function_index (via lookup_function) but before VM creation/use,
fetch the target function's parameter metadata (parameter count and per-param
has_default flags), ensure args.len() equals the number of parameters and that
any BexCallArg::OmittedDefault only appears at positions where the corresponding
parameter has_default == true; on violation return
Err(EngineError::TypeMismatch) with a clear message. Use the existing
function_name to obtain the signature/parameter info (the same place/function
you use in function_return_type/function_throws_type) and perform this guard
early in call_function_bound_args to prevent VM-time failures.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ce7b534f-4c1e-48b5-8c3a-b3e02dda1485
⛔ Files ignored due to path filters (2)
baml_language/crates/baml_tests/snapshots/diagnostic_errors/optional_parameter_defaults/baml_tests__diagnostic_errors__optional_parameter_defaults__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/optional_parameter_defaults/baml_tests__diagnostic_errors__optional_parameter_defaults__05_diagnostics.snapis excluded by!**/*.snap
📒 Files selected for processing (35)
baml_language/crates/baml_cli/src/run_command.rsbaml_language/crates/baml_codegen_types/src/ty.rsbaml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_ast/src/lower_expr_body.rsbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/signature.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_ppir/src/lib.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/package_interface.rsbaml_language/crates/baml_compiler_parser/src/parser.rsbaml_language/crates/baml_fmt/src/ast/expressions.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_lsp2_actions/src/completions.rsbaml_language/crates/baml_lsp2_actions/src/completions_tests.rsbaml_language/crates/baml_lsp2_actions/src/describe.rsbaml_language/crates/baml_lsp2_actions/src/lib.rsbaml_language/crates/baml_lsp2_actions/src/type_info.rsbaml_language/crates/baml_lsp2_actions/src/type_info_tests.rsbaml_language/crates/baml_lsp2_actions/src/utils.rsbaml_language/crates/baml_project/src/client_codegen.rsbaml_language/crates/baml_tests/src/compiler2_hir.rsbaml_language/crates/baml_tests/src/compiler2_tir/inference.rsbaml_language/crates/baml_tests/src/compiler2_tir/mod.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase3a.rsbaml_language/crates/baml_tests/src/engine.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_project/src/bex_lsp/multi_project/request.rsbaml_language/crates/tools_onionskin/src/compiler.rsbaml_language/languages/python/rust/codegen_python/src/leaf.rsbaml_language/languages/python/rust/codegen_python/src/lib.rsbaml_language/languages/python/tests/test_sim_phase3.py
✅ Files skipped from review due to trivial changes (1)
- baml_language/crates/baml_lsp2_actions/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- baml_language/crates/baml_lsp2_actions/src/utils.rs
- baml_language/crates/tools_onionskin/src/compiler.rs
- baml_language/crates/baml_compiler2_ast/src/lower_expr_body.rs
- baml_language/crates/baml_lsp2_actions/src/check.rs
- baml_language/crates/baml_codegen_types/src/ty.rs
- baml_language/crates/baml_tests/src/engine.rs
- baml_language/crates/baml_fmt/src/ast/expressions.rs
- baml_language/crates/baml_project/src/client_codegen.rs
- baml_language/crates/baml_compiler2_hir/src/builder.rs
- baml_language/crates/baml_compiler_parser/src/parser.rs
- baml_language/crates/baml_compiler2_ast/src/lower_cst.rs
- baml_language/crates/baml_compiler2_tir/src/inference.rs
- baml_language/crates/baml_compiler2_mir/src/lower.rs
- baml_language/crates/baml_compiler2_emit/src/lib.rs
- baml_language/crates/baml_tests/src/compiler2_tir/inference.rs
- baml_language/crates/baml_compiler2_tir/src/builder.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/crates/baml_cli/src/run_command.rs (1)
871-897:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThread default metadata into positional sugar too.
build_args_from(...)now knows which params are defaulted, butparse_auto_cli_args(...)still decides bare-token positional sugar fromparam_names.len() == 1. That meansfunction Search(query: string, max_results: int = 10)still rejectsbaml run --function Search -- cats, even though there is only one required parameter and the new omission path can fill the rest. Please passparam_has_defaultthrough and bind the lone required param instead.Suggested direction
- let cli_map = parse_auto_cli_args(target_args, param_names, param_types)?; + let cli_map = + parse_auto_cli_args(target_args, param_names, param_types, param_has_default)?;fn parse_auto_cli_args( tokens: &[String], param_names: &[String], param_types: &[Ty], param_has_default: &[bool], ) -> Result<HashMap<String, BexExternalValue>> { if tokens.is_empty() || param_names.is_empty() { return Ok(HashMap::new()); } let required_params: Vec<usize> = param_names .iter() .enumerate() .filter_map(|(idx, _)| (!param_has_default.get(idx).copied().unwrap_or(false)).then_some(idx)) .collect(); if tokens.len() == 1 && !tokens[0].starts_with("--") && required_params.len() == 1 { let idx = required_params[0]; let value = parse_cli_value(&tokens[0], ¶m_types[idx]) .with_context(|| format!("Invalid value for `{}`: {}", param_names[idx], tokens[0]))?; let mut map = HashMap::new(); map.insert(param_names[idx].clone(), value); return Ok(map); } // existing parsing... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_cli/src/run_command.rs` around lines 871 - 897, parse_auto_cli_args currently decides the single-token positional sugar only from param_names.len() == 1, so functions with one required param and other defaulted params (tracked by build_args_from via param_has_default) fail to accept a bare token; update parse_auto_cli_args signature to accept param_has_default: &[bool] and change its logic to compute required parameters (indexes where param_has_default[idx] is false), then if tokens.len() == 1 and the token is not an option and required_params.len() == 1, parse that token with parse_cli_value using the corresponding param_types[idx] and insert it into the returned HashMap under param_names[idx]; keep the existing parsing path otherwise. Ensure callers (e.g., where parse_auto_cli_args is invoked in the snippet) are updated to pass param_has_default through.
🧹 Nitpick comments (1)
baml_language/crates/baml_lsp2_actions/src/completions_tests.rs (1)
513-525: ⚡ Quick winAssert completion kind alongside insert text for optional params.
Line 523 and Line 524 verify formatting, but this test should also assert the returned items are
CompletionKind::Parameterto lock the LSP contract (not just label text).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_lsp2_actions/src/completions_tests.rs` around lines 513 - 525, The test currently asserts insert_text for the found completions but doesn't lock the completion kind; update the test after finding max_results and filter (from completions_at) to assert their kind is CompletionKind::Parameter (e.g. assert_eq!(max_results.kind, Some(CompletionKind::Parameter)); and same for filter) so the LSP contract is enforced; reference the CompletionKind enum in scope or import it if necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/baml_cli/src/run_command.rs`:
- Around line 2482-2489: The negative assertion on the generated help text is
too weak (it checks for "--max_results 1" while example_value(&ty_int())
currently renders "42"), so tighten the regression check by asserting against
the actual example usage string instead of just "1"; update the assertion that
currently references help to check that the help does not contain "--max_results
42" (or better, the full example invocation line like "baml run --function
Search -- --query <string> --max_results 42") so the test will fail if the
optional arg is shown in examples; locate and change the assertion around the
help variable in the test block that contains assert!(help.contains("query:
string")) / assert!(help.contains("max_results: int [optional]")) /
assert!(help.contains("--max_results <int> [optional]")) to use the stronger
negative check.
---
Outside diff comments:
In `@baml_language/crates/baml_cli/src/run_command.rs`:
- Around line 871-897: parse_auto_cli_args currently decides the single-token
positional sugar only from param_names.len() == 1, so functions with one
required param and other defaulted params (tracked by build_args_from via
param_has_default) fail to accept a bare token; update parse_auto_cli_args
signature to accept param_has_default: &[bool] and change its logic to compute
required parameters (indexes where param_has_default[idx] is false), then if
tokens.len() == 1 and the token is not an option and required_params.len() == 1,
parse that token with parse_cli_value using the corresponding param_types[idx]
and insert it into the returned HashMap under param_names[idx]; keep the
existing parsing path otherwise. Ensure callers (e.g., where parse_auto_cli_args
is invoked in the snippet) are updated to pass param_has_default through.
---
Nitpick comments:
In `@baml_language/crates/baml_lsp2_actions/src/completions_tests.rs`:
- Around line 513-525: The test currently asserts insert_text for the found
completions but doesn't lock the completion kind; update the test after finding
max_results and filter (from completions_at) to assert their kind is
CompletionKind::Parameter (e.g. assert_eq!(max_results.kind,
Some(CompletionKind::Parameter)); and same for filter) so the LSP contract is
enforced; reference the CompletionKind enum in scope or import it if necessary.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bc03c841-fccd-4c47-9a8b-a653e5f1d63f
⛔ Files ignored due to path filters (3)
baml_language/crates/baml_tests/snapshots/diagnostic_errors/optional_parameter_defaults/baml_tests__diagnostic_errors__optional_parameter_defaults__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/optional_parameter_defaults/baml_tests__diagnostic_errors__optional_parameter_defaults__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase3a__optional_param_default_forward_reference_checks_lambda_bodies.snapis excluded by!**/*.snap
📒 Files selected for processing (35)
baml_language/crates/baml_cli/src/run_command.rsbaml_language/crates/baml_codegen_types/src/ty.rsbaml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_ast/src/lower_expr_body.rsbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/signature.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_ppir/src/lib.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/package_interface.rsbaml_language/crates/baml_compiler_parser/src/parser.rsbaml_language/crates/baml_fmt/src/ast/expressions.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_lsp2_actions/src/completions.rsbaml_language/crates/baml_lsp2_actions/src/completions_tests.rsbaml_language/crates/baml_lsp2_actions/src/describe.rsbaml_language/crates/baml_lsp2_actions/src/lib.rsbaml_language/crates/baml_lsp2_actions/src/type_info.rsbaml_language/crates/baml_lsp2_actions/src/type_info_tests.rsbaml_language/crates/baml_lsp2_actions/src/utils.rsbaml_language/crates/baml_project/src/client_codegen.rsbaml_language/crates/baml_tests/src/compiler2_hir.rsbaml_language/crates/baml_tests/src/compiler2_tir/inference.rsbaml_language/crates/baml_tests/src/compiler2_tir/mod.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase3a.rsbaml_language/crates/baml_tests/src/engine.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_project/src/bex_lsp/multi_project/request.rsbaml_language/crates/tools_onionskin/src/compiler.rsbaml_language/languages/python/rust/codegen_python/src/leaf.rsbaml_language/languages/python/rust/codegen_python/src/lib.rsbaml_language/languages/python/tests/test_sim_phase3.py
✅ Files skipped from review due to trivial changes (1)
- baml_language/crates/baml_lsp2_actions/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (25)
- baml_language/crates/baml_compiler2_ppir/src/lib.rs
- baml_language/crates/baml_codegen_types/src/ty.rs
- baml_language/crates/baml_lsp2_actions/src/type_info.rs
- baml_language/crates/baml_lsp2_actions/src/utils.rs
- baml_language/crates/baml_compiler2_emit/src/lib.rs
- baml_language/crates/baml_compiler2_hir/src/signature.rs
- baml_language/crates/baml_tests/src/compiler2_tir/inference.rs
- baml_language/crates/baml_tests/src/compiler2_tir/phase3a.rs
- baml_language/crates/baml_lsp2_actions/src/completions.rs
- baml_language/crates/baml_compiler2_hir/src/builder.rs
- baml_language/crates/baml_lsp2_actions/src/type_info_tests.rs
- baml_language/crates/baml_compiler2_tir/src/package_interface.rs
- baml_language/crates/baml_tests/src/compiler2_tir/mod.rs
- baml_language/crates/baml_lsp2_actions/src/check.rs
- baml_language/crates/baml_project/src/client_codegen.rs
- baml_language/crates/baml_compiler2_tir/src/inference.rs
- baml_language/crates/baml_tests/src/engine.rs
- baml_language/crates/baml_compiler2_ast/src/ast.rs
- baml_language/crates/baml_compiler_parser/src/parser.rs
- baml_language/crates/baml_fmt/src/ast/expressions.rs
- baml_language/crates/baml_compiler2_ast/src/lower_cst.rs
- baml_language/crates/baml_tests/src/compiler2_hir.rs
- baml_language/crates/baml_compiler2_ast/src/lower_expr_body.rs
- baml_language/crates/baml_compiler2_tir/src/builder.rs
- baml_language/crates/baml_compiler2_mir/src/lower.rs
rossirpaulo
force-pushed
the
paulo/optional-function-parameters
branch
from
May 11, 2026 22:06
a6f1d19 to
129bee2
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
baml_language/crates/baml_lsp2_actions/src/check.rs (1)
307-315: 💤 Low valueConsider destructuring
builder.finish()to avoid relying on brittle positional tuple indexing.The
finish()method returns a 14-tuple where index 5 holdsTypeCheckDiagnostics<'db>. Using.5.diagnosticsis fragile—if the tuple structure ever changes, this site will silently break or read the wrong field. Destructuring with explicit names (or a named struct) makes the intent clear and protects against future refactoring.Example refactor
- for tir_diag in builder.finish().5.diagnostics { + let (.., diagnostics, ..) = builder.finish(); + for tir_diag in diagnostics.diagnostics {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_lsp2_actions/src/check.rs` around lines 307 - 315, The code is indexing the 14-tuple returned by builder.finish() with `.5` which is brittle; instead destructure the result of builder.finish() into named locals and use the appropriate binding that holds the TypeCheckDiagnostics (the value currently accessed as `.5`) so you can iterate its .diagnostics; update the loop to iterate over that named binding (e.g., let (.., type_check_diagnostics, ..) = builder.finish(); for tir_diag in type_check_diagnostics.diagnostics { ... }), replacing references to builder.finish().5 and keeping uses of is_function_default_signature_diagnostic, tir_rendered_to_diagnostic, and file_id unchanged.baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rs (1)
344-358: ⚡ Quick winAdd a focused unit test for optional function-param lowering.
This mapping is now carrying semantic mode information, so a small unit test that lowers a function type with both required and optional params would lock down
p.optional -> FunctionParamModewithout relying only on higher-level fixtures.As per coding guidelines, "Prefer writing Rust unit tests over integration tests where possible".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rs` around lines 344 - 358, Add a focused Rust unit test that asserts optionality is preserved when lowering function parameters: construct a small TypeExpr function type with at least one required and one optional parameter, call lower_type_expr_in_ns (or the public wrapper that invokes it) with minimal fake/real db, package_items and ns_context, then assert the resulting FunctionType's params map to FunctionParamTy entries whose .mode equals FunctionParamMode::Required or FunctionParamMode::Optional according to the original p.optional flags; reference FunctionParamTy, FunctionParamMode and lower_type_expr_in_ns in the test to locate and validate the mapping without relying on higher-level fixtures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@baml_language/crates/baml_compiler2_tir/src/lower_type_expr.rs`:
- Around line 344-358: Add a focused Rust unit test that asserts optionality is
preserved when lowering function parameters: construct a small TypeExpr function
type with at least one required and one optional parameter, call
lower_type_expr_in_ns (or the public wrapper that invokes it) with minimal
fake/real db, package_items and ns_context, then assert the resulting
FunctionType's params map to FunctionParamTy entries whose .mode equals
FunctionParamMode::Required or FunctionParamMode::Optional according to the
original p.optional flags; reference FunctionParamTy, FunctionParamMode and
lower_type_expr_in_ns in the test to locate and validate the mapping without
relying on higher-level fixtures.
In `@baml_language/crates/baml_lsp2_actions/src/check.rs`:
- Around line 307-315: The code is indexing the 14-tuple returned by
builder.finish() with `.5` which is brittle; instead destructure the result of
builder.finish() into named locals and use the appropriate binding that holds
the TypeCheckDiagnostics (the value currently accessed as `.5`) so you can
iterate its .diagnostics; update the loop to iterate over that named binding
(e.g., let (.., type_check_diagnostics, ..) = builder.finish(); for tir_diag in
type_check_diagnostics.diagnostics { ... }), replacing references to
builder.finish().5 and keeping uses of is_function_default_signature_diagnostic,
tir_rendered_to_diagnostic, and file_id unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1af52f68-6e1b-4fcd-9f5c-edc4aa852800
⛔ Files ignored due to path filters (18)
baml_language/crates/baml_tests/snapshots/broken_syntax/mismatched_brackets/baml_tests__broken_syntax__mismatched_brackets__02_parser__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/optional_parameter_defaults/baml_tests__broken_syntax__optional_parameter_defaults__01_lexer__function_type_defaults.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/optional_parameter_defaults/baml_tests__broken_syntax__optional_parameter_defaults__02_parser__function_type_defaults.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/optional_parameter_defaults/baml_tests__broken_syntax__optional_parameter_defaults__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/broken_syntax/test_invalid_contexts/baml_tests__broken_syntax__test_invalid_contexts__02_parser__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiler2_emit/baml_tests__compiler2_emit__optional_defaults_emit_snapshot.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiler2_mir/baml_tests__compiler2_mir__optional_default_prologue_and_source_omission.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiler2_mir/baml_tests__compiler2_mir__optional_dropping_adapter.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiler2_mir/baml_tests__compiler2_mir__optional_named_gap_and_explicit_null.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiler2_mir/baml_tests__compiler2_mir__optional_named_reordered_args_evaluate_in_source_order.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/byte_string_literals/baml_tests__compiles__byte_string_literals__02_parser__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/catch_all_keyword/baml_tests__compiles__catch_all_keyword__02_parser__catch_all_keyword.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/catch_throw/baml_tests__compiles__catch_throw__02_parser__catch_throw.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/closure_loop_variable/baml_tests__compiles__closure_loop_variable__02_parser__demo.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/closures/baml_tests__compiles__closures__02_parser__closures.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/event_system/baml_tests__compiles__event_system__02_parser__main.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/function_call/baml_tests__compiles__function_call__02_parser__builtin_call.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/function_call/baml_tests__compiles__function_call__02_parser__function_call.snapis excluded by!**/*.snap
📒 Files selected for processing (56)
baml_language/crates/baml_cli/src/run_command.rsbaml_language/crates/baml_cli/src/test_command.rsbaml_language/crates/baml_codegen_types/src/symbols.rsbaml_language/crates/baml_codegen_types/src/ty.rsbaml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_ast/src/auto_derive_json.rsbaml_language/crates/baml_compiler2_ast/src/companions.rsbaml_language/crates/baml_compiler2_ast/src/lib.rsbaml_language/crates/baml_compiler2_ast/src/lower_config_item.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_ast/src/lower_expr_body.rsbaml_language/crates/baml_compiler2_ast/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_ast/src/lowering_diagnostic.rsbaml_language/crates/baml_compiler2_emit/src/emit.rsbaml_language/crates/baml_compiler2_emit/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/item_tree.rsbaml_language/crates/baml_compiler2_hir/src/signature.rsbaml_language/crates/baml_compiler2_mir/src/ir.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_mir/src/optimize.rsbaml_language/crates/baml_compiler2_mir/src/pretty.rsbaml_language/crates/baml_compiler2_ppir/src/lib.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/callable.rsbaml_language/crates/baml_compiler2_tir/src/exhaustiveness.rsbaml_language/crates/baml_compiler2_tir/src/generics.rsbaml_language/crates/baml_compiler2_tir/src/infer_context.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/lower_type_expr.rsbaml_language/crates/baml_compiler2_tir/src/normalize.rsbaml_language/crates/baml_compiler2_tir/src/package_interface.rsbaml_language/crates/baml_compiler2_tir/src/throws_analysis.rsbaml_language/crates/baml_compiler2_tir/src/ty.rsbaml_language/crates/baml_compiler2_visualization/src/control_flow/from_ast.rsbaml_language/crates/baml_compiler_parser/src/parser.rsbaml_language/crates/baml_compiler_syntax/src/ast.rsbaml_language/crates/baml_compiler_syntax/src/syntax_kind.rsbaml_language/crates/baml_fmt/src/ast/declarations.rsbaml_language/crates/baml_fmt/src/ast/expressions.rsbaml_language/crates/baml_fmt/src/ast/types.rsbaml_language/crates/baml_lsp2_actions/Cargo.tomlbaml_language/crates/baml_lsp2_actions/src/annotations.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_lsp2_actions/src/completions.rsbaml_language/crates/baml_lsp2_actions/src/completions_tests.rsbaml_language/crates/baml_lsp2_actions/src/describe.rsbaml_language/crates/baml_lsp2_actions/src/lib.rsbaml_language/crates/baml_lsp2_actions/src/type_info.rsbaml_language/crates/baml_lsp2_actions/src/type_info_tests.rsbaml_language/crates/baml_lsp2_actions/src/utils.rsbaml_language/crates/baml_project/src/client_codegen.rsbaml_language/crates/baml_tests/projects/broken_syntax/optional_parameter_defaults/function_type_defaults.bamlbaml_language/crates/baml_tests/projects/compiles/optional_function_parameters/main.bamlbaml_language/crates/baml_tests/projects/diagnostic_errors/optional_parameter_defaults/call_binding.bamlbaml_language/crates/baml_tests/projects/diagnostic_errors/optional_parameter_defaults/unsupported_contexts.baml
✅ Files skipped from review due to trivial changes (5)
- baml_language/crates/baml_lsp2_actions/src/lib.rs
- baml_language/crates/baml_lsp2_actions/Cargo.toml
- baml_language/crates/baml_lsp2_actions/src/type_info_tests.rs
- baml_language/crates/baml_tests/projects/broken_syntax/optional_parameter_defaults/function_type_defaults.baml
- baml_language/crates/baml_compiler2_mir/src/pretty.rs
🚧 Files skipped from review as they are similar to previous changes (45)
- baml_language/crates/baml_compiler2_ast/src/lowering_diagnostic.rs
- baml_language/crates/baml_compiler2_mir/src/ir.rs
- baml_language/crates/baml_lsp2_actions/src/annotations.rs
- baml_language/crates/baml_compiler2_emit/src/emit.rs
- baml_language/crates/baml_codegen_types/src/ty.rs
- baml_language/crates/baml_compiler2_tir/src/ty.rs
- baml_language/crates/baml_compiler2_ast/src/lower_type_expr.rs
- baml_language/crates/baml_compiler2_tir/src/throws_analysis.rs
- baml_language/crates/baml_compiler2_tir/src/callable.rs
- baml_language/crates/baml_lsp2_actions/src/completions_tests.rs
- baml_language/crates/baml_compiler2_hir/src/item_tree.rs
- baml_language/crates/baml_compiler2_visualization/src/control_flow/from_ast.rs
- baml_language/crates/baml_cli/src/test_command.rs
- baml_language/crates/baml_lsp2_actions/src/type_info.rs
- baml_language/crates/baml_compiler2_tir/src/package_interface.rs
- baml_language/crates/baml_lsp2_actions/src/describe.rs
- baml_language/crates/baml_tests/projects/diagnostic_errors/optional_parameter_defaults/call_binding.baml
- baml_language/crates/baml_tests/projects/diagnostic_errors/optional_parameter_defaults/unsupported_contexts.baml
- baml_language/crates/baml_compiler2_hir/src/builder.rs
- baml_language/crates/baml_codegen_types/src/symbols.rs
- baml_language/crates/baml_compiler_syntax/src/ast.rs
- baml_language/crates/baml_compiler_syntax/src/syntax_kind.rs
- baml_language/crates/baml_compiler2_ast/src/lower_config_item.rs
- baml_language/crates/baml_fmt/src/ast/declarations.rs
- baml_language/crates/baml_compiler2_tir/src/infer_context.rs
- baml_language/crates/baml_compiler2_ast/src/companions.rs
- baml_language/crates/baml_compiler2_emit/src/lib.rs
- baml_language/crates/baml_compiler2_tir/src/generics.rs
- baml_language/crates/baml_compiler2_mir/src/optimize.rs
- baml_language/crates/baml_compiler_parser/src/parser.rs
- baml_language/crates/baml_compiler2_ast/src/lib.rs
- baml_language/crates/baml_fmt/src/ast/types.rs
- baml_language/crates/baml_tests/projects/compiles/optional_function_parameters/main.baml
- baml_language/crates/baml_lsp2_actions/src/utils.rs
- baml_language/crates/baml_project/src/client_codegen.rs
- baml_language/crates/baml_compiler2_ppir/src/lib.rs
- baml_language/crates/baml_compiler2_ast/src/ast.rs
- baml_language/crates/baml_compiler2_hir/src/signature.rs
- baml_language/crates/baml_lsp2_actions/src/completions.rs
- baml_language/crates/baml_compiler2_ast/src/lower_expr_body.rs
- baml_language/crates/baml_compiler2_tir/src/inference.rs
- baml_language/crates/baml_compiler2_ast/src/lower_cst.rs
- baml_language/crates/baml_fmt/src/ast/expressions.rs
- baml_language/crates/baml_compiler2_mir/src/lower.rs
- baml_language/crates/baml_compiler2_tir/src/builder.rs
Add parser, AST, HIR, TIR, MIR, bytecode, runtime, CLI, LSP, formatter, and Python codegen support for optional function parameters and defaults. Parse declaration defaults and named or reordered call arguments. Validate default semantics, forward references, and required-after-default ordering. Bind omitted defaults through TIR/MIR call plans and runtime omitted-argument sentinels. Preserve optional/default metadata in codegen, Python SDK stubs, CLI invocation, and LSP completions. Include compiler, runtime, CLI, and snapshot coverage for optional parameter behavior.
rossirpaulo
force-pushed
the
paulo/optional-function-parameters
branch
from
May 12, 2026 01:20
129bee2 to
3d1e5b1
Compare
Drop the `[[test]] run_pytest` block from bridge_python's Cargo.toml — it
re-introduced a Rust test binary on a cdylib whose `[lib]` already pinned
`test = false` for exactly this reason (pyo3 + abi3-py310 can't link a
test binary without libpython). The block caused undefined-symbol failures
on rust-lld (Linux) and clang (macOS) for every `cargo test` run.
Add ns_defaults to sdk_test_basic_type_shapes: a six-function BAML fixture
+ nine pytest assertions covering the Python codegen surface for BEP-033
(required_positional_count emission, literal-vs-expression default rendering,
the _UNSET sentinel for the nullable+expression "trickiest case", and the
Python-side _build_kwargs argument-binding contract). The compiler-level
rules (subtyping, function-type syntax, ordering, forward-ref rejection,
destructuring rejection) remain covered by the existing fixtures under
crates/baml_tests/projects/{compiles,diagnostic_errors,broken_syntax}/
optional_parameter_defaults/.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rossirpaulo
enabled auto-merge
May 12, 2026 05:10
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.
Summary
Implements BEP-033 optional function parameters with default values, named source-call arguments, function-type optionality, runtime omission semantics, and Python generated-client omission plumbing.
This PR follows
TASK/TASK.mdand completes the phased implementation inTASK/PLAN.md, including the Phase 5 end-to-end compatibility sweep.What Changed
name?: Tnull._UNSET/sentinel path, without evaluating BAML defaults in Python.baml_testsnow enablessys_nativewithaws-cryptoso HTTP/rustls tests have a crypto provider.baml_compiler2_visualizationtest AST helpers now constructCallArgvalues instead of raw expression ids.Test Coverage Added
Phase 5 adds vertical coverage for:
null[]None, and passing_UNSETSnapshot Note
This PR includes broad parser snapshot updates beyond the new optional-parameter fixtures.
That churn is mechanical and expected: call arguments are now represented consistently with
CALL_ARGnodes so the CST can preserve both positional and named call forms:As a result, existing fixtures containing calls get parser snapshot diffs even when their source behavior is unrelated to optional parameters. The semantic changes are concentrated in the optional-parameter fixtures, runtime tests, Python sim tests, and the small workspace compile fixes.
Verification
Ran successfully:
Note: plain
cargo test --workspaceselected/usr/bin/python33.9.6 locally and failed before tests ran because PyO3 requires Python 3.10+ viaabi3-py310. PinningPYO3_PYTHONto Python 3.14 made the full workspace test pass.No pending
.snap.newor.pending-snapfiles remain.Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Tests