Skip to content

Commit 1664b41

Browse files
authored
Merge pull request #235 from PhysShell/claude/precision-remeasure-note
docs: re-measure #218/#220/#222/#223/#224 delta, verify #225/#228 on real repos
2 parents d49ffa8 + c681960 commit 1664b41

1 file changed

Lines changed: 215 additions & 0 deletions

File tree

Lines changed: 215 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,215 @@
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 \#201),
4+
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

Comments
 (0)