Implement lexical scoping and shadowing for let bindings#3425
Merged
rossirpaulo merged 18 commits intocanaryfrom Apr 28, 2026
Merged
Implement lexical scoping and shadowing for let bindings#3425rossirpaulo merged 18 commits intocanaryfrom
rossirpaulo merged 18 commits intocanaryfrom
Conversation
Remove `_`-as-value test cases and regenerate snapshots. PR #3417 ("implement new patterns backend without parser support") canonicalizes `_` to a wildcard pattern at AST construction, so `let _ = expr; _` is no longer valid — `_` binds nothing and cannot be referenced. - Drop `repeated_underscore` and `for_underscore_binding` from the lexical_scoping project and the inlined regression strings. - Rename `declared_type_and_for_underscore_restore_outer_binding` to `declared_type_restored_across_scope` and keep only the half that exercises declared-type restoration across an inner scope (a real Phase 1 invariant); drop the underscore half. - Regenerate the 7 lexical_scoping snapshots; sizes shrink consistently with removing only the two functions. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughReplaces name-based local/capture tracking with semantic identity ( Changes
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 |
Binary size checks passed✅ 7 passed
Generated by |
Brings TIR/MIR scope handling in line with the HIR scope tree so
let-bindings and watch declarations stop leaking past their lexical
scope on every exit path (fallthrough, throw, break/continue/return,
match/catch arm fallthrough, while-body fallthrough).
- HIR: `Stmt::While` now pushes a Block scope around the body and the
C-style `after` step, mirroring `Stmt::For` so downstream consumers
(LSP find-refs, capture analysis, MIR `binding_locals`) anchor on
the while-statement boundary.
- TIR: snapshot/restore scoped locals around `Stmt::While`; route
match-arm and catch-arm bindings (including the catch-clause
stack-trace binding) through `declare_scoped_local` so their
PatId-keyed lifetime is tracked. Also type-check the C-style for
`after` step (previously skipped, surfaced in two LSP fixtures).
- MIR: consolidate every unwatch emission through
`emit_unwatch_to_depth` (renamed from `emit_unwatch_since`) and add
the call before `Stmt::Throw`, on match-arm and catch-arm body
fallthrough, so `watch let` declared inside an arm or before a throw
no longer leaks its watcher.
- Replace the name-keyed `scoped_local_assignments` tracker with
`Vec<ScopedAssignment { name, pattern: Option<PatId> }>` so
scope-restore can distinguish inner-shadow assignments (drop, rule
3) from outer-binding assignments (propagate, rule 2). Drops the
dead `preserve_assignments=false` branch that fell out as a result.
- Funnel every `self.locals.insert` through a named helper
(`add_local`, `narrow_local`, `seed_capture_unknown`) so the
locals-population paths are uniform; route lambda-param seeding
through `add_local`.
- Polish: DRY four inline `LocalBinding` pushes through
`register_local_pattern`; replace `WalkContext` enum with
`push_block_scope: bool`; replace inline lambda-scope linear search
with `index.lambda_scope_for(span)`; align
`type_info.rs::find_binding_ty_in_scopes` with the ancestor-scoped
pattern; collapse `local_binding_name` /
`match_or_catch_binding_name` into one helper; drop redundant Name
tiebreakers from capture resolution.
Tests: 6 new lexical-scoping runtime regressions (rule 1 / 2 / 3),
9 new watch-teardown tests across throw / arm fallthrough /
break / continue / return, 2 new HIR unit tests pinning the scope
tree shape and the while-body block scope. 2 LSP fixtures updated to
record now-correct unresolved-name diagnostics on the C-style for
`after` step.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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
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)
3715-3724:⚠️ Potential issue | 🟠 Major
whileafter-step bindings outlive the iteration.HIR/TIR now treat the C-style
afterstep as its own lexical block, but MIR lowers it directly inbb_afterwith no locals/watch snapshot. Anylet/watch letdeclared there remains inself.locals/watched_locals_stackfor the next iteration and after loop exit, which breaks the new shadowing rules and leaks watchers.Suggested fix
- if after.is_some() { - self.builder.set_current_block(bb_after); - } - if let Some(after_stmt) = after { - self.lower_stmt(after_stmt); - } + if let Some(after_stmt) = after { + self.builder.set_current_block(bb_after); + let saved_locals = self.locals.clone(); + let watched_depth = self.watched_locals_stack.len(); + self.lower_stmt(after_stmt); + if !self.builder.is_current_terminated() { + self.emit_unwatch_to_depth(watched_depth); + } + self.restore_locals_after_scope(saved_locals, watched_depth); + } if !self.builder.is_current_terminated() { self.builder.goto(bb_cond); }Please add a focused Rust unit regression that declares a shadowing
letorwatch letin theafterstep. As per coding guidelines,**/*.rs: Prefer writing Rust unit tests over integration tests where possible; Always runcargo test --libif you changed any Rust code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/baml_compiler2_mir/src/lower.rs` around lines 3715 - 3724, The after-step is lowered into bb_after without entering/exiting a fresh lexical scope, so locals and watched_locals_stack entries created by let/watch in the after-step persist across iterations; modify the lowering around bb_after to push a new locals scope and snapshot watched_locals_stack (e.g., save length or push marker), then call self.builder.set_current_block(bb_after); lower the after_stmt via self.lower_stmt(after_stmt); and finally restore/pop the locals and watched_locals_stack snapshot before jumping back to bb_cond. Also add a focused Rust unit regression (in the crate tests, not integration) that declares a shadowing let and a watch let inside the while after-step to ensure those bindings do not outlive the iteration and run cargo test --lib.
🧹 Nitpick comments (5)
baml_language/crates/baml_lsp2_actions/src/type_info_tests.rs (1)
83-83: Prefer semantic assertions over exact markdown equality here.This assertion is brittle to formatting-only hover changes. Asserting fenced code +
x: intcontent keeps intent while reducing churn.Suggested assertion update
- assert_eq!(markdown, "```baml\nx: int\n```"); + assert!( + markdown.contains("```baml"), + "expected fenced BAML hover markdown, got:\n{markdown}" + ); + assert!( + markdown.contains("x: int"), + "expected for-binding declaration hover type, got:\n{markdown}" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/baml_lsp2_actions/src/type_info_tests.rs` at line 83, The existing exact equality assert assert_eq!(markdown, "```baml\nx: int\n```") is brittle; replace it with semantic assertions that verify the hover contains a BAML fenced code block and the expected content. Locate the test assertion line (the assert_eq!(markdown, ...) in type_info_tests.rs), and change to two assertions using assert!(markdown.contains("```baml"), ...) and assert!(markdown.contains("x: int"), ...) with helpful failure messages to reduce churn from formatting-only changes.baml_language/crates/baml_lsp2_actions/src/completions_tests.rs (1)
470-496: Strengthen these shadowing assertions to avoid label-dedup false positives.Both tests currently pass if there is only one
"x"item, even when it resolves to the wrong binding. Consider asserting the selected item'sdetailas well, with distinct shadowed types.Suggested test hardening
fn test_value_completion_hides_shadowed_same_scope_local() { let test = CursorTest::new( r#" function Test() -> int { - let x = 1 + let x = "outer" let x = 2 x<[CURSOR] } "#, ); let completions = completions_at(&test.db, test.cursor.file, test.cursor.offset); - let x_count = completions.iter().filter(|c| c.label == "x").count(); + let x_items: Vec<_> = completions.iter().filter(|c| c.label == "x").collect(); - assert_eq!( - x_count, 1, + assert_eq!( + x_items.len(), 1, "Should only complete the innermost visible 'x', got: {completions:?}" ); + let detail = x_items[0].detail.as_deref().unwrap_or(""); + assert!( + detail.contains("int"), + "Expected innermost local `x: int`, got detail: {detail}" + ); } #[test] fn test_value_completion_hides_shadowed_parameter() { let test = CursorTest::new( r#" -function Test(x: int) -> int { +function Test(x: string) -> int { let x = 2 x<[CURSOR] } "#, ); let completions = completions_at(&test.db, test.cursor.file, test.cursor.offset); - let x_count = completions.iter().filter(|c| c.label == "x").count(); + let x_items: Vec<_> = completions.iter().filter(|c| c.label == "x").collect(); - assert_eq!( - x_count, 1, + assert_eq!( + x_items.len(), 1, "Should only complete the local that shadows parameter 'x', got: {completions:?}" ); + let detail = x_items[0].detail.as_deref().unwrap_or(""); + assert!( + detail.contains("int"), + "Expected shadowing local `x: int`, got detail: {detail}" + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/baml_lsp2_actions/src/completions_tests.rs` around lines 470 - 496, The current tests only assert there is one completion with label "x" which can hide incorrect binding matches; update the tests that call completions_at(...) and compute x_count to also locate the matching completion item (e.g., completions.iter().find(|c| c.label == "x")) and assert its detail (or another distinguishing field) equals the expected signature/type for the innermost local vs the parameter (for the test_value_completion_hides_shadowed_parameter and the adjacent shadowing test), so you verify both uniqueness and that the resolved binding is the correct one.baml_language/crates/baml_lsp2_actions/src/usages_at_tests.rs (1)
242-250: Strengthen this shadowing test with exact-location assertions.
assert_eq!(usages.len(), 2)is a good guard, but it can still pass with the wrong two references. Consider asserting the exact formatted locations (or names + spans) to lock in “outerxonly” behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/baml_lsp2_actions/src/usages_at_tests.rs` around lines 242 - 250, The test currently only checks usages.len() == 2 which can pass even if the wrong locations are returned; update the assertion to compare the exact expected formatted locations (or names and spans) using test.format_location_with_name(usages[i]) to lock the behavior to the outer `x` only. Replace or add after the length check an equality assertion like assert_eq!(usages.iter().map(|l| test.format_location_with_name(l)).collect::<Vec<_>>(), vec![expected_loc1, expected_loc2]) (or assert the pair of names+spans) using the same helpers so the test fails if any other references are returned. Ensure you reference the usages variable and test.format_location_with_name to generate the expected strings.baml_language/crates/baml_tests/tests/arrays.rs (1)
233-260: Strengthen this regression by shadowing with the same identifier.This currently uses
catch (e), so it doesn't directly guard same-name misresolution. Considercatch (x)to pin the exact shadowing case.♻️ Suggested tweak
- }) catch (e) { + }) catch (x) { _ => [0] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/baml_tests/tests/arrays.rs` around lines 233 - 260, Update the test array_map_callback_in_catch_base_keeps_parameter_scope to use a catch clause that shadows the map callback parameter by changing the catch pattern from (e) to (x) so the test exercises same-name shadowing of the callback parameter; keep the rest of the test (function main, items.map callback, and the assert on output.result expecting [1,12,3]) unchanged so the regression still verifies correct resolution when a later catch introduces an identically named binding.baml_language/crates/baml_lsp2_actions/src/describe.rs (1)
450-469: Code duplication withinference_owner_scopeininference.rs.This
body_owner_scopehelper is nearly identical toinference_owner_scopeinbaml_compiler2_tir/src/inference.rs(lines 37-54). Consider extracting this logic to a shared location (e.g., a method onFileSemanticIndexor a utility insemantic_index.rs) to avoid divergence during future maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/baml_lsp2_actions/src/describe.rs` around lines 450 - 469, Duplicate scope-walking logic exists in body_owner_scope and inference_owner_scope; extract into a single shared helper on FileSemanticIndex (or a new utility in semantic_index.rs). Create a method like FileSemanticIndex::nearest_body_owner(&self, start: FileScopeId) -> FileScopeId implementing the loop that checks scope.kind for ScopeKind::Function | ScopeKind::Let | ScopeKind::Lambda and walks parent links, then replace body_owner_scope and inference_owner_scope calls with that new method; ensure signature uses FileScopeId and references the same ScopeKind variants so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 4629-4637: The switch-lowering arm path currently only
snapshots/restores self.locals and therefore fails to unwind "watch let"
watchers when an arm falls through to the join; update the arm-lowering in
try_lower_as_switch (the block that does self.bind_pattern(...) /
self.lower_expr(...) / self.restore_active_locals(...)) to also snapshot the
active watcher depth/state before binding the pattern and after lowering the
body either restore that watcher state by calling
emit_unwatch_to_depth(saved_depth) (or equivalent) when the arm falls through
(i.e., when !self.builder.is_current_terminated()), and ensure the full restore
happens alongside restore_active_locals(saved_locals); also add a focused Rust
unit test that creates a match/catch arm containing a watch let and asserts the
watcher is unwound when the arm is switch-lowered (run cargo test --lib).
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 6089-6100: The current seeding in infer_lambda_body uses
self.add_local(name.clone(), ty.clone()) which preserves an outer declared_types
entry via entry().or_insert() and therefore fails when a lambda param shadows an
annotated outer local; change the seeding to directly overwrite the relevant
maps instead of calling add_local: for each (name, ty) in param_tys when
Some(name) assign/insert into self.declared_types and self.locals (replacing any
existing entry) and remove from self.let_binding_patterns, so the lambda
parameter's declared type and local binding replace any outer declaration and
shadowing works correctly.
---
Outside diff comments:
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 3715-3724: The after-step is lowered into bb_after without
entering/exiting a fresh lexical scope, so locals and watched_locals_stack
entries created by let/watch in the after-step persist across iterations; modify
the lowering around bb_after to push a new locals scope and snapshot
watched_locals_stack (e.g., save length or push marker), then call
self.builder.set_current_block(bb_after); lower the after_stmt via
self.lower_stmt(after_stmt); and finally restore/pop the locals and
watched_locals_stack snapshot before jumping back to bb_cond. Also add a focused
Rust unit regression (in the crate tests, not integration) that declares a
shadowing let and a watch let inside the while after-step to ensure those
bindings do not outlive the iteration and run cargo test --lib.
---
Nitpick comments:
In `@baml_language/crates/baml_lsp2_actions/src/completions_tests.rs`:
- Around line 470-496: The current tests only assert there is one completion
with label "x" which can hide incorrect binding matches; update the tests that
call completions_at(...) and compute x_count to also locate the matching
completion item (e.g., completions.iter().find(|c| c.label == "x")) and assert
its detail (or another distinguishing field) equals the expected signature/type
for the innermost local vs the parameter (for the
test_value_completion_hides_shadowed_parameter and the adjacent shadowing test),
so you verify both uniqueness and that the resolved binding is the correct one.
In `@baml_language/crates/baml_lsp2_actions/src/describe.rs`:
- Around line 450-469: Duplicate scope-walking logic exists in body_owner_scope
and inference_owner_scope; extract into a single shared helper on
FileSemanticIndex (or a new utility in semantic_index.rs). Create a method like
FileSemanticIndex::nearest_body_owner(&self, start: FileScopeId) -> FileScopeId
implementing the loop that checks scope.kind for ScopeKind::Function |
ScopeKind::Let | ScopeKind::Lambda and walks parent links, then replace
body_owner_scope and inference_owner_scope calls with that new method; ensure
signature uses FileScopeId and references the same ScopeKind variants so
behavior remains identical.
In `@baml_language/crates/baml_lsp2_actions/src/type_info_tests.rs`:
- Line 83: The existing exact equality assert assert_eq!(markdown, "```baml\nx:
int\n```") is brittle; replace it with semantic assertions that verify the hover
contains a BAML fenced code block and the expected content. Locate the test
assertion line (the assert_eq!(markdown, ...) in type_info_tests.rs), and change
to two assertions using assert!(markdown.contains("```baml"), ...) and
assert!(markdown.contains("x: int"), ...) with helpful failure messages to
reduce churn from formatting-only changes.
In `@baml_language/crates/baml_lsp2_actions/src/usages_at_tests.rs`:
- Around line 242-250: The test currently only checks usages.len() == 2 which
can pass even if the wrong locations are returned; update the assertion to
compare the exact expected formatted locations (or names and spans) using
test.format_location_with_name(usages[i]) to lock the behavior to the outer `x`
only. Replace or add after the length check an equality assertion like
assert_eq!(usages.iter().map(|l|
test.format_location_with_name(l)).collect::<Vec<_>>(), vec![expected_loc1,
expected_loc2]) (or assert the pair of names+spans) using the same helpers so
the test fails if any other references are returned. Ensure you reference the
usages variable and test.format_location_with_name to generate the expected
strings.
In `@baml_language/crates/baml_tests/tests/arrays.rs`:
- Around line 233-260: Update the test
array_map_callback_in_catch_base_keeps_parameter_scope to use a catch clause
that shadows the map callback parameter by changing the catch pattern from (e)
to (x) so the test exercises same-name shadowing of the callback parameter; keep
the rest of the test (function main, items.map callback, and the assert on
output.result expecting [1,12,3]) unchanged so the regression still verifies
correct resolution when a later catch introduces an identically named binding.
🪄 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: f689c349-7b41-4c86-a77f-75140c928e25
⛔ Files ignored due to path filters (35)
baml_language/crates/baml_tests/snapshots/__baml_std__/baml_tests____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/__baml_std__/baml_tests____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/__testing_std__/baml_tests____testing_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/catch_all_keyword/baml_tests__catch_all_keyword__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/catch_all_keyword/baml_tests__catch_all_keyword__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/catch_throw/baml_tests__catch_throw__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/catch_throw/baml_tests__catch_throw__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/catch_throw_regressions/baml_tests__catch_throw_regressions__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/closure_loop_variable/baml_tests__closure_loop_variable__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/closure_loop_variable/baml_tests__closure_loop_variable__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/closure_loop_variable/baml_tests__closure_loop_variable__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/format_checks/baml_tests__format_checks__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/format_checks/baml_tests__format_checks__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/generic_field_chain/baml_tests__generic_field_chain__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/lambda_field_access/baml_tests__lambda_field_access__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/lexical_scoping/baml_tests__lexical_scoping__01_lexer__lexical_scoping.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/lexical_scoping/baml_tests__lexical_scoping__02_parser__lexical_scoping.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/lexical_scoping/baml_tests__lexical_scoping__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/lexical_scoping/baml_tests__lexical_scoping__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/lexical_scoping/baml_tests__lexical_scoping__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/lexical_scoping/baml_tests__lexical_scoping__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/lexical_scoping/baml_tests__lexical_scoping__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/lexical_scoping/baml_tests__lexical_scoping__10_formatter__lexical_scoping.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/lexical_scoping_errors/baml_tests__lexical_scoping_errors__01_lexer__lexical_scoping_errors.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/lexical_scoping_errors/baml_tests__lexical_scoping_errors__02_parser__lexical_scoping_errors.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/lexical_scoping_errors/baml_tests__lexical_scoping_errors__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/lexical_scoping_errors/baml_tests__lexical_scoping_errors__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/lexical_scoping_errors/baml_tests__lexical_scoping_errors__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/lexical_scoping_errors/baml_tests__lexical_scoping_errors__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/lexical_scoping_errors/baml_tests__lexical_scoping_errors__06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/lexical_scoping_errors/baml_tests__lexical_scoping_errors__10_formatter__lexical_scoping_errors.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/parser_statements/baml_tests__parser_statements__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/testset_dynamic/baml_tests__testset_dynamic__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/testset_vibes_nested/baml_tests__testset_vibes_nested__04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/testset_with_setup/baml_tests__testset_with_setup__04_5_mir.snapis excluded by!**/*.snap
📒 Files selected for processing (36)
baml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_hir/src/semantic_index.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/inference.rsbaml_language/crates/baml_compiler2_tir/src/resolve.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/describe_tests.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/usages.rsbaml_language/crates/baml_lsp2_actions/src/usages_at_tests.rsbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/expr/extra_dot.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/headers/complex_headers_test.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/headers/nested_if_statements.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/loops/c_for.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/loops/header_requires_let_negative.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/misc/return.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/parens.bamlbaml_language/crates/baml_tests/projects/closure_loop_variable/demo.bamlbaml_language/crates/baml_tests/projects/lexical_scoping/lexical_scoping.bamlbaml_language/crates/baml_tests/projects/lexical_scoping_errors/lexical_scoping_errors.bamlbaml_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/phase6.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase7.rsbaml_language/crates/baml_tests/tests/arrays.rsbaml_language/crates/baml_tests/tests/functions.rsbaml_language/crates/baml_tests/tests/if_else.rsbaml_language/crates/baml_tests/tests/lambdas.rsbaml_language/crates/baml_tests/tests/lexical_scoping.rsbaml_language/crates/baml_tests/tests/watch.rsbaml_language/crates/tools_onionskin/src/compiler.rs
💤 Files with no reviewable changes (4)
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/misc/return.baml
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/expr/extra_dot.baml
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/headers/nested_if_statements.baml
- baml_language/crates/baml_lsp2_actions_tests/test_files/syntax/headers/complex_headers_test.baml
These are per-run scratch files insta writes when an inline snapshot mismatches; they're meant to be resolved via `cargo insta review` and deleted, not committed. The committed file had accumulated null bookkeeping records from every subsequent test run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 (2)
baml_language/crates/baml_compiler2_tir/src/builder.rs (1)
2529-2590:⚠️ Potential issue | 🟠 MajorAnalyze sibling match/catch arms from a shared pre-arm state.
restore_scoped_locals(snapshot)keeps outer-targeting assignments immediately, so the next arm/clause starts from the previous arm’s mutatedself.locals. That makes mutually exclusive alternatives order-dependent. For example, one arm establishingxsasint[]will cause a later sibling arm withxs.push("s")to be checked against the wrong baseline, andcatchalso loses the untouched no-throw path the same way. These outward mutations need to be joined after all alternatives, not fed arm-by-arm into subsequent siblings.Also applies to: 2647-2796
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/baml_compiler2_tir/src/builder.rs` around lines 2529 - 2590, The loop over arms leaks outward mutations because restore_scoped_locals(snapshot) is applied at the end of each arm, letting the next arm observe prior arm assignments; change the flow so each arm starts from the same pre-match snapshot: after taking snapshot = snapshot_scoped_locals() and before processing each arm body (i.e., after narrow_local/declare_scoped_local and after computing arm_ty), immediately restore_scoped_locals(snapshot) to revert any outer-targeting assignments so the next arm is unaffected, and instead accumulate per-arm mutations (a map of PatId/Name -> Ty) and, after the entire arms loop, perform a join/merge of those per-arm types into self.locals (implement a helper like join_scoped_locals or merge_scoped_local_changes that computes the least-upper-bound across all arms and applies only the common widened types). Ensure to update uses of declare_scoped_local, narrow_local, and restore_scoped_locals to work with this per-arm snapshot/merge approach.baml_language/crates/baml_compiler2_mir/src/lower.rs (1)
3690-3724:⚠️ Potential issue | 🟠 Major
whilestill leaksafter-scope shadows and watchers.The loop now tracks
watched_depthforbreak/continue, but the normalafterpath never restoresself.localsor unwinds watches before jumping back tobb_cond. Alet x = .../watch let x = ...inside the C-styleafterclause will stay visible on the next iteration and after loop exit, so laterxlookups can resolve to the stale inner local instead of the outer binding.♻️ Suggested fix
let prev_loop = self.loop_context.take(); + let saved_locals = self.locals.clone(); let watched_depth = self.watched_locals_stack.len(); self.loop_context = Some(LoopContext { break_target: bb_exit, continue_target: bb_after, watched_locals_depth: watched_depth, @@ if after.is_some() { self.builder.set_current_block(bb_after); } if let Some(after_stmt) = after { self.lower_stmt(after_stmt); } if !self.builder.is_current_terminated() { + self.emit_unwatch_to_depth(watched_depth); self.builder.goto(bb_cond); } + self.restore_locals_after_scope(saved_locals, watched_depth); self.loop_context = prev_loop; self.builder.set_current_block(bb_exit);Please add a focused regression for a
while (... ) after ...shadowing case (and ideallywatch letinafter) so this path stays covered. As per coding guidelines,**/*.rs: Prefer writing Rust unit tests over integration tests where possible; Always runcargo test --libif you changed any Rust code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/baml_compiler2_mir/src/lower.rs` around lines 3690 - 3724, The after-clause path in lower_while (where loop_context is set using LoopContext and watched_depth is captured) fails to restore self.locals and unwinds watched_locals_stack before jumping back to bb_cond, causing after-scope shadows and watches to leak; fix by saving prev_loop and prev_locals/watched_depth, and after lowering the after-stmt (before any goto bb_cond or on normal fallthrough) restore self.locals and pop/unwind watched_locals_stack to the saved depth and restore self.loop_context = prev_loop so inner let/watch bindings are removed; also add a focused unit test exercising while ... after ... shadowing (including watch let) to prevent regression.
🧹 Nitpick comments (1)
baml_language/crates/baml_tests/tests/watch.rs (1)
354-363: Consider adding one shadowing teardown regression (watch letwith same name).Given this PR’s lexical shadowing focus, a focused test with nested
watch let x(inner shadows outer) would help ensure inner teardown never detaches the outer watcher and validate BindingId-based behavior end-to-end.Suggested test shape
+#[tokio::test] +async fn watch_shadowed_binding_teardown_is_scope_correct() { + let output = baml_test!( + r#" + function main() -> int { + watch let x = 0; + { + watch let x = 1; + x = 2; + } + x = 3; + x + } + "# + ); + + insta::assert_snapshot!(output.bytecode, `@r`#" + // Expect inner `unwatch x` at block exit and outer `unwatch x` at fn exit. + "#); + assert_eq!(output.result, Ok(BexExternalValue::Int(3))); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@baml_language/crates/baml_tests/tests/watch.rs` around lines 354 - 363, Add a regression test in baml_tests/tests/watch.rs near the "Watch teardown across abnormal exits" group that exercises lexical shadowing: create nested watch bindings like `watch let x` where the inner `watch let x` shadows the outer, then trigger abnormal exits (break/continue/return) and assert the inner teardown does not detach the outer watcher; ensure the test uses the same binding name to validate BindingId-based behavior end-to-end and mirrors the existing tests' style/assertion helpers so it integrates with the current test harness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 3690-3724: The after-clause path in lower_while (where
loop_context is set using LoopContext and watched_depth is captured) fails to
restore self.locals and unwinds watched_locals_stack before jumping back to
bb_cond, causing after-scope shadows and watches to leak; fix by saving
prev_loop and prev_locals/watched_depth, and after lowering the after-stmt
(before any goto bb_cond or on normal fallthrough) restore self.locals and
pop/unwind watched_locals_stack to the saved depth and restore self.loop_context
= prev_loop so inner let/watch bindings are removed; also add a focused unit
test exercising while ... after ... shadowing (including watch let) to prevent
regression.
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 2529-2590: The loop over arms leaks outward mutations because
restore_scoped_locals(snapshot) is applied at the end of each arm, letting the
next arm observe prior arm assignments; change the flow so each arm starts from
the same pre-match snapshot: after taking snapshot = snapshot_scoped_locals()
and before processing each arm body (i.e., after
narrow_local/declare_scoped_local and after computing arm_ty), immediately
restore_scoped_locals(snapshot) to revert any outer-targeting assignments so the
next arm is unaffected, and instead accumulate per-arm mutations (a map of
PatId/Name -> Ty) and, after the entire arms loop, perform a join/merge of those
per-arm types into self.locals (implement a helper like join_scoped_locals or
merge_scoped_local_changes that computes the least-upper-bound across all arms
and applies only the common widened types). Ensure to update uses of
declare_scoped_local, narrow_local, and restore_scoped_locals to work with this
per-arm snapshot/merge approach.
---
Nitpick comments:
In `@baml_language/crates/baml_tests/tests/watch.rs`:
- Around line 354-363: Add a regression test in baml_tests/tests/watch.rs near
the "Watch teardown across abnormal exits" group that exercises lexical
shadowing: create nested watch bindings like `watch let x` where the inner
`watch let x` shadows the outer, then trigger abnormal exits
(break/continue/return) and assert the inner teardown does not detach the outer
watcher; ensure the test uses the same binding name to validate BindingId-based
behavior end-to-end and mirrors the existing tests' style/assertion helpers so
it integrates with the current test harness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3423d2b9-551f-45c3-8278-704c80eba26f
📒 Files selected for processing (7)
baml_language/.gitignorebaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_lsp2_actions/src/type_info.rsbaml_language/crates/baml_tests/tests/lambdas.rsbaml_language/crates/baml_tests/tests/lexical_scoping.rsbaml_language/crates/baml_tests/tests/watch.rs
✅ Files skipped from review due to trivial changes (1)
- baml_language/.gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- baml_language/crates/baml_tests/tests/lambdas.rs
- baml_language/crates/baml_lsp2_actions/src/type_info.rs
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.
Implements lexical block scoping and local shadowing for BAML
let,watch let, andforbindings across HIR, TIR, MIR, generated snapshots, and runtime regressions.Summary
for/whileloop bodies, lambdas, match arms, and catch arms own the right scopes.Stmt::Whilenow pushes a Block scope around the body and the C-styleafterstep, mirroringStmt::For._, and parameter shadowing while preserving top-level/member/parameter duplicate diagnostics.forloops,whileloops, match arms, and catch arms. PatId-keyed assignment tracking distinguishes inner-shadow assignments (drop on scope exit, rule 3) from outer-binding assignments (propagate, rule 2).declare_scoped_local, fixing a leak where the stack-trace binding was raw-inserted with no paired restore.emit_unwatch_to_depth): block fallthrough,for/whilebody fallthrough, match-arm and catch-arm body fallthrough,throw,break/continue/return.forafterstep (e.g.i += 1), which was previously skipped. This surfaces two correct missing-name diagnostics that update theloops_c_forandheader_requires_let_negativeLSP fixtures.Scoping and Evolving Container Rules
let,watch let, andfordeclarations are lexical: bindings introduced inside a block, loop body, lambda, match arm, or catch arm do not leak after that scope exits.push, index assignment, and similar mutations inside control-flow or loop bodies; the typechecker does not treat a loop body as non-establishing just because the loop might run zero times.let xs = []; for (let n in []) { xs.push("s") }; xs.push(1)establishesxsasstring[]at the loop-body push, so the later integer push is a type error.Validation
cargo test -p baml_lsp2_actions_tests --libcargo test -p baml_tests --libcargo test -p bex_engine --test concurrent test_closures_in_loop_varscargo test -p baml_tests --test lexical_scopingcargo test -p baml_tests --test watchmise run fmtmise run clippyprek run --all-filesCI status
1a0a1f8d5. Earlier failures inbaml_lsp2_actions_tests,baml_tests, andbex_engine --test concurrent(run https://github.com/BoundaryML/baml/actions/runs/25013954913) are resolved by this PR.Summary by CodeRabbit
Bug Fixes
New Features
Editor / LSP
Tests / Chores