Commit ea7f7fb
authored
fix: MIR analysis soundness bugs + emit simplifications (#3126)
## Summary
Fixes three miscompilation bugs in the MIR analysis pass and simplifies
the emit infrastructure with several targeted refactors. All changes are
in `analysis.rs`, `emit.rs`, `builder.rs`, and VM regression tests.
### Bug fixes (3 correctness issues)
- **Cross-block Virtual inlining unsoundness** — `can_be_virtual()`
would re-evaluate rvalues at use sites without checking whether
dependencies (including transitive ones and parameters with implicit
entry defs) could be modified on intermediate paths. Three related
fixes:
- Multi-def check: refuse to virtualize if any dependency has multiple
definitions
- Parameter awareness: parameters have an implicit entry def not in
`all_defs`, so any explicit reassignment means multiple defs
- Transitive tracking: use `collect_transitive_reads` instead of direct
`walk_rvalue_locals` to catch `let t = p; let x = t; p = 2; x` chains
- **CopyOf propagation for mutable parameters** — `get_copy_source()`
assumed parameters are never reassigned, but BAML allows `x = 3` where
`x` is a param. Added check that source parameter has no entries in
`all_defs`.
- **MIR construction invariant assertions** — Added `debug_assert!`
guards at MIR builder boundaries enforcing that
Call/Await/DispatchFuture destinations are always `Place::Local`.
### Refactors
- **`StatementRef` enum** — Replaced `TERMINATOR_IDX = usize::MAX`
sentinel with `enum StatementRef { Statement(usize), Terminator }` for
type-safe, exhaustiveness-checked references. All comparison sites now
use pattern matching.
- **Unified MIR tree walkers** — Consolidated 6 duplicate
rvalue/operand/place walker functions into 3 generic
`walk_{rvalue,operand,place}_locals` functions + 3 thin wrappers. Single
source of truth for tree traversal.
- **Merged def-use collection** — Combined `collect_def_use()` and
`collect_all_definitions()` into one pass by adding `all_defs:
Vec<(BlockId, StatementRef)>` to `LocalDefUse`. Eliminated a redundant
full-MIR walk.
- **Deduplicated `patch_jumps`** — `patch_jumps()` now delegates to
`patch_jump_to()` instead of duplicating the match logic.
- **Dead code removal** — Removed always-true `is_inlinable_rvalue()`,
unused `LocalDefUse::local` field, and made `emit_store_place`
Field/Index arms `unreachable!()`.
- **Struct cleanup** — Removed `dominators`/`predecessors` from
`AnalysisResult` (computation unchanged, just not stored in return value
since callers never read them). Simplified `entry().or_insert_with()`
boilerplate to `get_mut().unwrap()`.
## Test plan
- [x] 5 regression tests added for the 3 soundness bugs:
- `virtual_cross_block_soundness` — cross-block variable modification
- `virtual_cross_block_param_mutation_soundness` — parameter mutation on
intermediate path
- `virtual_cross_block_transitive_param_mutation_soundness` — transitive
`t = p; x = t` chain
- `copy_of_mutable_param_soundness` — CopyOf with reassigned param
- `virtual_multiple_defs_preserve_side_effects` — multiple defs must not
skip side effects
- [x] All existing tests pass: 375 baml_tests, 146 emit tests, 270+ VM
tests
- [x] `cargo fmt` and `cargo clippy` clean
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Refactor**
* Improved internal compiler analysis and code generation structure for
enhanced maintainability and code organization.
* **Tests**
* Extended test coverage for variable assignment handling and virtual
inlining across different control flow scenarios.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->1 parent eea0740 commit ea7f7fb
File tree
4 files changed
+385
-387
lines changed- baml_language/crates
- baml_compiler_emit/src
- baml_compiler_mir/src
- bex_vm/tests
4 files changed
+385
-387
lines changed
0 commit comments