|
| 1 | +# Precision re-measure — 2026-07-11 (verifying #218/#220/#222/#223/#224 and #225/#228) |
| 2 | + |
| 3 | +Follow-up to [`oracle-sweep-2026-07-10.md`](oracle-sweep-2026-07-10.md) (issue |
| 4 | +#201), which drafted 8 precision-gap issues from real own-only findings in 5 |
| 5 | +OSS repos: #218-#225. Four follow-up PRs have since shipped fixes: |
| 6 | + |
| 7 | +- [#230](https://github.com/PhysShell/Own.NET/pull/230) — Closes **#218** |
| 8 | + (not #219 — #219 is the separate WinForms `Controls`/`IContainer` disposal |
| 9 | + channel gap, still open). DependencyProperty/property-changed old→new |
| 10 | + subscription-rotation recognition. |
| 11 | +- [#231](https://github.com/PhysShell/Own.NET/pull/231) — four smaller gaps |
| 12 | + in one PR: `CommandManager.RequerySuggested` allowlist (#223), |
| 13 | + `using (field = new T())` release (#220), template-part local/pattern-var |
| 14 | + exemption (#222), self-detaching handler (#224). |
| 15 | +- [#232](https://github.com/PhysShell/Own.NET/pull/232) — Closes **#228** |
| 16 | + (sub-part (b) of the parent #221 split), curated app-scoped source |
| 17 | + exemption for an `Application`-derived subscriber. |
| 18 | +- [#233](https://github.com/PhysShell/Own.NET/pull/233) — Closes **#225**, |
| 19 | + exempt locals of user types whose `Dispose()` is provably empty in source. |
| 20 | + |
| 21 | +This note has two parts: **Part 1** formalizes yesterday's measurement of |
| 22 | +#230/#231 against the 2026-07-10 baseline (same method as before, now with |
| 23 | +the #218/#219 numbering corrected). **Part 2** reruns the same method against |
| 24 | +today's fresh `main` (with #232/#233 also merged) for the two repos those |
| 25 | +fixes target, and checks the actual delta against the expected delta each |
| 26 | +PR's own "validation plan" section predicted. |
| 27 | + |
| 28 | +## Method (unchanged from the 2026-07-10 rerun) |
| 29 | + |
| 30 | +- Same repo clones, same pinned commits as the original sweep: MahApps.Metro |
| 31 | + `72099e3`, MaterialDesignInXamlToolkit `ef3a5ea`, AvalonEdit `ed0bd14`, |
| 32 | + ShareX `0df9ca4`, ClosedXML `4e89dce` — no drift from upstream, an |
| 33 | + apples-to-apples comparison. |
| 34 | +- `OwnSharp.Extractor` built fresh off the `main` commit under test. |
| 35 | +- `scripts/own-check.sh --format sarif --severity warning -- <repo>`, same |
| 36 | + `OWN_EXTRA_REF_DIRS` (WindowsDesktop ref pack) as `oracle.yml` uses. |
| 37 | +- SARIF results diffed by normalized `(file, line)` per rule ID between the |
| 38 | + "before" and "after" runs. |
| 39 | + |
| 40 | +## Part 1 — #218/#220/#222/#223/#224 delta (`main` at `3ca4bde`, #230+#231 merged) |
| 41 | + |
| 42 | +| repo | OWN001+OWN014 before | after | Δ | new findings introduced | |
| 43 | +|---|---:|---:|---:|---:| |
| 44 | +| MahApps.Metro | 3 | 1 | **−2** | 0 | |
| 45 | +| MaterialDesignInXamlToolkit | 23 | 19 | **−4** | 0 | |
| 46 | +| AvalonEdit | 24 (23 OWN001 + 1 OWN014) | 17 | **−7** | 0 | |
| 47 | +| ShareX | 235 | 232 | **−3** | 0 | |
| 48 | +| **total** | **285** | **269** | **−16** | **0** | |
| 49 | + |
| 50 | +Zero new findings anywhere. OWN050 (unresolved-reference advisory) counts are |
| 51 | +byte-for-byte identical before/after in all 4 repos — coverage didn't shift, |
| 52 | +only the fixes' targets did. |
| 53 | + |
| 54 | +### Removed findings, mapped to the fix that closed them |
| 55 | + |
| 56 | +| repo | site | issue / PR | rule | |
| 57 | +|---|---|---|---| |
| 58 | +| MahApps.Metro | `Actions/CommandTriggerAction.cs:116` | **#218** / #230 (DP rotation) | OWN001 | |
| 59 | +| MahApps.Metro | `Controls/MetroWindow.cs:1448` | #222 / #231 (template-part pattern var) | OWN001 | |
| 60 | +| MaterialDesignInXamlToolkit | `SmartHint.cs:205-208` (×4) | **#218** / #230 (DP rotation) | OWN001 | |
| 61 | +| AvalonEdit | `Editing/AbstractMargin.cs:99` | **#218** / #230 (DP rotation) | OWN001 | |
| 62 | +| AvalonEdit | `Editing/LineNumberMargin.cs:114` | **#218** / #230 (DP rotation) | OWN001 | |
| 63 | +| AvalonEdit | `Folding/FoldingMargin.cs:218` | **#218** / #230 (DP rotation) | OWN001 | |
| 64 | +| AvalonEdit | `CodeCompletion/OverloadViewer.cs:58,64` | #222 / #231 (template-part local) | OWN001 | |
| 65 | +| AvalonEdit | `Search/DropDownButton.cs:78` | #224 / #231 (self-detaching handler) | OWN001 | |
| 66 | +| AvalonEdit | `Editing/ImeSupport.cs:47` (`CommandManager.RequerySuggested`) | #223 / #231 (weak-event allowlist) | **OWN014** | |
| 67 | +| ShareX | `Cryptographic/HashChecker.cs:42` | #220 / #231 (`using (field = new T())`) | OWN001 | |
| 68 | +| ShareX | `TaskEx.cs:41` | #220 / #231 | OWN001 | |
| 69 | +| ShareX | `IndexerJson.cs:35` | #220 / #231 | OWN001 | |
| 70 | + |
| 71 | +By fix: rotation (**#218**/#230) accounts for 8 of the 16 removed findings — |
| 72 | +beating the "−6+" estimate from the original sweep note (which only |
| 73 | +individually verified 6 sites; the rerun confirms all 8 rotation sites in |
| 74 | +the baseline actually clear). The other three #231 fixes account for the |
| 75 | +remaining 8: template-part locals (3), self-detach (1), weak-event allowlist |
| 76 | +(1, on the OWN014 hard-error tier as designed), using-field release (3). |
| 77 | + |
| 78 | +### Real bugs — confirmed still flagged |
| 79 | + |
| 80 | +ShareX `HistoryLib/HistoryItemManager_ContextMenu.cs` cluster (48 findings — |
| 81 | +no `Dispose()` at all), ShareX `ShapeManagerMenu.cs` cluster (the flagship |
| 82 | +`menuForm` leak), ShareX plain `Timer`/`ImageList` fields |
| 83 | +(`ColorUserControl.cs`, `AutoCaptureForm.cs`, `UploadersConfigForm.cs`), and |
| 84 | +AvalonEdit `TextView.cs:1843,1946` — all unchanged, still flagged. |
| 85 | + |
| 86 | +### Still open (out of #230/#231's scope) |
| 87 | + |
| 88 | +MahApps.Metro `Behaviors/TiltBehavior.cs:70` (self-owned `Behavior<T>` |
| 89 | +source, #227) and MaterialDesignInXamlToolkit's remaining 19 findings — |
| 90 | +`App.xaml.cs:22`, `ListsAndGridsViewModel.cs` (#229), `CircleWipe.cs`/ |
| 91 | +`FadeWipe.cs`, the 7 bulk-review `themeManager.ThemeChanged` sites, etc. — |
| 92 | +untouched, as expected. |
| 93 | + |
| 94 | +## Expected delta of the next measurement (before rerunning) |
| 95 | + |
| 96 | +Predicted from #232's and #233's own PR bodies, before this note's Part 2 |
| 97 | +below actually reran anything: |
| 98 | + |
| 99 | +- **After #232 (Closes #228):** MaterialDesignInXamlToolkit |
| 100 | + `MahMaterialDragablzMashUp/App.xaml.cs:10,22` (`themeManager.ThemeChanged` |
| 101 | + on the `App`-derived subscriber) should drop out of own-only. |
| 102 | +- **After #233 (Closes #225):** ClosedXML `Excel/Cells/Slice.cs:91,109,149,188,219` |
| 103 | + — five `Slice.Enumerator`/`ReverseEnumerator` locals, all sharing the |
| 104 | + same empty-`Dispose()` root — should drop out of own-only. |
| 105 | +- **Not yet fixed (#227, #229 — still open):** MahApps.Metro |
| 106 | + `Behaviors/TiltBehavior.cs:70` and MaterialDesignInXamlToolkit |
| 107 | + `ListsAndGridsViewModel.cs:16-17` (both demo variants) should remain |
| 108 | + flagged. |
| 109 | + |
| 110 | +## Part 2 — actual #232/#233 delta (`main` at `b1ee961`, #232+#233 also merged) |
| 111 | + |
| 112 | +| repo | before (after #230/#231) | after (+ #232/#233) | Δ | expected Δ | |
| 113 | +|---|---:|---:|---:|---:| |
| 114 | +| MaterialDesignInXamlToolkit | 19 | 18 | **−1** | −1 (1 site) | |
| 115 | +| ClosedXML | 270 | 5 | **−265** | −5 (1 site's 5 locals) | |
| 116 | + |
| 117 | +### MaterialDesignInXamlToolkit — matches expectation |
| 118 | + |
| 119 | +`MahMaterialDragablzMashUp/App.xaml.cs:22` (the only SARIF-reportable line |
| 120 | +for that site — line 10 in the issue's citation is the `PaletteHelper()` |
| 121 | +resolver-call context line, not a second finding) dropped out, exactly as |
| 122 | +predicted. No other site moved. **Match.** |
| 123 | + |
| 124 | +### ClosedXML — does NOT match expectation, and not in the way "more FPs cleared than expected" sounds |
| 125 | + |
| 126 | +The raw delta (−265) looks like a huge overachievement of the predicted −5, |
| 127 | +but breaking it down site-by-site shows it is **not** a clean superset — |
| 128 | +part of it is a real coverage gap, and the bulk of it is a genuine, unsound |
| 129 | +over-exemption that needs its own follow-up issue. Do not read "−265" as |
| 130 | +"the fix worked great"; read the breakdown below. |
| 131 | + |
| 132 | +**1) Only 2 of the 5 predicted Slice.cs sites actually cleared:** |
| 133 | + |
| 134 | +| line | local | enumerator type | `Dispose()` form | cleared? | |
| 135 | +|---:|---|---|---|:---:| |
| 136 | +| 91 | `enumerator` | `Enumerator` | `void IDisposable.Dispose() { }` — **explicit interface implementation** | ❌ still flagged | |
| 137 | +| 109 | `cellEnumerator` | `Enumerator` | same explicit-interface form | ❌ still flagged | |
| 138 | +| 149 | `cellEnumerator` | `Enumerator` | same explicit-interface form | ❌ still flagged | |
| 139 | +| 188 | `cellEnumerator` | `ReverseEnumerator` | `public void Dispose() { }` — plain method | ✅ cleared | |
| 140 | +| 219 | `enumerator` | `ReverseEnumerator` | same plain-method form | ✅ cleared | |
| 141 | + |
| 142 | +`HasEmptyDisposeBody` (the check #233 shipped) recognizes a plain |
| 143 | +`public void Dispose() { }` declaration but not an explicit interface |
| 144 | +implementation (`void IDisposable.Dispose() { }`) — same empty body, |
| 145 | +different declaration syntax. This is a **coverage gap** (the check is |
| 146 | +too narrow, not unsound): `Enumerator` and `ReverseEnumerator` sit right |
| 147 | +next to each other in the same file with the identical empty body, and |
| 148 | +only one form is recognized. |
| 149 | + |
| 150 | +**2) The other 263 removed findings are an unrelated, unsound over-exemption:** |
| 151 | +every one of them is a `wb`/`workbook`/`wb1`/`wb2`/`wb_saved`/`wbSource` |
| 152 | +local of type `XLWorkbook`, scattered across `ClosedXML.Examples` (263 |
| 153 | +call sites in total) — nothing to do with `Slice.cs` at all. The root |
| 154 | +cause: `ClosedXML/Excel/XLWorkbook.cs:874` |
| 155 | + |
| 156 | +```csharp |
| 157 | +public void Dispose() |
| 158 | +{ |
| 159 | + // Leave this empty so that Janitor.Fody can do its work |
| 160 | +} |
| 161 | +``` |
| 162 | + |
| 163 | +is empty **in source only**. `Janitor.Fody` is a build-time IL weaver that |
| 164 | +rewrites this exact method body at compile time to call the adjacent |
| 165 | +`DisposeManaged()` (`XLWorkbook.cs:868`, which does real cleanup — |
| 166 | +`Worksheets.ForEach(w => (w as XLWorksheet).Cleanup())`). A Roslyn-source |
| 167 | +analyzer has no visibility into post-compile IL weaving, so |
| 168 | +`HasEmptyDisposeBody`'s "zero-statement block ⇒ no-op" heuristic reads the |
| 169 | +comment-explained placeholder as a genuine no-op and exempts every |
| 170 | +`XLWorkbook` local in the compilation — even though disposing one |
| 171 | +(pre-weaving semantics aside) is exactly what real ClosedXML code is |
| 172 | +supposed to do, and never calling it is a real, if fairly benign |
| 173 | +(process-exit-scoped example code), leak. |
| 174 | + |
| 175 | +This is **not** what #233 set out to do (#233's own PR body scoped the fix |
| 176 | +to "only locals," and its acceptance checklist's regression diff was run |
| 177 | +against the existing sample corpus, which has no Fody-woven-Dispose type in |
| 178 | +it — so this shape never showed up in that PR's own local verification). |
| 179 | +It's a real precision regression surfaced only by running against actual |
| 180 | +OSS code that uses a code-weaving library, exactly the kind of gap a |
| 181 | +synthetic-sample-only regression suite can't catch. |
| 182 | + |
| 183 | +**Confirmed unaffected (real findings, unrelated to this fix):** |
| 184 | +`ClosedXML.Tests/Excel/CalcEngine/FunctionsTests.cs:285` (`cts`, a genuine |
| 185 | +undisposed `CancellationTokenSource`) and |
| 186 | +`ClosedXML.Tests/Excel/Ranges/UsedAndUnusedCellsTests.cs:10` (`workbook` |
| 187 | +field, real) — both still flagged, so the over-exemption is scoped to |
| 188 | +`XLWorkbook` **locals** specifically (matches #233's own "only locals, not |
| 189 | +fields" boundary), not a wholesale collapse of ClosedXML's findings. |
| 190 | + |
| 191 | +### Net read for the next iteration (analyzer code NOT touched in this note) |
| 192 | + |
| 193 | +Two separate, differently-shaped follow-ups for whoever picks this up next: |
| 194 | + |
| 195 | +1. **Coverage gap** — extend `HasEmptyDisposeBody` to also recognize an |
| 196 | + empty **explicit interface implementation** `Dispose()` |
| 197 | + (`void IDisposable.Dispose() { }`), not just a plain public method |
| 198 | + declaration. Closes the remaining 3/5 Slice.cs sites (91, 109, 149). |
| 199 | +2. **Soundness regression** — `HasEmptyDisposeBody` needs to stop treating |
| 200 | + a literally-empty `Dispose()` as proof of "nothing |
| 201 | + to release" when IL-weaving tools (Janitor.Fody being the concrete |
| 202 | + instance found here; PostSharp and other Fody plugins follow the same |
| 203 | + shape) are in play. A cheap, conservative signal: if the type or any |
| 204 | + partial-class fragment also declares a private method whose body the |
| 205 | + weaver targets by convention (Janitor.Fody's own convention is a method |
| 206 | + named `Dispose(Managed|Unmanaged)`/`DisposeManaged`/`DisposeUnmanaged` |
| 207 | + living alongside the empty `Dispose()`), don't exempt — treat it as |
| 208 | + "provably non-empty at runtime" and fall back to today's flagged |
| 209 | + behavior. Whether that specific heuristic is the right one is a design |
| 210 | + call for whoever picks up the fix, not decided here. |
| 211 | + |
| 212 | +This note's own instructions were read-only with respect to the analyzer — |
| 213 | +no code was changed to investigate or produce the breakdown above; the |
| 214 | +breakdown is entirely from comparing old/new SARIF output and reading the |
| 215 | +target repos' real source. |
0 commit comments