baml-language: Fix exhaustiveness alias expansion with cycle guard#3143
baml-language: Fix exhaustiveness alias expansion with cycle guard#3143miguelcsx wants to merge 3 commits intoBoundaryML:canaryfrom
Conversation
|
@miguelcsx is attempting to deploy a commit to the Boundary Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds cycle-guarded, recursive type-alias expansion and centralizes value-set generation by making Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
c64df80 to
ecf3b56
Compare
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_compiler_tir/src/exhaustiveness.rs (1)
258-270: 🧹 Nitpick | 🔵 TrivialMisleading comment in
ValueSet::Emptybranch ofcheck().The comment on line 264 says "Guarded patterns don't contribute", but guarded patterns never reach this branch:
pattern_to_value_setalready returnsValueSet::Emptyfor guarded arms (via the early return at the top ofpattern_to_value_set), and the surroundingif !has_guardat line 258 is a second gate. TheEmptyhere originates from void/error-typed patterns.🧹 Suggested clarification
- ValueSet::Empty => { - // Guarded patterns don't contribute - } + ValueSet::Empty => { + // void/error-typed patterns produce no values and don't contribute + }
|
Thanks for the PR! were you testing out the compiler? or what prompted you to tackle this? |
Hi @aaronvg, I was trying out the new compiler and noticed the exhaustiveness checker was missing a cycle guard for recursive type aliases (there was a TODO about it). While addressing that, I discovered typed patterns weren’t expanding through aliases during coverage checks, which caused valid matches to be reported as non-exhaustive. |
yep, i just checked it out. i really appreciate your help with that todo! i'll do some final tests before merging. that said, i'm curious to learn what sparked your interest in trying out baml. have you joined our discord yet? i'm happy to get your thoughts about what we're building :) |
|
Hey @rossirpaulo, thanks. I got into BAML while experimenting in a small project and trying to understand how the compiler and runtime fit together. I’m planning to keep exploring other parts of the repo and sending small fixes when I run into issues. If there’s an area that would benefit most from extra eyes right now, I’d be happy to focus there. |
09ec039 to
3c3702f
Compare
- collapse ValueSet unions containing All to a single catch-all - keep Empty filtering semantics intact
Summary by CodeRabbit
Improvements
Tests