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
32 changes: 32 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,38 @@ jobs:
fi
done
echo "OK: escape-via-projection UnitOfWork leak -> OWN001 'never disposed'; materialize + ownership-transfer fixes stay silent"
- name: WinForms modeless-form precision (--flow-locals, P-016)
run: |
# WinForms owns a *modeless* form's lifetime: a form shown via Form.Show()
# is disposed by the framework on close, so the extractor models that Show()
# as a RELEASE at the show site (ownership transfers to the framework there).
# A *modal* dialog shown via ShowDialog() is the caller's to dispose ->
# ShowDialog is NOT a release, so it stays tracked and an undisposed one is a
# real OWN001. Reduced from a ShareX (WinForms) false positive: our WPF-tuned
# local-disposable detector over-fired on the idiomatic `new SomeForm().Show()`.
dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \
frontend/roslyn/samples/WinFormsModelessSample.cs --flow-locals -o "$RUNNER_TEMP/winforms.json"
out=$(python -m ownlang ownir "$RUNNER_TEMP/winforms.json" || true)
echo "$out"
# the modal dialog never disposed is a real leak (ShowDialog is caller-owned):
echo "$out" | grep -qE "OWN001.*'modalLeak' is never disposed" \
|| { echo "FAIL: expected OWN001 on the undisposed ShowDialog modal dialog"; exit 1; }
echo "$out" | grep -q "WinFormsModelessSample.cs" \
|| { echo "FAIL: expected the finding at the C# sample location"; exit 1; }
# because Show() is a release AT THE SHOW SITE (path-sensitive, not a method-wide
# exemption), a form shown only on one branch leaks on the branch that never shows
# it -> OWN001 'may not be disposed on every path' (Codex review on PR #57).
echo "$out" | grep -qE "OWN001.*'condForm' may not be disposed on every path" \
|| { echo "FAIL: expected OWN001 on the conditionally-shown modeless form"; exit 1; }
# the precision fix: an unconditionally-shown modeless form (`.Show()`, ownership
# transferred to the framework) must stay silent, and so must a properly-disposed
# modal dialog (modalOk):
for ok in modeless modalOk; do
if echo "$out" | grep -q "'$ok'"; then
echo "FAIL: silent case '$ok' was reported (WinForms precision)"; exit 1
fi
done
echo "OK: WinForms modeless Form.Show() = call-site release (framework-owned); conditional show leaks on the no-show path; modal ShowDialog() leak caught; disposed modal silent"

# The distribution surface (Уровень 1): the own-check.sh orchestrator walks a
# directory of real C# and prints findings in the host-parseable formats the
Expand Down
128 changes: 128 additions & 0 deletions docs/notes/winforms-modeless-precision.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
# WinForms modeless-`Form` precision — the framework owns a `.Show()`'d form

A precision fix driven by mining **ShareX** (a large, real WinForms app) with the
`--flow-locals` local-`IDisposable` detector. The hunt's honest deliverable was a
*precision-hardening backlog*, not a pile of real bugs: our detector was tuned on
WPF, and WPF and WinForms differ on **who owns a window's lifetime**. This note is
the first item off that backlog.

## The false-positive class

The flow detector flags a local `IDisposable` that is constructed and never
disposed (OWN001). On WinForms that over-fires on the single most idiomatic line in
the framework:

```csharp
void OpenSettings()
{
var form = new SettingsForm(); // SettingsForm : System.Windows.Forms.Form
form.Show(); // modeless — returns immediately
}
```

`form` is a `Form` (an `IDisposable`), constructed, used, and never disposed — so
the WPF-tuned detector calls it a leak. **It is not.** `Control.Show()` opens the
form *modeless*: the window stays open after the method returns, and **WinForms
itself disposes the form when the user closes it** (the framework owns the
lifetime). Disposing it yourself in `OpenSettings` would be a bug — you'd dispose a
window that is still on screen. This shape is everywhere in a WinForms app (every
"open a tool window" handler), so the FP is not incidental; it is systematic.

The contrast is a **modal** dialog:

```csharp
var dlg = new ConfirmDialog();
dlg.ShowDialog(); // modal — blocks, returns a DialogResult
// dlg is the CALLER's to dispose here — leaked if never disposed
```

`ShowDialog()` blocks and returns; the caller owns the dialog and **must** dispose
it (the canonical `using var dlg = new ConfirmDialog();` pattern). An undisposed
`ShowDialog()`'d form *is* a real leak — and must stay flagged.

So the discriminator is exactly the show method: **`Show()` → framework-owned;
`ShowDialog()` → caller-owned (tracked).**

## The fix — `Show()` is a release at the call site

