diff --git a/audit/static/tools/xaml_check.py b/audit/static/tools/xaml_check.py index 2db11ee2..a76c6938 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) @@ -131,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``; @@ -190,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: @@ -396,24 +413,47 @@ 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 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, 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, node.text.strip()) + + 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 +1077,55 @@ 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)") + + # 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' 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 |