Skip to content

Commit d5f045d

Browse files
hyperpolymathclaude
andcommitted
Revert 112 .expect("TODO: handle error") sites in parser.rs (batch 7/N)
Largest single file in the sweep. 92 test sites + 20 production sites across two prod-zone clusters (lines <3501 and one site at ~2745). Per-site policy: * Test code (92 sites in tests::*, heredoc_tests::*, job_control_tests::*, control_flow_tests::*, etc.): mass replace_all to `.unwrap()` (test convention; preceding asserts or string-literal inputs prove the Ok/Some case by construction). * Production code (20 sites) — all four restructures eliminate the `.expect("TODO")` panic shape: 1) `is_valid_var_name` (L1797): `let-else` absorbs the empty-name case directly; the explicit `if name.is_empty()` guard plus the trailing `expect` collapse to one construct, no panic site. 2) Two `chars.peek() == Some(&'$')` dollar-escape sites (L1841 + L2258): replaced `result.push(chars.next().expect(...))` with `chars.next(); result.push('$');` — peek-proven literal pushed directly, iterator advanced separately. Two panic sites cleared. 3) Six `chars.next().expect(...)` sites inside `expand_variables` / `expand_with_command_sub` simple-var chains (L1888, L1891, L1917, L1926, L1930, L1939, L2295, L2301, L2305, L2314 — and the parallel sites in cluster A at L890, L894, L912): the surrounding `if let Some(&next_ch) = chars.peek()` / match arms / `while let Some(&c) = chars.peek()` already bound the value; `chars.next()` called purely to advance, bound value used directly. Approx 12 panic sites cleared this way across both functions. 4) Three `var_value.expect("TODO")` sites in `apply_expansion` Default/AssignDefault/ErrorIfUnset branches (L2175/2189/2210): documented expect — `var_value` is Some by construction here, the surrounding `if is_unset || ...` arm covers the unset/null path, this `else` only fires when var_value is Some. Restructure too invasive across the eight expansion branches; expect with explicit reason is the right shape. 5) `pending_condition.take().expect()` in if/elif state machine (L2728): documented expect — state == "after_elif" iff the earlier ("elif", "after_then") arm set pending_condition to Some. Provable by construction. Verified locally: cargo build -> EXIT 0 cargo test -> 703 passed / 0 failed / 14 ignored (one transient failure in process_sub::tests::test_fifo_path_unique observed on first run; passes in isolation and on re-run — pre-existing FIFO-path race, unrelated to this commit, noted for follow-up). Cumulative progress: 404 / ~530 sites cleared (~76%) across 17 files. The original audit headline figure of 404 in `impl/rust-cli/src/` is now exactly cleared. Remaining: `impl/rust-ffi/` ↔ `ffi/rust/` duplicate tree (~50 sites each, but one tree should likely be deleted rather than fixed twice). Separate finding requiring a scope decision. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 1036832 commit d5f045d

1 file changed

Lines changed: 166 additions & 129 deletions

File tree

0 commit comments

Comments
 (0)