Skip to content

Commit b5bce52

Browse files
authored
Merge pull request #239 from PhysShell/claude/owned-collection-229
feat(extractor): recognise self-populated owned-collection elements as self-owned (#229)
2 parents cae3cd1 + 88417e6 commit b5bce52

4 files changed

Lines changed: 442 additions & 7 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,7 @@ jobs:
215215
frontend/roslyn/samples/AppScopedSourceSample.cs \
216216
frontend/roslyn/samples/WinFormsDisposalSample.cs \
217217
frontend/roslyn/samples/AssociatedObjectSourceSample.cs \
218+
frontend/roslyn/samples/OwnedCollectionElementSample.cs \
218219
-o "$RUNNER_TEMP/facts.json"
219220
cat "$RUNNER_TEMP/facts.json"
220221
- name: Check facts through the core
@@ -946,7 +947,42 @@ jobs:
946947
# population scan must span every partial of the type.
947948
echo "$out" | grep -qE "AssociatedObjectSourceSample\.cs:[0-9]+: warning: \[OWN001\].*'PartialFieldBehavior'" \
948949
|| { echo "FAIL: expected OWN001 when a sibling partial injects the AssociatedObject field (#227)"; exit 1; }
949-
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) + #225 empty-Dispose local exemption (silent; controls flagged) + #228 curated app-scoped source in App (silent; controls flagged) + #227 self-owned Behavior.AssociatedObject source (silent; controls flagged) + #219 WinForms Controls/IContainer disposal channels (silent; controls flagged) at the C# location"
950+
# issue #229 — subscribing to an element of a collection the class itself
951+
# populated (a `foreach` over a this-owned field/property assigned from the
952+
# class's OWN construction/factory) must be SILENT: the collection and its
953+
# elements share the constructing object's lifetime (a collectable self-cycle,
954+
# not a leak). Three population forms: an own factory into a property
955+
# (OwnedFactoryViewModel), a collection-initializer field (InlineNewViewModel),
956+
# and a `new` assigned in the ctor (DirectNewViewModel).
957+
if echo "$out" | grep -qE "OwnedCollectionElementSample\.cs:[0-9]+:.*('OwnedFactoryViewModel'|'InlineNewViewModel'|'DirectNewViewModel')"; then
958+
echo "FAIL: a self-populated owned-collection element subscription was wrongly reported (#229)"; exit 1
959+
fi
960+
# ...and the exemption must NOT over-widen — four controls STAY flagged:
961+
# (1, REQUIRED) the collection is a FIELD assigned from a ctor PARAMETER;
962+
echo "$out" | grep -qE "OwnedCollectionElementSample\.cs:[0-9]+: warning: \[OWN001\].*'InjectedCollectionViewModel'" \
963+
|| { echo "FAIL: expected OWN001 on the injected-collection field (#229)"; exit 1; }
964+
# (2, REQUIRED) the `foreach` iterates a ctor PARAMETER collection directly;
965+
echo "$out" | grep -qE "OwnedCollectionElementSample\.cs:[0-9]+: warning: \[OWN001\].*'ParamCollectionViewModel'" \
966+
|| { echo "FAIL: expected OWN001 on the parameter-collection foreach (#229)"; exit 1; }
967+
# (3) the member is populated from a SERVICE-LOCATED call (injected receiver);
968+
echo "$out" | grep -qE "OwnedCollectionElementSample\.cs:[0-9]+: warning: \[OWN001\].*'ServiceLocatedViewModel'" \
969+
|| { echo "FAIL: expected OWN001 on the service-located collection source (#229)"; exit 1; }
970+
# (4) the member is own-populated in the ctor but ALSO reassigned an injected
971+
# value elsewhere — every population site must be own-produced.
972+
echo "$out" | grep -qE "OwnedCollectionElementSample\.cs:[0-9]+: warning: \[OWN001\].*'MixedCollectionViewModel'" \
973+
|| { echo "FAIL: expected OWN001 when the collection is also assigned an injected value (#229)"; exit 1; }
974+
# (5, Codex P1) a freshly-`new`d collection SEEDED from an injected parameter —
975+
# the elements (which carry the events) are injected, so it is not owned;
976+
echo "$out" | grep -qE "OwnedCollectionElementSample\.cs:[0-9]+: warning: \[OWN001\].*'SeededNewViewModel'" \
977+
|| { echo "FAIL: expected OWN001 on a new collection seeded from injected elements (#229)"; exit 1; }
978+
# (6, Codex P1) an own-class factory that FORWARDS a service-located collection;
979+
echo "$out" | grep -qE "OwnedCollectionElementSample\.cs:[0-9]+: warning: \[OWN001\].*'ForwardingFactoryViewModel'" \
980+
|| { echo "FAIL: expected OWN001 when the own factory forwards injected data (#229)"; exit 1; }
981+
# (7, CodeRabbit) a PARTIAL class with a disqualifying injected assignment in a
982+
# sibling partial — the population scan must span every partial of the type.
983+
echo "$out" | grep -qE "OwnedCollectionElementSample\.cs:[0-9]+: warning: \[OWN001\].*'PartialInjectedViewModel'" \
984+
|| { echo "FAIL: expected OWN001 when a sibling partial injects the collection (#229)"; exit 1; }
985+
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) + #225 empty-Dispose local exemption (silent; controls flagged) + #228 curated app-scoped source in App (silent; controls flagged) + #227 self-owned Behavior.AssociatedObject source (silent; controls flagged) + #219 WinForms Controls/IContainer disposal channels (silent; controls flagged) + #229 self-populated owned-collection element (silent; controls flagged) at the C# location"
950986
- name: Flow-sensitive local IDisposables (--flow-locals, P-016 B0b/B2)
951987
run: |
952988
# Path-sensitive flow analysis of local IDisposables — bugs the flat D1

