fix(extractor): recognize DP old→new subscription rotation as paired (Closes #218)#230
Conversation
…218) A property-changed callback that detaches the SAME handler from the OLD value and re-attaches it to the NEW one across a value change was false-flagged as an OWN001 leak: the extractor pairs `-=`/`+=` per source VARIABLE, so the `+=` on the new half (`newCommand`) looked unpaired even though the `-=` on the old half (`oldCommand`) a few lines up is the same rotation slot — at most one subscription is ever live. Confirmed FP in 3 real repos, 6+ call sites (MahApps CommandTriggerAction, MaterialDesign SmartHint, AvalonEdit margins); the single most-corroborated false-positive class from the issue #201 oracle sweep. Fix (extractor-only, no OwnIR change): a third `released` disjunct, IsRotationPairedRelease, stamps the `+=` on the new half as released when the same method holds a `-=` on the old half with the same event member and handler, and (oldRecv, newRecv) are the old/new halves of one change — either bound from a DependencyPropertyChangedEventArgs OldValue/NewValue (directly or via `is`-pattern / cast / assignment), or two same-type parameters of the enclosing method with old before new (the OnXChanged(old, new) override, matched by the param-pair shape, not the method name). The core then sees a balanced acquire/release and stays silent. No new fact, no OWNIR_VERSION bump — it reuses the existing `released` field. Precision-first, does not over-widen — three negative controls stay flagged: (a) a `+=` with a different handler than the `-=`; (b) a pair on two differently-typed params (not old/new halves); (c) a pair across two class fields. Sample DpRotationSample.cs pins both forms silent and all three controls flagged, wired into the wpf-extractor CI job. field-notes entry 11 marked shipped. Verified locally with the real extractor (.NET 8): the sample goes 5 findings -> 3 (2 rotations silenced, 3 controls flagged); a full-sample-set diff of old vs new extractor output is byte-identical (zero regression on existing findings). Gates: run_tests.py exit 0, test_ownir 271/271, ruff clean, mypy --strict green. Closes #218 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cg6DVXahkyY68Ruu7f76rG
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (e.OldValue is ICommand oldCommand) | ||
| oldCommand.CanExecuteChanged -= self.HandlerA; // unsub OLD with HandlerA | ||
| if (e.NewValue is ICommand newCommand) | ||
| newCommand.CanExecuteChanged += self.HandlerB; // sub NEW with HandlerB (different!) -> LEAK |
| private void Wire(ICommand primary, IOtherCommand secondary) | ||
| { | ||
| primary.CanExecuteChanged -= Handler; // unsub on primary (ICommand) | ||
| secondary.CanExecuteChanged += Handler; // sub on secondary (IOtherCommand — different type) -> LEAK |
| private void Swap() | ||
| { | ||
| _a.CanExecuteChanged -= Handler; // unsub on field _a | ||
| _b.CanExecuteChanged += Handler; // sub on field _b (not an old/new pair) -> LEAK |
|
Warning Review limit reached
Next review available in: 36 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 |
![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: 28b7ea0747
ℹ️ 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 (IsDpValueAccess(oldRecv, "OldValue") && IsDpValueAccess(newRecv, "NewValue")) | ||
| return true; |
There was a problem hiding this comment.
When the callback subscribes directly on a cast DP value, e.g. ((ICommand)e.NewValue).CanExecuteChanged += ... paired with ((ICommand)e.OldValue)... -= ..., the receiver passed here is a parenthesized/cast expression rather than the e.NewValue member access, so IsDpValueAccess returns false and the new rotation exemption is not applied. Since DependencyPropertyChangedEventArgs.OldValue/NewValue are object-typed, this direct-cast form is a common way to write the exact old→new rotation this change is meant to silence; use the existing StripCasts before this direct check so those balanced callbacks do not keep reporting OWN001.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in e942b95. You're right that this was a real gap: OldValue/NewValue are object-typed, so the inline form must cast, and the direct-check receiver was a parenthesized cast that IsDpValueAccess rejected. (My sample only covered the is-pattern form, which resolves via BoundFromDpValue, so the direct path was untested.)
StripCasts is now applied to both receivers before the direct check, and it also peels the null-forgiving !, so ((ICommand)e.NewValue!).CanExecuteChanged += H paired with the OldValue -= is recognised. Pinned by a new InlineCastRotation fixture (silent) wired into the CI silent-rotation assertion; the three negative controls still stay flagged and a full existing-sample diff is byte-identical (zero regression).
Generated by Claude Code
…eck (#218) Codex review catch. The direct old/new form matched a bare `e.OldValue`/`e.NewValue` member-access receiver, but DependencyPropertyChangedEventArgs.OldValue/NewValue are object-typed, so the inline form MUST cast: `((ICommand)e.NewValue!).CanExecuteChanged += H`. There the `+=` receiver is a parenthesized cast, so IsDpValueAccess returned false and the rotation stayed wrongly flagged. StripCasts (now also peeling the null-forgiving `!`) is applied before the direct check, so the inline-cast rotation — a common way to write this idiom — is recognised. The `is`-pattern / local-binding forms already worked via BoundFromDpValue. Pinned by a new InlineCastRotation class in DpRotationSample.cs (silent), added to the CI silent-rotation assertion. Verified locally: the sample's three rotation forms (pattern-cast, inline-cast, virtual-override) are all silent; the three controls stay flagged; a full existing-sample diff is byte-identical (zero regression). Refs #218 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cg6DVXahkyY68Ruu7f76rG
|
@coderabbitai review |
Both sides appended to the same wpf-extractor job tail: main's #215 [OwnIgnore] block (sample, assertions, SARIF rc-capture step) and this branch's #218 rotation block (sample, silent + three flagged controls). Kept both, single merged OK line. Program.cs auto-merged (additive). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LsWw4Ay8KLTHFom1HvRu3U
Lines starting with "#201)" / "#230/#231" after a soft wrap read as malformed ATX headings to markdownlint. No content change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D6Naf8CcezjCbikueKdeYv
3 participants
Что и зачем
Property-changed callback, который отвязывает ТОТ ЖЕ handler от старого значения и привязывает к новому при смене значения, ложно флагался как OWN001-утечка: экстрактор пара́ит
-=/+=по переменной-источнику, поэтому+=на new-половине (newCommand) выглядел непарным, хотя-=на old-половине (oldCommand) строкой выше — тот же rotation-slot. Живой подписки максимум одна. Самый подтверждённый FP-класс sweep'а #201: 3 репозитория, 6+ точек (MahAppsCommandTriggerAction, MaterialDesignSmartHint, AvalonEdit margins).Фикс — только в экстракторе, без изменений OwnIR: третий дизъюнкт
released—IsRotationPairedRelease— помечает+=на new-половине как released, когда в ТОМ ЖЕ методе есть-=на old-половине с тем же event-member и handler, а(oldRecv, newRecv)— old/new-половины одного изменения. Ядро видит сбалансированный acquire/release и молчит. Новый факт не нужен,OWNIR_VERSIONне бампается (переиспользую существующее полеreleased).Тип изменения
Как проверено
python tests/run_tests.py— exit 0 (test_ownir271/271)ruff check .иmypy --strictнаownlang/— clean / SuccessДве распознаваемые формы
oldRecv/newRecvпривязаны изDependencyPropertyChangedEventArgse.OldValue/e.NewValue: напрямую, или черезe.OldValue is T old/(T)e.OldValue/var old = e.OldValue.OnXChanged(T old, T new)— два параметра одного типа, old перед new (матч по паре параметров, НЕ по имени метода).Не перерасширяет (обязательные негативные контроли — остаются флагнутыми)
+=с хендлером, ОТЛИЧНЫМ от-=→ реальная непарная подписка.-=на одном поле класса,+=на другом → две независимые подписки.Существующий paired-случай (
-=/+=на ОДНОМ источнике) не затронут — он идёт через прежний per-variableunsub-матч.Связанные issue
Closes #218. Найден sweep'ом #201 (
docs/notes/oracle-sweep-2026-07-10.md), каталогизирован какfield-notes-patterns.mdзапись 11 (помечена shipped).План валидации на живых репозиториях (после мержа — отдельной задачей)
Фикс запинен на синтетическом сэмпле; подтверждение на реальном коде — перегнать oracle-sweep по трём репо-первоисточникам и показать дельту FP:
Actions/CommandTriggerAction.cs:102-117(OnCommandChanged,CanExecuteChangedold→new) должен уйти изown-only.SmartHint.cs:189-209(4 находки:IsVisibleChanged/ContentChanged/Loaded/FocusedChanged) должны уйти.AbstractMargin.cs:92-101,LineNumberMargin.cs:105-118,Folding/FoldingMargin.cs(виртуальныйOnTextViewChanged(old,new)) должны уйти.own-only, при этом ни одна ранее-подтверждённая настоящая находка не исчезает (проверить, что общий leak-count по репо падает ровно на число rotation-точек). Сама перегонка — вне этого PR (нужен клон репозиториев в sweep-окружении).Чеклист
field-notes-patterns.mdзапись 11 → shipped)fix:)🤖 Generated with Claude Code
Generated by Claude Code