Skip to content

replace unwrap() with error propagation across anise/src#693

Open
themouli wants to merge 1 commit intonyx-space:masterfrom
themouli:fix/replace-unwrap-with-error-propagation
Open

replace unwrap() with error propagation across anise/src#693
themouli wants to merge 1 commit intonyx-space:masterfrom
themouli:fix/replace-unwrap-with-error-propagation

Conversation

@themouli
Copy link
Copy Markdown
Contributor

Summary

Replaced unwrap() calls throughout anise/src/ with proper error propagation (?) or expect() carrying a meaningful invariant message where the call is provably infallible. Addresses #679 (follow-up to #677).

Improvements

  • Replaces unwrap() with ? propagation across the almanac, ephemerides, DAF, KPL, orientations, structure, and math modules so failures surface with typed errors instead of panicking.
  • Where an invariant genuinely guarantees success (e.g. a prior Some check, valid_up_to on UTF-8 decoding, a Keplerian-constructor contract), the unwrap() is replaced with expect("...") documenting why it cannot fail — making future debugging and audits easier.
  • DAF::persist now returns Result<(), DAFError> instead of io::Result<()>, so I/O errors carry the file path / action context and decoding failures propagate cleanly. Breaking change for external callers of persist.
  • KPL parser: file-open failure now reports as DataSetError::IO { action, source } preserving the underlying io::Error.

Bug Fixes

#679

Testing and validation

  • cargo check --all-targets clean (pre-existing pyo3 deprecation warnings only).
  • Existing test suite exercises the modified paths; no behavioral change on the happy path — only panics become recoverable errors.
  • No new tests added; a reviewer suggestion for targeted negative-path tests against DAF::persist and the KPL parser would be welcome if desired.

@themouli themouli force-pushed the fix/replace-unwrap-with-error-propagation branch from 36ce5a1 to 4889667 Compare April 24, 2026 09:28
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request replaces numerous instances of .unwrap() with .expect() or proper error handling throughout the codebase to improve robustness and error reporting. The review identified several areas for further improvement, including optimizing cloning operations in lookup table access, eliminating redundant parsing in file persistence, and correcting misleading variable naming in DER encoding logic.

Comment thread anise/src/almanac/planetary.rs
Comment thread anise/src/naif/daf/daf.rs Outdated
impl Encode for LookUpTable {
fn encoded_len(&self) -> der::Result<der::Length> {
let (ids, names, id_entries, name_entries) = self.der_encoding();
let (ids, names, id_entries, name_entries) = self.der_encoding()?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The destructuring of the tuple returned by der_encoding() has the names and id_entries variables swapped relative to the tuple's actual order (ids, id_entries, names, name_entries). While the subsequent addition in encoded_len is commutative and thus correct, the variable naming is misleading and could lead to bugs in future modifications.

Suggested change
let (ids, names, id_entries, name_entries) = self.der_encoding()?;
let (ids, id_entries, names, name_entries) = self.der_encoding()?;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope — the variable naming is pre-existing on master. My PR only added ? for error propagation, didn't touch the destructure or encode order.


fn encode(&self, encoder: &mut impl Writer) -> der::Result<()> {
let (ids, names, id_entries, name_entries) = self.der_encoding();
let (ids, names, id_entries, name_entries) = self.der_encoding()?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The variable names in this destructuring are swapped relative to the tuple returned by der_encoding(). This results in the names variable holding id_entries data and vice versa. Although they are encoded in the correct sequence (matching the Decode implementation), the naming is highly confusing for maintainers.

Suggested change
let (ids, names, id_entries, name_entries) = self.der_encoding()?;
let (ids, id_entries, names, name_entries) = self.der_encoding()?;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope — the variable naming is pre-existing on master. My PR only added ? for error propagation, didn't touch the destructure or encode order.

@themouli themouli force-pushed the fix/replace-unwrap-with-error-propagation branch from 4889667 to 7d6a7ca Compare April 24, 2026 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant