feat(own): fold cleanup into OnClosed + wider lambda delegates

fix/README.md

@@ -84,9 +84,13 @@ that escapes its block (return/out/ref/store) → `local-escapes`; a block-body
 unknown-delegate lambda → `lambda-shape-unsupported` / `unknown-event-delegate`; an
 unbraced guard → `unbraced-control-flow`; no safe teardown → `no-safe-teardown`.
 
+When the owner already has a `protected override void OnClosed(...)`, the cleanup is
+**folded into it** (a statement at the top of the body) instead of stacking another
+`this.Closed += …` lambda — a cleaner, more idiomatic patch.
+
 ## Next
 
 - Promote proven-mechanical rules into `tiers._T1_RULES` (auto-commit) from real diffs.
-- OWN fixer: fold into an existing `OnClosed`/`Dispose` override when one is present;
-widen lambda extraction to more event delegates.
+- OWN fixer: fold into an existing `Dispose()`/`Unloaded` handler too; widen lambda
+ extraction to more event delegates (`EventHandler<T>`, custom `*Changed`).
 - Windows-bound fix-spike: does `roslynator fix` load `Broker.sln` (docs/fix-arm.md §6).

fix/fixarm/own_fix.py

@@ -213,8 +213,71 @@ def _class_close(lines: list[str], cls_idx: int):
 # 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."""
+def _enclosing_class_idx(lines, idx):
+ """Line index of the nearest `class X` declaration at or above idx."""
+ for i in range(idx, -1, -1):
+ if re.search(r"\bclass\s+\w+", lines[i]):
+ return i
+ return None
+
+
+def _fold_after_open_brace(lines, decl_idx, end):
+ """For a method whose declaration is at decl_idx, return (brace_line, body_indent)
+ if it has a clean BLOCK body (its `{` ends a line) — so a statement can be folded
+ in right after it. Returns None for one-liner/expression bodies (don't fold)."""
+ for i in range(decl_idx, end):
+ sk = _code_skeleton(lines[i]).rstrip()
+ if sk.endswith("{"):
+ return i, re.match(r"\s*", lines[i]).group(0) + " "
+ if "{" in sk or sk.endswith(";") or "=>" in sk: # one-liner / expr body -> skip
+ return None
+ return None
+
+
+def _fold_target(lines, cls_idx, ev):
+ """If the class already has a teardown method to fold cleanup into, return its
+ block anchor. Today: a `protected override void OnClosed(...)` for ev == Closed —
+ it runs exactly when the Window's Closed event would, so folding is equivalent and
+ cleaner than stacking another lambda. Only a member-depth OnClosed of THIS class
+ counts — a nested type's override (deeper brace depth) must not be folded into."""
+ if ev != "Closed":
+ return None
+ end = _class_close(lines, cls_idx)
+ end = end if end is not None else len(lines)
+ depth, started = 0, False
+ for i in range(cls_idx, end):
+ if started and depth == 1 and re.search(r"\boverride\s+void\s+OnClosed\b", _code_skeleton(lines[i])):
+ return _fold_after_open_brace(lines, i, end)
+ for ch in _code_skeleton(lines[i]):
+ if ch == "{":
+ depth, started = depth + 1, True
+ elif ch == "}":
+ depth -= 1
+ return None
+
+
+_MODIFIER = r"(?:public|private|protected|internal|static|readonly|const|volatile|virtual|override|sealed|new|required)"
+
+
+def _is_class_member(lines, cls_idx, root):
+ """Is `root` a member (field/property) of the class — i.e. in scope inside a method
+ body like OnClosed? `this` always is. Members carry an access/field modifier;
+ constructor parameters and locals do not, so requiring a modifier separates them.
+ Used to keep the fold sound: a ctor-local source must stay on the captured lambda."""
+ if root == "this":
+ return True
+ end = _class_close(lines, cls_idx)
+ end = end if end is not None else len(lines)
+ decl = re.compile(rf"^\s*(\[[^\]]*\]\s*)?({_MODIFIER}\s+)+[\w<>\[\].,\s]*?\b{re.escape(root)}\b\s*(=|;|\{{|=>)")
+ return any(decl.match(_code_skeleton(lines[i])) for i in range(cls_idx, end))
+
+
+def _plan_teardown(lines, f, idx, stmt, anchor_missing="site-not-found", fold_root=None):
+ """Subscription / disposable-field: run `stmt` on the owner's teardown. Folds into
+ an existing OnClosed override when present AND the source is a class member (so it
+ stays in scope there); otherwise adds a fresh teardown lambda at the call site,
+ which captures whatever is in scope (incl. ctor locals). fold_root=None ⇒ always
+ foldable (a disposable field is a member by construction)."""
  if idx is None:
  return None, anchor_missing
  if _in_unbraced_control_flow(lines, idx):
