Skip to content

Commit 74fcbc1

Browse files
committed
review: harden self-owned-property exemption + baseline parsing (PR #156)
Addresses the Codex/CodeRabbit review on #156: - IsSelfOwnedSource: gate the get-only-property exemption to a THIS/bare access (`Channel` / `this.Channel`). `other.Channel.Event += h` reaches another instance's property, which may outlive this subscriber, so it stays flagged (Codex P2). - PropertyReturnsOwnedMember: require the returned member's CONTAINING TYPE to be the analyzed class, not just a name match — a same-named field on another type no longer satisfies ownership (CodeRabbit, major). - RefExchangeNulledField: also accept `default(T)` (DefaultExpressionSyntax), not only the `default` literal — `Interlocked.Exchange(ref _f, default(Timer))` was missed (CodeRabbit, minor). - oracle_compare _load_baseline: reject an empty message-substring (it would make `substr in message` a wildcard a typo could turn into a blanket suppressor), and preserve `|` in the reason via join (CodeRabbit, major). Selftest extended (31 checks). - field-dispose-via-helper/notes.md: drop the obsolete caveat — the Interlocked.Exchange teardown is now covered by the field-dispose-via-exchange fixture, not baselined (CodeRabbit, minor). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
1 parent edbf203 commit 74fcbc1

3 files changed

Lines changed: 42 additions & 25 deletions

File tree

‎corpus/real-world/field-dispose-via-helper/notes.md‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,11 @@ parameter. So an unknown or borrowing callee never credits a release.
3131

3232
This clears the NLog variants where the sink is called on the field **directly** or
3333
through a simple `var t = _field;` alias (`AsyncTaskTarget`, `AsyncTargetWrapper`,
34-
`BufferingTargetWrapper`). It does **not** reach `TimeoutContinuation`, where the
35-
receiver is the result of `Interlocked.Exchange(ref _timer, null)` — an
36-
exchange-and-null-out idiom whose result is not a tracked field alias. That one
37-
stays in the oracle FP baseline
38-
([`corpus/oracle-fp-baseline.txt`](../../oracle-fp-baseline.txt),
39-
[`docs/notes/oracle-known-fps.md`](../../../docs/notes/oracle-known-fps.md)) until
40-
ref-exchange alias tracking lands. Scope is intentionally limited to **extension
41-
methods**: an instance method disposing its own `this` is not a real
42-
dispose-delegation shape and would drag in virtual-dispatch reasoning.
34+
`BufferingTargetWrapper`). The `TimeoutContinuation` variant — where the receiver is
35+
the result of `Interlocked.Exchange(ref _timer, null)`, the detach-and-dispose
36+
teardown — is covered separately by the sibling
37+
[`field-dispose-via-exchange`](../field-dispose-via-exchange/notes.md) fixture
38+
(`RefExchangeNulledField` binds the exchange result to the field, then this same sink
39+
recognition applies). Scope is intentionally limited to **extension methods**: an
40+
instance method disposing its own `this` is not a real dispose-delegation shape and
41+
would drag in virtual-dispatch reasoning.

