fix(extractor): confine the empty-Dispose exemption to enumerators — source emptiness ≠ runtime no-op (Closes #238)

.github/workflows/ci.yml

@@ -837,14 +837,13 @@ jobs:
           # ...while the same-named local in the OTHER method must still warn.
           echo "$out" | grep -qE "TemplatePartLocalCaptureSample\.cs:[0-9]+: warning: \[OWN001\].*'SameNameDifferentScopeSubscriber'" \
             || { echo "FAIL: expected OWN001 on the same-named-but-unrelated local in a different method"; exit 1; }
-          # issue #225 — a user-defined type whose Dispose() body is provably EMPTY holds no resource,
-          # so a LOCAL of it never disposed is not a leak (extends the named-BCL no-op exemption to any
-          # type). Here the flat (non-flow) name path: a `*Reader`-named empty-Dispose local (ScratchReader)
-          # is silent, while a non-empty `*Reader` local (LeakyReader) still leaks. (The semantic enumerator
-          # form is exercised in the --flow-locals step below.)
-          if echo "$out" | grep -q "'ScratchReader'"; then
-            echo "FAIL: a local of a user type with a provably-empty Dispose() body was wrongly flagged (#225)"; exit 1
-          fi
+          # issue #225 (narrowed by #238) — the empty-Dispose exemption is confined to ENUMERATOR
+          # types: source emptiness proves nothing about the COMPILED body once an IL weaver is in
+          # play (Janitor.Fody wove real cleanup into ClosedXML's XLWorkbook.Dispose — 263 silently
+          # swallowed findings). ScratchReader (empty source Dispose, NOT an enumerator — the
+          # XLWorkbook shape) must therefore STAY flagged on the flat name path...
+          echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'s'.*'ScratchReader'" \
+            || { echo "FAIL: #238: a NON-enumerator empty-source-Dispose local must stay flagged (weaver soundness)"; exit 1; }
           echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'lr'.*'LeakyReader'" \
             || { echo "FAIL: a non-empty-Dispose local (LeakyReader) must still leak (#225 stays scoped)"; exit 1; }
           # Codex P2: an empty SYNC Dispose but a real DisposeAsync (a `*Reader` here) must still leak.
@@ -1124,11 +1123,44 @@ jobs:
           # must NOT be exempted — the real cleanup is async and the flow detector treats it as a release.
           echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'ar' is never disposed" \
             || { echo "FAIL: #225 flow: a type with a real DisposeAsync (empty sync Dispose) must still leak"; exit 1; }
-          # exactly 4 EmptyDisposeSample findings -> the two provably-empty-Dispose locals (the enumerator
-          # and ScratchReader) are SILENT; any leak of them would push the count past 4.
+          # #240 review P1: an enumerator whose sync Dispose is empty but whose BASE owns a real
+          # DisposeAsync (inherited IAsyncDisposable) must still leak — inherited async cleanup was
+          # the same silent-false-negative class #238 exists to close.
+          echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'ia' is never disposed" \
+            || { echo "FAIL: #240 flow: an inherited-IAsyncDisposable enumerator must still leak"; exit 1; }
+          # #240 review round 2: a BASE with a bare DisposeAsync (no IAsyncDisposable interface) must
+          # be caught by the base-chain scan -> still leak.
+          echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'ba' is never disposed" \
+            || { echo "FAIL: #240 flow: a base-chain bare DisposeAsync enumerator must still leak"; exit 1; }
+          # #240 review round 2: a NON-generic IEnumerator (does not force IDisposable) must stay
+          # flagged -> pins the IEnumerator<T>-only narrowing.
+          echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'ng' is never disposed" \
+            || { echo "FAIL: #240 flow: a non-generic-IEnumerator empty-Dispose local must still leak"; exit 1; }
+          # #238 soundness: ScratchReader (empty source Dispose, NOT an enumerator) must stay flagged
+          # in the flow path too — a weaver can rewrite a non-enumerator Dispose at build time.
+          echo "$out" | grep -qE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'s' is never disposed" \
+            || { echo "FAIL: #238 flow: a NON-enumerator empty-source-Dispose local must stay flagged"; exit 1; }
+          # exactly 5 EmptyDisposeSample findings -> the two provably-empty-Dispose ENUMERATOR locals
+          # ('e' plain, 'x' explicit-interface — the #238 coverage fix) are SILENT; any leak of them
+          # would push the count past 8 (controls: s,r,lr,d,ar,ia,ba,ng).
           n225=$(echo "$out" | grep -cE "EmptyDisposeSample\.cs:[0-9]+:.*\[OWN00")
-          [ "$n225" = "4" ] \
-            || { echo "FAIL: #225 flow: expected exactly 4 EmptyDisposeSample findings (controls only), got $n225"; exit 1; }
+          [ "$n225" = "8" ] \
+            || { echo "FAIL: #225/#238 flow: expected exactly 8 EmptyDisposeSample findings (controls only), got $n225"; exit 1; }
+          # #238 weaver kill-switch: a directory carrying FodyWeavers.xml disables the exemption even
+          # for a perfect enumerator shape — the weaved fixture's local must be FLAGGED.
+          dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \
+            frontend/roslyn/samples/weaved/WeavedEmptyDispose.cs --flow-locals -o "$RUNNER_TEMP/weaved.json"
+          wout=$(python -m ownlang ownir "$RUNNER_TEMP/weaved.json" || true)
+          echo "$wout" | grep -qE "WeavedEmptyDispose\.cs:[0-9]+:.*\[OWN001\].*'we' is never disposed" \
+            || { echo "FAIL: #238: FodyWeavers.xml present -> even an enumerator empty Dispose must stay flagged"; exit 1; }
+          # ...and the LINKED-source variant (Codex on #240): the shared file lives OUTSIDE the
+          # weaver project's directory (no FodyWeavers.xml above it) — analysed THROUGH the
+          # .csproj, the project's weaver config must still disable the exemption.
+          dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \
+            frontend/roslyn/samples/weaved-linked/Proj/Linked.csproj --flow-locals -o "$RUNNER_TEMP/weavedlink.json"
+          lout=$(python -m ownlang ownir "$RUNNER_TEMP/weavedlink.json" || true)
+          echo "$lout" | grep -qE "SharedEnumerator\.cs:[0-9]+:.*\[OWN001\].*'se' is never disposed" \
+            || { echo "FAIL: #240: a linked source of a Fody-enabled project must not get the empty-Dispose exemption"; exit 1; }
           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)"
       - name: Gallery C#-native bad/ok pairs (examples/gallery/cs/)
         run: |

frontend/roslyn/OwnSharp.Extractor/Program.cs

@@ -81,6 +81,10 @@
 // run that did not request it (the other config — emitEvents/flowLocals/reportStats — are locals,
 // re-initialized each call, so they need no reset; only this static one does). CodeRabbit.
 BodyThrowEdges = false;
+// #240 (review P2): WeaverOwnedFiles is the other process-global mutable state — a file recorded
+// as belonging to a Fody-enabled project must not stay "weaver-owned" for an unrelated later
+// invocation in the same process (a sticky false positive). Same reset discipline as above.
+WeaverOwnedFiles.Clear();
 // `extract` verb: the tool's one job is extraction, so an optional leading `extract`
 // makes the advertised `ownsharp-extract extract --project App.csproj --out facts.json`
 // UX real while the bare form (no verb) stays the default and back-compatible. (The
@@ -309,6 +313,13 @@
         result.RemoveAll(f => rmMatch.Any(m => m(f)));
     }
 
