Skip to content

Conversation

nicklasl
Copy link
Member

@nicklasl nicklasl commented Oct 8, 2025

Enable clippy arithmetic side effects lint and fix violations

This PR enables strict clippy lints to catch potential runtime panics from arithmetic operations and fixes all violations in the codebase.

Background

The Rust resolver has #![deny(...)] directives for several clippy lints including arithmetic_side_effects, panic, unwrap_used, expect_used, and indexing_slicing. These lints help prevent runtime panics in production code.

Changes

1. Fixed arithmetic operations throughout the codebase

  • err.rs: Use wrapping arithmetic for error code calculations
  • flag_logger.rs: Use saturating arithmetic for counter increments
  • gzip.rs: Use checked arithmetic operations with proper error handling
  • lib.rs: Use checked/saturating operations for bucket hashing and time calculations
  • resolve_logger.rs: Add overflow protection

2. Improved proto-generated code handling

The previous approach only suppressed arithmetic_side_effects in generated code. However, proto-generated code can violate multiple clippy lints (panic, unwrap, indexing, etc.).

Solution: Suppress all clippy warnings in proto-generated code by:

  • Adding comprehensive #[allow(...)] attributes to prost-generated types via config.type_attribute()
  • Post-processing pbjson-generated serde files to prepend allow attributes before each impl block
  • Extracting the allow attribute into a shared constant to avoid duplication

This approach follows the pattern from rust-clippy#702 for handling generated code.

Testing

  • cargo clippy --all-targets --all-features passes for confidence_resolver
  • ✅ All arithmetic operations use safe variants (checked, saturating, or wrapping as appropriate)
  • ✅ Generated proto code no longer produces clippy warnings

nicklasl and others added 11 commits October 8, 2025 11:25
- Remove unused imports
- Add Default trait implementations for Logger and ResolveLogger
- Simplify unnecessary clone and ref patterns

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

Co-Authored-By: Claude <noreply@anthropic.com>
Add clippy allow attributes to suppress arithmetic_side_effects warnings
in prost and pbjson generated code:
- Apply to all generated proto types via prost config
- Inject before each impl block in serde files post-generation

This reduces clippy errors from 363 to 43 when arithmetic_side_effects
lint is enabled.

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

Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@nicklasl nicklasl marked this pull request as ready for review October 8, 2025 13:26
- Change bucket() to return Fallible<usize> instead of Option<usize>
  for better error context when buckets is 0
- Add explicit zero-check with meaningful error tag :bucket.zero_buckets
- Remove unnecessary .or_fail() calls at bucket() call sites
- Add clippy::arithmetic_side_effects allow on bucket() function
  with comment explaining buckets != 0 is checked
- Add clippy allows to err.rs helper functions (fnv1a64, b64u6)

This addresses PR feedback from @andreas-karlsson about using
Result/Fallible for better error handling instead of Option.

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

Co-Authored-By: Claude <noreply@anthropic.com>
Remove unnecessary wrapping_add/wrapping_sub in loop counters and
base64 encoding. The #[allow(clippy::arithmetic_side_effects)]
attribute is sufficient - only the hash multiplication actually
needs wrapping behavior.

This makes the code simpler and closer to the main branch while
still satisfying clippy.

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

Co-Authored-By: Claude <noreply@anthropic.com>
@nicklasl nicklasl merged commit 83912c9 into main Oct 9, 2025
6 checks passed
@nicklasl nicklasl deleted the nicklasl/arithmetic-panics branch October 9, 2025 14:56
@github-actions github-actions bot mentioned this pull request Oct 9, 2025
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.

2 participants