refactor(payload): mark infallible .expect() sites as unreachable!()#1884
Draft
goxberry wants to merge 1 commit into
Draft
Conversation
Converts 26 infallible-by-construction `.expect()` sites in lading_payload
to `.unwrap_or_else(|| unreachable!("..."))`. The runtime behavior is
unchanged at every site — both `.expect()` and `unreachable!()` panic —
but the new form documents at the call site *why* the panic cannot fire,
not just *what message would be printed if it did*.
Sites covered:
- 20 `CONST_ARR.choose(rng)` against non-empty `const` arrays in
`apache_common.rs`, `datadog_logs.rs`, `json.rs`, `splunk_hec.rs`,
and `syslog.rs`.
- 5 `write!(&mut Vec<u8>, ...)` calls (`apache_common.rs`, `dogstatsd.rs`
×2, `splunk_hec.rs`, `syslog.rs`). `io::Write` on `Vec<u8>` cannot fail.
- 3 `Option`-after-guard sites: `as_mut().expect(...)` post-`ensure_reader()`
in `static_chunks.rs`, `take().expect(...)` post-`fill_next_block()` in
`static_timestamped.rs`, and `Byte::from_u64_with_unit(1, MiB)` in
`block.rs` (also drops the now-stale `# Panics` doc since the function
no longer documents an `.expect()`).
This is the second per-crate cleanup in the stack started by
"chore(clippy): forbid .expect() in production code". The crate-root
`#![allow(clippy::expect_used)]` quarantine on `lading_payload` is NOT
removed here — Cat-1 (?-prop), Cat-2 (signature changes), and Cat-4
(genuine panic intent) sites remain. The quarantine drop lands in the
final PR of this sub-stack after those layers.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
This was referenced May 22, 2026
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
1 participant
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.
What does this PR do?
Converts 26 infallible-by-construction
.expect()sites inlading_payloadto.unwrap_or_else(|| unreachable!("...")). The runtime behavior is identical at every site (both forms panic if the impossible happens); the new form documents why the panic is unreachable at the call site, and tags the branch with the standardunreachable!()macro that Rust optimizers and humans both recognize.This PR does not remove the
lading_payloadquarantine — that lands later in the sub-stack after Cat-1/Cat-2/Cat-4 sites are addressed.Sites covered
CONST_ARR.choose(rng).expect(...)against non-emptyconstarrays.choose(rng).unwrap_or_else(|| unreachable!("CONST_ARR is a non-empty const array"))write!(&mut Vec<u8>, ...).expect("formatting to Vec<u8> cannot fail").unwrap_or_else(|_| unreachable!("io::Write on Vec<u8> never errors"))Option-after-guard (as_mut/takeimmediately after a function that populates the value).unwrap_or_else(|| unreachable!("<guard> populates <field> on Ok"))Files touched:
apache_common.rs,block.rs,datadog_logs.rs,dogstatsd.rs,json.rs,splunk_hec.rs,static_chunks.rs,static_timestamped.rs,syslog.rs. +69 −31.Why this shape, not the alternatives
The first attempt at this PR replaced the const-array sites with
CONST_ARR[rng.random_range(0..CONST_ARR.len())]— eliminating theOptionentirely. While that approach has zero runtime panic surface, it (a) repeats the const name at the call site, (b) loses the "choose" reading, and (c) requires dropping theIndexedRandomimport in some files. Reviewer feedback on the draft preferred the more drop-in form. Both forms are equally vulnerable to a refactor leavingCONST_ARRempty — neither has compile-time protection against that. Addingconst _: () = assert!(!CONST.is_empty());next to each declaration is a reasonable follow-up but is out of scope here.Motivation
Second per-crate cleanup in the stack started by #1882. Strategy:
?-propagation) + Category-4 (genuine panic intent, where the.expect()is the contract).lading_payloadquarantine.Related issues
Stacked on #1883.
Additional Notes
cargo build --all-targets --all-features✓cargo clippy --all-targets --all-features✓ (fullci/clippy)cargo test -p lading-payload --lib✓ (244 passed, 0 failed).expect()sites inlading_payloadinto four categories; the 26 sites here are the strict Category-3 subset. Categories 1, 2, 4 are not touched by this PR.