+    // #240: if THIS project is weaver-enabled (FodyWeavers.xml in its directory or above),
+    // EVERY file it compiles — including linked sources outside its tree — is weavable at
+    // build time; record them so the empty-Dispose kill-switch sees past the file's own path.
+    if (AncestorsHaveWeaverConfig(dir))
+        foreach (var f in result)
+            WeaverOwnedFiles.Add(Path.GetFullPath(f));
+
     return result.Distinct().ToList();
 }
 
@@ -1420,36 +1431,134 @@
     return arg0 is not null && IsInMemoryDisposableBacking(model.GetTypeInfo(arg0).Type);
 }
 
-// Issue #225 — a user-defined type whose parameterless `Dispose()` body is provably EMPTY holds no
-// resource behind the interface: it implements IDisposable only to satisfy a contract (e.g.
-// `IEnumerator<T>`), so never calling Dispose() cannot leak. Recognised from SOURCE: the type
-// declares `void Dispose()` with a BLOCK body of ZERO statements, and no base in its chain carries a
-// Dispose to cascade to (the base is `object` / not IDisposable), so nothing real is skipped. A body
-// we cannot read (third-party metadata-only, or expression-bodied doing work) is NOT provably empty
-// and stays flagged — never a guessed drop. This generalises the named-BCL IsNoOpDisposeWrapper /
-// IsDisposeOptional idea to any type. Deliberately scoped to LOCAL disposables (the #201-sweep
+// Issue #225 (narrowed by #238) — a user-defined `IEnumerator<T>` whose parameterless `Dispose()`
+// body is provably EMPTY holds no resource behind the interface: `IEnumerator<T>` is the one
+// interface that FORCES IDisposable onto types that usually hold nothing, so never calling its
+// empty Dispose() cannot leak. Recognised from SOURCE: the type declares `void Dispose()` (plain or
+// explicit-interface) with a BLOCK body of ZERO statements, and no base in its chain carries a
+// Dispose to cascade to, no IAsyncDisposable / bare DisposeAsync anywhere in the type or its bases,
+// and no FodyWeavers.xml above it (an IL weaver could rewrite the "empty" body at build time — the
+// #238 soundness lesson). A body we cannot read (metadata-only, or expression-bodied doing work) is
+// NOT provably empty and stays flagged — never a guessed drop. Deliberately NOT "any type": that
+// over-broad framing was the #238 regression (263 silently-swallowed ClosedXML findings). Scoped to
+// LOCAL disposables (the #201-sweep
 // evidence — ClosedXML's `Slice.Enumerator` locals): a FIELD keeps its own disposal contract, so an
 // empty-Dispose field (the OwnIgnoreSample `Handle` stand-in) is unaffected.
 static bool HasEmptyDisposeBody(ITypeSymbol? t)
 {
     if (t is not INamedTypeSymbol nt)
         return false;
+    // Issue #238 SOUNDNESS gate: an empty SOURCE body proves nothing about the COMPILED body —
+    // IL weavers (Janitor.Fody wove the real cleanup into ClosedXML's XLWorkbook.Dispose(),
+    // 263 silently-swallowed findings) rewrite Dispose at build time, and source-level analysis
+    // cannot prove their absence. So the exemption is confined to the ONE shape whose empty
+    // Dispose is FORCED by the language, not authored: `IEnumerator<T>` — the generic enumerator
+    // interface is the only one that transitively requires IDisposable (IEnumerator<T> : IEnumerator,
+    // IDisposable). The non-generic System.Collections.IEnumerator does NOT extend IDisposable, so a
+    // type that pairs them coupled the two contracts itself — the motivating proof does not apply, and
+    // it keeps the honest warning (review #240).
+    // Matched by name/arity/namespace, not SpecialType: in the source-only extraction (no wired
+    // corelib reference) the well-known-type SpecialType is often unset, so `IEnumerator<T>` would
+    // slip through a SpecialType check. `System.Collections.Generic.IEnumerator`1` is the exact
+    // interface that transitively forces IDisposable.
+    if (!nt.AllInterfaces.Any(i => i.Name == "IEnumerator"
+                                   && i.TypeArguments.Length == 1
+                                   && i.ContainingNamespace?.ToString() == "System.Collections.Generic"))
+        return false;
     // The base must not own a Dispose we'd be silently skipping — only `object` / a non-IDisposable base.
     for (var b = nt.BaseType; b is not null && b.SpecialType != SpecialType.System_Object; b = b.BaseType)
         if (ImplementsIDisposable(b))
             return false;
     // A non-empty DisposeAsync() can do the REAL cleanup even when the sync Dispose() is an empty
-    // compatibility no-op (a type implementing both IDisposable AND IAsyncDisposable). The flow
-    // detector already treats DisposeAsync() as a release, so an undisposed local of such a type is a
-    // real leak — conservatively refuse to exempt any type that declares its own DisposeAsync (Codex).
-    if (nt.GetMembers("DisposeAsync").OfType<IMethodSymbol>().Any(m => m.Parameters.Length == 0))
+    // compatibility no-op. The flow detector treats DisposeAsync() as a release, so an undisposed
+    // local of such a type is a real leak. ANY IAsyncDisposable in the interface set — declared on
+    // this type OR INHERITED from a base (review #240 P1: the base's real DisposeAsync would
+    // otherwise be silently skipped, exactly the #238 false-negative class) — disqualifies the
+    // exemption: a type with async disposal is no longer the "simple enumerator holding nothing"
+    // this narrow rule is for.
+    if (nt.AllInterfaces.Any(i => i.Name == "IAsyncDisposable"
+                                  && i.ContainingNamespace?.ToString() == "System"))
         return false;
-    var dispose = nt.GetMembers("Dispose").OfType<IMethodSymbol>().FirstOrDefault(
-        m => m.Parameters.Length == 0 && m.ReturnsVoid && m.TypeParameters.Length == 0);
-    if (dispose is null || dispose.DeclaringSyntaxReferences.Length == 0)
+    // Belt-and-braces for a bare `DisposeAsync()` method that the flow detector may treat as a
+    // release without the type formally implementing IAsyncDisposable (declared or explicit-impl).
+    // Walk the ENTIRE base chain (review #240 round 2): GetMembers() returns only members declared
+    // on the current type, so a base with a bare DisposeAsync (no IAsyncDisposable, so AllInterfaces
+    // above misses it) would otherwise slip its real async cleanup through as an exemption.
+    for (var cur = nt; cur is not null && cur.SpecialType != SpecialType.System_Object; cur = cur.BaseType)
+        if (cur.GetMembers().OfType<IMethodSymbol>().Any(m => m.Parameters.Length == 0
+                && (m.Name == "DisposeAsync"
+                    || m.ExplicitInterfaceImplementations.Any(x => x.Name == "DisposeAsync"))))
+            return false;
+    // #238 coverage: `void IDisposable.Dispose() { }` (explicit interface implementation — the
+    // actual ClosedXML Slice.Enumerator spelling) is a Dispose too; GetMembers("Dispose") misses
+    // it because its metadata name is "System.IDisposable.Dispose".
+    var disposes = nt.GetMembers().OfType<IMethodSymbol>().Where(m =>
+            m.Parameters.Length == 0 && m.ReturnsVoid && m.TypeParameters.Length == 0
+            && (m.Name == "Dispose"
+                || m.ExplicitInterfaceImplementations.Any(
+                       x => x.ContainingType.SpecialType == SpecialType.System_IDisposable)))
+        .ToList();
+    if (disposes.Count == 0 || disposes.Any(d => d.DeclaringSyntaxReferences.Length == 0))
         return false;   // no readable source Dispose (metadata-only) -> cannot prove empty
-    return dispose.DeclaringSyntaxReferences.All(
-        sref => sref.GetSyntax() is MethodDeclarationSyntax { Body.Statements.Count: 0 });
+    if (!disposes.All(d => d.DeclaringSyntaxReferences.All(
+            sref => sref.GetSyntax() is MethodDeclarationSyntax { Body.Statements.Count: 0 })))
+        return false;
+    // #238 defense in depth: a FodyWeavers.xml anywhere above the type's source file means a
+    // weaver CAN rewrite this Dispose at build time — never exempt, even an enumerator.
+    return !SourceTreeHasWeaverConfig(nt);
+}
+
+// #238: walk up from the type's declaring source file looking for FodyWeavers.xml (the Fody
+// weaver manifest — Janitor/Costura/etc. rewrite method bodies after compilation). A type we
+// cannot locate on disk is conservatively treated as weavable (no exemption).
+static bool SourceTreeHasWeaverConfig(INamedTypeSymbol nt)
+{
+    var path = nt.DeclaringSyntaxReferences.FirstOrDefault()?.SyntaxTree.FilePath;
+    if (string.IsNullOrEmpty(path))
+        return true;
+    try
+    {
+        var full = Path.GetFullPath(path);
+        // A linked source compiled by a Fody-enabled project (Codex on #240): the weaver
+        // config sits next to the OWNING .csproj, not above the file — ProjectCsFiles
+        // recorded such files while expanding the project's <Compile> items.
+        if (WeaverOwnedFiles.Contains(full))
+            return true;
+        return AncestorsHaveWeaverConfig(Path.GetDirectoryName(full));
+    }
+    catch (Exception e) when (e is IOException or UnauthorizedAccessException or ArgumentException)
+    {
+        return true;   // cannot inspect the tree -> cannot rule a weaver out -> no exemption
+    }
+}
+
+// #238/#240: FodyWeavers.xml in `startDir` or any ancestor. GENUINELY fail-closed on an
+// unreadable ancestor (review #240 P3): `File.Exists` returns false on access-denied — swallowing
+// exactly the case we must treat as "a weaver may be present" — so we probe with
+// `File.GetAttributes`, which THROWS on a permission error while still reporting a plain
+// not-found. Absent config -> keep walking; unreadable state -> withhold the exemption.
+static bool AncestorsHaveWeaverConfig(string? startDir)
+{
+    try
+    {
+        for (var dir = startDir; !string.IsNullOrEmpty(dir); dir = Path.GetDirectoryName(dir))
+        {
+            var candidate = Path.Combine(dir, "FodyWeavers.xml");
+            try
+            {
+                _ = File.GetAttributes(candidate);
+                return true;   // present AND readable -> a weaver is configured here
+            }
+            catch (FileNotFoundException) { }        // no config in this dir — keep walking
+            catch (DirectoryNotFoundException) { }    // this ancestor is gone — keep walking
+        }
+    }
+    catch (Exception e) when (e is IOException or UnauthorizedAccessException
+                              or System.Security.SecurityException or ArgumentException)
+    {
+        return true;   // cannot prove the config is absent -> fail closed, no exemption
+    }
+    return false;
 }
 
 // A type that is System.Windows.Forms.Form or derives from it (semantic, walks the
