- Notifications
You must be signed in to change notification settings - Fork 0
audit(xaml): add XAML105 MergedDictionaryKeyShadowing (in-file)#130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -32,6 +32,7 @@ | ||
| XAML102 DynamicResourceLikelyStatic (cat 9) WPF-only, deferred-lookup cost | ||
| XAML103 SuspiciousSharedFalse (cat 9) WPF-only, x:Shared opt-out | ||
| XAML104 DuplicateMergedDictionaryInclude (cat 9) wasted load + order ambiguity | ||
| XAML105 MergedDictionaryKeyShadowing (cat 9) in-file key collision, order-dependent | ||
| XAML106 FreezableResourceShouldFreeze (cat 9) WPF-only, change-notify overhead | ||
| XAML107 VirtualizationExplicitlyDisabled (cat 8) virtualization accidentally killed | ||
| XAML108 PerKeystrokeBindingWithoutDelay (cat 6) per-keystroke source flooding | ||
| @@ -43,10 +44,11 @@ | ||
| 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. | ||
| covers the *in-file* merged-dictionary key collision (inline dictionaries + the | ||
| primary); the same shadowing across **external** ``Source=`` dictionaries needs | ||
| cross-file resolution and 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 | ||
| @@ -401,6 +403,52 @@ def _keyed_resources(rd: Node): | ||
| yield key, c | ||
| def _rule_merged_dict_shadowing(root: Node, avalonia: bool) -> list[XamlFinding]: | ||
| """XAML105 — the same ``x:Key`` defined in more than one **inline** merged | ||
| dictionary (or in the primary dictionary AND an inline merged one): the effective | ||
| value then depends on merge order (WPF: last merged wins, and a primary key beats | ||
| merged ones), a silent order-dependence that breaks when includes are reordered. | ||
| In-file only: dictionaries pulled in by ``Source="..."`` reference external files | ||
| this single-file pass can't resolve, so their keys aren't compared (the cross-file | ||
| variant is a documented later slice). FP-safe — only literal same-key collisions | ||
| flag, and a lone key (the normal case) never does.""" | ||
| out: list[XamlFinding] = [] | ||
| for md in root.walk(): | ||
| if md.local() != "MergedDictionaries" or not md.is_property_element(): | ||
| continue | ||
| host = md.parent # the owning ResourceDictionary / <X.Resources> | ||
| # key -> [(where, line)] across the primary dict and each inline merged dict | ||
| keymap: dict[str, list[tuple[str, int]]] = {} | ||
| if host is not None: | ||
| for key, c in _keyed_resources(host): | ||
| keymap.setdefault(key, []).append(("primary", c.line)) | ||
| inline_dicts = [c for c in md.children | ||
| if c.type_name() == "ResourceDictionary" and c.attr("Source") is None] | ||
| for i, mdict in enumerate(inline_dicts, start=1): | ||
| for key, c in _keyed_resources(mdict): | ||
| keymap.setdefault(key, []).append((f"merged #{i}", c.line)) | ||
| for key, occ in keymap.items(): | ||
| # Shadowing requires the key in 2+ DISTINCT scopes (primary vs merged, or | ||
| # two different merged dicts). Two entries in the SAME scope are a | ||
| # duplicate-key error (runtime-invalid) — a different problem, not XAML105. | ||
| # Collapse to one line per scope (first occurrence) so the count and the | ||
| # "where" list reflect distinct scopes, not repeated same-scope entries. | ||
| by_scope: dict[str, int] = {} | ||
| for w, ln in occ: | ||
| by_scope.setdefault(w, ln) | ||
| if len(by_scope) < 2: | ||
| continue | ||
| where = ", ".join(f"{w} (line {ln})" for w, ln in by_scope.items()) | ||
| out.append(XamlFinding( | ||
| "XAML105", next(iter(by_scope.values())), | ||
| f"resource key '{key}' is defined in {len(by_scope)} merged/primary scopes " | ||
| f"[{where}]; the effective value depends on merge order (last merged " | ||
| "wins, primary beats merged) [resource: merged dictionary]")) | ||
coderabbitai[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| return out | ||
| 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 | ||
| @@ -725,6 +773,7 @@ def _rule_layout_transform(root: Node, avalonia: bool) -> list[XamlFinding]: | ||
| RULES: list[Callable[[Node, bool], list[XamlFinding]]] = [ | ||
| _rule_resource_hoist, | ||
| _rule_merged_dict_shadowing, | ||
| _rule_virtualization, | ||
| _rule_template_complexity, | ||
| _rule_per_keystroke_binding, | ||
| @@ -941,6 +990,70 @@ def rules(text: str) -> dict[str, XamlFinding]: | ||
| check(r.get("XAML104") and r["XAML104"].line == 5, | ||
| "XAML104 must point at the duplicate include (line 5)") | ||
| # XAML105 — the same key in two inline merged dictionaries (order-dependent). | ||
| shadow = (f'<ResourceDictionary {_WPF_NS}>\n' | ||
| ' <ResourceDictionary.MergedDictionaries>\n' | ||
| ' <ResourceDictionary>\n' | ||
| ' <SolidColorBrush x:Key="accent" Color="Red" />\n' | ||
| ' </ResourceDictionary>\n' | ||
| ' <ResourceDictionary>\n' | ||
| ' <SolidColorBrush x:Key="accent" Color="Blue" />\n' | ||
| ' </ResourceDictionary>\n' | ||
| ' </ResourceDictionary.MergedDictionaries>\n' | ||
| '</ResourceDictionary>\n') | ||
| r = rules(shadow) | ||
| check("XAML105" in r, "XAML105 must flag a key defined in two inline merged dictionaries") | ||
| check(r.get("XAML105") and "accent" in r["XAML105"].message, "XAML105 must name the key") | ||
| # primary key + a merged-dictionary key collision is also shadowing. | ||
| primary = (f'<ResourceDictionary {_WPF_NS}>\n' | ||
| ' <SolidColorBrush x:Key="accent" Color="Green" />\n' | ||
| ' <ResourceDictionary.MergedDictionaries>\n' | ||
| ' <ResourceDictionary>\n' | ||
| ' <SolidColorBrush x:Key="accent" Color="Blue" />\n' | ||
| ' </ResourceDictionary>\n' | ||
| ' </ResourceDictionary.MergedDictionaries>\n' | ||
| '</ResourceDictionary>\n') | ||
| check("XAML105" in rules(primary), | ||
| "XAML105 must flag a primary key shadowed by a merged dictionary") | ||
| # distinct keys across merged dicts -> no collision, no finding. | ||
| nodup = shadow.replace('x:Key="accent" Color="Blue"', 'x:Key="other" Color="Blue"') | ||
| check("XAML105" not in rules(nodup), | ||
| "XAML105 false positive: distinct keys across merged dictionaries do not shadow") | ||
| # external Source dictionaries are not resolved -> not compared (deferred). | ||
| check("XAML105" not in rules(dup), | ||
| "XAML105 must not compare keys of external Source= dictionaries (cross-file deferred)") | ||
| # two primary entries with the SAME key are a duplicate-key error in ONE scope, not | ||
| # order-dependent merge shadowing -> XAML105 must not fire (needs 2+ distinct scopes). | ||
| samescope = (f'<ResourceDictionary {_WPF_NS}>\n' | ||
| ' <SolidColorBrush x:Key="accent" Color="Green" />\n' | ||
| ' <SolidColorBrush x:Key="accent" Color="Teal" />\n' | ||
| ' <ResourceDictionary.MergedDictionaries>\n' | ||
| ' <ResourceDictionary>\n' | ||
| ' <SolidColorBrush x:Key="other" Color="Blue" />\n' | ||
| ' </ResourceDictionary>\n' | ||
| ' </ResourceDictionary.MergedDictionaries>\n' | ||
| '</ResourceDictionary>\n') | ||
| check("XAML105" not in rules(samescope), | ||
| "XAML105 false positive: same-scope duplicate keys are not merge-order shadowing") | ||
| # a key duplicated in primary AND present in a merged dict still shadows across the | ||
| # two distinct scopes, but the same-scope dup must collapse: report 2 scopes (not 3), | ||
| # with 'primary' listed once. | ||
| dupprimary = (f'<ResourceDictionary {_WPF_NS}>\n' | ||
| ' <SolidColorBrush x:Key="accent" Color="Green" />\n' | ||
| ' <SolidColorBrush x:Key="accent" Color="Teal" />\n' | ||
| ' <ResourceDictionary.MergedDictionaries>\n' | ||
| ' <ResourceDictionary>\n' | ||
| ' <SolidColorBrush x:Key="accent" Color="Blue" />\n' | ||
| ' </ResourceDictionary>\n' | ||
| ' </ResourceDictionary.MergedDictionaries>\n' | ||
| '</ResourceDictionary>\n') | ||
| dp = rules(dupprimary) | ||
| check("XAML105" in dp, "XAML105 must flag a primary key also defined in a merged dict") | ||
| check(dp.get("XAML105") and "2 merged/primary scopes" in dp["XAML105"].message, | ||
| "XAML105 must collapse same-scope duplicates: 2 distinct scopes, not 3") | ||
| check(dp.get("XAML105") and dp["XAML105"].message.count("primary (line") == 1, | ||
| "XAML105 must list each distinct scope once (no repeated 'primary')") | ||
| # XAML101 — duplicate stateless converter across dictionaries. | ||
| conv = (f'<ResourceDictionary {_WPF_NS} xmlns:c="clr-namespace:App.Converters">\n' | ||
| ' <c:BoolToVisibilityConverter x:Key="b2v" />\n' | ||
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
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Uh oh!
There was an error while loading. Please reload this page.