Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions audit/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions audit/static/taxonomy/categories.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
99 changes: 94 additions & 5 deletions audit/static/tools/xaml_check.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 ``<X.Resources>``
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** ``<X.Resources>`` 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Include descendant values in XAML100 signatures

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 👍 / 👎.

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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -948,6 +1005,38 @@ def rules(text: str) -> dict[str, XamlFinding]:
'</StackPanel>\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'<UserControl {_WPF_NS}>\n'
' <Grid>\n'
' <Border><Border.Resources>\n'
' <SolidColorBrush x:Key="accent" Color="#FF0080FF" />\n'
' </Border.Resources></Border>\n'
' <StackPanel><StackPanel.Resources>\n'
' <SolidColorBrush x:Key="accent2" Color="#FF0080FF" />\n'
' </StackPanel.Resources></StackPanel>\n'
' </Grid>\n'
'</UserControl>\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'<UserControl {_WPF_NS}>\n'
' <UserControl.Resources>\n'
' <SolidColorBrush x:Key="accent" Color="#FF0080FF" />\n'
' </UserControl.Resources>\n'
' <Border><Border.Resources>\n'
' <SolidColorBrush x:Key="accent2" Color="#FF0080FF" />\n'
' </Border.Resources></Border>\n'
'</UserControl>\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'
' </StackPanel.Resources>',
'Color="#FF00FF80" />\n'
' </StackPanel.Resources>')
check("XAML100" not in rules(distinct),
"XAML100 false positive: structurally distinct local resources are not duplicates")

# Implicit <X.Resources> dictionary (the common WPF syntax, no <ResourceDictionary>
# wrapper) must feed the keyed-resource rules — else XAML101/102/106 miss most files.
implicit = (f'<Window {_WPF_NS}>\n'
Expand Down
2 changes: 1 addition & 1 deletion docs/notes/xaml-analyzer-design.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
Loading