diff --git a/audit/README.md b/audit/README.md index 5a344f5b..8adbb5a8 100644 --- a/audit/README.md +++ b/audit/README.md @@ -130,11 +130,11 @@ python audit/static/run_static.py --selftest # full pipeline end-to-end on fix HTML)**, the analyzer-injection props/targets, and selftests. - **XAML analyzer (Phase 1, markup-only) — done:** a build-free, stdlib-XML pass (`static/tools/xaml_check.py`) feeding the same pipeline as a second fact source — - line-preserving parse, the canonical SARIF record, and rules XAML101/102/103/104/ - 106/107/108/109/110/111/112/113 (virtualization-off, per-keystroke binding, template - complexity, Freezable/x:Shared/DynamicResource/merged-dictionary perf, image - decode-at-full-size, LayoutTransform cost, TemplateBinding opportunities, and inline - Freezable duplication). This makes category 8 (broken virtualization) statically + line-preserving parse, the canonical SARIF record, and rules XAML100/101/102/103/104/ + 106/107/108/109/110/111/112/113 (resource hoisting, virtualization-off, per-keystroke + binding, template complexity, Freezable/x:Shared/DynamicResource/merged-dictionary + perf, image decode-at-full-size, LayoutTransform cost, TemplateBinding opportunities, + and inline Freezable duplication). This makes category 8 (broken virtualization) statically covered, not NO-TOOL. Design + the full rule catalogue, phasing, and the Phase-2 binding-path join: [`../docs/notes/xaml-analyzer-design.md`](../docs/notes/xaml-analyzer-design.md). - **XAML Phase-2 seam — done:** `static/tools/xaml_facts.py` emits `xaml-facts.json` (resource graph diff --git a/audit/static/taxonomy/categories.yml b/audit/static/taxonomy/categories.yml index 2b573d7a..20a2452f 100644 --- a/audit/static/taxonomy/categories.yml +++ b/audit/static/taxonomy/categories.yml @@ -56,6 +56,7 @@ rules: # -> baseline -> ratchet path as a .cs finding. Exact ids win over the XAML1* # glob, which is a safety net so a not-yet-mapped XAML rule never lands in # `uncategorized` (the design note's "nothing quietly falls through"). + "XAML100": {category: 9, name: resource-hoist} # heavy resource duplicated per-scope "XAML107": {category: 8, name: broken-virtualization} # virtualization disabled "XAML109": {category: 8, name: template-complexity} # visual-tree inflation "XAML108": {category: 6, name: binding-update-frequency} # per-keystroke source flood diff --git a/audit/static/tools/xaml_check.py b/audit/static/tools/xaml_check.py index fa63077d..2db11ee2 100644 --- a/audit/static/tools/xaml_check.py +++ b/audit/static/tools/xaml_check.py @@ -27,6 +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 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 @@ -40,11 +41,12 @@ XAML112 TemplateBindingOpportunity (cat 9) cheaper compiled binding available XAML113 InlineFreezableDuplication (cat 9) identical inline brush/geometry -Deferred to a later Phase-1 slice (documented so nothing on the wishlist quietly -falls through — the design note's stated goal): XAML100 ResourceShouldBeHoisted -(needs the cross-sibling scope model) and XAML105 MergedDictionaryKeyShadowing -across *external* dictionaries (needs cross-file resolution). Phase 2 (Roslyn-linked -XAML2xx) and Phase 3 (runtime correlation) live elsewhere per the design note. +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. WPF-only rules (XAML102/103/106) are skipped on Avalonia ``.axaml`` because the ``DynamicResource`` / ``x:Shared`` / ``Freezable`` semantics differ or do not exist @@ -89,6 +91,11 @@ DYNAMIC_KEY_PREFIXES = ("System", "Theme", "{x:Static") DYNAMIC_KEY_TYPES = ("SystemColors", "SystemParameters", "SystemFonts") +# Resource scopes that are already shared (a hoist *target*, not a control-local copy): +# the document root and the window/app/page-level dictionaries (XAML100). +TOP_LEVEL_SCOPES = {"root", "Window", "UserControl", "Application", "Page", + "NavigationWindow", "ResourceDictionary"} + # Binding markers for XAML108 (per-keystroke source updates). _TWOWAY_RE = re.compile(r"Mode\s*=\s*TwoWay", re.IGNORECASE) _PROPCHANGED_RE = re.compile(r"UpdateSourceTrigger\s*=\s*PropertyChanged", re.IGNORECASE) @@ -377,6 +384,55 @@ def _keyed_resources(rd: Node): yield key, c +def _scope_owner(rd: Node) -> str: + """The element type that owns a resource dictionary — the nearest ```` + ancestor (``Grid``, ``Border``, ``DataTemplate`` …), or ``root`` for the document + root / a standalone ResourceDictionary file.""" + node: Node | None = rd + while node is not None: + if node.is_property_element() and node.local() == "Resources": + return node.type_name() + node = node.parent + return "root" + + +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.""" + 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: + continue + sig = _inline_freezable_sig(c) # type + non-key attrs + child types + if not sig[1] and not sig[2]: + continue # empty/defaulted element — nothing meaningful to share + by_sig.setdefault(sig, []).append(c) + + out: list[XamlFinding] = [] + for copies in by_sig.values(): + if len(copies) < min_copies: + continue + first = copies[0] + for c in copies[1:]: + out.append(XamlFinding( + "XAML100", c.line, + f"{c.type_name()} resource is declared identically in " + f"{len(copies)} control-local scopes (first at line {first.line}); " + "hoist it to one shared window/app resource instead of multiplying " + "working set across siblings [resource: hoistable resource]")) + return out + + def _rule_duplicate_converter(root: Node, avalonia: bool) -> list[XamlFinding]: """XAML101 — an identical stateless converter declared in several dictionaries. Stateless = a keyed element with no child content and no configuring attributes @@ -628,6 +684,7 @@ def _rule_layout_transform(root: Node, avalonia: bool) -> list[XamlFinding]: RULES: list[Callable[[Node, bool], list[XamlFinding]]] = [ + _rule_resource_hoist, _rule_virtualization, _rule_template_complexity, _rule_per_keystroke_binding, @@ -948,6 +1005,38 @@ def rules(text: str) -> dict[str, XamlFinding]: '\n') check("XAML111" in rules(lt), "XAML111 must flag a LayoutTransform") + # XAML100 — the same heavy brush keyed in two CONTROL-LOCAL scopes -> hoist. + hoist = (f'\n' + ' \n' + ' \n' + ' \n' + ' \n' + ' \n' + ' \n' + ' \n' + ' \n' + '\n') + r = rules(hoist) + check("XAML100" in r, "XAML100 must flag a heavy resource duplicated across local scopes") + # The SAME brush at the (shared) UserControl scope is the hoist target -> no finding. + topscope = (f'\n' + ' \n' + ' \n' + ' \n' + ' \n' + ' \n' + ' \n' + '\n') + check("XAML100" not in rules(topscope), + "XAML100 false positive: only one control-local copy (the other is a shared scope)") + # Distinct brushes in two local scopes -> not the same resource, no finding. + distinct = hoist.replace('Color="#FF0080FF" />\n' + ' ', + 'Color="#FF00FF80" />\n' + ' ') + check("XAML100" not in rules(distinct), + "XAML100 false positive: structurally distinct local resources are not duplicates") + # 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 03a73733..bca64b6a 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` | heavy shared resource (Brush/Style/Geometry/Transform/BitmapImage/template) declared in a control-local dictionary, recurring across siblings | per-instance control resources multiply working set; app/window scope shares (the 52×52 Brush collapse) | ✅ scope model maps | +| **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 | | **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 |