extractor: normalize new-delegate handlers in event release-matching#163
Conversation
Event release-matching keyed on raw handler text, so `+= new PropertyChangedEventHandler(H)` never matched a bare `-= H` (the dominant SectorTS teardown idiom), and IsHandler rejected delegate-creation outright — so a wrapped `-=` was not collected and the P-004 static-handler exemption was skipped for a wrapped static method. Correctly torn-down subscriptions were reported as OWN001 false positives (~253/330 STS subscription findings use explicit delegate-creation). Add NormalizeHandler (unwrap `new SomeHandler(X)` -> `X`) and apply it at the release-key sites (+= released check, -= collect + IsHandler gate) and inside IsStaticHandler; the emitted handler fact text is unchanged. Regression: corpus/wpf/subscription-explicit-delegate-release (before caught / after clean). Verified real STS eDeclarant.xaml.cs 4->0, BranchDescription.cs ->0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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 (11)
✅ Files skipped from review due to trivial changes (5)
📝 WalkthroughWalkthroughThis PR normalizes wrapped delegate-creation handlers in the Roslyn extractor and adds WPF corpus cases for subscription release and handler-use-after-dispose detection, including updated OWN001 and OWN002 expectations. ChangesExtractor normalization and WPF corpus cases
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: a4458be705
ℹ️ 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 (!isTimer && source == "static" && clsIsApp) | ||
| continue; | ||
| var released = unsub.Contains($"{a.Left}|{a.Right}") | ||
| var released = unsub.Contains($"{a.Left}|{NormalizeHandler(a.Right)}") |
There was a problem hiding this comment.
When the receiver is reassigned between the detach and attach (as in the new after.cs setter: _source.PropertyChanged -= ...; _source = value; _source.PropertyChanged += new ...), this class-wide string key treats both sides as _source.PropertyChanged|OnSourcePropertyChanged. The -= only removes the handler from the old source; the newly assigned injected source remains subscribed and can still retain SourceView if it is assigned once or not reset during teardown, but this change marks that subscription released and suppresses the leak.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — you're right about the soundness gap. Two clarifications on scope:
-
Pre-existing, not introduced here. own-check's release model is non-flow-sensitive ("any matching
-=in the class = released"); a bare-=/+=rebinding setter already behaved identically before this PR.NormalizeHandleronly makes the wrappednew …Handler(H)form consistent with the bare form — it doesn't change flow-sensitivity. -
The test no longer leans on it. I reworked
after.csto a single readonly source subscribed in the ctor and released inDispose(no receiver rebinding), so "after = clean" is unconditionally true and the regression asserts the delegate-normalization only. The rebind last-subscription gap is now documented innotes.mdas a separate, tracked follow-up — properly fixing it needs flow-sensitive release analysis (a receiver reassigned between the-=and the+=should not count as released), which is out of scope for this text-normalization change.
…x P2) Codex flagged that after.cs's setter-rebind teardown leaned on own-check's non-flow-sensitive "any -= in the class = released" model: the -= detaches only the old source, and the rebound source stays subscribed. Rework the case to a readonly single source subscribed in the ctor and released in Dispose (no rebinding), so "after = clean" is unconditionally true and the regression asserts the delegate-normalization only. Document the rebind soundness gap (pre-existing, orthogonal to this fix) in notes.md. before.cs -> OWN001 (caught) / after.cs -> clean, unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex @CodeRabbit review new changes |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr view 163 --repo PhysShell/Own.NET --json title,body,commits,files -q '.'
echo "-----DIFF-----"
gh pr diff 163 --repo PhysShell/Own.NETLength of output: 17615 🏁 Script executed: #!/bin/bash
cd /tmp && rm -rf ownnet && git clone --depth 50 https://github.com/PhysShell/Own.NET.git ownnet 2>&1 | tail -5
cd ownnet && git fetch origin pull/163/head:pr163 2>&1 | tail -5 && git checkout pr163 2>&1 | tail -5
echo "---- search for handler text usage ----"
rg -n 'IsHandler|a\.Right|a\.Left|NormalizeHandler|\.ToString\(\)' frontend/roslyn/OwnSharp.Extractor/Program.cs | sed -n '1,200p'Length of output: 5106 🏁 Script executed: #!/bin/bash
cd /tmp/ownnet
sed -n '3420,3530p' frontend/roslyn/OwnSharp.Extractor/Program.cs
echo "-----3740-3800-----"
sed -n '3740,3800p' frontend/roslyn/OwnSharp.Extractor/Program.csLength of output: 11854 🏁 Script executed: #!/bin/bash
cd /tmp/ownnet
rg -n 'static string\? FieldName' frontend/roslyn/OwnSharp.Extractor/Program.cs
sed -n '470,485p' frontend/roslyn/OwnSharp.Extractor/Program.csLength of output: 1161
Core fix (
Corpus regression
One related item worth a look (optional follow-up, not blocking) foreach (var a in assigns)
if (IsHandler(a.Right) && FieldName(a.Right) is { } hn)
{
var key = $"{a.Left}|{hn}";
if (a.IsKind(SyntaxKind.AddAssignmentExpression)) liveEventKeys.Add(key);
else if (a.IsKind(SyntaxKind.SubtractAssignmentExpression)) liveEventKeys.Remove(key);
}Since this also calls Otherwise, the fix, tests, and docs all look sound to me. ✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a0f2f1f70
ℹ️ 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".
| // to reject. Loops to peel a (rare) doubly-wrapped delegate. | ||
| static ExpressionSyntax NormalizeHandler(ExpressionSyntax e) | ||
| { | ||
| while (e is ObjectCreationExpressionSyntax { ArgumentList.Arguments: { Count: 1 } args }) |
There was a problem hiding this comment.
In C# 9+ projects, an event subscription can validly use target-typed delegate creation such as _source.PropertyChanged += new(OnSourcePropertyChanged); with a bare -= OnSourcePropertyChanged; Roslyn represents that RHS as ImplicitObjectCreationExpressionSyntax, so this loop leaves it unnormalized. The later IsHandler(...) and release-key comparison still compare new(...) with OnSourcePropertyChanged, preserving the same false OWN001/static-handler false positive this fix is intended to remove for valid modern syntax.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. NormalizeHandler now matches BaseObjectCreationExpressionSyntax — the shared base of ObjectCreationExpressionSyntax (explicit new T(H)) and ImplicitObjectCreationExpressionSyntax (target-typed new(H)) — so both normalize to the inner handler before keying. Added a regression twin corpus/wpf/subscription-target-typed-delegate-release (before → OWN001 / after → clean). 👍
NormalizeHandler unwrapped only ObjectCreationExpressionSyntax (explicit `new T(H)`); a C# 9 target-typed `new(H)` is ImplicitObjectCreationExpressionSyntax, so it stayed unnormalized and `new(H)` != bare `H` on the release key — the same release-match / static-handler false positive, for modern syntax. Match BaseObjectCreationExpressionSyntax, the shared base of both forms. Regression: corpus/wpf/subscription-target-typed-delegate-release (before caught / after clean). Explicit-delegate case unchanged; ownlang wpf 8/8. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex @CodeRabbit review new changes |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr view 163 --repo PhysShell/Own.NET --json commits,updatedAt -q '.commits[-5:] | .[] | {oid: .oid, messageHeadline: .messageHeadline, committedDate: .committedDate}'
echo "---- comments on PR ----"
gh api repos/PhysShell/Own.NET/issues/163/comments --jq '.[] | {user: .user.login, created_at: .created_at, body: (.body[0:80])}'
echo "---- latest commit sha vs reviewed ----"
gh pr view 163 --repo PhysShell/Own.NET --json headRefOid -q '.headRefOid'Length of output: 1798 🏁 Script executed: #!/bin/bash
cd /tmp && rm -rf ownnet && git clone --depth 100 https://github.com/PhysShell/Own.NET.git ownnet 2>&1 | tail -3
cd ownnet && git fetch origin pull/163/head:pr163 2>&1 | tail -3
git show 24c79f35f7935869fa781bd6f78dff7467d76d67 --stat
echo "-----DIFF-----"
git show 24c79f35f7935869fa781bd6f78dff7467d76d67Length of output: 10030
Fix (
Corpus regression
This is a clean, well-scoped extension of the same normalization fix to modern C# 9 syntax — no issues found. The earlier optional follow-up item I mentioned (the raw-text ✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24c79f35f7
ℹ️ 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".
| static ExpressionSyntax NormalizeHandler(ExpressionSyntax e) | ||
| { | ||
| while (e is BaseObjectCreationExpressionSyntax { ArgumentList.Arguments: { Count: 1 } args }) | ||
| e = args[0].Expression; | ||
| return e; |
There was a problem hiding this comment.
Because this helper is opt-in, the handler-use-after-dispose pass later in this file still checks IsHandler(a.Right)/FieldName(a.Right) on the raw RHS. For a live subscription written as source.Changed += new EventHandler(OnChanged), that pass never adds OnChanged to the subscribed-handler set, so a disposed field read from OnChanged is no longer reported as OWN002 even though the new normalization proves this spelling is the same handler. Please apply the normalized RHS to the remaining handler-name checks as well, not only release matching and the static-handler exemption.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done — closed the whole class. NormalizeHandler is now applied to the handler-use-after-dispose live-subscription keying too (not just release matching + the static-handler exemption), so += new EventHandler(OnX) and target-typed += new(OnX) register OnX as a live subscription target and its disposed-field read is flagged OWN002.
Added a regression case corpus/wpf/handler-use-after-dispose-wrapped-delegate — self-owned source, so OWN002 is isolated from OWN001. Verified pre-fix the OWN002 was missed; post-fix before.cs → OWN002 / after.cs → clean. 👍
…x P2) The handler-use-after-dispose pass keyed live subscription targets by IsHandler/ FieldName on the raw RHS, so a wrapped `+= new EventHandler(OnX)` never registered OnX as subscribed and a disposed-field read in OnX escaped OWN002 — the same NormalizeHandler gap as the release matching, in a second pass. Apply NormalizeHandler to the live-subscription keying so wrapped and target-typed spellings register consistently. Regression: corpus/wpf/handler-use-after-dispose-wrapped-delegate (before caught / after clean; self-owned source, so OWN002 is isolated from OWN001). Verified pre-fix that OWN002 was missed. ownlang wpf 9/9. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex @CodeRabbit review new changes |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr view 163 --repo PhysShell/Own.NET --json commits,headRefOid -q '{head: .headRefOid, last3: .commits[-3:] | map({oid, messageHeadline, committedDate})}'Length of output: 910 🏁 Script executed: #!/bin/bash
cd /tmp && rm -rf ownnet && git clone --depth 100 https://github.com/PhysShell/Own.NET.git ownnet 2>&1 | tail -3
cd ownnet && git fetch origin pull/163/head:pr163 2>&1 | tail -3
git show 02075b7810e96e9e0bd378ad255057799d7ed11f --stat
echo "-----DIFF-----"
git show 02075b7810e96e9e0bd378ad255057799d7ed11fLength of output: 11199
Fix (
Corpus regression (
This fully addresses the optional follow-up I raised earlier — no remaining concerns from me. Nice work tracking this down across both passes. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Что и зачем
Экстрактор сопоставлял
-=с+=по сырому тексту хендлера, поэтому+= new PropertyChangedEventHandler(H)не находил парный голый-= H(доминирующий teardown-идиом SectorTS), а
IsHandlerвовсе отвергалdelegate-creation — из-за чего обычный
-=не собирался, а P-004static-handler exemption пропускался для обычного статического метода.
Корректно отписанные подписки уходили в OWN001 как ложные срабатывания.
Фикс добавля
применяет его на всех точках сравнения release-ключа + в
IsStaticHandler.На полном прогоне STS: subscription-находки 323 → 156 (−52%), при неизменных
OWN050/OWN014/idisposable.
Тип изменения
Как проверено
Связанные issue
Нет связанного issue.
Чеклист
extractor: …)Summary by CodeRabbit