Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ jobs:
frontend/roslyn/samples/ExternalRefSubscription.cs \
frontend/roslyn/samples/StaticHandlerViewModel.cs \
frontend/roslyn/samples/StaticEventEscapeViewModel.cs \
frontend/roslyn/samples/WhenAnyValueViewModel.cs \
frontend/roslyn/samples/SampleTypes.cs \
-o "$RUNNER_TEMP/facts.json"
cat "$RUNNER_TEMP/facts.json"
Expand Down Expand Up @@ -233,6 +234,25 @@ jobs:
# subscription must still be reported, not silently suppressed.
echo "$out" | grep -qE "ExternalRefSubscription\.cs:[0-9]+: warning: \[OWN001\]" \
|| { echo "FAIL: expected OWN001 on the external-ref subscription (must not be exempted)"; exit 1; }
# P-004 self-WhenAnyValue classifier (docs/notes/self-whenany-precision.md):
# `this.WhenAnyValue(p => p.SelfProp[, q => q.Other]).Subscribe` over the
# component's OWN single-hop properties is a collectable self-cycle ->
# silent; a nested path through an INJECTED object, or a combinator that
# mixes in an EXTERNAL observable, stays a flagged leak (OWN001).
echo "$out" | grep -q "x.Svc.Name" \
|| { echo "FAIL: nested-path WhenAnyValue (injected Svc) must leak"; exit 1; }
echo "$out" | grep -q "CombineLatest" \
|| { echo "FAIL: combinator WhenAnyValue (external observable) must leak"; exit 1; }
# the multi-arg single-hop self chain must be SILENCED (the fix): `x => x.B`
# appears only in that chain, so it must not surface anywhere.
if echo "$out" | grep -q "x => x.B"; then
echo "FAIL: multi-arg single-hop self WhenAnyValue must be silenced"; exit 1
fi
# exactly two WhenAnyValueViewModel leaks (nested + combinator) — the three
# self-rooted chains produce nothing.
n=$(echo "$out" | grep -cE "WhenAnyValueViewModel\.cs:[0-9]+:.*\[OWN001\]")
[ "$n" = "2" ] \
|| { echo "FAIL: expected exactly 2 WhenAnyValueViewModel leaks, got $n"; exit 1; }
# P-004 static-handler exemption: a static-method handler has a null
# delegate target — no instance retained, so not a leak — silent.
if echo "$out" | grep -q "StaticHandlerViewModel.cs"; then
Expand Down
124 changes: 124 additions & 0 deletions docs/notes/self-whenany-precision.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
# Self-`WhenAnyValue` precision — the own-only subscription findings are (mostly) real

A dig into the honest caveat on the WalletWasabi oracle run — *"own-only may
include residual self-`WhenAnyValue` false positives"* — by checking the actual
target source. The headline: **most of the own-only nested findings are real
leaks, not FPs.** The genuine residual FP is one narrow shape.

## The run

