feat(own): disposable-local → using, and inline-lambda extraction#4
Conversation
Extends the OWN fixer (PR #2) to a second leak shape: an IDisposable field that is never disposed (Timer, CancellationTokenSource, ...). On a WPF owner it disposes the field on the teardown event, reusing the same Window->Closed / FrameworkElement-> Unloaded selection as the subscription detach: public ShareWindow() { InitializeComponent(); + this.Closed += (s, e) => _timer?.Dispose(); The dispose hook is anchored after the ctor's InitializeComponent() (in scope for 'this'). Honesty boundary kept: disposable-LOCAL stays suggest-only (needs a scoped using), as does the inline-lambda subscription. classify() now also returns the field shape; plan_file dispatches per shape to the right anchor + teardown statement. Fixture: ShareWindow._timer (real STS site). +2 tests; 13/13 own + 7/7 wrapper, incl -O. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoa
Two new OWN001 shapes for the T4 fixer, both conservative (T4: refuse rather
than emit a wrong patch). Generalized plan_file to an edit engine
((start,end,repl) applied bottom-up) so multi-line rewrites compose.
- disposable LOCAL: wrap a clean 'T x = new T(...);' in a block 'using (...) {}',
but only when the local doesn't escape its block (return/out/ref/store -> refuse
'local-escapes', no use-after-dispose).
- inline-LAMBDA subscription: extract the lambda to a named handler, rewrite '+='
to the method group, add the teardown detach. Only well-known event delegates
(PropertyChanged/ListChanged/CollectionChanged) with a 2-param expression lambda;
block bodies / unknown delegates / multi-line subs -> refuse.
Scope analysis is char-level brace matching that ignores strings and // comments.
Fixtures: Helper (local), DatabaseOptimizationWindow (lambda extract + a block-body
refusal variant). +4 tests incl. escaping-local and block-lambda refusals.
16/16 own + 7/7 wrapper, normal and -O.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoa
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR expands the OWN001 fixer to classify named-handler, disposable-field, disposable-local, and inline-lambda findings, adds planning and application logic for those shapes, and updates docs, fixtures, and tests to match the new supported and skipped cases. ChangesOWN001 fixer expansion
Sequence Diagram(s)sequenceDiagram
participant plan_file
participant classify
participant _plan_one
participant _plan_teardown
participant _plan_local
participant _plan_lambda
plan_file ->> classify: inspect finding message
classify -->> plan_file: shape category
plan_file ->> _plan_one: dispatch finding
_plan_one ->> _plan_teardown: named-handler / disposable-field
_plan_one ->> _plan_local: disposable-local
_plan_one ->> _plan_lambda: inline-lambda extract+detach
_plan_teardown -->> plan_file: edits or skip detail
_plan_local -->> plan_file: edits or skip detail
_plan_lambda -->> plan_file: edits or skip detail
plan_file ->> plan_file: apply edits in reverse index order
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.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: 67574ae272
ℹ️ 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 (re.search(rf"\breturn\b[^;]*\b{nm}\b", region) # returned | ||
| or re.search(rf"\b(out|ref)\s+{nm}\b", region) # passed out/ref | ||
| or re.search(rf"=\s*{nm}\s*[;,)]", region) # stored elsewhere | ||
| or "yield" in region): |
There was a problem hiding this comment.
When a disposable local is handed to a retaining API such as items.Add(p) or Register(p), this escape check still treats it as safe because it only rejects return, out/ref, direct assignments, and yield. The fixer will then wrap the rest of the block in using, disposing p at block exit even though another object may keep using it, which turns a leak finding into a use-after-dispose bug without a compiler error. For the advertised "only when the local doesn't escape" behavior, pass-as-argument/store-through-call cases need to be refused or proven safe.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex Fixed in 0088e83. _plan_local now also refuses when the local is passed as a call argument ([(,]\s*name\s*[),], e.g. items.Add(p) / Register(p)) — and additionally when the block contains any => (a closure that could capture and outlive it). Both route to local-escapes (suggest-only), so a leak is never turned into a use-after-dispose. Tests: test_local_passed_to_call_is_not_wrapped, test_local_captured_by_lambda_is_not_wrapped.
Generated by Claude Code
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
| touched = set() | ||
| for s, e, _ in edits: | ||
| touched.update(range(s, max(e, s + 1))) | ||
| if touched & occupied: | ||
| skipped.append((f, "overlapping-edit")) |
There was a problem hiding this comment.
For classes with more than one disposable field, every field disposal is anchored after the same InitializeComponent() line, but this overlap check marks pure insertions at the same index as conflicting. As a result only the first field gets a Closed/Unloaded disposal hook and the remaining field findings are skipped as overlapping-edit; the wrapper can still report ok if at least one selected finding disappeared, leaving the other leaks unfixed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex Fixed in 0088e83. The overlap check now distinguishes replacements from insertions: replacements still conflict on any shared line, but multiple insertions may share an anchor (e.g. several disposable fields after one InitializeComponent()). So every field in a class now gets its own disposal hook instead of all-but-first being skipped as overlapping-edit. Test: test_multiple_disposable_fields_all_disposed (2 fields → 2 hooks, no skips).
Generated by Claude Code
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
fix/tests/test_own_fix.py (1)
179-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a non-
returnescape regression for disposable locals.This new coverage only exercises
return p;, but the planner also rewrites locals captured by lambdas or passed as ordinary arguments today. Please add one of those cases here so the conservative OWN001 contract is enforced by tests as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fix/tests/test_own_fix.py` around lines 179 - 195, Add a regression test in test_escaping_local_is_not_wrapped() that covers a disposable local escaping without a return, such as being captured by a lambda or passed as a normal argument, and verify plan_file() still leaves the source unchanged and marks the OWN001 finding as skipped with local-escapes. Use the existing helpers and symbols already in the test (test_escaping_local_is_not_wrapped, _tmp_cs, Finding, plan_file) so the conservative escape behavior is enforced for non-return cases too.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fix/fixarm/own_fix.py`:
- Around line 147-149: The ctor-anchor lookup in _find_ctor_anchor is scanning
past the current class and can pick up an InitializeComponent() from a later
class, so bound the search to the enclosing class scope. Update the loop that
starts from cls_idx to stop at that class’s closing brace (using the class block
boundary already available in the parser) and only return an anchor found within
that class.
- Around line 249-255: The escape detection in the local-escapes check is
incomplete, so it still allows values that can outlive the block to be treated
as safe. Update the logic in the function that scans the `region` for escape
patterns to also reject cases where the local is passed into a call or captured
by a closure/lambda/event handler, not just `return`, `out/ref`, assignment, or
`yield`. Keep the existing `local-escapes` failure path in `own_fix.py` and
broaden the heuristics so OWN001 refuses to rewrite whenever the symbol can
escape through a callee or callback.
---
Nitpick comments:
In `@fix/tests/test_own_fix.py`:
- Around line 179-195: Add a regression test in
test_escaping_local_is_not_wrapped() that covers a disposable local escaping
without a return, such as being captured by a lambda or passed as a normal
argument, and verify plan_file() still leaves the source unchanged and marks the
OWN001 finding as skipped with local-escapes. Use the existing helpers and
symbols already in the test (test_escaping_local_is_not_wrapped, _tmp_cs,
Finding, plan_file) so the conservative escape behavior is enforced for
non-return cases too.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: adb97fd3-7f06-4f09-93e7-133fad26f605
📒 Files selected for processing (15)
docs/fix-arm.mdfix/README.mdfix/fixarm/own_fix.pyfix/fixtures/own001-disposable-field/after.findings.jsonfix/fixtures/own001-disposable-field/before.findings.jsonfix/fixtures/own001-disposable-field/before/Broker/ShareWindow.xaml.csfix/fixtures/own001-disposable-local/after.findings.jsonfix/fixtures/own001-disposable-local/before.findings.jsonfix/fixtures/own001-disposable-local/before/Broker/Helper.csfix/fixtures/own001-lambda-extract/after.findings.jsonfix/fixtures/own001-lambda-extract/before.findings.jsonfix/fixtures/own001-lambda-extract/before/Broker/DatabaseOptimizationWindow.xaml.csfix/fixtures/own001-lambda/before.findings.jsonfix/fixtures/own001-lambda/before/Broker/DatabaseOptimizationWindow.xaml.csfix/tests/test_own_fix.py
Address Codex (2) + CodeRabbit (3) review on PR #4: - local-escapes (Codex P1 + CodeRabbit): also refuse when the local is passed as a call argument ('Register(p)') or when a closure is present in the block ('=>' -> may capture and outlive it). Prevents turning a leak into a use-after-dispose. (+2 tests: arg-pass, lambda-capture) - _find_ctor_anchor (CodeRabbit): bound the InitializeComponent() search to the field's enclosing class via _class_close, so a later class can't anchor the hook in the wrong place. (+test: no-ctor-anchor when class lacks it) - overlap check (Codex P2): allow multiple INSERTIONS at one anchor (only replacements conflict), so every disposable field in a class gets its hook instead of all-but-first skipped as overlapping-edit. (+test: 2 fields) 20/20 own + 7/7 wrapper, normal and -O. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LLHz4cevQyXHKZnjzCqhoa
2 participants
OWN fixer: disposable-local →
using, and inline-lambda extractionAdds the two hardest OWN001 shapes to the T4 fixer — the ones deliberately left suggest-only in PR #2 because they need structural understanding. Both are conservative by design (T4: refuse rather than emit a wrong patch).
plan_filewas generalized to an edit engine ((start, end, repl)applied bottom-up) so multi-line rewrites compose; brace/scope analysis is char-level and ignores strings +//comments.3. disposable local → block
usingFires only when the local doesn't escape its block —
return/out/ref/store → refuselocal-escapes(otherwise we'd dispose before last use).4. inline-lambda subscription → extract + detach
Only for well-known event delegates (PropertyChanged / ListChanged / CollectionChanged) with a 2-param expression lambda; block bodies, unknown delegates, multi-line subs → refuse.
Honesty boundary
Every refusal is surfaced in
applier.skipped, never a fake patch:local-escapes,lambda-shape-unsupported,unknown-event-delegate,unbraced-control-flow,no-safe-teardown.The OWN fixer now covers four shapes (subscription, field, local, lambda). Fixtures are real STS sites (
Helper,DatabaseOptimizationWindow, plus a block-body refusal variant).Test plan
🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
using, and inline event lambdas extracted into named handlers with teardown detachment.