Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions CHECKLIST.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment thread
Copilot marked this conversation as resolved.
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.
Expand Down
88 changes: 88 additions & 0 deletions DESIGN-NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 id="a-failable-call-has-its-failure-handled-always"></a>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.