@@ -223,6 +286,11 @@ def _plan_teardown(lines, f, idx, stmt, anchor_missing="site-not-found"):
  ev = _teardown_event(decl)
  if ev is None:
  return None, "no-safe-teardown"
+ cls_idx = _enclosing_class_idx(lines, idx)
+ fold = _fold_target(lines, cls_idx, ev) if cls_idx is not None else None
+ if fold is not None and (fold_root is None or _is_class_member(lines, cls_idx, fold_root)):
+ brace_idx, body_indent = fold
+ return [(brace_idx + 1, brace_idx + 1, [f"{body_indent}{stmt};\n"])], f"{ev}/fold"
  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
@@ -265,11 +333,18 @@ def _plan_local(lines, f, name):
  return edits, "using"
 
 
-# well-known events whose delegate's EventArgs type we can name without a compiler
+# Events whose delegate's EventArgs type is UNAMBIGUOUS without a compiler — the
+# INotify* family. Names like Click/TextChanged are deliberately excluded: the same
+# name maps to different delegates across frameworks (RoutedEventArgs vs EventArgs),
+# so extracting them blindly could emit a wrong signature → they stay suggest-only.
 _EVENT_ARGS = {
- "PropertyChanged": "PropertyChangedEventArgs",
- "ListChanged": "ListChangedEventArgs",
- "CollectionChanged": "NotifyCollectionChangedEventArgs",
+ # fully qualified: the extracted method names the type explicitly, so it must
+ # compile even if the file's lambda relied on type inference without the using.
+ "PropertyChanged": "System.ComponentModel.PropertyChangedEventArgs",
+ "PropertyChanging": "System.ComponentModel.PropertyChangingEventArgs",
+ "ListChanged": "System.ComponentModel.ListChangedEventArgs",
+ "CollectionChanged": "System.Collections.Specialized.NotifyCollectionChangedEventArgs",
+ "ErrorsChanged": "System.ComponentModel.DataErrorsChangedEventArgs",
 }
 _LAMBDA2 = re.compile(r"^\(\s*(\w+)\s*,\s*(\w+)\s*\)\s*=>\s*(.+?)\s*$")
 
@@ -328,7 +403,8 @@ 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}")
+ return _plan_teardown(lines, f, _find_sub_line(lines, f.line, a), f"{a} -= {b}",
+ fold_root=a.split(".")[0]) # source's root must be in scope to fold
  if shape == DISPOSABLE_FIELD:
  return _plan_teardown(lines, f, _find_ctor_anchor(lines, f.line),
  f"{a}?.Dispose()", anchor_missing="no-ctor-anchor")

fix/fixtures/own001-sub-fold/after.findings.json

@@ -0,0 +1,3 @@
+{
+ "findings": []
+}

fix/fixtures/own001-sub-fold/before.findings.json

@@ -0,0 +1,12 @@
+{
+ "findings": [
+ {
+ "tool": "own-check",
+ "path": "Broker/FoldWindow.xaml.cs",
+ "line": 14,
+ "rule": "OWN001",
+ "category_name": "subscription-leak",
+ "message": "event 'fGoods.PropertyChanged' is subscribed (handler 'new PropertyChangedEventHandler(GoodsPropertyChanged)') but never unsubscribed; its source is an injected dependency whose lifetime is unknown, so it may outlive and keep 'FoldWindow' alive (possible leak) [resource: subscription token]"
+ }
+ ]
+}

