Repository navigation
Restore explicit local ID capture plumbing - #4066
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughExplicit ChangesRuntime-id call side channel
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BAMLSource
participant TIRBuilder
participant CallPlan
participant MIRLowering
participant Bytecode
BAMLSource->>TIRBuilder: parse call with $id
TIRBuilder->>CallPlan: validate and store runtime_id
CallPlan->>MIRLowering: provide side-channel runtime_id
MIRLowering->>Bytecode: emit *WithRuntimeId instruction
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
⏭️ 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):
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
baml_language/crates/baml_lsp2_actions/src/check.rs (1)
1720-1862: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding
RuntimeIdArgumentTypeMismatchto file-aware rendering.
source_aware_tir_type_error_messagegives several Ty-embedding variants (TypeMismatch,NotCallable,OrderingRequiresCompare, …) file-aware type rendering viaty(), but the newRuntimeIdArgumentTypeMismatch { got }falls through to the genericDisplayimpl, losing per-file qualified-name shortening forgot.♻️ Proposed addition
TirTypeError::InvalidInterfaceUpcastTarget { target } => { format!("`.as<T>` requires an interface target, got {}", ty(target)) } + TirTypeError::RuntimeIdArgumentTypeMismatch { got } => format!( + "`$id` at a call site expects `boundary.LocalId`, got {}", + ty(got) + ), _ => error.to_string(),🤖 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 1720 - 1862, Update source_aware_tir_type_error_message to explicitly handle TirTypeError::RuntimeIdArgumentTypeMismatch, rendering its got type through the existing ty() closure and preserving the generic error wording otherwise.
🤖 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_tir/src/builder.rs`:
- Around line 2382-2397: Update has_named_args in the argument-processing logic
to exclude the special "$id" label, matching ordinary_arg_count’s filtering.
Ensure "$id" does not affect reported_positional_arity or missing-argument
diagnostic selection, while preserving normal named-argument detection for all
other labels.
---
Nitpick comments:
In `@baml_language/crates/baml_lsp2_actions/src/check.rs`:
- Around line 1720-1862: Update source_aware_tir_type_error_message to
explicitly handle TirTypeError::RuntimeIdArgumentTypeMismatch, rendering its got
type through the existing ty() closure and preserving the generic error wording
otherwise.
🪄 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: 876d1599-5f04-4832-ad49-f4d88d817c33
⛔ Files ignored due to path filters (6)
baml_language/crates/baml_tests/snapshots/diagnostic_errors/runtime_id_misuse/baml_tests__diagnostic_errors__runtime_id_misuse__01_lexer__runtime_id.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/runtime_id_misuse/baml_tests__diagnostic_errors__runtime_id_misuse__02_parser__runtime_id.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/runtime_id_misuse/baml_tests__diagnostic_errors__runtime_id_misuse__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/runtime_id_misuse/baml_tests__diagnostic_errors__runtime_id_misuse__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/runtime_id_misuse/baml_tests__diagnostic_errors__runtime_id_misuse__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/runtime_id_misuse/baml_tests__diagnostic_errors__runtime_id_misuse__10_formatter__runtime_id.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
baml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/infer_context.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_tests/projects/diagnostic_errors/runtime_id_misuse/runtime_id.bamlbaml_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/phase3a.rsbaml_language/crates/bex_engine/tests/host_value_callable.rsbaml_language/crates/bex_engine/tests/identity.rsbaml_language/crates/bex_engine/tests/prof_gate.rs
Binary size checks passed✅ 7 passed
Generated by |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughExplicit ChangesRuntime-id call side channel
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BAMLSource
participant TIRBuilder
participant CallPlan
participant MIRLowering
participant Bytecode
BAMLSource->>TIRBuilder: parse call with $id
TIRBuilder->>CallPlan: validate and store runtime_id
CallPlan->>MIRLowering: provide side-channel runtime_id
MIRLowering->>Bytecode: emit *WithRuntimeId instruction
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 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 |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
…cit-local-id-capture
Summary
Restores explicit call-site capture IDs using the trailing
$idargument:The compiler now treats
$idas structural call metadata owned by the TIR call plan instead of as a normal function parameter. MIR consumes that metadata directly and routes it through the existing direct, indirect, optional, virtual, union-dispatch, and sysop VM branches.Root cause
The TIR cleanup accidentally changed
$idinto a rejected unknown call-site argument while lower layers still retained most of the runtime-ID plumbing. LLM-oriented playground paths continued to appear functional through other capture mechanisms, but arbitrary function-to-function propagation could no longer be expressed through the language.Behavior
$idmust be the final call argument.boundary.LocalId.Tests
Added or restored coverage for:
Validation completed:
cargo check --workspace --all-targetscargo test -p baml_tests --lib— 1,832 passedcargo test -p bex_engine --test prof_gate— 32 passedcargo test -p bex_engine --test identity— 19 passedcargo test -p bex_engine --test host_value_callable— 18 passed-D warningsThe workspace-wide Clippy hook also reports an unrelated existing
duration_suboptimal_unitswarning inbex_cache; the modified crates pass focused Clippy.Summary by CodeRabbit
$id = ...across direct calls, indirect/optional calls, virtual dispatch, unions, and system-operation calls.$idvalidation for duplicates, required placement, and type mismatches, with clearer$id-specific diagnostics.$iddoes not affect ordinary argument counting or arity/type checking.