diff --git a/CHECKLIST.md b/CHECKLIST.md index da22c0218..be63f3ad9 100644 --- a/CHECKLIST.md +++ b/CHECKLIST.md @@ -106,6 +106,85 @@ be settled rather than discovered later. completion, then submits the query. A compound entry is reserved for a measured performance argument and would be a fusion of these two entries rather than a capability they lack. Depends on M21.3. +## M22 -- Discharge the failable-call standard across the workspace + +The standard is recorded in +[DESIGN-NOTES.md](DESIGN-NOTES.md#a-failable-call-has-its-failure-handled-always): a call that +can fail has its failure handled, with no per-site analysis. These items apply it to code +written before it was stated. + +- [ ] **M22.1** -- Audit every bare `unsafe { Call(...) };` statement in the workspace and handle + the failure of each one that is failable. + + **Scope, re-measured against main at `dc2b463` (2026-09-09, after PR #83 merged):** 119 + single-line `unsafe`-block statement discards across 11 crates -- `windows-threadpool-sys` 57, + `windows-platform-probes` 23, `windows-file-watcher` 9, `windows-ioring-sys` 9, + `windows-overlapped-io-sys` 5, `windows-thread-ambient-sys` 5, `windows-namespace-request-sys` 4, + `windows-guard-alloc` 3, `windows-impersonation-token-sys` 2, `windows-file-enumeration-sys` 1, + `windows-placement-probe` 1. + + **Most are not violations.** `SubmitThreadpoolWork`, `SetThreadpoolWait`, `GetSystemInfo`, + `SetLastError` and the `WaitForThreadpool*Callbacks` family return `void`, and `SetErrorMode` + returns the previous mode rather than a status. Discarding those is correct. + + The failable set is **30 sites**, each confirmed against its `windows-sys` signature rather than + assumed: + + | call | returns | discarded sites | + |---|---|---| + | `CloseHandle` | `BOOL` | 24 | + | `SetEvent` | `BOOL` | 2 | + | `CancelIoEx` | `BOOL` | 1 | + | `RevertToSelf` | `BOOL` | 1 | + | `SetCurrentDirectoryW` | `BOOL` | 1 | + | `CloseIoRing` | `HRESULT` | 1 | + + **Treat every number here as stale on arrival and re-measure.** These figures moved between two + measurements a few hours apart (121 -> 119 raw, and `SetEvent` 4 -> 2) purely because PR #83 + landed in between. Re-run the lint below rather than trusting the table; it is a description of + the shape of the work, not an inventory to tick off. + + **Use the compiler, not a regex: `RUSTFLAGS="-W unused_results"`.** `unused_results` is a + rustc lint, allow-by-default, that fires on any expression statement discarding a non-unit + value -- which is exactly this rule's shape, and it does not care how many lines the statement + spans or whether the callee is `unsafe`. + + (Either spelling works: rustc normalises `_` and `-` in lint names on the command line, and then + echoes the hyphenated form back -- a run of `-W unused_results` reports "requested on the command + line with `-W unused-results`". Verified on 1.98.0; noted only because that echo reads like a + correction and is not one.) Measured on `windows-platform-probes`: it flagged + every raw Win32 discard the regex found, plus four in `doorbell_cost.rs` the regex had counted + but nobody had looked at, in a file already believed fixed. A per-crate total is not a list. + + It is too noisy to deny workspace-wide, which is why it is an audit tool rather than a CI gate. + Measured on `windows-platform-probes` at `dc2b463`: **104 warnings, of which 62 are ordinary + Rust** -- `HashMap::insert`, `HashSet::remove`, `Vec::pop`, `fetch_add`, `black_box` -- and 42 + name a raw Win32 call. Not even those 42 are all violations, since several of the calls return + `void`. Triage is required at every step, and `SetErrorMode`'s previous mode and `fetch_add`'s + prior value are the standing examples of a discarded return that is not discarded failure + information. + + Do not reach for `#[must_use]` here: it cannot be applied to `windows-sys`'s `extern` block, so + it enforces nothing at the sites that matter. It becomes available only after M22.2, on our own + wrappers -- which is the durable end state, because a `#[must_use]` wrapper gives permanent + enforcement with none of the lint's noise. + +- [ ] **M22.2** -- Introduce a checked owning handle type and route the `CloseHandle` sites through + it, so the rule is discharged by construction rather than by 24 written-out checks. + + This is the type-embedding half of the decision, and `CloseHandle` is its clearest case: one + `Drop` that checks once removes every visible check at every use site and cannot be forgotten at + a new one. Note that `std`'s `OwnedHandle` is not a discharge on its own -- it closes on drop but + discards the `BOOL`. + + Settle two questions while doing it, because both determine whether the type is usable at all. + What a failing close should do in `Drop`, given that panicking in a drop during unwind aborts -- + the honest options are abort, a debug assertion, or a recorded counter, and they are not + equivalent. And whether teardown paths that legitimately expect a close to fail exist in this + workspace; `windows-threadpool-sys` owns wait targets whose close routine is a caller-supplied + function pointer, which is exactly where such a path would be. Depends on M22.1's + classification. + ## M-inf -- Parked Ungated work with no identified predecessor deliverable. diff --git a/DESIGN-NOTES.md b/DESIGN-NOTES.md index 2418f1aaa..5cb85f68c 100644 --- a/DESIGN-NOTES.md +++ b/DESIGN-NOTES.md @@ -1762,3 +1762,91 @@ reviews with suppressed comments and no marker. Almost all were addressed during that followed them, but "almost all" is not evidence, and marking them wholesale would convert an honest absence of information into a false record. Mark a historical review only when somebody has actually read it. + +## A failable call has its failure handled, always + +**The standard is: a call that can fail has its failure handled. No analysis, no exemption +for calls that "cannot fail in practice".** + +It is written as a flat rule on purpose, because the analysis it replaces is the thing that +goes wrong. Deciding per site whether a particular call is worth checking requires judging +how a failure would manifest, and that judgement is made by whoever is looking at the site -- +which means the calls that get exempted are not the ones that are genuinely infallible, they +are the ones nobody looked at hard enough. "Trivial enough not to check" is a fact about the +reader's attention, not a property of the call. + +The cost is a compare and a branch that is not taken, plus a little source bloat. That is +worth paying outright for a rule that needs no thought, and it is overwhelmingly worth paying +the first time it catches a real defect. + +### The measurement that settled it + +`windows-platform-probes` timed `SetEvent`, `ResetEvent` and `WaitForSingleObject` in loops +that discarded every `BOOL`, while the `SubmitIoRing` loop twelve lines below carried a +careful note explaining why discarding a status is exactly the hazard the crate exists to +avoid. The argument was correct and had been applied only to the call that looked expensive +enough to deserve it. + +Running that probe against a deliberately invalid handle, so every event call fails, gives +what the unchecked version reported against the true figures (x86_64, ns/op): + +| timing | reported when unchecked and every call fails | reported when the calls succeed | +|---|---|---| +| `set_event_already_signalled` | 212.7 | 205 | +| `set_reset_event` | 422.9 | 531 | +| `wait_zero_signalled` | 231.5 | 280 | + +None of those looks wrong; the first is within 4%. A failing syscall still costs a +measurable transition, so the report would have read as evidence while describing operations +that never happened. **A discarded status does not produce an obviously bad result -- it +produces a plausible one**, which is why it survives review. + +The checks cost nothing detectable: the same host reports 204-206, 528-534 and 280.3-280.7 +with them in place, which is run-to-run spread rather than a shift. + +### Prefer to discharge the rule in a type, where no caller can see it + +The best version of this standard is one a caller never has to follow, because a type already +did. A handle that closes itself in `Drop`, with the close checked once inside that `Drop`, +satisfies the rule at every use site without a single visible check -- and cannot be forgotten +at a new one. Where such a type is available the rule is discharged by construction; where a +raw call is genuinely the right tool, the check is written out. + +This is the preferred direction, not a precondition. A raw checked call today is correct and +complete; it is simply worth asking whether an owning type would remove the question. + +Note that `std`'s `OwnedHandle` is **not** by itself a discharge: it closes on drop but +discards `CloseHandle`'s `BOOL`, so a wrapper is still required to meet the standard. + +### The compiler can find these, but cannot enforce the rule + +`unused_results` -- a rustc lint, allow-by-default -- fires on any expression statement that +discards a non-unit value, which is this rule's shape exactly. It is the right instrument for +*finding* violations and is strictly better than searching for them: on `windows-platform-probes` +it flagged four discarded statuses in a file that had just been fixed by hand and re-swept by +regex, because a sweep that reports a per-crate total is not a list of sites. + +It is not a gate, and should not be denied workspace-wide. Most of its hits are ordinary Rust -- +`HashMap::insert`, `Vec::pop`, `fetch_add`, `black_box` -- 62 of 104 on `windows-platform-probes`, +with the remaining 42 naming a raw Win32 call of which several return `void` and are correct as +written. Denying it would trade this rule's "no analysis required" property for a large, permanent +triage burden, which is the same trade the rule exists to refuse. + +`#[must_use]` is the enforcement mechanism, and it is unavailable at precisely the sites that +matter: it cannot be attached to `windows-sys`'s `extern` declarations. It becomes available on +our own wrappers, which is a further argument for the type-embedding direction above -- a +`#[must_use]` wrapper enforces permanently with none of the lint's noise. + +So: the lint is how the audit is run, and a type is how the rule is kept. + +### What this does not cover + +A call that returns a value which is not a status -- `SetErrorMode` returning the previous +mode, `GetLastError` returning a code -- is not a failable call, and discarding its return is +not a violation. Nor is a `void`-returning call such as `SubmitThreadpoolWork`, +`SetThreadpoolWait`, `GetSystemInfo` or `SetLastError`; a survey of the workspace found the +majority of bare `unsafe { Call(...) };` statements are of that kind and are correct as +written. The rule is about **discarded failure information**, not about discarded returns. + +The audit this decision implies is queued as +[CHECKLIST.md](CHECKLIST.md) -> `M22.1`; it is not scheduled by this note alone.