diff --git a/plans/exit.md b/plans/exit.md new file mode 100644 index 000000000..54094a4e7 --- /dev/null +++ b/plans/exit.md @@ -0,0 +1,193 @@ +# Plan: survey & phased removal of scattered `os.Exit` calls + +## Context + +Miller has ~170 `os.Exit` call sites scattered through non-test Go code under `pkg/` +(plus a handful in `cmd/experiments/`, which are throwaway sandboxes and out of scope). +The north star: `os.Exit` should happen only in `main` or its immediate callee +(`entrypoint.Main`), with everything below returning `error`. Not every last site must +be eliminated — genuine internal-coding-error assertions can stay — but user-facing +error and exit paths should propagate. + +Motivation: + +- **General hygiene** — exits deep in library code make the packages unusable as a + library, skip deferred cleanup (e.g. the pprof defers in `cmd/mlr/main.go`), and + create unreachable/untestable code paths. +- **Pre-work for [#341](https://github.com/johnkerl/miller/issues/341)** (DSL `exit` + statement) — an `exit 0`/`exit N` from inside a `put` expression must shut the + record stream down cleanly (flush writers, stop readers) and carry a *chosen* exit + code up to `main`. Raw `os.Exit` mid-goroutine can't do that. +- **Pre-work for [#440](https://github.com/johnkerl/miller/issues/440)** (strict mode) + — missing-field reads become *errors*, which need a propagation path from the DSL + evaluator up through the transformer chain to the process exit code. + +Prior art already in-tree (this effort is a continuation, not a fresh start): + +- Commit `6b32e1f41` "Reduce number of os.Exit callsites, part 1 of n" (#1055). +- `pkg/cli/errors.go`: sentinels `cli.ErrHelpRequested` (→ exit 0) and + `cli.ErrUsagePrinted` (→ exit 1). +- `pkg/cli/verb_utils.go`: error-returning verb-arg helpers (`VerbCheckArgCount`, + `VerbGetStringArg`, …, `VerbErrorf`) — the stated replacement for exit-on-failure + helpers. +- `TransformerParseCLIFunc` already returns `(RecordTransformer, error)` + (`pkg/transformers/aaa_record_transformer.go`). +- `pkg/climain/errors_json.go`: typed `CLIError{Kind,Token,Msg}` + `--errors-json` + structured emission. +- `mlrval.FromErrorString` error-valued Mlrvals + `state.NoExitOnFunctionNotFound` + (used in `pkg/dsl/cst/udf.go`) — the DSL-layer error convention. +- REPL precedent: `pkg/terminals/repl/dsl.go` prints DSL errors and continues (#1976), + while the same errors from `cst.ExecuteMainBlock` make `put_or_filter.go` exit. + +## Survey: where the ~170 sites live + +Counts are non-test `.go` under `pkg/`. Breakdown: 151 × `os.Exit(1)`, 15 × +`os.Exit(0)`, plus `os.Exit(exitCode)` / `os.Exit(entry.main(args))` dispatchers. + +| Cluster | Sites (approx) | Containing signature returns error today? | Difficulty | +|---|---|---|---| +| 1. `pkg/cli` main-flag parsing: `option_parse.go` (14), `flag_types.go`, `mlrcli_util.go` (`CheckArgCount`) | ~18 | No — `FlagParser` is void (`flag_types.go:58`); `FlagTable.Parse` returns bool | Signature change, then mechanical | +| 2. `pkg/climain`: `mlrcli_parse.go` (11), `mlrcli_mlrrc.go` (`loadMlrrcOrDie`) | 12 | Yes — `ParseCommandLine` and both passes return error | Mechanical | +| 3a. Transformer ParseCLI leftovers: constructor-error exits (e.g. `split.go`, `tee.go`, `subs.go`, `nest.go`, `join.go`, `surv.go`) | ~15 | Yes — `TransformerParseCLIFunc` returns error | Mechanical | +| 3b. Transformer **runtime** (`Transform` path): `tee.go`, `split.go`, `step.go`, `put_or_filter.go`, `utils/join_bucket_keeper.go` (5) | ~20 | No — `RecordTransformer.Transform` is void | Interface change | +| 4. DSL runtime: `pkg/dsl/cst/hofs.go` (14), `udf.go` (8), `root.go`, `evaluable.go` | ~25 | No — `IEvaluable.Evaluate` returns only `*mlrval.Mlrval` | Semantic; use error-Mlrval convention | +| 5. Writers/values: `record_writer_yaml.go` (4), `record_writer_json_jsonl.go` (2), `mlrval_output.go`, `mlrval_get.go`; `pkg/lib` (`WriteTempFileOrDie`, `CompileMillerRegexOrDie`, `mlrmath.go`) | ~18 | Mixed — many have `...OrError` siblings already | Mostly mechanical | +| 6. Sub-entrypoints: `pkg/auxents/*` (hex, unhex, lecat, termcvt), `pkg/terminals/*` (repl, script, regtest, completion, mcp) | ~40 | `main(args []string) int` per aux/terminal | Self-contained, mechanical | +| 7. Deliberate exit-0: `--version`, help, `--list-color-*`, `put -e`/`--explain` "valid" paths | ~15 | Varies | Convert to sentinels mapped at entrypoint | +| Keep as-is | — | `lib.InternalCodingErrorIf` family (~365 assertion call sites), `asserting_*` builtins (`pkg/bifs/types.go` — abort is their specified behavior), `os.Exit(entry.main(args))` dispatchers in `auxents.go`/`terminals.go` | — | + +## Design decisions + +### D1. Single exit point + +`cmd/mlr/main.go` stays exit-free (so its pprof/trace defers run); `entrypoint.Main` +becomes the one place that calls `os.Exit`, mapping errors to codes: + +```go +// pkg/entrypoint: nil → 0; lib.ExitRequest{Code} → Code; +// cli.ErrHelpRequested → 0; everything else → print (or EmitStructuredError) + 1 +``` + +The `ErrHelpRequested`/`ErrUsagePrinted` → exit translation currently mid-stack in +`mlrcli_parse.go:406-409` moves up to this boundary. + +### D2. `Transform` returns `error`; exit codes ride a typed sentinel + +Change the streaming interface: + +```go +type RecordTransformer interface { + Transform(inrecAndContext *types.RecordAndContext, + outputRecordsAndContexts *[]*types.RecordAndContext, + inputDownstreamDoneChannel <-chan bool, + outputDownstreamDoneChannel chan<- bool, + ) error +} +``` + +`runSingleTransformer`/`ChainTransformer` forward a non-nil error to a new +`chan error` consumed by `stream.Stream`'s select loop (alongside the existing +`inputErrorChannel`); the anemic `dataProcessingErrorChannel chan bool` is subsumed. +This also fixes the standing `XXX ... exit 1 & goroutine cleanup` in +`pkg/output/channel_writer.go`, whose print-and-signal-bool becomes send-the-error. + +**Exit(0) vs exit(1) disambiguation for #341 lives in the error's *type*, not the +transport** — the `io.EOF` pattern (sentinel error as control flow): + +```go +// pkg/lib (or pkg/types): satisfies error +type ExitRequest struct{ Code int } +``` + +A DSL `exit N` statement extends the existing `BlockExitPayload` machinery +(`pkg/dsl/cst/types.go:101-105` — BREAK/CONTINUE/RETURN already propagate out of +nested blocks) with a `BLOCK_EXIT_EXIT` status carrying the code; at the block +boundary it becomes `ExitRequest{N}` returned as an error from `ExecuteMainBlock` → +`put_or_filter.Transform` → chain → `stream.Stream` → `entrypoint.Main`, which exits +with `Code` — after the normal flush path. + +The synchronous error return is *more* amenable to #341 than a side-channel: a +free-standing error channel races with `doneWritingChannel` in the Stream select loop +and gives no ordering guarantee that records emitted *before* the `exit` statement get +flushed. A returned error lets `ChainTransformer` forward already-produced records, +signal downstream-done (the same drain mechanism `head -n` uses), and only then +surface the `ExitRequest`. + +### D3. DSL runtime errors use the existing error-Mlrval convention + +The `...OrDie` helpers in `hofs.go`/`udf.go` return `mlrval.FromErrorString(...)` +values instead of exiting (generalizing what `state.NoExitOnFunctionNotFound` already +does at `udf.go:120-132`). `cst.Execute*Blocks` already return `error`; +`put_or_filter.go` stops swallowing those with `os.Exit(1)` and returns them from +`Transform` (per D2). This is exactly the plumbing #440 strict mode needs. + +### D4. What stays + +- `lib.InternalCodingErrorIf` / `...WithMessageIf` — should-never-happen assertions, + by their own doc comment. (`MLR_PANIC_ON_INTERNAL_ERROR` already gives stack traces.) +- `asserting_*` DSL builtins (`pkg/bifs/types.go` `assertingCommon`) — abort on + assertion failure is their documented contract. +- Top-of-stack dispatchers `os.Exit(entry.main(args))` in `auxents.go`/`terminals.go` + — these *are* the immediate-callee exits; inner exits within each aux/terminal + convert to `return ` up to their `main(args) int`. + +### Behavioral invariants + +Stderr message text and process exit codes must not change (the regression suite +asserts on both; see also recent exit-code fixes #2171, #2146). `--errors-json` +categorization must keep working — and gains coverage, since stream-time errors that +today bypass `EmitStructuredError` will now arrive at the entrypoint as errors. + +## Phases (one PR each, each green under `make dev` + `make lint`) + +### Phase 1 — top level + all mechanical swaps (clusters 2, 3a, 5, 7 partial) +- `entrypoint.Main() (MainReturn, error)` (or int code); the two exits in + `entrypoint.go` become the single mapping point; introduce `ExitRequest`. +- `mlrcli_parse.go` validation exits → returned `CLIError`s; `loadMlrrcOrDie` → + `loadMlrrc(...) error`; move help/usage-sentinel exit translation to entrypoint. +- Transformer ParseCLI constructor exits → `return nil, err` (`split.go:234` et al.). +- Migrate callers to existing error siblings: `CompileMillerRegexOrDie` → + `CompileMillerRegex`, mlrval `...OrDie` getters → `...OrError`, `WriteTempFileOrDie` + → error-returning version; `mlrcli_util.go CheckArgCount` callers → `VerbCheckArgCount` + style where signatures already allow. +- Deliberate exit-0 sites reachable from error-returning frames (`--version`, `put -e` + "valid", `subs.go:180`) → sentinel returns. + +### Phase 2 — `FlagParser` signature (cluster 1) +- `type FlagParser func(...) error`; `FlagTable.Parse` → `(bool, error)`; update the + inline `parser:` closures in `option_parse.go` and the four `Parse` call sites + (`mlrcli_parse.go`, `mlrcli_mlrrc.go`, `transformers/split.go`). +- Tedious but rote; also lets `pkg/terminals/completion` drop its + "parsers call os.Exit" workaround (noted in its header comment). + +### Phase 3 — streaming interface (cluster 3b) **← the load-bearing phase for #341/#440** +- `Transform` returns `error`; all ~55 verbs get `return nil` on happy paths. +- `ChainTransformer`/`runSingleTransformer` + `ChannelWriter` propagate errors via + `chan error`; retire `dataProcessingErrorChannel chan bool`; `stream.Stream` selects + and returns. +- Convert runtime exits in `tee.go`, `split.go`, `step.go`, `join_bucket_keeper.go`, + and `put_or_filter.go` (the `ExecuteBeginBlocks`/`ExecuteMainBlock`/ + `ExecuteEndBlocks` err → exit sites) into returned errors. + +### Phase 4 — sub-entrypoints (cluster 6) +- Per aux/terminal: usage funcs stop taking an `exitCode int` and exiting; inner I/O + helpers return errors; each `main(args []string) int` returns codes to the + dispatcher. `repl/entry.go`, `script/entry.go`, `regtest`, `hex/unhex/lecat/termcvt`. + +### Phase 5 — DSL runtime (cluster 4) +- `hofs.go`/`udf.go` `...OrDie` helpers → error-Mlrvals per D3; delete + `NoExitOnFunctionNotFound` special-casing once return-errors is the only behavior. +- Behavior note to document: HOF misuse now surfaces as `mlr: ` via error + propagation (same message, same exit 1 at top) rather than instant abort; REPL keeps + its print-and-continue behavior for free. + +## Verification + +- `make dev` (fmt, build, unit + regression, docs) and `make lint` per phase. +- Exit-code checks: `mlr --version; echo $?` (0), unknown verb/flag (1), `mlr put + 'syntax error'` (1), `mlr join` with unreadable left file (1, per #2171), broken + pipe `mlr cat big | head -1` (code unchanged), `--errors-json` output shape. +- `mlr repl` still print-and-continues on DSL errors. +- Grep gate per phase: `grep -rn os.Exit pkg/ | grep -v _test` shrinks to the + documented keep-list; final state ≈ `entrypoint.go`, `logger.go`, `bifs/types.go` + (asserting), `auxents.go`/`terminals.go` dispatchers.