fix(extractor): WPF view-owns-VM exemption via sibling XAML (mined VideoSource FPs)#82
Conversation
…r-scope fix Two subscription-detector precision fixes from the ScreenToGif mining run. 1. View-owns-its-view-model (the 4 VideoSource OWN001 FPs). A WPF view that CONSTRUCTS its view-model in its own XAML (`<X.DataContext><VM/></X.DataContext>`) OWNS it — the view<->VM cycle is GC-collectable, so subscribing to the VM's events is not a leak. The extractor parses only `.cs`, where this is invisible (`_vm = DataContext as VM`), so it now reads the sibling `.xaml`: when the view's own XAML inline-constructs its DataContext (a type instantiation, not a Binding/resource reference), a field assigned from `this.DataContext` is folded into the self-owned set, exactly like a `new`'d field. Sound — XAML construction is provable ownership, with no aliasing hole. Mined from ScreenToGif's VideoSource. 2. Scope the App OWN014 exemption to non-timers (CodeRabbit, follow-up to #81). A timer is forced to source "static", so the merged `source == "static" && clsIsApp` guard also swallowed timer findings in `App`; a never-stopped timer there is still a real leak. Gated on `!isTimer`. Regression guards (wpf-extractor): ViewOwnsVmSample.xaml(.cs) — the view owns its XAML-declared VM, must stay silent; InjectedDcViewSample.xaml(.cs) — a BOUND (`<Binding/>`) DataContext is not owned, so its subscription must still warn, proving the gate keys off proven construction, not every `DataContext as T`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds XAML-sibling-parsing to the Roslyn extractor so it can detect when a WPF view constructs its own DataContext inline in XAML. Views that match are added to a ChangesWPF MVVM DataContext Ownership Detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| public void OnLoaded() | ||
| { | ||
| _vm.Changed += OnChanged; // unowned source -> possible leak -> warns |
![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: cb2bcc3012
ℹ️ 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".
| // unrecognised shape yields false (no exemption), never a wrongly-suppressed leak. | ||
| static bool XamlDeclaresOwnedDataContext(string xaml) | ||
| { | ||
| foreach (Match m in Regex.Matches(xaml, @"\.DataContext\s*>\s*<\s*(?:[\w]+:)?([\w.]+)")) |
There was a problem hiding this comment.
This match treats any property-element *.DataContext in the sibling XAML as proof that the code-behind view owns this.DataContext. In a common WPF layout where the root view has an injected/shared DataContext but a nested control declares its own <Grid.DataContext><local:ChildVm/></Grid.DataContext>, this returns true for the whole .xaml.cs file; the later ReadsDataContext path then adds fields assigned from this.DataContext to selfOwned, so subscriptions to the external VM are silently dropped instead of producing OWN001. The check needs to verify the constructed DataContext belongs to the XAML root/class being reviewed, not just any child element in the file.
Useful? React with 👍 / 👎.
…nt (Codex P2) XamlDeclaresOwnedDataContext matched ANY `*.DataContext` property-element in the sibling XAML. But the code-behind's `this.DataContext` is the ROOT element's, so a nested `<Grid.DataContext><ChildVm/>` (a different element's DataContext) would wrongly mark the whole `.xaml.cs` as owning — silently dropping a real OWN001 on the root's injected/bound VM. Now: strip comments / processing-instructions, find the root element tag, and match only `<Root.DataContext>` with a constructed (type-instantiation) child. The InjectedDcView control XAML gains exactly this shape — a BOUND root DataContext plus a nested `<Grid.DataContext><local:NestedChildVm/>` — so the existing "must warn" assertion now also guards the root-restriction (without it the nested construction would make the view look owned and the subscription would be wrongly suppressed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
307-308: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winBind the injected-wording assertion to
InjectedDcViewSampleexplicitly.Current checks assert warning presence for
InjectedDcViewSample, but injected-wording is validated globally and could be satisfied by another finding. Add a file-scoped wording assertion to prevent this scenario from silently regressing.Suggested CI assertion
echo "$out" | grep -qE "InjectedDcViewSample\.xaml\.cs:[0-9]+: warning: \[OWN001\]" \ || { echo "FAIL: a bound (unowned) DataContext subscription must still warn"; exit 1; } + echo "$out" | grep -qE "InjectedDcViewSample\.xaml\.cs:[0-9]+: warning: \[OWN001\].*injected dependency whose lifetime is unknown" \ + || { echo "FAIL: expected injected-source wording for InjectedDcViewSample"; exit 1; }🤖 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 @.github/workflows/ci.yml around lines 307 - 308, The current grep assertion for InjectedDcViewSample.xaml.cs with OWN001 warning is too broad and could pass if the warning appears in any file. Add an additional explicit file-scoped wording assertion that strictly validates the injected-wording is present in the InjectedDcViewSample output specifically, ensuring the warning is tied directly to that file and preventing silent regression if the warning moves to another source.
🤖 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 `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 253-263: The XamlDeclaresOwnedDataContext method uses an overly
broad regex pattern that matches any .DataContext property-element in the XAML
file, incorrectly classifying nested template DataContexts or non-instantiation
objects like x:Static as owned DataContexts. Narrow the detection to only the
view's actual root DataContext object element by refining the regex pattern or
preferably by using structural XAML parsing instead of a broad pattern-matching
approach. This will prevent the selfOwned path from incorrectly suppressing
OWN001 violations for these edge cases.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 307-308: The current grep assertion for
InjectedDcViewSample.xaml.cs with OWN001 warning is too broad and could pass if
the warning appears in any file. Add an additional explicit file-scoped wording
assertion that strictly validates the injected-wording is present in the
InjectedDcViewSample output specifically, ensuring the warning is tied directly
to that file and preventing silent regression if the warning moves to another
source.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f2100df-4af0-4b20-98f3-2f0dd0b1e10e
📒 Files selected for processing (6)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/InjectedDcViewSample.xamlfrontend/roslyn/samples/InjectedDcViewSample.xaml.csfrontend/roslyn/samples/ViewOwnsVmSample.xamlfrontend/roslyn/samples/ViewOwnsVmSample.xaml.cs
…p (CodeRabbit) Replace the textual regex in XamlDeclaresOwnedDataContext with structural XML parsing (XDocument), per CodeRabbit's recommendation. Strictly more correct: it inspects only the ROOT element's own `<Root.DataContext>` (a direct child, in the root's namespace) and excludes the entire `x:` language namespace — so x:Static / x:Null / x:Reference (external or shared values the view does NOT own) no longer count as construction, closing a residual false-negative the regex left open. A malformed `.xaml` parses to false (conservative). Drops the now-unused Regex import. Also tighten the InjectedDcView CI assertion to require the OWN001 warning AND the injected-source wording on that file's line specifically (CodeRabbit nitpick), so the negative control cannot be satisfied by an unrelated finding. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
3 participants
Mined from
NickeManarin/ScreenToGif(continued)Closes the last false positives from the ScreenToGif (WPF) mining run, plus a small correctness fix to the App exemption that landed in #81.
1. View-owns-its-view-model — the 4
VideoSourceOWN001 FPsVideoSource.xaml.cssubscribes to four events on its own view-model (_viewModel.ShowErrorRequested += …, etc.), and the detector warned "injected dependency … may keep VideoSource alive". But the view constructs its VM in its own XAML:So the view owns the VM — the view↔VM cycle is GC-collectable and the subscriptions aren't leaks.
The extractor parses only
.cs, where this ownership is invisible (_viewModel = DataContext as VideoSourceViewModel). So it now reads the sibling.xaml: when aFoo.xaml.cs'sFoo.xamlinline-constructs its DataContext (<X.DataContext><VM/>— a type instantiation, not aBinding/resource reference), a field assigned fromthis.DataContextis folded into the self-owned set, exactly like anew'd field. This is sound — XAML construction is provable ownership, with none of the aliasing hole that sank the ProcessHelper attempt: a<Binding/>/<StaticResource/>DataContext (externally supplied) is explicitly not treated as owned.2. Scope the App OWN014 exemption to non-timers (CodeRabbit follow-up to #81)
#81's
source == "static" && clsIsAppguard also swallowed timer findings inApp, because a timer is forced tosource = "static"a few lines up — and a never-stopped timer inAppis still a real leak. Now gated on!isTimer.Regression guards (
wpf-extractor)ViewOwnsVmSample.xaml(.cs) — the view owns its XAML-declared VM → must stay silent.InjectedDcViewSample.xaml(.cs) — a bound (<Binding/>) DataContext is not owned → the subscription must still warn, proving the gate keys off proven construction, not everyDataContext as T.Mining scorecard (ScreenToGif, 8 actionable verdicts)
HttpClientleak) — left firing🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
Tests
ViewOwnsVmSampleandInjectedDcViewSampleinputs.ViewOwnsVmSampleis flagged as leaking, and requires an OWN001 warning for injected DataContext with unknown lifetime.Improvements
DataContextbased on sibling XAML heuristics.Appstatic-source suppression to apply only to non-timer events.