From f5d7c6784fdc9ae98c682fee577e5f3c610c6307 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 25 Jun 2026 16:09:43 +0000 Subject: [PATCH 1/3] feat(own): T4 fixer handles the disposable-field shape Extends the OWN fixer (PR #2) to a second leak shape: an IDisposable field that is never disposed (Timer, CancellationTokenSource, ...). On a WPF owner it disposes the field on the teardown event, reusing the same Window->Closed / FrameworkElement-> Unloaded selection as the subscription detach: public ShareWindow() { InitializeComponent(); + this.Closed += (s, e) => _timer?.Dispose(); The dispose hook is anchored after the ctor's InitializeComponent() (in scope for 'this'). Honesty boundary kept: disposable-LOCAL stays suggest-only (needs a scoped using), as does the inline-lambda subscription. classify() now also returns the field shape; plan_file dispatches per shape to the right anchor + teardown statement. Fixture: ShareWindow._timer (real STS site). +2 tests; 13/13 own + 7/7 wrapper, incl -O. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoa --- docs/fix-arm.md | 16 ++-- fix/README.md | 21 +++-- fix/fixarm/own_fix.py | 93 +++++++++++++------ .../after.findings.json | 3 + .../before.findings.json | 12 +++ .../before/Broker/ShareWindow.xaml.cs | 17 ++++ fix/tests/test_own_fix.py | 21 ++++- 7 files changed, 140 insertions(+), 43 deletions(-) create mode 100644 fix/fixtures/own001-disposable-field/after.findings.json create mode 100644 fix/fixtures/own001-disposable-field/before.findings.json create mode 100644 fix/fixtures/own001-disposable-field/before/Broker/ShareWindow.xaml.cs diff --git a/docs/fix-arm.md b/docs/fix-arm.md index c87cfd4..a11367e 100644 --- a/docs/fix-arm.md +++ b/docs/fix-arm.md @@ -115,13 +115,15 @@ No step is AI-judged. The asserts are exact, mirroring Arm 2's scenario discipli Build-free, develops and unit-tests on Linux against synthetic fixtures (Roslyn and both CLIs are cross-platform). - **Build — the OWN fixer** (T4): `OWN001`/`OWN014`. Structural, build-free, reviewable - patches — built in [`../fix/fixarm/own_fix.py`](../fix/fixarm/own_fix.py). First slice - fixes the **named-handler subscription** shape by inserting a teardown detach - (`Window` → `Closed`, `FrameworkElement` → `Unloaded`); fixtures are real STS sites - (`AmountWindow` OWN001, `KTSGoods2` OWN014). The **inline-lambda** shape is classified - **suggest-only** and never patched — own-check itself flags it has "no `-=` handle, so - it could never be detached", so it needs lambda extraction first. Still to build: - disposable-field/local shapes, lambda extraction, and folding into an existing + patches — built in [`../fix/fixarm/own_fix.py`](../fix/fixarm/own_fix.py). On a WPF + owner it hangs cleanup on a teardown event (`Window` → `Closed`, `FrameworkElement` → + `Unloaded`) and fixes two shapes: the **named-handler subscription** (insert + `src.Event -= Handler`) and the **disposable field** (insert `field?.Dispose()`, + anchored after the ctor's `InitializeComponent()`). Fixtures are real STS sites + (`AmountWindow` OWN001, `KTSGoods2` OWN014, `ShareWindow._timer`). The **inline-lambda** + subscription and the **disposable-local** are classified **suggest-only** and never + patched (lambda needs extraction; a local needs a scoped `using`). Still to build: + disposable-local → `using`, lambda extraction, folding into an existing `OnClosed`/`Dispose`. Every OWN result is queued-for-review (T4), never auto. --- diff --git a/fix/README.md b/fix/README.md index 76336b2..d40488a 100644 --- a/fix/README.md +++ b/fix/README.md @@ -53,21 +53,28 @@ PYTHONPATH=fix python3 -m fixarm.cli --fixture fix/fixtures/own001-sub-window \ --rule OWN001 --applier own --show-diff ``` -This slice fixes the **named-handler subscription** shape by inserting a teardown -detach (`Window` → `Closed`, `FrameworkElement` → `Unloaded`): +It fixes two shapes on a WPF owner by hanging cleanup on a teardown event +(`Window` → `Closed`, `FrameworkElement` → `Unloaded`): ```diff + // named-handler subscription fGoods.PropertyChanged += new PropertyChangedEventHandler(GoodsPropertyChanged); + this.Closed += (s, e) => fGoods.PropertyChanged -= new PropertyChangedEventHandler(GoodsPropertyChanged); + + // disposable field (Timer / CancellationTokenSource / …), anchored after the ctor + public ShareWindow() { + InitializeComponent(); ++ this.Closed += (s, e) => _timer?.Dispose(); ``` -It **refuses** the inline-lambda shape (own-check: "no `-=` handle … could never be -detached") — a lambda must be extracted to a named handler first, so it's classified -suggest-only and surfaced in `applier.skipped`, never patched with a fake fix. +It **refuses** the inline-lambda subscription (own-check: "no `-=` handle … could never +be detached" → needs extraction first) and the disposable-**local** (needs a scoped +`using`) — both are classified suggest-only and surfaced in `applier.skipped`, never +patched with a fake fix. ## Next - Promote proven-mechanical rules into `tiers._T1_RULES` (auto-commit) from real diffs. -- OWN fixer: handle disposable-field/local shapes; lambda **extraction** then detach; - consolidate into an existing `OnClosed`/`Dispose` override when one is present. +- OWN fixer: disposable-**local** → scoped `using`; inline-lambda **extraction** then + detach; consolidate into an existing `OnClosed`/`Dispose` override when one is present. - Windows-bound fix-spike: does `roslynator fix` load `Broker.sln` (docs/fix-arm.md §6). diff --git a/fix/fixarm/own_fix.py b/fix/fixarm/own_fix.py index a904f08..c858532 100644 --- a/fix/fixarm/own_fix.py +++ b/fix/fixarm/own_fix.py @@ -6,18 +6,18 @@ OWN rules are tier T4 → the wrapper always routes the result to REVIEW; nothing here auto-commits, because lifetime-correct teardown placement is a judgement call. -Scope of THIS slice — honest about the boundary: - * FIXES the named-handler subscription shape - src.Event += Handler; (Handler a method group or `new D(M)`) - by inserting a teardown detach next to it: - this. += (s, e) => src.Event -= Handler; - Closed for a Window, Unloaded for a FrameworkElement; anything else is left - for review (we can't pick a safe teardown blind). - * REFUSES the inline-lambda shape - src.Event += (s, e) => ...; - own-check itself says it "has no '-=' handle, so it could never be detached". - A lambda must be extracted to a named handler FIRST, which is a real refactor — - so we classify it suggest-only and never emit a patch that pretends to fix it. +Scope of THIS slice — honest about the boundary. For a WPF owner we hang the cleanup +on a teardown event (Closed for a Window, Unloaded for a FrameworkElement); anything +else is left for review (we can't pick a safe teardown blind). + * FIXES the named-handler subscription shape — `src.Event += Handler;` (method group + or `new D(M)`) → `this. += (s, e) => src.Event -= Handler;` + * FIXES the disposable-field shape — an IDisposable field never disposed (a Timer, + CancellationTokenSource, …) → `this. += (s, e) => field?.Dispose();`, + anchored after the ctor's InitializeComponent(). + * REFUSES the inline-lambda subscription — own-check says it "has no '-=' handle, so + it could never be detached"; a lambda needs extraction to a named handler first. + * REFUSES the disposable-local shape — wrapping a local needs a scoped `using`, not a + teardown hook. Both refusals are classified suggest-only, never a fake patch. """ from __future__ import annotations @@ -27,10 +27,14 @@ # event '' is subscribed (handler '') _SUB_RE = re.compile(r"event '([^']+)' is subscribed \(handler '(.+?)'\)", re.S) +_FIELD_RE = re.compile(r"IDisposable field '([^']+)'") # ... is never disposed +_LOCAL_RE = re.compile(r"IDisposable local '([^']+)'") NAMED_HANDLER_SUB = "named-handler-sub" # fixable: insert a detach -INLINE_LAMBDA_SUB = "inline-lambda-sub" # suggest-only: needs extraction first -OTHER = "other" # not a subscription shape we handle here +DISPOSABLE_FIELD = "disposable-field" # fixable on a WPF owner: dispose on teardown +INLINE_LAMBDA_SUB = "inline-lambda-sub" # suggest-only: needs lambda extraction first +DISPOSABLE_LOCAL = "disposable-local" # suggest-only: needs a scoped `using` +OTHER = "other" # not a shape we handle here def _safe_join(workdir: str, rel: str) -> str: @@ -45,15 +49,23 @@ def _safe_join(workdir: str, rel: str) -> str: def classify(message: str): - """(shape, src_event, handler). Inline lambdas (handler contains `=>`, or the - message flags 'inline lambda') are suggest-only — they have no detach handle.""" - m = _SUB_RE.search(message or "") - if not m: - return OTHER, None, None - src_event, handler = m.group(1), m.group(2) - if "=>" in handler or "inline lambda" in (message or ""): - return INLINE_LAMBDA_SUB, src_event, handler - return NAMED_HANDLER_SUB, src_event, handler + """(shape, a, b). For subscriptions a=src.event, b=handler; for a disposable + field a=field name, b=None. Inline lambdas (handler has `=>`) and disposable + locals are suggest-only — they have no detach handle / need a scoped `using`.""" + msg = message or "" + m = _SUB_RE.search(msg) + if m: + src_event, handler = m.group(1), m.group(2) + if "=>" in handler or "inline lambda" in msg: + return INLINE_LAMBDA_SUB, src_event, handler + return NAMED_HANDLER_SUB, src_event, handler + m = _FIELD_RE.search(msg) + if m: + return DISPOSABLE_FIELD, m.group(1), None + m = _LOCAL_RE.search(msg) + if m: + return DISPOSABLE_LOCAL, m.group(1), None + return OTHER, None, None def _teardown_event(decl_tail: str): @@ -117,6 +129,25 @@ def _find_sub_line(lines: list[str], line_1based: int, src_event: str): return None +def _find_ctor_anchor(lines: list[str], field_line_1based: int): + """For a disposable field (reported at its declaration), find an in-ctor anchor to + hang the teardown on: the `InitializeComponent()` call of the field's enclosing + class. WPF code-behind reliably has one, and a statement after it is in scope for + `this. += ...`. Returns None (→ suggest-only) if there's no such anchor.""" + start = field_line_1based - 1 + cls_idx = None + for i in range(min(start, len(lines) - 1), -1, -1): + if re.search(r"\bclass\s+\w+", lines[i]): + cls_idx = i + break + if cls_idx is None: + return None + for i in range(cls_idx, len(lines)): + if "InitializeComponent()" in lines[i]: + return i + return None + + def plan_file(path: str, findings): """Compute (new_content, applied, skipped) for one file. `applied`/`skipped` are (finding, detail) lists so the ledger can report exactly what was and @@ -127,13 +158,19 @@ def plan_file(path: str, findings): seen: set[tuple[int, str]] = set() applied, skipped = [], [] for f in findings: - shape, src_event, handler = classify(f.message) - if shape != NAMED_HANDLER_SUB: + shape, a, b = classify(f.message) + # Per shape: find the in-scope anchor line and the statement to run on teardown. + if shape == NAMED_HANDLER_SUB: # a=src.event, b=handler + idx = _find_sub_line(lines, f.line, a) + stmt = None if idx is None else f"{a} -= {b}" + elif shape == DISPOSABLE_FIELD: # a=field name + idx = _find_ctor_anchor(lines, f.line) + stmt = None if idx is None else f"{a}?.Dispose()" + else: # lambda / local / other -> suggest-only skipped.append((f, shape)) continue - idx = _find_sub_line(lines, f.line, src_event) if idx is None: - skipped.append((f, "site-not-found")) + skipped.append((f, "site-not-found" if shape == NAMED_HANDLER_SUB else "no-ctor-anchor")) continue if _in_unbraced_control_flow(lines, idx): skipped.append((f, "unbraced-control-flow")) @@ -144,7 +181,7 @@ def plan_file(path: str, findings): skipped.append((f, "no-safe-teardown")) continue indent = re.match(r"\s*", lines[idx]).group(0) - ins = (idx, f"{indent}this.{ev} += (s, e) => {src_event} -= {handler};\n") + ins = (idx, f"{indent}this.{ev} += (s, e) => {stmt};\n") if ins in seen: skipped.append((f, "duplicate-site")) # detach already planned; keep the ledger complete continue diff --git a/fix/fixtures/own001-disposable-field/after.findings.json b/fix/fixtures/own001-disposable-field/after.findings.json new file mode 100644 index 0000000..2ef5648 --- /dev/null +++ b/fix/fixtures/own001-disposable-field/after.findings.json @@ -0,0 +1,3 @@ +{ + "findings": [] +} diff --git a/fix/fixtures/own001-disposable-field/before.findings.json b/fix/fixtures/own001-disposable-field/before.findings.json new file mode 100644 index 0000000..4b864ce --- /dev/null +++ b/fix/fixtures/own001-disposable-field/before.findings.json @@ -0,0 +1,12 @@ +{ + "findings": [ + { + "tool": "own-check", + "path": "Broker/ShareWindow.xaml.cs", + "line": 8, + "rule": "OWN001", + "category_name": "idisposable-leak", + "message": "IDisposable field '_timer' (type 'Timer') is never disposed — its owner 'ShareWindow' leaks it (leak) [resource: disposable field]" + } + ] +} diff --git a/fix/fixtures/own001-disposable-field/before/Broker/ShareWindow.xaml.cs b/fix/fixtures/own001-disposable-field/before/Broker/ShareWindow.xaml.cs new file mode 100644 index 0000000..9b42a54 --- /dev/null +++ b/fix/fixtures/own001-disposable-field/before/Broker/ShareWindow.xaml.cs @@ -0,0 +1,17 @@ +using System.Timers; +using System.Windows; + +namespace Sts.Broker +{ + public partial class ShareWindow : Window + { + private readonly Timer _timer; + + public ShareWindow() + { + InitializeComponent(); + _timer = new Timer(1000); + _timer.Start(); + } + } +} diff --git a/fix/tests/test_own_fix.py b/fix/tests/test_own_fix.py index 0aca05a..239aac4 100644 --- a/fix/tests/test_own_fix.py +++ b/fix/tests/test_own_fix.py @@ -19,7 +19,7 @@ from fixarm.appliers import ReplayReaudit # noqa: E402 from fixarm.own_fix import ( # noqa: E402 OwnFixApplier, classify, plan_file, - NAMED_HANDLER_SUB, INLINE_LAMBDA_SUB, + NAMED_HANDLER_SUB, INLINE_LAMBDA_SUB, DISPOSABLE_FIELD, DISPOSABLE_LOCAL, ) from fixarm.orchestrate import ( # noqa: E402 Finding, load_findings, run_fix, OK, REJECTED, @@ -75,6 +75,10 @@ def test_classify_named_vs_lambda(): assert classify(named)[0] == NAMED_HANDLER_SUB assert classify(named)[1:] == ("fGoods.PropertyChanged", "new PropertyChangedEventHandler(GoodsPropertyChanged)") assert classify(lam)[0] == INLINE_LAMBDA_SUB + field = "IDisposable field '_timer' (type 'Timer') is never disposed — its owner 'ShareWindow' leaks it" + local = "IDisposable local 'MyProc' is never disposed (leak)" + assert classify(field)[0] == DISPOSABLE_FIELD and classify(field)[1] == "_timer" + assert classify(local)[0] == DISPOSABLE_LOCAL # suggest-only # ---- OWN001 named handler on a Window -> Closed teardown ------------------- @@ -109,6 +113,21 @@ def test_own014_usercontrol_inserts_unloaded_detach(): "this.Unloaded += (s, e) => fThis.PropertyChanged -= data_PropertyChanged;") +# ---- OWN001 disposable field on a Window -> dispose on Closed --------------- + +def test_own001_disposable_field_disposes_on_closed(): + rel = "Broker/ShareWindow.xaml.cs" + before = _read(_seed("own001-disposable-field"), rel) + with _wrapped("own001-disposable-field", "OWN001") as (res, wd, _applier): + assert res.status == OK, res.ledger() + assert res.tier == tiers.T4 and res.gate == tiers.REVIEW + lines = _read(wd, rel) + assert len(lines) == len(before) + 1 + # the dispose hook is anchored right after InitializeComponent(), in a Closed hook + init = next(i for i, line in enumerate(lines) if "InitializeComponent()" in line) + assert lines[init + 1].strip() == "this.Closed += (s, e) => _timer?.Dispose();" + + # ---- inline lambda is suggest-only: NOT patched ---------------------------- def test_inline_lambda_is_not_patched(): From 67574ae27269b65dd35f72b59045ae2ce1484390 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 25 Jun 2026 16:20:09 +0000 Subject: [PATCH 2/3] feat(own): disposable-local -> using, and inline-lambda extraction Two new OWN001 shapes for the T4 fixer, both conservative (T4: refuse rather than emit a wrong patch). Generalized plan_file to an edit engine ((start,end,repl) applied bottom-up) so multi-line rewrites compose. - disposable LOCAL: wrap a clean 'T x = new T(...);' in a block 'using (...) {}', but only when the local doesn't escape its block (return/out/ref/store -> refuse 'local-escapes', no use-after-dispose). - inline-LAMBDA subscription: extract the lambda to a named handler, rewrite '+=' to the method group, add the teardown detach. Only well-known event delegates (PropertyChanged/ListChanged/CollectionChanged) with a 2-param expression lambda; block bodies / unknown delegates / multi-line subs -> refuse. Scope analysis is char-level brace matching that ignores strings and // comments. Fixtures: Helper (local), DatabaseOptimizationWindow (lambda extract + a block-body refusal variant). +4 tests incl. escaping-local and block-lambda refusals. 16/16 own + 7/7 wrapper, normal and -O. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoa --- docs/fix-arm.md | 22 +- fix/README.md | 32 ++- fix/fixarm/own_fix.py | 269 +++++++++++++++--- .../after.findings.json | 3 + .../before.findings.json | 12 + .../before/Broker/Helper.cs | 14 + .../own001-lambda-extract/after.findings.json | 3 + .../before.findings.json | 12 + .../Broker/DatabaseOptimizationWindow.xaml.cs | 16 ++ .../own001-lambda/before.findings.json | 4 +- .../Broker/DatabaseOptimizationWindow.xaml.cs | 5 +- fix/tests/test_own_fix.py | 61 +++- 12 files changed, 378 insertions(+), 75 deletions(-) create mode 100644 fix/fixtures/own001-disposable-local/after.findings.json create mode 100644 fix/fixtures/own001-disposable-local/before.findings.json create mode 100644 fix/fixtures/own001-disposable-local/before/Broker/Helper.cs create mode 100644 fix/fixtures/own001-lambda-extract/after.findings.json create mode 100644 fix/fixtures/own001-lambda-extract/before.findings.json create mode 100644 fix/fixtures/own001-lambda-extract/before/Broker/DatabaseOptimizationWindow.xaml.cs diff --git a/docs/fix-arm.md b/docs/fix-arm.md index a11367e..8c8c7d7 100644 --- a/docs/fix-arm.md +++ b/docs/fix-arm.md @@ -115,16 +115,18 @@ No step is AI-judged. The asserts are exact, mirroring Arm 2's scenario discipli Build-free, develops and unit-tests on Linux against synthetic fixtures (Roslyn and both CLIs are cross-platform). - **Build — the OWN fixer** (T4): `OWN001`/`OWN014`. Structural, build-free, reviewable - patches — built in [`../fix/fixarm/own_fix.py`](../fix/fixarm/own_fix.py). On a WPF - owner it hangs cleanup on a teardown event (`Window` → `Closed`, `FrameworkElement` → - `Unloaded`) and fixes two shapes: the **named-handler subscription** (insert - `src.Event -= Handler`) and the **disposable field** (insert `field?.Dispose()`, - anchored after the ctor's `InitializeComponent()`). Fixtures are real STS sites - (`AmountWindow` OWN001, `KTSGoods2` OWN014, `ShareWindow._timer`). The **inline-lambda** - subscription and the **disposable-local** are classified **suggest-only** and never - patched (lambda needs extraction; a local needs a scoped `using`). Still to build: - disposable-local → `using`, lambda extraction, folding into an existing - `OnClosed`/`Dispose`. Every OWN result is queued-for-review (T4), never auto. + patches — built in [`../fix/fixarm/own_fix.py`](../fix/fixarm/own_fix.py). Fixes **four** + shapes, conservatively (refuse rather than emit a wrong patch): **named-handler + subscription** and **disposable field** → cleanup on the owner's teardown event + (`Window` → `Closed`, `FrameworkElement` → `Unloaded`); **disposable local** → block + `using` (only when it doesn't escape the block); **inline-lambda subscription** → + extract to a named handler then detach (only well-known event delegates, 2-param + expression lambdas). Refusals (`local-escapes`, `lambda-shape-unsupported`, + `unbraced-control-flow`, `no-safe-teardown`, …) are surfaced in `applier.skipped`, + never faked. Fixtures are real STS sites (`AmountWindow`, `KTSGoods2`, `ShareWindow`, + `Helper`, `DatabaseOptimizationWindow`). Brace/scope analysis is char-level (ignores + strings + `//` comments). Still to build: fold into an existing `OnClosed`/`Dispose`; + more event delegates. Every OWN result is queued-for-review (T4), never auto. --- diff --git a/fix/README.md b/fix/README.md index d40488a..a24cf63 100644 --- a/fix/README.md +++ b/fix/README.md @@ -53,28 +53,40 @@ PYTHONPATH=fix python3 -m fixarm.cli --fixture fix/fixtures/own001-sub-window \ --rule OWN001 --applier own --show-diff ``` -It fixes two shapes on a WPF owner by hanging cleanup on a teardown event -(`Window` → `Closed`, `FrameworkElement` → `Unloaded`): +It fixes **four** shapes (conservatively — refuses rather than emit a wrong patch): ```diff - // named-handler subscription + // 1. named-handler subscription -> detach on the owner's teardown event fGoods.PropertyChanged += new PropertyChangedEventHandler(GoodsPropertyChanged); + this.Closed += (s, e) => fGoods.PropertyChanged -= new PropertyChangedEventHandler(GoodsPropertyChanged); - // disposable field (Timer / CancellationTokenSource / …), anchored after the ctor + // 2. disposable field (Timer / CTS / …) -> dispose on teardown, after the ctor public ShareWindow() { InitializeComponent(); + this.Closed += (s, e) => _timer?.Dispose(); + + // 3. disposable local -> block `using` (only when it doesn't escape the block) +- var myProcess = new Process(); ++ using (var myProcess = new Process()) ++ { + myProcess.Start(); ++ } + + // 4. inline-lambda subscription -> extract to a named handler, then detach +- stage.PropertyChanged += (s2, e2) => OnPropertyChanged("Stages"); ++ stage.PropertyChanged += OnStagePropertyChanged; ++ this.Closed += (s, e) => stage.PropertyChanged -= OnStagePropertyChanged; ++ private void OnStagePropertyChanged(object s2, PropertyChangedEventArgs e2) => OnPropertyChanged("Stages"); ``` -It **refuses** the inline-lambda subscription (own-check: "no `-=` handle … could never -be detached" → needs extraction first) and the disposable-**local** (needs a scoped -`using`) — both are classified suggest-only and surfaced in `applier.skipped`, never -patched with a fake fix. +**Refusals stay honest** (surfaced in `applier.skipped`, never a fake patch): a local +that escapes its block (return/out/ref/store) → `local-escapes`; a block-body or +unknown-delegate lambda → `lambda-shape-unsupported` / `unknown-event-delegate`; an +unbraced guard → `unbraced-control-flow`; no safe teardown → `no-safe-teardown`. ## Next - Promote proven-mechanical rules into `tiers._T1_RULES` (auto-commit) from real diffs. -- OWN fixer: disposable-**local** → scoped `using`; inline-lambda **extraction** then - detach; consolidate into an existing `OnClosed`/`Dispose` override when one is present. +- OWN fixer: fold into an existing `OnClosed`/`Dispose` override when one is present; + widen lambda extraction to more event delegates. - Windows-bound fix-spike: does `roslynator fix` load `Broker.sln` (docs/fix-arm.md §6). diff --git a/fix/fixarm/own_fix.py b/fix/fixarm/own_fix.py index c858532..d65353f 100644 --- a/fix/fixarm/own_fix.py +++ b/fix/fixarm/own_fix.py @@ -6,18 +6,20 @@ OWN rules are tier T4 → the wrapper always routes the result to REVIEW; nothing here auto-commits, because lifetime-correct teardown placement is a judgement call. -Scope of THIS slice — honest about the boundary. For a WPF owner we hang the cleanup -on a teardown event (Closed for a Window, Unloaded for a FrameworkElement); anything -else is left for review (we can't pick a safe teardown blind). - * FIXES the named-handler subscription shape — `src.Event += Handler;` (method group - or `new D(M)`) → `this. += (s, e) => src.Event -= Handler;` - * FIXES the disposable-field shape — an IDisposable field never disposed (a Timer, - CancellationTokenSource, …) → `this. += (s, e) => field?.Dispose();`, - anchored after the ctor's InitializeComponent(). - * REFUSES the inline-lambda subscription — own-check says it "has no '-=' handle, so - it could never be detached"; a lambda needs extraction to a named handler first. - * REFUSES the disposable-local shape — wrapping a local needs a scoped `using`, not a - teardown hook. Both refusals are classified suggest-only, never a fake patch. +Scope — conservative on purpose (T4: refuse rather than emit a wrong patch). For a WPF +owner we hang cleanup on a teardown event (Closed for a Window, Unloaded for a +FrameworkElement). Four shapes are fixed; anything ambiguous is left suggest-only. + * named-handler subscription — `src.Event += Handler;` → + `this. += (s, e) => src.Event -= Handler;` + * disposable field — never-disposed IDisposable field (Timer, CTS, …) → + `this. += (s, e) => field?.Dispose();` (after InitializeComponent()) + * disposable local — a clean `T x = new T(...);` that does NOT escape its block → + wrap it in a block `using (...) { … }`. Escapes (return/out/ref/store) → refuse. + * inline-lambda subscription — extract the lambda to a named method, rewrite `+=` to + the method group, then add the detach. ONLY for well-known events + (PropertyChanged/ListChanged/CollectionChanged) with a 2-param expression lambda; + block bodies, unknown delegates, multi-line subscriptions → refuse. +Every refusal is surfaced in `applier.skipped`, never patched with a fake fix. """ from __future__ import annotations @@ -148,49 +150,222 @@ def _find_ctor_anchor(lines: list[str], field_line_1based: int): return None +# ---- brace/scope helpers (char-level, ignoring strings + // comments) ------- + +def _code_skeleton(line: str) -> str: + """`line` with string/char-literal contents and // comments blanked, so that + only structural braces survive. Best-effort (no verbatim/interpolated strings), + but the fixers refuse (suggest-only) whenever a block close can't be matched.""" + out, i, n = [], 0, len(line) + while i < n: + c = line[i] + if c == "/" and i + 1 < n and line[i + 1] == "/": + break + if c in "\"'": + q, i = c, i + 1 + while i < n: + if line[i] == "\\": + i += 2 + continue + if line[i] == q: + i += 1 + break + i += 1 + out.append(" ") + continue + out.append(c) + i += 1 + return "".join(out) + + +def _enclosing_block_close(lines: list[str], decl_idx: int): + """Line index of the `}` that closes the block directly containing decl_idx.""" + depth = 0 + for i in range(decl_idx + 1, len(lines)): + for ch in _code_skeleton(lines[i]): + if ch == "{": + depth += 1 + elif ch == "}": + if depth == 0: + return i + depth -= 1 + return None + + +def _class_close(lines: list[str], cls_idx: int): + """Line index of the `}` closing the class whose declaration is at cls_idx.""" + depth, started = 0, False + for i in range(cls_idx, len(lines)): + for ch in _code_skeleton(lines[i]): + if ch == "{": + depth, started = depth + 1, True + elif ch == "}": + depth -= 1 + if started and depth == 0: + return i + return None + + +# ---- per-shape planners: each returns (edits, detail) or (None, skip_reason) - +# An edit is (start, end, repl_lines): `lines[start:end] = repl_lines` (insert when +# start == end). plan_file applies them bottom-up so indices stay valid. + +def _plan_teardown(lines, f, idx, stmt, anchor_missing="site-not-found"): + """Subscription / disposable-field: hang `stmt` on the owner's teardown event.""" + if idx is None: + return None, anchor_missing + if _in_unbraced_control_flow(lines, idx): + return None, "unbraced-control-flow" + _, decl = _enclosing_class(lines, idx) + ev = _teardown_event(decl) + if ev is None: + return None, "no-safe-teardown" + indent = re.match(r"\s*", lines[idx]).group(0) + hook = f"{indent}this.{ev} += (s, e) => {stmt};\n" + return [(idx + 1, idx + 1, [hook])], ev + + +_LOCAL_NEW = r"^(\s*)((?:var|[A-Za-z_][\w.<>\[\]]*)\s+{name}\s*=\s*new\b[^;{{}}]*);\s*$" + + +def _plan_local(lines, f, name): + """Disposable local: wrap a clean `T x = new T(...);` in a block `using`, but only + when the local clearly doesn't escape its block (no return/out/ref/store of it).""" + rx = re.compile(_LOCAL_NEW.format(name=re.escape(name))) + target = f.line - 1 + hit = None + for cand in [target] + [target + d for d in (1, -1, 2, -2, 3, -3)]: + if 0 <= cand < len(lines): + m = rx.match(lines[cand]) + if m: + hit = (cand, m.group(1), m.group(2)) + break + if hit is None: + return None, "decl-not-simple-new" # object initializers, multi-line, etc. + idx, indent, core = hit + close = _enclosing_block_close(lines, idx) + if close is None: + return None, "no-block-close" + region = "".join(_code_skeleton(line) for line in lines[idx + 1:close]) + nm = re.escape(name) + if (re.search(rf"\breturn\b[^;]*\b{nm}\b", region) # returned + or re.search(rf"\b(out|ref)\s+{nm}\b", region) # passed out/ref + or re.search(rf"=\s*{nm}\s*[;,)]", region) # stored elsewhere + or "yield" in region): + return None, "local-escapes" # disposing here would be use-after-dispose + edits = [ + (idx, idx + 1, [f"{indent}using ({core})\n", f"{indent}{{\n"]), + (close, close, [f"{indent}}}\n"]), + ] + return edits, "using" + + +# well-known events whose delegate's EventArgs type we can name without a compiler +_EVENT_ARGS = { + "PropertyChanged": "PropertyChangedEventArgs", + "ListChanged": "ListChangedEventArgs", + "CollectionChanged": "NotifyCollectionChangedEventArgs", +} +_LAMBDA2 = re.compile(r"^\(\s*(\w+)\s*,\s*(\w+)\s*\)\s*=>\s*(.+?)\s*$") + + +def _ident_exists(lines, name): + rx = re.compile(rf"\b{re.escape(name)}\b") + return any(rx.search(_code_skeleton(line)) for line in lines) + + +def _plan_lambda(lines, f, src_event, handler): + """Inline-lambda subscription: extract the lambda to a named method, rewrite the + `+=` to the method group, and add the teardown detach. Only for well-known events + (known delegate args) with a two-param expression lambda — else suggest-only.""" + args_type = _EVENT_ARGS.get(src_event.rsplit(".", 1)[-1]) + if args_type is None: + return None, "unknown-event-delegate" + m = _LAMBDA2.match(handler.strip()) + if not m: + return None, "lambda-shape-unsupported" # arity != 2 + p1, p2, expr = m.group(1), m.group(2), m.group(3) + if "{" in expr or ";" in expr: + return None, "lambda-shape-unsupported" # block body + idx = _find_sub_line(lines, f.line, src_event) + if idx is None: + return None, "site-not-found" + if not lines[idx].rstrip().endswith(";"): + return None, "multiline-subscription" + if _in_unbraced_control_flow(lines, idx): + return None, "unbraced-control-flow" + cls_idx = next((i for i in range(idx, -1, -1) if re.search(r"\bclass\s+\w+", lines[i])), None) + if cls_idx is None: + return None, "no-class" + _, decl = _enclosing_class(lines, idx) + ev = _teardown_event(decl) + if ev is None: + return None, "no-safe-teardown" + close = _class_close(lines, cls_idx) + if close is None: + return None, "no-class-close" + name = "On" + "".join(w[:1].upper() + w[1:] for w in re.split(r"\W+", src_event) if w) + base, n = name, 2 + while _ident_exists(lines, name): + name, n = f"{base}{n}", n + 1 + indent = re.match(r"\s*", lines[idx]).group(0) + mindent = re.match(r"\s*", lines[cls_idx]).group(0) + " " + method = ["\n", f"{mindent}private void {name}(object {p1}, {args_type} {p2}) => {expr};\n"] + edits = [ + (idx, idx + 1, [f"{indent}{src_event} += {name};\n"]), + (idx + 1, idx + 1, [f"{indent}this.{ev} += (s, e) => {src_event} -= {name};\n"]), + (close, close, method), + ] + return edits, "extract+detach" + + +def _plan_one(lines, f): + """Dispatch a finding to its shape's planner.""" + shape, a, b = classify(f.message) + if shape == NAMED_HANDLER_SUB: + return _plan_teardown(lines, f, _find_sub_line(lines, f.line, a), f"{a} -= {b}") + if shape == DISPOSABLE_FIELD: + return _plan_teardown(lines, f, _find_ctor_anchor(lines, f.line), + f"{a}?.Dispose()", anchor_missing="no-ctor-anchor") + if shape == DISPOSABLE_LOCAL: + return _plan_local(lines, f, a) + if shape == INLINE_LAMBDA_SUB: + return _plan_lambda(lines, f, a, b) + return None, shape # OTHER + + def plan_file(path: str, findings): """Compute (new_content, applied, skipped) for one file. `applied`/`skipped` - are (finding, detail) lists so the ledger can report exactly what was and - wasn't fixed — no silent drops (docs/fix-arm.md §8).""" + are (finding, detail) lists so the ledger reports exactly what was and wasn't + fixed — no silent drops (docs/fix-arm.md §8).""" with open(path, encoding="utf-8") as fh: lines = fh.readlines() - inserts: list[tuple[int, str]] = [] - seen: set[tuple[int, str]] = set() - applied, skipped = [], [] + planned, applied, skipped = [], [], [] + seen: set = set() # identical edit-sets -> duplicate-site + occupied: set = set() # original line indices already targeted -> avoid overlaps for f in findings: - shape, a, b = classify(f.message) - # Per shape: find the in-scope anchor line and the statement to run on teardown. - if shape == NAMED_HANDLER_SUB: # a=src.event, b=handler - idx = _find_sub_line(lines, f.line, a) - stmt = None if idx is None else f"{a} -= {b}" - elif shape == DISPOSABLE_FIELD: # a=field name - idx = _find_ctor_anchor(lines, f.line) - stmt = None if idx is None else f"{a}?.Dispose()" - else: # lambda / local / other -> suggest-only - skipped.append((f, shape)) - continue - if idx is None: - skipped.append((f, "site-not-found" if shape == NAMED_HANDLER_SUB else "no-ctor-anchor")) - continue - if _in_unbraced_control_flow(lines, idx): - skipped.append((f, "unbraced-control-flow")) + edits, detail = _plan_one(lines, f) + if edits is None: + skipped.append((f, detail)) continue - _, decl = _enclosing_class(lines, idx) - ev = _teardown_event(decl) - if ev is None: - skipped.append((f, "no-safe-teardown")) + key = tuple((s, e, tuple(r)) for s, e, r in edits) + if key in seen: + skipped.append((f, "duplicate-site")) continue - indent = re.match(r"\s*", lines[idx]).group(0) - ins = (idx, f"{indent}this.{ev} += (s, e) => {stmt};\n") - if ins in seen: - skipped.append((f, "duplicate-site")) # detach already planned; keep the ledger complete + touched = set() + for s, e, _ in edits: + touched.update(range(s, max(e, s + 1))) + if touched & occupied: + skipped.append((f, "overlapping-edit")) continue - seen.add(ins) - inserts.append(ins) - applied.append((f, ev)) - # insert bottom-up so earlier indices stay valid - for idx, text in sorted(inserts, key=lambda t: t[0], reverse=True): - lines.insert(idx + 1, text) + seen.add(key) + occupied |= touched + planned.append(edits) + applied.append((f, detail)) + # apply every edit bottom-up so earlier line indices stay valid + for s, e, repl in sorted((ed for edits in planned for ed in edits), + key=lambda t: t[0], reverse=True): + lines[s:e] = repl return "".join(lines), applied, skipped diff --git a/fix/fixtures/own001-disposable-local/after.findings.json b/fix/fixtures/own001-disposable-local/after.findings.json new file mode 100644 index 0000000..2ef5648 --- /dev/null +++ b/fix/fixtures/own001-disposable-local/after.findings.json @@ -0,0 +1,3 @@ +{ + "findings": [] +} diff --git a/fix/fixtures/own001-disposable-local/before.findings.json b/fix/fixtures/own001-disposable-local/before.findings.json new file mode 100644 index 0000000..9b65a10 --- /dev/null +++ b/fix/fixtures/own001-disposable-local/before.findings.json @@ -0,0 +1,12 @@ +{ + "findings": [ + { + "tool": "own-check", + "path": "Broker/Helper.cs", + "line": 9, + "rule": "OWN001", + "category_name": "idisposable-leak", + "message": "IDisposable local 'myProcess' is never disposed (leak) [resource: disposable]" + } + ] +} diff --git a/fix/fixtures/own001-disposable-local/before/Broker/Helper.cs b/fix/fixtures/own001-disposable-local/before/Broker/Helper.cs new file mode 100644 index 0000000..7553de0 --- /dev/null +++ b/fix/fixtures/own001-disposable-local/before/Broker/Helper.cs @@ -0,0 +1,14 @@ +using System.Diagnostics; + +namespace Sts.Broker +{ + public static class Helper + { + public static void Run() + { + var myProcess = new Process(); + myProcess.StartInfo.FileName = "x.exe"; + myProcess.Start(); + } + } +} diff --git a/fix/fixtures/own001-lambda-extract/after.findings.json b/fix/fixtures/own001-lambda-extract/after.findings.json new file mode 100644 index 0000000..2ef5648 --- /dev/null +++ b/fix/fixtures/own001-lambda-extract/after.findings.json @@ -0,0 +1,3 @@ +{ + "findings": [] +} diff --git a/fix/fixtures/own001-lambda-extract/before.findings.json b/fix/fixtures/own001-lambda-extract/before.findings.json new file mode 100644 index 0000000..137c638 --- /dev/null +++ b/fix/fixtures/own001-lambda-extract/before.findings.json @@ -0,0 +1,12 @@ +{ + "findings": [ + { + "tool": "own-check", + "path": "Broker/DatabaseOptimizationWindow.xaml.cs", + "line": 11, + "rule": "OWN001", + "category_name": "subscription-leak", + "message": "event 'stage.PropertyChanged' is subscribed (handler '(s2, e2) => OnPropertyChanged(\"Stages\")') but never unsubscribed; its source is an injected dependency whose lifetime is unknown, so it may outlive and keep 'DatabaseOptimizationWindow' alive (possible leak - and being an inline lambda it has no '-=' handle, so it could never be detached) [resource: subscription token]" + } + ] +} diff --git a/fix/fixtures/own001-lambda-extract/before/Broker/DatabaseOptimizationWindow.xaml.cs b/fix/fixtures/own001-lambda-extract/before/Broker/DatabaseOptimizationWindow.xaml.cs new file mode 100644 index 0000000..1eb3ff1 --- /dev/null +++ b/fix/fixtures/own001-lambda-extract/before/Broker/DatabaseOptimizationWindow.xaml.cs @@ -0,0 +1,16 @@ +using System.ComponentModel; +using System.Windows; + +namespace Sts.Broker +{ + public partial class DatabaseOptimizationWindow : Window + { + public DatabaseOptimizationWindow(Stage stage) + { + InitializeComponent(); + stage.PropertyChanged += (s2, e2) => OnPropertyChanged("Stages"); + } + + private void OnPropertyChanged(string name) { } + } +} diff --git a/fix/fixtures/own001-lambda/before.findings.json b/fix/fixtures/own001-lambda/before.findings.json index a3245f1..b55160b 100644 --- a/fix/fixtures/own001-lambda/before.findings.json +++ b/fix/fixtures/own001-lambda/before.findings.json @@ -3,10 +3,10 @@ { "tool": "own-check", "path": "Broker/DatabaseOptimizationWindow.xaml.cs", - "line": 10, + "line": 11, "rule": "OWN001", "category_name": "subscription-leak", - "message": "event 'stage.PropertyChanged' is subscribed (handler '(s2, e2) => OnPropertyChanged(\"Stages\")') but never unsubscribed; its source is an injected dependency whose lifetime is unknown, so it may outlive and keep 'DatabaseOptimizationWindow' alive (possible leak - and being an inline lambda it has no '-=' handle, so it could never be detached) [resource: subscription token]" + "message": "event 'stage.PropertyChanged' is subscribed (handler '(s2, e2) => { OnPropertyChanged(\"Stages\"); }') but never unsubscribed; its source is an injected dependency whose lifetime is unknown, so it may outlive and keep 'DatabaseOptimizationWindow' alive (possible leak - and being an inline lambda it has no '-=' handle, so it could never be detached) [resource: subscription token]" } ] } diff --git a/fix/fixtures/own001-lambda/before/Broker/DatabaseOptimizationWindow.xaml.cs b/fix/fixtures/own001-lambda/before/Broker/DatabaseOptimizationWindow.xaml.cs index c35dc73..91f3ae2 100644 --- a/fix/fixtures/own001-lambda/before/Broker/DatabaseOptimizationWindow.xaml.cs +++ b/fix/fixtures/own001-lambda/before/Broker/DatabaseOptimizationWindow.xaml.cs @@ -1,3 +1,4 @@ +using System.ComponentModel; using System.Windows; namespace Sts.Broker @@ -7,7 +8,9 @@ public partial class DatabaseOptimizationWindow : Window public DatabaseOptimizationWindow(Stage stage) { InitializeComponent(); - stage.PropertyChanged += (s2, e2) => OnPropertyChanged("Stages"); + stage.PropertyChanged += (s2, e2) => { OnPropertyChanged("Stages"); }; } + + private void OnPropertyChanged(string name) { } } } diff --git a/fix/tests/test_own_fix.py b/fix/tests/test_own_fix.py index 239aac4..4ad1752 100644 --- a/fix/tests/test_own_fix.py +++ b/fix/tests/test_own_fix.py @@ -128,11 +128,42 @@ def test_own001_disposable_field_disposes_on_closed(): assert lines[init + 1].strip() == "this.Closed += (s, e) => _timer?.Dispose();" -# ---- inline lambda is suggest-only: NOT patched ---------------------------- +# ---- OWN001 disposable local -> block `using` wrap ------------------------- -def test_inline_lambda_is_not_patched(): - fdir = os.path.join(FIX, "own001-lambda") +def test_own001_disposable_local_wraps_in_using(): + rel = "Broker/Helper.cs" + with _wrapped("own001-disposable-local", "OWN001") as (res, wd, _applier): + assert res.status == OK, res.ledger() + text = "".join(_read(wd, rel)) + assert "using (var myProcess = new Process())" in text # wrapped + assert "var myProcess = new Process();" not in text # bare decl gone + # the using opener is immediately followed by its block brace + lines = _read(wd, rel) + u = next(i for i, line in enumerate(lines) if "using (var myProcess" in line) + assert lines[u + 1].strip() == "{" + + +# ---- OWN001 inline lambda -> extract to a named handler + detach ------------ + +def test_own001_inline_lambda_extracted_and_detached(): + rel = "Broker/DatabaseOptimizationWindow.xaml.cs" + with _wrapped("own001-lambda-extract", "OWN001") as (res, wd, _applier): + assert res.status == OK, res.ledger() + assert res.tier == tiers.T4 + text = "".join(_read(wd, rel)) + assert "stage.PropertyChanged += OnStagePropertyChanged;" in text # method group + assert "this.Closed += (s, e) => stage.PropertyChanged -= OnStagePropertyChanged;" in text + assert ('private void OnStagePropertyChanged(object s2, PropertyChangedEventArgs e2) ' + '=> OnPropertyChanged("Stages");') in text # extracted method + assert "+= (s2, e2) =>" not in text # the lambda is gone + + +# ---- refused shapes stay suggest-only: NOT patched ------------------------- + +def test_block_lambda_is_not_patched(): + # a block-body lambda can't be a clean expression method -> suggest-only, untouched rel = "Broker/DatabaseOptimizationWindow.xaml.cs" + fdir = os.path.join(FIX, "own001-lambda") before = load_findings(os.path.join(fdir, "before.findings.json")) wd = _seed("own001-lambda") try: @@ -140,12 +171,32 @@ def test_inline_lambda_is_not_patched(): original = _read(wd, rel) applier.apply(wd, "OWN001") assert _read(wd, rel) == original # tree untouched - assert len(applier.skipped) == 1 # surfaced, not dropped - assert applier.skipped[0][1] == INLINE_LAMBDA_SUB + assert [r for _, r in applier.skipped] == ["lambda-shape-unsupported"] finally: shutil.rmtree(wd, ignore_errors=True) +def test_escaping_local_is_not_wrapped(): + # a local that is returned must NOT be wrapped (would dispose before use) + src = ("public static class H\n" + "{\n" + " public static Process Make()\n" + " {\n" + " var p = new Process();\n" + " return p;\n" + " }\n" + "}\n") + d, path = _tmp_cs(src) + try: + f = Finding("OWN001", "H.cs", 5, tool="own-check", + message="IDisposable local 'p' is never disposed (leak)") + new, applied, skipped = plan_file(path, [f]) + assert new == src and applied == [] + assert [r for _, r in skipped] == ["local-escapes"] + finally: + shutil.rmtree(d, ignore_errors=True) + + # ---- the fixer's own revert: a rejected OWN fix rolls back ----------------- def test_own_fix_reverts_on_regression(): From 0088e8311740295b973dce66534db9aabce79826 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 25 Jun 2026 16:28:31 +0000 Subject: [PATCH 3/3] fix(own): tighten local-escape + bound ctor anchor + allow multi-insert Address Codex (2) + CodeRabbit (3) review on PR #4: - local-escapes (Codex P1 + CodeRabbit): also refuse when the local is passed as a call argument ('Register(p)') or when a closure is present in the block ('=>' -> may capture and outlive it). Prevents turning a leak into a use-after-dispose. (+2 tests: arg-pass, lambda-capture) - _find_ctor_anchor (CodeRabbit): bound the InitializeComponent() search to the field's enclosing class via _class_close, so a later class can't anchor the hook in the wrong place. (+test: no-ctor-anchor when class lacks it) - overlap check (Codex P2): allow multiple INSERTIONS at one anchor (only replacements conflict), so every disposable field in a class gets its hook instead of all-but-first skipped as overlapping-edit. (+test: 2 fields) 20/20 own + 7/7 wrapper, normal and -O. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoa --- fix/fixarm/own_fix.py | 29 +++++++++---- fix/tests/test_own_fix.py | 88 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 8 deletions(-) diff --git a/fix/fixarm/own_fix.py b/fix/fixarm/own_fix.py index d65353f..aa31367 100644 --- a/fix/fixarm/own_fix.py +++ b/fix/fixarm/own_fix.py @@ -144,7 +144,10 @@ def _find_ctor_anchor(lines: list[str], field_line_1based: int): break if cls_idx is None: return None - for i in range(cls_idx, len(lines)): + # bound the search to THIS class — a later class's InitializeComponent() must + # not anchor the hook in the wrong class (would emit uncompilable code). + end = _class_close(lines, cls_idx) + for i in range(cls_idx, end + 1 if end is not None else len(lines)): if "InitializeComponent()" in lines[i]: return i return None @@ -251,6 +254,8 @@ def _plan_local(lines, f, name): if (re.search(rf"\breturn\b[^;]*\b{nm}\b", region) # returned or re.search(rf"\b(out|ref)\s+{nm}\b", region) # passed out/ref or re.search(rf"=\s*{nm}\s*[;,)]", region) # stored elsewhere + or re.search(rf"[(,]\s*{nm}\s*[),]", region) # passed as a call arg (may be retained) + or "=>" in region # a closure here may capture + outlive it or "yield" in region): return None, "local-escapes" # disposing here would be use-after-dispose edits = [ @@ -341,8 +346,9 @@ def plan_file(path: str, findings): with open(path, encoding="utf-8") as fh: lines = fh.readlines() planned, applied, skipped = [], [], [] - seen: set = set() # identical edit-sets -> duplicate-site - occupied: set = set() # original line indices already targeted -> avoid overlaps + seen: set = set() # identical edit-sets -> duplicate-site + replaced: set = set() # original line indices a REPLACEMENT consumes + inserted_at: set = set() # anchors used by INSERTIONS (several may share one anchor) for f in findings: edits, detail = _plan_one(lines, f) if edits is None: @@ -350,16 +356,23 @@ def plan_file(path: str, findings): continue key = tuple((s, e, tuple(r)) for s, e, r in edits) if key in seen: - skipped.append((f, "duplicate-site")) + skipped.append((f, "duplicate-site")) # same fix already planned continue - touched = set() + f_repl, f_ins = set(), set() for s, e, _ in edits: - touched.update(range(s, max(e, s + 1))) - if touched & occupied: + if e > s: + f_repl.update(range(s, e)) + else: + f_ins.add(s) + # Replacements must not overlap anything; insertions may share an anchor with + # other insertions (e.g. two disposable fields after one InitializeComponent()) + # but must not land inside a replaced range. + if (f_repl & replaced) or (f_repl & inserted_at) or (f_ins & replaced): skipped.append((f, "overlapping-edit")) continue seen.add(key) - occupied |= touched + replaced |= f_repl + inserted_at |= f_ins planned.append(edits) applied.append((f, detail)) # apply every edit bottom-up so earlier line indices stay valid diff --git a/fix/tests/test_own_fix.py b/fix/tests/test_own_fix.py index 4ad1752..88ffe5a 100644 --- a/fix/tests/test_own_fix.py +++ b/fix/tests/test_own_fix.py @@ -176,6 +176,94 @@ def test_block_lambda_is_not_patched(): shutil.rmtree(wd, ignore_errors=True) +def test_local_passed_to_call_is_not_wrapped(): + # local handed to a retaining API (Add) may be kept alive -> refuse, no use-after-dispose + src = ("public static class H\n" + "{\n" + " public static void Run()\n" + " {\n" + " var p = new Process();\n" + " sink.Add(p);\n" + " }\n" + "}\n") + d, path = _tmp_cs(src) + try: + f = Finding("OWN001", "H.cs", 5, tool="own-check", + message="IDisposable local 'p' is never disposed (leak)") + new, applied, skipped = plan_file(path, [f]) + assert new == src and applied == [] + assert [r for _, r in skipped] == ["local-escapes"] + finally: + shutil.rmtree(d, ignore_errors=True) + + +def test_local_captured_by_lambda_is_not_wrapped(): + # local captured by a closure that may outlive the block -> refuse + src = ("public partial class W : Window\n" + "{\n" + " public void Run()\n" + " {\n" + " var p = new Process();\n" + " button.Click += (s, e) => p.Start();\n" + " }\n" + "}\n") + d, path = _tmp_cs(src) + try: + f = Finding("OWN001", "W.cs", 5, tool="own-check", + message="IDisposable local 'p' is never disposed (leak)") + new, applied, skipped = plan_file(path, [f]) + assert new == src and applied == [] + assert [r for _, r in skipped] == ["local-escapes"] + finally: + shutil.rmtree(d, ignore_errors=True) + + +def test_ctor_anchor_bounded_to_enclosing_class(): + # the field's class has no InitializeComponent(); a LATER class does. The hook + # must NOT be anchored in the wrong class -> suggest-only (no-ctor-anchor). + src = ("public class A\n" + "{\n" + " private readonly Timer _t;\n" + "}\n" + "public partial class B : Window\n" + "{\n" + " public B() { InitializeComponent(); }\n" + "}\n") + d, path = _tmp_cs(src) + try: + f = Finding("OWN001", "A.cs", 3, tool="own-check", + message="IDisposable field '_t' (type 'Timer') is never disposed — its owner 'A' leaks it") + new, applied, skipped = plan_file(path, [f]) + assert new == src and applied == [] + assert [r for _, r in skipped] == ["no-ctor-anchor"] + finally: + shutil.rmtree(d, ignore_errors=True) + + +def test_multiple_disposable_fields_all_disposed(): + # two fields anchored after the same InitializeComponent() must BOTH get a hook + src = ("public partial class W : Window\n" + "{\n" + " private readonly Timer _t1;\n" + " private readonly Timer _t2;\n" + " public W()\n" + " {\n" + " InitializeComponent();\n" + " }\n" + "}\n") + d, path = _tmp_cs(src) + try: + msg = "IDisposable field '{}' (type 'Timer') is never disposed — its owner 'W' leaks it" + fs = [Finding("OWN001", "W.cs", 3, tool="own-check", message=msg.format("_t1")), + Finding("OWN001", "W.cs", 4, tool="own-check", message=msg.format("_t2"))] + new, applied, skipped = plan_file(path, fs) + assert len(applied) == 2 and skipped == [] # neither skipped as overlap + assert new.count("this.Closed += (s, e) => _t1?.Dispose();") == 1 + assert new.count("this.Closed += (s, e) => _t2?.Dispose();") == 1 + finally: + shutil.rmtree(d, ignore_errors=True) + + def test_escaping_local_is_not_wrapped(): # a local that is returned must NOT be wrapped (would dispose before use) src = ("public static class H\n"