Skip to content

chore: Apply defensive programming patterns to improve code safety#336

Merged
tlepoint merged 1 commit into
mainfrom
fix/defensive-programming
Dec 7, 2025
Merged

chore: Apply defensive programming patterns to improve code safety#336
tlepoint merged 1 commit into
mainfrom
fix/defensive-programming

Conversation

@tlepoint

@tlepoint tlepoint commented Dec 7, 2025

Copy link
Copy Markdown
Owner

This commit implements defensive programming techniques from the Rust
patterns guide to make the codebase more robust and prevent runtime
panics by leveraging compiler guarantees.

https://corrode.dev/blog/defensive-programming/

Key Changes

Compiler-Enforced Safety Improvements

  1. PartialEq with destructuring pattern (4 implementations)

    • Modulus: Now explicitly lists all 7 fields
    • Plaintext: Now compares all 5 fields (previously missed poly_ntt & level)
    • ContextLevel: Explicitly handles all 9 fields (now includes poly_context)
    • NttOperator: Added destructuring with documentation

    These changes ensure the compiler will error when new fields are added,
    forcing developers to explicitly decide how to handle them in equality
    comparisons.

  2. From → TryFrom conversion

    • Fixed Vec conversion from Poly that could panic on unwrap()
    • Now properly returns Result for fallible conversions
    • Updated all call sites to handle errors appropriately
  3. Safe vector access patterns

    • Ciphertext::new: Uses .first() instead of [0] indexing
    • Ciphertext serialization: Uses .split_last() pattern matching
    • fhe-util functions: Uses .get() with clear error messages

Defensive Clippy Lints

Added workspace-level lints in Cargo.toml:

  • indexing_slicing = "deny" - Prevents unsafe array access
  • fallible_impl_from = "deny" - Ensures From impls don't panic
  • wildcard_enum_match_arm = "deny" - Forces exhaustive enum matching
  • fn_params_excessive_bools = "deny" - Encourages clearer APIs
  • must_use_candidate = "warn" - Suggests #[must_use] attributes

Note: unneeded_field_pattern intentionally excluded as it conflicts
with the destructuring pattern for compiler-enforced field handling.

Additional Improvements

  • Added #[must_use] attributes to 48+ functions to prevent ignoring
    return values
  • Performance-critical crypto code (NTT, RNS, RQ, BFV modules) exempted
    from indexing lint with clear documentation
  • Tests, benchmarks, and examples allowed indexing for simplicity

Impact

  • All 200+ tests passing
  • Zero clippy errors with -D warnings
  • Future struct modifications will trigger compile errors instead of
    runtime panics
  • Better error handling with explicit Result types

🤖 Generated with Claude Code

Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com

This commit implements defensive programming techniques from the Rust
patterns guide to make the codebase more robust and prevent runtime
panics by leveraging compiler guarantees.

## Key Changes

### Compiler-Enforced Safety Improvements

1. **PartialEq with destructuring pattern** (4 implementations)
   - Modulus: Now explicitly lists all 7 fields
   - Plaintext: Now compares all 5 fields (previously missed poly_ntt & level)
   - ContextLevel: Explicitly handles all 9 fields (now includes poly_context)
   - NttOperator: Added destructuring with documentation

   These changes ensure the compiler will error when new fields are added,
   forcing developers to explicitly decide how to handle them in equality
   comparisons.

2. **From → TryFrom conversion**
   - Fixed Vec<u64> conversion from Poly that could panic on unwrap()
   - Now properly returns Result for fallible conversions
   - Updated all call sites to handle errors appropriately

3. **Safe vector access patterns**
   - Ciphertext::new: Uses .first() instead of [0] indexing
   - Ciphertext serialization: Uses .split_last() pattern matching
   - fhe-util functions: Uses .get() with clear error messages

### Defensive Clippy Lints

Added workspace-level lints in Cargo.toml:
- indexing_slicing = "deny" - Prevents unsafe array access
- fallible_impl_from = "deny" - Ensures From impls don't panic
- wildcard_enum_match_arm = "deny" - Forces exhaustive enum matching
- fn_params_excessive_bools = "deny" - Encourages clearer APIs
- must_use_candidate = "warn" - Suggests #[must_use] attributes

Note: unneeded_field_pattern intentionally excluded as it conflicts
with the destructuring pattern for compiler-enforced field handling.

### Additional Improvements

- Added #[must_use] attributes to 48+ functions to prevent ignoring
  return values
- Performance-critical crypto code (NTT, RNS, RQ, BFV modules) exempted
  from indexing lint with clear documentation
- Tests, benchmarks, and examples allowed indexing for simplicity

## Impact

- All 200+ tests passing
- Zero clippy errors with -D warnings
- Future struct modifications will trigger compile errors instead of
  runtime panics
- Better error handling with explicit Result types

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread Cargo.toml
Comment on lines +40 to +42
[workspace.lints.clippy]
indexing_slicing = "deny"
fallible_impl_from = "deny"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge New clippy lint breaks workspace build

Enabling fallible_impl_from = "deny" at workspace level will cause cargo clippy --all-targets -- -D warnings to fail immediately because there are existing From implementations that call unwrap (for example impl From<&Poly> for Rq in crates/fhe-math/src/rq/convert.rs:15-47 uses v.as_slice().unwrap()). That lint forbids fallible From impls, so adding it without converting those impls to TryFrom or adding an allow will make clippy/CI fail.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

This is indeed changed in this PR.

@tlepoint
tlepoint merged commit 930f190 into main Dec 7, 2025
6 checks passed
@tlepoint
tlepoint deleted the fix/defensive-programming branch December 7, 2025 22:26
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