fix/fixtures/own001-sub-fold/before/Broker/FoldWindow.xaml.cs

@@ -0,0 +1,25 @@
+using System;
+using System.ComponentModel;
+using System.Windows;
+
+namespace Sts.Broker
+{
+ public partial class FoldWindow : Window
+ {
+ private readonly Goods fGoods;
+
+ public FoldWindow(Goods goods)
+ {
+ fGoods = goods;
+ InitializeComponent();
+ fGoods.PropertyChanged += new PropertyChangedEventHandler(GoodsPropertyChanged);
+ }
+
+ protected override void OnClosed(EventArgs e)
+ {
+ base.OnClosed(e);
+ }
+
+ private void GoodsPropertyChanged(object sender, PropertyChangedEventArgs e) { }
+ }
+}

fix/tests/test_own_fix.py

@@ -113,6 +113,102 @@ def test_own014_usercontrol_inserts_unloaded_detach():
  "this.Unloaded += (s, e) => fThis.PropertyChanged -= data_PropertyChanged;")
 
 
+# ---- OWN001 subscription folds into an existing OnClosed override -----------
+
+def test_own001_subscription_folds_into_onclosed():
+ rel = "Broker/FoldWindow.xaml.cs"
+ fdir = os.path.join(FIX, "own001-sub-fold")
+ before = load_findings(os.path.join(fdir, "before.findings.json"))
+ wd = _seed("own001-sub-fold")
+ try:
+ new, applied, skipped = plan_file(os.path.join(wd, rel), before)
+ assert [d for _, d in applied] == ["Closed/fold"], (applied, skipped) # fold, not plain Closed
+ lines = new.splitlines(keepends=True)
+ oc = next(i for i, line in enumerate(lines) if "override void OnClosed" in line)
+ assert lines[oc + 1].strip() == "{"
+ assert lines[oc + 2].strip() == (
+ "fGoods.PropertyChanged -= new PropertyChangedEventHandler(GoodsPropertyChanged);")
+ assert "this.Closed += (s, e) =>" not in new # no stacked lambda
+ finally:
+ shutil.rmtree(wd, ignore_errors=True)
+
+
+def test_fold_skips_ctor_local_source():
+ # source `goods` is a ctor parameter (not a member) -> folding the raw detach into
+ # OnClosed would be out of scope; must keep the capturing lambda at the call site.
+ src = ("public partial class W : Window\n"
+ "{\n"
+ " public W(Goods goods)\n"
+ " {\n"
+ " InitializeComponent();\n"
+ " goods.PropertyChanged += new PropertyChangedEventHandler(H);\n"
+ " }\n"
+ " protected override void OnClosed(EventArgs e)\n"
+ " {\n"
+ " base.OnClosed(e);\n"
+ " }\n"
+ " private void H(object s, PropertyChangedEventArgs e) { }\n"
+ "}\n")
+ d, path = _tmp_cs(src)
+ try:
+ f = Finding("OWN001", "W.cs", 6, tool="own-check",
+ message="event 'goods.PropertyChanged' is subscribed (handler 'new PropertyChangedEventHandler(H)') ...")
+ new, applied, skipped = plan_file(path, [f])
+ assert [d for _, d in applied] == ["Closed"], (applied, skipped) # lambda, not fold
+ assert "this.Closed += (s, e) => goods.PropertyChanged -= new PropertyChangedEventHandler(H);" in new
+ # OnClosed body must be untouched (no out-of-scope detach folded in)
+ assert "goods.PropertyChanged -=" in new and new.count("goods.PropertyChanged -=") == 1
+ finally:
+ shutil.rmtree(d, ignore_errors=True)
+
+
+def test_fold_ignores_commented_onclosed():
+ # a commented-out OnClosed must not be matched as a fold anchor (skeleton-based)
+ src = ("public partial class W : Window\n"
+ "{\n"
+ " private readonly Timer _t;\n"
+ " public W()\n"
+ " {\n"
+ " InitializeComponent();\n"
+ " }\n"
+ " // protected override void OnClosed(EventArgs e) { }\n"
+ "}\n")
+ d, path = _tmp_cs(src)
+ try:
+ f = Finding("OWN001", "W.cs", 3, tool="own-check",
+ message="IDisposable field '_t' (type 'Timer') is never disposed — its owner 'W' leaks it")
+ new, applied, skipped = plan_file(path, [f])
+ assert [dt for _, dt in applied] == ["Closed"], (applied, skipped) # lambda, not a bogus fold
+ assert "this.Closed += (s, e) => _t?.Dispose();" in new
+ finally:
+ shutil.rmtree(d, ignore_errors=True)
+
+
+def test_fold_ignores_nested_type_onclosed():
+ # the field's class has no OnClosed; a NESTED type does. Must not fold into it.
+ src = ("public partial class Outer : Window\n"
+ "{\n"
+ " private readonly Timer _t;\n"
+ " public Outer()\n"
+ " {\n"
+ " InitializeComponent();\n"
+ " }\n"
+ " private class Inner\n"
+ " {\n"
+ " protected override void OnClosed(EventArgs e) { }\n"
+ " }\n"
+ "}\n")
+ d, path = _tmp_cs(src)
+ try:
+ f = Finding("OWN001", "Outer.cs", 3, tool="own-check",
+ message="IDisposable field '_t' (type 'Timer') is never disposed — its owner 'Outer' leaks it")
+ new, applied, skipped = plan_file(path, [f])
+ assert [d for _, d in applied] == ["Closed"], (applied, skipped) # lambda, not folded into Inner
+ assert "this.Closed += (s, e) => _t?.Dispose();" in new
+ finally:
+ shutil.rmtree(d, ignore_errors=True)
+
+
 # ---- OWN001 disposable field on a Window -> dispose on Closed ---------------
 
 def test_own001_disposable_field_disposes_on_closed():