‎frontend/roslyn/OwnSharp.Extractor/Program.cs‎

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -479,7 +479,8 @@ left is MemberAccessExpressionSyntax m
479479
// text), and `owned` is AST-based — not a regex. NOTE: callers must exclude timers
480480
// — a *running* timer is rooted by the dispatcher regardless of who owns the field.
481481
static bool IsSelfOwnedSource(ExpressionSyntax left, IEventSymbol ev,
482-
SemanticModel model, HashSet<string> owned)
482+
SemanticModel model, HashSet<string> owned,
483+
ISymbol? cls)
483484
{
484485
if (left is not MemberAccessExpressionSyntax m)
485486
return !ev.IsStatic; // bare event => an instance event on `this`
@@ -488,12 +489,15 @@ static bool IsSelfOwnedSource(ExpressionSyntax left, IEventSymbol ev,
488489
var recv = model.GetSymbolInfo(m.Expression).Symbol;
489490
if ((recv is IFieldSymbol or ILocalSymbol) && owned.Contains(recv.Name))
490491
return true;
491-
// A `this`-instance get-only PROPERTY whose value the class owns (`this.Child.Event
492-
// += h`, Child a `=> _owned` / `{ get; } = new()` property) is the SAME collectable
493-
// self-cycle as the owned field directly — this, the owned child, and the handler
494-
// are collected together. The property must resolve to a member the class constructs.
492+
// A get-only PROPERTY over a member the class owns (`this.Child.Event += h`, Child a
493+
// `=> _owned` / `{ get; } = new()` property) is the SAME collectable self-cycle as the
494+
// owned field directly. GATED on a `this`/bare access (`Channel` / `this.Channel`):
495+
// `other.Channel.Event += h` reaches ANOTHER instance's property, which may outlive
496+
// this subscriber and retain the handler, so it stays flagged (Codex P2).
495497
return recv is IPropertySymbol { IsStatic: false } p
496-
&& PropertyReturnsOwnedMember(p, owned, model);
498+
&& m.Expression is (IdentifierNameSyntax
499+
or MemberAccessExpressionSyntax { Expression: ThisExpressionSyntax })
500+
&& PropertyReturnsOwnedMember(p, owned, model, cls);
497501
}
498502

499503
// A GET-ONLY property whose value the class OWNS: an auto-property initialised to
@@ -506,7 +510,7 @@ static bool IsSelfOwnedSource(ExpressionSyntax left, IEventSymbol ev,
506510
// drop a real leak). Only the dominant owned-property shapes are recognised; anything
507511
// else (a computed getter, a getter returning a parameter/injected field) falls through.
508512
static bool PropertyReturnsOwnedMember(IPropertySymbol prop, HashSet<string> owned,
509-
SemanticModel model)
513+
SemanticModel model, ISymbol? cls)
510514
{
511515
if (prop.SetMethod is not null)
512516
return false;
@@ -528,7 +532,13 @@ static bool PropertyReturnsOwnedMember(IPropertySymbol prop, HashSet<string> own
528532
continue;
529533
var pm = model.Compilation.GetSemanticModel(pd.SyntaxTree);
530534
var rsym = pm.GetSymbolInfo(returned).Symbol;
531-
if (rsym is IFieldSymbol or IPropertySymbol && owned.Contains(rsym.Name))
535+
// The returned member must be one the ANALYZED class declares — compare the
536+
// symbol's CONTAINING TYPE, not just its name, so a same-named field on another
537+
// type cannot satisfy ownership (CodeRabbit). `owned` holds only this class's
538+
// constructed-field names, so the name check + the containing-type check agree.
539+
if (rsym is IFieldSymbol or IPropertySymbol
540+
&& owned.Contains(rsym.Name)
541+
&& (cls is null || SymbolEqualityComparer.Default.Equals(rsym.ContainingType, cls)))
532542
return true;
533543
}
534544
return false;
@@ -2582,7 +2592,8 @@ static bool CallReleasesReceiver(IMethodSymbol? sym, SemanticModel model)
25822592
if (args.Count != 2
25832593
|| !args[0].RefKindKeyword.IsKind(SyntaxKind.RefKeyword)
25842594
|| !(args[1].Expression.IsKind(SyntaxKind.NullLiteralExpression)
2585-
|| args[1].Expression.IsKind(SyntaxKind.DefaultLiteralExpression)))
2595+
|| args[1].Expression.IsKind(SyntaxKind.DefaultLiteralExpression)
2596+
|| args[1].Expression is DefaultExpressionSyntax)) // default(T) for a ref type is null too
25862597
return null;
25872598
return ThisFieldName(args[0].Expression) is { } f
25882599
&& model.GetSymbolInfo(args[0].Expression).Symbol is IFieldSymbol
@@ -3296,7 +3307,7 @@ or ImplicitObjectCreationExpressionSyntax
32963307
// intent, not a leak (mined: Npgsql PoolManager's `AppDomain.CurrentDomain.
32973308
// ProcessExit += (_,_) => ClearAll()` shutdown hook). A handler that captures
32983309
// instance state still pins it to the process, so it stays OWN014 (Codex).
3299-
if (!isTimer && (IsSelfOwnedSource(a.Left, ev, model, selfOwned)
3310+
if (!isTimer && (IsSelfOwnedSource(a.Left, ev, model, selfOwned, clsSymbol)
33003311
|| IsStaticHandler(a.Right, model)
33013312
|| (IsProcessLifetimeAppDomainEvent(ev)
33023313
&& HandlerRetainsNoInstance(a.Right, model))))

‎scripts/oracle_compare.py‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -261,10 +261,13 @@ def _load_baseline(path: str) -> list[BaselineRule]:
261261
if not s or s.startswith("#"):
262262
continue
263263
parts = [p.strip() for p in s.split("|")]
264-
if len(parts) < 4:
264+
# need all four key fields, and a NON-EMPTY message-substring: an empty substr
265+
# would make `substr in message` always true — a wildcard that a typo could turn
266+
# into a blanket suppressor for that file/rule (CodeRabbit). Reason keeps any `|`.
267+
if len(parts) < 4 or not parts[3]:
265268
continue
266269
repo, basename, rule, substr = parts[:4]
267-
reason = parts[4] if len(parts) > 4 else ""
270+
reason = " | ".join(parts[4:]).strip() if len(parts) > 4 else ""
268271
rules.append(BaselineRule(repo, basename, rule, substr, reason))
269272
return rules
270273

@@ -769,14 +772,18 @@ def _selftest() -> int:
769772
# the loader: comments/blank lines ignored, <4 fields skipped, reason optional.
770773
import tempfile
771774
with tempfile.NamedTemporaryFile("w", suffix=".txt", delete=False) as tf:
772-
tf.write("# comment\n\no/r | B.cs | OWN001 | local 'b' | a reason\n"
773-
"too | few | fields\n")
775+
tf.write("# comment\n\no/r | B.cs | OWN001 | local 'b' | a reason | with a pipe\n"
776+
"too | few | fields\n"
777+
"o/r | B.cs | OWN001 | | empty substr is a wildcard — must be skipped\n")
774778
tf.flush()
775779
loaded = _load_baseline(tf.name)
776-
if len(loaded) != 1 or loaded[0].reason != "a reason":
780+
# the <4-field line and the empty-substring line are both skipped; reason keeps its `|`.
781+
if len(loaded) != 1 or loaded[0].reason != "a reason | with a pipe":
777782
fails.append(f"baseline loader wrong: {loaded}")
783+
if any(r.substr == "" for r in loaded):
784+
fails.append("baseline loader must reject an empty message-substring (wildcard)")
778785

779-
total = 30
786+
total = 31
780787
for f in fails:
781788
print(f"ORACLE SELFTEST FAIL: {f}")
782789
print(f"oracle_compare selftest: {total - len(fails)}/{total} checks passed")

0 commit comments

Comments
 (0)