Repository navigation
fix(compiler2): reject unsound local reassignment and validate index subscripts (B-236) - #3779
Merged
Merged
Conversation
…subscripts (B-236)
An empty `[]` in an if/else else-branch could crash the VM with `expected map,
got array`. Root cause: reassigning an *unannotated* local was never
type-checked and never updated the local's flow type, so a map-typed local could
hold an array at runtime; indexing it then aborted the VM. Reassignments are now
validated against the local's current type — an empty evolving container (`[]` /
`{}`) may still adopt a same-kind value, but cross-kind and unrelated-type
reassignments are rejected.
Two adjacent index-subscript soundness holes, surfaced while investigating, are
fixed in the same pass:
- The index key type was never validated, so `list["s"]` / `map[0]` aborted the
VM (`expected int, got string` / `expected string, got int`). A list or
byte-array now requires an `int` subscript and a map its declared key type.
- A null subscript aborted the VM with the confusing `got any`. Plain `a[i]` now
requires a non-null index; the null-safe `a?.[i]` short-circuits to null on a
null index too, symmetric with its existing base guard (only when the index
type is nullable).
Adds regression tests (TIR diagnostics, runtime null-safety) and regenerates the
LSP diagnostic fixtures that had encoded the missing diagnostics.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe PR adds compile-time enforcement of index subscript types in the TIR builder (rejecting non- ChangesIndex key type checking, reassignment compatibility, and null-safe optional index lowering
Sequence Diagram(s)sequenceDiagram
participant TIR as TIR Builder
participant KeyCheck as check_index_key_type
participant MIR as MIR Lowerer
participant BB as Basic Blocks
TIR->>KeyCheck: infer_index_expr(index_ty, allow_null=false)
KeyCheck-->>TIR: TypeMismatch if wrong/nullable key
TIR->>KeyCheck: infer_optional_index_expr(index_ty, allow_null=true)
KeyCheck-->>TIR: TypeMismatch if wrong key (nullable allowed)
TIR->>MIR: lower_optional_index(base, index)
MIR->>MIR: lower_optional_index_access(base, index, bb_null)
alt index_ty is nullable
MIR->>BB: branch to bb_null on null index
BB-->>MIR: result = null
else index is non-null
MIR->>BB: emit_index_access(base, index)
BB-->>MIR: result = element
end
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 |
⏭️ 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):
|
|
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: 1
🧹 Nitpick comments (1)
baml_language/crates/baml_tests/src/compiler2_tir/inference.rs (1)
694-701: ⚡ Quick winMake mismatch assertions specific to each scenario.
has_type_mismatch()currently passes on any diagnostic containing"type mismatch", so tests can succeed due to an unrelated mismatch in the snippet. Consider parameterizing the helper with an expected fragment (or expected/got pair) per test to keep these regressions precise.Suggested tightening
-fn has_type_mismatch(source: &str) -> bool { +fn has_type_mismatch(source: &str, expected_fragment: &str) -> bool { let mut db = make_db(); db.add_file("test.baml", source); baml_project::collect_compiler2_diagnostics(&db) .iter() - .any(|diag| diag.message.contains("type mismatch")) + .any(|diag| { + diag.message.contains("type mismatch") + && diag.message.contains(expected_fragment) + }) }🤖 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/inference.rs` around lines 694 - 701, The `has_type_mismatch` function currently matches any diagnostic containing "type mismatch" which is too broad and can cause tests to pass for unrelated type mismatches. Parameterize the function to accept an additional parameter containing an expected message fragment (such as a specific expected or got type string), and update the diagnostic check to verify that the message contains both "type mismatch" and this expected fragment. Then update all call sites of `has_type_mismatch` throughout the test file to pass the relevant expected fragment for each specific test scenario.
🤖 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_mir/src/lower.rs`:
- Around line 5811-5826: The index expression is being evaluated twice: once at
line 5811 when creating index_op for the null check, and again when lower_index
is called at line 5826. To fix this, refactor the code to reuse the
already-evaluated index_op operand instead of passing the original index
expression to lower_index again. You may need to either modify the lower_index
method signature to accept an operand instead of an expression, or restructure
the control flow to pass the pre-computed operand through to avoid re-evaluation
of potentially side-effectful index expressions.
---
Nitpick comments:
In `@baml_language/crates/baml_tests/src/compiler2_tir/inference.rs`:
- Around line 694-701: The `has_type_mismatch` function currently matches any
diagnostic containing "type mismatch" which is too broad and can cause tests to
pass for unrelated type mismatches. Parameterize the function to accept an
additional parameter containing an expected message fragment (such as a specific
expected or got type string), and update the diagnostic check to verify that the
message contains both "type mismatch" and this expected fragment. Then update
all call sites of `has_type_mismatch` throughout the test file to pass the
relevant expected fragment for each specific test scenario.
🪄 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: ff192b1c-9810-41ac-8926-f65d522b6ab6
📒 Files selected for processing (13)
baml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/invalid_assignment/arrays.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/invalid_assignment/class_arrays.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/invalid_assignment/classes.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/invalid_assignment/maps.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/invalid_assignment/nested_arrays.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/invalid_assignment/nominal_typing.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/invalid_assignment/primitives.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/loops/for.bamlbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/maps/key_and_value_typecheck.bamlbaml_language/crates/baml_tests/src/compiler2_tir/inference.rsbaml_language/crates/baml_tests/tests/type_error_repro.rs
Addresses review feedback on the `?.[]` null-safe index guard: the index expression was lowered once for the null check and again in the access, so a side-effectful / non-deterministic subscript would be evaluated twice. Extract a shared `emit_index_access` that takes the already-lowered operand, so the index is evaluated once. No behavior change for pure indices.
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_mir/src/lower.rs (2)
5748-5767:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid evaluating the optional-index base twice.
Line 5748 lowers
basefor the null check, but Lines 5802-5803 lower the samebaseagain before the actual access. Fornext()?.[i], this can callnext()twice and index a different value than the one that was null-checked.Proposed fix
- fn lower_optional_index(&mut self, base: AstExprId, index: AstExprId, dest: Place) { + fn lower_optional_index(&mut self, base: AstExprId, index: AstExprId, dest: Place) { + let base_ty = self.expr_ty(base); let base_op = self.lower_to_operand(base); let is_null = Rvalue::BinaryOp { op: BinOp::Eq, - left: base_op, + left: base_op.clone(), right: Operand::Constant(Constant::Null), }; @@ self.builder.set_current_block(bb_access); - self.lower_optional_index_access(base, index, dest, bb_null); + self.lower_optional_index_access(base_op, &base_ty, index, dest, bb_null); } else { @@ self.builder.set_current_block(bb_access); - self.lower_optional_index_access(base, index, dest.clone(), bb_null); + self.lower_optional_index_access(base_op, &base_ty, index, dest.clone(), bb_null); @@ fn lower_optional_index_access( &mut self, - base: AstExprId, + base_op: Operand, + base_ty: &RuntimeTy, index: AstExprId, dest: Place, bb_null: BlockId, ) { - let base_ty = self.expr_ty(base); - let base_op = self.lower_to_operand(base); let index_ty = self.expr_ty(index); @@ - self.emit_index_access(base_op, &base_ty, index_op, index_ty, dest); + self.emit_index_access(base_op, base_ty, index_op, index_ty, dest); }Also applies to: 5802-5803
🤖 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 5748 - 5767, The code lowers the base expression twice: once at line 5748 to create the null check (stored in base_op), and again around lines 5802-5803 during the actual access. This causes side effects in the base expression to execute twice, potentially null-checking one value but accessing a different one. Store the lowered base operand from line 5748 in base_op and pass this pre-computed operand to the lower_optional_index_access method instead of passing the raw base expression again, ensuring the same value is used for both the null check and the subsequent access.
11537-11573:⚠️ Potential issue | 🟠 MajorAdd index null-guard to optional-index lvalue lowering.
The rvalue path (
lower_optional_index_access, lines 5791–5822) properly guards nullable index subscripts with a null-check that short-circuits tobb_null. However, the lvalue path (lines 11537–11587) only guards the base and directly uses the index operand without checking for null. While TIR'scheck_index_key_typewithallow_null=true(line 4488) strips null from the index type at compile time, this narrowing is a type-level assumption that doesn't guarantee runtime non-nullability when the index is a plain variable (e.g.,arr?.[i]wherei: int?). The index guard in the rvalue path mirrors the base guard as documented (lines 5791–5794); the lvalue path should do the same.🤖 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 11537 - 11573, The lvalue path for OptionalIndex is missing a null-guard for the index operand. While the base is properly checked for null and guarded with a branch to bb_null, the index operand is used directly without a null-check. The rvalue path (lower_optional_index_access) correctly guards both base and index with identical null-checks. Add an index null-guard after converting the base to a local: create a null-check using BinaryOp::Eq comparing the index operand against Constant::Null, assign the result to a temporary boolean local, branch to bb_null if the index is null, and only proceed to use the index operand within the bb_continue block. This mirrors the base guard pattern already implemented in the same lvalue block.
🤖 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 5748-5767: The code lowers the base expression twice: once at line
5748 to create the null check (stored in base_op), and again around lines
5802-5803 during the actual access. This causes side effects in the base
expression to execute twice, potentially null-checking one value but accessing a
different one. Store the lowered base operand from line 5748 in base_op and pass
this pre-computed operand to the lower_optional_index_access method instead of
passing the raw base expression again, ensuring the same value is used for both
the null check and the subsequent access.
- Around line 11537-11573: The lvalue path for OptionalIndex is missing a
null-guard for the index operand. While the base is properly checked for null
and guarded with a branch to bb_null, the index operand is used directly without
a null-check. The rvalue path (lower_optional_index_access) correctly guards
both base and index with identical null-checks. Add an index null-guard after
converting the base to a local: create a null-check using BinaryOp::Eq comparing
the index operand against Constant::Null, assign the result to a temporary
boolean local, branch to bb_null if the index is null, and only proceed to use
the index operand within the bb_continue block. This mirrors the base guard
pattern already implemented in the same lvalue block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3b1b712f-161d-400a-8dc3-0a3c54757795
📒 Files selected for processing (1)
baml_language/crates/baml_compiler2_mir/src/lower.rs
This branch was previously deployed
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
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
Fixes B-236: an empty
[]in an if/else else-branch crashed the VM withexpected map, got array. Investigating it surfaced a small family of related index/assignment soundness holes — all programs that compiled but aborted the VM at runtime. All are now caught at compile time (or made null-safe).Root causes & fixes
1. Unannotated-local reassignment was unchecked (the B-236 crash).
let x = {}givesxa (evolving) map type; reassigning[](directly or viaif/else) was silently accepted, soxstayed map-typed while holding an array at runtime →as_mapabort. Reassignments to an unannotated local are now validated against the local's current flow type. An empty evolving container ([]/{}, elementnever) may still adopt a value of the same kind (let a = []; a = [1,2,3]keeps working), but cross-kind and unrelated-type reassignments are rejected. Generalizes to scalars (int ← string), classes, and nominal typing.2. Index key type was never validated.
list["s"]andmap[0]aborted the VM (expected int, got string/expected string, got int). A list / byte-array now requires anintsubscript and a map its declared key type.3. Null subscript aborted the VM with a confusing
got any.Plain
a[i]with a nullableiis now a compile error (must narrow). The null-safea?.[i]only guarded the base; it now also short-circuits tonullon a null index (only when the index type is nullable), symmetric with the base guard. This keeps valid chains likenode?.children?.[node?.value]working and makesb?.items?.[b?.idx]returnnullinstead of crashing.Net rule:
[]requires a non-null, correctly-typed subscript;?.[]is fully null-safe.Tests
?.[]null-safety (returnsnull, not a crash).baml_srccorpus green (1919), fmt + clippy clean.Files
crates/baml_compiler2_tir/src/builder.rs— reassignment check + index-key validationcrates/baml_compiler2_mir/src/lower.rs— null-safe index guard for?.[]crates/baml_tests/...— regression testscrates/baml_lsp2_actions_tests/test_files/...— regenerated goldensSummary by CodeRabbit
Bug Fixes
nullwhen the index is null.Tests