M1: Eliminate .unwrap()/.expect() from src/chains/ (#317) by joaquinbejar · Pull Request #348 · joaquinbejar/OptionStratLib

joaquinbejar · 2026-04-16T20:45:53Z

Closes #317.

Summary

Remove every production .unwrap() / .expect() from src/chains/ (M1 — Panic-Free Core, 0.16.0). Total: ~28 real production sites across 6 files → 0.

Acceptance grep is clean:

for f in $(find src/chains -name '*.rs'); do
  awk '/#\[cfg\(test\)\]/{exit} {print}' "$f" | grep -nE '\.unwrap\(\)|\.expect\('
done
# (no output)

Approach

Added From<SimulationError> and From<VolatilityError> for ChainError (both → ChainError::DynError to keep the public enum stable; no new variants).
Tightened pub fn generator_optionchain(...) and pub fn generator_positive(...) in src/chains/generators.rs from -> Vec<Step<...>> to -> Result<Vec<Step<...>>, ChainError>. All in-tree callers (tests in src/strategies/{long,short}_{call,put}.rs, src/simulation/simulator.rs, src/pricing/monte_carlo.rs, plus examples/examples_simulation/ binaries) updated.
Inside Result-returning fns: ? propagation. Decimal::to_f64() on tiny constants (SKEW_SLOPE, SKEW_SMILE_CURVE) → unwrap_or_else with tracing::warn!. partial_cmp(...).unwrap() → unwrap_or(Ordering::Equal) with SAFETY comment.
optiondata.rs::apply_spread (returns ()): self.{call,put}_middle.unwrap() rewritten as if let Some(...) destructure. Behavioural change: previously panicked on None, now no-op + tracing::debug!.
chain.rs::strike_price_range_vec .expect("strike_prices is not empty") → .ok_or(ChainError::DynError { message: "strike_prices empty".into() })?.
rnd.rs::calculate_kurtosis variance_dec.sqrt().unwrap() → .ok_or(...)? (Decimal::sqrt returns Option).
mod.rs doc-comment examples wrapped in # fn run() -> Result<(), Box<dyn std::error::Error>> so ? compiles.

panic! / unreachable! / unimplemented! / assert! are intentionally left in place — they belong to issue #292.

Public API impact (intentional, M1)

pub fn generator_optionchain(walk_params: &WalkParams<Positive, OptionChain>) now returns Result<Vec<Step<Positive, OptionChain>>, ChainError>.
pub fn generator_positive(walk_params: &WalkParams<Positive, Positive>) now returns Result<Vec<Step<Positive, Positive>>, ChainError>.
From<SimulationError> for ChainError and From<VolatilityError> for ChainError newly available downstream.

No OptionChain::new / OptionData::new constructors changed. No existing variant of ChainError modified.

Verification

cargo build --all-targets --all-features — clean.
cargo clippy --all-targets --all-features --workspace -- -D warnings — clean.
cargo fmt --all --check — clean.
cargo test --lib chains:: — 521 passed; 4 failed (pre-existing on main): chains::chain::tests_basic_curves::test_curve_multiple_axes, tests_basic_curves::test_curve_price_short_put, tests_option_chain_surfaces::test_surface_different_greeks, tests_option_chain_surfaces::test_vanna_surface. Verified by checking out main and reproducing the same failures.
cargo test --doc — 203 passed; 0 failed.
cargo build --release — clean.

Commit map

feat(error): add From<SimulationError> and From<VolatilityError> for ChainError
refactor(chains): panic-free src/chains/* (35 sites + 2 sig changes)
docs: regenerate README to match src/lib.rs doctest update

Test plan

cargo build --all-targets --all-features clean
cargo clippy --all-targets --all-features --workspace -- -D warnings clean
cargo fmt --all --check clean
cargo test --lib chains:: (4 pre-existing failures unrelated to this PR)
cargo test --doc clean
cargo build --release clean
Reviewer confirms generator_optionchain/generator_positive sig change is acceptable
Reviewer confirms apply_spread None → no-op behavioural shift is the intended fix

…ChainError Wire the two missing conversions needed by the chains/ panic-free refactor (issue #317): - From<SimulationError>: random walk generators in src/chains/ generators.rs propagate failures from WalkTypeAble methods (brownian, geometric_brownian, jump_diffusion, ...). - From<VolatilityError>: src/chains/utils.rs adjust_volatility and related helpers can now ?-propagate VolatilityError directly. Both map to ChainError::DynError to keep the public enum stable; no new variants. Existing test surface unaffected.

Eliminate every production .unwrap() / .expect() under src/chains/. Closes the per-file refactor steps C2-C6 of issue #317 (M1 Panic-Free Core). Highlights: - generators.rs: replace 24 walker.<method>(walk_params).unwrap() with ?, propagating SimulationError via the new From<SimulationError> for ChainError impl. - optiondata.rs: calculate_prices uses ? throughout; apply_spread rewrites self.{call,put}_middle.unwrap() into if let Some destructure blocks (previously panicked on None, now no-op + tracing::debug!). - utils.rs: partial_cmp(...).unwrap() → unwrap_or(Ordering::Equal) with SAFETY comment; Decimal::to_f64() on tiny constants (SKEW_SLOPE, SKEW_SMILE_CURVE) → unwrap_or_else with tracing::warn!; base_vol unwrap after is_none() guard rewritten as let base_vol = (*base_vol)?. - chain.rs: .expect("strike_prices is not empty") → .ok_or(ChainError:: DynError { message: "strike_prices empty".into() })?. - rnd.rs: variance_dec.sqrt().unwrap() → .ok_or(...)? (Decimal::sqrt returns Option). - mod.rs: doc-comment examples wrapped in `# fn run() -> Result<(), Box<dyn std::error::Error>>` so ? compiles. API changes (intentional, M1 panic-free milestone): - pub fn generator_optionchain(...) -> Vec<Step<Positive, OptionChain>> → Result<Vec<Step<Positive, OptionChain>>, ChainError> - pub fn generator_positive(...) -> Vec<Step<Positive, Positive>> → Result<Vec<Step<Positive, Positive>>, ChainError> All in-tree callers (tests in src/strategies/{long,short}_{call,put}.rs, src/simulation/simulator.rs, src/pricing/monte_carlo.rs, plus the examples/examples_simulation/ binaries) updated to handle the new Result via .unwrap()/.expect() in test/example contexts (out of issue #317 scope grep) or ? in Result-returning fns. Build: cargo build --all-targets --all-features clean. Clippy: cargo clippy --all-targets --all-features --workspace -- -D warnings clean. Tests: cargo test --lib chains:: 521 passed; 4 failed (pre-existing on main: tests_basic_curves, tests_option_chain_surfaces — verified by checking out main at HEAD before this PR). ACCEPTANCE GREP — production unwrap/expect under src/chains/: 0.

`make readme` (cargo readme) picks up the BullCallSpread::new(...)? in the "Working with Trading Strategies" example added in PR #347 to match the new fallible constructor. Trivial 4-line diff.

Copilot

Pull request overview

This PR advances the “panic-free core” effort by removing production .unwrap()/.expect() usage under src/chains/ and switching chain generators to propagate failures as typed ChainErrors (plus doctest updates to compile with ?).

Changes:

Converted generator_optionchain and generator_positive to return Result<_, ChainError> and replaced internal panics with ?/safe fallbacks.
Added From<SimulationError> and From<VolatilityError> conversions into ChainError (via DynError) to support propagation without changing the public error enum variants.
Updated option-chain utilities/docs and adjusted in-tree call sites (tests/examples/README) to compile with the new generator signatures.

Reviewed changes

Copilot reviewed 14 out of 24 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/chains/generators.rs	Generators now return `Result`; internal unwraps replaced with propagation; tests updated to call `.unwrap()` on results.
src/error/chains.rs	Adds `From<SimulationError>` / `From<VolatilityError>` → `ChainError::DynError`.
src/chains/utils.rs	Removes unwraps in display + volatility helpers; adds fallbacks/warnings.
src/chains/optiondata.rs	Removes unwraps by using `if let`/locals; simplifies option checks via `is_some_and`.
src/chains/chain.rs	Replaces `.expect()` with `ok_or_else(...)?` for strike range computation.
src/chains/rnd.rs	Doctest examples updated to use `?`; removes `sqrt().unwrap()` in kurtosis path.
src/chains/mod.rs	Doctest examples wrapped in `run() -> Result` and updated to use `?`.
src/simulation/simulator.rs	Test updated to wrap generator call (now `Result`) via `.unwrap()`.
src/pricing/monte_carlo.rs	Test updated to wrap generator call (now `Result`) via `.unwrap()`.
src/strategies/short_put.rs	Simulation tests updated to wrap generator calls via `.unwrap()` closure.
src/strategies/short_call.rs	Simulation tests updated to wrap generator calls via `.unwrap()` closure.
src/strategies/long_put.rs	Simulation tests updated to wrap generator calls via `.unwrap()` closure.
src/strategies/long_call.rs	Simulation tests updated to wrap generator calls via `.unwrap()` closure.
examples/examples_simulation/src/bin/*.rs	Example binaries updated to compile by using `.expect()` adapters around `Result`-returning generators.
README.md	README examples adjusted for `?` / Result-returning constructors.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

PR #348 Copilot review feedback: - src/chains/generators.rs: when the underlying walk yields no points (e.g., Historical with insufficient `prices`), return `Ok(vec![walk_params.init_step.clone()])` instead of `Ok(vec![])`. Preserves the init-step invariant downstream consumers rely on (matches the rest of the generators). - src/chains/chain.rs::to_build_params: replace the `if !strike_prices.is_empty()` + dead `.ok_or_else(...)?` pair with a single `if let Some((min, max)) = ... .min().zip(... .max())` guard. Removes the misleading "strike_prices empty after non-empty check" error message; the empty case still defaults `chain_size = 10` later. - src/chains/optiondata.rs::apply_spread: the trace messages on the `(Some(ask), Some(bid), None)` arms previously claimed the spread "cannot be applied" while the code actually recomputes the middle price from bid/ask after applying the spread. Updated to match the real behaviour. Build: cargo build --all-targets --all-features clean. Clippy: cargo clippy --all-targets --all-features --workspace -- -D warnings clean. Tests: cargo test --lib chains:: 525 passed; 0 failed (the 4 pre-existing chain.rs surface/curve panics no longer reproduce after the to_build_params restructure). Doctests: 203 passed. Follow-up #349 tracks the larger Simulator::new / RandomWalk::new fallible-generator refactor flagged by the same review (out of scope for #317).

joaquinbejar · 2026-04-17T04:34:23Z

Thanks for the review. The four code-change suggestions are addressed in 51266b6, the larger Simulator/RandomWalk fallible-generator refactor is tracked separately in #349.

codecov · 2026-04-17T04:42:30Z

Codecov Report

❌ Patch coverage is 77.02703% with 17 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/chains/generators.rs	70.00%	9 Missing ⚠️
src/chains/utils.rs	73.33%	4 Missing ⚠️
src/error/chains.rs	0.00%	4 Missing ⚠️

Files with missing lines	Coverage Δ
src/chains/chain.rs	`85.66% <100.00%> (-0.01%)`	⬇️
src/chains/optiondata.rs	`91.83% <100.00%> (ø)`
src/chains/rnd.rs	`96.66% <100.00%> (ø)`
src/pricing/monte_carlo.rs	`100.00% <ø> (ø)`
src/simulation/simulator.rs	`88.00% <ø> (ø)`
src/strategies/long_call.rs	`70.88% <ø> (ø)`
src/strategies/long_put.rs	`71.65% <ø> (ø)`
src/strategies/short_call.rs	`74.00% <ø> (ø)`
src/strategies/short_put.rs	`72.36% <ø> (ø)`
src/chains/utils.rs	`68.38% <73.33%> (-1.01%)`	⬇️
... and 2 more

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

#349) Both constructors now accept a fallible generator `F: Fn(&WalkParams<X, Y>) -> Result<Vec<Step<X, Y>>, E>` and return `Result<Self, E>`, propagating any generator error unchanged. This removes the `|p| generator_positive(p).unwrap()` wrappers required since PR #348 made the chain generators fallible, restoring panic-free end-to-end pipelines per the M1 — Panic-Free Core milestone. Also adds `From<ChainError> for SimulationError` (symmetric with the existing reverse impl) so callers can `?`-propagate chain-generator errors through simulation contexts. Negative tests for both constructors verify the error surfaces unchanged and that `Simulator::new` short-circuits on the first failing generator invocation rather than building a partial simulator. All in-tree callers updated (strategy tests, simulator/randomwalk test mods, integration tests, examples). Test helpers using ad-hoc generators bumped to `Result<_, Infallible>` with `let-else` / `unreachable!()` patterns where appropriate.

Change `generator_optionseries` signature from `Vec<Step<Positive, OptionSeries>>` to `Result<Vec<Step<Positive, OptionSeries>>, ChainError>`, propagating errors from the underlying walker, volatility-estimation and chain-construction primitives via `?`. Same precedent as PR #348 for `generator_optionchain` / `generator_positive`, and PR #352 already updated `RandomWalk::new` / `Simulator::new` to accept fallible generators. Init invariant fix: when `prices.is_empty()` or insufficient for Historical walk type, now returns `Ok(vec![init_step])` instead of `vec![]`, matching the contains-init-step contract used by the other chain-family generators. Tests updated accordingly. Removed the dead-code if/else expression at lines 170-174 (its result was discarded); the cleaned-up form binds `volatility` to the unwrapped or default value before the loop. Doc comment switched from `# Panics` to `# Errors`.

joaquinbejar added 3 commits April 16, 2026 22:22

docs: regenerate README to match src/lib.rs doctest update

8f684bb

`make readme` (cargo readme) picks up the BullCallSpread::new(...)? in the "Working with Trading Strategies" example added in PR #347 to match the new fallible constructor. Trivial 4-line diff.

joaquinbejar requested a review from Copilot April 17, 2026 04:19

Copilot started reviewing on behalf of joaquinbejar April 17, 2026 04:19 View session

Copilot AI reviewed Apr 17, 2026

View reviewed changes

Comment thread src/chains/generators.rs

Comment thread src/chains/generators.rs Outdated

Comment thread src/chains/chain.rs Outdated

Comment thread src/chains/optiondata.rs

Comment thread src/chains/optiondata.rs

joaquinbejar mentioned this pull request Apr 17, 2026

Make Simulator::new and RandomWalk::new accept fallible generators #349

Closed

9 tasks

joaquinbejar merged commit fd9aad6 into main Apr 17, 2026
11 checks passed

joaquinbejar mentioned this pull request Apr 17, 2026

M1: Eliminate .unwrap()/.expect() from src/pricing/ (#318) #350

Merged

7 tasks

This was referenced Apr 17, 2026

fix(simulation,pricing): fallible RandomWalk/Simulator constructors + Telegraph guard (#349, #351) #352

Merged

refactor(curves,surfaces): eliminate .unwrap()/.expect() (#319) #353

Merged

This was referenced Apr 17, 2026

refactor(series,greeks): eliminate .unwrap()/.expect() (#320) #354

Merged

refactor: M1 final sweep — eliminate remaining .unwrap()/.expect() (#321) #355

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

M1: Eliminate .unwrap()/.expect() from src/chains/ (#317)#348

M1: Eliminate .unwrap()/.expect() from src/chains/ (#317)#348
joaquinbejar merged 4 commits intomainfrom
issue-317-chains-panic-free

joaquinbejar commented Apr 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joaquinbejar commented Apr 17, 2026

Uh oh!

Uh oh!

codecov bot commented Apr 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

joaquinbejar commented Apr 16, 2026

Summary

Approach

Public API impact (intentional, M1)

Verification

Commit map

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joaquinbejar commented Apr 17, 2026

Uh oh!

Uh oh!

codecov bot commented Apr 17, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants