Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (35)
📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughAdds Comparable and Sortable interfaces and builtin Comparable impls; moves Array.sort() to a Sortable blanket impl for T[]; implements primitive fast-path and VM native Comparable-based stable sorter; updates compiler projection/inference and MIR lowering for interface dispatch; and adds extensive BAML and Rust tests. ChangesComparable/Sortable array sorting feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6a9291045
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| )); | ||
| } | ||
| }; | ||
| format!("{fqn}.baml.Comparable.compare") |
There was a problem hiding this comment.
When an element is a class instance, this hard-codes the in-body method name {Class}.baml.Comparable.compare. Top-level implementations like implements baml.Comparable for Score { ... } are valid and the compiler registers those methods through def_to_item_ref as an out-of-body $for$ method instead, so Score[].sort() type-checks but sort_comparable_native will fail at runtime with compare dispatch: function ... not found. Please resolve the actual out-of-body Comparable method (or use the same dispatch metadata as MIR) rather than assuming every class Comparable implementation is in-body.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Doesn't reproduce — out-of-body implements baml.Comparable for <class> sorts correctly end-to-end. Added two runtime regression tests pinning it: sort_user_class_with_out_of_body_comparable_impl (Rust harness, flat user package) and sort_user_class_out_of_body_impl (BAML corpus, namespaced ns_arrays), both green. Note the dispatch shim was also renamed compare_dynamic → _compare_shim in this round; if a packaging shape exists where the lookup still misses, the dispatch error names the function it looked for, and the new tests are the place to extend.
|
No description provided. |
![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: 2
🧹 Nitpick comments (1)
baml_language/crates/baml_tests/tests/comparable_sort.rs (1)
481-504: ⚡ Quick win
phase2_user_class_compare_with_explicit_binding_in_maincurrently duplicates the prior test body.This doesn’t exercise a distinct path despite the name; either remove it or change the snippet so it validates a different dispatch/binding shape.
🤖 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/tests/comparable_sort.rs` around lines 481 - 504, The test phase2_user_class_compare_with_explicit_binding_in_main duplicates the previous test body and doesn't exercise explicit interface binding; either remove this test or change its snippet so main performs the compare through an explicit baml.Comparable binding (e.g. declare variables in main with the type baml.Comparable or cast Score to baml.Comparable and call compare via that interface) so the dispatch path differs from the prior test; update the test function phase2_user_class_compare_with_explicit_binding_in_main accordingly.
🤖 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_tests/tests/comparable_sort.rs`:
- Around line 844-912: Each test currently asserts only that
collect_compile_errors returned something, which is too permissive; update each
test (phase5_union_int_float_sort_is_compile_error,
phase5_optional_element_sort_is_compile_error,
phase5_union_int_string_sort_is_compile_error,
phase5_class_without_comparable_sort_is_compile_error) to assert that the errors
contain the specific expected message about sort/Comparable and the offending
type rather than just !errors.is_empty(); locate the collect_compile_errors call
in each test and replace the broad assert with a targeted assertion that scans
errors for a substring (e.g., the offending union type like "(int | float)" or
"int?" or "(int | string)" or "Resume" together with mention of "sort" or
"Comparable") so the test fails only when the specific compile error is missing.
In `@baml_language/crates/bex_vm/src/package_baml/mod.rs`:
- Around line 292-301: Update the stale comment/docs that mention `sort2` so
they consistently refer to `baml.Sortable$for$T[].sort`: locate the special-case
wiring in `attach_builtins` (the branch checking `function.name.as_str() ==
"baml.Sortable$for$T[].sort"` that returns
`bex_vm_types::FunctionKind::Native((array::sort_comparable_native as
NativeFunction) as *const ())` in `mod.rs`) and the related comment in
`package_baml/array.rs`, then change any occurrences of `.sort2` in
comments/docs to `.sort` so they match the actual runtime wiring and test
snapshots.
---
Nitpick comments:
In `@baml_language/crates/baml_tests/tests/comparable_sort.rs`:
- Around line 481-504: The test
phase2_user_class_compare_with_explicit_binding_in_main duplicates the previous
test body and doesn't exercise explicit interface binding; either remove this
test or change its snippet so main performs the compare through an explicit
baml.Comparable binding (e.g. declare variables in main with the type
baml.Comparable or cast Score to baml.Comparable and call compare via that
interface) so the dispatch path differs from the prior test; update the test
function phase2_user_class_compare_with_explicit_binding_in_main accordingly.
🪄 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: 432c0d42-3d4a-48ab-9931-36c8a36261e0
⛔ Files ignored due to path filters (10)
baml_language/crates/baml_tests/snapshots/baml_src/_root.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/arrays.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/for_loops.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/iter.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/iter_impl_generics_only.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/lambdas.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/lexical_scoping.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/patterns_new_runtime.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/typed_inputs.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/watch.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
baml_language/crates/baml_builtins2/baml_std/baml/comparable.bamlbaml_language/crates/baml_builtins2/baml_std/baml/containers.bamlbaml_language/crates/baml_builtins2/src/lib.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/associated_projection.rsbaml_language/crates/baml_tests/baml_src/ns_arrays/arrays.bamlbaml_language/crates/baml_tests/baml_src/ns_arrays/sort_comparable.bamlbaml_language/crates/baml_tests/tests/comparable_sort.rsbaml_language/crates/bex_vm/src/package_baml/array.rsbaml_language/crates/bex_vm/src/package_baml/mod.rsbaml_language/crates/bex_vm/src/vm.rs
Contributor
There was a problem hiding this comment.
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_compiler2_mir/src/lower.rs (1)
6060-6092:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle captured lambda parameters in this fallback path.
lambda_param_tir_typesis only consulted inside theself.locals.get(&segments[0])branch. In a nested lambda, an outer lambda parameter is a capture rather than a local, soa.compare(b)still skips this recovery path and falls back to the old non-dispatched lowering. That leaves the “saved/restored across nested lambdas” case broken for captured receivers. Please move this lookup into the shared receiver-type resolver, or widen this branch to accept capture-rooted paths too.🤖 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_mir/src/lower.rs` around lines 6060 - 6092, The receiver-type fallback currently only checks self.locals.get(&segments[0]) before consulting path_segment_types and lambda_param_tir_types, so captured outer lambda parameters (not locals) never hit the lambda_param_tir_types recovery; update the resolver so the lambda_param_tir_types lookup is performed from the shared receiver-type resolution path (the same code that computes recv_tir_ty) rather than only inside the self.locals branch, or alternatively widen the initial conditional that sets recv_root_local to also accept capture-rooted paths and then consult self.lambda_param_tir_types there; touch the symbols segments, recv_root_local, recv_seg_idx, path_segment_types, lambda_param_tir_types, and the code that computes recv_tir_ty to ensure captured lambda parameters are recovered the same way as local receivers.
🤖 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.
Outside diff comments:
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 6060-6092: The receiver-type fallback currently only checks
self.locals.get(&segments[0]) before consulting path_segment_types and
lambda_param_tir_types, so captured outer lambda parameters (not locals) never
hit the lambda_param_tir_types recovery; update the resolver so the
lambda_param_tir_types lookup is performed from the shared receiver-type
resolution path (the same code that computes recv_tir_ty) rather than only
inside the self.locals branch, or alternatively widen the initial conditional
that sets recv_root_local to also accept capture-rooted paths and then consult
self.lambda_param_tir_types there; touch the symbols segments, recv_root_local,
recv_seg_idx, path_segment_types, lambda_param_tir_types, and the code that
computes recv_tir_ty to ensure captured lambda parameters are recovered the same
way as local receivers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 55bb9fb8-5712-461f-9773-79ac65ba36dc
⛔ Files ignored due to path filters (34)
baml_language/crates/baml_tests/snapshots/baml_src/_root.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/arrays.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/for_loops.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/iter.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/iter_impl_generics_only.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/lambdas.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/lexical_scoping.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/patterns_new_runtime.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/typed_inputs.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/watch.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__testing_std__/baml_tests__compiles____testing_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__testing_std__/baml_tests__compiles____testing_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/closure_loop_variable/baml_tests__compiles__closure_loop_variable__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/closure_loop_variable/baml_tests__compiles__closure_loop_variable__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_basic/baml_tests__compiles__lambda_basic__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_fat_arrow/baml_tests__compiles__lambda_fat_arrow__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lexical_scoping/baml_tests__compiles__lexical_scoping__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lexical_scoping/baml_tests__compiles__lexical_scoping__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/parser_statements/baml_tests__compiles__parser_statements__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/parser_statements/baml_tests__compiles__parser_statements__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/patterns_new/baml_tests__compiles__patterns_new__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/patterns_new/baml_tests__compiles__patterns_new__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_dynamic/baml_tests__compiles__testset_dynamic__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_vibes_nested/baml_tests__compiles__testset_vibes_nested__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase5__snapshot_baml_package_items.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_textual.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
baml_language/crates/baml_builtins2/baml_std/baml/comparable.bamlbaml_language/crates/baml_builtins2/baml_std/baml/containers.bamlbaml_language/crates/baml_builtins2/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/associated_projection.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_tests/baml_src/ns_arrays/arrays.bamlbaml_language/crates/baml_tests/baml_src/ns_arrays/sort_comparable.bamlbaml_language/crates/baml_tests/tests/comparable_sort.rsbaml_language/crates/bex_vm/src/package_baml/array.rsbaml_language/crates/bex_vm/src/package_baml/mod.rsbaml_language/crates/bex_vm/src/package_baml/root.rsbaml_language/crates/bex_vm/src/vm.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- baml_language/crates/baml_builtins2/src/lib.rs
- baml_language/crates/baml_compiler2_tir/src/associated_projection.rs
- baml_language/crates/baml_builtins2/baml_std/baml/comparable.baml
- baml_language/crates/baml_tests/baml_src/ns_arrays/sort_comparable.baml
Contributor
There was a problem hiding this comment.
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_compiler2_mir/src/lower.rs (1)
4236-4252:⚠️ Potential issue | 🟠 Major | ⚡ Quick winShadowed untyped lambda params can inherit stale outer receiver type metadata.
lambda_param_tir_typesis restored per lambda, but inner params without annotations never clear same-name outer entries. If an inner lambda has an untyped param that shadows an outer typed param, fallback dispatch (Line 6086) can resolve using the outer type.Suggested fix
- for (param_idx, param) in func_def.params.iter().enumerate() { + for (param_idx, param) in func_def.params.iter().enumerate() { + // Prevent stale outer-lambda entries when this lambda shadows a name. + self.lambda_param_tir_types.remove(¶m.name); let param_ty = match ¶m.type_expr { Some(spanned_te) => { let mut diags = Vec::new(); let tir_ty = lower_type_expr_in_ns( self.db, &spanned_te.expr, pkg_items, &pkg_info.namespace_path, &enclosing_generics, &mut diags, ); self.lambda_param_tir_types .insert(param.name.clone(), tir_ty.clone()); self.resolved_aliases.convert(&tir_ty) } None => baml_type::Ty::Null { attr: baml_type::TyAttr::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_compiler2_mir/src/lower.rs` around lines 4236 - 4252, The loop over func_def.params only inserts entries into self.lambda_param_tir_types when a param has a type, but does nothing when a param.type_expr is None, allowing an inner untyped param to inherit an outer mapping; modify the loop in lower.rs so that for each param when param.type_expr is None you explicitly remove any existing mapping for that param name (e.g., self.lambda_param_tir_types.remove(¶m.name)) or set a clear sentinel before proceeding, while keeping the existing behavior of inserting the resolved tir_ty in the Some branch (the code that calls lower_type_expr_in_ns, self.lambda_param_tir_types.insert(...), and self.resolved_aliases.convert(...)); this ensures untyped lambda parameters do not reuse outer receiver type metadata during later fallback dispatch.
🤖 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.
Outside diff comments:
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 4236-4252: The loop over func_def.params only inserts entries into
self.lambda_param_tir_types when a param has a type, but does nothing when a
param.type_expr is None, allowing an inner untyped param to inherit an outer
mapping; modify the loop in lower.rs so that for each param when param.type_expr
is None you explicitly remove any existing mapping for that param name (e.g.,
self.lambda_param_tir_types.remove(¶m.name)) or set a clear sentinel before
proceeding, while keeping the existing behavior of inserting the resolved tir_ty
in the Some branch (the code that calls lower_type_expr_in_ns,
self.lambda_param_tir_types.insert(...), and
self.resolved_aliases.convert(...)); this ensures untyped lambda parameters do
not reuse outer receiver type metadata during later fallback dispatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 773cbd29-aeab-4720-ae97-3e02184e076d
⛔ Files ignored due to path filters (34)
baml_language/crates/baml_tests/snapshots/baml_src/_root.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/arrays.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/for_loops.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/iter.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/iter_impl_generics_only.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/lambdas.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/lexical_scoping.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/patterns_new_runtime.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/typed_inputs.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/watch.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__testing_std__/baml_tests__compiles____testing_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__testing_std__/baml_tests__compiles____testing_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/closure_loop_variable/baml_tests__compiles__closure_loop_variable__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/closure_loop_variable/baml_tests__compiles__closure_loop_variable__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_basic/baml_tests__compiles__lambda_basic__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_fat_arrow/baml_tests__compiles__lambda_fat_arrow__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lexical_scoping/baml_tests__compiles__lexical_scoping__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lexical_scoping/baml_tests__compiles__lexical_scoping__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/parser_statements/baml_tests__compiles__parser_statements__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/parser_statements/baml_tests__compiles__parser_statements__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/patterns_new/baml_tests__compiles__patterns_new__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/patterns_new/baml_tests__compiles__patterns_new__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_dynamic/baml_tests__compiles__testset_dynamic__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_vibes_nested/baml_tests__compiles__testset_vibes_nested__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase5__snapshot_baml_package_items.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_textual.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
baml_language/crates/baml_builtins2/baml_std/baml/comparable.bamlbaml_language/crates/baml_builtins2/baml_std/baml/containers.bamlbaml_language/crates/baml_builtins2/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/associated_projection.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_tests/baml_src/ns_arrays/arrays.bamlbaml_language/crates/baml_tests/baml_src/ns_arrays/sort_comparable.bamlbaml_language/crates/baml_tests/tests/comparable_sort.rsbaml_language/crates/bex_vm/src/package_baml/array.rsbaml_language/crates/bex_vm/src/package_baml/mod.rsbaml_language/crates/bex_vm/src/package_baml/root.rsbaml_language/crates/bex_vm/src/vm.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- baml_language/crates/baml_compiler2_tir/src/inference.rs
- baml_language/crates/baml_builtins2/src/lib.rs
- baml_language/crates/baml_compiler2_tir/src/associated_projection.rs
- baml_language/crates/baml_compiler2_hir/src/builder.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_tests/baml_src/ns_arrays/sort_comparable.baml`:
- Around line 299-316: The test computes `caught` but never asserts it; update
the `test "sort_fallible_comparator_happy_path"` to assert the no-throw contract
by adding an assertion that `caught` is false (e.g., using
`assert.is_false(caught)` or equivalent) immediately after `caught` is computed
to ensure `sort_flaky(xs)` did not throw on the happy path; reference the
`caught` variable and the `sort_flaky` call inside that test when making the
change.
- Around line 326-330: The test "sort_generic_propagation" currently invokes the
instance method xs.sort() and therefore skips the runtime generic path; replace
that call with the generic helper invocation sort_generic(xs) (or
baml.sort_generic(xs) if the helper is namespaced) so the runtime instantiation
of sort_generic<U extends baml.Comparable> is exercised, leaving the rest of the
test (xs, expected, and assert) unchanged.
- Around line 4-8: Update the header comment that currently states float compare
preserves runtime `InvalidArgument` to match the test assertions: change it to
state that float comparisons use a total-order NaN behavior and `throws never`
(no runtime `InvalidArgument`), so the comment aligns with the file's
tests/expectations about NaN ordering and error semantics.
🪄 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: 2289de44-8cd7-4ccd-af85-298e80468a7f
⛔ Files ignored due to path filters (35)
baml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_builtin_package_listing.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/_root.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/arrays.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/for_loops.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/iter.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/iter_impl_generics_only.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/lambdas.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/lexical_scoping.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/patterns_new_runtime.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/typed_inputs.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/watch.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__testing_std__/baml_tests__compiles____testing_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__testing_std__/baml_tests__compiles____testing_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/closure_loop_variable/baml_tests__compiles__closure_loop_variable__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/closure_loop_variable/baml_tests__compiles__closure_loop_variable__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_advanced/baml_tests__compiles__lambda_advanced__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_basic/baml_tests__compiles__lambda_basic__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lambda_fat_arrow/baml_tests__compiles__lambda_fat_arrow__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lexical_scoping/baml_tests__compiles__lexical_scoping__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/lexical_scoping/baml_tests__compiles__lexical_scoping__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/parser_statements/baml_tests__compiles__parser_statements__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/parser_statements/baml_tests__compiles__parser_statements__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/patterns_new/baml_tests__compiles__patterns_new__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/patterns_new/baml_tests__compiles__patterns_new__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_dynamic/baml_tests__compiles__testset_dynamic__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/testset_vibes_nested/baml_tests__compiles__testset_vibes_nested__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase5__snapshot_baml_package_items.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_textual.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
baml_language/crates/baml_builtins2/baml_std/baml/comparable.bamlbaml_language/crates/baml_builtins2/baml_std/baml/containers.bamlbaml_language/crates/baml_builtins2/src/lib.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/associated_projection.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_tests/baml_src/ns_arrays/arrays.bamlbaml_language/crates/baml_tests/baml_src/ns_arrays/sort_comparable.bamlbaml_language/crates/baml_tests/tests/comparable_sort.rsbaml_language/crates/bex_vm/src/package_baml/array.rsbaml_language/crates/bex_vm/src/package_baml/mod.rsbaml_language/crates/bex_vm/src/package_baml/root.rsbaml_language/crates/bex_vm/src/vm.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- baml_language/crates/baml_builtins2/src/lib.rs
- baml_language/crates/baml_tests/baml_src/ns_arrays/arrays.baml
- baml_language/crates/baml_compiler2_tir/src/inference.rs
- baml_language/crates/baml_compiler2_hir/src/builder.rs
- baml_language/crates/baml_compiler2_mir/src/lower.rs
- baml_language/crates/baml_builtins2/baml_std/baml/comparable.baml
- baml_language/crates/baml_compiler2_tir/src/associated_projection.rs
- baml_language/crates/baml_builtins2/baml_std/baml/containers.baml
…e primitive fast path Replace the builtin-only `Array.sort()` with a `Sortable` interface blanket-implemented for `T[]` where `T implements Comparable`. Adds a `Comparable` interface (`compare` + associated `CmpError`) with builtin impls for int/float/bigint/string. `Sortable.sort` dispatches on the element's runtime type via a native guard (`baml._is_primitive_array`, first element's type tag): - Primitive arrays (`int`/`bigint`/`string`/`float`) take `baml._rust_sort` — a native, stable, in-place natural sort with a pure-Rust comparator (no per-pair BAML round trips; the int domain sorts the backing Vec with zero scratch). - Every other `Comparable` element type falls back to native `Array.sort_by` (CPS insertion sort) with a `baml._compare_shim` comparator that resolves `compare` on the element's runtime class — `compare`'s two `Self` params are undispatchable through an interface-typed value, and the stdlib can't see downstream impls. The dispatch tests runtime values, not types: array type patterns can't discriminate element types at runtime (`IsType` only sees the LIST tag) and a BAML-level `is int` on a `T`-typed value constant-folds to false (`T ∩ int = Never`) — both pinned as tests in `comparable_sort.rs`. Error typing: - int[]/string[]/bigint[]/float[] sort is infallible (`throws never`). - float orders by IEEE 754 totalOrder (`f64::total_cmp`, via the shared native `baml._float_total_cmp`): NaN no longer throws — it sorts deterministically after +inf. Both sort paths share one comparator definition, so they can never disagree. - User classes sort by implementing `Comparable`; fallible comparators propagate their error and roll the array back on a mid-sort throw. Also: resolve associated-type projections off primitive base types (`int.CmpError`); single-candidate interface dispatch skips its (sometimes unrepresentable) IsType guard and a concrete container receiver resolves to its single definitive blanket-impl candidate (also simplifies array `.iter()`-style dispatch). BREAKING CHANGE: `sort()` now requires the element type to implement `Comparable`. `(int | float)[]`, `(float | bigint)[]`, `int?[]`, `(int | string)[]`, and classes without a `Comparable` impl — which previously sorted by domain coercion or threw `InvalidArgument` at runtime — are now compile errors; use `sort_by` with an explicit comparator. `float[].sort()` and `float.compare` no longer throw `InvalidArgument` on NaN: floats are totally ordered by `total_cmp` (`… < +inf < NaN`), and their `CmpError` is `never`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
10 tasks
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.
Replace the builtin-only
Array.sort()with aSortableinterface blanket-implemented forT[]whereT implements Comparable. Adds aComparableinterface (compare+ associatedCmpError) with builtin impls for int/float/bigint/string, and a native VM implementation (array::sort_comparable_native) that dispatchescompareby runtime type — a primitive natural-order fast path plus a stable per-paircomparecontinuation for userComparabletypes.throws never).CmpError = InvalidArgument).Comparable; fallible comparators propagate their error and roll the array back on a mid-sort throw.Also: resolve associated-type projections off primitive base types (
int.CmpError); single-candidate interface dispatch skips its (sometimes unrepresentable) IsType guard and a concrete container receiver resolves to its single definitive blanket-impl candidate (also simplifies array.iter()-style dispatch).BREAKING CHANGE:
sort()now requires the element type to implementComparable.(int | float)[],(float | bigint)[],int?[],(int | string)[], and classes without aComparableimpl — which previously sorted by domain coercion or threwInvalidArgumentat runtime — are now compile errors. Usesort_bywith an explicit comparator for those cases.Summary by CodeRabbit
New Features
Changes
Tests