From 371116b823a35670ab4e819ed82d3c75b899f278 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 26 Jun 2026 20:40:24 +0000 Subject: [PATCH 1/2] =?UTF-8?q?audit(xaml):=20XAML100=20=E2=80=94=20extend?= =?UTF-8?q?=20resource-hoist=20to=20Styles/templates=20(deep=20signature)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_015zoG4YNCUf7RA5r8GAXMmb --- audit/static/tools/xaml_check.py | 74 +++++++++++++++++++++++------- docs/notes/xaml-analyzer-design.md | 2 +- 2 files changed, 58 insertions(+), 18 deletions(-) diff --git a/audit/static/tools/xaml_check.py b/audit/static/tools/xaml_check.py index 2db11ee2..eedaa77f 100644 --- a/audit/static/tools/xaml_check.py +++ b/audit/static/tools/xaml_check.py @@ -27,7 +27,7 @@ perf/lifetime axis that WpfAnalyzers / PropertyChangedAnalyzers do **not** cover — we deliberately do not re-implement their correctness rules: - XAML100 ResourceShouldBeHoisted (cat 9) Freezable keyed in N local scopes + XAML100 ResourceShouldBeHoisted (cat 9) Freezable/Style/template dup'd per-scope XAML101 DuplicateStatelessConverterResource (cat 9) per-instance resource churn XAML102 DynamicResourceLikelyStatic (cat 9) WPF-only, deferred-lookup cost XAML103 SuspiciousSharedFalse (cat 9) WPF-only, x:Shared opt-out @@ -41,12 +41,12 @@ XAML112 TemplateBindingOpportunity (cat 9) cheaper compiled binding available XAML113 InlineFreezableDuplication (cat 9) identical inline brush/geometry -XAML100 covers the recurring-Freezable case (the same keyed brush/geometry/transform -declared in several control-local scopes); the Style/template variant (deep -structural equivalence) and XAML105 MergedDictionaryKeyShadowing across *external* -dictionaries (cross-file resolution) remain a later slice — documented so nothing on -the wishlist quietly falls through. Phase 2 (Roslyn-linked XAML2xx) and Phase 3 -(runtime correlation) live elsewhere per the design note. +XAML100 covers the recurring heavy-resource case across control-local scopes — both +Freezables and Styles/templates (the latter on a full-subtree signature). XAML105 +MergedDictionaryKeyShadowing across *external* dictionaries (cross-file resolution) +remains a later slice — documented so nothing on the wishlist quietly falls through. +Phase 2 (Roslyn-linked XAML2xx) and Phase 3 (runtime correlation) live elsewhere per +the design note. WPF-only rules (XAML102/103/106) are skipped on Avalonia ``.axaml`` because the ``DynamicResource`` / ``x:Shared`` / ``Freezable`` semantics differ or do not exist @@ -95,6 +95,14 @@ # the document root and the window/app/page-level dictionaries (XAML100). TOP_LEVEL_SCOPES = {"root", "Window", "UserControl", "Application", "Page", "NavigationWindow", "ResourceDictionary"} +# Heavy keyed resources worth hoisting when duplicated across control-local scopes +# (XAML100): Freezables (a shallow sig would do) plus Styles/templates (which need the +# DEEP structural signature, since their direct-child types alone — all `Setter` — say +# nothing about the values). +HOISTABLE_RESOURCE_TYPES = FREEZABLE_TYPES | { + "Style", "ControlTemplate", "DataTemplate", "HierarchicalDataTemplate", + "ItemsPanelTemplate", +} # Binding markers for XAML108 (per-keystroke source updates). _TWOWAY_RE = re.compile(r"Mode\s*=\s*TwoWay", re.IGNORECASE) @@ -396,24 +404,38 @@ def _scope_owner(rd: Node) -> str: return "root" +def _deep_resource_sig(node: Node) -> tuple[Any, ...]: + """A *recursive* structural signature: the element's tag (sans namespace prefix, + keeping the property-element dotted form), its non-``x:Key`` attributes, and the + signatures of all its children in document order. Two resources share this only + when their entire subtree is structurally identical — so a `Style`/`ControlTemplate` + is matched on its actual setters/triggers/template, not just "has Setter children". + Child order is significant (a conservative choice: only literal duplicates flag).""" + head = node.tag.split(":", 1)[-1] + attrs = tuple(sorted((k.split(":", 1)[-1], v) for k, v in node.attrib.items() + if k.split(":", 1)[-1] != "Key")) + kids = tuple(_deep_resource_sig(c) for c in node.children) + return (head, attrs, kids) + + def _rule_resource_hoist(root: Node, avalonia: bool, min_copies: int = 2) -> list[XamlFinding]: - """XAML100 — a heavy Freezable resource (brush/geometry/transform/image) declared - with ``x:Key`` in several **control-local** ```` scopes with the same - structure: each copy is a separate object multiplying working set across siblings, - when one shared resource at window/app scope would do (the "52x52 Brush collapse" - of the design note). Restricted to Freezables, where the shallow structural - signature is reliable; Styles/templates (deep structural equivalence) are a later - refinement. Resources already at a shared (root/window/app) scope are the hoist - *target*, not a finding.""" + """XAML100 — a heavy keyed resource (Freezable brush/geometry/transform/image, or a + ``Style``/template) declared in several **control-local** ```` scopes + with the same *deep* structure: each copy is a separate object multiplying working + set across siblings, when one shared resource at window/app scope would do (the + "52x52 Brush collapse" of the design note). Equivalence is the full-subtree + ``_deep_resource_sig``, so a `Style`/template only collapses with a structurally + identical twin (same setters/values), never merely a same-shaped one. Resources + already at a shared (root/window/app) scope are the hoist *target*, not a finding.""" by_sig: dict[tuple[Any, ...], list[Node]] = {} for rd in _resource_dictionaries(root): if _scope_owner(rd) in TOP_LEVEL_SCOPES: continue # already a shared scope — the place to hoist *to* for _key, c in _keyed_resources(rd): - if c.type_name() not in FREEZABLE_TYPES: + if c.type_name() not in HOISTABLE_RESOURCE_TYPES: continue - sig = _inline_freezable_sig(c) # type + non-key attrs + child types + sig = _deep_resource_sig(c) # full subtree: tag + non-key attrs + children if not sig[1] and not sig[2]: continue # empty/defaulted element — nothing meaningful to share by_sig.setdefault(sig, []).append(c) @@ -1037,6 +1059,24 @@ def rules(text: str) -> dict[str, XamlFinding]: check("XAML100" not in rules(distinct), "XAML100 false positive: structurally distinct local resources are not duplicates") + # XAML100 for Styles — the deep signature: an IDENTICAL style in two local scopes + # is a duplicate, but two styles with different Setter values are NOT. + def _two_styles(setter1: str, setter2: str) -> str: + def blk(owner: str, setter: str) -> str: + return (f' <{owner}><{owner}.Resources>\n' + ' \n' + f' \n') + return (f'\n \n' + + blk("Border", setter1) + blk("StackPanel", setter2) + + ' \n\n') + check("XAML100" in rules(_two_styles("Red", "Red")), + "XAML100 must flag an identical Style duplicated across control-local scopes") + check("XAML100" not in rules(_two_styles("Red", "Blue")), + "XAML100 false positive: Styles with different Setter values are not duplicates " + "(deep structural signature)") + # Implicit dictionary (the common WPF syntax, no # wrapper) must feed the keyed-resource rules — else XAML101/102/106 miss most files. implicit = (f'\n' diff --git a/docs/notes/xaml-analyzer-design.md b/docs/notes/xaml-analyzer-design.md index bca64b6a..679c7446 100644 --- a/docs/notes/xaml-analyzer-design.md +++ b/docs/notes/xaml-analyzer-design.md @@ -110,7 +110,7 @@ offending element's stamped line; a rule that can only locate a file-level issue | Rule | What it flags | Doc rationale | Avalonia | |---|---|---|---| -| **XAML100** `ResourceShouldBeHoisted` ✅ *(Freezables)* | heavy shared resource (Brush/Geometry/Transform/Image — Style/template deferred) keyed identically in ≥2 control-local `.Resources` scopes | per-instance control resources multiply working set; app/window scope shares (the 52×52 Brush collapse) | ✅ scope model maps | +| **XAML100** `ResourceShouldBeHoisted` ✅ | heavy shared resource (Brush/Geometry/Transform/Image, or Style/template via a full-subtree signature) keyed identically in ≥2 control-local `.Resources` scopes | per-instance control resources multiply working set; app/window scope shares (the 52×52 Brush collapse) | ✅ scope model maps | | **XAML101** `DuplicateStatelessConverterResource` | identical stateless converter declared in many local dictionaries | converters are normally one shared instance; duplication is churn | ✅ | | **XAML102** `DynamicResourceLikelyStatic` | `DynamicResource` for an app-local, lexically-stable, non-theme/system key | StaticResource recommended unless runtime-mutated; dynamic carries deferred lookup cost | ❌ Avalonia DynamicResource semantics differ | | **XAML103** `SuspiciousSharedFalse` | `x:Shared="False"` on converters/styles/brushes outside documented exceptions | resources shared by default; `x:Shared=false` is the deliberate opt-out | ❌ WPF-only attribute | From 57ed39f573656fea45953199811b979fc23fce43 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 26 Jun 2026 20:47:30 +0000 Subject: [PATCH 2/2] =?UTF-8?q?audit(xaml):=20XAML100=20deep=20signature?= =?UTF-8?q?=20=E2=80=94=20capture=20text=20+=20precise=20x:Key=20exclusion?= =?UTF-8?q?=20(review)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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 Claude-Session: https://claude.ai/code/session_015zoG4YNCUf7RA5r8GAXMmb --- audit/static/tools/xaml_check.py | 65 ++++++++++++++++++++++++++++---- 1 file changed, 57 insertions(+), 8 deletions(-) diff --git a/audit/static/tools/xaml_check.py b/audit/static/tools/xaml_check.py index eedaa77f..a76c6938 100644 --- a/audit/static/tools/xaml_check.py +++ b/audit/static/tools/xaml_check.py @@ -139,6 +139,7 @@ class Node: line: int children: list[Node] = field(default_factory=list) parent: Node | None = None + text: str = "" # accumulated character data (used by the deep XAML100 signature) def local(self) -> str: """The unqualified element name: ``controls:DataGrid`` -> ``DataGrid``; @@ -198,8 +199,16 @@ def end(_name: str) -> None: if stack: stack.pop() + def chardata(data: str) -> None: + # expat may deliver a run of text in several pieces; accumulate on the open + # element. Used by the deep XAML100 signature so templates that differ only + # in text (OK vs Cancel) are not treated as identical. + if stack: + stack[-1].text += data + parser.StartElementHandler = start parser.EndElementHandler = end + parser.CharacterDataHandler = chardata try: parser.Parse(text, True) except xml.parsers.expat.ExpatError: @@ -404,18 +413,27 @@ def _scope_owner(rd: Node) -> str: return "root" +def _is_xkey(name: str) -> bool: + """The XAML resource-key directive ``x:Key`` (any prefix bound to the xaml + namespace) — but NOT a CLR ``Key`` property such as ``KeyBinding.Key``, which is a + real value that must stay in the signature.""" + return ":" in name and name.rsplit(":", 1)[-1] == "Key" + + def _deep_resource_sig(node: Node) -> tuple[Any, ...]: """A *recursive* structural signature: the element's tag (sans namespace prefix, - keeping the property-element dotted form), its non-``x:Key`` attributes, and the - signatures of all its children in document order. Two resources share this only - when their entire subtree is structurally identical — so a `Style`/`ControlTemplate` - is matched on its actual setters/triggers/template, not just "has Setter children". - Child order is significant (a conservative choice: only literal duplicates flag).""" + keeping the property-element dotted form), its attributes (excluding only the + resource directive ``x:Key`` — a descendant CLR ``Key`` like ``KeyBinding Key=...`` + is kept), its trimmed character data, and the signatures of all its children in + document order. Two resources share this only when their entire subtree — + elements, attribute values AND text — is structurally identical, so a + `Style`/`ControlTemplate` is matched on its actual setters/triggers/template/text, + not just "has Setter children". Child order is significant (a conservative choice: + only literal duplicates flag).""" head = node.tag.split(":", 1)[-1] - attrs = tuple(sorted((k.split(":", 1)[-1], v) for k, v in node.attrib.items() - if k.split(":", 1)[-1] != "Key")) + attrs = tuple(sorted((k, v) for k, v in node.attrib.items() if not _is_xkey(k))) kids = tuple(_deep_resource_sig(c) for c in node.children) - return (head, attrs, kids) + return (head, attrs, kids, node.text.strip()) def _rule_resource_hoist(root: Node, avalonia: bool, @@ -1077,6 +1095,37 @@ def blk(owner: str, setter: str) -> str: "XAML100 false positive: Styles with different Setter values are not duplicates " "(deep structural signature)") + # Deep signature must include character data: two DataTemplates that differ ONLY in + # text are not duplicates; identical text still collapses (recall preserved). + def _two_templates(text1: str, text2: str) -> str: + def blk(owner: str, txt: str) -> str: + return (f' <{owner}><{owner}.Resources>\n' + f' {txt}' + '\n' + f' \n') + return (f'\n \n' + + blk("Border", text1) + blk("StackPanel", text2) + + ' \n\n') + check("XAML100" not in rules(_two_templates("OK", "Cancel")), + "XAML100 false positive: templates differing only in text are not duplicates") + check("XAML100" in rules(_two_templates("OK", "OK")), + "XAML100 must still flag templates that are identical incl. text") + + # The x:Key exclusion must be prefix-aware: a CLR `Key` (e.g. KeyBinding.Key) is a + # real value, so templates differing only by it are NOT duplicates. + kb = ('\n \n' + ' \n' + ' \n' + ' \n\n') + check("XAML100" not in rules(kb), + "XAML100 false positive: descendant CLR Key (KeyBinding Key=) must stay in the signature") + # Implicit dictionary (the common WPF syntax, no # wrapper) must feed the keyed-resource rules — else XAML101/102/106 miss most files. implicit = (f'\n'