Oracle `27839799505`, **WalletWasabi @ `b67080d`**, scope `WalletWasabi.Fluent`,
via the SARIF own-check (#43), ~5.5 min. **2-way** — Infer# was skipped
(`WalletWasabi.Fluent` does not build for Infer# on the Linux runner), so this is
**Own.NET vs CodeQL**. Leak-class file overlap:

| bucket | files | what |
|---|---:|---|
| Own.NET only | 34 | reactive subscription leaks (`WhenAnyValue(…).Subscribe` ignored) |
| CodeQL only | 20 | Dispose/RAII (`cs/dispose-not-called-on-throw`, `cs/local-not-disposed`) — 48 findings |
| Agree | 3 | `historyviewmodel.cs`, `mainviewmodel.cs`, `qrcodereader.cs` |

CodeQL flags **zero** subscription leaks (it has no "subscribed, never disposed"
query); its other **932** findings are all *quality* queries (missed-readonly,
catch-all, path-combine, …), outside the leak class. The tools are complementary
and near-disjoint — the documented differentiation, on real cross-tool output.

## The dig: are the own-only `WhenAnyValue` findings FPs?

The extractor silences **only** `this.WhenAnyValue(p => p.Member)
.<self-preserving ops>.Subscribe` — single-arg, single-**hop** self property — as
a collectable self-cycle (`source: "self"`); everything else stays flagged. So the
own-only findings are the shapes it does *not* silence:

- a **nested** path — `x => x.Settings.Foo`
- **multi-arg** — `x => x.A, x => x.B`
- an **unrecognised op** — e.g. `.ToSignal()`

To separate FP from real leak, I pulled the actual WalletWasabi source.

### Nested `x => x.Settings.Foo` → a **real leak**, not an FP

`BitcoinTabSettingsViewModel` — `Settings` is a **constructor-injected
`ApplicationSettings`**:

```csharp
public BitcoinTabSettingsViewModel(UiContext uiContext, ApplicationSettings settings)
public ApplicationSettings Settings { get; } // = settings;

this.WhenAnyValue(x => x.Settings.BitcoinRpcUri)
.Subscribe(x => BitcoinRpcUri = x); // result ignored, no DisposeWith
```

`WhenAnyValue(x => x.Settings.BitcoinRpcUri)` attaches a `PropertyChanged` handler
to `this.Settings` — the **long-lived injected `ApplicationSettings`** — and the
handler closes over `this`. So the app-wide settings object keeps the transient
settings-tab view-model alive: **a real subscription leak.** The classifier is
*correctly conservative* here — a nested path can observe an injected/shared
object, so it must not be silenced. The same holds for `CoordinatorTabSettings­
ViewModel` (also `x => x.Settings.…` over the injected `ApplicationSettings`) and
`SettingsPageViewModel` (`x => x.UiContext.ApplicationSettings.DarkModeEnabled`).

**So most own-only nested findings are real leaks — the differentiation is
*stronger* than the caveat implied, not weaker.**

### Multi-arg single-hop own (`x => x.A, x => x.B`) → the genuine residual FP

`BitcoinTabSettingsViewModel.cs:66`:

```csharp
this.WhenAnyValue(x => x.BitcoinRpcUri, x => x.BitcoinRpcCredentialString)
.Subscribe(…);
```

Both selectors are **single-hop OWN properties**. `WhenAnyValue` over single-hop
own properties observes only `this`, so the observable, its handler and `this`
form one cycle the GC collects together — a self-cycle, **not** a leak. The
classifier misses it *solely* because it requires `Arguments.Count == 1`. → a
genuine false positive.

## The fix (proposed)

Generalise `IsSelfRootedWhenAny` from "one single-hop self selector" to "**one or
more** single-hop self selectors" — multi-arg `WhenAnyValue` over own properties
roots at `this` exactly as a single one does:

```csharp
// A WhenAnyValue selector `p => p.Member` rooted at the lambda parameter (a
// single-hop self property — not `p => p.A.B`, not a result-combiner lambda).
static bool IsSelfMemberSelector(ArgumentSyntax arg) =>
arg.Expression is SimpleLambdaExpressionSyntax lam
&& lam.Body is MemberAccessExpressionSyntax body
&& body.Expression is IdentifierNameSyntax pid
&& pid.Identifier.Text == lam.Parameter.Identifier.Text;

// (no System.Linq dependency — it is not imported in Program.cs)
static bool AllSelfMemberSelectors(SeparatedSyntaxList<ArgumentSyntax> args)
{
if (args.Count < 1) return false;
foreach (var a in args)
if (!IsSelfMemberSelector(a)) return false;
return true;
}

// in IsSelfRootedWhenAny, replace the single-arg branch with:
return ma.Name.Identifier.Text == "WhenAnyValue"
&& AllSelfMemberSelectors(iv.ArgumentList.Arguments);
```

Soundness is unchanged — N single-hop own-property selectors root at `this`
identically to one. **Nested paths and the result-combiner overload
(`…, (a, b) => …`) stay flagged** (conservative: they can observe an injected
object). This silences the `BitcoinTabSettingsViewModel.cs:66`-class FP and leaves
every real injected-`Settings` leak untouched.

## Why it is not done here (the coverage gap)

`IsSelfRootedWhenAny` is currently exercised **only by the mine/oracle runs on
real code** — there is **no `WhenAnyValue` sample** under
`frontend/roslyn/samples/`, so the `wpf-extractor` CI job does not cover it, and
there is no local .NET SDK to validate a change. The fix should therefore land
*with* a `WhenAnyValueViewModel.cs` sample wired into the `wpf-extractor` job —
asserting: single-arg `x => x.Member` silenced, **multi-arg `x => x.A, x => x.B`
silenced**, nested `x => x.Svc.Prop` flagged, and a combinator (`CombineLatest`)
flagged — so the classifier finally gets a CI regression pin and the change is
validated where the frontend always is: in CI, not blind.
33 changes: 24 additions & 9 deletions frontend/roslyn/OwnSharp.Extractor/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,16 @@
or "WhereNotNull" or "Cast" or "OfType" or "StartWith" or "Scan"
or "Finally" or "AsObservable" or "Synchronize" or "Timestamp";

// One WhenAnyValue selector `p => p.Member` rooted at the lambda parameter — a
// single-hop self property. NOT `p => p.A.B` (a path through a possibly-injected
// object that can keep `this` alive — a real leak), and NOT a result-combiner
// lambda (`(a, b) => ...`). Purely syntactic.
static bool IsSelfMemberSelector(ArgumentSyntax arg) =>
arg.Expression is SimpleLambdaExpressionSyntax lam
&& lam.Body is MemberAccessExpressionSyntax body
&& body.Expression is IdentifierNameSyntax pid
&& pid.Identifier.Text == lam.Parameter.Identifier.Text;

static bool IsSelfRootedWhenAny(ExpressionSyntax chain)
{
// Walk the fluent chain leftwards. The HEAD must be `this.WhenAnyValue(...)`;
Expand All @@ -266,15 +276,20 @@
{
if (ma.Expression is ThisExpressionSyntax)
{
// Head: `this.<op>(...)`. A self-cycle requires WhenAnyValue with a
// single-hop self-member lambda (`p => p.Member`, not `p => p.A.B`,
// not multi-arg).
return ma.Name.Identifier.Text == "WhenAnyValue"
&& iv.ArgumentList.Arguments.Count == 1
&& iv.ArgumentList.Arguments[0].Expression is SimpleLambdaExpressionSyntax lam
&& lam.Body is MemberAccessExpressionSyntax body
&& body.Expression is IdentifierNameSyntax pid
&& pid.Identifier.Text == lam.Parameter.Identifier.Text;
// Head: `this.WhenAnyValue(p => p.Member[, q => q.Other, ...])`. A
// self-cycle requires WhenAnyValue with one-or-more single-hop
// self-member selectors — each roots at `this`. `p => p.A.B` (a path
// through a possibly-injected object) and a result-combiner overload
// (`..., (a, b) => ...`) stay flagged. Multi-arg over own properties
// observes only `this`, the SAME self-cycle as single-arg (see
// docs/notes/self-whenany-precision.md).
if (ma.Name.Identifier.Text != "WhenAnyValue"
|| iv.ArgumentList.Arguments.Count < 1)
return false;
foreach (var arg in iv.ArgumentList.Arguments)
if (!IsSelfMemberSelector(arg))
return false;
return true;
}
if (!IsSelfPreservingOp(ma.Name.Identifier.Text))
return false; // a combinator / unknown op -> possibly external
Expand Down Expand Up @@ -745,7 +760,7 @@
.Split(Path.PathSeparator, StringSplitOptions.RemoveEmptyEntries)
.Where(p => p.EndsWith(".dll", StringComparison.OrdinalIgnoreCase))
.ToList();
var refNames = new HashSet<string>(tpa.Select(Path.GetFileName), StringComparer.OrdinalIgnoreCase);

Check warning on line 763 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / C# leak extractor (Roslyn) -> OwnIR -> core

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 763 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / C# leak extractor (Roslyn) -> OwnIR -> core

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 763 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 763 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.
var references = tpa.Select(p => (MetadataReference)MetadataReference.CreateFromFile(p)).ToList();
// P-004 WPF profile: widen the reference set with assemblies named by the
// OWN_EXTRA_REF_DIRS env var (colon-separated dirs) — e.g. the WindowsDesktop ref
Expand Down
54 changes: 54 additions & 0 deletions frontend/roslyn/samples/WhenAnyValueViewModel.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
using System;

// Self-rooted `this.WhenAnyValue(...).Subscribe(...)` classifier (P-004 / WPF004).
// An ignored Subscribe over a chain rooted at `this`'s OWN single-hop properties is
// a GC-collectible self-cycle — the observable, its handler and `this` collect
// together — so the extractor tags `source: "self"` and the core stays SILENT. A
// nested path through an injected object, or a combinator that mixes in an external
// observable, can keep `this` alive and stays a FLAGGED leak (OWN001).
//
// The detection is purely syntactic, so the ReactiveUI surface (`WhenAnyValue`,
// `Where`, `CombineLatest`, `Subscribe`) need not be referenced — the chains only
// have to parse. See docs/notes/self-whenany-precision.md.

namespace Sample
{
public sealed class WhenAnyValueViewModel
{
public string A { get; set; } = "";
public string B { get; set; } = "";
public AppSettings Svc { get; } // injected, long-lived dependency
private readonly Feed _bus;

public WhenAnyValueViewModel(AppSettings svc, Feed bus)
{
Svc = svc;
_bus = bus;

// SILENCED — single-hop self property (the original supported shape).
this.WhenAnyValue(x => x.A).Subscribe(v => A = v);

// SILENCED — multi-arg, every selector a single-hop self property. This
// observes only `this`, the same self-cycle as a single selector. The
// classifier used to miss it solely because it required one argument.
this.WhenAnyValue(x => x.A, x => x.B).Subscribe(t => { });

// SILENCED — single-hop self + a self-preserving operator chain.
this.WhenAnyValue(x => x.A).Where(v => v.Length > 0).Subscribe(v => B = v);

// FLAGGED — nested path through the injected `Svc`: the PropertyChanged
// handler attaches to the long-lived settings object and keeps `this`
// alive. A real leak, not a self-cycle.
this.WhenAnyValue(x => x.Svc.Name).Subscribe(v => A = v);

// FLAGGED — a combinator mixes in an external observable (`_bus.Stream`),
// rooting the subscription outside `this`.
this.WhenAnyValue(x => x.A).CombineLatest(_bus.Stream).Subscribe(t => { });
}
}

// Minimal in-sample stand-ins so the chains resolve cleanly (no ReactiveUI).
public sealed class AppSettings { public string Name { get; set; } = ""; }

public sealed class Feed { public IObservable<string> Stream => null!; }
}
Loading