@@ -3931,7 +4040,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);
 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
@@ -4342,7 +4451,7 @@
                              && aliasToField.TryGetValue(ls, out var fa) ? fa : df);
         // Also the NULL-CONDITIONAL form `field?.Dispose()` (a ConditionalAccess whose
         // WhenNotNull is the `.Dispose()` invocation, NOT a plain MemberAccess) — the
         // dominant disposal shape the match above misses. Mined as an FP across ImageSharp:
         // `this.memoryStream?.Dispose()` (ZipExrCompressor/DeflateCompressor/IccDataWriter)
         // and the BufferedStreams benchmark's `[GlobalCleanup]` `field?.Dispose()` calls.
         // (The same alias-by-symbol translation applies — `cts?.Dispose()` on an aliasing local.)
@@ -5142,4 +5251,10 @@
 partial class Program
 {
     internal static bool BodyThrowEdges;
+
+    // #240 (Codex review): full paths of files compiled by a Fody-enabled PROJECT, recorded
+    // while expanding its <Compile> items — a linked source outside the project directory
+    // would otherwise dodge the ancestor-walk weaver check (the FodyWeavers.xml lives next
+    // to the .csproj, not above the linked file).
+    internal static readonly HashSet<string> WeaverOwnedFiles = new(StringComparer.Ordinal);
 }