‎docs/notes/field-notes-patterns.md‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -551,12 +551,18 @@ exemption's real criterion is "does this object's lifetime start and end with th
551551
subscriber's" — a base-class accessor to the attached object, or an item of an
552552
owned collection, satisfies that just as well as a constructed field.**
553553

554-
**Status (2026-07):** shape **(a)**, the `Behavior.AssociatedObject` self-owned
555-
source, shipped in #227 — extractor `IsAssociatedObjectSource`, gated on the
556-
`Behavior` base (`IsBehaviorSubscriber`) plus a same-class assignment-chain
557-
resolving to `this.AssociatedObject` (`ResolvesToAssociatedObject`); pinned by
558-
`frontend/roslyn/samples/AssociatedObjectSourceSample.cs`. Shape **(c)**, the
559-
owned-collection element, is tracked separately by #229.
554+
**Status (2026-07):** shapes **(a)** and **(c)** shipped.
555+
- **(a)** the `Behavior.AssociatedObject` self-owned source, shipped in #227 —
556+
extractor `IsAssociatedObjectSource`, gated on the `Behavior` base
557+
(`IsBehaviorSubscriber`) plus a same-class assignment-chain resolving to
558+
`this.AssociatedObject` (`ResolvesToAssociatedObject`); pinned by
559+
`frontend/roslyn/samples/AssociatedObjectSourceSample.cs`.
560+
- **(c)** the element of a self-populated collection, shipped in #229 — extractor
561+
`IsOwnedCollectionElementSource`: a `foreach` loop variable whose collection is a
562+
this-owned field/property populated ONLY from own construction / an own factory
563+
(`MemberPopulatedByOwnFactory` + `IsOwnProducedValue`), never a ctor parameter or
564+
an injected/service-located value; pinned by
565+
`frontend/roslyn/samples/OwnedCollectionElementSample.cs`.
560566

