Skip to content

Commit 4c5a86b

Browse files
authored
Merge pull request #240 from PhysShell/claude/plans-tasks-review-u77dfs
fix(extractor): confine the empty-Dispose exemption to enumerators — source emptiness ≠ runtime no-op (Closes #238)
2 parents 1664b41 + 40756e3 commit 4c5a86b

8 files changed

Lines changed: 377 additions & 40 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 44 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -840,14 +840,13 @@ jobs:
840840
# ...while the same-named local in the OTHER method must still warn.
841841
echo "$out" | grep -qE "TemplatePartLocalCaptureSample\.cs:[0-9]+: warning: \[OWN001\].*'SameNameDifferentScopeSubscriber'" \
842842
|| { echo "FAIL: expected OWN001 on the same-named-but-unrelated local in a different method"; exit 1; }
843-
# issue #225 — a user-defined type whose Dispose() body is provably EMPTY holds no resource,
844-
# so a LOCAL of it never disposed is not a leak (extends the named-BCL no-op exemption to any
845-
# type). Here the flat (non-flow) name path: a `*Reader`-named empty-Dispose local (ScratchReader)
846-
# is silent, while a non-empty `*Reader` local (LeakyReader) still leaks. (The semantic enumerator
847-
# form is exercised in the --flow-locals step below.)
848-
if echo "$out" | grep -q "'ScratchReader'"; then
849-
echo "FAIL: a local of a user type with a provably-empty Dispose() body was wrongly flagged (#225)"; exit 1
850-
fi
843+
# issue #225 (narrowed by #238) — the empty-Dispose exemption is confined to ENUMERATOR
844+
# types: source emptiness proves nothing about the COMPILED body once an IL weaver is in
845+
# play (Janitor.Fody wove real cleanup into ClosedXML's XLWorkbook.Dispose — 263 silently
846+
# swallowed findings). ScratchReader (empty source Dispose, NOT an enumerator — the
847+
# XLWorkbook shape) must therefore STAY flagged on the flat name path...
848+
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'s'.*'ScratchReader'" \
849+
|| { echo "FAIL: #238: a NON-enumerator empty-source-Dispose local must stay flagged (weaver soundness)"; exit 1; }
851850
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'lr'.*'LeakyReader'" \
852851
|| { echo "FAIL: a non-empty-Dispose local (LeakyReader) must still leak (#225 stays scoped)"; exit 1; }
853852
# Codex P2: an empty SYNC Dispose but a real DisposeAsync (a `*Reader` here) must still leak.
@@ -1228,11 +1227,44 @@ jobs:
12281227
# must NOT be exempted — the real cleanup is async and the flow detector treats it as a release.
12291228
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'ar' is never disposed" \
12301229
|| { echo "FAIL: #225 flow: a type with a real DisposeAsync (empty sync Dispose) must still leak"; exit 1; }
1231-
# exactly 4 EmptyDisposeSample findings -> the two provably-empty-Dispose locals (the enumerator
1232-
# and ScratchReader) are SILENT; any leak of them would push the count past 4.
1230+
# #240 review P1: an enumerator whose sync Dispose is empty but whose BASE owns a real
1231+
# DisposeAsync (inherited IAsyncDisposable) must still leak — inherited async cleanup was
1232+
# the same silent-false-negative class #238 exists to close.
1233+
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'ia' is never disposed" \
1234+
|| { echo "FAIL: #240 flow: an inherited-IAsyncDisposable enumerator must still leak"; exit 1; }
1235+
# #240 review round 2: a BASE with a bare DisposeAsync (no IAsyncDisposable interface) must
1236+
# be caught by the base-chain scan -> still leak.
1237+
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'ba' is never disposed" \
1238+
|| { echo "FAIL: #240 flow: a base-chain bare DisposeAsync enumerator must still leak"; exit 1; }
1239+
# #240 review round 2: a NON-generic IEnumerator (does not force IDisposable) must stay
1240+
# flagged -> pins the IEnumerator<T>-only narrowing.
1241+
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'ng' is never disposed" \
1242+
|| { echo "FAIL: #240 flow: a non-generic-IEnumerator empty-Dispose local must still leak"; exit 1; }
1243+
# #238 soundness: ScratchReader (empty source Dispose, NOT an enumerator) must stay flagged
1244+
# in the flow path too — a weaver can rewrite a non-enumerator Dispose at build time.
1245+
echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'s' is never disposed" \
1246+
|| { echo "FAIL: #238 flow: a NON-enumerator empty-source-Dispose local must stay flagged"; exit 1; }
1247+
# exactly 5 EmptyDisposeSample findings -> the two provably-empty-Dispose ENUMERATOR locals
1248+
# ('e' plain, 'x' explicit-interface — the #238 coverage fix) are SILENT; any leak of them
1249+
# would push the count past 8 (controls: s,r,lr,d,ar,ia,ba,ng).
12331250
n225=$(echo "$out" | grep -cE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN00")
1234-
[ "$n225" = "4" ] \
1235-
|| { echo "FAIL: #225 flow: expected exactly 4 EmptyDisposeSample findings (controls only), got $n225"; exit 1; }
1251+
[ "$n225" = "8" ] \
1252+
|| { echo "FAIL: #225/#238 flow: expected exactly 8 EmptyDisposeSample findings (controls only), got $n225"; exit 1; }
1253+
# #238 weaver kill-switch: a directory carrying FodyWeavers.xml disables the exemption even
1254+
# for a perfect enumerator shape — the weaved fixture's local must be FLAGGED.
1255+
dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \
1256+
frontend/roslyn/samples/weaved/WeavedEmptyDispose.cs --flow-locals -o "$RUNNER_TEMP/weaved.json"
1257+
wout=$(python -m ownlang ownir "$RUNNER_TEMP/weaved.json" || true)
1258+
echo "$wout" | grep -qE "WeavedEmptyDispose\.cs:[0-9]+:.*\[OWN001\].*'we' is never disposed" \
1259+
|| { echo "FAIL: #238: FodyWeavers.xml present -> even an enumerator empty Dispose must stay flagged"; exit 1; }
1260+
# ...and the LINKED-source variant (Codex on #240): the shared file lives OUTSIDE the
1261+
# weaver project's directory (no FodyWeavers.xml above it) — analysed THROUGH the
1262+
# .csproj, the project's weaver config must still disable the exemption.
1263+
dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \
1264+
frontend/roslyn/samples/weaved-linked/Proj/Linked.csproj --flow-locals -o "$RUNNER_TEMP/weavedlink.json"
1265+
lout=$(python -m ownlang ownir "$RUNNER_TEMP/weavedlink.json" || true)
1266+
echo "$lout" | grep -qE "SharedEnumerator\.cs:[0-9]+:.*\[OWN001\].*'se' is never disposed" \
1267+
|| { echo "FAIL: #240: a linked source of a Fody-enabled project must not get the empty-Dispose exemption"; exit 1; }
12361268
echo "OK: flow-sensitive OWN001/002/003 on real C# (path-sensitive, loops via while/foreach/for, try/finally sequential, never-vs-every-path wording, dispose-optional exempt, #225 empty-Dispose local exempt, beyond flat)"
12371269
- name: Gallery C#-native bad/ok pairs (examples/gallery/cs/)
12381270
run: |

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

Lines changed: 132 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,10 @@
8181
// run that did not request it (the other config — emitEvents/flowLocals/reportStats — are locals,
8282
// re-initialized each call, so they need no reset; only this static one does). CodeRabbit.
8383
BodyThrowEdges = false;
84+
// #240 (review P2): WeaverOwnedFiles is the other process-global mutable state — a file recorded
85+
// as belonging to a Fody-enabled project must not stay "weaver-owned" for an unrelated later
86+
// invocation in the same process (a sticky false positive). Same reset discipline as above.
87+
WeaverOwnedFiles.Clear();
8488
// `extract` verb: the tool's one job is extraction, so an optional leading `extract`
8589
// makes the advertised `ownsharp-extract extract --project App.csproj --out facts.json`
8690
// UX real while the bare form (no verb) stays the default and back-compatible. (The
@@ -309,6 +313,13 @@ static List<string> ProjectCsFiles(string csproj, EnumerationOptions opts)
309313
result.RemoveAll(f => rmMatch.Any(m => m(f)));
310314
}
311315

316+
// #240: if THIS project is weaver-enabled (FodyWeavers.xml in its directory or above),
317+
// EVERY file it compiles — including linked sources outside its tree — is weavable at
318+
// build time; record them so the empty-Dispose kill-switch sees past the file's own path.
319+
if (AncestorsHaveWeaverConfig(dir))
320+
foreach (var f in result)
321+
WeaverOwnedFiles.Add(Path.GetFullPath(f));
322+
312323
return result.Distinct().ToList();
313324
}
314325

