Add try_new variants for panicking constructors - #246
Merged
Conversation
📝 WalkthroughWalkthroughAdds fallible validation wrappers for several algebra module constructors and validation helpers, while keeping the existing infallible APIs as ChangesFallible constructors for algebra modules
BinomialIterator lint allowance
Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
Add fallible variants for the remaining module constructors and FD builder methods that panic on bad preconditions, following the existing try_compute_homotopy_step precedent (the panicking function delegates via .unwrap()): - HomModule::try_new: Err when target is not bounded above - QuotientModule::try_new: Err on too-low truncation / i32 overflow - HomPullback::try_new: Err on inconsistent source/target/map wiring - FiniteDimensionalModule::try_extend_actions / try_check_validity: Err when output_deg <= input_deg instead of asserting Each returns anyhow::Result, matching the from_json/secondary-lift error style. Adds valid+error path tests for all five.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ext/crates/algebra/src/module/quotient_module.rs`:
- Around line 37-47: The span validation in QuotientModule::try_new only checks
truncation + 1, but it still allows overflow when computing the allocation span
against module.min_degree(). Replace the current arithmetic with checked
operations for the actual span used to size the backing storage (or delegate the
guard to BiVec::with_capacity), and keep the existing min_degree/truncation
validation aligned with the final capacity calculation.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 53573fdd-2e45-4075-b7cd-c323adbcab97
📒 Files selected for processing (5)
ext/crates/algebra/src/module/finite_dimensional_module.rsext/crates/algebra/src/module/hom_module.rsext/crates/algebra/src/module/homomorphism/hom_pullback.rsext/crates/algebra/src/module/quotient_module.rsext/crates/fp/src/prime/iter.rs
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)
ext/crates/algebra/src/module/quotient_module.rs (1)
250-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a dedicated test for the span-overflow (checked_sub) branch.
The two tests cover truncation-too-low and
capacityadd-overflow, but not the case wherecapacity.checked_sub(min_degree)itself overflows (e.g., a very negativemin_degreepaired withtruncationneari32::MAX). Not blocking, since the add-overflow test already exercises the overflow-handling error path.🤖 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 `@ext/crates/algebra/src/module/quotient_module.rs` around lines 250 - 266, Add a dedicated test in QuotientModule::try_new that forces the checked_sub span-overflow branch, not just the truncation-too-low and capacity add-overflow cases. Use a module from joker() but override the degree inputs so capacity.checked_sub(min_degree) overflows (for example a very negative min_degree with truncation near i32::MAX), then assert that QuotientModule::try_new returns an error containing the overflow path. Keep the new case alongside try_new_truncation_too_low_errors and try_new_truncation_overflow_errors so the error handling branches are covered separately.
🤖 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 `@ext/crates/algebra/src/module/quotient_module.rs`:
- Around line 250-266: Add a dedicated test in QuotientModule::try_new that
forces the checked_sub span-overflow branch, not just the truncation-too-low and
capacity add-overflow cases. Use a module from joker() but override the degree
inputs so capacity.checked_sub(min_degree) overflows (for example a very
negative min_degree with truncation near i32::MAX), then assert that
QuotientModule::try_new returns an error containing the overflow path. Keep the
new case alongside try_new_truncation_too_low_errors and
try_new_truncation_overflow_errors so the error handling branches are covered
separately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 290d4754-7a17-466f-8145-52f6a5018b03
📒 Files selected for processing (1)
ext/crates/algebra/src/module/quotient_module.rs
hoodmane
changed the title
github-actions Bot
added a commit
that referenced
this pull request
Jul 2, 2026
Add fallible variants for the remaining module constructors and FD builder methods that panic on bad preconditions, following the existing try_compute_homotopy_step precedent (the panicking function delegates via .unwrap()): - HomModule::try_new: Err when target is not bounded above - QuotientModule::try_new: Err on too-low truncation / i32 overflow - HomPullback::try_new: Err on inconsistent source/target/map wiring - FiniteDimensionalModule::try_extend_actions / try_check_validity: Err when output_deg <= input_deg instead of asserting Each returns anyhow::Result, matching the from_json/secondary-lift error style. Adds valid+error path tests for all five.
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
2 participants
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.
Add fallible variants for the remaining module constructors and FD builder methods that panic on bad preconditions.