diff --git a/docs/design/hresult-analyzer.md b/docs/design/hresult-analyzer.md new file mode 100644 index 00000000..0418c21e --- /dev/null +++ b/docs/design/hresult-analyzer.md @@ -0,0 +1,123 @@ +**Analyzer goal:** prevent callers from accidentally ignoring the result of any method that returns the CsWin32 `HRESULT` type. +# Scope + +Analyze every method invocation whose compile-time return type is the CsWin32 `HRESULT` type. + +The method does not need to be generated by CsWin32 or marked PreserveSig. This includes helper and wrapper methods that return an `HRESULT`. +# Recognized consumption + +1. Throw on failure: `obj.DoWork().ThrowOnFailure();` +2. Test success: `if (obj.DoWork().Succeeded) { ... }` +3. Test failure: `if (obj.DoWork().Failed) return;` +4. Compare with an HRESULT: `if (hr == HRESULT.S_FALSE) { ... }` +5. Switch on outcomes: `switch (hr) { case HRESULT.S_OK: ... }` +6. Explicitly discard the result: `_ = obj.DoWork();` +7. Pass the result to another method: `Log(obj.DoWork());` +8. Store the result in a field or collection: `this.lastResult = obj.DoWork();` +9. Return the result to the caller: `return obj.DoWork();` + +`COMHR001` is conservative. It accepts any explicit use of the result because the caller did not accidentally ignore it. + +Reading `HRESULT.Value` does not by itself count as use. It transfers the tracking obligation to the resulting integer. Storing that integer in a local variable is insufficient until the value is read or otherwise used according to the same rules as an `HRESULT`. + +Some accepted uses have clearer alternatives: + +1. Prefer `hr.Failed` to `hr.Value < 0`. +2. Prefer `hr.ThrowOnFailure()` to `Marshal.ThrowExceptionForHR(hr.Value)`. + +Use may occur through a local variable: + +```csharp +HRESULT hr = obj.DoWork(); + +if (hr.Failed) +{ + return; +} +``` +## Switch requirements + +Any switch that consumes the `HRESULT` counts as intentional use. For example: + +```csharp +switch (hr) +{ + case HRESULT.S_OK: + UseResult(); + break; + + case HRESULT.S_FALSE: + HandleNoResult(); + break; + + default: + hr.ThrowOnFailure(); + break; +} +``` + +`COMHR001` does not require the switch to distinguish a specific HRESULT or success/failure category. This is intentional to avoid excessively restricting handling patterns while still preventing consumers from accidentally ignoring HRESULTs. +# Control-flow requirement + +Track each `HRESULT` through local variables. Every reachable path must consume the value before it is overwritten or the method exits. + +This is insufficient because the false path does not consume the HRESULT: + +```csharp +HRESULT hr = obj.DoWork(); + +if (condition) +{ + hr.ThrowOnFailure(); +} +``` + +# Diagnostic conditions + +Report `COMHR001` if an HRESULT: + +1. Is ignored as an unused invocation result. +2. Is assigned to a local that is never read. +3. Is overwritten before its value is read. +4. Has its `Value` stored in a local that is never read. +5. Reaches a normal method exit without being consumed. +6. Is not consumed on all reachable paths. +# Insufficient handling + +```csharp +obj.DoWork(); +HRESULT hr = obj.DoWork(); // Never checked +int value = obj.DoWork().Value; // Never used +``` + +Logging, storing outside a local, returning, or passing the HRESULT or its numeric value counts as intentional use and does not produce `COMHR001`. +# Propagation through wrappers + +Consider a wrapper around a CsWin32 method: + +```csharp +HRESULT Initialize() +{ + return native.Initialize(); +} +``` + +Returning the HRESULT transfers responsibility. It does not handle the result. + +Because the analyzer checks every method that returns `HRESULT`, it also enforces handling at this call site: + +```csharp +Initialize(); +``` + +This supports transparent HRESULT wrappers without requiring explicit annotation. +# Diagnostics + +| Rule | Condition | +| ---------- | ------------------------------------------------- | +| `COMHR001` | HRESULT invocation result is accidentally ignored | +# Analysis boundary + +Start analysis at every invocation whose compile-time return type is the CsWin32 `HRESULT` type. Track the value through local variables and control flow until every reachable path consumes it, overwrites it, or reaches a method exit. + +When analysis reaches an `HRESULT.Value` access, continue tracking the resulting integer. Apply the same control-flow and consumption rules to that integer as to the original `HRESULT`. \ No newline at end of file