audit(xaml): XAML100 — extend resource-hoist to Styles/templates (deep signature)#128
Merged
Merged
Conversation
…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
|
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 (1)
📝 WalkthroughWalkthroughXAML100 now treats structurally identical heavy keyed resources, including styles and templates, as hoist candidates. The analyzer text, matching logic, parsing support, and selftests were updated to match that broader scope. ChangesXAML100 resource hoisting expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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: 371116b823
ℹ️ 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".
![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
🤖 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 `@audit/static/tools/xaml_check.py`:
- Around line 407-418: The deep resource signature currently ignores node text,
so structurally identical elements with different character data can still
collide. Update the parser/tree model used by _deep_resource_sig so Node retains
relevant text content, then incorporate that text into the signature alongside
tag, attributes, and children. Make sure the change is applied consistently
where Node is built and where _deep_resource_sig walks the tree.
🪄 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: 75841c5a-7bac-4ba1-825a-3f388057812b
📒 Files selected for processing (2)
audit/static/tools/xaml_check.pydocs/notes/xaml-analyzer-design.md
…clusion (review) Two false-positive fixes from the Codex/CodeRabbit review on PR #128's deep template signature: - Capture character data. Node never stored text, so the deep signature treated <DataTemplate><TextBlock>OK</> and ...Cancel... as identical and XAML100 would flag text-different templates as duplicates. Added Node.text + an expat CharacterDataHandler (accumulates across split runs), and the trimmed text is now part of _deep_resource_sig. - Make the x:Key exclusion prefix-aware. The signature dropped every attribute whose local name is "Key", not just the resource directive x:Key — so a descendant CLR `KeyBinding Key="Enter"` vs "Escape" was erased, collapsing non-identical templates. _is_xkey now excludes only a prefixed `x:Key`; a bare CLR `Key` stays in the signature. Attribute names are kept raw (prefix included) for fidelity. Selftest grows to 42: templates differing only in text are not duplicates (identical text still collapses); a descendant KeyBinding Key keeps templates distinct. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zoG4YNCUf7RA5r8GAXMmb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows #124 (XAML100 for Freezables). Extends the resource-hoist rule to Styles and templates, the higher-value half of the "52x52 collapse" — a duplicated
Style/ControlTemplateis far heavier than a duplicated brush.Why a deep signature
Styles/templates can't use the shallow type+attrs signature: two unrelated
Styles both haveSetterchildren, so a shallow sig would collapse them and mis-flag distinct styles as duplicates. So this adds_deep_resource_sig— a recursive full-subtree signature (tag + non-x:Keyattrs + ordered child signatures). AStyle/ControlTemplate/DataTemplatecollapses only with a structurally identical twin (same setters / values / template tree), never a merely same-shaped one. Child order is significant — a conservative choice so only literal duplicates flag.Freezable behavior is unchanged: a leaf brush's deep sig equals its old shallow sig.
Changes
xaml_check.py—_deep_resource_sig;HOISTABLE_RESOURCE_TYPES= Freezables +Style/ControlTemplate/DataTemplate/HierarchicalDataTemplate/ItemsPanelTemplate;_rule_resource_hoistuses the deep sig for all of them.Styleduplicated across control-local scopes flags; twoStyles with differentSettervalues do not (proves deep equivalence). End-to-end confirmed identicalControlTemplates also flag.Validation
Full audit selftest suite green (184 checks: normalize/score/report/xaml_check/xaml_facts/xaml_join/run_static/ingest),
ruff check .clean.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Style/template resources when their full subtrees are structurally equivalent.Keyvalues don’t trigger hoisting mismatches.Documentation
Tests
Keyhandling.