From 795f51bd3d884bd5e64b698959b1015131d9b643 Mon Sep 17 00:00:00 2001 From: Mark Gross Date: Fri, 2 Oct 2026 15:37:34 -0700 Subject: [PATCH 1/5] add hresult analyzer writeup --- docs/design/hresult-analyzer.md | 161 ++++++++++++++++++++++++++++++++ 1 file changed, 161 insertions(+) create mode 100644 docs/design/hresult-analyzer.md diff --git a/docs/design/hresult-analyzer.md b/docs/design/hresult-analyzer.md new file mode 100644 index 00000000..d38bb0e0 --- /dev/null +++ b/docs/design/hresult-analyzer.md @@ -0,0 +1,161 @@ +**Analyzer goal:** prevent callers from ignoring failures from CsWin32-generated methods that are marked PreserveSig and return `HRESULT`. +# Scope + +Analyze only CsWin32-generated methods that meet both conditions: + +1. The method is marked PreserveSig. +2. The method returns the CsWin32 `HRESULT` type. +# Sufficient handling + +1. Throw on failure: `obj.DoWork().ThrowOnFailure();` +2. Test success: `if (obj.DoWork().Succeeded) { ... }` + 1. Require `else` block +3. Test failure: `if (obj.DoWork().Failed) return;` +4. Compare with an HRESULT: `if (hr == HRESULT.S_FALSE) { ... }` +5. Switch on meaningful outcomes: `switch (hr) { case HRESULT.S_OK: ... }` +6. Pass to an approved consumer: `HandleHResult(hr);` + 1. **OPEN QUESTION**: Do we want to allow this? Or require handling in the immediate scope of the invocation +7. Suppress intentionally: `#pragma warning disable COMHR001` + 1. **OPEN QUESTION**: Do we want to prohibit this at the static analysis level? Or allow it and ensure authors are sufficiently grilled in code review for including it? + +## Banned handling + +1. Test the numeric value: `if (hr.Value < 0) return;` + 1. Prefer `hr.Failed` +2. `Marshal.ThrowExceptionForHR(hr.Value)` + 1. Prefer `hr.ThrowOnFailure()` + +Handling may occur through a local variable: + +```csharp +HRESULT hr = obj.DoWork(); + +if (hr.Failed) +{ + return; +} +``` +## Switch requirements + + A switch is sufficient only if it distinguishes at least one meaningful HRESULT outcome and has a default arm. For example: + +```csharp +switch (hr) +{ + case HRESULT.S_OK: + UseResult(); + break; + + case HRESULT.S_FALSE: + HandleNoResult(); + break; + + default: + hr.ThrowOnFailure(); + break; +} +``` + +These are insufficient: + +```csharp +switch (hr) +{ + default: + break; +} +``` + +```csharp +switch (hr) +{ + case var result: + Log(result); + break; +} +``` + +The analyzer should require at least one non-catch-all case or guard that tests a specific HRESULT or success/failure category. A `default`, discard, or unguarded variable pattern does not count as inspection. +# Control-flow requirement + + Track each HRESULT through local variables. Every reachable path must perform sufficient handling before the value is lost. + + Report a diagnostic if an unchecked HRESULT: + +1. Is discarded. +2. Is overwritten. +3. Reaches a normal method exit. +4. Is handled on only some reachable paths. +5. Escapes into a field, collection, delegate, or unknown method. + +This is insufficient because the false path does not handle the HRESULT: + +```csharp +HRESULT hr = obj.DoWork(); + +if (condition) +{ + hr.ThrowOnFailure(); +} +``` +# Insufficient handling + +```csharp +obj.DoWork(); +_ = obj.DoWork(); +HRESULT hr = obj.DoWork(); // Never checked +Log(obj.DoWork()); +Console.WriteLine(obj.DoWork()); +int value = obj.DoWork().Value; +this.lastResult = obj.DoWork(); +results.Add(obj.DoWork()); +callback(obj.DoWork()); +``` + +Merely reading, logging, storing, or passing the value to unknown code does not prove that failure was handled. +# Propagation through wrappers: open question + +Consider a wrapper around a CsWin32 method: + +```csharp +HRESULT Initialize() +{ + return native.Initialize(); +} +``` + +Returning the HRESULT transfers responsibility. It does not handle the result. + +If the analyzer checks only direct calls to CsWin32-generated methods, it will not enforce handling at this call site: + +```csharp +Initialize(); +``` + +The design must choose between two policies: + +| Policy | Behavior | Tradeoff | +|---|---|---| +| Immediate-scope handling | Each CsWin32 invocation must be handled in the method containing the invocation. Returning the HRESULT is insufficient. | Simple and enforceable, but prevents transparent HRESULT wrappers. | +| Wrapper propagation | A wrapper can return the HRESULT, and calls to qualifying wrappers are also analyzed. | Supports abstraction, but requires a reliable way to identify wrappers and expands the analyzer beyond direct CsWin32 calls. | +# Diagnostics + +| Rule | Condition | +| ---------- | ------------------------------------------------------- | +| `COMHR001` | HRESULT is directly discarded | +| `COMHR002` | HRESULT is stored locally but not handled on every path | + + `COMHR001` is simple and should have few false positives. `COMHR002` requires control-flow analysis. +# Analysis boundary + +Use local control-flow analysis to track HRESULT values through local variables. + +Do not try to follow values through: + +1. Fields. +2. Collections. +3. Delegates. +4. Reflection. +5. Arbitrary external methods. + +Treat these cases as unchecked escapes unless the destination is an explicitly approved HRESULT consumer. \ No newline at end of file From 4fdc5e13d35824e010e4b51e01f4dc19c1b2fa2f Mon Sep 17 00:00:00 2001 From: Mark Gross Date: Tue, 6 Oct 2026 16:51:19 -0700 Subject: [PATCH 2/5] broaden scope --- docs/design/hresult-analyzer.md | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/docs/design/hresult-analyzer.md b/docs/design/hresult-analyzer.md index d38bb0e0..63bb9d7d 100644 --- a/docs/design/hresult-analyzer.md +++ b/docs/design/hresult-analyzer.md @@ -1,10 +1,9 @@ -**Analyzer goal:** prevent callers from ignoring failures from CsWin32-generated methods that are marked PreserveSig and return `HRESULT`. +**Analyzer goal:** prevent callers from accidentally ignoring the result of any method that returns the CsWin32 `HRESULT` type. # Scope -Analyze only CsWin32-generated methods that meet both conditions: +Analyze every method invocation whose compile-time return type is the CsWin32 `HRESULT` type. -1. The method is marked PreserveSig. -2. The method returns 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`. # Sufficient handling 1. Throw on failure: `obj.DoWork().ThrowOnFailure();` @@ -113,7 +112,7 @@ callback(obj.DoWork()); ``` Merely reading, logging, storing, or passing the value to unknown code does not prove that failure was handled. -# Propagation through wrappers: open question +# Propagation through wrappers Consider a wrapper around a CsWin32 method: @@ -126,18 +125,13 @@ HRESULT Initialize() Returning the HRESULT transfers responsibility. It does not handle the result. -If the analyzer checks only direct calls to CsWin32-generated methods, it will not enforce handling at this call site: +Because the analyzer checks every method that returns `HRESULT`, it also enforces handling at this call site: ```csharp Initialize(); ``` -The design must choose between two policies: - -| Policy | Behavior | Tradeoff | -|---|---|---| -| Immediate-scope handling | Each CsWin32 invocation must be handled in the method containing the invocation. Returning the HRESULT is insufficient. | Simple and enforceable, but prevents transparent HRESULT wrappers. | -| Wrapper propagation | A wrapper can return the HRESULT, and calls to qualifying wrappers are also analyzed. | Supports abstraction, but requires a reliable way to identify wrappers and expands the analyzer beyond direct CsWin32 calls. | +This supports transparent HRESULT wrappers without requiring the analyzer to identify them through interprocedural analysis. The wrapper invocation is in scope because its declared return type is `HRESULT`. # Diagnostics | Rule | Condition | @@ -148,7 +142,7 @@ The design must choose between two policies: `COMHR001` is simple and should have few false positives. `COMHR002` requires control-flow analysis. # Analysis boundary -Use local control-flow analysis to track HRESULT values through local variables. +Start analysis at every invocation whose compile-time return type is the CsWin32 `HRESULT` type. Use local control-flow analysis to track HRESULT values through local variables. Do not try to follow values through: From ca3a57effc38ad36a62e30ce52320c720e7f1f96 Mon Sep 17 00:00:00 2001 From: Mark Gross Date: Tue, 6 Oct 2026 16:57:19 -0700 Subject: [PATCH 3/5] allow explicit discard --- docs/design/hresult-analyzer.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/design/hresult-analyzer.md b/docs/design/hresult-analyzer.md index 63bb9d7d..03ad08a4 100644 --- a/docs/design/hresult-analyzer.md +++ b/docs/design/hresult-analyzer.md @@ -12,9 +12,10 @@ The method does not need to be generated by CsWin32 or marked PreserveSig. This 3. Test failure: `if (obj.DoWork().Failed) return;` 4. Compare with an HRESULT: `if (hr == HRESULT.S_FALSE) { ... }` 5. Switch on meaningful outcomes: `switch (hr) { case HRESULT.S_OK: ... }` -6. Pass to an approved consumer: `HandleHResult(hr);` +6. Explicitly discard the result: `_ = obj.DoWork();` +7. Pass to an approved consumer: `HandleHResult(hr);` 1. **OPEN QUESTION**: Do we want to allow this? Or require handling in the immediate scope of the invocation -7. Suppress intentionally: `#pragma warning disable COMHR001` +8. Suppress intentionally: `#pragma warning disable COMHR001` 1. **OPEN QUESTION**: Do we want to prohibit this at the static analysis level? Or allow it and ensure authors are sufficiently grilled in code review for including it? ## Banned handling @@ -81,7 +82,7 @@ The analyzer should require at least one non-catch-all case or guard that tests Report a diagnostic if an unchecked HRESULT: -1. Is discarded. +1. Is ignored as an unused invocation result. 2. Is overwritten. 3. Reaches a normal method exit. 4. Is handled on only some reachable paths. @@ -101,7 +102,6 @@ if (condition) ```csharp obj.DoWork(); -_ = obj.DoWork(); HRESULT hr = obj.DoWork(); // Never checked Log(obj.DoWork()); Console.WriteLine(obj.DoWork()); @@ -136,7 +136,7 @@ This supports transparent HRESULT wrappers without requiring the analyzer to ide | Rule | Condition | | ---------- | ------------------------------------------------------- | -| `COMHR001` | HRESULT is directly discarded | +| `COMHR001` | HRESULT invocation result is ignored | | `COMHR002` | HRESULT is stored locally but not handled on every path | `COMHR001` is simple and should have few false positives. `COMHR002` requires control-flow analysis. From 908392acdde41faeda4ecc71a39fbb86ab624139 Mon Sep 17 00:00:00 2001 From: Mark Gross Date: Tue, 6 Oct 2026 16:58:41 -0700 Subject: [PATCH 4/5] remove pragma warning disable suggestion --- docs/design/hresult-analyzer.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/docs/design/hresult-analyzer.md b/docs/design/hresult-analyzer.md index 03ad08a4..3a222410 100644 --- a/docs/design/hresult-analyzer.md +++ b/docs/design/hresult-analyzer.md @@ -15,8 +15,6 @@ The method does not need to be generated by CsWin32 or marked PreserveSig. This 6. Explicitly discard the result: `_ = obj.DoWork();` 7. Pass to an approved consumer: `HandleHResult(hr);` 1. **OPEN QUESTION**: Do we want to allow this? Or require handling in the immediate scope of the invocation -8. Suppress intentionally: `#pragma warning disable COMHR001` - 1. **OPEN QUESTION**: Do we want to prohibit this at the static analysis level? Or allow it and ensure authors are sufficiently grilled in code review for including it? ## Banned handling From cf6d8c58f76d12fd03fd28c571b35097451e26e7 Mon Sep 17 00:00:00 2001 From: Mark Gross Date: Wed, 7 Oct 2026 14:51:04 -0700 Subject: [PATCH 5/5] handful of revisions --- docs/design/hresult-analyzer.md | 102 +++++++++++--------------------- 1 file changed, 36 insertions(+), 66 deletions(-) diff --git a/docs/design/hresult-analyzer.md b/docs/design/hresult-analyzer.md index 3a222410..0418c21e 100644 --- a/docs/design/hresult-analyzer.md +++ b/docs/design/hresult-analyzer.md @@ -4,26 +4,28 @@ 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`. -# Sufficient handling +# Recognized consumption 1. Throw on failure: `obj.DoWork().ThrowOnFailure();` 2. Test success: `if (obj.DoWork().Succeeded) { ... }` - 1. Require `else` block 3. Test failure: `if (obj.DoWork().Failed) return;` 4. Compare with an HRESULT: `if (hr == HRESULT.S_FALSE) { ... }` -5. Switch on meaningful outcomes: `switch (hr) { case HRESULT.S_OK: ... }` +5. Switch on outcomes: `switch (hr) { case HRESULT.S_OK: ... }` 6. Explicitly discard the result: `_ = obj.DoWork();` -7. Pass to an approved consumer: `HandleHResult(hr);` - 1. **OPEN QUESTION**: Do we want to allow this? Or require handling in the immediate scope of the invocation +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();` -## Banned handling +`COMHR001` is conservative. It accepts any explicit use of the result because the caller did not accidentally ignore it. -1. Test the numeric value: `if (hr.Value < 0) return;` - 1. Prefer `hr.Failed` -2. `Marshal.ThrowExceptionForHR(hr.Value)` - 1. Prefer `hr.ThrowOnFailure()` +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`. -Handling may occur through a local variable: +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(); @@ -35,7 +37,7 @@ if (hr.Failed) ``` ## Switch requirements - A switch is sufficient only if it distinguishes at least one meaningful HRESULT outcome and has a default arm. For example: +Any switch that consumes the `HRESULT` counts as intentional use. For example: ```csharp switch (hr) @@ -54,39 +56,12 @@ switch (hr) } ``` -These are insufficient: - -```csharp -switch (hr) -{ - default: - break; -} -``` - -```csharp -switch (hr) -{ - case var result: - Log(result); - break; -} -``` - -The analyzer should require at least one non-catch-all case or guard that tests a specific HRESULT or success/failure category. A `default`, discard, or unguarded variable pattern does not count as inspection. +`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 perform sufficient handling before the value is lost. - - Report a diagnostic if an unchecked HRESULT: - -1. Is ignored as an unused invocation result. -2. Is overwritten. -3. Reaches a normal method exit. -4. Is handled on only some reachable paths. -5. Escapes into a field, collection, delegate, or unknown method. +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 handle the HRESULT: +This is insufficient because the false path does not consume the HRESULT: ```csharp HRESULT hr = obj.DoWork(); @@ -96,20 +71,26 @@ 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 -Log(obj.DoWork()); -Console.WriteLine(obj.DoWork()); -int value = obj.DoWork().Value; -this.lastResult = obj.DoWork(); -results.Add(obj.DoWork()); -callback(obj.DoWork()); +int value = obj.DoWork().Value; // Never used ``` -Merely reading, logging, storing, or passing the value to unknown code does not prove that failure was handled. +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: @@ -129,25 +110,14 @@ Because the analyzer checks every method that returns `HRESULT`, it also enforce Initialize(); ``` -This supports transparent HRESULT wrappers without requiring the analyzer to identify them through interprocedural analysis. The wrapper invocation is in scope because its declared return type is `HRESULT`. +This supports transparent HRESULT wrappers without requiring explicit annotation. # Diagnostics -| Rule | Condition | -| ---------- | ------------------------------------------------------- | -| `COMHR001` | HRESULT invocation result is ignored | -| `COMHR002` | HRESULT is stored locally but not handled on every path | - - `COMHR001` is simple and should have few false positives. `COMHR002` requires control-flow analysis. +| 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. Use local control-flow analysis to track HRESULT values through local variables. - -Do not try to follow values through: - -1. Fields. -2. Collections. -3. Delegates. -4. Reflection. -5. Arbitrary external methods. +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. -Treat these cases as unchecked escapes unless the destination is an explicitly approved HRESULT consumer. \ No newline at end of file +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