diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bb04d95b..6e78134e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -206,6 +206,7 @@ jobs: frontend/roslyn/samples/VoidSubscribeSample.cs \ frontend/roslyn/samples/ReturnedPublisherSample.cs \ frontend/roslyn/samples/OwnIgnoreSample.cs \ + frontend/roslyn/samples/DpRotationSample.cs \ -o "$RUNNER_TEMP/facts.json" cat "$RUNNER_TEMP/facts.json" - name: Check facts through the core @@ -753,7 +754,25 @@ jobs: "$RUNNER_TEMP/own.sarif" >/dev/null \ || { echo "FAIL: SARIF must carry SuppressedLeak once, with an inSource suppressions justification"; \ jq '.runs[0].results[]|{ruleId,component:.properties.component,suppressions}' "$RUNNER_TEMP/own.sarif"; exit 1; } - echo "OK: real C# -> facts -> OWN001 (subscription + timer + field + Subscribe + pool + local) + OWN014 (static-event region escape) + DI001 (captive dependency) + DI002 (scoped captured weakly) + DI003 (transient IDisposable captured by a singleton) + DI004 (transient IDisposable service-located from the root provider) + DI005 (scoped service cached from a created scope) + [OwnIgnore] suppression (silent-but-counted, SARIF suppressions) at the C# location" + # issue #218 — DP/property-changed old->new subscription ROTATION (unsub OLD, sub NEW, same + # handler = one paired lifecycle) must be SILENT. Two recognised forms: a DP callback reading + # e.OldValue/e.NewValue (CommandTriggerAction), and a plain virtual OnXChanged(old, new) + # override with two same-type params (AbstractMargin). Confirmed FP in 3 real repos, 6+ sites. + # InlineCastRotation covers the direct inline-cast receiver `((ICommand)e.NewValue!).Event +=` + # (OldValue/NewValue are object-typed, so the cast is mandatory) — Codex review catch. + if echo "$out" | grep -qE "'CommandTriggerAction'|'AbstractMargin'|'InlineCastRotation'"; then + echo "FAIL: a DP old->new subscription rotation was wrongly flagged as a leak (#218)"; exit 1 + fi + # ...and it must NOT over-widen — three controls STAY flagged: (a) the += uses a DIFFERENT + # handler than the -= (a genuine unpaired subscription); (b) two UNRELATED, differently-typed + # params (not the old/new halves of one change); (c) `-=` on one class FIELD, `+=` on another. + echo "$out" | grep -qE "\[OWN001\].*'MismatchedHandlerRotation'" \ + || { echo "FAIL: a rotation with a DIFFERENT += handler must stay flagged (no over-widen)"; exit 1; } + echo "$out" | grep -qE "\[OWN001\].*'UnrelatedPairRotation'" \ + || { echo "FAIL: a -=/+= pair on differently-typed params must stay flagged (not old/new halves)"; exit 1; } + echo "$out" | grep -qE "\[OWN001\].*'TwoFieldsRotation'" \ + || { echo "FAIL: a -=/+= pair across two class fields must stay flagged (not a rotation)"; exit 1; } + echo "OK: real C# -> facts -> OWN001 (subscription + timer + field + Subscribe + pool + local) + OWN014 (static-event region escape) + DI001 (captive dependency) + DI002 (scoped captured weakly) + DI003 (transient IDisposable captured by a singleton) + DI004 (transient IDisposable service-located from the root provider) + DI005 (scoped service cached from a created scope) + [OwnIgnore] suppression (silent-but-counted, SARIF suppressions) + #218 DP old->new subscription rotation (silent; controls flagged) at the C# location" - name: Flow-sensitive local IDisposables (--flow-locals, P-016 B0b/B2) run: | # Path-sensitive flow analysis of local IDisposables — bugs the flat D1 diff --git a/docs/notes/field-notes-patterns.md b/docs/notes/field-notes-patterns.md index 579ad234..4f36b739 100644 --- a/docs/notes/field-notes-patterns.md +++ b/docs/notes/field-notes-patterns.md @@ -382,6 +382,22 @@ call's value. **Rule of thumb: when a `+=`/`-=` pair straddles the old/new halve a property-changed callback (or an equivalent `OnChanged(old, new)` override), treat them as one paired lifecycle, not two independent, unpaired subscriptions.** +**Shipped (issue #218).** The extractor now recognises the rotation and stamps the +`+=` on the new half as `released` — so the core sees a balanced acquire/release and +stays silent (no new OwnIR fact, no `OWNIR_VERSION` bump; it reuses the existing +`released` field). A `+=` on `.` with handler `H` is paired when the +same method holds a `-=` on `.` with the same `H`, and `(oldRecv, +newRecv)` are the **old/new halves of one change**: either bound from a +`DependencyPropertyChangedEventArgs` `e.OldValue`/`e.NewValue` (directly, or via +`e.OldValue is T old` / `(T)e.OldValue` / `var old = e.OldValue`), or two **same-type +parameters** of the enclosing method with old positioned before new (the +`OnXChanged(T old, T new)` override — matched by the param-pair shape, not the method +name). It deliberately does **not** widen: a `+=` with a *different* handler than the +`-=`, a pair on two class **fields**, or on **differently-typed** params are not a +rotation and stay flagged. Pinned by `frontend/roslyn/samples/DpRotationSample.cs` +(`CommandTriggerAction`/`AbstractMargin` silent; `MismatchedHandlerRotation`/ +`UnrelatedPairRotation`/`TwoFieldsRotation` flagged) in the `wpf-extractor` CI job. + ## 12. WinForms `Controls`/`ToolStripItemCollection` membership as a disposal channel **Seen in:** ShareX `ShareX.HelpersLib/Forms/ImageViewer.cs` (`pbPreview`, diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 208d4fc4..505aae99 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -486,6 +486,106 @@ static bool IsTimerEvent(ExpressionSyntax left) => left is MemberAccessExpressionSyntax m && (m.Name.Identifier.Text == "Tick" || m.Name.Identifier.Text == "Elapsed"); +// Issue #218 — DP/property-changed old->new subscription ROTATION. A property-changed callback (a +// `PropertyChangedCallback` reading `e.OldValue`/`e.NewValue`, or a virtual `OnXChanged(T old, T new)` +// override) detaches the SAME handler from the OLD value and re-attaches it to the NEW one across a +// value change. The extractor pairs `-=`/`+=` per source VARIABLE, so the `+=` on the new half looks +// unpaired and false-flags OWN001 — yet at most one subscription is ever live. This treats the `+=` +// on the new half as RELEASED when its matching `-=` sits on the OLD half of the SAME change: same +// event member, same handler, same method. Precise by construction (see IsOldNewPair) — it never +// pairs a `-=`/`+=` on unrelated sources (two fields, differently-typed params), which stay flagged. +static bool IsRotationPairedRelease(AssignmentExpressionSyntax add, SemanticModel model) +{ + if (add.Left is not MemberAccessExpressionSyntax addLhs) + return false; // only `receiver.EventMember += H` + var eventMember = addLhs.Name.Identifier.Text; // e.g. CanExecuteChanged + var newRecv = addLhs.Expression; // the NEW half's receiver + var handler = NormalizeHandler(add.Right).ToString(); + if (add.FirstAncestorOrSelf() is not { } method) + return false; // a rotation lives in one callback + foreach (var node in method.DescendantNodes().OfType()) + if (node.IsKind(SyntaxKind.SubtractAssignmentExpression) + && node.Left is MemberAccessExpressionSyntax subLhs + && subLhs.Name.Identifier.Text == eventMember // same event member + && NormalizeHandler(node.Right).ToString() == handler // same handler + && IsOldNewPair(subLhs.Expression, newRecv, method, model)) // old/new halves of one change + return true; + return false; +} + +// Are (oldRecv, newRecv) — the receivers of a `-=`/`+=` on the same event+handler — the OLD/NEW +// halves of ONE property change? Two provable forms, nothing wider: +// (1) DependencyPropertyChangedEventArgs: `e.OldValue` / `e.NewValue` — used directly, or bound to +// a local (`e.OldValue is T old`, `(T)e.OldValue`, `var old = e.OldValue`). +// (2) two parameters of the enclosing method of the SAME type, OLD positioned before NEW — the +// `OnXChanged(T oldValue, T newValue)` override, matched by the param-pair shape, not by name. +// Two class fields, differently-typed parameters, or unrelated locals are NOT a pair -> stay flagged. +static bool IsOldNewPair(ExpressionSyntax oldRecv, ExpressionSyntax newRecv, + BaseMethodDeclarationSyntax method, SemanticModel model) +{ + // (1) direct e.OldValue / e.NewValue receivers — including the inline CAST form + // `((ICommand)e.NewValue).CanExecuteChanged += H` (OldValue/NewValue are object-typed, so a + // cast is mandatory to reach the event), which is the common way to write this rotation. + if (IsDpValueAccess(StripCasts(oldRecv), "OldValue") && IsDpValueAccess(StripCasts(newRecv), "NewValue")) + return true; + var oldSym = model.GetSymbolInfo(oldRecv).Symbol; + var newSym = model.GetSymbolInfo(newRecv).Symbol; + if (oldSym is null || newSym is null + || SymbolEqualityComparer.Default.Equals(oldSym, newSym)) + return false; // unresolved, or the SAME source (that is the existing per-variable pairing) + // (1') locals bound from e.OldValue / e.NewValue. + if (BoundFromDpValue(oldSym, "OldValue", method, model) + && BoundFromDpValue(newSym, "NewValue", method, model)) + return true; + // (2) two SAME-TYPE parameters, old before new. + return oldSym is IParameterSymbol op && newSym is IParameterSymbol np + && SymbolEqualityComparer.Default.Equals(op.Type, np.Type) + && op.Ordinal < np.Ordinal; +} + +// `.OldValue` / `.NewValue` member access — the DependencyPropertyChangedEventArgs +// halves. Matched by member name (those two members ARE the canonical DP-changed shape); the +// extractor is syntactic by design, so this stays robust when the WPF ref is absent. +static bool IsDpValueAccess(ExpressionSyntax expr, string member) => + expr is MemberAccessExpressionSyntax ma && ma.Name.Identifier.Text == member; + +// Was the local/pattern variable `sym` bound from `e.` (OldValue/NewValue) inside `method`? +// Recognises a declaration pattern (`e.OldValue is T sym`) and a declarator initializer +// (`var sym = e.OldValue`, `T sym = (T)e.OldValue`) — the ways a callback names its old/new half. +static bool BoundFromDpValue(ISymbol sym, string member, + BaseMethodDeclarationSyntax method, SemanticModel model) +{ + foreach (var node in method.DescendantNodes()) + { + if (node is SingleVariableDesignationSyntax svd + && svd.Parent is DeclarationPatternSyntax { Parent: IsPatternExpressionSyntax ip } + && IsDpValueAccess(StripCasts(ip.Expression), member) + && SymbolEqualityComparer.Default.Equals(model.GetDeclaredSymbol(svd), sym)) + return true; + if (node is VariableDeclaratorSyntax { Initializer: { } init } vd + && IsDpValueAccess(StripCasts(init.Value), member) + && SymbolEqualityComparer.Default.Equals(model.GetDeclaredSymbol(vd), sym)) + return true; + } + return false; +} + +// Peel enclosing casts / parentheses / the null-forgiving `!` so `(ICommand)e.OldValue`, +// `(e.OldValue)` and `((ICommand)e.NewValue!)` all reach the underlying `e.OldValue`/`e.NewValue`. +static ExpressionSyntax StripCasts(ExpressionSyntax e) +{ + while (true) + switch (e) + { + case CastExpressionSyntax c: e = c.Expression; continue; + case ParenthesizedExpressionSyntax p: e = p.Expression; continue; + case PostfixUnaryExpressionSyntax s + when s.IsKind(SyntaxKind.SuppressNullableWarningExpression): + e = s.Operand; continue; + default: return e; + } +} + // P-004 self-owned exemption: is the event SOURCE owned by (and so never longer- // lived than) the subscriber? True for a bare instance event on `this`, or a // receiver that resolves to a field/local the class OWNS. "Owns" is the `owned` @@ -3819,7 +3919,11 @@ or ImplicitObjectCreationExpressionSyntax if (!isTimer && source == "static" && HandlerRetainsNoInstance(a.Right, model)) continue; var released = unsub.Contains($"{a.Left}|{NormalizeHandler(a.Right)}") - || (isTimer && Receiver(a.Left) is { } recv && stopped.Contains(recv)); + || (isTimer && Receiver(a.Left) is { } recv && stopped.Contains(recv)) + // #218: the `+=` on the NEW half of a property-changed old->new rotation whose + // `-=` is on the OLD half (same event member, same handler, same method) — a + // single paired lifecycle, not an unpaired leak. + || IsRotationPairedRelease(a, model); // #146 — interprocedural publisher provenance: a `+=` on a PARAMETER // publisher stays `injected` (locally honest — param→param syntax is // identical for a DI singleton bus, a real leak, and for the diff --git a/frontend/roslyn/samples/DpRotationSample.cs b/frontend/roslyn/samples/DpRotationSample.cs new file mode 100644 index 00000000..a51987e5 --- /dev/null +++ b/frontend/roslyn/samples/DpRotationSample.cs @@ -0,0 +1,126 @@ +using System; + +namespace Own.Samples; + +// Issue #218 — DP/property-changed old->new subscription ROTATION must not be flagged as a leak: +// a callback detaches the SAME handler from the OLD value and re-attaches it to the NEW one across +// a value change, so at most one subscription is ever live (no leak). Two recognised forms plus +// three negative controls that must STAY flagged (proving the fix does not over-widen). Stand-in +// types (the extractor build has no WPF ref) mirror the real cases: MahApps CommandTriggerAction, +// MaterialDesign SmartHint, AvalonEdit margins. + +public interface ICommand +{ + event EventHandler CanExecuteChanged; +} + +// A different type that also exposes CanExecuteChanged — used by a negative control so the +// discriminator is the parameter *types*, not the event/handler names. +public interface IOtherCommand +{ + event EventHandler CanExecuteChanged; +} + +// Stand-in for System.Windows.DependencyPropertyChangedEventArgs (matched syntactically by its +// OldValue/NewValue members, the canonical DP-changed shape). +public sealed class DependencyPropertyChangedEventArgs +{ + public object? OldValue { get; init; } + public object? NewValue { get; init; } +} + +public class TextView +{ + public event EventHandler VisualLinesChanged = delegate { }; +} + +// FORM (1) — DependencyProperty callback, pattern-cast old/new (the MahApps CommandTriggerAction +// shape). unsub OLD, sub NEW, same handler -> one paired lifecycle -> SILENT. +public sealed class CommandTriggerAction +{ + private void OnCommandCanExecuteChanged(object? sender, EventArgs e) { } + + private static void OnCommandChanged(CommandTriggerAction action, DependencyPropertyChangedEventArgs e) + { + if (e.OldValue is ICommand oldCommand) + oldCommand.CanExecuteChanged -= action.OnCommandCanExecuteChanged; // unsub OLD + if (e.NewValue is ICommand newCommand) + newCommand.CanExecuteChanged += action.OnCommandCanExecuteChanged; // sub NEW -> paired (SILENT) + } +} + +// FORM (1, inline cast) — DP callback that subscribes DIRECTLY on the cast DP value, no intermediate +// local: `((ICommand)e.NewValue!).CanExecuteChanged += H` paired with the OldValue `-=`. Since +// OldValue/NewValue are object-typed the cast is mandatory, so this direct-cast form is a common way +// to write the rotation -> SILENT. (Codex review: strip casts / `!` before the direct DP-receiver check.) +public sealed class InlineCastRotation +{ + private void OnCanExecuteChanged(object? sender, EventArgs e) { } + + private void OnCommandChanged(DependencyPropertyChangedEventArgs e) + { + ((ICommand)e.OldValue!).CanExecuteChanged -= OnCanExecuteChanged; // unsub OLD (inline cast) + ((ICommand)e.NewValue!).CanExecuteChanged += OnCanExecuteChanged; // sub NEW (inline cast) -> paired (SILENT) + } +} + +// FORM (2) — plain virtual OnXChanged(old, new) override, two SAME-type parameters (the AvalonEdit +// AbstractMargin shape — not even a DP callback). unsub param0, sub param1, same handler -> SILENT. +public class AbstractMargin +{ + private void OnVisualLinesChanged(object? sender, EventArgs e) { } + + protected virtual void OnTextViewChanged(TextView oldTextView, TextView newTextView) + { + if (oldTextView != null) + oldTextView.VisualLinesChanged -= OnVisualLinesChanged; // unsub OLD (param 0) + if (newTextView != null) + newTextView.VisualLinesChanged += OnVisualLinesChanged; // sub NEW (param 1) -> paired (SILENT) + } +} + +// NEGATIVE CONTROL (a) — the rotation shape but a DIFFERENT handler on the += than the -=: the new +// subscription is genuinely unpaired (the -= detached a different delegate) -> STAYS FLAGGED. +public sealed class MismatchedHandlerRotation +{ + private void HandlerA(object? sender, EventArgs e) { } + private void HandlerB(object? sender, EventArgs e) { } + + private static void OnChanged(MismatchedHandlerRotation self, DependencyPropertyChangedEventArgs e) + { + if (e.OldValue is ICommand oldCommand) + oldCommand.CanExecuteChanged -= self.HandlerA; // unsub OLD with HandlerA + if (e.NewValue is ICommand newCommand) + newCommand.CanExecuteChanged += self.HandlerB; // sub NEW with HandlerB (different!) -> LEAK + } +} + +// NEGATIVE CONTROL (b) — a rotation-LIKE pair outside a property-changed context: two UNRELATED +// parameters of DIFFERENT types (same event/handler names, but not the old/new halves of one value +// change) -> the += is a real unpaired subscription -> STAYS FLAGGED. +public sealed class UnrelatedPairRotation +{ + private void Handler(object? sender, EventArgs e) { } + + private void Wire(ICommand primary, IOtherCommand secondary) + { + primary.CanExecuteChanged -= Handler; // unsub on primary (ICommand) + secondary.CanExecuteChanged += Handler; // sub on secondary (IOtherCommand — different type) -> LEAK + } +} + +// NEGATIVE CONTROL (c) — `-=` on one class FIELD, `+=` on another (same event + handler). Fields are +// not the old/new halves of a change; this is two independent subscriptions -> STAYS FLAGGED. +public sealed class TwoFieldsRotation +{ + private ICommand _a = default!; + private ICommand _b = default!; + + private void Handler(object? sender, EventArgs e) { } + + private void Swap() + { + _a.CanExecuteChanged -= Handler; // unsub on field _a + _b.CanExecuteChanged += Handler; // sub on field _b (not an old/new pair) -> LEAK + } +}