audit(xaml): add XAML100 ResourceShouldBeHoisted (build-free)#124
Conversation
A markup-only Phase-1 rule that finds what own.net structurally cannot — pure working-set waste that lives only in XAML, no C# representation, so no overlap with own-check. XAML100: a heavy Freezable resource (brush/geometry/transform/image) declared with x:Key in >=2 control-local <X.Resources> scopes with the same structure. Each copy is a separate object multiplying working set across siblings; one shared resource at window/app scope collapses them (the design note's "52x52 Brush" case). Restricted to Freezables, where the shallow structural signature (reused from XAML113) is reliable; Style/template deep-equivalence is a later slice. Resources already at a shared root/window/app scope are the hoist target, not a finding. - xaml_check.py: _scope_owner + _rule_resource_hoist; registered in RULES; docstring updated (XAML100 implemented for Freezables). - taxonomy: XAML100 -> category 9 (resource-hoist). - selftest grows to 37: duplicate-across-local-scopes flags; single copy / shared scope / structurally-distinct brushes do not. - README + design note catalogue updated. Distinct from XAML113 (inline non-keyed dup) and XAML101 (converters): XAML100 is keyed Freezables recurring across control-local dictionaries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zoG4YNCUf7RA5r8GAXMmb
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughThe PR adds the ChangesXAML100 Resource Hoist
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: f6a3f7b3a0
ℹ️ 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".
| for _key, c in _keyed_resources(rd): | ||
| if c.type_name() not in FREEZABLE_TYPES: | ||
| continue | ||
| sig = _inline_freezable_sig(c) # type + non-key attrs + child types |
There was a problem hiding this comment.
When XAML100 analyzes keyed Freezables whose distinguishing data is in child elements, such as two LinearGradientBrush resources with different GradientStop Color/Offset values, this line buckets them through _inline_freezable_sig, which only records the child element types. Those visually different resources are therefore reported as "declared identically", and following the hoist recommendation would collapse distinct brushes into one shared resource.
Useful? React with 👍 / 👎.
…p signature) Follows #124 (XAML100 for Freezables). Styles/templates couldn't use the shallow signature — two unrelated Styles both have `Setter` children, so a shallow type-only sig would collapse them and mis-flag distinct styles as duplicates. - _deep_resource_sig: a recursive full-subtree signature (tag + non-x:Key attrs + ordered child signatures), so a Style/ControlTemplate/DataTemplate only collapses with a structurally identical twin (same setters/values/template), never a merely same-shaped one. Child order is significant (conservative: only literal duplicates flag). - HOISTABLE_RESOURCE_TYPES = Freezables + Style/ControlTemplate/DataTemplate/ HierarchicalDataTemplate/ItemsPanelTemplate; _rule_resource_hoist now uses the deep sig for all of them (Freezable behavior unchanged — a leaf brush's deep sig equals its old shallow sig). - selftest grows to 39: an identical Style duplicated across control-local scopes flags; two Styles with different Setter values do NOT (proves deep equivalence). - docstring + design-note catalogue updated (Style/template no longer deferred). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zoG4YNCUf7RA5r8GAXMmb
2 participants
A markup-only Phase-1 rule that finds what own.net structurally cannot — pure working-set waste that lives only in XAML, with no C# representation, so it never overlaps with own-check (unlike the Phase-2 join, which re-frames a C# leak).
XAML100 ResourceShouldBeHoisted
A heavy Freezable resource (brush / geometry / transform / image) declared with
x:Keyin ≥2 control-local<X.Resources>scopes with the same structure. Each copy is a separate object multiplying working set across siblings; one shared resource at window/app scope collapses them — the design note's "52x52 Brush" case._inline_freezable_sig) is reliable. Style/template deep-equivalence is a documented later slice._scope_owner∈ TOP_LEVEL_SCOPES → skipped).Example
Why this one (not the join's timer/disposable extension)
The Phase-2 join's value is re-framing leaks own-check already finds — extending it to timer/disposable would double-report what own.net already reports. XAML100 is the opposite: a markup-only perf problem with no C# representation at all, so it's net-new detection with no redundancy. That made it the highest value-per-effort next step.
Changes
xaml_check.py—_scope_owner+_rule_resource_hoist, registered inRULES; docstring updated (XAML100 implemented for Freezables, Style/template deferred).XAML100→ category 9 (resource-hoist).Validation
Full audit selftest suite green (182 checks: normalize/score/report/xaml_check/xaml_facts/xaml_join/run_static/ingest),
ruff check .clean. End-to-end confirmed on a 3-Border fixture (2 findings pointing at the first declaration).🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
Documentation