fix(extractor): recognize WinForms Controls/IContainer disposal channels (Closes #219)#236
Conversation
…els (#219) WinForms child fields were false-flagged as OWN001 leaks because the field-disposal scan didn't model two designer-generated disposal channels: (a) a Control/ToolStripItem field added to THIS object's own Controls/Items collection is disposed transitively by Control.Dispose(bool) (the framework's contract); (b) a component built with `new T(components)` or registered via `components.Add(x)` is disposed by the designer's `components.Dispose()`. ~30 own-only FPs on a single ShareX scan; issue #201 sweep, field-notes 12-13. Fix (extractor-only, no OwnIR change): both channels credit a field's release ONLY when the class reaches the framework disposal root — a designer `Dispose(bool)` calling `base.Dispose(disposing)` (ClassReachesDisposalRoot). A class with no such Dispose (the ShareX HistoryItemManager true-positive) is never rooted, so its owned controls stay flagged. - (a) Collect `<coll>.Add/AddRange(this.child)` edges keyed by their container ("" = this, a field name, or null = foreign), then fixpoint from the rooted set so a ToolStrip added to this.Controls cascades to its Items. AddRange over an array initializer is flattened. The container is resolved SEMANTICALLY, so an add into a foreign (parameter/local) container yields no credit. - (b) The IContainer sink is a disposed field of type System.ComponentModel. IContainer; a field constructed with it (`new T(components)`) or passed to `components.Add(...)` is credited. A component not registered stays flagged. Precision-first, does not over-widen — negative controls all stay flagged: an add into a foreign container (lblForeign); a plain owner with no Dispose (cms/item); a component not container-registered (unregisteredIcon). Verified locally with the real extractor (.NET 8): WinFormsDisposalSample goes 10 findings -> 4 (six channel-disposed fields silent, four controls flagged); a full existing-sample diff in BOTH modes (49 samples) is byte-identical (zero regression). Gates: run_tests.py exit 0, test_ownir 276/276, ruff clean, mypy --strict green. Closes #219 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cg6DVXahkyY68Ruu7f76rG
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://cdn.github.com/in/57789?s=60&v=4){kind=small}
| private ContextMenuStrip menu; | ||
| private ToolStripMenuItem menuItem; | ||
| private NotifyIcon trayIcon; | ||
| private NotifyIcon unregisteredIcon; // NEGATIVE CONTROL (b): NOT container-registered |
| // Controls -> not a disposal channel for this class -> STAYS FLAGGED. | ||
| public sealed class ForeignContainerAdder : Control | ||
| { | ||
| private Label lblForeign; |
| // (the ShareX HistoryItemManager true-positive shape). | ||
| public sealed class MenuOwnerNoDispose | ||
| { | ||
| private ContextMenuStrip cms = new ContextMenuStrip(); |
| public sealed class MenuOwnerNoDispose | ||
| { | ||
| private ContextMenuStrip cms = new ContextMenuStrip(); | ||
| private ToolStripMenuItem item; |
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 294a4cd2bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| if (collExpr is IdentifierNameSyntax { Identifier.Text: "Controls" or "Items" }) | ||
| return ""; // an inherited `Controls`/`Items` member accessed bare -> this object | ||
| if (collExpr is not MemberAccessExpressionSyntax { Name.Identifier.Text: "Controls" or "Items" } coll) |
There was a problem hiding this comment.
Because this accepts any member named Items, a rooted field with a non-owning WinForms item collection is treated like a ToolStrip disposal channel. For example, after this.Controls.Add(combo); combo.Items.Add(this.stream);, the fixpoint below marks stream released, but ComboBox/ListBox item collections do not dispose arbitrary item objects; only ToolStripItemCollection has that ownership behavior. This can hide OWN001 for real owned disposables stored as UI items.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in e3404af. You're right that ComboBox/ListBox.Items (an ObjectCollection) doesn't dispose its items; only a ToolStripItemCollection does. CollectionContainerKey now gates an Items add on the collection's type being a ToolStripItemCollection (Controls/ControlCollection stays an unconditional channel). Pinned by a new ComboItemForm control: a real disposable stored as a ComboBox item stays flagged even though the combo itself is added to this.Controls.
Generated by Claude Code
| // explicit `components.Add(this.G)`. | ||
| foreach (var inv in cls.DescendantNodes().OfType<InvocationExpressionSyntax>()) | ||
| if (inv.Expression is MemberAccessExpressionSyntax { Name.Identifier.Text: "Add" } addMa | ||
| && inv.ArgumentList.Arguments.Count == 1 |
There was a problem hiding this comment.
IContainer also registers components through the named overload Add(IComponent component, string name), but this branch only credits calls with exactly one argument. A designer/custom registration such as components.Add(this.icon, "icon") is therefore ignored, so icon remains reported as OWN001 even though the same components.Dispose() sink will dispose it. Consider using the first argument for both overloads when the receiver is a container sink.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in e3404af. The (b) branch now accepts Add with >= 1 argument and credits Arguments[0] (the registered IComponent), so the named overload components.Add(this.icon, "name") is recognized just like the single-arg form. Pinned by a new NamedRegistrationForm control (a named-registered component is silent).
Generated by Claude Code
…ner.Add (#219) Two Codex P2 catches on the #219 disposal-channel fix: 1. Items must be a ToolStripItemCollection. The (a) channel accepted ANY member named `Items`, but only a ToolStripItemCollection disposes its items — a ComboBox/ListBox `Items` (ObjectCollection) does not. `this.Controls.Add(combo); combo.Items.Add(this.stream);` would have wrongly credited a real owned disposable stored as a combo item. CollectionContainerKey now gates an `Items` add on the collection's type being a ToolStripItemCollection; `Controls` (ControlCollection) is always a channel. 2. Named IContainer.Add overload. The (b) channel only credited `components.Add(x)` (1 arg), missing the `Add(IComponent, string name)` overload (`components.Add(x, "name")`) — the first argument is the registered component in both, so credit it whenever the receiver is a container sink. Pinned by two new controls in WinFormsDisposalSample.cs: `ComboItemForm` (a disposable stored as a ComboBox item STAYS flagged) and `NamedRegistrationForm` (a named-registered component is SILENT). ImplementsIContainer now matches `IContainer` by simple name so the sample's stand-in and the real System.ComponentModel.IContainer both resolve. Verified locally: sample now 5 findings (comboItem added to the controls), the eight channel-disposed fields silent; full existing-sample diff still byte-identical in both modes (zero regression); gates green. Refs #219 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cg6DVXahkyY68Ruu7f76rG
| public sealed class ComboItemForm : Control | ||
| { | ||
| private ComboBox combo; | ||
| private NotifyIcon comboItem; // a real disposable stored as a combo item |
|
@coderabbitai review |
…sal-channels-219 # Conflicts: # .github/workflows/ci.yml
3 participants
Что и зачем
WinForms-поля ложно флагались как OWN001-утечки, потому что field-disposal-скан не моделировал два designer-канала диспоза:
Control/ToolStripItem-поле, добавленное в своюControls/Items-коллекцию, диспозится рекурсивно черезControl.Dispose(bool)(контракт фреймворка);new T(components)или зарегистрированныйcomponents.Add(x), диспозится через designer-овскийcomponents.Dispose().~30 own-only FP на одном ShareX; sweep #201, field-notes 12–13.
Фикс — только в экстракторе, без изменений OwnIR: оба канала засчитывают освобождение поля только когда класс достигает disposal-корня — designer-овский
Dispose(bool)сbase.Dispose(disposing)(ClassReachesDisposalRoot). Класс без такого Dispose (ShareXHistoryItemManager, настоящий баг) — не rooted, его контролы остаются флагнутыми.Тип изменения
Как проверено
python tests/run_tests.py— exit 0 (test_ownir276/276)ruff check .иmypy --strictнаownlang/— clean / SuccessНе перерасширяет (обязательные негативные контроли — флагнуты)
param.Controls(контейнер — параметр, резолвится семантически) → лик.Disposeвообще → его контейнер и дети остаются флагнуты (шейпHistoryItemManager).new NotifyIcon()безcomponents→ лик.Детали
<coll>.Add/AddRange(this.child)с ключом-контейнером (""= this / имя поля / null = чужой), затем fixpoint: ребёнок rooted-контейнера сам становится rooted (ToolStrip вthis.Controlsкаскадит на свои Items).AddRangeпо массиву-инициализатору разворачивается.System.ComponentModel.IContainer; поле, построенное с ним или переданное в.Add(...), засчитывается.Связанные issue
Closes #219. Найден sweep'ом #201, каталог —
field-notes-patterns.mdзаписи 12–13 (помечены shipped). Отдельная зона экстрактора от #218/#225 — можно ревьюить параллельно.План валидации на живом репозитории (после мержа — отдельной задачей)
Перегнать oracle-sweep по ShareX и показать дельту:
own-only:Controls.Add-шейп (~24, напр.ImageViewer.cs:396-399,ColorPicker.cs:84-85,InputBox.cs:145-147) +IContainer-регистрация (~6, напр.TrayForm.cs:41TrayIcon).HistoryItemManager_ContextMenu(класс безDispose— 48 находок) иShapeManagerMenu(ShapeManager.Dispose()диспозитhistory, но неmenuForm— flagship). Сама перегонка — вне этого PR.Чеклист
field-notes-patterns.mdзаписи 12–13 → shipped)fix:)🤖 Generated with Claude Code
Generated by Claude Code