From 516f733b495385713d6f85f877b4530ad4860e21 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Wed, 9 Sep 2026 12:54:46 -0400 Subject: [PATCH 1/4] docs: adopt the failable-call standard, and queue its workspace audit The standard: a call that can fail has its failure handled. No per-site analysis, no exemption for calls that "cannot fail in practice". It is flat on purpose, because the analysis it replaces is the thing that goes wrong. Judging per site whether a call is worth checking means the exempted calls are not the genuinely infallible ones, 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 an untaken branch, which is worth paying outright for a rule that needs no thought. The measurement that settled it, from `windows-platform-probes`: three timing loops discarded every `BOOL` while the `SubmitIoRing` loop twelve lines below carried a careful note explaining why that is the hazard the crate exists to avoid. Run against a deliberately invalid handle so every event call fails, the unchecked version reported 212.7 ns for a redundant `SetEvent` (true 205), 422.9 for the set/reset cycle (true 531) and 231.5 for a satisfied wait (true 280). None looks wrong; the first is within 4%. A discarded status does not produce an obviously bad result, it produces a plausible one, which is why it survives review. The note also records the preferred direction -- discharge the rule in a type, where no caller can see it -- and what the rule does not cover: a `void` return or a non-status return like `SetErrorMode`'s previous mode is not discarded failure information. Queues M22.1 (classify and fix the failable sites) and M22.2 (the checked owning handle type). Measured scope: 121 bare `unsafe { Call(...) };` statements across 11 crates, of which most are void-returning and correct; the failable set is roughly 28 and is dominated by `CloseHandle` at 24 sites. The item records that the regex saw single-line statements only, so 121 is a floor rather than a total. Recorded here rather than in the probes crate because the standard is workspace-wide, and queued as checklist items because a decision recorded only in a design note schedules no work. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHECKLIST.md | 45 +++++++++++++++++++++++++++++++++ DESIGN-NOTES.md | 67 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+) diff --git a/CHECKLIST.md b/CHECKLIST.md index da22c0218..21ee1cee2 100644 --- a/CHECKLIST.md +++ b/CHECKLIST.md @@ -106,6 +106,51 @@ 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, as measured on 2026-09-09:** 121 single-line `unsafe`-block statement discards across + 11 crates. 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. The failable set is roughly 28 + sites and is dominated by **`CloseHandle` (24 sites)**, with `SetEvent`/`ResetEvent` making up + most of the rest. Per-crate totals of the raw 121: `windows-threadpool-sys` 57, + `windows-platform-probes` 25 (since discharged), `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. + + Classify before changing anything: the count above is a starting point from a regex, not a + verdict, and each site needs its callee's return type confirmed. Record the classification so + the next reader does not redo it. + + **The multi-line form is not covered by that count.** The regex matched single-line statements + only, so a discarded status spread across several lines was not counted and the real figure is + at least 121. Widen the search before declaring the audit complete. + +- [ ] **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..d450d7a74 100644 --- a/DESIGN-NOTES.md +++ b/DESIGN-NOTES.md @@ -1762,3 +1762,70 @@ 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 | unchecked, every call failing | true | +|---|---|---| +| `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. + +### 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. From ca6b4b9e47d653323f4dde6fb0c5e483af95a6c4 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Wed, 9 Sep 2026 13:02:09 -0400 Subject: [PATCH 2/4] docs: record that unused_results is the audit tool, and must_use the end state The standard needed a mechanism, and the answer is two things rather than one. `unused_results` is a rustc lint, allow-by-default, that fires on any expression statement discarding a non-unit value -- this rule's shape exactly, and indifferent to how many lines the statement spans or whether the callee is `unsafe`. It found four discarded statuses in a `windows-platform-probes` file that had just been fixed by hand and re-swept by regex, which is the argument for it: a sweep reporting a per-crate total is not a list of sites. It is not a gate. Two thirds of its 112 hits on that crate are ordinary Rust -- `HashMap::insert`, `Vec::pop`, `fetch_add`, `black_box` -- so denying it workspace-wide would trade the rule's "no analysis required" property for a permanent triage burden, which is the trade the rule exists to refuse. `#[must_use]` is the enforcement mechanism and is unavailable where it would matter: it cannot be attached to `windows-sys`'s `extern` declarations. It becomes available on our own wrappers, which is a further argument for M22.2 -- 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. Both recorded on the decision and on M22.1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHECKLIST.md | 20 +++++++++++++++++--- DESIGN-NOTES.md | 20 ++++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/CHECKLIST.md b/CHECKLIST.md index 21ee1cee2..c73ee82bd 100644 --- a/CHECKLIST.md +++ b/CHECKLIST.md @@ -131,9 +131,23 @@ written before it was stated. verdict, and each site needs its callee's return type confirmed. Record the classification so the next reader does not redo it. - **The multi-line form is not covered by that count.** The regex matched single-line statements - only, so a discarded status spread across several lines was not counted and the real figure is - at least 121. Widen the search before declaring the audit complete. + **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`. 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: + 112 warnings on that one crate, of which roughly two thirds are legitimate Rust idioms -- + `HashMap::insert`, `HashSet::remove`, `Vec::pop`, `fetch_add`, `black_box`. Triage is required, + 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. diff --git a/DESIGN-NOTES.md b/DESIGN-NOTES.md index d450d7a74..81ca28ec8 100644 --- a/DESIGN-NOTES.md +++ b/DESIGN-NOTES.md @@ -1818,6 +1818,26 @@ complete; it is simply worth asking whether an owning type would remove the ques 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. Two thirds of its hits are ordinary +Rust -- `HashMap::insert`, `Vec::pop`, `fetch_add`, `black_box` -- so 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 From 4809f48f140e9e0c17fb9afb0df5130f9ff37e52 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Wed, 9 Sep 2026 16:27:45 -0400 Subject: [PATCH 3/4] docs: name the comparison column, and re-measure the audit scope against merged main Merges `origin/main` (PR #83) and reconciles this branch's measured claims with what landed, since every figure here describes code that just changed. The measurement table's third column was headed "true", which does not say what it is true *of*. Both columns are now named for the run they came from -- what the unchecked probe reported when every call failed, against what it reports when they succeed. Re-measured against `dc2b463` rather than left as written: - 121 bare `unsafe { Call(...) };` statements became 119, and `windows-platform-probes` 25 became 23, because #83 fixed sites inside it. - `SetEvent` discards fell from 4 to 2 for the same reason. - The `unused_results` count on that crate fell from 112 to 104. - "Two thirds are ordinary Rust" was an overstatement: it is 62 of 104, about sixty percent, with the remaining 42 naming a raw Win32 call -- and not even all of those are violations, since several of the calls return `void`. The failable set is now a table of six calls totalling 30 sites, each confirmed against its `windows-sys` signature rather than assumed. `CancelIoEx`, `RevertToSelf`, `SetCurrentDirectoryW` and `FindCloseChangeNotification` were checked and all return `BOOL`; `CloseIoRing` returns an `HRESULT`. The entry now says plainly that every number in it is stale on arrival and the lint should be re-run instead. That is the honest reading of figures that moved twice in one afternoon from an unrelated merge, and it keeps the item a description of the work's shape rather than an inventory somebody ticks off against a tree that has moved underneath it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHECKLIST.md | 53 +++++++++++++++++++++++++++++++------------------ DESIGN-NOTES.md | 11 +++++----- 2 files changed, 40 insertions(+), 24 deletions(-) diff --git a/CHECKLIST.md b/CHECKLIST.md index c73ee82bd..9d369ceba 100644 --- a/CHECKLIST.md +++ b/CHECKLIST.md @@ -116,20 +116,33 @@ 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, as measured on 2026-09-09:** 121 single-line `unsafe`-block statement discards across - 11 crates. 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. The failable set is roughly 28 - sites and is dominated by **`CloseHandle` (24 sites)**, with `SetEvent`/`ResetEvent` making up - most of the rest. Per-crate totals of the raw 121: `windows-threadpool-sys` 57, - `windows-platform-probes` 25 (since discharged), `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. - - Classify before changing anything: the count above is a starting point from a regex, not a - verdict, and each site needs its callee's return type confirmed. Record the classification so - the next reader does not redo it. + **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 @@ -138,11 +151,13 @@ written before it was stated. 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: - 112 warnings on that one crate, of which roughly two thirds are legitimate Rust idioms -- - `HashMap::insert`, `HashSet::remove`, `Vec::pop`, `fetch_add`, `black_box`. Triage is required, - 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. + 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 diff --git a/DESIGN-NOTES.md b/DESIGN-NOTES.md index 81ca28ec8..5cb85f68c 100644 --- a/DESIGN-NOTES.md +++ b/DESIGN-NOTES.md @@ -1790,7 +1790,7 @@ 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 | unchecked, every call failing | true | +| 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 | @@ -1826,10 +1826,11 @@ discards a non-unit value, which is this rule's shape exactly. It is the right i 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. Two thirds of its hits are ordinary -Rust -- `HashMap::insert`, `Vec::pop`, `fetch_add`, `black_box` -- so 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. +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 From 36ba6c3535d9bf1005c92ded3f6e827a6ea31576 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Wed, 9 Sep 2026 17:16:41 -0400 Subject: [PATCH 4/4] docs: note that rustc normalises the lint-name spelling it then echoes back Review read `RUSTFLAGS="-W unused_results"` as likely to be rejected, on the grounds that command-line lint names use dashes. The command is correct as written -- rustc normalises `_` and `-` in lint names, and the audit figures in this very item were produced by running it -- so nothing changes about the instruction. The premise behind the finding is real, though, and worth heading off. rustc accepts the underscore form and then echoes the HYPHENATED form back: a run of `-W unused_results` reports "requested on the command line with `-W unused-results`". To a reader following the instruction that echo reads like a correction, which is presumably where the finding came from. Recorded as a parenthetical so the next reader is not sent looking for a typo that is not there. Verified on 1.98.0 by invoking rustc directly with each spelling: both are accepted, both warn, neither reports an unknown lint. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CHECKLIST.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/CHECKLIST.md b/CHECKLIST.md index 9d369ceba..be63f3ad9 100644 --- a/CHECKLIST.md +++ b/CHECKLIST.md @@ -147,7 +147,12 @@ written before it was stated. **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`. Measured on `windows-platform-probes`: it flagged + 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.