Treat underscore patterns as explicit discards#3402
Open
rossirpaulo wants to merge 1 commit intocanaryfrom
Open
Treat underscore patterns as explicit discards#3402rossirpaulo wants to merge 1 commit intocanaryfrom
rossirpaulo wants to merge 1 commit intocanaryfrom
Conversation
- Add discard and typed-discard AST patterns - Prevent `_` from creating bindings in lowering, HIR, TIR, and MIR - Add syntax and compiler tests for discard bindings
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe PR introduces discard pattern support ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![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
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)
2876-2903:⚠️ Potential issue | 🟡 MinorFix
_arm unreachable warnings inCatchAllPanicsclauses.
Pattern::Discard(_) returnsNonefor the panic subset, causinghas_panic_componentto be false. When aCatchAllPanicsclause has no remaining throws to match, the checkmatches.may_match.is_empty() && !has_panic_componentwill incorrectly warn that a_arm is unreachable, even though_inCatchAllPanicsshould always be reachable (panics occur at runtime regardless of declared throws). Special-case_patterns whenclause.kind == CatchAllPanicsto skip the unreachable warning. Add a regression test:catch all panics { _ => ... }with no declared throws should not emit UnreachableArm diagnostics.🤖 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 2876 - 2903, The unreachable-`_` warning arises because Pattern::Discard returns None for ty_panic_subset, making has_panic_component false; modify the reachability check used when validating clause arms to special-case the `Pattern::Discard` (the `_` arm) when `clause.kind == CatchAllPanics` so it is treated as having a panic component and thus never flagged unreachable. Concretely, in the arm-reachability logic where you compute `has_panic_component` from `ty_panic_subset` and check `matches.may_match.is_empty() && !has_panic_component`, change that condition to consider `(clause.kind == CatchAllPanics && pattern is Pattern::Discard)` as equivalent to `has_panic_component` (or bypass the unreachable diagnostic for that case). Add a regression test `catch all panics { _ => ... }` with no declared throws to assert no UnreachableArm is emitted.baml_language/crates/baml_compiler2_mir/src/lower.rs (1)
4977-4993:⚠️ Potential issue | 🟡 MinorSkip stack-trace local creation for discarded stack traces.
binding_name()returnsNonefor_, but thismapstill declares an anonymousstack_trace_localand records it in the catch region. That keeps_out of scope, but it still materializes a local for an explicit discard.Proposed fix
let stack_trace_local = clauses.first().and_then(|c| { - c.stack_trace_binding.map(|st_pat| { - let st_name = self.body.patterns[st_pat].binding_name().cloned(); + c.stack_trace_binding.and_then(|st_pat| { + let st_pat = &self.body.patterns[st_pat]; + if st_pat.is_discard() { + return None; + } + let st_name = st_pat.binding_name().cloned()?; let local = self.builder.declare_local( - st_name.clone(), + Some(st_name.clone()), Ty::BuiltinUnknown { attr: TyAttr::default(), }, None, false, ); - if let Some(name) = st_name { - self.locals.insert(name, local); - } - local + self.locals.insert(st_name, local); + Some(local) }) });🤖 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 4977 - 4993, The code currently creates a stack_trace_local even when the pattern's binding_name() is None (i.e., `_`), so change the closure used for c.stack_trace_binding handling to only declare and insert a local when binding_name() returns Some(name): fetch st_name = self.body.patterns[st_pat].binding_name(), if st_name.is_none() return None, otherwise call self.builder.declare_local(...) and then self.locals.insert(name, local) and return local; update the stack_trace_local computation (the map/and_then chain around clauses.first(), c.stack_trace_binding, declare_local, and self.locals.insert) accordingly so discarded `_` stack traces do not materialize a local.
🤖 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_ast/src/lower_expr_body.rs`:
- Around line 1048-1050: The code currently uses
self.patterns.alloc(Pattern::Discard) and self.exprs.alloc(Expr::Missing)
bypassing the arena helpers, which desynchronizes patterns/exprs from
source_map.pattern_spans; replace those direct alloc calls in the match-arm
construction with the proper helpers (call self.alloc_pattern(Pattern::Discard)
and self.alloc_expr(Expr::Missing) or the existing alloc_* methods used
elsewhere in lower_expr_body.rs) so recovered Pattern::Discard and Expr::Missing
are recorded in the source maps (keep references to patterns, exprs,
alloc_pattern, alloc_expr, Pattern::Discard, Expr::Missing and
source_map.pattern_spans).
---
Outside diff comments:
In `@baml_language/crates/baml_compiler2_mir/src/lower.rs`:
- Around line 4977-4993: The code currently creates a stack_trace_local even
when the pattern's binding_name() is None (i.e., `_`), so change the closure
used for c.stack_trace_binding handling to only declare and insert a local when
binding_name() returns Some(name): fetch st_name =
self.body.patterns[st_pat].binding_name(), if st_name.is_none() return None,
otherwise call self.builder.declare_local(...) and then self.locals.insert(name,
local) and return local; update the stack_trace_local computation (the
map/and_then chain around clauses.first(), c.stack_trace_binding, declare_local,
and self.locals.insert) accordingly so discarded `_` stack traces do not
materialize a local.
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 2876-2903: The unreachable-`_` warning arises because
Pattern::Discard returns None for ty_panic_subset, making has_panic_component
false; modify the reachability check used when validating clause arms to
special-case the `Pattern::Discard` (the `_` arm) when `clause.kind ==
CatchAllPanics` so it is treated as having a panic component and thus never
flagged unreachable. Concretely, in the arm-reachability logic where you compute
`has_panic_component` from `ty_panic_subset` and check
`matches.may_match.is_empty() && !has_panic_component`, change that condition to
consider `(clause.kind == CatchAllPanics && pattern is Pattern::Discard)` as
equivalent to `has_panic_component` (or bypass the unreachable diagnostic for
that case). Add a regression test `catch all panics { _ => ... }` with no
declared throws to assert no UnreachableArm is emitted.
🪄 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: 16f37261-acec-421f-95da-2e3b9a6b3f29
📒 Files selected for processing (11)
baml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_ast/src/disambiguate.rsbaml_language/crates/baml_compiler2_ast/src/lower_expr_body.rsbaml_language/crates/baml_compiler2_hir/src/builder.rsbaml_language/crates/baml_compiler2_mir/src/lower.rsbaml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/throw_inference.rsbaml_language/crates/baml_compiler2_visualization/src/control_flow/from_ast.rsbaml_language/crates/baml_lsp2_actions_tests/test_files/syntax/expr/discard_bindings.bamlbaml_language/crates/baml_tests/src/compiler2_hir.rsbaml_language/crates/baml_tests/src/compiler2_tir/mod.rs
Comment on lines
+1048
to
1050
| pattern: pattern.unwrap_or_else(|| self.patterns.alloc(Pattern::Discard)), | ||
| guard, | ||
| body: body.unwrap_or_else(|| self.exprs.alloc(Expr::Missing)), |
Contributor
There was a problem hiding this comment.
Keep recovered match-arm nodes in the source maps.
Line 1048 bypasses alloc_pattern, so a missing-pattern recovery arm desynchronizes patterns from source_map.pattern_spans; later pattern spans in the same body can fall back to defaults. The adjacent missing-body fallback has the same arena/source-map hazard.
Proposed fix
let arm = MatchArm {
- pattern: pattern.unwrap_or_else(|| self.patterns.alloc(Pattern::Discard)),
+ pattern: pattern.unwrap_or_else(|| self.alloc_pattern(Pattern::Discard, arm_span)),
guard,
- body: body.unwrap_or_else(|| self.exprs.alloc(Expr::Missing)),
+ body: body.unwrap_or_else(|| self.alloc_expr(Expr::Missing, arm_span)),
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pattern: pattern.unwrap_or_else(|| self.patterns.alloc(Pattern::Discard)), | |
| guard, | |
| body: body.unwrap_or_else(|| self.exprs.alloc(Expr::Missing)), | |
| pattern: pattern.unwrap_or_else(|| self.alloc_pattern(Pattern::Discard, arm_span)), | |
| guard, | |
| body: body.unwrap_or_else(|| self.alloc_expr(Expr::Missing, arm_span)), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@baml_language/crates/baml_compiler2_ast/src/lower_expr_body.rs` around lines
1048 - 1050, The code currently uses self.patterns.alloc(Pattern::Discard) and
self.exprs.alloc(Expr::Missing) bypassing the arena helpers, which
desynchronizes patterns/exprs from source_map.pattern_spans; replace those
direct alloc calls in the match-arm construction with the proper helpers (call
self.alloc_pattern(Pattern::Discard) and self.alloc_expr(Expr::Missing) or the
existing alloc_* methods used elsewhere in lower_expr_body.rs) so recovered
Pattern::Discard and Expr::Missing are recorded in the source maps (keep
references to patterns, exprs, alloc_pattern, alloc_expr, Pattern::Discard,
Expr::Missing and source_map.pattern_spans).
Binary size checks passed✅ 7 passed
Generated by |
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.
Summary
_and typed_, instead of modeling them as bindings named_._._references.Testing
discard_bindings.baml.Summary by CodeRabbit
New Features
Tests