The first cut dropped a `Form` local from the tracked set whenever a `local.Show()`
appeared *anywhere* in the method body — a method-wide exemption. That is not
path-sensitive: `var f = new Form(); if (open) f.Show();` would go silent even though
the `open == false` path constructs a form that is never handed to WinForms and never
disposed (a real leak). Codex flagged exactly this (PR #57 review).

The right model is the project's own **call-site release** shape — the same one the
pool `Return` and the inter-procedural consume contract already use: a modeless
`local.Show()` *transfers ownership to the framework on that path*, so the flow
detector emits a **`release` of the local at the show site**. One helper plus one
branch in the flow lowering (`EmitFlowExpr`):

```csharp
// System.Windows.Forms.Form or a subclass (semantic, walks the base chain).
static bool DerivesFromWinFormsForm(ITypeSymbol? t)
{
for (var b = t; b is not null; b = b.BaseType)
if (b.Name == "Form" && b.ContainingNamespace?.ToString() == "System.Windows.Forms")
return true;
return false;
}

// in EmitFlowExpr, alongside the Dispose()/Close()/pool-Return/consume releases:
// x.Show() on a tracked Form-derived local -> release (ownership -> framework).
if (expr is InvocationExpressionSyntax sinv
&& sinv.Expression is MemberAccessExpressionSyntax sma
&& sma.Name.Identifier.Text == "Show"
&& sma.Expression is IdentifierNameSyntax sid
&& tracked.Contains(sid.Identifier.Text)
&& DerivesFromWinFormsForm(model.GetTypeInfo(sma.Expression).Type))
{
nodes.Add(new { op = "release", var = sid.Identifier.Text, line = LineOf(sinv) });
return;
}
```

Because the release lands *on the show path*, the flow engine does the rest
path-sensitively:

- `var f = new Form(); f.Show();` — acquire + release → balanced → **silent**;
- `var f = new Form(); if (open) f.Show();` — released on the `then` path, not the
`else` → **OWN001 "may not be disposed on every path"** (the leak the method-wide
exemption hid);
- `var d = new Form(); d.ShowDialog();` — `ShowDialog` is **not** matched (only
`Show`), so it stays a tracked *use* → an undisposed modal dialog is still
**OWN001**.

It is `Show()`-only and `Form`-derived-guarded, so a `ShowDialog()`'d form and any
non-`Form` disposable with a `Show()` method are untouched. No core change — it reuses
the existing `release` op exactly as the other call-site-release branches do.

## Pinned in CI (validated where the frontend always is)

The fix lands *with* `frontend/roslyn/samples/WinFormsModelessSample.cs`, wired into
the `wpf-extractor` job's `--flow-locals` steps. It is **self-contained**: a stub
`namespace System.Windows.Forms { public class Form : System.IDisposable { … } }`
stands in for the framework type (the WinForms reference pack is not loaded in that
step), matched by simple name + namespace exactly as the real one is. The sample
asserts the whole discriminator, path-sensitivity included:

- `OpenModeless` — `new ModelessForm().Show()` → **silent** (ownership → framework;
the FP the fix removes);
- `OpenModelessConditional` — `if (open) condForm.Show()` → **OWN001** (`'condForm'
may not be disposed on every path` — the no-show path leaks; pins the
path-sensitivity Codex asked for);
- `OpenModalLeak` — `new ModalDialog().ShowDialog()`, never disposed → **OWN001**
(`'modalLeak' is never disposed`, caller-owned leak — stays flagged);
- `OpenModalOk` — the same modal dialog disposed on every path → **silent** (proving
the leak above is about *disposal*, not poisoned by `ShowDialog` itself).

The Python-core half (facts → verdict) was validated locally on the hand-built flow
facts before pushing; the C# half (source → facts) is validated in CI by the sample,
the same place the rest of the frontend is.

## Scope and the rest of the backlog

This closes the one systematic WinForms FP — modeless forms — with a tight,
`Show()`-only call-site release. The broader WinForms ownership story (a `Control` added to a
parent's `Controls` collection is disposed by the parent; `components`-container
disposal) is a separate slice on the precision backlog the ShareX hunt produced, to
be taken the same way: one FP class, one sample, one CI pin.
30 changes: 30 additions & 0 deletions frontend/roslyn/OwnSharp.Extractor/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,17 @@
|| (ns == "System.Data" && t.Name is "DataTable" or "DataSet" or "DataView");
}

// A type that is System.Windows.Forms.Form or derives from it (semantic, walks the
// base chain). A modeless `Form.Show()` transfers ownership to the framework, which
// disposes the form on close — EmitFlowExpr models that show as a release.
static bool DerivesFromWinFormsForm(ITypeSymbol? t)
{
for (var b = t; b is not null; b = b.BaseType)
if (b.Name == "Form" && b.ContainingNamespace?.ToString() == "System.Windows.Forms")
return true;
return false;
}