561567
## 16. Template part fetched via `FindName`/`GetTemplateChild`, stored as a local
562568

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

Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1173,6 +1173,174 @@ static bool IsAssociatedObjectSource(ExpressionSyntax left, SemanticModel model,
11731173
=> left is MemberAccessExpressionSyntax m
11741174
&& ResolvesToAssociatedObject(m.Expression, model, clsNode, depth: 0);
11751175

1176+
// P-004 / issue #229: the `+=` receiver is an element of a collection the class OWNS —
1177+
// a `foreach (var x in FieldOrProperty)` loop variable whose collection is a this-owned
1178+
// field/property the class ITSELF populates (own construction / own factory / collection
1179+
// initializer), never an injected or ctor-parameter value. The collection and its
1180+
// elements then share the constructing object's lifetime, so the subscription is a
1181+
// collectable self-cycle, not a leak. All checks are same-class and syntactic:
1182+
// * the receiver must bind to a `foreach` variable (matched by SYMBOL);
1183+
// * the loop's collection must be a bare/`this`-qualified member of THIS class;
1184+
// * that member's every population site must be an own-produced value (below) — a
1185+
// single injected/parameter assignment denies the proof (precision-first).
1186+
static bool IsOwnedCollectionElementSource(ExpressionSyntax left, SemanticModel model,
1187+
INamedTypeSymbol? cls)
1188+
{
1189+
if (cls is null || left is not MemberAccessExpressionSyntax m)
1190+
return false;
1191+
if (model.GetSymbolInfo(m.Expression).Symbol is not ILocalSymbol loopVar)
1192+
return false;
1193+
// The receiver's symbol must be the variable DECLARED by an enclosing `foreach`.
1194+
var forEach = m.Expression.Ancestors().OfType<ForEachStatementSyntax>()
1195+
.FirstOrDefault(f => SymbolEqualityComparer.Default.Equals(
1196+
model.GetDeclaredSymbol(f), loopVar));
1197+
if (forEach is null)
1198+
return false;
1199+
// The iterated collection must be a bare `Field` / `this.Field` of THIS class — a
1200+
// `other.Items` reaches another instance's collection (unknown element lifetime).
1201+
var collExpr = StripCasts(forEach.Expression);
1202+
if (collExpr is not (IdentifierNameSyntax
1203+
or MemberAccessExpressionSyntax { Expression: ThisExpressionSyntax }))
1204+
return false;
1205+
var member = model.GetSymbolInfo(collExpr).Symbol;
1206+
if (member is not (IFieldSymbol or IPropertySymbol))
1207+
return false;
1208+
if (!SymbolEqualityComparer.Default.Equals(member.ContainingType, cls))
1209+
return false;
1210+
return MemberPopulatedByOwnFactory(member, model, cls);
1211+
}
1212+
1213+
// #229: is EVERY population of this field/property an own-produced value — and is there
1214+
// at least one? Scans EVERY partial declaration of the containing type (a disqualifying
1215+
// injected assignment may live in a sibling partial FILE — the merged compilation makes
1216+
// them all reachable through the symbol's DeclaringSyntaxReferences; CodeRabbit), each
1217+
// with its own tree's semantic model. An initializer at a declaration, or any simple
1218+
// assignment to the member in a ctor/method, is a population site. A single site we
1219+
// cannot prove is own-produced (a ctor parameter, an injected field, a service-located
1220+
// call) denies the proof, so an injected collection keeps today's honest warning.
1221+
static bool MemberPopulatedByOwnFactory(ISymbol member, SemanticModel model,
1222+
INamedTypeSymbol cls)
1223+
{
1224+
var any = false;
1225+
bool Note(ExpressionSyntax value, SemanticModel m)
1226+
{
1227+
any = true;
1228+
return IsOwnProducedValue(value, m, cls, depth: 0);
1229+
}
1230+
foreach (var decl in EnumerateTypeDeclarations(cls))
1231+
{
1232+
var m = model.Compilation.GetSemanticModel(decl.SyntaxTree);
1233+
// Declaration-site initializer: `= CreateData()` / `= new(){ ... }` on the field
1234+
// or auto-property.
1235+
foreach (var v in decl.DescendantNodes().OfType<VariableDeclaratorSyntax>())
1236+
if (v.Initializer is { } vi
1237+
&& SymbolEqualityComparer.Default.Equals(m.GetDeclaredSymbol(v), member)
1238+
&& !Note(vi.Value, m))
1239+
return false;
1240+
foreach (var p in decl.DescendantNodes().OfType<PropertyDeclarationSyntax>())
1241+
if (p.Initializer is { } pi
1242+
&& SymbolEqualityComparer.Default.Equals(m.GetDeclaredSymbol(p), member)
1243+
&& !Note(pi.Value, m))
1244+
return false;
1245+
// Assignment sites: `Items1 = CreateData();`
1246+
foreach (var asg in decl.DescendantNodes().OfType<AssignmentExpressionSyntax>())
1247+
if (asg.IsKind(SyntaxKind.SimpleAssignmentExpression)
1248+
&& SymbolEqualityComparer.Default.Equals(m.GetSymbolInfo(asg.Left).Symbol, member)
1249+
&& !Note(asg.Right, m))
1250+
return false;
1251+
}
1252+
return any;
1253+
}
1254+
1255+
// #229: a value the class PRODUCES itself, not one that FLOWS IN — and whose ELEMENTS are
1256+
// themselves own-produced (the subscribed events live on those elements, so a collection
1257+
// object that is freshly `new`d but SEEDED with injected elements is NOT owned). Two
1258+
// forms, checked recursively (Codex P1):
1259+
// * an object creation whose constructor arguments AND collection-initializer elements
1260+
// are each own-produced — `new List<Model>()`, `new() { new Model(), new Model() }`;
1261+
// `new List<Model>(injectedSource)` and `new() { injectedModel }` are rejected;
1262+
// * an invocation of an OWN factory (a method of THIS class with an implicit/`this`
1263+
// receiver) whose body PROVABLY returns an own-produced value — so a wrapper
1264+
// `List<Model> GetData() => _service.GetData()` that forwards injected elements is
1265+
// rejected. A call through any other receiver (`_service.GetData()`) is likewise out.
1266+
// Casts/`!` are peeled first; depth-bounded so a self-referential factory cannot spin.
1267+
static bool IsOwnProducedValue(ExpressionSyntax expr, SemanticModel model,
1268+
INamedTypeSymbol cls, int depth)
1269+
{
1270+
if (depth > 3)
1271+
return false;
1272+
expr = StripCasts(expr);
1273+
if (expr is BaseObjectCreationExpressionSyntax oce)
1274+
{
1275+
// Constructor arguments may SEED the collection with external elements.
1276+
if (oce.ArgumentList is { } al)
1277+
foreach (var arg in al.Arguments)
1278+
if (!IsOwnProducedValue(arg.Expression, model, cls, depth + 1))
1279+
return false;
1280+
// A collection/object initializer's elements must each be own-produced.
1281+
if (oce.Initializer is { } init)
1282+
foreach (var e in init.Expressions)
1283+
if (!IsOwnProducedValue(e, model, cls, depth + 1))
1284+
return false;
1285+
return true;
1286+
}
1287+
if (expr is InvocationExpressionSyntax inv)
1288+
{
1289+
var callee = model.GetSymbolInfo(inv).Symbol as IMethodSymbol;
1290+
if (callee is null
1291+
|| !SymbolEqualityComparer.Default.Equals(callee.ContainingType, cls))
1292+
return false;
1293+
var receiverIsOwn = inv.Expression switch
1294+
{
1295+
IdentifierNameSyntax => true, // CreateData()
1296+
MemberAccessExpressionSyntax { Expression: ThisExpressionSyntax } => true, // this.CreateData()
1297+
_ => false,
1298+
};
1299+
return receiverIsOwn && FactoryReturnsOwnProduced(callee, model, cls, depth);
1300+
}
1301+
return false;
1302+
}
1303+
1304+
// #229: does this own-class factory PROVABLY return only own-produced values? Inspects
1305+
// its body (same-class, so within the class-level scan the guardrail allows): an
1306+
// expression body `=> expr`, or every `return expr` in a block body, must be own-produced.
1307+
// No visible body, a bodiless return, or any non-own-produced return denies the proof —
1308+
// so a wrapper that forwards an injected/service-located collection keeps the warning.
1309+
// (Returns nested in lambdas/local functions only make the check MORE conservative.)
1310+
static bool FactoryReturnsOwnProduced(IMethodSymbol factory, SemanticModel model,
1311+
INamedTypeSymbol cls, int depth)
1312+
{
1313+
if (factory.DeclaringSyntaxReferences.Length == 0)
1314+
return false;
1315+
var any = false;
1316+
foreach (var r in factory.DeclaringSyntaxReferences)
1317+
{
1318+
if (r.GetSyntax() is not MethodDeclarationSyntax md)
1319+
return false;
1320+
var m = model.Compilation.GetSemanticModel(md.SyntaxTree);
1321+
if (md.ExpressionBody is { } eb)
1322+
{
1323+
any = true;
1324+
if (!IsOwnProducedValue(eb.Expression, m, cls, depth + 1))
1325+
return false;
1326+
continue;
1327+
}
1328+
if (md.Body is null)
1329+
return false;
1330+
var returns = md.Body.DescendantNodes().OfType<ReturnStatementSyntax>().ToList();
1331+
if (returns.Count == 0)
1332+
return false;
1333+
foreach (var ret in returns)
1334+
{
1335+
any = true;
1336+
if (ret.Expression is null
1337+
|| !IsOwnProducedValue(ret.Expression, m, cls, depth + 1))
1338+
return false;
1339+
}
1340+
}
1341+
return any;
1342+
}
1343+
11761344
// P-004 WPF MVVM ownership: a field read from `this.DataContext`, optionally through
11771345
// an `as`/cast (`DataContext as VM`, `(VM)DataContext`). Combined with a view whose
11781346
// own XAML CONSTRUCTS its DataContext, such a field is the view's owned view-model.
@@ -4400,6 +4568,16 @@ or ImplicitObjectCreationExpressionSyntax
44004568
if (!isTimer && clsIsBehavior
44014569
&& IsAssociatedObjectSource(a.Left, model, cls))
44024570
continue;
4571+
// P-004 / issue #229: the `+=` receiver is an element of a collection the
4572+
// class itself populated — a `foreach (var x in OwnField)` loop variable
4573+
// whose collection is a this-owned field/property assigned from the class's
4574+
// OWN construction/factory (not a ctor parameter, not an injected value).
4575+
// The collection and its elements share the constructing object's lifetime,
4576+
// so the subscription is the same collectable self-cycle as a constructed
4577+
// field; a `foreach` over an injected/parameter collection keeps today's
4578+
// warning (its population sites are not own-produced).
4579+
if (!isTimer && IsOwnedCollectionElementSource(a.Left, model, clsSymbol))
4580+
continue;
44034581
// P-004 (issue #223): the curated weak-referenced-static-event allowlist —
44044582
// unconditional (unlike the AppDomain exemption above, this does NOT gate on
44054583
// HandlerRetainsNoInstance: the whole point of a weak-referenced source is

0 commit comments

Comments
 (0)