Skip to content

refactor(payload): mark fallible-context infallibles as unreachable!()#1885

Draft
goxberry wants to merge 1 commit into
goxberry/expect-cleanup-payload-infalliblefrom
goxberry/expect-cleanup-payload-result-prop
Draft

refactor(payload): mark fallible-context infallibles as unreachable!()#1885
goxberry wants to merge 1 commit into
goxberry/expect-cleanup-payload-infalliblefrom
goxberry/expect-cleanup-payload-result-prop

Conversation

@goxberry
Copy link
Copy Markdown
Contributor

@goxberry goxberry commented May 22, 2026

What does this PR do?

Converts 5 .expect() sites in lading_payload to .unwrap_or_else(... unreachable!("...")). The sites all live inside Result-returning functions but are themselves guarded by local invariants — propagating with ? would introduce dead error paths.

Sites

File:line Site Local invariant
common/strings/string_list_pool.rs:179 start.parse::<u32>().expect(...) start_is_num was set to start.parse::<u32>().is_ok() 27 lines above
common/strings/string_list_pool.rs:182 end.parse::<u32>().expect(...) same, for end
common/strings/string_list_pool.rs:192 start.chars().next().expect(...) start_is_char = start.len() == 1 && !start_is_num
common/strings/string_list_pool.rs:196 end.chars().next().expect(...) same, for end
templated_json/resolver.rs:155 self.raw_defs[idx].take().expect(...) three-color visit machine: reachable only when !black[idx] && !grey[idx], which corresponds to raw_defs[idx] being Some

Why unreachable!() and not ?

An earlier draft of this PR used ?-propagation (the agent's initial recommendation, since the containing functions return Result). On review the choice changes: these aren't real error paths — the parses cannot fail because start_is_num was set from a successful parse::<u32>().is_ok(), and chars().next() cannot return None on a string the local code has just established has len() == 1. unreachable!() documents that fact; ? would leave a dead Err(Error::MalformedPattern{...}) branch behind that's never produced.

These sites fit the same shape as the previous unwrap_or_else(|| unreachable!()) refactor (#1884), but were initially split out because they live in Result-returning functions. They're being landed in their own PR for review clarity, not because they need a different treatment.

Motivation

Third per-crate cleanup-PR in the stack started by #1882. Future PRs in this sub-stack:

  • Group B (next): 6 sites in block.rs, random_string_pool.rs, string_list_pool.rs where the invariant is established in the same function (same pattern as this PR; separated by the "originally in a Result fn" distinction).
  • Group C: ~16 sites where the .expect() is the contract (handle-table lookups, documented API panics) — annotated with #[expect(clippy::expect_used, reason = "...")].
  • Group D: real bug fix in block.rs:123 (the arbitrary::Arbitrary impl for Block can panic on u32::arbitrary returning 0).
  • Final: drop the lading_payload quarantine.

Related issues

Stacked on #1884.

Additional Notes

  • cargo build --all-targets --all-features
  • cargo clippy --all-targets --all-features
  • cargo test -p lading-payload --lib ✓ (244 passed)
  • No public-API change. Diff: 3 files, +12 −5.

Copy link
Copy Markdown
Contributor Author

goxberry commented May 22, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@datadog-prod-us1-6

This comment has been minimized.

Five `.expect()` sites in `lading_payload` live inside `Result`-returning
functions but are themselves guarded by local invariants that make their
`Err` branch unreachable. The agent inventory initially classified them
as "Cat-1: `?`-propagation feasible" because the surrounding function
returns `Result`, but propagating with `?` would introduce dead error
paths — the parse cannot fail when `start_is_num` is true, and the
chars-iter cannot return `None` when `start_is_char` (which requires
`start.len() == 1`) is true.

Sites:
- `common/strings/string_list_pool.rs:179,182` (parse::<u32>() guarded
  by `start_is_num`/`end_is_num` flags set just above)
- `common/strings/string_list_pool.rs:192,196` (chars().next() on
  strings guaranteed non-empty by `start_is_char`/`end_is_char`)
- `templated_json/resolver.rs:155` (raw_defs[idx].take() inside the
  three-color (white/grey/black) visit state machine; reachable only
  when `black[idx] == false && grey[idx] == false`, which corresponds
  to raw_defs[idx] being Some)

Same pattern as the prior `unreachable!()` refactor (PR #1884). No
runtime behavior change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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