static string MethodName(BaseMethodDeclarationSyntax m) => m switch
{
MethodDeclarationSyntax md => md.Identifier.Text,
Expand Down Expand Up @@ -682,6 +693,25 @@
nodes.Add(new { op = "release", var = rid.Identifier.Text, line = LineOf(inv) });
return;
}
// x.Show() on a tracked WinForms Form-derived local -> release: a modeless form's
// ownership transfers to the framework, which disposes it when the user closes it.
// Modeled as a release AT THE SHOW SITE (not a method-wide exemption), so it stays
// path-sensitive — a form shown only on one branch still leaks on the branch that
// never shows it (Codex review on PR #57). ShowDialog() is a *modal* show and is
// NOT matched: the caller owns a ShowDialog'd form and must dispose it, so it stays
// a tracked use (a real leak if never disposed). Guarded by the Form-derived type so
// an unrelated IDisposable with a Show() method is not mistaken for an ownership
// transfer.
if (expr is InvocationExpressionSyntax sinv
&& sinv.Expression is MemberAccessExpressionSyntax sma
&& sma.Name.Identifier.Text == "Show"
&& sma.Expression is IdentifierNameSyntax sid
&& tracked.Contains(sid.Identifier.Text)
&& DerivesFromWinFormsForm(model.GetTypeInfo(sma.Expression).Type))
{
nodes.Add(new { op = "release", var = sid.Identifier.Text, line = LineOf(sinv) });
return;
}
// x?.Dispose()/x?.Close()/x?.DisposeAsync() (null-conditional) is the release too — the
// call is a member BINDING under a conditional access, not a member access. Mirrors
// IsDisposeShaped so a `?.` dispose (e.g. in a finally) is not mistaken for a bare use,
Expand Down Expand Up @@ -1051,7 +1081,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 1084 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check SARIF -> GitHub code scanning (dog-food)

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 1084 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 1084 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check SARIF -> GitHub code scanning (dog-food)

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 1084 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 1084 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 1084 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
77 changes: 77 additions & 0 deletions frontend/roslyn/samples/WinFormsModelessSample.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
using System;

// P-016 precision (--flow-locals): WinForms owns a *modeless* form's lifetime.
// A form shown modeless via Form.Show() is disposed by the framework when the user
// closes it, so the extractor models that Show() as a RELEASE at the show site —
// ownership transfers to the framework there (the same call-site release shape as
// pool Return and the consume contract). Because it is modeled per-path, not as a
// method-wide exemption, a form shown only on one branch still leaks on the branch
// that never shows it (see OpenModelessConditional). A *modal* dialog shown via
// ShowDialog() is the caller's to dispose: ShowDialog is NOT a release, so it stays
// tracked, and an undisposed one is a real OWN001.
//
// Reduced from a false positive found mining ShareX (WinForms), where our WPF-tuned
// local-disposable detector over-fired on the idiomatic `new SomeForm().Show()`.
//
// Self-contained: the stub System.Windows.Forms.Form below stands in for the
// framework type (the WinForms reference pack is not loaded in this flow step). The
// extractor matches Form by simple name + `System.Windows.Forms` namespace, walking
// the base chain, so these stubs reproduce the real shape exactly.
public class WinFormsModelessSample
{
// NOT a leak: a modeless form (`.Show()`) transfers ownership to the framework
// on that path -> acquire+release balanced -> silent (the precision fix; was a
// false OWN001 before).
public void OpenModeless()
{
var modeless = new ModelessForm();
modeless.Show();
}

// OWN001 (recall, Codex review on PR #57): a form shown only on ONE branch leaks
// on the path that never shows it. Show() is a release AT THE SHOW SITE, so the
// `open == false` path — construct, never shown, never disposed — is correctly
// caught ('condForm' may not be disposed on every path). A method-wide exemption
// would have wrongly silenced this.
public void OpenModelessConditional(bool open)
{
var condForm = new ModelessForm();
if (open)
condForm.Show();
}

// OWN001: a modal dialog (`.ShowDialog()`) is the caller's to dispose; this one
// never is -> real leak. ShowDialog() is NOT modeled as a release (only Show is).
public void OpenModalLeak()
{
var modalLeak = new ModalDialog();
modalLeak.ShowDialog();
}

// NOT a leak: the same modal dialog, this time disposed on every path -> silent.
// Proves the ShowDialog leak above is about disposal (not poisoned by ShowDialog
// itself): ShowDialog + Dispose is balanced, ShowDialog alone leaks.
public void OpenModalOk()
{
var modalOk = new ModalDialog();
modalOk.ShowDialog();
modalOk.Dispose();
}
}

public class ModelessForm : System.Windows.Forms.Form { }

public class ModalDialog : System.Windows.Forms.Form { }

// Stub standing in for the WinForms framework type (the reference pack is not loaded
// in the self-contained flow step). The extractor matches Form by simple name +
// `System.Windows.Forms` namespace, so this reproduces the real ownership shape.
namespace System.Windows.Forms
{
public class Form : System.IDisposable
{
public void Show() { }
public int ShowDialog() => 0;
public void Dispose() { }
}
}
Loading