@@ -153,11 +249,43 @@ def test_own001_inline_lambda_extracted_and_detached():
  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 ('private void OnStagePropertyChanged(object s2, '
+ 'System.ComponentModel.PropertyChangedEventArgs e2) '
+ '=> OnPropertyChanged("Stages");') in text # extracted, qualified args
  assert "+= (s2, e2) =>" not in text # the lambda is gone
 
 
+# ---- wider lambda-extraction delegates (PropertyChanging / ErrorsChanged) --
+
+def test_lambda_extraction_more_delegates():
+ # both newly-added INotify-family events are unambiguous -> extractable with the
+ # right (fully-qualified) args type.
+ cases = [
+ ("PropertyChanging", "System.ComponentModel.PropertyChangingEventArgs", "OnMPropertyChanging"),
+ ("ErrorsChanged", "System.ComponentModel.DataErrorsChangedEventArgs", "OnMErrorsChanged"),
+ ]
+ for ev, args_type, method in cases:
+ src = ("public partial class W : Window\n"
+ "{\n"
+ " public W(Model m)\n"
+ " {\n"
+ " InitializeComponent();\n"
+ f" m.{ev} += (s, e) => Refresh();\n"
+ " }\n"
+ " private void Refresh() { }\n"
+ "}\n")
+ d, path = _tmp_cs(src)
+ try:
+ f = Finding("OWN001", "W.cs", 6, tool="own-check",
+ message=f"event 'm.{ev}' is subscribed (handler '(s, e) => Refresh()') ...")
+ new, applied, skipped = plan_file(path, [f])
+ assert [dt for _, dt in applied] == ["extract+detach"], (ev, skipped)
+ assert f"m.{ev} += {method};" in new
+ assert f"private void {method}(object s, {args_type} e) => Refresh();" in new
+ finally:
+ shutil.rmtree(d, ignore_errors=True)
+
+
 # ---- refused shapes stay suggest-only: NOT patched -------------------------
 
 def test_block_lambda_is_not_patched():