diff --git a/fix/README.md b/fix/README.md index a24cf63..dc0454a 100644 --- a/fix/README.md +++ b/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`, custom `*Changed`). - 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 aa31367..60f9b60 100644 --- a/fix/fixarm/own_fix.py +++ b/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") diff --git a/fix/fixtures/own001-sub-fold/after.findings.json b/fix/fixtures/own001-sub-fold/after.findings.json new file mode 100644 index 0000000..2ef5648 --- /dev/null +++ b/fix/fixtures/own001-sub-fold/after.findings.json @@ -0,0 +1,3 @@ +{ + "findings": [] +} diff --git a/fix/fixtures/own001-sub-fold/before.findings.json b/fix/fixtures/own001-sub-fold/before.findings.json new file mode 100644 index 0000000..5be6ea8 --- /dev/null +++ b/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]" + } + ] +} diff --git a/fix/fixtures/own001-sub-fold/before/Broker/FoldWindow.xaml.cs b/fix/fixtures/own001-sub-fold/before/Broker/FoldWindow.xaml.cs new file mode 100644 index 0000000..ec222f7 --- /dev/null +++ b/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) { } + } +} diff --git a/fix/tests/test_own_fix.py b/fix/tests/test_own_fix.py index 88ffe5a..eab3e25 100644 --- a/fix/tests/test_own_fix.py +++ b/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():