[codex] Report missing catch_all throw arms#3723
Open
aaronvg wants to merge 2 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds exhaustiveness checking for ChangesCatch-All Exhaustiveness Checking
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 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_compiler2_tir/src/builder.rs (1)
6580-6592: Testing guideline: baml_compiler2_tir unit coverage appears to be exercised via CI
The repo’s CI workflow.github/workflows/cargo-tests.reusable.yamlrunscargo nextest run --all-featuresfor thebaml_languageworkspace excluding onlybaml_testsandsdk_test_*, which should includebaml_compiler2_tirunit tests. There’s no explicitcargo test --lib -p baml_compiler2_tircommand in the workflows, so align the PR/test notes with what was actually run (or run that exact command locally per the guideline).🤖 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 6580 - 6592, Update the PR/test notes to accurately reflect how tests are executed in CI: state that the workflow cargo-tests.reusable.yaml runs `cargo nextest run --all-features` for the baml_language workspace (which includes the baml_compiler2_tir crate except for the excluded baml_tests and sdk_test_* crates), or alternatively run and report the exact local command `cargo test --lib -p baml_compiler2_tir` if you validated tests locally; ensure the PR description or testing section explicitly mentions which command was run and the observed results so readers know baml_compiler2_tir was covered by CI.Source: Coding guidelines
🤖 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_lsp2_actions/src/check.rs`:
- Around line 6240-6253: Run the library tests and attach their output to the
PR: from the repository root run `cargo test --lib` in the crate
baml_language/crates/baml_lsp2_actions (or `cd
baml_language/crates/baml_lsp2_actions && cargo test --lib`), capture the full
terminal output (stdout/stderr) showing all tests (116/116) passing, and add
that output to the PR either as a pasted comment or as an attached file (e.g.,
test-output.txt). Confirm the command ran successfully and include the exact
captured output in the PR so reviewers can verify the library tests passed.
---
Nitpick comments:
In `@baml_language/crates/baml_compiler2_tir/src/builder.rs`:
- Around line 6580-6592: Update the PR/test notes to accurately reflect how
tests are executed in CI: state that the workflow cargo-tests.reusable.yaml runs
`cargo nextest run --all-features` for the baml_language workspace (which
includes the baml_compiler2_tir crate except for the excluded baml_tests and
sdk_test_* crates), or alternatively run and report the exact local command
`cargo test --lib -p baml_compiler2_tir` if you validated tests locally; ensure
the PR description or testing section explicitly mentions which command was run
and the observed results so readers know baml_compiler2_tir was covered by CI.
🪄 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: 6135c818-9c6a-4f27-b5d5-a915e2afea09
⛔ Files ignored due to path filters (6)
baml_language/crates/baml_tests/snapshots/diagnostic_errors/catch_throw_regressions/baml_tests__diagnostic_errors__catch_throw_regressions__01_lexer__regressions.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/catch_throw_regressions/baml_tests__diagnostic_errors__catch_throw_regressions__02_parser__regressions.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/catch_throw_regressions/baml_tests__diagnostic_errors__catch_throw_regressions__03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/catch_throw_regressions/baml_tests__diagnostic_errors__catch_throw_regressions__04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/catch_throw_regressions/baml_tests__diagnostic_errors__catch_throw_regressions__05_diagnostics.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/diagnostic_errors/catch_throw_regressions/baml_tests__diagnostic_errors__catch_throw_regressions__10_formatter__regressions.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
baml_language/crates/baml_compiler2_tir/src/builder.rsbaml_language/crates/baml_compiler2_tir/src/infer_context.rsbaml_language/crates/baml_lsp2_actions/src/check.rsbaml_language/crates/baml_tests/projects/diagnostic_errors/catch_throw_regressions/regressions.baml
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
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
catch_alldiagnostic when typed catch arms leave inferred throw types unhandled.BuildErrorandstring, where onlyBuildErroris caught.E0062) while rendering catch-specific wording.Root Cause
catch_allcleared the residual throw set after checking its arms, so unhandled non-panic throw facts were treated as fully handled and no diagnostic escaped.Validation
cargo nextest run -p baml_tests diagnostic_errors::catch_throw_regressionsgit diff --checkNote
Low Risk
Type-checker-only change to catch exhaustiveness; no runtime or auth paths, with behavior covered by diagnostic tests.
Overview
catch_allhandlers that leave inferred throw types unhandled now emit a compile error instead of silently clearing the residual throw set.TIR catch inference reports
TirTypeError::NonExhaustiveCatchAllwhen acatch_all/catch_all_panicsclause still has unhandled types inresidualafter its arms run, listing the missing throw types against the clause binding type. LSP/check rendering uses catch-specific wording while reusingE0062(NonExhaustiveMatch).Tests now expect this diagnostic for partial
catch_all(e.g. onlystringwhenint | stringcan throw) and add a regression where a call chain throwsBuildError | stringbut onlyBuildErroris caught.Reviewed by Cursor Bugbot for commit f774e33. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Tests