Repository navigation
fix(compiler): allow reassigning narrowed inferred locals - #4212
antoniosarosi wants to merge 2 commits into
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):
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesInferred reassignment contracts
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Narrowing
participant LocalBinding
participant Reassignment
Narrowing->>LocalBinding: store narrowed current_ty
Reassignment->>LocalBinding: read inferred_declared_ty
Reassignment->>Reassignment: check assignment compatibility
Reassignment->>LocalBinding: replace current_ty with assigned type
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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_tir/src/builder.rs (1)
7622-7724: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftUse
current_tyor updateinferred_declared_tyfor evolved containers.For unannotated locals,
assign_local(...)only updatescurrent_ty, whileget_inferred_declared_type(...)returns the declaration-time type. Soa.push(1)makesa: EvolvingList(int), but a latera = ["hi"]still checks against the stillEvolvingList(Never)stable bound and is accepted. Make the mutable-update sites also updateinferred_declared_ty, or keep this branch checkingcurrent_tyfor this compatibility decision.🤖 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_tir/src/builder.rs` around lines 7622 - 7724, Update the unannotated-local assignment branch around inferred_declared_ty and reassignment_is_compatible so evolved container types from prior assign_local updates are used for compatibility checks. Either make mutable container updates refresh inferred_declared_ty or use the local’s current_ty for this decision, ensuring a.push(1) followed by a = ["hi"] is rejected while preserving existing narrowing behavior.
🤖 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_tir/src/builder.rs`:
- Around line 7622-7724: Update the unannotated-local assignment branch around
inferred_declared_ty and reassignment_is_compatible so evolved container types
from prior assign_local updates are used for compatibility checks. Either make
mutable container updates refresh inferred_declared_ty or use the local’s
current_ty for this decision, ensuring a.push(1) followed by a = ["hi"] is
rejected while preserving existing narrowing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 33ab7a44-5f87-4f71-b5e7-60671beee9b9
📒 Files selected for processing (3)
baml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/narrowing.rsbaml_language/crates/baml_tests/src/compiler2_tir/phase7.rs
Binary size checks passed✅ 7 passed
Generated by |
|
Closing — this was an orchestration experiment run; not landing. |
Summary
Root cause
Unannotated locals only retained their flow-sensitive current type. An is check therefore replaced the only type available to assignment checking, so the narrowed branch rejected values allowed by the original inferred declaration type.
Validation
Fixes B-618
Summary by CodeRabbit
Bug Fixes
type mismatchdiagnostics when reassigned values fall outside the inferred union type.Tests
isnarrowing.