@@ -1831,36 +1842,134 @@ static bool IsNoOpDisposeWrapper(BaseObjectCreationExpressionSyntax oce, Semanti
18311842
return arg0 is not null && IsInMemoryDisposableBacking(model.GetTypeInfo(arg0).Type);
18321843
}
18331844

1834-
// Issue #225 — a user-defined type whose parameterless `Dispose()` body is provably EMPTY holds no
1835-
// resource behind the interface: it implements IDisposable only to satisfy a contract (e.g.
1836-
// `IEnumerator<T>`), so never calling Dispose() cannot leak. Recognised from SOURCE: the type
1837-
// declares `void Dispose()` with a BLOCK body of ZERO statements, and no base in its chain carries a
1838-
// Dispose to cascade to (the base is `object` / not IDisposable), so nothing real is skipped. A body
1839-
// we cannot read (third-party metadata-only, or expression-bodied doing work) is NOT provably empty
1840-
// and stays flagged — never a guessed drop. This generalises the named-BCL IsNoOpDisposeWrapper /
1841-
// IsDisposeOptional idea to any type. Deliberately scoped to LOCAL disposables (the #201-sweep
1845+
// Issue #225 (narrowed by #238) — a user-defined `IEnumerator<T>` whose parameterless `Dispose()`
1846+
// body is provably EMPTY holds no resource behind the interface: `IEnumerator<T>` is the one
1847+
// interface that FORCES IDisposable onto types that usually hold nothing, so never calling its
1848+
// empty Dispose() cannot leak. Recognised from SOURCE: the type declares `void Dispose()` (plain or
1849+
// explicit-interface) with a BLOCK body of ZERO statements, and no base in its chain carries a
1850+
// Dispose to cascade to, no IAsyncDisposable / bare DisposeAsync anywhere in the type or its bases,
1851+
// and no FodyWeavers.xml above it (an IL weaver could rewrite the "empty" body at build time — the
1852+
// #238 soundness lesson). A body we cannot read (metadata-only, or expression-bodied doing work) is
1853+
// NOT provably empty and stays flagged — never a guessed drop. Deliberately NOT "any type": that
1854+
// over-broad framing was the #238 regression (263 silently-swallowed ClosedXML findings). Scoped to
1855+
// LOCAL disposables (the #201-sweep
18421856
// evidence — ClosedXML's `Slice.Enumerator` locals): a FIELD keeps its own disposal contract, so an
18431857
// empty-Dispose field (the OwnIgnoreSample `Handle` stand-in) is unaffected.
18441858
static bool HasEmptyDisposeBody(ITypeSymbol? t)
18451859
{
18461860
if (t is not INamedTypeSymbol nt)
18471861
return false;
1862+
// Issue #238 SOUNDNESS gate: an empty SOURCE body proves nothing about the COMPILED body —
1863+
// IL weavers (Janitor.Fody wove the real cleanup into ClosedXML's XLWorkbook.Dispose(),
1864+
// 263 silently-swallowed findings) rewrite Dispose at build time, and source-level analysis
1865+
// cannot prove their absence. So the exemption is confined to the ONE shape whose empty
1866+
// Dispose is FORCED by the language, not authored: `IEnumerator<T>` — the generic enumerator
1867+
// interface is the only one that transitively requires IDisposable (IEnumerator<T> : IEnumerator,
1868+
// IDisposable). The non-generic System.Collections.IEnumerator does NOT extend IDisposable, so a
1869+
// type that pairs them coupled the two contracts itself — the motivating proof does not apply, and
1870+
// it keeps the honest warning (review #240).
1871+
// Matched by name/arity/namespace, not SpecialType: in the source-only extraction (no wired
1872+
// corelib reference) the well-known-type SpecialType is often unset, so `IEnumerator<T>` would
1873+
// slip through a SpecialType check. `System.Collections.Generic.IEnumerator`1` is the exact
1874+
// interface that transitively forces IDisposable.
1875+
if (!nt.AllInterfaces.Any(i => i.Name == "IEnumerator"
1876+
&& i.TypeArguments.Length == 1
1877+
&& i.ContainingNamespace?.ToString() == "System.Collections.Generic"))
1878+
return false;
18481879
// The base must not own a Dispose we'd be silently skipping — only `object` / a non-IDisposable base.
18491880
for (var b = nt.BaseType; b is not null && b.SpecialType != SpecialType.System_Object; b = b.BaseType)
18501881
if (ImplementsIDisposable(b))
18511882
return false;
18521883
// A non-empty DisposeAsync() can do the REAL cleanup even when the sync Dispose() is an empty
1853-
// compatibility no-op (a type implementing both IDisposable AND IAsyncDisposable). The flow
1854-
// detector already treats DisposeAsync() as a release, so an undisposed local of such a type is a
1855-
// real leak — conservatively refuse to exempt any type that declares its own DisposeAsync (Codex).
1856-
if (nt.GetMembers("DisposeAsync").OfType<IMethodSymbol>().Any(m => m.Parameters.Length == 0))
1884+
// compatibility no-op. The flow detector treats DisposeAsync() as a release, so an undisposed
1885+
// local of such a type is a real leak. ANY IAsyncDisposable in the interface set — declared on
1886+
// this type OR INHERITED from a base (review #240 P1: the base's real DisposeAsync would
1887+
// otherwise be silently skipped, exactly the #238 false-negative class) — disqualifies the
1888+
// exemption: a type with async disposal is no longer the "simple enumerator holding nothing"
1889+
// this narrow rule is for.
1890+
if (nt.AllInterfaces.Any(i => i.Name == "IAsyncDisposable"
1891+
&& i.ContainingNamespace?.ToString() == "System"))
18571892
return false;
1858-
var dispose = nt.GetMembers("Dispose").OfType<IMethodSymbol>().FirstOrDefault(
1859-
m => m.Parameters.Length == 0 && m.ReturnsVoid && m.TypeParameters.Length == 0);
1860-
if (dispose is null || dispose.DeclaringSyntaxReferences.Length == 0)
1893+
// Belt-and-braces for a bare `DisposeAsync()` method that the flow detector may treat as a
1894+
// release without the type formally implementing IAsyncDisposable (declared or explicit-impl).
1895+
// Walk the ENTIRE base chain (review #240 round 2): GetMembers() returns only members declared
1896+
// on the current type, so a base with a bare DisposeAsync (no IAsyncDisposable, so AllInterfaces
1897+
// above misses it) would otherwise slip its real async cleanup through as an exemption.
1898+
for (var cur = nt; cur is not null && cur.SpecialType != SpecialType.System_Object; cur = cur.BaseType)
1899+
if (cur.GetMembers().OfType<IMethodSymbol>().Any(m => m.Parameters.Length == 0
1900+
&& (m.Name == "DisposeAsync"
1901+
|| m.ExplicitInterfaceImplementations.Any(x => x.Name == "DisposeAsync"))))
1902+
return false;
1903+
// #238 coverage: `void IDisposable.Dispose() { }` (explicit interface implementation — the
1904+
// actual ClosedXML Slice.Enumerator spelling) is a Dispose too; GetMembers("Dispose") misses
1905+
// it because its metadata name is "System.IDisposable.Dispose".
1906+
var disposes = nt.GetMembers().OfType<IMethodSymbol>().Where(m =>
1907+
m.Parameters.Length == 0 && m.ReturnsVoid && m.TypeParameters.Length == 0
1908+
&& (m.Name == "Dispose"
1909+
|| m.ExplicitInterfaceImplementations.Any(
1910+
x => x.ContainingType.SpecialType == SpecialType.System_IDisposable)))
1911+
.ToList();
1912+
if (disposes.Count == 0 || disposes.Any(d => d.DeclaringSyntaxReferences.Length == 0))
18611913
return false; // no readable source Dispose (metadata-only) -> cannot prove empty
1862-
return dispose.DeclaringSyntaxReferences.All(
1863-
sref => sref.GetSyntax() is MethodDeclarationSyntax { Body.Statements.Count: 0 });
1914+
if (!disposes.All(d => d.DeclaringSyntaxReferences.All(
1915+
sref => sref.GetSyntax() is MethodDeclarationSyntax { Body.Statements.Count: 0 })))
1916+
return false;
1917+
// #238 defense in depth: a FodyWeavers.xml anywhere above the type's source file means a
1918+
// weaver CAN rewrite this Dispose at build time — never exempt, even an enumerator.
1919+
return !SourceTreeHasWeaverConfig(nt);
1920+
}
1921+
1922+
// #238: walk up from the type's declaring source file looking for FodyWeavers.xml (the Fody
1923+
// weaver manifest — Janitor/Costura/etc. rewrite method bodies after compilation). A type we
1924+
// cannot locate on disk is conservatively treated as weavable (no exemption).
1925+
static bool SourceTreeHasWeaverConfig(INamedTypeSymbol nt)
1926+
{
1927+
var path = nt.DeclaringSyntaxReferences.FirstOrDefault()?.SyntaxTree.FilePath;
1928+
if (string.IsNullOrEmpty(path))
1929+
return true;
1930+
try
1931+
{
1932+
var full = Path.GetFullPath(path);
1933+
// A linked source compiled by a Fody-enabled project (Codex on #240): the weaver
1934+
// config sits next to the OWNING .csproj, not above the file — ProjectCsFiles
1935+
// recorded such files while expanding the project's <Compile> items.
1936+
if (WeaverOwnedFiles.Contains(full))
1937+
return true;
1938+
return AncestorsHaveWeaverConfig(Path.GetDirectoryName(full));
1939+
}
1940+
catch (Exception e) when (e is IOException or UnauthorizedAccessException or ArgumentException)
1941+
{
1942+
return true; // cannot inspect the tree -> cannot rule a weaver out -> no exemption
1943+
}
1944+
}
1945+
1946+
// #238/#240: FodyWeavers.xml in `startDir` or any ancestor. GENUINELY fail-closed on an
1947+
// unreadable ancestor (review #240 P3): `File.Exists` returns false on access-denied — swallowing
1948+
// exactly the case we must treat as "a weaver may be present" — so we probe with
1949+
// `File.GetAttributes`, which THROWS on a permission error while still reporting a plain
1950+
// not-found. Absent config -> keep walking; unreadable state -> withhold the exemption.
1951+
static bool AncestorsHaveWeaverConfig(string? startDir)
1952+
{
1953+
try
1954+
{
1955+
for (var dir = startDir; !string.IsNullOrEmpty(dir); dir = Path.GetDirectoryName(dir))
1956+
{
1957+
var candidate = Path.Combine(dir, "FodyWeavers.xml");
1958+
try
1959+
{
1960+
_ = File.GetAttributes(candidate);
1961+
return true; // present AND readable -> a weaver is configured here
1962+
}
1963+
catch (FileNotFoundException) { } // no config in this dir — keep walking
1964+
catch (DirectoryNotFoundException) { } // this ancestor is gone — keep walking
1965+
}
1966+
}
1967+
catch (Exception e) when (e is IOException or UnauthorizedAccessException
1968+
or System.Security.SecurityException or ArgumentException)
1969+
{
1970+
return true; // cannot prove the config is absent -> fail closed, no exemption
1971+
}
1972+
return false;
18641973
}
18651974

18661975
// A type that is System.Windows.Forms.Form or derives from it (semantic, walks the
@@ -5657,4 +5766,10 @@ or ImplicitObjectCreationExpressionSyntax } init
56575766
partial class Program
56585767
{
56595768
internal static bool BodyThrowEdges;
5769+
5770+
// #240 (Codex review): full paths of files compiled by a Fody-enabled PROJECT, recorded
5771+
// while expanding its <Compile> items — a linked source outside the project directory
5772+
// would otherwise dodge the ancestor-walk weaver check (the FodyWeavers.xml lives next
5773+
// to the .csproj, not above the linked file).
5774+
internal static readonly HashSet<string> WeaverOwnedFiles = new(StringComparer.Ordinal);
56605775
}

0 commit comments

Comments
 (0)