From be4cbd5f94ffe2a23454d93188cc1c82cabc5be1 Mon Sep 17 00:00:00 2001 From: Alexandre Zollinger Chohfi Date: Wed, 14 Oct 2026 16:37:38 -0700 Subject: [PATCH 01/10] Add REACTOR_DYM_006: flag color strings BrushHelper can't parse (#1375) Constant color strings that BrushHelper.ParseColor doesn't recognize silently render gray. The new ColorStringLiteralAnalyzer flags them on BrushHelper.Parse, ResourceBuilder.Set(string, string) and the string overloads of Background/Foreground/BorderBrush/WithBorder, with a did-you-mean: - *FontFamily resource key -> new FontFamily(...) (only when the Set(string, FontFamily) overload exists, PR #1370) - #RGB/#ARGB shorthand -> #RRGGBB/#AARRGGBB; missing '#' -> add it - CSS/WinUI named color -> exact hex ColorStringLiteralCodeFix applies these rewrites. The mur check ColorStringLiteralRule mirrors it, anchored on the analyzer ID since the mistake raises no compiler error. BrushHelper gains an internal TryParseColor (no behavior change) so ColorStringParityTests can lock BrushHelper and both classifier copies together. Fixes the in-repo hits it found (AliceBlue in wct-controls, #888 in ReactorHostControlDemo), which all rendered gray. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b58aacd0-29b6-46c1-9603-b8a8b4eecbd8 --- .../templates/analyzer-architecture.md.dt | 1 + docs/guide/analyzer-architecture.md | 1 + docs/reference/mur-check-did-you-mean.md | 3 +- docs/specs/061-in-build-did-you-mean.md | 61 +++- .../skills/reactor-build-and-check/SKILL.md | 1 + .../ReactorHostControlDemo/MainWindow.xaml.cs | 4 +- samples/apps/wct-controls/Program.cs | 6 +- .../AnalyzerReleases.Unshipped.md | 1 + .../ColorStringClassifier.cs | 333 +++++++++++++++++ .../ColorStringLiteralAnalyzer.cs | 267 ++++++++++++++ .../ColorStringLiteralCodeFix.cs | 92 +++++ .../Check/Rules/ColorStringClassifier.cs | 335 +++++++++++++++++ .../Check/Rules/ColorStringLiteralRule.cs | 180 ++++++++++ src/Reactor.Cli/Check/Rules/RuleRegistry.cs | 1 + src/Reactor/Elements/BrushHelper.cs | 66 +++- src/Reactor/Elements/ResourceBuilder.cs | 7 +- .../ColorStringLiteralAnalyzerTests.cs | 339 ++++++++++++++++++ .../AnalyzerTests/ColorStringParityTests.cs | 100 ++++++ .../DidYouMeanAnalyzerParityTests.cs | 28 ++ .../Rules/ColorStringLiteralRuleTests.cs | 154 ++++++++ .../Rules/RulePerformanceTests.cs | 15 + 21 files changed, 1965 insertions(+), 30 deletions(-) create mode 100644 src/Reactor.Analyzers/ColorStringClassifier.cs create mode 100644 src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs create mode 100644 src/Reactor.Analyzers/ColorStringLiteralCodeFix.cs create mode 100644 src/Reactor.Cli/Check/Rules/ColorStringClassifier.cs create mode 100644 src/Reactor.Cli/Check/Rules/ColorStringLiteralRule.cs create mode 100644 tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs create mode 100644 tests/Reactor.Tests/AnalyzerTests/ColorStringParityTests.cs create mode 100644 tests/Reactor.Tests/CheckCommandTests/Rules/ColorStringLiteralRuleTests.cs diff --git a/docs/_pipeline/templates/analyzer-architecture.md.dt b/docs/_pipeline/templates/analyzer-architecture.md.dt index a58fe08c2..a76c71a06 100644 --- a/docs/_pipeline/templates/analyzer-architecture.md.dt +++ b/docs/_pipeline/templates/analyzer-architecture.md.dt @@ -179,6 +179,7 @@ sibling `src/Reactor.Analyzers.Internal/` project instead; see | `REACTOR_DYM_003` | Warning | Mistyped Reactor factory name in call position (e.g. `Buton(...)`) | `FuzzyFactoryNameAnalyzer.cs` | | `REACTOR_DYM_004` | Warning | Reactor factory called with too few arguments (CS7036), single unique overload — suggests the parameter shape | `MissingFactoryArgumentAnalyzer.cs` | | `REACTOR_DYM_005` | Warning | String passed where a Reactor `Element` is expected (CS1503) — wrap it in a text factory | `StringForElementArgumentAnalyzer.cs` | +| `REACTOR_DYM_006` | Warning | Constant color string `BrushHelper` can't parse, so it renders gray (e.g. `.Background("orange")`, `.Foreground("#888")`, a font name under a `*FontFamily` resource key) — suggests the hex value or `new FontFamily(...)` | `ColorStringLiteralAnalyzer.cs` | `REACTOR_HOOKS_002` and `_003` were the reserved control-flow / data-flow slots (variable hook counts across early returns, async boundaries inside `UseEffect`); diff --git a/docs/guide/analyzer-architecture.md b/docs/guide/analyzer-architecture.md index 073fb2cf3..f3d0876fa 100644 --- a/docs/guide/analyzer-architecture.md +++ b/docs/guide/analyzer-architecture.md @@ -205,6 +205,7 @@ sibling `src/Reactor.Analyzers.Internal/` project instead; see | `REACTOR_DYM_003` | Warning | Mistyped Reactor factory name in call position (e.g. `Buton(...)`) | `FuzzyFactoryNameAnalyzer.cs` | | `REACTOR_DYM_004` | Warning | Reactor factory called with too few arguments (CS7036), single unique overload — suggests the parameter shape | `MissingFactoryArgumentAnalyzer.cs` | | `REACTOR_DYM_005` | Warning | String passed where a Reactor `Element` is expected (CS1503) — wrap it in a text factory | `StringForElementArgumentAnalyzer.cs` | +| `REACTOR_DYM_006` | Warning | Constant color string `BrushHelper` can't parse, so it renders gray (e.g. `.Background("orange")`, `.Foreground("#888")`, a font name under a `*FontFamily` resource key) — suggests the hex value or `new FontFamily(...)` | `ColorStringLiteralAnalyzer.cs` | `REACTOR_HOOKS_002` and `_003` were the reserved control-flow / data-flow slots (variable hook counts across early returns, async boundaries inside `UseEffect`); diff --git a/docs/reference/mur-check-did-you-mean.md b/docs/reference/mur-check-did-you-mean.md index 51052ef99..14fe7f119 100644 --- a/docs/reference/mur-check-did-you-mean.md +++ b/docs/reference/mur-check-did-you-mean.md @@ -168,7 +168,7 @@ Each rule is a small file under `src/Reactor.Cli/Check/Rules/`, registered into Each rule passes a six-bar **Validation Gate** before merge (frequency, cross-agent reproducibility, positive fixtures, negative counter-examples, independent reviewer signoff, kill-switch). The gate exists because a bad rule is worse than no rule — it contaminates every downstream agent session. Class-B (vocabulary-translation) rules waive bar #1 (frequency) — their justification is the documented prior-framework citation rather than a corpus cluster. The audit at `docs/specs/tasks/038-tuning-reports/2026-05-11-cross-agent-audit.md` is where bar #2 (cross-agent) gets formally cleared per cluster. -**Six rules shipped to date** (four Class-A induced + two Class-B vocabulary): +**Seven rules shipped to date** (four Class-A induced + two Class-B vocabulary + one analyzer mirror): | Rule | Class | Code(s) | Receiver | What it suggests | Cross-agent events | |---|---|---|---|---|---| @@ -178,6 +178,7 @@ Each rule passes a six-bar **Validation Gate** before merge (frequency, cross-ag | `GridSizePxRenameRule` | A | CS0117 | `GridSize` | `GridSize.Px(...)` for `Pixel`/`Pixels`/`Fixed` legacy names | 9 | | `TextBlockStyleHintRule` | A | CS1061 / CS0117 | `TextBlockElement` | fluent text helpers (`.FontSize`, `.SemiBold`, …); Reactor has no `Style` member | 5 across both `.Style(...)` and `with { Style = ... }` shapes | | `ThemeRawResourceKeyRule` | A | CS0117 | `Theme` | real `Theme.*` aliases (`Theme.LayerFill`, `Theme.Accent`, …) for WinUI `FillColor` ThemeResource key names | run5 opus-4.7 only; bar #2 partial | +| `ColorStringLiteralRule` | analyzer mirror | `REACTOR_DYM_006` | `BrushHelper` / `ResourceBuilder` / `ElementExtensions` | the hex for an unparseable constant color string (`"orange"` → `"#FFA500"`, `"#888"` → `"#888888"`), or `new FontFamily("…")` under a `*FontFamily` resource key. **First rule anchored on an analyzer ID**: the mistake compiles, so there is no CS code to hang it on (issue #1375) | n/a — mirrors the in-build analyzer | ### Suggest-gate carve-out for Tier-3 rules diff --git a/docs/specs/061-in-build-did-you-mean.md b/docs/specs/061-in-build-did-you-mean.md index 9a0254676..656556afb 100644 --- a/docs/specs/061-in-build-did-you-mean.md +++ b/docs/specs/061-in-build-did-you-mean.md @@ -5,8 +5,9 @@ **Implemented (in part) — 2026-07-07.** Phase 1 (`REACTOR_DYM_001`) and Phase 2 (`REACTOR_DYM_002`) have shipped, along with the first fuzzy analyzer (`REACTOR_DYM_003`, the CS0103 mistyped-factory-name case) and the argument-shape analyzers (`REACTOR_DYM_004`, CS7036 missing -argument; `REACTOR_DYM_005`, the CS1503 string-for-Element case — see §7.2); the remaining fuzzy -member-resolution phases and the shared match-engine extraction remain design-only. This spec +argument; `REACTOR_DYM_005`, the CS1503 string-for-Element case — see §7.2), and the first did-you-mean +that rides **no** compiler error (`REACTOR_DYM_006`, unparseable color-string literals — see §7.3); the +remaining fuzzy member-resolution phases and the shared match-engine extraction remain design-only. This spec is the design of record for bringing Reactor's "did you mean" suggestions — today reachable only through the `mur check` CLI — into a plain `dotnet build` and the IDE, for consumers of the `Microsoft.UI.Reactor` NuGet package. @@ -321,14 +322,66 @@ produces uncompilable code is worse than none, so neither ships one. and the IDE. Tunable via `.editorconfig`. The shared gating lives in one `ArgumentShapeGate` helper so the two analyzers can't drift. +### §7.3 `REACTOR_DYM_006` — unparseable color-string literal (shipped) + +Issue #1375. Every Reactor API that takes a color *string* goes through `BrushHelper.ParseColor`, which +knows eight names (`red`, `green`, `blue`, `white`, `black`, `gray`/`grey`, `lightgray`/`lightgrey`, +`transparent`) plus `#RRGGBB` / `#AARRGGBB`. Anything else silently becomes a **gray** brush. So +`.Background("orange")`, `.Foreground("#888")`, and — once `ResourceBuilder.Set(string, FontFamily)` exists +(PR #1370) — `.Resources(r => r.Set("ContentControlThemeFontFamily", "Consolas"))` all compile and render +wrong. `ColorStringLiteralAnalyzer` + `ColorStringLiteralCodeFix` catch the constant cases. + +**The first DYM that rides no compiler error.** DYM_001–005 always co-occur with a CS error, which is what +made Warning safe under `TreatWarningsAsErrors`. This one fires on code that compiles. It is still a +**Warning**: the result is always a wrong brush, never a style preference (the same reasoning as +`REACTOR_MOD_003`). In-repo hits were fixed in the PR that introduced it rather than suppressed. + +- **Targets (symbol identity, resolved once per compilation).** `BrushHelper.Parse(string)`, + `ResourceBuilder.Set(string key, string color)`, and the `string color` overloads of `Background` / + `Foreground` / `BorderBrush` / `WithBorder` on `ElementExtensions` (generic and RichText). A look-alike API + in another namespace never matches. `D3Charts.Brush(string)` is out of scope: it parses with `D3Color`. +- **Values.** Only compile-time constants: literals, `const`s, constant concatenation, and each constant + branch of a `?:` / `??` / switch expression. A wholly-constant expression is checked as its single value, + so a dead branch is never reported. Runtime strings are out of reach; a runtime throw was considered and + left as a separate decision (it would change behaviour for apps relying on the fallback). +- **Grammar parity.** The analyzer can't reference `src/Reactor`, so `ColorStringClassifier` carries a + netstandard2.0 copy of the grammar, including its quirks (`TrimStart('#')` accepts `##FF0000`; + `NumberStyles.HexNumber` tolerates whitespace inside a byte pair). `BrushHelper` exposes an internal + `TryParseColor` (no behaviour change) so `ColorStringParityTests` can compare exactly — a plain + "result is gray" oracle can't tell an unparseable string from a legitimate `"gray"`. +- **Did-you-mean, in priority order.** (1) a `ResourceBuilder.Set` key ending in `FontFamily` suggests + `new FontFamily("…")`, but **only when** `Set(string, FontFamily)` is present in the compilation; without it + the message says the overload only stores color brushes and no fix is offered. (2) `#RGB` / `#ARGB` + shorthand → expanded `#RRGGBB` / `#AARRGGBB`. (3) six or eight hex digits without `#` → add it. (4) other + `#…` → "must be `#RRGGBB` or `#AARRGGBB`". (5) a standard CSS / WinUI named color (139 entries, including + the `grey` spellings) → its exact hex, cross-checked against `System.Drawing.KnownColor`. (6) anything else + → the supported list, hex, or a `Theme` token. +- **Fix.** Cases 1–3 and 5 carry the replacement in the property bag. Literal fixes rewrite the string + literal (including one inside a `?:` branch); the FontFamily fix wraps the whole argument (a literal or a + `const` reference) in a `Simplifier`-annotated `new FontFamily(...)`. + +**`mur check` mirror.** `ColorStringLiteralRule` is the first Tier-3 rule anchored on an **analyzer** ID +rather than a CS code — there is no compiler diagnostic to augment. `mur check` wraps `dotnet build`, which +runs the analyzer bundle, so `REACTOR_DYM_006` appears in the parsed output and the rule adds the same +rewrite as a `→ try:` suffix. It stays silent when the argument's unparseable branches disagree on a +rewrite, or when there is none. There is deliberately no Tier-1 `HintFor` entry, which would mask the +suggestion. `ColorStringParityTests` locks the two classifier copies together, and +`DidYouMeanAnalyzerParityTests` feeds real analyzer diagnostics to the rule and asserts it suggests the same +rewrite as the code fix. + +**Overlap with `REACTOR_THEME_001`.** THEME_001 already flags every string literal passed to `.Background` / +`.Foreground` / `.WithBorder` as "use a Theme token". That is a style nudge (and is downgraded to +`suggestion` across `samples/`). DYM_006 is the correctness subset: the literal doesn't just ignore the +theme, it renders gray. Both can fire on one literal. + ## §8 Risks and guardrails | Risk | Guardrail | |---|---| | False positives under cascading / partial-edit errors | Bail when the receiver type is unresolved or an error type; only fire when the invocation didn't bind; gate to Reactor symbols; (fuzzy phases) require high similarity thresholds. | | IDE typing latency | Use the provided `context.SemanticModel` (never `Compilation.GetSemanticModel`, RS1030); narrow syntax-node actions; cheap symbol pre-checks before any fuzzy match. | -| Divergence between analyzer and `mur check` | Phase 2: local maps locked to the CLI Tier-3 rules by a cross-check parity test (`DidYouMeanAnalyzerParityTests`). Argument-shape phase (§7.2): the same test asserts `REACTOR_DYM_004`/`_005` and the CLI `SymbolSuggester` both fire on the representative CS7036 / CS1503 shapes. Fuzzy phases: one shared match library + shared fixtures (§7 item 5). | -| Noise (a second diagnostic atop the CS error) | Concise message; `REACTOR_DYM_*` only ever accompanies a genuine compile error; consumers can tune severity via `.editorconfig`. | +| Divergence between analyzer and `mur check` | Phase 2: local maps locked to the CLI Tier-3 rules by a cross-check parity test (`DidYouMeanAnalyzerParityTests`). Argument-shape phase (§7.2): the same test asserts `REACTOR_DYM_004`/`_005` and the CLI `SymbolSuggester` both fire on the representative CS7036 / CS1503 shapes. `REACTOR_DYM_006` (§7.3): `ColorStringParityTests` locks `BrushHelper.TryParseColor` and both classifier copies together, and the rule is driven by real analyzer output. Fuzzy phases: one shared match library + shared fixtures (§7 item 5). | +| Noise (a second diagnostic atop the CS error) | Concise message; `REACTOR_DYM_001`–`_005` only ever accompany a genuine compile error; consumers can tune severity via `.editorconfig`. `REACTOR_DYM_006` is the exception (§7.3) and fires only on a value that provably renders gray. | ## §9 Relationship to `mur check` diff --git a/plugins/reactor/skills/reactor-build-and-check/SKILL.md b/plugins/reactor/skills/reactor-build-and-check/SKILL.md index 03022c386..2326caf94 100644 --- a/plugins/reactor/skills/reactor-build-and-check/SKILL.md +++ b/plugins/reactor/skills/reactor-build-and-check/SKILL.md @@ -142,6 +142,7 @@ Additional flags: | `REACTOR_DYM_003` | warning | An unresolved bare call closely matches a Reactor factory name (e.g. `Buton(...)`) — pairs with compiler `CS0103` | Rename to the suggested factory: `Button(...)`. Fires only on a close, unambiguous factory match (the typo flavour of `CS0103`); the IDE offers a one-click rename. | | `REACTOR_DYM_004` | warning | A Reactor factory is called with too few arguments (e.g. `ScrollViewer()`) — pairs with compiler `CS7036` | Supply the missing argument(s); the message lists the factory's full parameter shape as named arguments. Fires only when a single overload uniquely matches, so multi-overload factories (e.g. `Button()`) are deliberately left to the raw compiler error. Message only — no code fix (the `` placeholder wouldn't compile). | | `REACTOR_DYM_005` | warning | A `string` is passed where a Reactor `Element` is expected (e.g. `ScrollViewer("hi")`) — pairs with compiler `CS1503` | Wrap the string in a text factory: `ScrollViewer(TextBlock("hi"))` (or `Heading`/`Caption`). Narrow, high-confidence special case only; general type mismatches degrade to the raw `CS1503`. Message only — no code fix. | +| `REACTOR_DYM_006` | warning | A constant color string `BrushHelper` can't parse (e.g. `.Background("orange")`, `.Foreground("#888")`, `.Resources(r => r.Set("ContentControlThemeFontFamily", "Consolas"))`). It compiles and silently renders **gray**. | Color strings accept only `red`, `green`, `blue`, `white`, `black`, `gray`/`grey`, `lightgray`/`lightgrey`, `transparent`, `#RRGGBB` and `#AARRGGBB`. Use the suggested hex (`"orange"` → `"#FFA500"`, `"#888"` → `"#888888"`, `"FF0000"` → `"#FF0000"`) or a `Theme` token. A font under a `*FontFamily` key needs `new FontFamily("…")` (when `ResourceBuilder.Set(string, FontFamily)` exists). The IDE fix and `mur check`'s `→ try:` apply the same rewrite. | | `CS0103` | error | "The name 'X' does not exist in the current context" | Missing `using` — most often `Microsoft.UI.Reactor.Layout` (FlexAlign), `Microsoft.UI.Xaml.Controls` (InfoBarSeverity, Orientation), or `static Microsoft.UI.Reactor.Factories`. (A close-but-wrong factory name instead raises `REACTOR_DYM_003` above.) | | `CS1061` | error | "'X' does not contain a definition for 'Y'" | A type-specific modifier called on an element type that does not declare it — `.Bold()` / `.FontSize()` / `.TextWrapping()` are `TextBlockElement`-only, so `Button("Go").Bold()` fails. This is **not** an ordering problem: modifiers are generic (` where T : Element`) and preserve the concrete type, so `.Margin(16).Bold()` and `.Bold().Margin(16)` compile alike. Check the receiver's element type against `reactor-dsl/references/reactor.api.txt`. | | `CS0117` | error | "'Element' does not contain a definition for X" | You're calling a factory or static member that doesn't exist — confirm the name against `reactor-dsl/references/reactor.api.txt`. (For a missing *instance* modifier see `CS1061` above.) | diff --git a/samples/ReactorHostControlDemo/MainWindow.xaml.cs b/samples/ReactorHostControlDemo/MainWindow.xaml.cs index adc4f8a64..ff74f7143 100644 --- a/samples/ReactorHostControlDemo/MainWindow.xaml.cs +++ b/samples/ReactorHostControlDemo/MainWindow.xaml.cs @@ -47,9 +47,9 @@ public MainWindow() ).Margin(16, 8, 16, 16), items.Count == 0 - ? TextBlock("All done!").Foreground("#888").HAlign(HorizontalAlignment.Center).Margin(16) + ? TextBlock("All done!").Foreground("#888888").HAlign(HorizontalAlignment.Center).Margin(16) : TextBlock($"{items.Count} item{(items.Count == 1 ? "" : "s")} remaining") - .Foreground("#888").HAlign(HorizontalAlignment.Center).Margin(0, 0, 0, 16) + .Foreground("#888888").HAlign(HorizontalAlignment.Center).Margin(0, 0, 0, 16) ); }); TodoPanel.Child = todoHost; diff --git a/samples/apps/wct-controls/Program.cs b/samples/apps/wct-controls/Program.cs index 46179b8ef..2d705bcf9 100644 --- a/samples/apps/wct-controls/Program.cs +++ b/samples/apps/wct-controls/Program.cs @@ -339,7 +339,7 @@ public override Element Render() => private static Element Pane(string label) => Border(TextBlock(label).Center()) - .Background("AliceBlue") + .Background("#F0F8FF") .CornerRadius(8); } @@ -426,7 +426,7 @@ public override Element Render() => VStack(6, TextBlock("1 Microsoft Way"), TextBlock("Redmond, WA 98052"))) - .Background("AliceBlue").CornerRadius(8).Margin(0)) + .Background("#F0F8FF").CornerRadius(8).Margin(0)) .Width(360)); } @@ -484,7 +484,7 @@ public override Element Render() => "A ribbon-style NavigationView whose tabs are TabbedCommandBarItem (a CommandBar) objects in MenuItems — a WinUI control subtree the prop/event wrapper can't express declaratively, so the tabs are built once through the imperative Setters escape hatch (legitimate here: it's controls, not data). The document body is the declarative content slot.", (TabbedCommandBar( content: Border(TextBlock("Document body — pick a ribbon tab above.").Center()) - .Background("AliceBlue").CornerRadius(8).Height(150)) + .Background("#F0F8FF").CornerRadius(8).Height(150)) .Set(BuildTabs)) .Height(260)); diff --git a/src/Reactor.Analyzers/AnalyzerReleases.Unshipped.md b/src/Reactor.Analyzers/AnalyzerReleases.Unshipped.md index e71c30f3a..3a2d39aa4 100644 --- a/src/Reactor.Analyzers/AnalyzerReleases.Unshipped.md +++ b/src/Reactor.Analyzers/AnalyzerReleases.Unshipped.md @@ -64,5 +64,6 @@ REACTOR_DYM_002 | Reactor.DidYouMean | Warning | ThemeBackgroundSuffixAnalyzer - REACTOR_DYM_003 | Reactor.DidYouMean | Warning | FuzzyFactoryNameAnalyzer - Mistyped Reactor factory name in call position (e.g. Buton(...)); did-you-mean the closest factory REACTOR_DYM_004 | Reactor.DidYouMean | Warning | MissingFactoryArgumentAnalyzer - Reactor factory called with too few arguments (CS7036); suggests the full parameter shape (single unique overload only) REACTOR_DYM_005 | Reactor.DidYouMean | Warning | StringForElementArgumentAnalyzer - String passed where a Reactor Element is expected (CS1503); wrap it in a text factory (e.g. TextBlock) +REACTOR_DYM_006 | Reactor.DidYouMean | Warning | ColorStringLiteralAnalyzer - Constant color string BrushHelper cannot parse (renders gray); suggests hex, #RRGGBB expansion, or new FontFamily(...) for a *FontFamily resource key REACTOR_ITEMS_002 | Reactor.Collections | Warning | ItemsViewContainerRootAnalyzer - ItemsView viewBuilder returns a non-ItemContainer root (mount-time InvalidOperationException); wrap it with ItemContainer(...) REACTOR_ICON_001 | Reactor.Windowing | Warning | WindowIconSurfaceAnalyzer - WindowIcon source kind the target surface silently skips (FromResource on tray/overlay/thumbnail, which need an HICON; FromBytes/FromRgba on the window caption or a jump list, which need a path or Uri); only fires when the kind is provably known at the use site diff --git a/src/Reactor.Analyzers/ColorStringClassifier.cs b/src/Reactor.Analyzers/ColorStringClassifier.cs new file mode 100644 index 000000000..881b6d11b --- /dev/null +++ b/src/Reactor.Analyzers/ColorStringClassifier.cs @@ -0,0 +1,333 @@ +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Globalization; + +namespace Microsoft.UI.Reactor.Analyzers; + +/// What is wrong with a color string that BrushHelper cannot parse. +internal enum ColorStringIssue +{ + /// A font name passed to ResourceBuilder.Set under a *FontFamily key. + FontFamilyKey, + /// #RGB / #ARGB shorthand, which BrushHelper does not expand. + HexShorthand, + /// Six or eight hex digits without the leading #. + MissingHash, + /// Starts with # but is not #RRGGBB / #AARRGGBB. + MalformedHex, + /// A standard CSS / WinUI named color that BrushHelper's 8-name table lacks. + NamedColor, + /// Anything else. + Unknown, +} + +/// +/// The did-you-mean for one unparseable color string. is the new string +/// literal value for the literal fixes, the family name for +/// (only when the FontFamily overload exists), or when there is no fix. +/// +internal readonly struct ColorStringFinding +{ + public ColorStringFinding(ColorStringIssue issue, string? replacement, string detail) + { + Issue = issue; + Replacement = replacement; + Detail = detail; + } + + public ColorStringIssue Issue { get; } + public string? Replacement { get; } + public string Detail { get; } +} + +/// +/// Classifies color strings for REACTOR_DYM_006 (). +/// +/// +/// is a netstandard2.0 copy of Microsoft.UI.Reactor.BrushHelper.TryParseColor +/// (the analyzer cannot reference src/Reactor), and the whole class is mirrored +/// byte-for-byte by the mur check CLI's ColorStringClassifier. ColorStringParityTests +/// locks all three together — edit them in lockstep. +/// +internal static class ColorStringClassifier +{ + /// The names BrushHelper's switch recognizes (case-insensitive). + public const string SupportedNames = "red, green, blue, white, black, gray, lightgray, transparent"; + + /// The suffix that marks a WinUI FontFamily resource key (e.g. ContentControlThemeFontFamily). + public const string FontFamilyKeySuffix = "FontFamily"; + + /// + /// Standard CSS / WinUI (Microsoft.UI.Colors) named colors that BrushHelper does not + /// recognize, mapped to their exact #RRGGBB. The 8 names BrushHelper does support are excluded. + /// + public static readonly ImmutableDictionary NamedColorHex = + ImmutableDictionary.CreateRange(StringComparer.OrdinalIgnoreCase, new[] + { + new KeyValuePair("aliceblue", "#F0F8FF"), + new KeyValuePair("antiquewhite", "#FAEBD7"), + new KeyValuePair("aqua", "#00FFFF"), + new KeyValuePair("aquamarine", "#7FFFD4"), + new KeyValuePair("azure", "#F0FFFF"), + new KeyValuePair("beige", "#F5F5DC"), + new KeyValuePair("bisque", "#FFE4C4"), + new KeyValuePair("blanchedalmond", "#FFEBCD"), + new KeyValuePair("blueviolet", "#8A2BE2"), + new KeyValuePair("brown", "#A52A2A"), + new KeyValuePair("burlywood", "#DEB887"), + new KeyValuePair("cadetblue", "#5F9EA0"), + new KeyValuePair("chartreuse", "#7FFF00"), + new KeyValuePair("chocolate", "#D2691E"), + new KeyValuePair("coral", "#FF7F50"), + new KeyValuePair("cornflowerblue", "#6495ED"), + new KeyValuePair("cornsilk", "#FFF8DC"), + new KeyValuePair("crimson", "#DC143C"), + new KeyValuePair("cyan", "#00FFFF"), + new KeyValuePair("darkblue", "#00008B"), + new KeyValuePair("darkcyan", "#008B8B"), + new KeyValuePair("darkgoldenrod", "#B8860B"), + new KeyValuePair("darkgray", "#A9A9A9"), + new KeyValuePair("darkgreen", "#006400"), + new KeyValuePair("darkgrey", "#A9A9A9"), + new KeyValuePair("darkkhaki", "#BDB76B"), + new KeyValuePair("darkmagenta", "#8B008B"), + new KeyValuePair("darkolivegreen", "#556B2F"), + new KeyValuePair("darkorange", "#FF8C00"), + new KeyValuePair("darkorchid", "#9932CC"), + new KeyValuePair("darkred", "#8B0000"), + new KeyValuePair("darksalmon", "#E9967A"), + new KeyValuePair("darkseagreen", "#8FBC8F"), + new KeyValuePair("darkslateblue", "#483D8B"), + new KeyValuePair("darkslategray", "#2F4F4F"), + new KeyValuePair("darkslategrey", "#2F4F4F"), + new KeyValuePair("darkturquoise", "#00CED1"), + new KeyValuePair("darkviolet", "#9400D3"), + new KeyValuePair("deeppink", "#FF1493"), + new KeyValuePair("deepskyblue", "#00BFFF"), + new KeyValuePair("dimgray", "#696969"), + new KeyValuePair("dimgrey", "#696969"), + new KeyValuePair("dodgerblue", "#1E90FF"), + new KeyValuePair("firebrick", "#B22222"), + new KeyValuePair("floralwhite", "#FFFAF0"), + new KeyValuePair("forestgreen", "#228B22"), + new KeyValuePair("fuchsia", "#FF00FF"), + new KeyValuePair("gainsboro", "#DCDCDC"), + new KeyValuePair("ghostwhite", "#F8F8FF"), + new KeyValuePair("gold", "#FFD700"), + new KeyValuePair("goldenrod", "#DAA520"), + new KeyValuePair("greenyellow", "#ADFF2F"), + new KeyValuePair("honeydew", "#F0FFF0"), + new KeyValuePair("hotpink", "#FF69B4"), + new KeyValuePair("indianred", "#CD5C5C"), + new KeyValuePair("indigo", "#4B0082"), + new KeyValuePair("ivory", "#FFFFF0"), + new KeyValuePair("khaki", "#F0E68C"), + new KeyValuePair("lavender", "#E6E6FA"), + new KeyValuePair("lavenderblush", "#FFF0F5"), + new KeyValuePair("lawngreen", "#7CFC00"), + new KeyValuePair("lemonchiffon", "#FFFACD"), + new KeyValuePair("lightblue", "#ADD8E6"), + new KeyValuePair("lightcoral", "#F08080"), + new KeyValuePair("lightcyan", "#E0FFFF"), + new KeyValuePair("lightgoldenrodyellow", "#FAFAD2"), + new KeyValuePair("lightgreen", "#90EE90"), + new KeyValuePair("lightpink", "#FFB6C1"), + new KeyValuePair("lightsalmon", "#FFA07A"), + new KeyValuePair("lightseagreen", "#20B2AA"), + new KeyValuePair("lightskyblue", "#87CEFA"), + new KeyValuePair("lightslategray", "#778899"), + new KeyValuePair("lightslategrey", "#778899"), + new KeyValuePair("lightsteelblue", "#B0C4DE"), + new KeyValuePair("lightyellow", "#FFFFE0"), + new KeyValuePair("lime", "#00FF00"), + new KeyValuePair("limegreen", "#32CD32"), + new KeyValuePair("linen", "#FAF0E6"), + new KeyValuePair("magenta", "#FF00FF"), + new KeyValuePair("maroon", "#800000"), + new KeyValuePair("mediumaquamarine", "#66CDAA"), + new KeyValuePair("mediumblue", "#0000CD"), + new KeyValuePair("mediumorchid", "#BA55D3"), + new KeyValuePair("mediumpurple", "#9370DB"), + new KeyValuePair("mediumseagreen", "#3CB371"), + new KeyValuePair("mediumslateblue", "#7B68EE"), + new KeyValuePair("mediumspringgreen", "#00FA9A"), + new KeyValuePair("mediumturquoise", "#48D1CC"), + new KeyValuePair("mediumvioletred", "#C71585"), + new KeyValuePair("midnightblue", "#191970"), + new KeyValuePair("mintcream", "#F5FFFA"), + new KeyValuePair("mistyrose", "#FFE4E1"), + new KeyValuePair("moccasin", "#FFE4B5"), + new KeyValuePair("navajowhite", "#FFDEAD"), + new KeyValuePair("navy", "#000080"), + new KeyValuePair("oldlace", "#FDF5E6"), + new KeyValuePair("olive", "#808000"), + new KeyValuePair("olivedrab", "#6B8E23"), + new KeyValuePair("orange", "#FFA500"), + new KeyValuePair("orangered", "#FF4500"), + new KeyValuePair("orchid", "#DA70D6"), + new KeyValuePair("palegoldenrod", "#EEE8AA"), + new KeyValuePair("palegreen", "#98FB98"), + new KeyValuePair("paleturquoise", "#AFEEEE"), + new KeyValuePair("palevioletred", "#DB7093"), + new KeyValuePair("papayawhip", "#FFEFD5"), + new KeyValuePair("peachpuff", "#FFDAB9"), + new KeyValuePair("peru", "#CD853F"), + new KeyValuePair("pink", "#FFC0CB"), + new KeyValuePair("plum", "#DDA0DD"), + new KeyValuePair("powderblue", "#B0E0E6"), + new KeyValuePair("purple", "#800080"), + new KeyValuePair("rebeccapurple", "#663399"), + new KeyValuePair("rosybrown", "#BC8F8F"), + new KeyValuePair("royalblue", "#4169E1"), + new KeyValuePair("saddlebrown", "#8B4513"), + new KeyValuePair("salmon", "#FA8072"), + new KeyValuePair("sandybrown", "#F4A460"), + new KeyValuePair("seagreen", "#2E8B57"), + new KeyValuePair("seashell", "#FFF5EE"), + new KeyValuePair("sienna", "#A0522D"), + new KeyValuePair("silver", "#C0C0C0"), + new KeyValuePair("skyblue", "#87CEEB"), + new KeyValuePair("slateblue", "#6A5ACD"), + new KeyValuePair("slategray", "#708090"), + new KeyValuePair("slategrey", "#708090"), + new KeyValuePair("snow", "#FFFAFA"), + new KeyValuePair("springgreen", "#00FF7F"), + new KeyValuePair("steelblue", "#4682B4"), + new KeyValuePair("tan", "#D2B48C"), + new KeyValuePair("teal", "#008080"), + new KeyValuePair("thistle", "#D8BFD8"), + new KeyValuePair("tomato", "#FF6347"), + new KeyValuePair("turquoise", "#40E0D0"), + new KeyValuePair("violet", "#EE82EE"), + new KeyValuePair("wheat", "#F5DEB3"), + new KeyValuePair("whitesmoke", "#F5F5F5"), + new KeyValuePair("yellow", "#FFFF00"), + new KeyValuePair("yellowgreen", "#9ACD32"), + }); + + /// + /// Mirrors BrushHelper.TryParseColor: iff the string does not fall + /// back to gray. + /// + public static bool TryParse(string color) + { + switch (color.ToLowerInvariant()) + { + case "red": + case "green": + case "blue": + case "white": + case "black": + case "gray": + case "grey": + case "lightgray": + case "lightgrey": + case "transparent": + return true; + } + return color.StartsWith("#", StringComparison.Ordinal) && TryParseHex(color); + } + + private static bool TryParseHex(string hex) + { + hex = hex.TrimStart('#'); + if (hex.Length == 6) + return IsByte(hex, 0) && IsByte(hex, 2) && IsByte(hex, 4); + if (hex.Length == 8) + return IsByte(hex, 0) && IsByte(hex, 2) && IsByte(hex, 4) && IsByte(hex, 6); + return false; + } + + // Same call as BrushHelper (`byte.TryParse(hex[i..i+2], NumberStyles.HexNumber, null, out _)`), + // including HexNumber's leading/trailing-whitespace allowance. + private static bool IsByte(string hex, int start) => + byte.TryParse(hex.Substring(start, 2), NumberStyles.HexNumber, null, out _); + + /// + /// Returns the finding for , or when BrushHelper + /// parses it. is the constant key of a ResourceBuilder.Set + /// call (otherwise ); says whether + /// ResourceBuilder.Set(string, FontFamily) is available to rewrite to. + /// + public static ColorStringFinding? Classify(string value, string? resourceKey, bool fontFamilyOverloadExists) + { + if (TryParse(value)) + return null; + + if (resourceKey is not null && resourceKey.EndsWith(FontFamilyKeySuffix, StringComparison.Ordinal)) + { + return fontFamilyOverloadExists + ? new ColorStringFinding( + ColorStringIssue.FontFamilyKey, + value, + $"the key '{resourceKey}' names a FontFamily resource; did you mean new FontFamily(\"{value}\")?") + : new ColorStringFinding( + ColorStringIssue.FontFamilyKey, + null, + $"the key '{resourceKey}' names a FontFamily resource, but Set(string, string) only stores color brushes"); + } + + if (value.StartsWith("#", StringComparison.Ordinal)) + { + var digits = value.Substring(1); + if ((digits.Length == 3 || digits.Length == 4) && IsAllHex(digits)) + { + var expanded = Expand(digits); + return new ColorStringFinding( + ColorStringIssue.HexShorthand, + expanded, + $"BrushHelper has no #RGB/#ARGB shorthand; did you mean \"{expanded}\"?"); + } + return new ColorStringFinding( + ColorStringIssue.MalformedHex, + null, + "hex colors must be #RRGGBB or #AARRGGBB"); + } + + if ((value.Length == 6 || value.Length == 8) && IsAllHex(value)) + { + var hashed = "#" + value; + return new ColorStringFinding( + ColorStringIssue.MissingHash, + hashed, + $"hex colors need a leading '#'; did you mean \"{hashed}\"?"); + } + + if (NamedColorHex.TryGetValue(value, out var hex)) + { + return new ColorStringFinding( + ColorStringIssue.NamedColor, + hex, + $"BrushHelper only knows {SupportedNames}; did you mean \"{hex}\"?"); + } + + return new ColorStringFinding( + ColorStringIssue.Unknown, + null, + $"use #RRGGBB or #AARRGGBB, a Theme token, or one of {SupportedNames}"); + } + + private static bool IsAllHex(string s) + { + foreach (var c in s) + { + var isHex = (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F'); + if (!isHex) + return false; + } + return true; + } + + private static string Expand(string digits) + { + var chars = new char[1 + digits.Length * 2]; + chars[0] = '#'; + for (var i = 0; i < digits.Length; i++) + { + chars[1 + i * 2] = digits[i]; + chars[2 + i * 2] = digits[i]; + } + return new string(chars); + } +} diff --git a/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs b/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs new file mode 100644 index 000000000..3218f9673 --- /dev/null +++ b/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs @@ -0,0 +1,267 @@ +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using System.Threading; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; + +namespace Microsoft.UI.Reactor.Analyzers; + +/// +/// REACTOR_DYM_006 — a compile-time-constant color string that BrushHelper cannot parse, so +/// it silently renders gray (e.g. .Background("orange"), .Foreground("#888"), or a font +/// name stored through ResourceBuilder.Set(key, "Consolas")). Paired with +/// . +/// +/// +/// +/// Covers every Reactor API whose color string goes through BrushHelper.Parse: +/// BrushHelper.Parse itself, ResourceBuilder.Set(string, string), and the +/// string overloads of the Background / Foreground / BorderBrush / +/// WithBorder modifiers on ElementExtensions. Targets are resolved once per +/// compilation and matched by symbol identity, so a look-alike API in another namespace never fires. +/// +/// +/// Unlike REACTOR_DYM_001–005 this does not ride a compiler error — the code compiles — but +/// the result is always a wrong (gray) brush, so it is a Warning. Only constant values are checked +/// (literals, consts, constant concatenation, and each constant branch of a ?: / +/// ?? / switch expression); runtime strings are out of reach. The mur check +/// ColorStringLiteralRule anchors on this diagnostic's ID and repeats the did-you-mean. +/// +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class ColorStringLiteralAnalyzer : DiagnosticAnalyzer +{ + public const string DiagnosticId = "REACTOR_DYM_006"; + + public const string BrushHelperMetadataName = "Microsoft.UI.Reactor.BrushHelper"; + public const string ResourceBuilderMetadataName = "Microsoft.UI.Reactor.Elements.ResourceBuilder"; + public const string ElementExtensionsMetadataName = "Microsoft.UI.Reactor.ElementExtensions"; + public const string FontFamilyMetadataName = "Microsoft.UI.Xaml.Media.FontFamily"; + + /// The modifier names whose string color overloads parse through BrushHelper. + public static readonly ImmutableHashSet ModifierNames = + ImmutableHashSet.Create(StringComparer.Ordinal, "Background", "Foreground", "BorderBrush", "WithBorder"); + + /// The parameter name every covered API uses for its color string. + public const string ColorParameterName = "color"; + + /// The resource-key parameter name of ResourceBuilder.Set. + public const string KeyParameterName = "key"; + + /// Property-bag key: the name. + public const string IssueProperty = "Issue"; + + /// Property-bag key: the replacement value (literal text, or family name). Absent when there is no fix. + public const string ReplacementProperty = "Replacement"; + + private static readonly LocalizableString Title = + "Color string that BrushHelper cannot parse renders gray"; + private static readonly LocalizableString MessageFormat = + "'{0}' is not a color BrushHelper can parse, so it renders gray; {1}"; + private static readonly LocalizableString Description = + "BrushHelper.Parse understands only red, green, blue, white, black, gray/grey, lightgray/lightgrey, transparent, #RRGGBB and #AARRGGBB. Anything else silently falls back to a gray brush. This covers BrushHelper.Parse, ResourceBuilder.Set(string, string), and the string overloads of the Background/Foreground/BorderBrush/WithBorder modifiers."; + + private static readonly DiagnosticDescriptor Rule = new( + DiagnosticId, + Title, + MessageFormat, + "Reactor.DidYouMean", + DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: Description); + + public override ImmutableArray SupportedDiagnostics => + ImmutableArray.Create(Rule); + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + + context.RegisterCompilationStartAction(start => + { + var targets = Targets.Resolve(start.Compilation); + if (targets is null) + return; // No Reactor color-string API referenced. + + start.RegisterSyntaxNodeAction( + ctx => AnalyzeInvocation(ctx, targets), + SyntaxKind.InvocationExpression); + }); + } + + /// The resolved color-string APIs for one compilation. + internal sealed class Targets + { + private Targets(ImmutableHashSet methods, IMethodSymbol? resourceSet, bool fontFamilyOverloadExists) + { + Methods = methods; + ResourceSet = resourceSet; + FontFamilyOverloadExists = fontFamilyOverloadExists; + } + + public ImmutableHashSet Methods { get; } + public IMethodSymbol? ResourceSet { get; } + public bool FontFamilyOverloadExists { get; } + + public static Targets? Resolve(Compilation compilation) + { + var methods = ImmutableHashSet.CreateBuilder(SymbolEqualityComparer.Default); + var str = compilation.GetSpecialType(SpecialType.System_String); + + var brushHelper = compilation.GetTypeByMetadataName(BrushHelperMetadataName); + if (brushHelper is not null) + { + foreach (var m in brushHelper.GetMembers("Parse").OfType()) + if (HasStringColorParameter(m, str)) + methods.Add(m); + } + + IMethodSymbol? resourceSet = null; + var fontFamilyOverload = false; + var resourceBuilder = compilation.GetTypeByMetadataName(ResourceBuilderMetadataName); + if (resourceBuilder is not null) + { + foreach (var m in resourceBuilder.GetMembers("Set").OfType()) + { + if (m.Parameters.Length != 2) + continue; + if (HasStringColorParameter(m, str) && SymbolEqualityComparer.Default.Equals(m.Parameters[0].Type, str)) + { + resourceSet = m; + methods.Add(m); + } + else if (m.Parameters[1].Type.ToDisplayString() == FontFamilyMetadataName) + { + fontFamilyOverload = true; + } + } + } + + var extensions = compilation.GetTypeByMetadataName(ElementExtensionsMetadataName); + if (extensions is not null) + { + foreach (var name in ModifierNames) + foreach (var m in extensions.GetMembers(name).OfType()) + if (m.IsExtensionMethod && HasStringColorParameter(m, str)) + methods.Add(m); + } + + return methods.Count == 0 ? null : new Targets(methods.ToImmutable(), resourceSet, fontFamilyOverload); + } + + private static bool HasStringColorParameter(IMethodSymbol method, ITypeSymbol str) + { + foreach (var p in method.Parameters) + if (p.Name == ColorParameterName && SymbolEqualityComparer.Default.Equals(p.Type, str)) + return true; + return false; + } + } + + private static void AnalyzeInvocation(SyntaxNodeAnalysisContext context, Targets targets) + { + var invocation = (InvocationExpressionSyntax)context.Node; + + // Cheap syntactic pre-filter before any binding. + var calledName = invocation.Expression switch + { + MemberAccessExpressionSyntax m => m.Name.Identifier.ValueText, + IdentifierNameSyntax id => id.Identifier.ValueText, + GenericNameSyntax g => g.Identifier.ValueText, + MemberBindingExpressionSyntax b => b.Name.Identifier.ValueText, + _ => null, + }; + if (calledName is null || !(calledName == "Parse" || calledName == "Set" || ModifierNames.Contains(calledName))) + return; + // The ubiquitous `.Set(c => …)` setter modifier has one argument; ResourceBuilder.Set has two. + if (calledName == "Set" && invocation.ArgumentList.Arguments.Count != 2) + return; + + if (context.SemanticModel.GetOperation(invocation, context.CancellationToken) is not IInvocationOperation operation) + return; + + var target = operation.TargetMethod; + var method = (target.ReducedFrom ?? target).OriginalDefinition; + if (!targets.Methods.Contains(method)) + return; + + IArgumentOperation? colorArg = null; + IArgumentOperation? keyArg = null; + foreach (var arg in operation.Arguments) + { + if (arg.Parameter is null) + continue; + if (arg.Parameter.Name == ColorParameterName) + colorArg = arg; + else if (arg.Parameter.Name == KeyParameterName) + keyArg = arg; + } + if (colorArg is null || colorArg.ArgumentKind != ArgumentKind.Explicit) + return; + + string? resourceKey = null; + if (targets.ResourceSet is not null + && SymbolEqualityComparer.Default.Equals(method, targets.ResourceSet) + && keyArg?.Value.ConstantValue is { HasValue: true, Value: string key }) + { + resourceKey = key; + } + + foreach (var (expression, value) in ConstantStrings(colorArg.Value.Syntax as ExpressionSyntax, context.SemanticModel, context.CancellationToken)) + { + var finding = ColorStringClassifier.Classify(value, resourceKey, targets.FontFamilyOverloadExists); + if (finding is not { } f) + continue; + + var properties = ImmutableDictionary.CreateBuilder(); + properties.Add(IssueProperty, f.Issue.ToString()); + if (f.Replacement is not null) + properties.Add(ReplacementProperty, f.Replacement); + + context.ReportDiagnostic(Diagnostic.Create( + Rule, + expression.GetLocation(), + properties.ToImmutable(), + value, + f.Detail)); + } + } + + /// + /// The constant strings an argument can evaluate to, each with the expression that produced it. + /// A wholly-constant expression yields itself; otherwise only value-selecting shapes are + /// walked (parentheses, ?:, ??, switch arms) — never into nested calls. + /// + internal static IEnumerable<(ExpressionSyntax Expression, string Value)> ConstantStrings( + ExpressionSyntax? expression, SemanticModel model, CancellationToken cancellationToken) + { + if (expression is null) + yield break; + + var constant = model.GetConstantValue(expression, cancellationToken); + if (constant.HasValue) + { + if (constant.Value is string s) + yield return (expression, s); + yield break; + } + + IEnumerable children = expression switch + { + ParenthesizedExpressionSyntax p => new[] { p.Expression }, + ConditionalExpressionSyntax c => new[] { c.WhenTrue, c.WhenFalse }, + BinaryExpressionSyntax b when b.IsKind(SyntaxKind.CoalesceExpression) => new[] { b.Left, b.Right }, + SwitchExpressionSyntax sw => sw.Arms.Select(a => a.Expression), + _ => Array.Empty(), + }; + foreach (var child in children) + foreach (var item in ConstantStrings(child, model, cancellationToken)) + yield return item; + } +} diff --git a/src/Reactor.Analyzers/ColorStringLiteralCodeFix.cs b/src/Reactor.Analyzers/ColorStringLiteralCodeFix.cs new file mode 100644 index 000000000..be749e30f --- /dev/null +++ b/src/Reactor.Analyzers/ColorStringLiteralCodeFix.cs @@ -0,0 +1,92 @@ +using System.Collections.Immutable; +using System.Composition; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Simplification; + +namespace Microsoft.UI.Reactor.Analyzers; + +/// +/// Code fix for (REACTOR_DYM_006). Rewrites a string literal +/// to the suggested color ("#888" → "#888888", "FF0000" → "#FF0000", +/// "orange" → "#FFA500"), or — for a font name stored under a *FontFamily +/// resource key — rewrites the argument to new FontFamily("…") so it binds to the +/// ResourceBuilder.Set(string, FontFamily) overload. The replacement travels in the +/// diagnostic's property bag; no fix is offered when the analyzer attached none. The literal fixes +/// need a plain string literal (a flagged const reference is fixed at its declaration instead). +/// +[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(ColorStringLiteralCodeFix))] +[Shared] +public sealed class ColorStringLiteralCodeFix : CodeFixProvider +{ + private const string LiteralEquivalenceKey = "Reactor_ColorStringLiteral"; + private const string FontFamilyEquivalenceKey = "Reactor_ColorStringFontFamily"; + + public override ImmutableArray FixableDiagnosticIds => + ImmutableArray.Create(ColorStringLiteralAnalyzer.DiagnosticId); + + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + public override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); + if (root is null) return; + + foreach (var diagnostic in context.Diagnostics) + { + if (!diagnostic.Properties.TryGetValue(ColorStringLiteralAnalyzer.ReplacementProperty, out var replacement) + || replacement is null + || !diagnostic.Properties.TryGetValue(ColorStringLiteralAnalyzer.IssueProperty, out var issue)) + continue; + + var node = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true); + if (node is ArgumentSyntax argument) + node = argument.Expression; + if (node is not ExpressionSyntax expression) + continue; + + if (issue == nameof(ColorStringIssue.FontFamilyKey)) + { + // Only when the flagged value *is* the argument — inside a ?: branch the other branch + // would still be a string, and the expression would no longer type-check. A const + // reference is fine to wrap as-is. + if (expression.Parent is not ArgumentSyntax) + continue; + + var creation = SyntaxFactory.ObjectCreationExpression( + SyntaxFactory.ParseTypeName("global::" + ColorStringLiteralAnalyzer.FontFamilyMetadataName) + .WithAdditionalAnnotations(Simplifier.Annotation)) + .WithArgumentList(SyntaxFactory.ArgumentList(SyntaxFactory.SingletonSeparatedList( + SyntaxFactory.Argument(expression.WithoutTrivia())))) + .WithTriviaFrom(expression); + + context.RegisterCodeFix( + CodeAction.Create( + $"Use new FontFamily(\"{replacement}\")", + _ => Task.FromResult(context.Document.WithSyntaxRoot(root.ReplaceNode(expression, creation))), + equivalenceKey: FontFamilyEquivalenceKey), + diagnostic); + continue; + } + + if (expression is not LiteralExpressionSyntax literal || !literal.IsKind(SyntaxKind.StringLiteralExpression)) + continue; + + var newLiteral = SyntaxFactory.LiteralExpression( + SyntaxKind.StringLiteralExpression, + SyntaxFactory.Literal(replacement!)) + .WithTriviaFrom(literal); + + context.RegisterCodeFix( + CodeAction.Create( + $"Change to \"{replacement}\"", + _ => Task.FromResult(context.Document.WithSyntaxRoot(root.ReplaceNode(literal, newLiteral))), + equivalenceKey: LiteralEquivalenceKey), + diagnostic); + } + } +} diff --git a/src/Reactor.Cli/Check/Rules/ColorStringClassifier.cs b/src/Reactor.Cli/Check/Rules/ColorStringClassifier.cs new file mode 100644 index 000000000..0054f55ca --- /dev/null +++ b/src/Reactor.Cli/Check/Rules/ColorStringClassifier.cs @@ -0,0 +1,335 @@ +// Mirror of src/Reactor.Analyzers/ColorStringClassifier.cs — see the remarks below. + +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Globalization; + +namespace Microsoft.UI.Reactor.Cli.Check.Rules; + +/// What is wrong with a color string that BrushHelper cannot parse. +internal enum ColorStringIssue +{ + /// A font name passed to ResourceBuilder.Set under a *FontFamily key. + FontFamilyKey, + /// #RGB / #ARGB shorthand, which BrushHelper does not expand. + HexShorthand, + /// Six or eight hex digits without the leading #. + MissingHash, + /// Starts with # but is not #RRGGBB / #AARRGGBB. + MalformedHex, + /// A standard CSS / WinUI named color that BrushHelper's 8-name table lacks. + NamedColor, + /// Anything else. + Unknown, +} + +/// +/// The did-you-mean for one unparseable color string. is the new string +/// literal value for the literal fixes, the family name for +/// (only when the FontFamily overload exists), or when there is no fix. +/// +internal readonly struct ColorStringFinding +{ + public ColorStringFinding(ColorStringIssue issue, string? replacement, string detail) + { + Issue = issue; + Replacement = replacement; + Detail = detail; + } + + public ColorStringIssue Issue { get; } + public string? Replacement { get; } + public string Detail { get; } +} + +/// +/// Classifies color strings for the mur check (mirror of REACTOR_DYM_006). +/// +/// +/// A copy of the REACTOR_DYM_006 analyzer's Microsoft.UI.Reactor.Analyzers.ColorStringClassifier +/// (the netstandard2.0 analyzer and this CLI cannot share a project), whose in turn +/// mirrors Microsoft.UI.Reactor.BrushHelper.TryParseColor. ColorStringParityTests locks all +/// three together — edit them in lockstep. +/// +internal static class ColorStringClassifier +{ + /// The names BrushHelper's switch recognizes (case-insensitive). + public const string SupportedNames = "red, green, blue, white, black, gray, lightgray, transparent"; + + /// The suffix that marks a WinUI FontFamily resource key (e.g. ContentControlThemeFontFamily). + public const string FontFamilyKeySuffix = "FontFamily"; + + /// + /// Standard CSS / WinUI (Microsoft.UI.Colors) named colors that BrushHelper does not + /// recognize, mapped to their exact #RRGGBB. The 8 names BrushHelper does support are excluded. + /// + public static readonly ImmutableDictionary NamedColorHex = + ImmutableDictionary.CreateRange(StringComparer.OrdinalIgnoreCase, new[] + { + new KeyValuePair("aliceblue", "#F0F8FF"), + new KeyValuePair("antiquewhite", "#FAEBD7"), + new KeyValuePair("aqua", "#00FFFF"), + new KeyValuePair("aquamarine", "#7FFFD4"), + new KeyValuePair("azure", "#F0FFFF"), + new KeyValuePair("beige", "#F5F5DC"), + new KeyValuePair("bisque", "#FFE4C4"), + new KeyValuePair("blanchedalmond", "#FFEBCD"), + new KeyValuePair("blueviolet", "#8A2BE2"), + new KeyValuePair("brown", "#A52A2A"), + new KeyValuePair("burlywood", "#DEB887"), + new KeyValuePair("cadetblue", "#5F9EA0"), + new KeyValuePair("chartreuse", "#7FFF00"), + new KeyValuePair("chocolate", "#D2691E"), + new KeyValuePair("coral", "#FF7F50"), + new KeyValuePair("cornflowerblue", "#6495ED"), + new KeyValuePair("cornsilk", "#FFF8DC"), + new KeyValuePair("crimson", "#DC143C"), + new KeyValuePair("cyan", "#00FFFF"), + new KeyValuePair("darkblue", "#00008B"), + new KeyValuePair("darkcyan", "#008B8B"), + new KeyValuePair("darkgoldenrod", "#B8860B"), + new KeyValuePair("darkgray", "#A9A9A9"), + new KeyValuePair("darkgreen", "#006400"), + new KeyValuePair("darkgrey", "#A9A9A9"), + new KeyValuePair("darkkhaki", "#BDB76B"), + new KeyValuePair("darkmagenta", "#8B008B"), + new KeyValuePair("darkolivegreen", "#556B2F"), + new KeyValuePair("darkorange", "#FF8C00"), + new KeyValuePair("darkorchid", "#9932CC"), + new KeyValuePair("darkred", "#8B0000"), + new KeyValuePair("darksalmon", "#E9967A"), + new KeyValuePair("darkseagreen", "#8FBC8F"), + new KeyValuePair("darkslateblue", "#483D8B"), + new KeyValuePair("darkslategray", "#2F4F4F"), + new KeyValuePair("darkslategrey", "#2F4F4F"), + new KeyValuePair("darkturquoise", "#00CED1"), + new KeyValuePair("darkviolet", "#9400D3"), + new KeyValuePair("deeppink", "#FF1493"), + new KeyValuePair("deepskyblue", "#00BFFF"), + new KeyValuePair("dimgray", "#696969"), + new KeyValuePair("dimgrey", "#696969"), + new KeyValuePair("dodgerblue", "#1E90FF"), + new KeyValuePair("firebrick", "#B22222"), + new KeyValuePair("floralwhite", "#FFFAF0"), + new KeyValuePair("forestgreen", "#228B22"), + new KeyValuePair("fuchsia", "#FF00FF"), + new KeyValuePair("gainsboro", "#DCDCDC"), + new KeyValuePair("ghostwhite", "#F8F8FF"), + new KeyValuePair("gold", "#FFD700"), + new KeyValuePair("goldenrod", "#DAA520"), + new KeyValuePair("greenyellow", "#ADFF2F"), + new KeyValuePair("honeydew", "#F0FFF0"), + new KeyValuePair("hotpink", "#FF69B4"), + new KeyValuePair("indianred", "#CD5C5C"), + new KeyValuePair("indigo", "#4B0082"), + new KeyValuePair("ivory", "#FFFFF0"), + new KeyValuePair("khaki", "#F0E68C"), + new KeyValuePair("lavender", "#E6E6FA"), + new KeyValuePair("lavenderblush", "#FFF0F5"), + new KeyValuePair("lawngreen", "#7CFC00"), + new KeyValuePair("lemonchiffon", "#FFFACD"), + new KeyValuePair("lightblue", "#ADD8E6"), + new KeyValuePair("lightcoral", "#F08080"), + new KeyValuePair("lightcyan", "#E0FFFF"), + new KeyValuePair("lightgoldenrodyellow", "#FAFAD2"), + new KeyValuePair("lightgreen", "#90EE90"), + new KeyValuePair("lightpink", "#FFB6C1"), + new KeyValuePair("lightsalmon", "#FFA07A"), + new KeyValuePair("lightseagreen", "#20B2AA"), + new KeyValuePair("lightskyblue", "#87CEFA"), + new KeyValuePair("lightslategray", "#778899"), + new KeyValuePair("lightslategrey", "#778899"), + new KeyValuePair("lightsteelblue", "#B0C4DE"), + new KeyValuePair("lightyellow", "#FFFFE0"), + new KeyValuePair("lime", "#00FF00"), + new KeyValuePair("limegreen", "#32CD32"), + new KeyValuePair("linen", "#FAF0E6"), + new KeyValuePair("magenta", "#FF00FF"), + new KeyValuePair("maroon", "#800000"), + new KeyValuePair("mediumaquamarine", "#66CDAA"), + new KeyValuePair("mediumblue", "#0000CD"), + new KeyValuePair("mediumorchid", "#BA55D3"), + new KeyValuePair("mediumpurple", "#9370DB"), + new KeyValuePair("mediumseagreen", "#3CB371"), + new KeyValuePair("mediumslateblue", "#7B68EE"), + new KeyValuePair("mediumspringgreen", "#00FA9A"), + new KeyValuePair("mediumturquoise", "#48D1CC"), + new KeyValuePair("mediumvioletred", "#C71585"), + new KeyValuePair("midnightblue", "#191970"), + new KeyValuePair("mintcream", "#F5FFFA"), + new KeyValuePair("mistyrose", "#FFE4E1"), + new KeyValuePair("moccasin", "#FFE4B5"), + new KeyValuePair("navajowhite", "#FFDEAD"), + new KeyValuePair("navy", "#000080"), + new KeyValuePair("oldlace", "#FDF5E6"), + new KeyValuePair("olive", "#808000"), + new KeyValuePair("olivedrab", "#6B8E23"), + new KeyValuePair("orange", "#FFA500"), + new KeyValuePair("orangered", "#FF4500"), + new KeyValuePair("orchid", "#DA70D6"), + new KeyValuePair("palegoldenrod", "#EEE8AA"), + new KeyValuePair("palegreen", "#98FB98"), + new KeyValuePair("paleturquoise", "#AFEEEE"), + new KeyValuePair("palevioletred", "#DB7093"), + new KeyValuePair("papayawhip", "#FFEFD5"), + new KeyValuePair("peachpuff", "#FFDAB9"), + new KeyValuePair("peru", "#CD853F"), + new KeyValuePair("pink", "#FFC0CB"), + new KeyValuePair("plum", "#DDA0DD"), + new KeyValuePair("powderblue", "#B0E0E6"), + new KeyValuePair("purple", "#800080"), + new KeyValuePair("rebeccapurple", "#663399"), + new KeyValuePair("rosybrown", "#BC8F8F"), + new KeyValuePair("royalblue", "#4169E1"), + new KeyValuePair("saddlebrown", "#8B4513"), + new KeyValuePair("salmon", "#FA8072"), + new KeyValuePair("sandybrown", "#F4A460"), + new KeyValuePair("seagreen", "#2E8B57"), + new KeyValuePair("seashell", "#FFF5EE"), + new KeyValuePair("sienna", "#A0522D"), + new KeyValuePair("silver", "#C0C0C0"), + new KeyValuePair("skyblue", "#87CEEB"), + new KeyValuePair("slateblue", "#6A5ACD"), + new KeyValuePair("slategray", "#708090"), + new KeyValuePair("slategrey", "#708090"), + new KeyValuePair("snow", "#FFFAFA"), + new KeyValuePair("springgreen", "#00FF7F"), + new KeyValuePair("steelblue", "#4682B4"), + new KeyValuePair("tan", "#D2B48C"), + new KeyValuePair("teal", "#008080"), + new KeyValuePair("thistle", "#D8BFD8"), + new KeyValuePair("tomato", "#FF6347"), + new KeyValuePair("turquoise", "#40E0D0"), + new KeyValuePair("violet", "#EE82EE"), + new KeyValuePair("wheat", "#F5DEB3"), + new KeyValuePair("whitesmoke", "#F5F5F5"), + new KeyValuePair("yellow", "#FFFF00"), + new KeyValuePair("yellowgreen", "#9ACD32"), + }); + + /// + /// Mirrors BrushHelper.TryParseColor: iff the string does not fall + /// back to gray. + /// + public static bool TryParse(string color) + { + switch (color.ToLowerInvariant()) + { + case "red": + case "green": + case "blue": + case "white": + case "black": + case "gray": + case "grey": + case "lightgray": + case "lightgrey": + case "transparent": + return true; + } + return color.StartsWith("#", StringComparison.Ordinal) && TryParseHex(color); + } + + private static bool TryParseHex(string hex) + { + hex = hex.TrimStart('#'); + if (hex.Length == 6) + return IsByte(hex, 0) && IsByte(hex, 2) && IsByte(hex, 4); + if (hex.Length == 8) + return IsByte(hex, 0) && IsByte(hex, 2) && IsByte(hex, 4) && IsByte(hex, 6); + return false; + } + + // Same call as BrushHelper (`byte.TryParse(hex[i..i+2], NumberStyles.HexNumber, null, out _)`), + // including HexNumber's leading/trailing-whitespace allowance. + private static bool IsByte(string hex, int start) => + byte.TryParse(hex.Substring(start, 2), NumberStyles.HexNumber, null, out _); + + /// + /// Returns the finding for , or when BrushHelper + /// parses it. is the constant key of a ResourceBuilder.Set + /// call (otherwise ); says whether + /// ResourceBuilder.Set(string, FontFamily) is available to rewrite to. + /// + public static ColorStringFinding? Classify(string value, string? resourceKey, bool fontFamilyOverloadExists) + { + if (TryParse(value)) + return null; + + if (resourceKey is not null && resourceKey.EndsWith(FontFamilyKeySuffix, StringComparison.Ordinal)) + { + return fontFamilyOverloadExists + ? new ColorStringFinding( + ColorStringIssue.FontFamilyKey, + value, + $"the key '{resourceKey}' names a FontFamily resource; did you mean new FontFamily(\"{value}\")?") + : new ColorStringFinding( + ColorStringIssue.FontFamilyKey, + null, + $"the key '{resourceKey}' names a FontFamily resource, but Set(string, string) only stores color brushes"); + } + + if (value.StartsWith("#", StringComparison.Ordinal)) + { + var digits = value.Substring(1); + if ((digits.Length == 3 || digits.Length == 4) && IsAllHex(digits)) + { + var expanded = Expand(digits); + return new ColorStringFinding( + ColorStringIssue.HexShorthand, + expanded, + $"BrushHelper has no #RGB/#ARGB shorthand; did you mean \"{expanded}\"?"); + } + return new ColorStringFinding( + ColorStringIssue.MalformedHex, + null, + "hex colors must be #RRGGBB or #AARRGGBB"); + } + + if ((value.Length == 6 || value.Length == 8) && IsAllHex(value)) + { + var hashed = "#" + value; + return new ColorStringFinding( + ColorStringIssue.MissingHash, + hashed, + $"hex colors need a leading '#'; did you mean \"{hashed}\"?"); + } + + if (NamedColorHex.TryGetValue(value, out var hex)) + { + return new ColorStringFinding( + ColorStringIssue.NamedColor, + hex, + $"BrushHelper only knows {SupportedNames}; did you mean \"{hex}\"?"); + } + + return new ColorStringFinding( + ColorStringIssue.Unknown, + null, + $"use #RRGGBB or #AARRGGBB, a Theme token, or one of {SupportedNames}"); + } + + private static bool IsAllHex(string s) + { + foreach (var c in s) + { + var isHex = (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F'); + if (!isHex) + return false; + } + return true; + } + + private static string Expand(string digits) + { + var chars = new char[1 + digits.Length * 2]; + chars[0] = '#'; + for (var i = 0; i < digits.Length; i++) + { + chars[1 + i * 2] = digits[i]; + chars[2 + i * 2] = digits[i]; + } + return new string(chars); + } +} diff --git a/src/Reactor.Cli/Check/Rules/ColorStringLiteralRule.cs b/src/Reactor.Cli/Check/Rules/ColorStringLiteralRule.cs new file mode 100644 index 000000000..bde204ef1 --- /dev/null +++ b/src/Reactor.Cli/Check/Rules/ColorStringLiteralRule.cs @@ -0,0 +1,180 @@ +// Mirror of the REACTOR_DYM_006 analyzer (ColorStringLiteralAnalyzer) — issue #1375. +// +// A compile-time-constant color string that BrushHelper can't parse compiles +// fine and silently renders gray, so unlike every other Tier-3 rule there is +// no compiler error to hang this on. The rule therefore anchors on the +// analyzer's own diagnostic ID: `dotnet build` (which `mur check` wraps) runs +// the shipped Reactor.Analyzers bundle, REACTOR_DYM_006 lands in the parsed +// output, and this rule re-derives the concrete rewrite for the `→ try:` +// suffix. It deliberately has NO Tier-1 HintFor entry in CheckCommand — that +// table wins ties and would mask the suggestion. +// +// Parity with the analyzer is locked by DidYouMeanAnalyzerParityTests (same +// shapes → same suggestion) and ColorStringParityTests (the two classifier +// copies agree with each other and with BrushHelper.TryParseColor). + +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Operations; + +namespace Microsoft.UI.Reactor.Cli.Check.Rules; + +internal sealed class ColorStringLiteralRule : IRulePattern +{ + internal const string DiagnosticId = "REACTOR_DYM_006"; + const string BrushHelperFqn = "Microsoft.UI.Reactor.BrushHelper"; + const string ResourceBuilderFqn = "Microsoft.UI.Reactor.Elements.ResourceBuilder"; + const string ElementExtensionsFqn = "Microsoft.UI.Reactor.ElementExtensions"; + const string FontFamilyFqn = "Microsoft.UI.Xaml.Media.FontFamily"; + const string ColorParameterName = "color"; + const string KeyParameterName = "key"; + + static readonly HashSet ModifierNames = new(StringComparer.Ordinal) + { + "Background", "Foreground", "BorderBrush", "WithBorder", + }; + + public string Name => "ColorStringLiteralRule"; + public string Provenance => "analyzer:REACTOR_DYM_006"; + public IReadOnlyList DiagnosticCodes { get; } = new[] { DiagnosticId }; + public IReadOnlyList DeclaredTargets { get; } = new[] { BrushHelperFqn, ResourceBuilderFqn, ElementExtensionsFqn }; + + public RuleSuggestion TryMatch(in RuleContext ctx) + { + if (ctx.Diagnostic.Id != DiagnosticId) return RuleSuggestion.Silent; + + var brushHelper = ctx.Resolver.ResolveType(BrushHelperFqn); + var resourceBuilder = ctx.Resolver.ResolveType(ResourceBuilderFqn); + var extensions = ctx.Resolver.ResolveType(ElementExtensionsFqn); + if (brushHelper is null && resourceBuilder is null && extensions is null) + return RuleSuggestion.Silent; + + // PickRelevantNode lands on the ArgumentSyntax (a literal or ?: branch + // walks up to it) or on the IdentifierName of a const reference. + var argument = ctx.Node.FirstAncestorOrSelf(); + if (argument?.Parent?.Parent is not InvocationExpressionSyntax invocation) + return RuleSuggestion.Silent; + + if (ctx.SemanticModel.GetOperation(invocation) is not IInvocationOperation operation) + return RuleSuggestion.Silent; + + var target = operation.TargetMethod; + var method = (target.ReducedFrom ?? target).OriginalDefinition; + var str = ctx.Compilation.GetSpecialType(SpecialType.System_String); + if (!HasStringColorParameter(method, str)) + return RuleSuggestion.Silent; + + var owner = method.ContainingType; + var isResourceSet = false; + if (SymbolEqualityComparer.Default.Equals(owner, brushHelper)) + { + if (method.Name != "Parse") return RuleSuggestion.Silent; + } + else if (SymbolEqualityComparer.Default.Equals(owner, resourceBuilder)) + { + if (method.Name != "Set" || method.Parameters.Length != 2) return RuleSuggestion.Silent; + isResourceSet = true; + } + else if (SymbolEqualityComparer.Default.Equals(owner, extensions)) + { + if (!method.IsExtensionMethod || !ModifierNames.Contains(method.Name)) return RuleSuggestion.Silent; + } + else + { + return RuleSuggestion.Silent; + } + + IArgumentOperation? colorArg = null; + IArgumentOperation? keyArg = null; + foreach (var arg in operation.Arguments) + { + if (arg.Parameter is null) continue; + if (arg.Parameter.Name == ColorParameterName) colorArg = arg; + else if (arg.Parameter.Name == KeyParameterName) keyArg = arg; + } + if (colorArg is null || colorArg.ArgumentKind != ArgumentKind.Explicit) + return RuleSuggestion.Silent; + // The diagnostic must sit on the color argument, not (say) the key. + if (!colorArg.Syntax.Span.Contains(ctx.Node.Span) && !ctx.Node.Span.Contains(colorArg.Syntax.Span)) + return RuleSuggestion.Silent; + + string? resourceKey = null; + var fontFamilyOverload = false; + if (isResourceSet) + { + if (keyArg?.Value.ConstantValue is { HasValue: true, Value: string key }) + resourceKey = key; + fontFamilyOverload = resourceBuilder!.GetMembers("Set").OfType() + .Any(m => m.Parameters.Length == 2 && m.Parameters[1].Type.ToDisplayString() == FontFamilyFqn); + } + + // Every constant value the argument can take; emit only when the + // unparseable ones agree on a single concrete rewrite. + string? text = null; + ColorStringIssue issue = default; + foreach (var value in ConstantStrings(colorArg.Value.Syntax as ExpressionSyntax, ctx.SemanticModel)) + { + if (ColorStringClassifier.Classify(value, resourceKey, fontFamilyOverload) is not { } finding) + continue; + if (finding.Replacement is null) + return RuleSuggestion.Silent; + var candidate = SuggestionText(finding); + if (text is not null && !string.Equals(text, candidate, StringComparison.Ordinal)) + return RuleSuggestion.Silent; + text = candidate; + issue = finding.Issue; + } + if (text is null) return RuleSuggestion.Silent; + + return new RuleSuggestion( + Text: text, + Confidence: 0.95, + Evidence: issue switch + { + ColorStringIssue.FontFamilyKey => "*FontFamily key → FontFamily overload", + ColorStringIssue.HexShorthand => "BrushHelper has no #RGB shorthand", + ColorStringIssue.MissingHash => "hex color needs a leading '#'", + _ => "named color BrushHelper doesn't know → hex", + }); + } + + /// The → try: text for a finding that carries a replacement. + internal static string SuggestionText(ColorStringFinding finding) => + finding.Issue == ColorStringIssue.FontFamilyKey + ? $"new FontFamily(\"{finding.Replacement}\")" + : $"\"{finding.Replacement}\""; + + static bool HasStringColorParameter(IMethodSymbol method, ITypeSymbol str) + { + foreach (var p in method.Parameters) + if (p.Name == ColorParameterName && SymbolEqualityComparer.Default.Equals(p.Type, str)) + return true; + return false; + } + + // Same walk as ColorStringLiteralAnalyzer.ConstantStrings. + static IEnumerable ConstantStrings(ExpressionSyntax? expression, SemanticModel model) + { + if (expression is null) yield break; + + var constant = model.GetConstantValue(expression); + if (constant.HasValue) + { + if (constant.Value is string s) yield return s; + yield break; + } + + IEnumerable children = expression switch + { + ParenthesizedExpressionSyntax p => [p.Expression], + ConditionalExpressionSyntax c => [c.WhenTrue, c.WhenFalse], + BinaryExpressionSyntax b when b.IsKind(SyntaxKind.CoalesceExpression) => [b.Left, b.Right], + SwitchExpressionSyntax sw => sw.Arms.Select(a => a.Expression), + _ => [], + }; + foreach (var child in children) + foreach (var item in ConstantStrings(child, model)) + yield return item; + } +} diff --git a/src/Reactor.Cli/Check/Rules/RuleRegistry.cs b/src/Reactor.Cli/Check/Rules/RuleRegistry.cs index 22a2cfefe..81aa5f537 100644 --- a/src/Reactor.Cli/Check/Rules/RuleRegistry.cs +++ b/src/Reactor.Cli/Check/Rules/RuleRegistry.cs @@ -150,6 +150,7 @@ public IReadOnlyList Statuses(CSharpCompilation? compilation, ISet BuiltInRules() => [ new ButtonOnClickFactoryMoveRule(), + new ColorStringLiteralRule(), new GridSizeFactoryParensRule(), new GridSizePxRenameRule(), new TextBlockStyleHintRule(), diff --git a/src/Reactor/Elements/BrushHelper.cs b/src/Reactor/Elements/BrushHelper.cs index 3795add41..f57f2811d 100644 --- a/src/Reactor/Elements/BrushHelper.cs +++ b/src/Reactor/Elements/BrushHelper.cs @@ -26,7 +26,8 @@ public static class BrushHelper /// caller. Supports named colors (red, green, blue, white, black, gray, lightgray, /// transparent) and hex codes (#RRGGBB or #AARRGGBB). The parsed color is cached, /// but a new brush instance is returned on every call, so callers may safely mutate - /// it and a brush is never shared between controls. + /// it and a brush is never shared between controls. Any other string silently + /// yields a gray brush; the REACTOR_DYM_006 analyzer flags constant strings that do. /// public static SolidColorBrush Parse(string color) => new(ParseColor(color)); @@ -36,21 +37,46 @@ public static class BrushHelper /// internal static global::Windows.UI.Color ParseColor(string color) => _colorCache.GetOrAdd(color, static c => - c.ToLowerInvariant() switch - { - "red" => global::Windows.UI.Color.FromArgb(255, 255, 0, 0), - "green" => global::Windows.UI.Color.FromArgb(255, 0, 128, 0), - "blue" => global::Windows.UI.Color.FromArgb(255, 0, 0, 255), - "white" => global::Windows.UI.Color.FromArgb(255, 255, 255, 255), - "black" => global::Windows.UI.Color.FromArgb(255, 0, 0, 0), - "gray" or "grey" => global::Windows.UI.Color.FromArgb(255, 128, 128, 128), - "lightgray" or "lightgrey" => global::Windows.UI.Color.FromArgb(255, 211, 211, 211), - "transparent" => global::Windows.UI.Color.FromArgb(0, 0, 0, 0), - _ when c.StartsWith('#') => ParseHex(c), - _ => global::Windows.UI.Color.FromArgb(255, 128, 128, 128), - }); + TryParseColor(c, out var parsed) ? parsed : FallbackGray); - internal static global::Windows.UI.Color ParseHex(string hex) + /// + /// The color every unrecognized string silently falls back to. + /// + internal static readonly global::Windows.UI.Color FallbackGray = global::Windows.UI.Color.FromArgb(255, 128, 128, 128); + + /// + /// Parses a color string without the gray fallback, so callers can tell a genuine + /// "gray" from an unrecognized string. Uncached. + /// + /// + /// The REACTOR_DYM_006 analyzer (ColorStringClassifier in Reactor.Analyzers) and + /// the mur check ColorStringLiteralRule carry byte-for-byte copies of this + /// grammar; ColorStringParityTests locks all three together. + /// + internal static bool TryParseColor(string color, out global::Windows.UI.Color parsed) + { + switch (color.ToLowerInvariant()) + { + case "red": parsed = global::Windows.UI.Color.FromArgb(255, 255, 0, 0); return true; + case "green": parsed = global::Windows.UI.Color.FromArgb(255, 0, 128, 0); return true; + case "blue": parsed = global::Windows.UI.Color.FromArgb(255, 0, 0, 255); return true; + case "white": parsed = global::Windows.UI.Color.FromArgb(255, 255, 255, 255); return true; + case "black": parsed = global::Windows.UI.Color.FromArgb(255, 0, 0, 0); return true; + case "gray" or "grey": parsed = global::Windows.UI.Color.FromArgb(255, 128, 128, 128); return true; + case "lightgray" or "lightgrey": parsed = global::Windows.UI.Color.FromArgb(255, 211, 211, 211); return true; + case "transparent": parsed = global::Windows.UI.Color.FromArgb(0, 0, 0, 0); return true; + } + if (color.StartsWith('#')) + return TryParseHex(color, out parsed); + parsed = FallbackGray; + return false; + } + + internal static global::Windows.UI.Color ParseHex(string hex) => + // Fallback to gray for malformed hex, consistent with named color fallback + TryParseHex(hex, out var parsed) ? parsed : FallbackGray; + + private static bool TryParseHex(string hex, out global::Windows.UI.Color parsed) { hex = hex.TrimStart('#'); if (hex.Length == 6 @@ -58,7 +84,8 @@ _ when c.StartsWith('#') => ParseHex(c), && byte.TryParse(hex[2..4], global::System.Globalization.NumberStyles.HexNumber, null, out var g6) && byte.TryParse(hex[4..6], global::System.Globalization.NumberStyles.HexNumber, null, out var b6)) { - return global::Windows.UI.Color.FromArgb(255, r6, g6, b6); + parsed = global::Windows.UI.Color.FromArgb(255, r6, g6, b6); + return true; } if (hex.Length == 8 && byte.TryParse(hex[0..2], global::System.Globalization.NumberStyles.HexNumber, null, out var a8) @@ -66,9 +93,10 @@ _ when c.StartsWith('#') => ParseHex(c), && byte.TryParse(hex[4..6], global::System.Globalization.NumberStyles.HexNumber, null, out var g8) && byte.TryParse(hex[6..8], global::System.Globalization.NumberStyles.HexNumber, null, out var b8)) { - return global::Windows.UI.Color.FromArgb(a8, r8, g8, b8); + parsed = global::Windows.UI.Color.FromArgb(a8, r8, g8, b8); + return true; } - // Fallback to gray for malformed hex, consistent with named color fallback - return global::Windows.UI.Color.FromArgb(255, 128, 128, 128); + parsed = FallbackGray; + return false; } } diff --git a/src/Reactor/Elements/ResourceBuilder.cs b/src/Reactor/Elements/ResourceBuilder.cs index c45a049db..bb43f643c 100644 --- a/src/Reactor/Elements/ResourceBuilder.cs +++ b/src/Reactor/Elements/ResourceBuilder.cs @@ -43,7 +43,12 @@ public sealed class ResourceBuilder private readonly Dictionary _literals = new(); private readonly Dictionary _themeRefs = new(); - /// Sets a color resource override from a hex string or named color. + /// + /// Sets a color resource override from a hex string (#RRGGBB / #AARRGGBB) or named color + /// (see ). Anything else — including a font name under a + /// *FontFamily key — silently stores a gray brush; the REACTOR_DYM_006 analyzer + /// flags constant values that do. + /// public ResourceBuilder Set(string key, string color) { _literals[key] = BrushHelper.Parse(color); diff --git a/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs b/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs new file mode 100644 index 000000000..1e9260ca2 --- /dev/null +++ b/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs @@ -0,0 +1,339 @@ +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.CSharp.Testing; +using Microsoft.CodeAnalysis.Testing; +using Microsoft.UI.Reactor.Analyzers; +using Xunit; + +namespace Microsoft.UI.Reactor.Tests.AnalyzerTests; + +/// +/// Tests for REACTOR_DYM_006 ( + +/// ): a constant color string that BrushHelper can't parse and +/// so silently renders gray. Unlike DYM_001–005 the flagged code compiles, so compiler-error +/// verification stays ON — every fixed snippet must still compile. +/// +public class ColorStringLiteralAnalyzerTests +{ + // The color-string API surface, minus the FontFamily overload of ResourceBuilder.Set (PR #1370). + internal const string CommonStubs = @" +namespace Microsoft.UI.Xaml.Media +{ + public class Brush { } + public class SolidColorBrush : Brush { } + public class FontFamily { public FontFamily(string familyName) { } } +} +namespace Microsoft.UI.Reactor.Core +{ + public abstract class Element { } + public sealed class ThemeRef { } + public sealed class TextBlockElement : Element { } +} +namespace Microsoft.UI.Reactor +{ + using Microsoft.UI.Reactor.Core; + using Microsoft.UI.Xaml.Media; + public sealed class RichTextRun { } + public static class BrushHelper + { + public static SolidColorBrush Parse(string color) => null!; + } + public static partial class ElementExtensions + { + public static T Background(this T el, string color) where T : Element => el; + public static T Background(this T el, Brush brush) where T : Element => el; + public static T Background(this T el, ThemeRef theme) where T : Element => el; + public static T Foreground(this T el, string color) where T : Element => el; + public static T BorderBrush(this T el, string color) where T : Element => el; + public static T WithBorder(this T el, string color, double thickness = 1) where T : Element => el; + public static RichTextRun Foreground(this RichTextRun run, string color) => run; + public static T Margin(this T el, string label) where T : Element => el; + } + public static class Factories + { + public static TextBlockElement TextBlock(string text) => null!; + } +} +namespace Acme +{ + using Microsoft.UI.Reactor.Core; + public static class ElementExtensions + { + public static T Paint(this T el, string color) where T : Element => el; + } + public static class BrushHelper + { + public static object Parse(string color) => null!; + } +} +"; + + internal const string ResourceBuilderWithoutFontFamily = @" +namespace Microsoft.UI.Reactor.Elements +{ + using Microsoft.UI.Xaml.Media; + public sealed class ResourceBuilder + { + public ResourceBuilder Set(string key, string color) => this; + public ResourceBuilder Set(string key, Brush brush) => this; + public ResourceBuilder Set(string key, double value) => this; + } +} +"; + + internal const string ResourceBuilderWithFontFamily = @" +namespace Microsoft.UI.Reactor.Elements +{ + using Microsoft.UI.Xaml.Media; + public sealed class ResourceBuilder + { + public ResourceBuilder Set(string key, string color) => this; + public ResourceBuilder Set(string key, Brush brush) => this; + public ResourceBuilder Set(string key, double value) => this; + public ResourceBuilder Set(string key, FontFamily fontFamily) => this; + } +} +"; + + private const string Usings = @" +namespace App +{ + using Microsoft.UI.Reactor; + using Microsoft.UI.Reactor.Core; + using Microsoft.UI.Reactor.Elements; + using Microsoft.UI.Xaml.Media; + using static Microsoft.UI.Reactor.Factories; +"; + + private static string Wrap(string members, bool fontFamilyOverload = false) => + CommonStubs + + (fontFamilyOverload ? ResourceBuilderWithFontFamily : ResourceBuilderWithoutFontFamily) + + Usings + + " static class C\n {\n" + members + "\n }\n}\n"; + + private static CSharpAnalyzerTest MakeTest(string code) + { + var test = new CSharpAnalyzerTest { TestCode = code }; + test.DisabledDiagnostics.Add("CS1591"); + return test; + } + + private static Task RunAsync(string members, bool fontFamilyOverload = false) => + MakeTest(Wrap(members, fontFamilyOverload)).RunAsync(TestContext.Current.CancellationToken); + + private static DiagnosticResult Dym006(int markup) => + new DiagnosticResult(ColorStringLiteralAnalyzer.DiagnosticId, Microsoft.CodeAnalysis.DiagnosticSeverity.Warning) + .WithLocation(markup); + + // ── Positives: every covered API ───────────────────────────────────────────────────────────── + + [Fact] + public async Task Flags_Background_with_css_name_and_suggests_hex() + { + var test = MakeTest(Wrap(@" static object M() => TextBlock(""hi"").Background({|#0:""orange""|});")); + test.ExpectedDiagnostics.Add(Dym006(0).WithArguments( + "orange", + "BrushHelper only knows red, green, blue, white, black, gray, lightgray, transparent; did you mean \"#FFA500\"?")); + await test.RunAsync(TestContext.Current.CancellationToken); + } + + [Fact] + public Task Flags_Foreground_hex_shorthand() => + RunAsync(@" static object M() => TextBlock(""hi"").Foreground({|REACTOR_DYM_006:""#888""|});"); + + [Fact] + public Task Flags_BorderBrush_missing_hash() => + RunAsync(@" static object M() => TextBlock(""hi"").BorderBrush({|REACTOR_DYM_006:""FF0000""|});"); + + [Fact] + public Task Flags_WithBorder_malformed_hex() => + RunAsync(@" static object M() => TextBlock(""hi"").WithBorder({|REACTOR_DYM_006:""#12345""|}, 2);"); + + [Fact] + public Task Flags_RichTextRun_Foreground() => + RunAsync(@" static object M(RichTextRun run) => run.Foreground({|REACTOR_DYM_006:""Yellow""|});"); + + [Fact] + public Task Flags_BrushHelper_Parse() => + RunAsync(@" static object M() => BrushHelper.Parse({|REACTOR_DYM_006:""AliceBlue""|});"); + + [Fact] + public Task Flags_ResourceBuilder_Set_color_key() => + RunAsync(@" static object M(ResourceBuilder r) => r.Set(""ButtonBackground"", {|REACTOR_DYM_006:""LightCoral""|});"); + + [Fact] + public Task Flags_named_argument() => + RunAsync(@" static object M() => TextBlock(""hi"").Background(color: {|REACTOR_DYM_006:""orange""|});"); + + [Fact] + public async Task Flags_unknown_name_with_supported_list() + { + var test = MakeTest(Wrap(@" static object M() => TextBlock(""hi"").Background({|#0:""accent""|});")); + test.ExpectedDiagnostics.Add(Dym006(0).WithArguments( + "accent", + "use #RRGGBB or #AARRGGBB, a Theme token, or one of red, green, blue, white, black, gray, lightgray, transparent")); + await test.RunAsync(TestContext.Current.CancellationToken); + } + + [Fact] + public Task Flags_empty_string() => + RunAsync(@" static object M() => TextBlock(""hi"").Background({|REACTOR_DYM_006:""""|});"); + + // ── Positives: constant shapes ─────────────────────────────────────────────────────────────── + + [Fact] + public Task Flags_const_reference() => + RunAsync(@" const string Brand = ""orange""; + static object M() => TextBlock(""hi"").Background({|REACTOR_DYM_006:Brand|});"); + + [Fact] + public Task Flags_constant_concatenation() => + RunAsync(@" const string Hash = ""#""; + static object M() => TextBlock(""hi"").Background({|REACTOR_DYM_006:Hash + ""888""|});"); + + [Fact] + public Task Flags_only_the_unparseable_conditional_branch() => + RunAsync(@" static object M(bool on) => TextBlock(""hi"").Background(on ? ""red"" : {|REACTOR_DYM_006:""orange""|});"); + + [Fact] + public Task Flags_coalesce_and_switch_branches() => + RunAsync(@" static object M(string s, int i) => TextBlock(""hi"") + .Background(s ?? {|REACTOR_DYM_006:""teal""|}) + .Foreground(i switch { 0 => ""black"", _ => {|REACTOR_DYM_006:""navy""|} });"); + + // ── FontFamily key ────────────────────────────────────────────────────────────────────────── + + [Fact] + public async Task Flags_FontFamily_key_and_suggests_FontFamily_when_overload_exists() + { + var test = MakeTest(Wrap( + @" static object M(ResourceBuilder r) => r.Set(""ContentControlThemeFontFamily"", {|#0:""Consolas""|});", + fontFamilyOverload: true)); + test.ExpectedDiagnostics.Add(Dym006(0).WithArguments( + "Consolas", + "the key 'ContentControlThemeFontFamily' names a FontFamily resource; did you mean new FontFamily(\"Consolas\")?")); + await test.RunAsync(TestContext.Current.CancellationToken); + } + + [Fact] + public async Task Flags_FontFamily_key_without_suggestion_when_overload_missing() + { + var test = MakeTest(Wrap( + @" static object M(ResourceBuilder r) => r.Set(""ContentControlThemeFontFamily"", {|#0:""Consolas""|});")); + test.ExpectedDiagnostics.Add(Dym006(0).WithArguments( + "Consolas", + "the key 'ContentControlThemeFontFamily' names a FontFamily resource, but Set(string, string) only stores color brushes")); + await test.RunAsync(TestContext.Current.CancellationToken); + } + + // ── Negatives ─────────────────────────────────────────────────────────────────────────────── + + [Fact] + public Task Silent_on_every_supported_name_in_any_casing() => + RunAsync(@" static object M() => TextBlock(""hi"") + .Background(""red"").Background(""Green"").Background(""BLUE"").Background(""White"") + .Background(""black"").Background(""Gray"").Background(""grey"").Background(""LightGray"") + .Background(""lightgrey"").Background(""Transparent"");"); + + [Fact] + public Task Silent_on_valid_hex() => + RunAsync(@" static object M() => TextBlock(""hi"") + .Background(""#FF0000"").Foreground(""#80ff0000"").BorderBrush(""#aAbBcC"").WithBorder(""#00000000"");"); + + [Fact] + public Task Silent_on_non_constant_values() => + RunAsync(@" static object M(string c, int n) => TextBlock(""hi"") + .Background(c).Foreground($""#{n:X6}"").BorderBrush(c + ""00"");"); + + [Fact] + public Task Silent_on_brush_and_theme_overloads_and_non_color_params() => + RunAsync(@" static object M(Brush b, ThemeRef t, ResourceBuilder r) + { + r.Set(""ButtonBorderThemeThickness"", 2.0); + r.Set(""ButtonBackground"", ""#0078D4""); + r.Set(""ContentControlThemeFontFamily"", b); + return TextBlock(""orange"").Background(b).Background(t).Margin(""orange""); + }"); + + [Fact] + public Task Silent_on_lookalike_apis_outside_Reactor() => + RunAsync(@" static object M() => Acme.BrushHelper.Parse(""orange"") ?? Acme.ElementExtensions.Paint(TextBlock(""hi""), ""orange"");"); + + [Fact] + public Task Silent_on_FontFamily_key_with_a_valid_color() => + RunAsync(@" static object M(ResourceBuilder r) => r.Set(""SomethingFontFamily"", ""red"");", fontFamilyOverload: true); + + [Fact] + public Task Silent_on_unreachable_constant_branch() => + // The whole expression is a constant ("red"), so the dead "orange" branch is never parsed. + RunAsync(@" static object M() => TextBlock(""hi"").Background(true ? ""red"" : ""orange"");"); + + [Fact] + public async Task Silent_without_any_Reactor_color_api() + { + var test = MakeTest(@"static class C { static string M() => ""orange""; }"); + await test.RunAsync(TestContext.Current.CancellationToken); + } + + // ── Code fixes ────────────────────────────────────────────────────────────────────────────── + + private static Task RunFixAsync(string before, string after, bool fontFamilyOverload = false) + { + var test = new CSharpCodeFixTest + { + TestCode = Wrap(before, fontFamilyOverload), + FixedCode = Wrap(after, fontFamilyOverload), + }; + test.DisabledDiagnostics.Add("CS1591"); + return test.RunAsync(TestContext.Current.CancellationToken); + } + + [Fact] + public Task Fix_expands_hex_shorthand() => + RunFixAsync( + @" static object M() => TextBlock(""hi"").Foreground({|REACTOR_DYM_006:""#888""|}).Background({|REACTOR_DYM_006:""#8abc""|});", + @" static object M() => TextBlock(""hi"").Foreground(""#888888"").Background(""#88aabbcc"");"); + + [Fact] + public Task Fix_adds_missing_hash() => + RunFixAsync( + @" static object M() => TextBlock(""hi"").Background({|REACTOR_DYM_006:""FF0000""|});", + @" static object M() => TextBlock(""hi"").Background(""#FF0000"");"); + + [Fact] + public Task Fix_maps_css_name_to_hex_including_inside_a_branch() => + RunFixAsync( + @" static object M(bool on) => BrushHelper.Parse(on ? {|REACTOR_DYM_006:""AliceBlue""|} : ""white"");", + @" static object M(bool on) => BrushHelper.Parse(on ? ""#F0F8FF"" : ""white"");"); + + [Fact] + public Task Fix_rewrites_FontFamily_key_to_FontFamily_overload() => + RunFixAsync( + @" static object M(ResourceBuilder r) => r.Set(""ContentControlThemeFontFamily"", {|REACTOR_DYM_006:""Consolas""|});", + @" static object M(ResourceBuilder r) => r.Set(""ContentControlThemeFontFamily"", new FontFamily(""Consolas""));", + fontFamilyOverload: true); + + [Fact] + public Task Fix_wraps_const_FontFamily_reference() => + RunFixAsync( + @" const string Mono = ""Consolas""; + static object M(ResourceBuilder r) => r.Set(""ContentControlThemeFontFamily"", {|REACTOR_DYM_006:Mono|});", + @" const string Mono = ""Consolas""; + static object M(ResourceBuilder r) => r.Set(""ContentControlThemeFontFamily"", new FontFamily(Mono));", + fontFamilyOverload: true); + + [Fact] + public Task No_fix_without_FontFamily_overload_or_for_unknown_or_const_color() => + RunFixAsync( + @" const string Brand = ""orange""; + static object M(ResourceBuilder r) + { + r.Set(""ContentControlThemeFontFamily"", {|REACTOR_DYM_006:""Consolas""|}); + return TextBlock(""hi"").Background({|REACTOR_DYM_006:""accent""|}).Foreground({|REACTOR_DYM_006:Brand|}).BorderBrush({|REACTOR_DYM_006:""#12345""|}); + }", + @" const string Brand = ""orange""; + static object M(ResourceBuilder r) + { + r.Set(""ContentControlThemeFontFamily"", {|REACTOR_DYM_006:""Consolas""|}); + return TextBlock(""hi"").Background({|REACTOR_DYM_006:""accent""|}).Foreground({|REACTOR_DYM_006:Brand|}).BorderBrush({|REACTOR_DYM_006:""#12345""|}); + }"); +} diff --git a/tests/Reactor.Tests/AnalyzerTests/ColorStringParityTests.cs b/tests/Reactor.Tests/AnalyzerTests/ColorStringParityTests.cs new file mode 100644 index 000000000..66175d7fc --- /dev/null +++ b/tests/Reactor.Tests/AnalyzerTests/ColorStringParityTests.cs @@ -0,0 +1,100 @@ +using AnalyzerClassifier = Microsoft.UI.Reactor.Analyzers.ColorStringClassifier; +using CliClassifier = Microsoft.UI.Reactor.Cli.Check.Rules.ColorStringClassifier; +using Xunit; + +namespace Microsoft.UI.Reactor.Tests.AnalyzerTests; + +/// +/// REACTOR_DYM_006 parity lock (issue #1375). Three copies of one color grammar must agree: +/// BrushHelper.TryParseColor (the runtime truth), the netstandard2.0 analyzer's +/// ColorStringClassifier, and the mur check CLI's copy. If any of them drifts, the +/// analyzer either flags colors that render fine or misses ones that render gray. +/// +public class ColorStringParityTests +{ + // Covers every branch of BrushHelper's grammar, including its quirks: case folding, the + // '#'-only hex route, TrimStart('#') swallowing repeated hashes, and NumberStyles.HexNumber + // accepting leading/trailing whitespace inside a byte pair. + public static TheoryData Battery => new() + { + "", " ", "red", "RED", "Red", "green", "blue", "white", "black", "gray", "GREY", "lightgray", + "LightGrey", "transparent", "Transparent", " red", "red ", "reds", "orange", "AliceBlue", + "aliceblue", "LightCoral", "Yellow", "slategrey", "rebeccapurple", "accent", "Consolas", + "Segoe UI", "#", "##", "#FF0000", "#ff0000", "#aAbBcC", "#80FF0000", "#00000000", "##FF0000", + "###FF0000", "#FFF", "#ffff", "#8abc", "#12345", "#1234567", "#123456789", "#GGHHII", + "#FF00GG", "#0 0000", "# 00000", "#00000 ", "# FF0000", "#-10000", "#+10000", "#0x1234", + "FF0000", "80FF0000", "ff00ff", "FFF", "decade", "#FF0000\n", "\u0130", "#\u0661\u0662\u0663\u0664\u0665\u0666", + }; + + [Theory] + [MemberData(nameof(Battery))] + public void Analyzer_TryParse_matches_BrushHelper(string value) + { + var runtime = BrushHelper.TryParseColor(value, out _); + Assert.Equal(runtime, AnalyzerClassifier.TryParse(value)); + Assert.Equal(runtime, CliClassifier.TryParse(value)); + } + + [Theory] + [MemberData(nameof(Battery))] + public void Analyzer_and_cli_classify_identically(string value) + { + foreach (var key in new[] { null, "ButtonBackground", "ContentControlThemeFontFamily", "FontFamily", "fontfamily" }) + { + foreach (var overload in new[] { false, true }) + { + var a = AnalyzerClassifier.Classify(value, key, overload); + var c = CliClassifier.Classify(value, key, overload); + Assert.Equal(a.HasValue, c.HasValue); + if (a is { } af && c is { } cf) + { + Assert.Equal(af.Issue.ToString(), cf.Issue.ToString()); + Assert.Equal(af.Replacement, cf.Replacement); + Assert.Equal(af.Detail, cf.Detail); + } + } + } + } + + [Fact] + public void Named_color_tables_are_identical() + { + Assert.Equal( + AnalyzerClassifier.NamedColorHex.OrderBy(p => p.Key, StringComparer.Ordinal), + CliClassifier.NamedColorHex.OrderBy(p => p.Key, StringComparer.Ordinal)); + Assert.Equal(AnalyzerClassifier.SupportedNames, CliClassifier.SupportedNames); + } + + [Fact] + public void Every_named_color_is_unsupported_by_BrushHelper_and_maps_to_its_exact_hex() + { + Assert.True(AnalyzerClassifier.NamedColorHex.Count >= 130, $"Only {AnalyzerClassifier.NamedColorHex.Count} names."); + foreach (var (name, hex) in AnalyzerClassifier.NamedColorHex) + { + // A name BrushHelper already parses would be a false positive. + Assert.False(BrushHelper.TryParseColor(name, out _), $"BrushHelper already parses '{name}'."); + + // The suggested hex must parse, and must be the color the name means. System.Drawing's + // KnownColor table is an independent oracle (it spells the grey aliases 'gray'). + Assert.True(BrushHelper.TryParseColor(hex, out var parsed), $"'{hex}' for '{name}' does not parse."); + var known = global::System.Drawing.Color.FromName(name.Replace("grey", "gray", StringComparison.Ordinal)); + Assert.True(known.IsKnownColor, $"'{name}' is not a known color."); + Assert.Equal((known.R, known.G, known.B), (parsed.R, parsed.G, parsed.B)); + Assert.Equal(255, parsed.A); + } + } + + [Theory] + [InlineData("#888", "#888888")] + [InlineData("#8abc", "#88aabbcc")] + [InlineData("FF0000", "#FF0000")] + [InlineData("80FF0000", "#80FF0000")] + [InlineData("orange", "#FFA500")] + public void Every_literal_replacement_actually_parses(string value, string expected) + { + var finding = AnalyzerClassifier.Classify(value, null, false); + Assert.NotNull(finding); + Assert.Equal(expected, finding!.Value.Replacement); + Assert.True(BrushHelper.TryParseColor(expected, out _)); + } +} diff --git a/tests/Reactor.Tests/AnalyzerTests/DidYouMeanAnalyzerParityTests.cs b/tests/Reactor.Tests/AnalyzerTests/DidYouMeanAnalyzerParityTests.cs index 9a930e8cc..d3ef39377 100644 --- a/tests/Reactor.Tests/AnalyzerTests/DidYouMeanAnalyzerParityTests.cs +++ b/tests/Reactor.Tests/AnalyzerTests/DidYouMeanAnalyzerParityTests.cs @@ -10,6 +10,7 @@ using Microsoft.UI.Reactor.Cli.Check.Rules; using Microsoft.UI.Reactor.Cli.Check.Suggesters; using Microsoft.UI.Reactor.Tests.CheckCommandTests; +using Microsoft.UI.Reactor.Tests.CheckCommandTests.Rules; using Xunit; namespace Microsoft.UI.Reactor.Tests.AnalyzerTests; @@ -167,4 +168,31 @@ private static SuggestionResult RunSymbolSuggester(string userCode, string code) var ctx = new SuggesterContext(comp, diag, pick, receiver, FactoryIndex.Build(comp)); return new SymbolSuggester().Suggest(ctx); } + + // ── REACTOR_DYM_006 (ColorStringLiteralAnalyzer) <-> ColorStringLiteralRule ── + // + // The analyzer's code-fix replacement and the rule's `→ try:` text must name the same rewrite. + // Driven end-to-end: the real analyzer reports, its diagnostic is fed to the rule. + + [Theory] + [InlineData(@"TextBlock(""hi"").Foreground(""#888"")", false)] + [InlineData(@"TextBlock(""hi"").Background(""ff00ff"")", false)] + [InlineData(@"BrushHelper.Parse(""orange"")", false)] + [InlineData(@"new ResourceBuilder().Set(""ButtonBackground"", ""Wheat"")", false)] + [InlineData(@"new ResourceBuilder().Set(""ContentControlThemeFontFamily"", ""Consolas"")", true)] + public void Color_string_analyzer_replacement_matches_cli_rule(string expression, bool fontFamilyOverload) + { + var (diagnostics, c) = ColorStringLiteralRuleTests.AnalyzeRaw(expression, fontFamilyOverload); + var d = Assert.Single(diagnostics); + var replacement = d.Properties[ColorStringLiteralAnalyzer.ReplacementProperty]; + Assert.False(string.IsNullOrEmpty(replacement)); + var expected = d.Properties[ColorStringLiteralAnalyzer.IssueProperty] == "FontFamilyKey" + ? $"new FontFamily(\"{replacement}\")" + : $"\"{replacement}\""; + + var s = ColorStringLiteralRuleTests.RunRule(ColorStringLiteralRuleTests.ToCheckDiag(d), c); + Assert.NotNull(s); + Assert.Equal("ColorStringLiteralRule", s!.SuggesterName); + Assert.Equal(expected, s.Text); + } } diff --git a/tests/Reactor.Tests/CheckCommandTests/Rules/ColorStringLiteralRuleTests.cs b/tests/Reactor.Tests/CheckCommandTests/Rules/ColorStringLiteralRuleTests.cs new file mode 100644 index 000000000..23e6fbccd --- /dev/null +++ b/tests/Reactor.Tests/CheckCommandTests/Rules/ColorStringLiteralRuleTests.cs @@ -0,0 +1,154 @@ +// Tests for ColorStringLiteralRule — the `mur check` mirror of REACTOR_DYM_006 +// (issue #1375). +// +// There is no compiler error for this mistake, so the rule anchors on the +// analyzer's own diagnostic. These tests therefore run the REAL analyzer over +// a stub compilation, turn each REACTOR_DYM_006 it reports into the +// MSBuild-shaped `CheckCommand.Diag` that `mur check` parses, and drive the +// orchestrator with it — the same path a `dotnet build` line takes. + +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.UI.Reactor.Analyzers; +using Microsoft.UI.Reactor.Cli.Check; +using Microsoft.UI.Reactor.Cli.Check.Rules; +using Microsoft.UI.Reactor.Tests.AnalyzerTests; +using Xunit; + +namespace Microsoft.UI.Reactor.Tests.CheckCommandTests.Rules; + +public class ColorStringLiteralRuleTests +{ + [Theory] + [InlineData(@"TextBlock(""hi"").Background(""orange"")", "\"#FFA500\"")] + [InlineData(@"TextBlock(""hi"").Foreground(""#888"")", "\"#888888\"")] + [InlineData(@"TextBlock(""hi"").BorderBrush(""FF0000"")", "\"#FF0000\"")] + [InlineData(@"BrushHelper.Parse(""AliceBlue"")", "\"#F0F8FF\"")] + [InlineData(@"new ResourceBuilder().Set(""ButtonBackground"", ""LightCoral"")", "\"#F08080\"")] + [InlineData(@"TextBlock(""hi"").Background(Brand)", "\"#FFA500\"")] + public void Fires_with_the_concrete_rewrite(string expression, string expected) + { + var (diags, c) = Analyze(expression); + var diag = Assert.Single(diags); + var s = RunRule(diag, c); + Assert.NotNull(s); + Assert.Equal("ColorStringLiteralRule", s!.SuggesterName); + Assert.True(s.IsRule); + Assert.Equal(expected, s.Text); + Assert.Contains("analyzer:REACTOR_DYM_006", s.Evidence); + } + + [Fact] + public void Suggests_FontFamily_when_the_overload_exists() + { + var (diags, c) = Analyze(@"new ResourceBuilder().Set(""ContentControlThemeFontFamily"", ""Consolas"")", fontFamilyOverload: true); + var s = RunRule(Assert.Single(diags), c); + Assert.NotNull(s); + Assert.Equal("new FontFamily(\"Consolas\")", s!.Text); + } + + [Theory] + // No concrete rewrite exists for these — the analyzer's message carries the advice. + [InlineData(@"new ResourceBuilder().Set(""ContentControlThemeFontFamily"", ""Consolas"")")] + [InlineData(@"TextBlock(""hi"").Background(""accent"")")] + [InlineData(@"TextBlock(""hi"").Background(""#12345"")")] + public void Silent_when_there_is_no_concrete_rewrite(string expression) + { + var (diags, c) = Analyze(expression); + Assert.Null(RunRule(Assert.Single(diags), c)); + } + + [Fact] + public void Each_branch_gets_its_own_rewrite_and_disagreeing_branches_stay_silent() + { + // Two unparseable branches with different rewrites: the rule only sees the argument, so it + // cannot tell which literal the diagnostic is about — silent beats a wrong suggestion. + var (diags, c) = Analyze(@"TextBlock(""hi"").Background(on ? ""orange"" : ""teal"")"); + Assert.Equal(2, diags.Length); + Assert.All(diags, d => Assert.Null(RunRule(d, c))); + + // One unparseable branch: unambiguous. + (diags, c) = Analyze(@"TextBlock(""hi"").Background(on ? ""orange"" : ""red"")"); + Assert.Equal("\"#FFA500\"", RunRule(Assert.Single(diags), c)!.Text); + } + + [Fact] + public void Silent_on_other_diagnostic_codes() + { + var (diags, c) = Analyze(@"TextBlock(""hi"").Background(""orange"")"); + var other = Assert.Single(diags) with { Code = "REACTOR_THEME_001" }; + Assert.Null(RunRule(other, c)); + } + + [Fact] + public void Silent_when_the_targets_are_missing() + { + // Same source, but the Reactor color APIs were renamed (API churn): the rule self-disables + // instead of guessing. Positive control first so the null below isn't a mis-anchored span. + var (diags, c) = Analyze(@"TextBlock(""hi"").Background(""orange"")"); + var diag = Assert.Single(diags); + Assert.NotNull(RunRule(diag, c)); + + var renamed = c.ReplaceSyntaxTree( + c.SyntaxTrees.First(t => t.FilePath == "Stub.cs"), + CSharpSyntaxTree.ParseText( + ColorStringLiteralAnalyzerTests.CommonStubs + .Replace("public static partial class ElementExtensions", "public static partial class RenamedExtensions", StringComparison.Ordinal) + .Replace("public static class BrushHelper\r\n {\r\n public static SolidColorBrush", "public static class RenamedBrushHelper\r\n {\r\n public static SolidColorBrush", StringComparison.Ordinal) + .Replace("public static class BrushHelper\n {\n public static SolidColorBrush", "public static class RenamedBrushHelper\n {\n public static SolidColorBrush", StringComparison.Ordinal), + path: "Stub.cs", + cancellationToken: TestContext.Current.CancellationToken)); + Assert.Null(renamed.GetTypeByMetadataName("Microsoft.UI.Reactor.ElementExtensions")); + Assert.Null(renamed.GetTypeByMetadataName("Microsoft.UI.Reactor.BrushHelper")); + Assert.Null(RunRule(diag, renamed)); + } + + internal static (CheckCommand.Diag[] Diags, CSharpCompilation Compilation) Analyze(string expression, bool fontFamilyOverload = false) + { + var (found, c) = AnalyzeRaw(expression, fontFamilyOverload); + return (found.Select(ToCheckDiag).ToArray(), c); + } + + internal static CheckCommand.Diag ToCheckDiag(Diagnostic d) + { + var span = d.Location.GetLineSpan(); + return new CheckCommand.Diag( + span.Path, span.StartLinePosition.Line + 1, span.StartLinePosition.Character + 1, + "warning", d.Id, d.GetMessage()); + } + + internal static (Diagnostic[] Diagnostics, CSharpCompilation Compilation) AnalyzeRaw(string expression, bool fontFamilyOverload = false) + { + var source = $@" +namespace App +{{ + using Microsoft.UI.Reactor; + using Microsoft.UI.Reactor.Elements; + using static Microsoft.UI.Reactor.Factories; + static class C + {{ + const string Brand = ""orange""; + static object M(bool on) => {expression}; + }} +}}"; + var c = TestCompilation.Create(new[] + { + (ColorStringLiteralAnalyzerTests.CommonStubs, "Stub.cs"), + (fontFamilyOverload ? ColorStringLiteralAnalyzerTests.ResourceBuilderWithFontFamily : ColorStringLiteralAnalyzerTests.ResourceBuilderWithoutFontFamily, "ResourceBuilder.cs"), + (source, "Test.cs"), + }); + Assert.Empty(c.GetDiagnostics(TestContext.Current.CancellationToken).Where(d => d.Severity == DiagnosticSeverity.Error)); + + var found = c.WithAnalyzers(ImmutableArray.Create(new ColorStringLiteralAnalyzer())) + .GetAnalyzerDiagnosticsAsync(TestContext.Current.CancellationToken).GetAwaiter().GetResult(); + return (found.Where(d => d.Id == ColorStringLiteralAnalyzer.DiagnosticId).ToArray(), c); + } + + internal static Suggestion? RunRule(CheckCommand.Diag diag, CSharpCompilation c) + { + var registry = RuleRegistry.Of(new IRulePattern[] { new ColorStringLiteralRule() }); + return new SuggesterOrchestrator(rules: registry).SuggestAgainst(diag, c); + } +} diff --git a/tests/Reactor.Tests/CheckCommandTests/Rules/RulePerformanceTests.cs b/tests/Reactor.Tests/CheckCommandTests/Rules/RulePerformanceTests.cs index c37edc7aa..1a1b5ff70 100644 --- a/tests/Reactor.Tests/CheckCommandTests/Rules/RulePerformanceTests.cs +++ b/tests/Reactor.Tests/CheckCommandTests/Rules/RulePerformanceTests.cs @@ -66,6 +66,21 @@ public static class Factories { public static ButtonElement Button(string label, Action? onClick = null) => new(label); } + public static class BrushHelper + { + public static object Parse(string color) => null!; + } + public static class ElementExtensions + { + public static T Background(this T el, string color) where T : Element => el; + } +} +namespace Microsoft.UI.Reactor.Elements +{ + public sealed class ResourceBuilder + { + public ResourceBuilder Set(string key, string color) => this; + } }"; const string TestSource = @" From 4075b265877eeeeb03ba1f066dff9a3ea859eb38 Mon Sep 17 00:00:00 2001 From: Alexandre Zollinger Chohfi Date: Thu, 15 Oct 2026 11:05:44 -0700 Subject: [PATCH 02/10] Address pr-review findings for REACTOR_DYM_006 - Prune branches a constant condition / non-null ?? left side / constant switch rules out, so dead code is never reported. - FontFamily: without the Set(string, FontFamily) overload (or inside a ?: branch) point at the .FontFamily(...) modifier; the fix renames a named color: argument to the overload's parameter. - mur check rule now reads the rewrite from the analyzer's message instead of re-deriving it; drops the CLI classifier copy and gives each ?: branch its own suggestion. - Tighter message wording; supported list includes grey/lightgrey. - Tests: live-Reactor target test, dead-branch, FontFamily-in-branch, named-argument fix, BrushHelper built-in colors + gray fallback. - Docs: skill row no longer recommends a non-existent overload; stale rule counts updated. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b58aacd0-29b6-46c1-9603-b8a8b4eecbd8 --- .../templates/analyzer-architecture.md.dt | 2 +- docs/guide/analyzer-architecture.md | 2 +- docs/reference/mur-check-did-you-mean.md | 8 +- docs/specs/061-in-build-did-you-mean.md | 27 +- .../skills/reactor-build-and-check/SKILL.md | 2 +- .../ColorStringClassifier.cs | 17 +- .../ColorStringLiteralAnalyzer.cs | 73 +++- .../ColorStringLiteralCodeFix.cs | 15 +- .../Check/Rules/ColorStringClassifier.cs | 335 ------------------ .../Check/Rules/ColorStringLiteralRule.cs | 178 ++-------- .../ColorStringLiteralAnalyzerTests.cs | 109 +++++- .../AnalyzerTests/ColorStringParityTests.cs | 50 ++- .../Rules/ColorStringLiteralRuleTests.cs | 34 +- .../Rules/RuleTargetResolutionTests.cs | 2 +- tests/Reactor.Tests/PerfDiffSkipPathTests.cs | 11 + 15 files changed, 301 insertions(+), 564 deletions(-) delete mode 100644 src/Reactor.Cli/Check/Rules/ColorStringClassifier.cs diff --git a/docs/_pipeline/templates/analyzer-architecture.md.dt b/docs/_pipeline/templates/analyzer-architecture.md.dt index a76c71a06..09ba2e011 100644 --- a/docs/_pipeline/templates/analyzer-architecture.md.dt +++ b/docs/_pipeline/templates/analyzer-architecture.md.dt @@ -179,7 +179,7 @@ sibling `src/Reactor.Analyzers.Internal/` project instead; see | `REACTOR_DYM_003` | Warning | Mistyped Reactor factory name in call position (e.g. `Buton(...)`) | `FuzzyFactoryNameAnalyzer.cs` | | `REACTOR_DYM_004` | Warning | Reactor factory called with too few arguments (CS7036), single unique overload — suggests the parameter shape | `MissingFactoryArgumentAnalyzer.cs` | | `REACTOR_DYM_005` | Warning | String passed where a Reactor `Element` is expected (CS1503) — wrap it in a text factory | `StringForElementArgumentAnalyzer.cs` | -| `REACTOR_DYM_006` | Warning | Constant color string `BrushHelper` can't parse, so it renders gray (e.g. `.Background("orange")`, `.Foreground("#888")`, a font name under a `*FontFamily` resource key) — suggests the hex value or `new FontFamily(...)` | `ColorStringLiteralAnalyzer.cs` | +| `REACTOR_DYM_006` | Warning | Constant color string `BrushHelper` can't parse, so it renders gray (e.g. `.Background("orange")`, `.Foreground("#888")`, a font name under a `*FontFamily` resource key) — suggests the hex value (or `new FontFamily(...)` once that `ResourceBuilder.Set` overload exists) | `ColorStringLiteralAnalyzer.cs` | `REACTOR_HOOKS_002` and `_003` were the reserved control-flow / data-flow slots (variable hook counts across early returns, async boundaries inside `UseEffect`); diff --git a/docs/guide/analyzer-architecture.md b/docs/guide/analyzer-architecture.md index f3d0876fa..17d4a8e8a 100644 --- a/docs/guide/analyzer-architecture.md +++ b/docs/guide/analyzer-architecture.md @@ -205,7 +205,7 @@ sibling `src/Reactor.Analyzers.Internal/` project instead; see | `REACTOR_DYM_003` | Warning | Mistyped Reactor factory name in call position (e.g. `Buton(...)`) | `FuzzyFactoryNameAnalyzer.cs` | | `REACTOR_DYM_004` | Warning | Reactor factory called with too few arguments (CS7036), single unique overload — suggests the parameter shape | `MissingFactoryArgumentAnalyzer.cs` | | `REACTOR_DYM_005` | Warning | String passed where a Reactor `Element` is expected (CS1503) — wrap it in a text factory | `StringForElementArgumentAnalyzer.cs` | -| `REACTOR_DYM_006` | Warning | Constant color string `BrushHelper` can't parse, so it renders gray (e.g. `.Background("orange")`, `.Foreground("#888")`, a font name under a `*FontFamily` resource key) — suggests the hex value or `new FontFamily(...)` | `ColorStringLiteralAnalyzer.cs` | +| `REACTOR_DYM_006` | Warning | Constant color string `BrushHelper` can't parse, so it renders gray (e.g. `.Background("orange")`, `.Foreground("#888")`, a font name under a `*FontFamily` resource key) — suggests the hex value (or `new FontFamily(...)` once that `ResourceBuilder.Set` overload exists) | `ColorStringLiteralAnalyzer.cs` | `REACTOR_HOOKS_002` and `_003` were the reserved control-flow / data-flow slots (variable hook counts across early returns, async boundaries inside `UseEffect`); diff --git a/docs/reference/mur-check-did-you-mean.md b/docs/reference/mur-check-did-you-mean.md index 14fe7f119..8b060bed2 100644 --- a/docs/reference/mur-check-did-you-mean.md +++ b/docs/reference/mur-check-did-you-mean.md @@ -109,8 +109,8 @@ mur check │ fuzzy-match against the real Reactor API surface. │ │ Tier 3 — induced + vocabulary rules (SHIPPED in part, Phase 3) - │ Small symbol-bound rewriters. Six rules live today - │ (four Class-A induced + two Class-B vocabulary). + │ Small symbol-bound rewriters. Seven rules live today + │ (4 Class-A + 2 Class-B + 1 analyzer mirror). │ Rules can cover diagnostic codes Tier-2 doesn't │ (e.g. CS1955). Rules WIN over Tier-2 when both match. │ @@ -178,7 +178,7 @@ Each rule passes a six-bar **Validation Gate** before merge (frequency, cross-ag | `GridSizePxRenameRule` | A | CS0117 | `GridSize` | `GridSize.Px(...)` for `Pixel`/`Pixels`/`Fixed` legacy names | 9 | | `TextBlockStyleHintRule` | A | CS1061 / CS0117 | `TextBlockElement` | fluent text helpers (`.FontSize`, `.SemiBold`, …); Reactor has no `Style` member | 5 across both `.Style(...)` and `with { Style = ... }` shapes | | `ThemeRawResourceKeyRule` | A | CS0117 | `Theme` | real `Theme.*` aliases (`Theme.LayerFill`, `Theme.Accent`, …) for WinUI `FillColor` ThemeResource key names | run5 opus-4.7 only; bar #2 partial | -| `ColorStringLiteralRule` | analyzer mirror | `REACTOR_DYM_006` | `BrushHelper` / `ResourceBuilder` / `ElementExtensions` | the hex for an unparseable constant color string (`"orange"` → `"#FFA500"`, `"#888"` → `"#888888"`), or `new FontFamily("…")` under a `*FontFamily` resource key. **First rule anchored on an analyzer ID**: the mistake compiles, so there is no CS code to hang it on (issue #1375) | n/a — mirrors the in-build analyzer | +| `ColorStringLiteralRule` | analyzer mirror | `REACTOR_DYM_006` | `BrushHelper` / `ResourceBuilder` / `ElementExtensions` | the hex for an unparseable constant color string (`"orange"` → `"#FFA500"`, `"#888"` → `"#888888"`), or `new FontFamily("…")` under a `*FontFamily` resource key when that `ResourceBuilder.Set` overload exists. **First rule anchored on an analyzer ID**: the mistake compiles, so there is no CS code to hang it on (issue #1375); the rewrite is read from the analyzer's message | n/a — mirrors the in-build analyzer | ### Suggest-gate carve-out for Tier-3 rules @@ -504,7 +504,7 @@ Branch: `eval/spec-038-ec3-2026-05-11` at commit `2b7090f`. Six rules + the two - `RuleSymbolResolver.cs` — `ResolveType(string)`, `ResolveMethod(INamedTypeSymbol, string)`, `ResolveMember(INamedTypeSymbol, string)`. Cached via `ConditionalWeakTable` so two callers with the same compilation reference share caches; per-compilation resolver identity is test-locked. - CLI: `--disable-rule ` (repeatable), `--list-rules` (prints name/provenance/status table; short-circuits before `dotnet build` runs). Unknown `--disable-rule` names warn instead of error. -**The six rules** — see the table in §3 for the full inventory. Four Class-A (induced from the cross-corpus audit) + two Class-B (vocabulary-translation, structurally justified). All six bind through `RuleSymbolResolver`; no string-matching. +**The six rules** — see the table in §3 for the full inventory. Four Class-A (induced from the cross-corpus audit) + two Class-B (vocabulary-translation, structurally justified). All six bind through `RuleSymbolResolver`; no string-matching. A seventh, `ColorStringLiteralRule` (issue #1375), landed later on `main`: it mirrors the in-build `REACTOR_DYM_006` analyzer rather than a corpus cluster, anchors on that analyzer ID, and reads its rewrite from the analyzer's message. **Two critical correctness fixes** (both blocked any real-world rule firing prior; surfaced by end-to-end smoke testing during Phase 3): diff --git a/docs/specs/061-in-build-did-you-mean.md b/docs/specs/061-in-build-did-you-mean.md index 656556afb..b74f9c745 100644 --- a/docs/specs/061-in-build-did-you-mean.md +++ b/docs/specs/061-in-build-did-you-mean.md @@ -342,7 +342,9 @@ made Warning safe under `TreatWarningsAsErrors`. This one fires on code that com in another namespace never matches. `D3Charts.Brush(string)` is out of scope: it parses with `D3Color`. - **Values.** Only compile-time constants: literals, `const`s, constant concatenation, and each constant branch of a `?:` / `??` / switch expression. A wholly-constant expression is checked as its single value, - so a dead branch is never reported. Runtime strings are out of reach; a runtime throw was considered and + and the walk also prunes branches the compiler doesn't fold but a constant rules out (a `const bool` + condition, a constant non-null `??` left side, a constant-valued switch over constant patterns), so a dead + branch is never reported. Runtime strings are out of reach; a runtime throw was considered and left as a separate decision (it would change behaviour for apps relying on the fallback). - **Grammar parity.** The analyzer can't reference `src/Reactor`, so `ColorStringClassifier` carries a netstandard2.0 copy of the grammar, including its quirks (`TrimStart('#')` accepts `##FF0000`; @@ -350,24 +352,27 @@ made Warning safe under `TreatWarningsAsErrors`. This one fires on code that com `TryParseColor` (no behaviour change) so `ColorStringParityTests` can compare exactly — a plain "result is gray" oracle can't tell an unparseable string from a legitimate `"gray"`. - **Did-you-mean, in priority order.** (1) a `ResourceBuilder.Set` key ending in `FontFamily` suggests - `new FontFamily("…")`, but **only when** `Set(string, FontFamily)` is present in the compilation; without it - the message says the overload only stores color brushes and no fix is offered. (2) `#RGB` / `#ARGB` + `new FontFamily("…")`, but **only when** `Set(string, FontFamily)` is present in the compilation and the + value is the whole argument (it can't replace one branch of a `?:`); otherwise the message says the overload + only stores color brushes, points at the `.FontFamily("…")` element modifier, and offers no fix. (2) `#RGB` / `#ARGB` shorthand → expanded `#RRGGBB` / `#AARRGGBB`. (3) six or eight hex digits without `#` → add it. (4) other `#…` → "must be `#RRGGBB` or `#AARRGGBB`". (5) a standard CSS / WinUI named color (139 entries, including the `grey` spellings) → its exact hex, cross-checked against `System.Drawing.KnownColor`. (6) anything else → the supported list, hex, or a `Theme` token. - **Fix.** Cases 1–3 and 5 carry the replacement in the property bag. Literal fixes rewrite the string literal (including one inside a `?:` branch); the FontFamily fix wraps the whole argument (a literal or a - `const` reference) in a `Simplifier`-annotated `new FontFamily(...)`. + `const` reference) in a `Simplifier`-annotated `new FontFamily(...)`, renaming a named `color:` argument to + the FontFamily overload's parameter so the call still binds. **`mur check` mirror.** `ColorStringLiteralRule` is the first Tier-3 rule anchored on an **analyzer** ID rather than a CS code — there is no compiler diagnostic to augment. `mur check` wraps `dotnet build`, which -runs the analyzer bundle, so `REACTOR_DYM_006` appears in the parsed output and the rule adds the same -rewrite as a `→ try:` suffix. It stays silent when the argument's unparseable branches disagree on a -rewrite, or when there is none. There is deliberately no Tier-1 `HintFor` entry, which would mask the -suggestion. `ColorStringParityTests` locks the two classifier copies together, and -`DidYouMeanAnalyzerParityTests` feeds real analyzer diagnostics to the rule and asserts it suggests the same -rewrite as the code fix. +runs the analyzer bundle, so `REACTOR_DYM_006` appears in the parsed output. Every fixable message ends in +`did you mean X?`, computed for the exact value the diagnostic points at, so the rule lifts `X` out of the +message rather than re-deriving it. It has no grammar of its own and can't disagree with the code fix, and +the `→ try:` suffix keeps the rewrite visible after `mur check` truncates the message. Messages without a +did-you-mean stay silent. There is deliberately no Tier-1 `HintFor` entry, which would mask the suggestion. +`DidYouMeanAnalyzerParityTests` feeds real analyzer diagnostics to the rule and asserts it suggests the +code fix's rewrite; `ColorStringParityTests` pins the `did you mean` suffix. **Overlap with `REACTOR_THEME_001`.** THEME_001 already flags every string literal passed to `.Background` / `.Foreground` / `.WithBorder` as "use a Theme token". That is a style nudge (and is downgraded to @@ -380,7 +385,7 @@ theme, it renders gray. Both can fire on one literal. |---|---| | False positives under cascading / partial-edit errors | Bail when the receiver type is unresolved or an error type; only fire when the invocation didn't bind; gate to Reactor symbols; (fuzzy phases) require high similarity thresholds. | | IDE typing latency | Use the provided `context.SemanticModel` (never `Compilation.GetSemanticModel`, RS1030); narrow syntax-node actions; cheap symbol pre-checks before any fuzzy match. | -| Divergence between analyzer and `mur check` | Phase 2: local maps locked to the CLI Tier-3 rules by a cross-check parity test (`DidYouMeanAnalyzerParityTests`). Argument-shape phase (§7.2): the same test asserts `REACTOR_DYM_004`/`_005` and the CLI `SymbolSuggester` both fire on the representative CS7036 / CS1503 shapes. `REACTOR_DYM_006` (§7.3): `ColorStringParityTests` locks `BrushHelper.TryParseColor` and both classifier copies together, and the rule is driven by real analyzer output. Fuzzy phases: one shared match library + shared fixtures (§7 item 5). | +| Divergence between analyzer and `mur check` | Phase 2: local maps locked to the CLI Tier-3 rules by a cross-check parity test (`DidYouMeanAnalyzerParityTests`). Argument-shape phase (§7.2): the same test asserts `REACTOR_DYM_004`/`_005` and the CLI `SymbolSuggester` both fire on the representative CS7036 / CS1503 shapes. `REACTOR_DYM_006` (§7.3): `ColorStringParityTests` locks `BrushHelper.TryParseColor` and the analyzer's classifier together; the rule reads the analyzer's own message, and a live-assembly test pins the analyzer's targets to the real Reactor surface. Fuzzy phases: one shared match library + shared fixtures (§7 item 5). | | Noise (a second diagnostic atop the CS error) | Concise message; `REACTOR_DYM_001`–`_005` only ever accompany a genuine compile error; consumers can tune severity via `.editorconfig`. `REACTOR_DYM_006` is the exception (§7.3) and fires only on a value that provably renders gray. | ## §9 Relationship to `mur check` diff --git a/plugins/reactor/skills/reactor-build-and-check/SKILL.md b/plugins/reactor/skills/reactor-build-and-check/SKILL.md index 2326caf94..18294109b 100644 --- a/plugins/reactor/skills/reactor-build-and-check/SKILL.md +++ b/plugins/reactor/skills/reactor-build-and-check/SKILL.md @@ -142,7 +142,7 @@ Additional flags: | `REACTOR_DYM_003` | warning | An unresolved bare call closely matches a Reactor factory name (e.g. `Buton(...)`) — pairs with compiler `CS0103` | Rename to the suggested factory: `Button(...)`. Fires only on a close, unambiguous factory match (the typo flavour of `CS0103`); the IDE offers a one-click rename. | | `REACTOR_DYM_004` | warning | A Reactor factory is called with too few arguments (e.g. `ScrollViewer()`) — pairs with compiler `CS7036` | Supply the missing argument(s); the message lists the factory's full parameter shape as named arguments. Fires only when a single overload uniquely matches, so multi-overload factories (e.g. `Button()`) are deliberately left to the raw compiler error. Message only — no code fix (the `` placeholder wouldn't compile). | | `REACTOR_DYM_005` | warning | A `string` is passed where a Reactor `Element` is expected (e.g. `ScrollViewer("hi")`) — pairs with compiler `CS1503` | Wrap the string in a text factory: `ScrollViewer(TextBlock("hi"))` (or `Heading`/`Caption`). Narrow, high-confidence special case only; general type mismatches degrade to the raw `CS1503`. Message only — no code fix. | -| `REACTOR_DYM_006` | warning | A constant color string `BrushHelper` can't parse (e.g. `.Background("orange")`, `.Foreground("#888")`, `.Resources(r => r.Set("ContentControlThemeFontFamily", "Consolas"))`). It compiles and silently renders **gray**. | Color strings accept only `red`, `green`, `blue`, `white`, `black`, `gray`/`grey`, `lightgray`/`lightgrey`, `transparent`, `#RRGGBB` and `#AARRGGBB`. Use the suggested hex (`"orange"` → `"#FFA500"`, `"#888"` → `"#888888"`, `"FF0000"` → `"#FF0000"`) or a `Theme` token. A font under a `*FontFamily` key needs `new FontFamily("…")` (when `ResourceBuilder.Set(string, FontFamily)` exists). The IDE fix and `mur check`'s `→ try:` apply the same rewrite. | +| `REACTOR_DYM_006` | warning | A constant color string `BrushHelper` can't parse (e.g. `.Background("orange")`, `.Foreground("#888")`, `.Resources(r => r.Set("ContentControlThemeFontFamily", "Consolas"))`). It compiles and silently renders **gray**. | Color strings accept only `red`, `green`, `blue`, `white`, `black`, `gray`/`grey`, `lightgray`/`lightgrey`, `transparent`, `#RRGGBB` and `#AARRGGBB`. Use the suggested hex (`"orange"` → `"#FFA500"`, `"#888"` → `"#888888"`, `"FF0000"` → `"#FF0000"`) or a `Theme` token; the IDE fix and `mur check`'s `→ try:` apply the same rewrite. A font name under a `*FontFamily` resource key is never a color: set the font with the `.FontFamily("…")` modifier on the element instead. | | `CS0103` | error | "The name 'X' does not exist in the current context" | Missing `using` — most often `Microsoft.UI.Reactor.Layout` (FlexAlign), `Microsoft.UI.Xaml.Controls` (InfoBarSeverity, Orientation), or `static Microsoft.UI.Reactor.Factories`. (A close-but-wrong factory name instead raises `REACTOR_DYM_003` above.) | | `CS1061` | error | "'X' does not contain a definition for 'Y'" | A type-specific modifier called on an element type that does not declare it — `.Bold()` / `.FontSize()` / `.TextWrapping()` are `TextBlockElement`-only, so `Button("Go").Bold()` fails. This is **not** an ordering problem: modifiers are generic (` where T : Element`) and preserve the concrete type, so `.Margin(16).Bold()` and `.Bold().Margin(16)` compile alike. Check the receiver's element type against `reactor-dsl/references/reactor.api.txt`. | | `CS0117` | error | "'Element' does not contain a definition for X" | You're calling a factory or static member that doesn't exist — confirm the name against `reactor-dsl/references/reactor.api.txt`. (For a missing *instance* modifier see `CS1061` above.) | diff --git a/src/Reactor.Analyzers/ColorStringClassifier.cs b/src/Reactor.Analyzers/ColorStringClassifier.cs index 881b6d11b..bda4eb0d4 100644 --- a/src/Reactor.Analyzers/ColorStringClassifier.cs +++ b/src/Reactor.Analyzers/ColorStringClassifier.cs @@ -46,14 +46,15 @@ public ColorStringFinding(ColorStringIssue issue, string? replacement, string de /// /// /// is a netstandard2.0 copy of Microsoft.UI.Reactor.BrushHelper.TryParseColor -/// (the analyzer cannot reference src/Reactor), and the whole class is mirrored -/// byte-for-byte by the mur check CLI's ColorStringClassifier. ColorStringParityTests -/// locks all three together — edit them in lockstep. +/// (the analyzer cannot reference src/Reactor); ColorStringParityTests locks the two +/// together — edit them in lockstep. Every fixable ends in +/// did you mean X?: the mur check ColorStringLiteralRule lifts X out of the +/// diagnostic message, so keep that suffix stable. /// internal static class ColorStringClassifier { /// The names BrushHelper's switch recognizes (case-insensitive). - public const string SupportedNames = "red, green, blue, white, black, gray, lightgray, transparent"; + public const string SupportedNames = "red, green, blue, white, black, gray/grey, lightgray/lightgrey, transparent"; /// The suffix that marks a WinUI FontFamily resource key (e.g. ContentControlThemeFontFamily). public const string FontFamilyKeySuffix = "FontFamily"; @@ -261,11 +262,11 @@ private static bool IsByte(string hex, int start) => ? new ColorStringFinding( ColorStringIssue.FontFamilyKey, value, - $"the key '{resourceKey}' names a FontFamily resource; did you mean new FontFamily(\"{value}\")?") + $"'{resourceKey}' is a FontFamily resource; did you mean new FontFamily(\"{value}\")?") : new ColorStringFinding( ColorStringIssue.FontFamilyKey, null, - $"the key '{resourceKey}' names a FontFamily resource, but Set(string, string) only stores color brushes"); + $"'{resourceKey}' is a FontFamily resource, but this Set overload only stores color brushes; to change the font, use .FontFamily(\"{value}\") on the element"); } if (value.StartsWith("#", StringComparison.Ordinal)) @@ -277,7 +278,7 @@ private static bool IsByte(string hex, int start) => return new ColorStringFinding( ColorStringIssue.HexShorthand, expanded, - $"BrushHelper has no #RGB/#ARGB shorthand; did you mean \"{expanded}\"?"); + $"#RGB/#ARGB shorthand isn't supported; did you mean \"{expanded}\"?"); } return new ColorStringFinding( ColorStringIssue.MalformedHex, @@ -299,7 +300,7 @@ private static bool IsByte(string hex, int start) => return new ColorStringFinding( ColorStringIssue.NamedColor, hex, - $"BrushHelper only knows {SupportedNames}; did you mean \"{hex}\"?"); + $"the only built-in names are {SupportedNames}; did you mean \"{hex}\"?"); } return new ColorStringFinding( diff --git a/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs b/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs index 3218f9673..d9e16d68a 100644 --- a/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs +++ b/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs @@ -59,10 +59,16 @@ public sealed class ColorStringLiteralAnalyzer : DiagnosticAnalyzer /// Property-bag key: the replacement value (literal text, or family name). Absent when there is no fix. public const string ReplacementProperty = "Replacement"; + /// + /// Property-bag key (FontFamily fix only): the value parameter name of + /// ResourceBuilder.Set(string, FontFamily), so a named color: argument can be renamed. + /// + public const string FontFamilyParameterProperty = "FontFamilyParameter"; + private static readonly LocalizableString Title = - "Color string that BrushHelper cannot parse renders gray"; + "Unrecognized color string renders gray"; private static readonly LocalizableString MessageFormat = - "'{0}' is not a color BrushHelper can parse, so it renders gray; {1}"; + "'{0}' is not a color string Reactor can parse, so it renders gray — {1}"; private static readonly LocalizableString Description = "BrushHelper.Parse understands only red, green, blue, white, black, gray/grey, lightgray/lightgrey, transparent, #RRGGBB and #AARRGGBB. Anything else silently falls back to a gray brush. This covers BrushHelper.Parse, ResourceBuilder.Set(string, string), and the string overloads of the Background/Foreground/BorderBrush/WithBorder modifiers."; @@ -98,16 +104,18 @@ public override void Initialize(AnalysisContext context) /// The resolved color-string APIs for one compilation. internal sealed class Targets { - private Targets(ImmutableHashSet methods, IMethodSymbol? resourceSet, bool fontFamilyOverloadExists) + private Targets(ImmutableHashSet methods, IMethodSymbol? resourceSet, string? fontFamilyParameterName) { Methods = methods; ResourceSet = resourceSet; - FontFamilyOverloadExists = fontFamilyOverloadExists; + FontFamilyParameterName = fontFamilyParameterName; } public ImmutableHashSet Methods { get; } public IMethodSymbol? ResourceSet { get; } - public bool FontFamilyOverloadExists { get; } + + /// The value parameter of ResourceBuilder.Set(string, FontFamily), or null when that overload doesn't exist. + public string? FontFamilyParameterName { get; } public static Targets? Resolve(Compilation compilation) { @@ -123,7 +131,7 @@ private Targets(ImmutableHashSet methods, IMethodSymbol? resource } IMethodSymbol? resourceSet = null; - var fontFamilyOverload = false; + string? fontFamilyParameter = null; var resourceBuilder = compilation.GetTypeByMetadataName(ResourceBuilderMetadataName); if (resourceBuilder is not null) { @@ -138,7 +146,7 @@ private Targets(ImmutableHashSet methods, IMethodSymbol? resource } else if (m.Parameters[1].Type.ToDisplayString() == FontFamilyMetadataName) { - fontFamilyOverload = true; + fontFamilyParameter = m.Parameters[1].Name; } } } @@ -152,7 +160,7 @@ private Targets(ImmutableHashSet methods, IMethodSymbol? resource methods.Add(m); } - return methods.Count == 0 ? null : new Targets(methods.ToImmutable(), resourceSet, fontFamilyOverload); + return methods.Count == 0 ? null : new Targets(methods.ToImmutable(), resourceSet, fontFamilyParameter); } private static bool HasStringColorParameter(IMethodSymbol method, ITypeSymbol str) @@ -215,7 +223,10 @@ private static void AnalyzeInvocation(SyntaxNodeAnalysisContext context, Targets foreach (var (expression, value) in ConstantStrings(colorArg.Value.Syntax as ExpressionSyntax, context.SemanticModel, context.CancellationToken)) { - var finding = ColorStringClassifier.Classify(value, resourceKey, targets.FontFamilyOverloadExists); + // new FontFamily(...) can only replace the whole argument: inside a ?: branch the other + // branch would still be a string and the expression would stop type-checking. + var canUseFontFamily = targets.FontFamilyParameterName is not null && expression.Parent is ArgumentSyntax; + var finding = ColorStringClassifier.Classify(value, resourceKey, canUseFontFamily); if (finding is not { } f) continue; @@ -223,6 +234,8 @@ private static void AnalyzeInvocation(SyntaxNodeAnalysisContext context, Targets properties.Add(IssueProperty, f.Issue.ToString()); if (f.Replacement is not null) properties.Add(ReplacementProperty, f.Replacement); + if (f.Issue == ColorStringIssue.FontFamilyKey && f.Replacement is not null) + properties.Add(FontFamilyParameterProperty, targets.FontFamilyParameterName); context.ReportDiagnostic(Diagnostic.Create( Rule, @@ -236,7 +249,9 @@ private static void AnalyzeInvocation(SyntaxNodeAnalysisContext context, Targets /// /// The constant strings an argument can evaluate to, each with the expression that produced it. /// A wholly-constant expression yields itself; otherwise only value-selecting shapes are - /// walked (parentheses, ?:, ??, switch arms) — never into nested calls. + /// walked (parentheses, ?:, ??, switch arms) — never into nested calls. Branches a + /// constant condition or a constant non-null ?? left side rules out are skipped, so dead + /// code (DebugBuild ? "magenta" : color with a const bool) is never reported. /// internal static IEnumerable<(ExpressionSyntax Expression, string Value)> ConstantStrings( ExpressionSyntax? expression, SemanticModel model, CancellationToken cancellationToken) @@ -255,13 +270,45 @@ private static void AnalyzeInvocation(SyntaxNodeAnalysisContext context, Targets IEnumerable children = expression switch { ParenthesizedExpressionSyntax p => new[] { p.Expression }, - ConditionalExpressionSyntax c => new[] { c.WhenTrue, c.WhenFalse }, - BinaryExpressionSyntax b when b.IsKind(SyntaxKind.CoalesceExpression) => new[] { b.Left, b.Right }, - SwitchExpressionSyntax sw => sw.Arms.Select(a => a.Expression), + ConditionalExpressionSyntax c => model.GetConstantValue(c.Condition, cancellationToken) is { HasValue: true, Value: bool taken } + ? new[] { taken ? c.WhenTrue : c.WhenFalse } + : new[] { c.WhenTrue, c.WhenFalse }, + BinaryExpressionSyntax b when b.IsKind(SyntaxKind.CoalesceExpression) => + model.GetConstantValue(b.Left, cancellationToken) is { HasValue: true, Value: not null } + ? new[] { b.Left } + : new[] { b.Left, b.Right }, + // Arms are only pruned when every pattern is a plain constant and the governing value is a + // constant; anything fancier (relational, type, `when`) keeps every arm. + SwitchExpressionSyntax sw => LiveArms(sw, model, cancellationToken), _ => Array.Empty(), }; foreach (var child in children) foreach (var item in ConstantStrings(child, model, cancellationToken)) yield return item; } + + private static IEnumerable LiveArms(SwitchExpressionSyntax sw, SemanticModel model, CancellationToken cancellationToken) + { + var governing = model.GetConstantValue(sw.GoverningExpression, cancellationToken); + if (!governing.HasValue) + return sw.Arms.Select(a => a.Expression); + + foreach (var arm in sw.Arms) + { + if (arm.WhenClause is not null) + return sw.Arms.Select(a => a.Expression); + switch (arm.Pattern) + { + case DiscardPatternSyntax: + return new[] { arm.Expression }; + case ConstantPatternSyntax cp when model.GetConstantValue(cp.Expression, cancellationToken) is { HasValue: true } armValue: + if (Equals(armValue.Value, governing.Value)) + return new[] { arm.Expression }; + break; + default: + return sw.Arms.Select(a => a.Expression); + } + } + return Array.Empty(); + } } diff --git a/src/Reactor.Analyzers/ColorStringLiteralCodeFix.cs b/src/Reactor.Analyzers/ColorStringLiteralCodeFix.cs index be749e30f..a434a1505 100644 --- a/src/Reactor.Analyzers/ColorStringLiteralCodeFix.cs +++ b/src/Reactor.Analyzers/ColorStringLiteralCodeFix.cs @@ -64,10 +64,23 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) SyntaxFactory.Argument(expression.WithoutTrivia())))) .WithTriviaFrom(expression); + // `color: "Consolas"` must become `: new FontFamily(...)`, or the call + // would no longer bind to the FontFamily overload. + var oldArgument = (ArgumentSyntax)expression.Parent!; + var newArgument = oldArgument.WithExpression(creation); + if (oldArgument.NameColon is { } nameColon) + { + if (!diagnostic.Properties.TryGetValue(ColorStringLiteralAnalyzer.FontFamilyParameterProperty, out var parameterName) + || string.IsNullOrEmpty(parameterName)) + continue; + newArgument = newArgument.WithNameColon( + nameColon.WithName(SyntaxFactory.IdentifierName(parameterName!).WithTriviaFrom(nameColon.Name))); + } + context.RegisterCodeFix( CodeAction.Create( $"Use new FontFamily(\"{replacement}\")", - _ => Task.FromResult(context.Document.WithSyntaxRoot(root.ReplaceNode(expression, creation))), + _ => Task.FromResult(context.Document.WithSyntaxRoot(root.ReplaceNode(oldArgument, newArgument))), equivalenceKey: FontFamilyEquivalenceKey), diagnostic); continue; diff --git a/src/Reactor.Cli/Check/Rules/ColorStringClassifier.cs b/src/Reactor.Cli/Check/Rules/ColorStringClassifier.cs deleted file mode 100644 index 0054f55ca..000000000 --- a/src/Reactor.Cli/Check/Rules/ColorStringClassifier.cs +++ /dev/null @@ -1,335 +0,0 @@ -// Mirror of src/Reactor.Analyzers/ColorStringClassifier.cs — see the remarks below. - -using System; -using System.Collections.Generic; -using System.Collections.Immutable; -using System.Globalization; - -namespace Microsoft.UI.Reactor.Cli.Check.Rules; - -/// What is wrong with a color string that BrushHelper cannot parse. -internal enum ColorStringIssue -{ - /// A font name passed to ResourceBuilder.Set under a *FontFamily key. - FontFamilyKey, - /// #RGB / #ARGB shorthand, which BrushHelper does not expand. - HexShorthand, - /// Six or eight hex digits without the leading #. - MissingHash, - /// Starts with # but is not #RRGGBB / #AARRGGBB. - MalformedHex, - /// A standard CSS / WinUI named color that BrushHelper's 8-name table lacks. - NamedColor, - /// Anything else. - Unknown, -} - -/// -/// The did-you-mean for one unparseable color string. is the new string -/// literal value for the literal fixes, the family name for -/// (only when the FontFamily overload exists), or when there is no fix. -/// -internal readonly struct ColorStringFinding -{ - public ColorStringFinding(ColorStringIssue issue, string? replacement, string detail) - { - Issue = issue; - Replacement = replacement; - Detail = detail; - } - - public ColorStringIssue Issue { get; } - public string? Replacement { get; } - public string Detail { get; } -} - -/// -/// Classifies color strings for the mur check (mirror of REACTOR_DYM_006). -/// -/// -/// A copy of the REACTOR_DYM_006 analyzer's Microsoft.UI.Reactor.Analyzers.ColorStringClassifier -/// (the netstandard2.0 analyzer and this CLI cannot share a project), whose in turn -/// mirrors Microsoft.UI.Reactor.BrushHelper.TryParseColor. ColorStringParityTests locks all -/// three together — edit them in lockstep. -/// -internal static class ColorStringClassifier -{ - /// The names BrushHelper's switch recognizes (case-insensitive). - public const string SupportedNames = "red, green, blue, white, black, gray, lightgray, transparent"; - - /// The suffix that marks a WinUI FontFamily resource key (e.g. ContentControlThemeFontFamily). - public const string FontFamilyKeySuffix = "FontFamily"; - - /// - /// Standard CSS / WinUI (Microsoft.UI.Colors) named colors that BrushHelper does not - /// recognize, mapped to their exact #RRGGBB. The 8 names BrushHelper does support are excluded. - /// - public static readonly ImmutableDictionary NamedColorHex = - ImmutableDictionary.CreateRange(StringComparer.OrdinalIgnoreCase, new[] - { - new KeyValuePair("aliceblue", "#F0F8FF"), - new KeyValuePair("antiquewhite", "#FAEBD7"), - new KeyValuePair("aqua", "#00FFFF"), - new KeyValuePair("aquamarine", "#7FFFD4"), - new KeyValuePair("azure", "#F0FFFF"), - new KeyValuePair("beige", "#F5F5DC"), - new KeyValuePair("bisque", "#FFE4C4"), - new KeyValuePair("blanchedalmond", "#FFEBCD"), - new KeyValuePair("blueviolet", "#8A2BE2"), - new KeyValuePair("brown", "#A52A2A"), - new KeyValuePair("burlywood", "#DEB887"), - new KeyValuePair("cadetblue", "#5F9EA0"), - new KeyValuePair("chartreuse", "#7FFF00"), - new KeyValuePair("chocolate", "#D2691E"), - new KeyValuePair("coral", "#FF7F50"), - new KeyValuePair("cornflowerblue", "#6495ED"), - new KeyValuePair("cornsilk", "#FFF8DC"), - new KeyValuePair("crimson", "#DC143C"), - new KeyValuePair("cyan", "#00FFFF"), - new KeyValuePair("darkblue", "#00008B"), - new KeyValuePair("darkcyan", "#008B8B"), - new KeyValuePair("darkgoldenrod", "#B8860B"), - new KeyValuePair("darkgray", "#A9A9A9"), - new KeyValuePair("darkgreen", "#006400"), - new KeyValuePair("darkgrey", "#A9A9A9"), - new KeyValuePair("darkkhaki", "#BDB76B"), - new KeyValuePair("darkmagenta", "#8B008B"), - new KeyValuePair("darkolivegreen", "#556B2F"), - new KeyValuePair("darkorange", "#FF8C00"), - new KeyValuePair("darkorchid", "#9932CC"), - new KeyValuePair("darkred", "#8B0000"), - new KeyValuePair("darksalmon", "#E9967A"), - new KeyValuePair("darkseagreen", "#8FBC8F"), - new KeyValuePair("darkslateblue", "#483D8B"), - new KeyValuePair("darkslategray", "#2F4F4F"), - new KeyValuePair("darkslategrey", "#2F4F4F"), - new KeyValuePair("darkturquoise", "#00CED1"), - new KeyValuePair("darkviolet", "#9400D3"), - new KeyValuePair("deeppink", "#FF1493"), - new KeyValuePair("deepskyblue", "#00BFFF"), - new KeyValuePair("dimgray", "#696969"), - new KeyValuePair("dimgrey", "#696969"), - new KeyValuePair("dodgerblue", "#1E90FF"), - new KeyValuePair("firebrick", "#B22222"), - new KeyValuePair("floralwhite", "#FFFAF0"), - new KeyValuePair("forestgreen", "#228B22"), - new KeyValuePair("fuchsia", "#FF00FF"), - new KeyValuePair("gainsboro", "#DCDCDC"), - new KeyValuePair("ghostwhite", "#F8F8FF"), - new KeyValuePair("gold", "#FFD700"), - new KeyValuePair("goldenrod", "#DAA520"), - new KeyValuePair("greenyellow", "#ADFF2F"), - new KeyValuePair("honeydew", "#F0FFF0"), - new KeyValuePair("hotpink", "#FF69B4"), - new KeyValuePair("indianred", "#CD5C5C"), - new KeyValuePair("indigo", "#4B0082"), - new KeyValuePair("ivory", "#FFFFF0"), - new KeyValuePair("khaki", "#F0E68C"), - new KeyValuePair("lavender", "#E6E6FA"), - new KeyValuePair("lavenderblush", "#FFF0F5"), - new KeyValuePair("lawngreen", "#7CFC00"), - new KeyValuePair("lemonchiffon", "#FFFACD"), - new KeyValuePair("lightblue", "#ADD8E6"), - new KeyValuePair("lightcoral", "#F08080"), - new KeyValuePair("lightcyan", "#E0FFFF"), - new KeyValuePair("lightgoldenrodyellow", "#FAFAD2"), - new KeyValuePair("lightgreen", "#90EE90"), - new KeyValuePair("lightpink", "#FFB6C1"), - new KeyValuePair("lightsalmon", "#FFA07A"), - new KeyValuePair("lightseagreen", "#20B2AA"), - new KeyValuePair("lightskyblue", "#87CEFA"), - new KeyValuePair("lightslategray", "#778899"), - new KeyValuePair("lightslategrey", "#778899"), - new KeyValuePair("lightsteelblue", "#B0C4DE"), - new KeyValuePair("lightyellow", "#FFFFE0"), - new KeyValuePair("lime", "#00FF00"), - new KeyValuePair("limegreen", "#32CD32"), - new KeyValuePair("linen", "#FAF0E6"), - new KeyValuePair("magenta", "#FF00FF"), - new KeyValuePair("maroon", "#800000"), - new KeyValuePair("mediumaquamarine", "#66CDAA"), - new KeyValuePair("mediumblue", "#0000CD"), - new KeyValuePair("mediumorchid", "#BA55D3"), - new KeyValuePair("mediumpurple", "#9370DB"), - new KeyValuePair("mediumseagreen", "#3CB371"), - new KeyValuePair("mediumslateblue", "#7B68EE"), - new KeyValuePair("mediumspringgreen", "#00FA9A"), - new KeyValuePair("mediumturquoise", "#48D1CC"), - new KeyValuePair("mediumvioletred", "#C71585"), - new KeyValuePair("midnightblue", "#191970"), - new KeyValuePair("mintcream", "#F5FFFA"), - new KeyValuePair("mistyrose", "#FFE4E1"), - new KeyValuePair("moccasin", "#FFE4B5"), - new KeyValuePair("navajowhite", "#FFDEAD"), - new KeyValuePair("navy", "#000080"), - new KeyValuePair("oldlace", "#FDF5E6"), - new KeyValuePair("olive", "#808000"), - new KeyValuePair("olivedrab", "#6B8E23"), - new KeyValuePair("orange", "#FFA500"), - new KeyValuePair("orangered", "#FF4500"), - new KeyValuePair("orchid", "#DA70D6"), - new KeyValuePair("palegoldenrod", "#EEE8AA"), - new KeyValuePair("palegreen", "#98FB98"), - new KeyValuePair("paleturquoise", "#AFEEEE"), - new KeyValuePair("palevioletred", "#DB7093"), - new KeyValuePair("papayawhip", "#FFEFD5"), - new KeyValuePair("peachpuff", "#FFDAB9"), - new KeyValuePair("peru", "#CD853F"), - new KeyValuePair("pink", "#FFC0CB"), - new KeyValuePair("plum", "#DDA0DD"), - new KeyValuePair("powderblue", "#B0E0E6"), - new KeyValuePair("purple", "#800080"), - new KeyValuePair("rebeccapurple", "#663399"), - new KeyValuePair("rosybrown", "#BC8F8F"), - new KeyValuePair("royalblue", "#4169E1"), - new KeyValuePair("saddlebrown", "#8B4513"), - new KeyValuePair("salmon", "#FA8072"), - new KeyValuePair("sandybrown", "#F4A460"), - new KeyValuePair("seagreen", "#2E8B57"), - new KeyValuePair("seashell", "#FFF5EE"), - new KeyValuePair("sienna", "#A0522D"), - new KeyValuePair("silver", "#C0C0C0"), - new KeyValuePair("skyblue", "#87CEEB"), - new KeyValuePair("slateblue", "#6A5ACD"), - new KeyValuePair("slategray", "#708090"), - new KeyValuePair("slategrey", "#708090"), - new KeyValuePair("snow", "#FFFAFA"), - new KeyValuePair("springgreen", "#00FF7F"), - new KeyValuePair("steelblue", "#4682B4"), - new KeyValuePair("tan", "#D2B48C"), - new KeyValuePair("teal", "#008080"), - new KeyValuePair("thistle", "#D8BFD8"), - new KeyValuePair("tomato", "#FF6347"), - new KeyValuePair("turquoise", "#40E0D0"), - new KeyValuePair("violet", "#EE82EE"), - new KeyValuePair("wheat", "#F5DEB3"), - new KeyValuePair("whitesmoke", "#F5F5F5"), - new KeyValuePair("yellow", "#FFFF00"), - new KeyValuePair("yellowgreen", "#9ACD32"), - }); - - /// - /// Mirrors BrushHelper.TryParseColor: iff the string does not fall - /// back to gray. - /// - public static bool TryParse(string color) - { - switch (color.ToLowerInvariant()) - { - case "red": - case "green": - case "blue": - case "white": - case "black": - case "gray": - case "grey": - case "lightgray": - case "lightgrey": - case "transparent": - return true; - } - return color.StartsWith("#", StringComparison.Ordinal) && TryParseHex(color); - } - - private static bool TryParseHex(string hex) - { - hex = hex.TrimStart('#'); - if (hex.Length == 6) - return IsByte(hex, 0) && IsByte(hex, 2) && IsByte(hex, 4); - if (hex.Length == 8) - return IsByte(hex, 0) && IsByte(hex, 2) && IsByte(hex, 4) && IsByte(hex, 6); - return false; - } - - // Same call as BrushHelper (`byte.TryParse(hex[i..i+2], NumberStyles.HexNumber, null, out _)`), - // including HexNumber's leading/trailing-whitespace allowance. - private static bool IsByte(string hex, int start) => - byte.TryParse(hex.Substring(start, 2), NumberStyles.HexNumber, null, out _); - - /// - /// Returns the finding for , or when BrushHelper - /// parses it. is the constant key of a ResourceBuilder.Set - /// call (otherwise ); says whether - /// ResourceBuilder.Set(string, FontFamily) is available to rewrite to. - /// - public static ColorStringFinding? Classify(string value, string? resourceKey, bool fontFamilyOverloadExists) - { - if (TryParse(value)) - return null; - - if (resourceKey is not null && resourceKey.EndsWith(FontFamilyKeySuffix, StringComparison.Ordinal)) - { - return fontFamilyOverloadExists - ? new ColorStringFinding( - ColorStringIssue.FontFamilyKey, - value, - $"the key '{resourceKey}' names a FontFamily resource; did you mean new FontFamily(\"{value}\")?") - : new ColorStringFinding( - ColorStringIssue.FontFamilyKey, - null, - $"the key '{resourceKey}' names a FontFamily resource, but Set(string, string) only stores color brushes"); - } - - if (value.StartsWith("#", StringComparison.Ordinal)) - { - var digits = value.Substring(1); - if ((digits.Length == 3 || digits.Length == 4) && IsAllHex(digits)) - { - var expanded = Expand(digits); - return new ColorStringFinding( - ColorStringIssue.HexShorthand, - expanded, - $"BrushHelper has no #RGB/#ARGB shorthand; did you mean \"{expanded}\"?"); - } - return new ColorStringFinding( - ColorStringIssue.MalformedHex, - null, - "hex colors must be #RRGGBB or #AARRGGBB"); - } - - if ((value.Length == 6 || value.Length == 8) && IsAllHex(value)) - { - var hashed = "#" + value; - return new ColorStringFinding( - ColorStringIssue.MissingHash, - hashed, - $"hex colors need a leading '#'; did you mean \"{hashed}\"?"); - } - - if (NamedColorHex.TryGetValue(value, out var hex)) - { - return new ColorStringFinding( - ColorStringIssue.NamedColor, - hex, - $"BrushHelper only knows {SupportedNames}; did you mean \"{hex}\"?"); - } - - return new ColorStringFinding( - ColorStringIssue.Unknown, - null, - $"use #RRGGBB or #AARRGGBB, a Theme token, or one of {SupportedNames}"); - } - - private static bool IsAllHex(string s) - { - foreach (var c in s) - { - var isHex = (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F'); - if (!isHex) - return false; - } - return true; - } - - private static string Expand(string digits) - { - var chars = new char[1 + digits.Length * 2]; - chars[0] = '#'; - for (var i = 0; i < digits.Length; i++) - { - chars[1 + i * 2] = digits[i]; - chars[2 + i * 2] = digits[i]; - } - return new string(chars); - } -} diff --git a/src/Reactor.Cli/Check/Rules/ColorStringLiteralRule.cs b/src/Reactor.Cli/Check/Rules/ColorStringLiteralRule.cs index bde204ef1..094e81200 100644 --- a/src/Reactor.Cli/Check/Rules/ColorStringLiteralRule.cs +++ b/src/Reactor.Cli/Check/Rules/ColorStringLiteralRule.cs @@ -4,177 +4,57 @@ // fine and silently renders gray, so unlike every other Tier-3 rule there is // no compiler error to hang this on. The rule therefore anchors on the // analyzer's own diagnostic ID: `dotnet build` (which `mur check` wraps) runs -// the shipped Reactor.Analyzers bundle, REACTOR_DYM_006 lands in the parsed -// output, and this rule re-derives the concrete rewrite for the `→ try:` -// suffix. It deliberately has NO Tier-1 HintFor entry in CheckCommand — that -// table wins ties and would mask the suggestion. +// the shipped Reactor.Analyzers bundle and REACTOR_DYM_006 lands in the parsed +// output. // -// Parity with the analyzer is locked by DidYouMeanAnalyzerParityTests (same -// shapes → same suggestion) and ColorStringParityTests (the two classifier -// copies agree with each other and with BrushHelper.TryParseColor). +// The analyzer has already done the work: every fixable REACTOR_DYM_006 +// message ends in `did you mean X?`, computed for the exact literal the +// diagnostic points at. The rule lifts X out of that message rather than +// re-deriving it, so it can never disagree with the IDE code fix — and since +// CheckCommand truncates messages to 100 chars, the `→ try:` suffix is what +// keeps the rewrite visible. Messages without a did-you-mean (unknown name, +// malformed hex, FontFamily key without the FontFamily overload) stay silent. +// It deliberately has NO Tier-1 HintFor entry in CheckCommand — that table +// wins ties and would mask the suggestion. +// +// DidYouMeanAnalyzerParityTests feeds real analyzer diagnostics through this +// rule and asserts it suggests the code fix's rewrite. -using Microsoft.CodeAnalysis; -using Microsoft.CodeAnalysis.CSharp; -using Microsoft.CodeAnalysis.CSharp.Syntax; -using Microsoft.CodeAnalysis.Operations; +using System.Text.RegularExpressions; namespace Microsoft.UI.Reactor.Cli.Check.Rules; -internal sealed class ColorStringLiteralRule : IRulePattern +internal sealed partial class ColorStringLiteralRule : IRulePattern { internal const string DiagnosticId = "REACTOR_DYM_006"; const string BrushHelperFqn = "Microsoft.UI.Reactor.BrushHelper"; const string ResourceBuilderFqn = "Microsoft.UI.Reactor.Elements.ResourceBuilder"; const string ElementExtensionsFqn = "Microsoft.UI.Reactor.ElementExtensions"; - const string FontFamilyFqn = "Microsoft.UI.Xaml.Media.FontFamily"; - const string ColorParameterName = "color"; - const string KeyParameterName = "key"; - - static readonly HashSet ModifierNames = new(StringComparer.Ordinal) - { - "Background", "Foreground", "BorderBrush", "WithBorder", - }; public string Name => "ColorStringLiteralRule"; public string Provenance => "analyzer:REACTOR_DYM_006"; public IReadOnlyList DiagnosticCodes { get; } = new[] { DiagnosticId }; public IReadOnlyList DeclaredTargets { get; } = new[] { BrushHelperFqn, ResourceBuilderFqn, ElementExtensionsFqn }; + // `'' is not a color string Reactor can parse, so it renders gray — ; did you mean ?` + [GeneratedRegex(@"^'(?.*?)' is not a color string Reactor can parse\b.*; did you mean (?.+)\?$", RegexOptions.Singleline)] + private static partial Regex MessagePattern(); + public RuleSuggestion TryMatch(in RuleContext ctx) { if (ctx.Diagnostic.Id != DiagnosticId) return RuleSuggestion.Silent; - var brushHelper = ctx.Resolver.ResolveType(BrushHelperFqn); - var resourceBuilder = ctx.Resolver.ResolveType(ResourceBuilderFqn); - var extensions = ctx.Resolver.ResolveType(ElementExtensionsFqn); - if (brushHelper is null && resourceBuilder is null && extensions is null) - return RuleSuggestion.Silent; - - // PickRelevantNode lands on the ArgumentSyntax (a literal or ?: branch - // walks up to it) or on the IdentifierName of a const reference. - var argument = ctx.Node.FirstAncestorOrSelf(); - if (argument?.Parent?.Parent is not InvocationExpressionSyntax invocation) - return RuleSuggestion.Silent; - - if (ctx.SemanticModel.GetOperation(invocation) is not IInvocationOperation operation) - return RuleSuggestion.Silent; - - var target = operation.TargetMethod; - var method = (target.ReducedFrom ?? target).OriginalDefinition; - var str = ctx.Compilation.GetSpecialType(SpecialType.System_String); - if (!HasStringColorParameter(method, str)) - return RuleSuggestion.Silent; - - var owner = method.ContainingType; - var isResourceSet = false; - if (SymbolEqualityComparer.Default.Equals(owner, brushHelper)) - { - if (method.Name != "Parse") return RuleSuggestion.Silent; - } - else if (SymbolEqualityComparer.Default.Equals(owner, resourceBuilder)) - { - if (method.Name != "Set" || method.Parameters.Length != 2) return RuleSuggestion.Silent; - isResourceSet = true; - } - else if (SymbolEqualityComparer.Default.Equals(owner, extensions)) - { - if (!method.IsExtensionMethod || !ModifierNames.Contains(method.Name)) return RuleSuggestion.Silent; - } - else - { - return RuleSuggestion.Silent; - } - - IArgumentOperation? colorArg = null; - IArgumentOperation? keyArg = null; - foreach (var arg in operation.Arguments) - { - if (arg.Parameter is null) continue; - if (arg.Parameter.Name == ColorParameterName) colorArg = arg; - else if (arg.Parameter.Name == KeyParameterName) keyArg = arg; - } - if (colorArg is null || colorArg.ArgumentKind != ArgumentKind.Explicit) - return RuleSuggestion.Silent; - // The diagnostic must sit on the color argument, not (say) the key. - if (!colorArg.Syntax.Span.Contains(ctx.Node.Span) && !ctx.Node.Span.Contains(colorArg.Syntax.Span)) - return RuleSuggestion.Silent; + var match = MessagePattern().Match(ctx.Diagnostic.GetMessage()); + if (!match.Success) return RuleSuggestion.Silent; - string? resourceKey = null; - var fontFamilyOverload = false; - if (isResourceSet) - { - if (keyArg?.Value.ConstantValue is { HasValue: true, Value: string key }) - resourceKey = key; - fontFamilyOverload = resourceBuilder!.GetMembers("Set").OfType() - .Any(m => m.Parameters.Length == 2 && m.Parameters[1].Type.ToDisplayString() == FontFamilyFqn); - } + var suggestion = match.Groups["suggestion"].Value; + var value = match.Groups["value"].Value; + var evidence = suggestion.StartsWith("new FontFamily(", StringComparison.Ordinal) + ? "*FontFamily key → FontFamily overload" + : $"'{Truncate(value)}' renders gray"; - // Every constant value the argument can take; emit only when the - // unparseable ones agree on a single concrete rewrite. - string? text = null; - ColorStringIssue issue = default; - foreach (var value in ConstantStrings(colorArg.Value.Syntax as ExpressionSyntax, ctx.SemanticModel)) - { - if (ColorStringClassifier.Classify(value, resourceKey, fontFamilyOverload) is not { } finding) - continue; - if (finding.Replacement is null) - return RuleSuggestion.Silent; - var candidate = SuggestionText(finding); - if (text is not null && !string.Equals(text, candidate, StringComparison.Ordinal)) - return RuleSuggestion.Silent; - text = candidate; - issue = finding.Issue; - } - if (text is null) return RuleSuggestion.Silent; - - return new RuleSuggestion( - Text: text, - Confidence: 0.95, - Evidence: issue switch - { - ColorStringIssue.FontFamilyKey => "*FontFamily key → FontFamily overload", - ColorStringIssue.HexShorthand => "BrushHelper has no #RGB shorthand", - ColorStringIssue.MissingHash => "hex color needs a leading '#'", - _ => "named color BrushHelper doesn't know → hex", - }); - } - - /// The → try: text for a finding that carries a replacement. - internal static string SuggestionText(ColorStringFinding finding) => - finding.Issue == ColorStringIssue.FontFamilyKey - ? $"new FontFamily(\"{finding.Replacement}\")" - : $"\"{finding.Replacement}\""; - - static bool HasStringColorParameter(IMethodSymbol method, ITypeSymbol str) - { - foreach (var p in method.Parameters) - if (p.Name == ColorParameterName && SymbolEqualityComparer.Default.Equals(p.Type, str)) - return true; - return false; + return new RuleSuggestion(Text: suggestion, Confidence: 0.95, Evidence: evidence); } - // Same walk as ColorStringLiteralAnalyzer.ConstantStrings. - static IEnumerable ConstantStrings(ExpressionSyntax? expression, SemanticModel model) - { - if (expression is null) yield break; - - var constant = model.GetConstantValue(expression); - if (constant.HasValue) - { - if (constant.Value is string s) yield return s; - yield break; - } - - IEnumerable children = expression switch - { - ParenthesizedExpressionSyntax p => [p.Expression], - ConditionalExpressionSyntax c => [c.WhenTrue, c.WhenFalse], - BinaryExpressionSyntax b when b.IsKind(SyntaxKind.CoalesceExpression) => [b.Left, b.Right], - SwitchExpressionSyntax sw => sw.Arms.Select(a => a.Expression), - _ => [], - }; - foreach (var child in children) - foreach (var item in ConstantStrings(child, model)) - yield return item; - } + static string Truncate(string s) => s.Length <= 24 ? s : s[..21] + "..."; } diff --git a/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs b/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs index 1e9260ca2..d32e66a2c 100644 --- a/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs +++ b/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs @@ -1,5 +1,6 @@ using System.Threading.Tasks; using Microsoft.CodeAnalysis.CSharp.Testing; +using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Testing; using Microsoft.UI.Reactor.Analyzers; using Xunit; @@ -132,7 +133,7 @@ public async Task Flags_Background_with_css_name_and_suggests_hex() var test = MakeTest(Wrap(@" static object M() => TextBlock(""hi"").Background({|#0:""orange""|});")); test.ExpectedDiagnostics.Add(Dym006(0).WithArguments( "orange", - "BrushHelper only knows red, green, blue, white, black, gray, lightgray, transparent; did you mean \"#FFA500\"?")); + "the only built-in names are red, green, blue, white, black, gray/grey, lightgray/lightgrey, transparent; did you mean \"#FFA500\"?")); await test.RunAsync(TestContext.Current.CancellationToken); } @@ -170,7 +171,7 @@ public async Task Flags_unknown_name_with_supported_list() var test = MakeTest(Wrap(@" static object M() => TextBlock(""hi"").Background({|#0:""accent""|});")); test.ExpectedDiagnostics.Add(Dym006(0).WithArguments( "accent", - "use #RRGGBB or #AARRGGBB, a Theme token, or one of red, green, blue, white, black, gray, lightgray, transparent")); + "use #RRGGBB or #AARRGGBB, a Theme token, or one of red, green, blue, white, black, gray/grey, lightgray/lightgrey, transparent")); await test.RunAsync(TestContext.Current.CancellationToken); } @@ -210,7 +211,7 @@ public async Task Flags_FontFamily_key_and_suggests_FontFamily_when_overload_exi fontFamilyOverload: true)); test.ExpectedDiagnostics.Add(Dym006(0).WithArguments( "Consolas", - "the key 'ContentControlThemeFontFamily' names a FontFamily resource; did you mean new FontFamily(\"Consolas\")?")); + "'ContentControlThemeFontFamily' is a FontFamily resource; did you mean new FontFamily(\"Consolas\")?")); await test.RunAsync(TestContext.Current.CancellationToken); } @@ -221,7 +222,20 @@ public async Task Flags_FontFamily_key_without_suggestion_when_overload_missing( @" static object M(ResourceBuilder r) => r.Set(""ContentControlThemeFontFamily"", {|#0:""Consolas""|});")); test.ExpectedDiagnostics.Add(Dym006(0).WithArguments( "Consolas", - "the key 'ContentControlThemeFontFamily' names a FontFamily resource, but Set(string, string) only stores color brushes")); + "'ContentControlThemeFontFamily' is a FontFamily resource, but this Set overload only stores color brushes; to change the font, use .FontFamily(\"Consolas\") on the element")); + await test.RunAsync(TestContext.Current.CancellationToken); + } + + [Fact] + public async Task FontFamily_key_inside_a_branch_gets_no_FontFamily_suggestion() + { + // new FontFamily(...) can't replace one branch of a ?: — the other branch is still a string. + var test = MakeTest(Wrap( + @" static object M(ResourceBuilder r, bool on) => r.Set(""ContentControlThemeFontFamily"", on ? {|#0:""Consolas""|} : ""red"");", + fontFamilyOverload: true)); + test.ExpectedDiagnostics.Add(Dym006(0).WithArguments( + "Consolas", + "'ContentControlThemeFontFamily' is a FontFamily resource, but this Set overload only stores color brushes; to change the font, use .FontFamily(\"Consolas\") on the element")); await test.RunAsync(TestContext.Current.CancellationToken); } @@ -267,6 +281,24 @@ public Task Silent_on_unreachable_constant_branch() => // The whole expression is a constant ("red"), so the dead "orange" branch is never parsed. RunAsync(@" static object M() => TextBlock(""hi"").Background(true ? ""red"" : ""orange"");"); + [Fact] + public Task Silent_on_dead_branches_the_compiler_does_not_fold() => + // Each expression as a whole is non-constant, so only the constant condition / left side + // proves which branch runs. The live branches are still checked. + RunAsync(@" const bool DebugBuild = false; + const string Mode = ""dark""; + static object M(string c) => TextBlock(""hi"") + .Background(DebugBuild ? ""magenta"" : c) + .Foreground(!DebugBuild ? c : ""magenta"") + .BorderBrush(""red"" ?? c) + .WithBorder(Mode switch { ""light"" => ""orange"", ""dark"" => c, _ => ""teal"" }) + .Background(DebugBuild ? c : {|REACTOR_DYM_006:""navy""|});"); + + [Fact] + public Task Non_constant_switch_keeps_every_arm() => + RunAsync(@" static object M(string mode, string c) => TextBlock(""hi"") + .Background(mode switch { ""light"" => {|REACTOR_DYM_006:""orange""|}, _ => c });"); + [Fact] public async Task Silent_without_any_Reactor_color_api() { @@ -274,6 +306,61 @@ public async Task Silent_without_any_Reactor_color_api() await test.RunAsync(TestContext.Current.CancellationToken); } + // ── Live Reactor surface ──────────────────────────────────────────────────────────────────── + // Every other test runs against stubs that hardcode `string color`. This one binds to the real + // Reactor assembly, so renaming a `color` parameter or moving a type (which would silently turn + // the analyzer off) fails here. + + [Fact] + public async Task Fires_on_every_covered_api_of_the_live_Reactor_assembly() + { + var live = Microsoft.UI.Reactor.Tests.CheckCommandTests.Rules.RuleTargetResolutionTests.BuildLiveReactorCompilation(); + + var targets = ColorStringLiteralAnalyzer.Targets.Resolve(live); + Assert.NotNull(targets); + var covered = targets!.Methods + .Select(m => $"{m.ContainingType.Name}.{m.Name}({string.Join(", ", m.Parameters.Select(p => p.Type.Name + " " + p.Name))})") + .OrderBy(s => s, global::System.StringComparer.Ordinal) + .ToArray(); + Assert.Equal(new[] + { + "BrushHelper.Parse(String color)", + "ElementExtensions.Background(T el, String color)", + "ElementExtensions.BorderBrush(T el, String color)", + "ElementExtensions.Foreground(RichTextBlockElement el, String color)", + "ElementExtensions.Foreground(RichTextHyperlink link, String color)", + "ElementExtensions.Foreground(RichTextParagraph paragraph, String color)", + "ElementExtensions.Foreground(RichTextRun run, String color)", + "ElementExtensions.Foreground(T el, String color)", + "ElementExtensions.WithBorder(T el, String color, Double thickness)", + "ResourceBuilder.Set(String key, String color)", + }, covered); + + var source = Microsoft.CodeAnalysis.CSharp.CSharpSyntaxTree.ParseText(@" +using Microsoft.UI.Reactor; +using Microsoft.UI.Reactor.Elements; +using static Microsoft.UI.Reactor.Factories; +static class LiveProbe +{ + static object A() => TextBlock(""x"").Background(""orange""); + static object B() => new ResourceBuilder().Set(""ButtonBackground"", ""#888""); + static object C() => BrushHelper.Parse(""FF0000""); + static object D() => TextBlock(""x"").Foreground(""red"").BorderBrush(""#80FF0000""); +}", cancellationToken: TestContext.Current.CancellationToken); + var compilation = live.AddSyntaxTrees(source); + Assert.Empty(compilation.GetDiagnostics(TestContext.Current.CancellationToken) + .Where(d => d.Severity == Microsoft.CodeAnalysis.DiagnosticSeverity.Error && d.Location.SourceTree == source)); + + var diagnostics = await compilation + .WithAnalyzers(global::System.Collections.Immutable.ImmutableArray.Create(new ColorStringLiteralAnalyzer())) + .GetAnalyzerDiagnosticsAsync(TestContext.Current.CancellationToken); + Assert.Equal( + new[] { "orange", "#888", "FF0000" }, + diagnostics.Where(d => d.Id == ColorStringLiteralAnalyzer.DiagnosticId) + .OrderBy(d => d.Location.SourceSpan.Start) + .Select(d => d.Properties.ContainsKey(ColorStringLiteralAnalyzer.ReplacementProperty) ? source.GetText(TestContext.Current.CancellationToken).ToString(d.Location.SourceSpan).Trim('"') : "?")); + } + // ── Code fixes ────────────────────────────────────────────────────────────────────────────── private static Task RunFixAsync(string before, string after, bool fontFamilyOverload = false) @@ -321,6 +408,20 @@ public Task Fix_wraps_const_FontFamily_reference() => static object M(ResourceBuilder r) => r.Set(""ContentControlThemeFontFamily"", new FontFamily(Mono));", fontFamilyOverload: true); + [Fact] + public Task Fix_renames_named_color_argument_to_the_FontFamily_parameter() => + RunFixAsync( + @" static object M(ResourceBuilder r) => r.Set(key: ""ContentControlThemeFontFamily"", color: {|REACTOR_DYM_006:""Consolas""|});", + @" static object M(ResourceBuilder r) => r.Set(key: ""ContentControlThemeFontFamily"", fontFamily: new FontFamily(""Consolas""));", + fontFamilyOverload: true); + + [Fact] + public Task No_FontFamily_fix_inside_a_branch() => + RunFixAsync( + @" static object M(ResourceBuilder r, bool on) => r.Set(""ContentControlThemeFontFamily"", on ? {|REACTOR_DYM_006:""Consolas""|} : ""red"");", + @" static object M(ResourceBuilder r, bool on) => r.Set(""ContentControlThemeFontFamily"", on ? {|REACTOR_DYM_006:""Consolas""|} : ""red"");", + fontFamilyOverload: true); + [Fact] public Task No_fix_without_FontFamily_overload_or_for_unknown_or_const_color() => RunFixAsync( diff --git a/tests/Reactor.Tests/AnalyzerTests/ColorStringParityTests.cs b/tests/Reactor.Tests/AnalyzerTests/ColorStringParityTests.cs index 66175d7fc..89fc914d2 100644 --- a/tests/Reactor.Tests/AnalyzerTests/ColorStringParityTests.cs +++ b/tests/Reactor.Tests/AnalyzerTests/ColorStringParityTests.cs @@ -1,14 +1,14 @@ using AnalyzerClassifier = Microsoft.UI.Reactor.Analyzers.ColorStringClassifier; -using CliClassifier = Microsoft.UI.Reactor.Cli.Check.Rules.ColorStringClassifier; using Xunit; namespace Microsoft.UI.Reactor.Tests.AnalyzerTests; /// -/// REACTOR_DYM_006 parity lock (issue #1375). Three copies of one color grammar must agree: -/// BrushHelper.TryParseColor (the runtime truth), the netstandard2.0 analyzer's -/// ColorStringClassifier, and the mur check CLI's copy. If any of them drifts, the -/// analyzer either flags colors that render fine or misses ones that render gray. +/// REACTOR_DYM_006 parity lock (issue #1375). Two copies of one color grammar must agree: +/// BrushHelper.TryParseColor (the runtime truth) and the netstandard2.0 analyzer's +/// ColorStringClassifier. If they drift, the analyzer either flags colors that render fine or +/// misses ones that render gray. (The mur check rule reads the analyzer's message, so it has no +/// grammar of its own; DidYouMeanAnalyzerParityTests covers it.) /// public class ColorStringParityTests { @@ -32,37 +32,33 @@ public void Analyzer_TryParse_matches_BrushHelper(string value) { var runtime = BrushHelper.TryParseColor(value, out _); Assert.Equal(runtime, AnalyzerClassifier.TryParse(value)); - Assert.Equal(runtime, CliClassifier.TryParse(value)); + // Classify must agree too: a finding exactly when BrushHelper falls back to gray. + Assert.Equal(!runtime, AnalyzerClassifier.Classify(value, null, false).HasValue); } - [Theory] - [MemberData(nameof(Battery))] - public void Analyzer_and_cli_classify_identically(string value) + [Fact] + public void Every_fixable_detail_ends_in_the_did_you_mean_the_cli_rule_reads() { - foreach (var key in new[] { null, "ButtonBackground", "ContentControlThemeFontFamily", "FontFamily", "fontfamily" }) + // ColorStringLiteralRule lifts the rewrite out of "...; did you mean X?". Keep the suffix stable. + foreach (var (value, key, overload) in new (string, string?, bool)[] + { + ("#888", null, false), ("FF0000", null, false), ("orange", null, false), + ("Consolas", "ContentControlThemeFontFamily", true), + }) { - foreach (var overload in new[] { false, true }) - { - var a = AnalyzerClassifier.Classify(value, key, overload); - var c = CliClassifier.Classify(value, key, overload); - Assert.Equal(a.HasValue, c.HasValue); - if (a is { } af && c is { } cf) - { - Assert.Equal(af.Issue.ToString(), cf.Issue.ToString()); - Assert.Equal(af.Replacement, cf.Replacement); - Assert.Equal(af.Detail, cf.Detail); - } - } + var finding = AnalyzerClassifier.Classify(value, key, overload)!.Value; + Assert.NotNull(finding.Replacement); + Assert.EndsWith("; did you mean " + (finding.Issue.ToString() == "FontFamilyKey" + ? $"new FontFamily(\"{finding.Replacement}\")" + : $"\"{finding.Replacement}\"") + "?", finding.Detail, StringComparison.Ordinal); } } [Fact] - public void Named_color_tables_are_identical() + public void Supported_names_list_matches_BrushHelper() { - Assert.Equal( - AnalyzerClassifier.NamedColorHex.OrderBy(p => p.Key, StringComparer.Ordinal), - CliClassifier.NamedColorHex.OrderBy(p => p.Key, StringComparer.Ordinal)); - Assert.Equal(AnalyzerClassifier.SupportedNames, CliClassifier.SupportedNames); + foreach (var name in AnalyzerClassifier.SupportedNames.Split(", ").SelectMany(n => n.Split('/'))) + Assert.True(BrushHelper.TryParseColor(name, out _), $"'{name}' is listed as supported but BrushHelper rejects it."); } [Fact] diff --git a/tests/Reactor.Tests/CheckCommandTests/Rules/ColorStringLiteralRuleTests.cs b/tests/Reactor.Tests/CheckCommandTests/Rules/ColorStringLiteralRuleTests.cs index 23e6fbccd..377ec04de 100644 --- a/tests/Reactor.Tests/CheckCommandTests/Rules/ColorStringLiteralRuleTests.cs +++ b/tests/Reactor.Tests/CheckCommandTests/Rules/ColorStringLiteralRuleTests.cs @@ -40,6 +40,18 @@ public void Fires_with_the_concrete_rewrite(string expression, string expected) Assert.Contains("analyzer:REACTOR_DYM_006", s.Evidence); } + [Fact] + public void Reads_the_rewrite_from_the_diagnostic_message_not_the_source() + { + // The rule trusts the analyzer's message: a message from another source shape still yields its + // own did-you-mean, and a REACTOR_DYM_006 whose message carries none stays silent. + var (diags, c) = Analyze(@"TextBlock(""hi"").Background(""orange"")"); + var diag = Assert.Single(diags); + var forged = diag with { Message = "'teal' is not a color string Reactor can parse, so it renders gray — the only built-in names are red; did you mean \"#008080\"?" }; + Assert.Equal("\"#008080\"", RunRule(forged, c)!.Text); + Assert.Null(RunRule(diag with { Message = "something else entirely" }, c)); + } + [Fact] public void Suggests_FontFamily_when_the_overload_exists() { @@ -61,17 +73,23 @@ public void Silent_when_there_is_no_concrete_rewrite(string expression) } [Fact] - public void Each_branch_gets_its_own_rewrite_and_disagreeing_branches_stay_silent() + public void Each_branch_gets_its_own_rewrite() { - // Two unparseable branches with different rewrites: the rule only sees the argument, so it - // cannot tell which literal the diagnostic is about — silent beats a wrong suggestion. var (diags, c) = Analyze(@"TextBlock(""hi"").Background(on ? ""orange"" : ""teal"")"); Assert.Equal(2, diags.Length); - Assert.All(diags, d => Assert.Null(RunRule(d, c))); + Assert.Equal(new[] { "\"#FFA500\"", "\"#008080\"" }, diags.Select(d => RunRule(d, c)!.Text)); - // One unparseable branch: unambiguous. - (diags, c) = Analyze(@"TextBlock(""hi"").Background(on ? ""orange"" : ""red"")"); - Assert.Equal("\"#FFA500\"", RunRule(Assert.Single(diags), c)!.Text); + // A branch with no rewrite doesn't silence its fixable sibling. + (diags, c) = Analyze(@"TextBlock(""hi"").Background(on ? ""orange"" : ""accent"")"); + Assert.Equal(new[] { "\"#FFA500\"", null }, diags.Select(d => RunRule(d, c)?.Text)); + } + + [Fact] + public void FontFamily_value_inside_a_branch_gets_no_FontFamily_suggestion() + { + // Matches the code fix, which can't wrap one branch of a ?: in new FontFamily(...). + var (diags, c) = Analyze(@"new ResourceBuilder().Set(""ContentControlThemeFontFamily"", on ? ""Consolas"" : ""red"")", fontFamilyOverload: true); + Assert.Null(RunRule(Assert.Single(diags), c)); } [Fact] @@ -143,7 +161,7 @@ static class C var found = c.WithAnalyzers(ImmutableArray.Create(new ColorStringLiteralAnalyzer())) .GetAnalyzerDiagnosticsAsync(TestContext.Current.CancellationToken).GetAwaiter().GetResult(); - return (found.Where(d => d.Id == ColorStringLiteralAnalyzer.DiagnosticId).ToArray(), c); + return (found.Where(d => d.Id == ColorStringLiteralAnalyzer.DiagnosticId).OrderBy(d => d.Location.SourceSpan.Start).ToArray(), c); } internal static Suggestion? RunRule(CheckCommand.Diag diag, CSharpCompilation c) diff --git a/tests/Reactor.Tests/CheckCommandTests/Rules/RuleTargetResolutionTests.cs b/tests/Reactor.Tests/CheckCommandTests/Rules/RuleTargetResolutionTests.cs index 51b3643f3..f6d6fe2d4 100644 --- a/tests/Reactor.Tests/CheckCommandTests/Rules/RuleTargetResolutionTests.cs +++ b/tests/Reactor.Tests/CheckCommandTests/Rules/RuleTargetResolutionTests.cs @@ -56,7 +56,7 @@ public void Every_registered_rule_resolves_its_declared_targets_against_live_Rea /// authoritative gate. /// [global::System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessage("SingleFile", "IL3000", Justification = "Test-only: reads loaded assemblies' on-disk Location to build a live Reactor Roslyn compilation (MetadataReference.CreateFromFile). IL3000 only affects single-file publish (Location is empty there) — already handled by the surrounding try/catch + skip; this host is not single-file-published. Behaviour-neutral.")] - static CSharpCompilation BuildLiveReactorCompilation() + internal static CSharpCompilation BuildLiveReactorCompilation() { // Force Reactor.dll into the AppDomain BEFORE enumerating loaded // assemblies — otherwise xunit's lazy assembly loading can leave diff --git a/tests/Reactor.Tests/PerfDiffSkipPathTests.cs b/tests/Reactor.Tests/PerfDiffSkipPathTests.cs index faba943c7..3b0e53983 100644 --- a/tests/Reactor.Tests/PerfDiffSkipPathTests.cs +++ b/tests/Reactor.Tests/PerfDiffSkipPathTests.cs @@ -616,9 +616,20 @@ public void InterspersedGrid_Builds_Item_And_Separator_Tracks() [Theory] [InlineData("red", 255, 255, 0, 0)] + [InlineData("green", 255, 0, 128, 0)] + [InlineData("blue", 255, 0, 0, 255)] + [InlineData("white", 255, 255, 255, 255)] + [InlineData("black", 255, 0, 0, 0)] + [InlineData("gray", 255, 128, 128, 128)] + [InlineData("grey", 255, 128, 128, 128)] + [InlineData("lightgray", 255, 211, 211, 211)] + [InlineData("lightgrey", 255, 211, 211, 211)] [InlineData("transparent", 0, 0, 0, 0)] [InlineData("#FF8800", 255, 255, 0x88, 0)] [InlineData("#8000FF00", 0x80, 0, 255, 0)] + // Unrecognized input falls back to gray — the behaviour REACTOR_DYM_006 warns about. + [InlineData("orange", 255, 128, 128, 128)] + [InlineData("#888", 255, 128, 128, 128)] public void ParseColor_Resolves_Named_And_Hex(string input, int a, int r, int g, int b) { var color = BrushHelper.ParseColor(input); From 90192bf9c1c6ea6e4b41d950e81dfe9166c7d6f2 Mon Sep 17 00:00:00 2001 From: Alexandre Zollinger Chohfi Date: Thu, 15 Oct 2026 11:25:51 -0700 Subject: [PATCH 03/10] Address review round 1 - FontFamily suggestion escapes the value as a regular C# literal and includes the named-argument rename, so mur check's try: text compiles. - ?? pruning uses proven non-null results from nested selectors. - Switch pruning compares the pattern constant converted to the governing type. - Explicit Where filters in Targets.Resolve and the code fix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b58aacd0-29b6-46c1-9603-b8a8b4eecbd8 --- .../ColorStringClassifier.cs | 20 ++++-- .../ColorStringLiteralAnalyzer.cs | 62 ++++++++++++++----- .../ColorStringLiteralCodeFix.cs | 14 +++-- .../ColorStringLiteralAnalyzerTests.cs | 28 +++++++++ .../AnalyzerTests/ColorStringParityTests.cs | 17 ++++- .../DidYouMeanAnalyzerParityTests.cs | 24 +++---- 6 files changed, 127 insertions(+), 38 deletions(-) diff --git a/src/Reactor.Analyzers/ColorStringClassifier.cs b/src/Reactor.Analyzers/ColorStringClassifier.cs index bda4eb0d4..0adb3f76c 100644 --- a/src/Reactor.Analyzers/ColorStringClassifier.cs +++ b/src/Reactor.Analyzers/ColorStringClassifier.cs @@ -249,9 +249,10 @@ private static bool IsByte(string hex, int start) => /// Returns the finding for , or when BrushHelper /// parses it. is the constant key of a ResourceBuilder.Set /// call (otherwise ); says whether - /// ResourceBuilder.Set(string, FontFamily) is available to rewrite to. + /// ResourceBuilder.Set(string, FontFamily) is available to rewrite to; + /// is that overload's parameter name when the flagged argument is named (color: "…") and must be renamed. /// - public static ColorStringFinding? Classify(string value, string? resourceKey, bool fontFamilyOverloadExists) + public static ColorStringFinding? Classify(string value, string? resourceKey, bool fontFamilyOverloadExists, string? fontFamilyArgumentName = null) { if (TryParse(value)) return null; @@ -262,11 +263,11 @@ private static bool IsByte(string hex, int start) => ? new ColorStringFinding( ColorStringIssue.FontFamilyKey, value, - $"'{resourceKey}' is a FontFamily resource; did you mean new FontFamily(\"{value}\")?") + $"'{resourceKey}' is a FontFamily resource; did you mean {FontFamilySuggestion(value, fontFamilyArgumentName)}?") : new ColorStringFinding( ColorStringIssue.FontFamilyKey, null, - $"'{resourceKey}' is a FontFamily resource, but this Set overload only stores color brushes; to change the font, use .FontFamily(\"{value}\") on the element"); + $"'{resourceKey}' is a FontFamily resource, but this Set overload only stores color brushes; to change the font, use .FontFamily({Literal(value)}) on the element"); } if (value.StartsWith("#", StringComparison.Ordinal)) @@ -309,6 +310,17 @@ private static bool IsByte(string hex, int start) => $"use #RRGGBB or #AARRGGBB, a Theme token, or one of {SupportedNames}"); } + /// + /// The C# source the FontFamily rewrite produces, e.g. new FontFamily("Consolas"), prefixed with + /// name: for a renamed argument. The value is escaped as a regular C# string literal. + /// + public static string FontFamilySuggestion(string value, string? argumentName) => + (argumentName is null ? "" : argumentName + ": ") + "new FontFamily(" + Literal(value) + ")"; + + // A regular C# string literal for the value (quotes, backslashes and control characters escaped). + private static string Literal(string value) => + Microsoft.CodeAnalysis.CSharp.SymbolDisplay.FormatLiteral(value, quote: true); + private static bool IsAllHex(string s) { foreach (var c in s) diff --git a/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs b/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs index d9e16d68a..c5f625223 100644 --- a/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs +++ b/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs @@ -125,9 +125,8 @@ private Targets(ImmutableHashSet methods, IMethodSymbol? resource var brushHelper = compilation.GetTypeByMetadataName(BrushHelperMetadataName); if (brushHelper is not null) { - foreach (var m in brushHelper.GetMembers("Parse").OfType()) - if (HasStringColorParameter(m, str)) - methods.Add(m); + foreach (var m in brushHelper.GetMembers("Parse").OfType().Where(m => HasStringColorParameter(m, str))) + methods.Add(m); } IMethodSymbol? resourceSet = null; @@ -135,10 +134,8 @@ private Targets(ImmutableHashSet methods, IMethodSymbol? resource var resourceBuilder = compilation.GetTypeByMetadataName(ResourceBuilderMetadataName); if (resourceBuilder is not null) { - foreach (var m in resourceBuilder.GetMembers("Set").OfType()) + foreach (var m in resourceBuilder.GetMembers("Set").OfType().Where(m => m.Parameters.Length == 2)) { - if (m.Parameters.Length != 2) - continue; if (HasStringColorParameter(m, str) && SymbolEqualityComparer.Default.Equals(m.Parameters[0].Type, str)) { resourceSet = m; @@ -155,9 +152,8 @@ private Targets(ImmutableHashSet methods, IMethodSymbol? resource if (extensions is not null) { foreach (var name in ModifierNames) - foreach (var m in extensions.GetMembers(name).OfType()) - if (m.IsExtensionMethod && HasStringColorParameter(m, str)) - methods.Add(m); + foreach (var m in extensions.GetMembers(name).OfType().Where(m => m.IsExtensionMethod && HasStringColorParameter(m, str))) + methods.Add(m); } return methods.Count == 0 ? null : new Targets(methods.ToImmutable(), resourceSet, fontFamilyParameter); @@ -226,7 +222,11 @@ private static void AnalyzeInvocation(SyntaxNodeAnalysisContext context, Targets // new FontFamily(...) can only replace the whole argument: inside a ?: branch the other // branch would still be a string and the expression would stop type-checking. var canUseFontFamily = targets.FontFamilyParameterName is not null && expression.Parent is ArgumentSyntax; - var finding = ColorStringClassifier.Classify(value, resourceKey, canUseFontFamily); + // A named `color:` argument must be renamed too, or the rewritten call won't bind. + var renamedArgument = canUseFontFamily && expression.Parent is ArgumentSyntax { NameColon: not null } + ? targets.FontFamilyParameterName + : null; + var finding = ColorStringClassifier.Classify(value, resourceKey, canUseFontFamily, renamedArgument); if (finding is not { } f) continue; @@ -267,14 +267,23 @@ private static void AnalyzeInvocation(SyntaxNodeAnalysisContext context, Targets yield break; } - IEnumerable children = expression switch + foreach (var child in LiveChildren(expression, model, cancellationToken)) + foreach (var item in ConstantStrings(child, model, cancellationToken)) + yield return item; + } + + /// + /// The sub-expressions a value-selecting shape can actually yield. Empty for any other shape. + /// + private static IEnumerable LiveChildren(ExpressionSyntax expression, SemanticModel model, CancellationToken cancellationToken) => + expression switch { ParenthesizedExpressionSyntax p => new[] { p.Expression }, ConditionalExpressionSyntax c => model.GetConstantValue(c.Condition, cancellationToken) is { HasValue: true, Value: bool taken } ? new[] { taken ? c.WhenTrue : c.WhenFalse } : new[] { c.WhenTrue, c.WhenFalse }, BinaryExpressionSyntax b when b.IsKind(SyntaxKind.CoalesceExpression) => - model.GetConstantValue(b.Left, cancellationToken) is { HasValue: true, Value: not null } + IsKnownNonNull(b.Left, model, cancellationToken) ? new[] { b.Left } : new[] { b.Left, b.Right }, // Arms are only pruned when every pattern is a plain constant and the governing value is a @@ -282,9 +291,28 @@ BinaryExpressionSyntax b when b.IsKind(SyntaxKind.CoalesceExpression) => SwitchExpressionSyntax sw => LiveArms(sw, model, cancellationToken), _ => Array.Empty(), }; - foreach (var child in children) - foreach (var item in ConstantStrings(child, model, cancellationToken)) - yield return item; + + /// + /// Whether the expression provably evaluates to a non-null value: a non-null constant, or a + /// value-selecting shape whose every live branch is. Unknown (e.g. a variable) is . + /// + private static bool IsKnownNonNull(ExpressionSyntax expression, SemanticModel model, CancellationToken cancellationToken) + { + var constant = model.GetConstantValue(expression, cancellationToken); + if (constant.HasValue) + return constant.Value is not null; + + if (expression is BinaryExpressionSyntax b && b.IsKind(SyntaxKind.CoalesceExpression)) + return IsKnownNonNull(b.Left, model, cancellationToken) || IsKnownNonNull(b.Right, model, cancellationToken); + + var any = false; + foreach (var child in LiveChildren(expression, model, cancellationToken)) + { + if (!IsKnownNonNull(child, model, cancellationToken)) + return false; + any = true; + } + return any; } private static IEnumerable LiveArms(SwitchExpressionSyntax sw, SemanticModel model, CancellationToken cancellationToken) @@ -301,7 +329,9 @@ private static IEnumerable LiveArms(SwitchExpressionSyntax sw, { case DiscardPatternSyntax: return new[] { arm.Expression }; - case ConstantPatternSyntax cp when model.GetConstantValue(cp.Expression, cancellationToken) is { HasValue: true } armValue: + // Compare the pattern constant *converted to the governing type* (`1` against a + // `const long` is a long 1), not the boxed source literal. + case ConstantPatternSyntax when model.GetOperation(arm.Pattern, cancellationToken) is IConstantPatternOperation { Value.ConstantValue: { HasValue: true } armValue }: if (Equals(armValue.Value, governing.Value)) return new[] { arm.Expression }; break; diff --git a/src/Reactor.Analyzers/ColorStringLiteralCodeFix.cs b/src/Reactor.Analyzers/ColorStringLiteralCodeFix.cs index a434a1505..7296e9676 100644 --- a/src/Reactor.Analyzers/ColorStringLiteralCodeFix.cs +++ b/src/Reactor.Analyzers/ColorStringLiteralCodeFix.cs @@ -1,4 +1,5 @@ using System.Collections.Immutable; +using System.Linq; using System.Composition; using System.Threading.Tasks; using Microsoft.CodeAnalysis; @@ -36,12 +37,13 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); if (root is null) return; - foreach (var diagnostic in context.Diagnostics) + // Only diagnostics the analyzer attached a rewrite to are fixable. + foreach (var diagnostic in context.Diagnostics.Where(d => + d.Properties.TryGetValue(ColorStringLiteralAnalyzer.ReplacementProperty, out var r) && r is not null + && d.Properties.ContainsKey(ColorStringLiteralAnalyzer.IssueProperty))) { - if (!diagnostic.Properties.TryGetValue(ColorStringLiteralAnalyzer.ReplacementProperty, out var replacement) - || replacement is null - || !diagnostic.Properties.TryGetValue(ColorStringLiteralAnalyzer.IssueProperty, out var issue)) - continue; + var replacement = diagnostic.Properties[ColorStringLiteralAnalyzer.ReplacementProperty]!; + var issue = diagnostic.Properties[ColorStringLiteralAnalyzer.IssueProperty]; var node = root.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true); if (node is ArgumentSyntax argument) @@ -79,7 +81,7 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) context.RegisterCodeFix( CodeAction.Create( - $"Use new FontFamily(\"{replacement}\")", + $"Use {ColorStringClassifier.FontFamilySuggestion(replacement, null)}", _ => Task.FromResult(context.Document.WithSyntaxRoot(root.ReplaceNode(oldArgument, newArgument))), equivalenceKey: FontFamilyEquivalenceKey), diagnostic); diff --git a/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs b/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs index d32e66a2c..7955d3e9d 100644 --- a/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs +++ b/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs @@ -299,6 +299,34 @@ public Task Non_constant_switch_keeps_every_arm() => RunAsync(@" static object M(string mode, string c) => TextBlock(""hi"") .Background(mode switch { ""light"" => {|REACTOR_DYM_006:""orange""|}, _ => c });"); + [Fact] + public Task Coalesce_fallback_is_dead_when_a_nested_selector_is_provably_non_null() => + RunAsync(@" const bool DebugBuild = false; + static object M(string c, bool on) => TextBlock(""hi"") + .Background((DebugBuild ? c : ""red"") ?? ""orange"") + .Foreground((on ? ""red"" : ""white"") ?? ""teal"") + .BorderBrush((on ? c : ""red"") ?? {|REACTOR_DYM_006:""navy""|});"); + + [Fact] + public Task Switch_pruning_compares_the_converted_pattern_constant() => + // `1` is an int literal but the pattern converts it to the governing `long`. + RunAsync(@" const long Mode = 1; + static object M(string c) => TextBlock(""hi"") + .Background(Mode switch { 1 => c, _ => ""orange"" }) + .Foreground(Mode switch { 2 => c, _ => {|REACTOR_DYM_006:""teal""|} });"); + + [Fact] + public async Task FontFamily_suggestion_renames_a_named_argument_and_escapes_the_value() + { + var test = MakeTest(Wrap( + @" static object M(ResourceBuilder r) => r.Set(key: ""ContentControlThemeFontFamily"", color: {|#0:@""C:\Fonts\Mono.ttf#Mono""|});", + fontFamilyOverload: true)); + test.ExpectedDiagnostics.Add(Dym006(0).WithArguments( + @"C:\Fonts\Mono.ttf#Mono", + @"'ContentControlThemeFontFamily' is a FontFamily resource; did you mean fontFamily: new FontFamily(""C:\\Fonts\\Mono.ttf#Mono"")?")); + await test.RunAsync(TestContext.Current.CancellationToken); + } + [Fact] public async Task Silent_without_any_Reactor_color_api() { diff --git a/tests/Reactor.Tests/AnalyzerTests/ColorStringParityTests.cs b/tests/Reactor.Tests/AnalyzerTests/ColorStringParityTests.cs index 89fc914d2..e690be0cd 100644 --- a/tests/Reactor.Tests/AnalyzerTests/ColorStringParityTests.cs +++ b/tests/Reactor.Tests/AnalyzerTests/ColorStringParityTests.cs @@ -49,11 +49,26 @@ public void Every_fixable_detail_ends_in_the_did_you_mean_the_cli_rule_reads() var finding = AnalyzerClassifier.Classify(value, key, overload)!.Value; Assert.NotNull(finding.Replacement); Assert.EndsWith("; did you mean " + (finding.Issue.ToString() == "FontFamilyKey" - ? $"new FontFamily(\"{finding.Replacement}\")" + ? AnalyzerClassifier.FontFamilySuggestion(finding.Replacement!, null) : $"\"{finding.Replacement}\"") + "?", finding.Detail, StringComparison.Ordinal); } } + [Theory] + [InlineData("Consolas", null, "new FontFamily(\"Consolas\")")] + [InlineData("Consolas", "fontFamily", "fontFamily: new FontFamily(\"Consolas\")")] + [InlineData(@"C:\Fonts\a""b.ttf#A", null, @"new FontFamily(""C:\\Fonts\\a\""b.ttf#A"")")] + public void FontFamily_suggestion_is_compilable_CSharp(string value, string? argumentName, string expected) + { + Assert.Equal(expected, AnalyzerClassifier.FontFamilySuggestion(value, argumentName)); + // And it round-trips: the literal parses back to the original value. + var creation = (Microsoft.CodeAnalysis.CSharp.Syntax.ObjectCreationExpressionSyntax) + Microsoft.CodeAnalysis.CSharp.SyntaxFactory.ParseExpression(AnalyzerClassifier.FontFamilySuggestion(value, null)); + Assert.Empty(creation.GetDiagnostics()); + var literal = (Microsoft.CodeAnalysis.CSharp.Syntax.LiteralExpressionSyntax)creation.ArgumentList!.Arguments[0].Expression; + Assert.Equal(value, literal.Token.ValueText); + } + [Fact] public void Supported_names_list_matches_BrushHelper() { diff --git a/tests/Reactor.Tests/AnalyzerTests/DidYouMeanAnalyzerParityTests.cs b/tests/Reactor.Tests/AnalyzerTests/DidYouMeanAnalyzerParityTests.cs index d3ef39377..9bd87582c 100644 --- a/tests/Reactor.Tests/AnalyzerTests/DidYouMeanAnalyzerParityTests.cs +++ b/tests/Reactor.Tests/AnalyzerTests/DidYouMeanAnalyzerParityTests.cs @@ -175,20 +175,22 @@ private static SuggestionResult RunSymbolSuggester(string userCode, string code) // Driven end-to-end: the real analyzer reports, its diagnostic is fed to the rule. [Theory] - [InlineData(@"TextBlock(""hi"").Foreground(""#888"")", false)] - [InlineData(@"TextBlock(""hi"").Background(""ff00ff"")", false)] - [InlineData(@"BrushHelper.Parse(""orange"")", false)] - [InlineData(@"new ResourceBuilder().Set(""ButtonBackground"", ""Wheat"")", false)] - [InlineData(@"new ResourceBuilder().Set(""ContentControlThemeFontFamily"", ""Consolas"")", true)] - public void Color_string_analyzer_replacement_matches_cli_rule(string expression, bool fontFamilyOverload) + // `expected` is the source text the IDE code fix writes in place of the flagged argument (the + // code-fix round-trips in ColorStringLiteralAnalyzerTests pin that side); the rule must suggest it. + [InlineData(@"TextBlock(""hi"").Foreground(""#888"")", false, @"""#888888""")] + [InlineData(@"TextBlock(""hi"").Background(""ff00ff"")", false, @"""#ff00ff""")] + [InlineData(@"BrushHelper.Parse(""orange"")", false, @"""#FFA500""")] + [InlineData(@"new ResourceBuilder().Set(""ButtonBackground"", ""Wheat"")", false, @"""#F5DEB3""")] + [InlineData(@"new ResourceBuilder().Set(""ContentControlThemeFontFamily"", ""Consolas"")", true, @"new FontFamily(""Consolas"")")] + // A named argument must be renamed to bind to the FontFamily overload. + [InlineData(@"new ResourceBuilder().Set(key: ""ContentControlThemeFontFamily"", color: ""Consolas"")", true, @"fontFamily: new FontFamily(""Consolas"")")] + // Backslashes and quotes are escaped as a regular C# literal, whatever form the source used. + [InlineData(@"new ResourceBuilder().Set(""ContentControlThemeFontFamily"", @""Assets\Fonts\Mono.ttf#Mono """"x"""""")", true, @"new FontFamily(""Assets\\Fonts\\Mono.ttf#Mono \""x\"""")")] + public void Color_string_analyzer_replacement_matches_cli_rule(string expression, bool fontFamilyOverload, string expected) { var (diagnostics, c) = ColorStringLiteralRuleTests.AnalyzeRaw(expression, fontFamilyOverload); var d = Assert.Single(diagnostics); - var replacement = d.Properties[ColorStringLiteralAnalyzer.ReplacementProperty]; - Assert.False(string.IsNullOrEmpty(replacement)); - var expected = d.Properties[ColorStringLiteralAnalyzer.IssueProperty] == "FontFamilyKey" - ? $"new FontFamily(\"{replacement}\")" - : $"\"{replacement}\""; + Assert.False(string.IsNullOrEmpty(d.Properties[ColorStringLiteralAnalyzer.ReplacementProperty])); var s = ColorStringLiteralRuleTests.RunRule(ColorStringLiteralRuleTests.ToCheckDiag(d), c); Assert.NotNull(s); From a076d44d1281d3388ee7a0a9a832e0eb0fba6b30 Mon Sep 17 00:00:00 2001 From: Alexandre Zollinger Chohfi Date: Thu, 15 Oct 2026 11:42:47 -0700 Subject: [PATCH 04/10] Address review round 2 Qualify the suggested type as Microsoft.UI.Xaml.Media.FontFamily so the mur check try: text compiles without a using, and add a test that applies each suggestion, recompiles, and re-runs the analyzer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b58aacd0-29b6-46c1-9603-b8a8b4eecbd8 --- .../ColorStringClassifier.cs | 8 +++-- .../Check/Rules/ColorStringLiteralRule.cs | 2 +- .../ColorStringLiteralAnalyzerTests.cs | 4 +-- .../AnalyzerTests/ColorStringParityTests.cs | 6 ++-- .../DidYouMeanAnalyzerParityTests.cs | 6 ++-- .../Rules/ColorStringLiteralRuleTests.cs | 33 ++++++++++++++++++- 6 files changed, 46 insertions(+), 13 deletions(-) diff --git a/src/Reactor.Analyzers/ColorStringClassifier.cs b/src/Reactor.Analyzers/ColorStringClassifier.cs index 0adb3f76c..68d7f3fda 100644 --- a/src/Reactor.Analyzers/ColorStringClassifier.cs +++ b/src/Reactor.Analyzers/ColorStringClassifier.cs @@ -311,11 +311,13 @@ private static bool IsByte(string hex, int start) => } /// - /// The C# source the FontFamily rewrite produces, e.g. new FontFamily("Consolas"), prefixed with - /// name: for a renamed argument. The value is escaped as a regular C# string literal. + /// The C# source the FontFamily rewrite produces, e.g. new Microsoft.UI.Xaml.Media.FontFamily("Consolas"), + /// prefixed with name: for a renamed argument. The type is namespace-qualified so the text compiles + /// without a using Microsoft.UI.Xaml.Media; (mur check shows it verbatim), and the value is escaped + /// as a regular C# string literal. /// public static string FontFamilySuggestion(string value, string? argumentName) => - (argumentName is null ? "" : argumentName + ": ") + "new FontFamily(" + Literal(value) + ")"; + (argumentName is null ? "" : argumentName + ": ") + "new Microsoft.UI.Xaml.Media.FontFamily(" + Literal(value) + ")"; // A regular C# string literal for the value (quotes, backslashes and control characters escaped). private static string Literal(string value) => diff --git a/src/Reactor.Cli/Check/Rules/ColorStringLiteralRule.cs b/src/Reactor.Cli/Check/Rules/ColorStringLiteralRule.cs index 094e81200..43b85cefe 100644 --- a/src/Reactor.Cli/Check/Rules/ColorStringLiteralRule.cs +++ b/src/Reactor.Cli/Check/Rules/ColorStringLiteralRule.cs @@ -49,7 +49,7 @@ public RuleSuggestion TryMatch(in RuleContext ctx) var suggestion = match.Groups["suggestion"].Value; var value = match.Groups["value"].Value; - var evidence = suggestion.StartsWith("new FontFamily(", StringComparison.Ordinal) + var evidence = suggestion.Contains(".FontFamily(", StringComparison.Ordinal) ? "*FontFamily key → FontFamily overload" : $"'{Truncate(value)}' renders gray"; diff --git a/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs b/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs index 7955d3e9d..8c3a5cea4 100644 --- a/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs +++ b/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs @@ -211,7 +211,7 @@ public async Task Flags_FontFamily_key_and_suggests_FontFamily_when_overload_exi fontFamilyOverload: true)); test.ExpectedDiagnostics.Add(Dym006(0).WithArguments( "Consolas", - "'ContentControlThemeFontFamily' is a FontFamily resource; did you mean new FontFamily(\"Consolas\")?")); + "'ContentControlThemeFontFamily' is a FontFamily resource; did you mean new Microsoft.UI.Xaml.Media.FontFamily(\"Consolas\")?")); await test.RunAsync(TestContext.Current.CancellationToken); } @@ -323,7 +323,7 @@ public async Task FontFamily_suggestion_renames_a_named_argument_and_escapes_the fontFamilyOverload: true)); test.ExpectedDiagnostics.Add(Dym006(0).WithArguments( @"C:\Fonts\Mono.ttf#Mono", - @"'ContentControlThemeFontFamily' is a FontFamily resource; did you mean fontFamily: new FontFamily(""C:\\Fonts\\Mono.ttf#Mono"")?")); + @"'ContentControlThemeFontFamily' is a FontFamily resource; did you mean fontFamily: new Microsoft.UI.Xaml.Media.FontFamily(""C:\\Fonts\\Mono.ttf#Mono"")?")); await test.RunAsync(TestContext.Current.CancellationToken); } diff --git a/tests/Reactor.Tests/AnalyzerTests/ColorStringParityTests.cs b/tests/Reactor.Tests/AnalyzerTests/ColorStringParityTests.cs index e690be0cd..27851f303 100644 --- a/tests/Reactor.Tests/AnalyzerTests/ColorStringParityTests.cs +++ b/tests/Reactor.Tests/AnalyzerTests/ColorStringParityTests.cs @@ -55,9 +55,9 @@ public void Every_fixable_detail_ends_in_the_did_you_mean_the_cli_rule_reads() } [Theory] - [InlineData("Consolas", null, "new FontFamily(\"Consolas\")")] - [InlineData("Consolas", "fontFamily", "fontFamily: new FontFamily(\"Consolas\")")] - [InlineData(@"C:\Fonts\a""b.ttf#A", null, @"new FontFamily(""C:\\Fonts\\a\""b.ttf#A"")")] + [InlineData("Consolas", null, "new Microsoft.UI.Xaml.Media.FontFamily(\"Consolas\")")] + [InlineData("Consolas", "fontFamily", "fontFamily: new Microsoft.UI.Xaml.Media.FontFamily(\"Consolas\")")] + [InlineData(@"C:\Fonts\a""b.ttf#A", null, @"new Microsoft.UI.Xaml.Media.FontFamily(""C:\\Fonts\\a\""b.ttf#A"")")] public void FontFamily_suggestion_is_compilable_CSharp(string value, string? argumentName, string expected) { Assert.Equal(expected, AnalyzerClassifier.FontFamilySuggestion(value, argumentName)); diff --git a/tests/Reactor.Tests/AnalyzerTests/DidYouMeanAnalyzerParityTests.cs b/tests/Reactor.Tests/AnalyzerTests/DidYouMeanAnalyzerParityTests.cs index 9bd87582c..920d7edfe 100644 --- a/tests/Reactor.Tests/AnalyzerTests/DidYouMeanAnalyzerParityTests.cs +++ b/tests/Reactor.Tests/AnalyzerTests/DidYouMeanAnalyzerParityTests.cs @@ -181,11 +181,11 @@ private static SuggestionResult RunSymbolSuggester(string userCode, string code) [InlineData(@"TextBlock(""hi"").Background(""ff00ff"")", false, @"""#ff00ff""")] [InlineData(@"BrushHelper.Parse(""orange"")", false, @"""#FFA500""")] [InlineData(@"new ResourceBuilder().Set(""ButtonBackground"", ""Wheat"")", false, @"""#F5DEB3""")] - [InlineData(@"new ResourceBuilder().Set(""ContentControlThemeFontFamily"", ""Consolas"")", true, @"new FontFamily(""Consolas"")")] + [InlineData(@"new ResourceBuilder().Set(""ContentControlThemeFontFamily"", ""Consolas"")", true, @"new Microsoft.UI.Xaml.Media.FontFamily(""Consolas"")")] // A named argument must be renamed to bind to the FontFamily overload. - [InlineData(@"new ResourceBuilder().Set(key: ""ContentControlThemeFontFamily"", color: ""Consolas"")", true, @"fontFamily: new FontFamily(""Consolas"")")] + [InlineData(@"new ResourceBuilder().Set(key: ""ContentControlThemeFontFamily"", color: ""Consolas"")", true, @"fontFamily: new Microsoft.UI.Xaml.Media.FontFamily(""Consolas"")")] // Backslashes and quotes are escaped as a regular C# literal, whatever form the source used. - [InlineData(@"new ResourceBuilder().Set(""ContentControlThemeFontFamily"", @""Assets\Fonts\Mono.ttf#Mono """"x"""""")", true, @"new FontFamily(""Assets\\Fonts\\Mono.ttf#Mono \""x\"""")")] + [InlineData(@"new ResourceBuilder().Set(""ContentControlThemeFontFamily"", @""Assets\Fonts\Mono.ttf#Mono """"x"""""")", true, @"new Microsoft.UI.Xaml.Media.FontFamily(""Assets\\Fonts\\Mono.ttf#Mono \""x\"""")")] public void Color_string_analyzer_replacement_matches_cli_rule(string expression, bool fontFamilyOverload, string expected) { var (diagnostics, c) = ColorStringLiteralRuleTests.AnalyzeRaw(expression, fontFamilyOverload); diff --git a/tests/Reactor.Tests/CheckCommandTests/Rules/ColorStringLiteralRuleTests.cs b/tests/Reactor.Tests/CheckCommandTests/Rules/ColorStringLiteralRuleTests.cs index 377ec04de..4251e6c84 100644 --- a/tests/Reactor.Tests/CheckCommandTests/Rules/ColorStringLiteralRuleTests.cs +++ b/tests/Reactor.Tests/CheckCommandTests/Rules/ColorStringLiteralRuleTests.cs @@ -58,7 +58,7 @@ public void Suggests_FontFamily_when_the_overload_exists() var (diags, c) = Analyze(@"new ResourceBuilder().Set(""ContentControlThemeFontFamily"", ""Consolas"")", fontFamilyOverload: true); var s = RunRule(Assert.Single(diags), c); Assert.NotNull(s); - Assert.Equal("new FontFamily(\"Consolas\")", s!.Text); + Assert.Equal("new Microsoft.UI.Xaml.Media.FontFamily(\"Consolas\")", s!.Text); } [Theory] @@ -123,6 +123,37 @@ public void Silent_when_the_targets_are_missing() Assert.Null(RunRule(diag, renamed)); } + [Theory] + [InlineData(@"TextBlock(""hi"").Background(""orange"")", false)] + [InlineData(@"TextBlock(""hi"").Foreground(color: ""#888"")", false)] + [InlineData(@"new ResourceBuilder().Set(""ContentControlThemeFontFamily"", ""Consolas"")", true)] + [InlineData(@"new ResourceBuilder().Set(key: ""ContentControlThemeFontFamily"", color: ""Consolas"")", true)] + [InlineData(@"new ResourceBuilder().Set(""ContentControlThemeFontFamily"", @""C:\Fonts\a""""b.ttf#A"")", true)] + public async Task Applying_the_suggestion_compiles_and_clears_the_diagnostic(string expression, bool fontFamilyOverload) + { + // The source under test has no `using Microsoft.UI.Xaml.Media;`, so an unqualified + // `new FontFamily(...)` suggestion would fail with CS0246 here. + var (diagnostics, c) = AnalyzeRaw(expression, fontFamilyOverload); + var d = Assert.Single(diagnostics); + var s = RunRule(ToCheckDiag(d), c); + Assert.NotNull(s); + + // A FontFamily suggestion replaces the whole argument (it may carry a renamed `name:`); a color + // suggestion replaces only the flagged literal, keeping any `color:` name. + var tree = d.Location.SourceTree!; + var flagged = tree.GetRoot(TestContext.Current.CancellationToken).FindNode(d.Location.SourceSpan, getInnermostNodeForTie: true); + var span = s!.Text.Contains(".FontFamily(", StringComparison.Ordinal) + ? flagged.FirstAncestorOrSelf()!.Span + : d.Location.SourceSpan; + var text = tree.GetText(TestContext.Current.CancellationToken).Replace(span, s.Text); + var fixedCompilation = c.ReplaceSyntaxTree(tree, tree.WithChangedText(text)); + + Assert.Empty(fixedCompilation.GetDiagnostics(TestContext.Current.CancellationToken).Where(x => x.Severity == DiagnosticSeverity.Error)); + var after = await fixedCompilation.WithAnalyzers(ImmutableArray.Create(new ColorStringLiteralAnalyzer())) + .GetAnalyzerDiagnosticsAsync(TestContext.Current.CancellationToken); + Assert.DoesNotContain(after, x => x.Id == ColorStringLiteralAnalyzer.DiagnosticId); + } + internal static (CheckCommand.Diag[] Diags, CSharpCompilation Compilation) Analyze(string expression, bool fontFamilyOverload = false) { var (found, c) = AnalyzeRaw(expression, fontFamilyOverload); From 7902633911c3b42c62d5479536be5648af13c565 Mon Sep 17 00:00:00 2001 From: Alexandre Zollinger Chohfi Date: Thu, 15 Oct 2026 11:57:24 -0700 Subject: [PATCH 05/10] Address review round 3 Without a rewrite of the argument, a *FontFamily key's .FontFamily(...) modifier advice was truncated out of mur check output. The rule now surfaces it as the try: text; test asserts on the final formatted line. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b58aacd0-29b6-46c1-9603-b8a8b4eecbd8 --- docs/specs/061-in-build-did-you-mean.md | 5 ++-- .../Check/Rules/ColorStringLiteralRule.cs | 25 ++++++++++++++++--- .../Rules/ColorStringLiteralRuleTests.cs | 24 +++++++++++++++--- 3 files changed, 44 insertions(+), 10 deletions(-) diff --git a/docs/specs/061-in-build-did-you-mean.md b/docs/specs/061-in-build-did-you-mean.md index b74f9c745..b2021bd0b 100644 --- a/docs/specs/061-in-build-did-you-mean.md +++ b/docs/specs/061-in-build-did-you-mean.md @@ -369,8 +369,9 @@ rather than a CS code — there is no compiler diagnostic to augment. `mur check runs the analyzer bundle, so `REACTOR_DYM_006` appears in the parsed output. Every fixable message ends in `did you mean X?`, computed for the exact value the diagnostic points at, so the rule lifts `X` out of the message rather than re-deriving it. It has no grammar of its own and can't disagree with the code fix, and -the `→ try:` suffix keeps the rewrite visible after `mur check` truncates the message. Messages without a -did-you-mean stay silent. There is deliberately no Tier-1 `HintFor` entry, which would mask the suggestion. +the `→ try:` suffix keeps the rewrite visible after `mur check` truncates the message. A `*FontFamily` +key with no rewrite surfaces the message's `.FontFamily("…")` modifier advice the same way; messages with no +concrete next step (unknown name, malformed hex) stay silent. There is deliberately no Tier-1 `HintFor` entry, which would mask the suggestion. `DidYouMeanAnalyzerParityTests` feeds real analyzer diagnostics to the rule and asserts it suggests the code fix's rewrite; `ColorStringParityTests` pins the `did you mean` suffix. diff --git a/src/Reactor.Cli/Check/Rules/ColorStringLiteralRule.cs b/src/Reactor.Cli/Check/Rules/ColorStringLiteralRule.cs index 43b85cefe..5cd061423 100644 --- a/src/Reactor.Cli/Check/Rules/ColorStringLiteralRule.cs +++ b/src/Reactor.Cli/Check/Rules/ColorStringLiteralRule.cs @@ -12,8 +12,11 @@ // diagnostic points at. The rule lifts X out of that message rather than // re-deriving it, so it can never disagree with the IDE code fix — and since // CheckCommand truncates messages to 100 chars, the `→ try:` suffix is what -// keeps the rewrite visible. Messages without a did-you-mean (unknown name, -// malformed hex, FontFamily key without the FontFamily overload) stay silent. +// keeps the rewrite visible. A *FontFamily key without the FontFamily overload +// has no rewrite of the flagged argument, but its message names the +// `.FontFamily("…")` element modifier as the way to set the font; the rule +// surfaces that too, because truncation would otherwise hide it. Messages with +// no concrete next step (unknown name, malformed hex) stay silent. // It deliberately has NO Tier-1 HintFor entry in CheckCommand — that table // wins ties and would mask the suggestion. // @@ -40,12 +43,26 @@ internal sealed partial class ColorStringLiteralRule : IRulePattern [GeneratedRegex(@"^'(?.*?)' is not a color string Reactor can parse\b.*; did you mean (?.+)\?$", RegexOptions.Singleline)] private static partial Regex MessagePattern(); + // `… — '' is a FontFamily resource, but this Set overload only stores color brushes; to change the font, use on the element` + [GeneratedRegex(@"^'.*?' is not a color string Reactor can parse\b.*; to change the font, use (?\.FontFamily\(.+\)) on the element$", RegexOptions.Singleline)] + private static partial Regex FontModifierPattern(); + public RuleSuggestion TryMatch(in RuleContext ctx) { if (ctx.Diagnostic.Id != DiagnosticId) return RuleSuggestion.Silent; - var match = MessagePattern().Match(ctx.Diagnostic.GetMessage()); - if (!match.Success) return RuleSuggestion.Silent; + var message = ctx.Diagnostic.GetMessage(); + var match = MessagePattern().Match(message); + if (!match.Success) + { + var modifier = FontModifierPattern().Match(message); + return modifier.Success + ? new RuleSuggestion( + Text: modifier.Groups["modifier"].Value, + Confidence: 0.9, + Evidence: "*FontFamily key: Set(key, string) only stores brushes") + : RuleSuggestion.Silent; + } var suggestion = match.Groups["suggestion"].Value; var value = match.Groups["value"].Value; diff --git a/tests/Reactor.Tests/CheckCommandTests/Rules/ColorStringLiteralRuleTests.cs b/tests/Reactor.Tests/CheckCommandTests/Rules/ColorStringLiteralRuleTests.cs index 4251e6c84..ad444f32f 100644 --- a/tests/Reactor.Tests/CheckCommandTests/Rules/ColorStringLiteralRuleTests.cs +++ b/tests/Reactor.Tests/CheckCommandTests/Rules/ColorStringLiteralRuleTests.cs @@ -61,9 +61,24 @@ public void Suggests_FontFamily_when_the_overload_exists() Assert.Equal("new Microsoft.UI.Xaml.Media.FontFamily(\"Consolas\")", s!.Text); } + [Fact] + public void Without_the_FontFamily_overload_the_modifier_guidance_survives_mur_check_truncation() + { + // No rewrite of the argument exists, but the message's `.FontFamily(...)` workaround must still + // reach the agent: CheckCommand truncates messages to 100 chars, which cuts it off. + var (diags, c) = Analyze(@"new ResourceBuilder().Set(""ContentControlThemeFontFamily"", ""Consolas"")"); + var diag = Assert.Single(diags); + var s = RunRule(diag, c); + Assert.NotNull(s); + Assert.Equal(".FontFamily(\"Consolas\")", s!.Text); + + var line = diag.Format(s); + Assert.DoesNotContain("to change the font", line, StringComparison.Ordinal); // truncated away... + Assert.Contains("→ try: .FontFamily(\"Consolas\")", line, StringComparison.Ordinal); // ...but surfaced here + } + [Theory] - // No concrete rewrite exists for these — the analyzer's message carries the advice. - [InlineData(@"new ResourceBuilder().Set(""ContentControlThemeFontFamily"", ""Consolas"")")] + // No concrete next step exists for these — the analyzer's message carries the advice. [InlineData(@"TextBlock(""hi"").Background(""accent"")")] [InlineData(@"TextBlock(""hi"").Background(""#12345"")")] public void Silent_when_there_is_no_concrete_rewrite(string expression) @@ -87,9 +102,10 @@ public void Each_branch_gets_its_own_rewrite() [Fact] public void FontFamily_value_inside_a_branch_gets_no_FontFamily_suggestion() { - // Matches the code fix, which can't wrap one branch of a ?: in new FontFamily(...). + // Matches the code fix, which can't wrap one branch of a ?: in new FontFamily(...); the element + // modifier is the remaining advice. var (diags, c) = Analyze(@"new ResourceBuilder().Set(""ContentControlThemeFontFamily"", on ? ""Consolas"" : ""red"")", fontFamilyOverload: true); - Assert.Null(RunRule(Assert.Single(diags), c)); + Assert.Equal(".FontFamily(\"Consolas\")", RunRule(Assert.Single(diags), c)?.Text); } [Fact] From 7fe1c19445974d6e027f54031a520722282c41e6 Mon Sep 17 00:00:00 2001 From: Alexandre Zollinger Chohfi Date: Thu, 15 Oct 2026 12:10:59 -0700 Subject: [PATCH 06/10] Address review round 4 Record REACTOR_DYM_006 and its TreatWarningsAsErrors impact in the CHANGELOG, per the changelog's contributor conventions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b58aacd0-29b6-46c1-9603-b8a8b4eecbd8 --- CHANGELOG.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99ad437fa..49b9ed7b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,18 @@ Conventions for contributors: ### Added +- **`REACTOR_DYM_006`: color strings that render gray** (issue #1375, spec 061 §7.3). A new + default-on **Warning** flags compile-time-constant color strings that `BrushHelper` can't + parse and therefore silently fall back to gray: `.Background("orange")`, `.Foreground("#888")`, + or a font name passed to `ResourceBuilder.Set(key, string)` under a `*FontFamily` key. It covers + `BrushHelper.Parse`, `ResourceBuilder.Set(string, string)` and the `string` overloads of + `Background` / `Foreground` / `BorderBrush` / `WithBorder`, suggests the exact hex (or + `new FontFamily(...)` once that `Set` overload exists), and ships a code fix; `mur check`'s new + `ColorStringLiteralRule` repeats the rewrite as a `→ try:` hint. Unlike `REACTOR_DYM_001`–`005` + it fires on code that compiles, so a project with `TreatWarningsAsErrors` can stop building on + an existing gray color. Fix the value, or adjust the severity with + `dotnet_diagnostic.REACTOR_DYM_006.severity` in `.editorconfig`. + - **Inspector diagnostics: call-site column and a per-component render event** (issue #1326, spec 010 §1.1): - `SourceLocation.ColumnNumber` (1-based, `0` = unknown), a third positional From 5d22042bea617bdcb03e2fc97a559dcfdfc62545 Mon Sep 17 00:00:00 2001 From: Alexandre Zollinger Chohfi Date: Thu, 15 Oct 2026 12:24:27 -0700 Subject: [PATCH 07/10] Address review round 5 A throw branch never yields null, so a ?? fallback after (cond ? "red" : throw ...) is unreachable and no longer reported. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b58aacd0-29b6-46c1-9603-b8a8b4eecbd8 --- src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs | 8 ++++++-- .../AnalyzerTests/ColorStringLiteralAnalyzerTests.cs | 7 +++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs b/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs index c5f625223..fc36a5074 100644 --- a/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs +++ b/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs @@ -293,11 +293,15 @@ BinaryExpressionSyntax b when b.IsKind(SyntaxKind.CoalesceExpression) => }; /// - /// Whether the expression provably evaluates to a non-null value: a non-null constant, or a - /// value-selecting shape whose every live branch is. Unknown (e.g. a variable) is . + /// Whether the expression provably evaluates to a non-null value: a non-null constant, a + /// throw (it never yields at all), or a value-selecting shape whose every live branch is one + /// of those. Unknown (e.g. a variable) is . /// private static bool IsKnownNonNull(ExpressionSyntax expression, SemanticModel model, CancellationToken cancellationToken) { + if (expression is ThrowExpressionSyntax) + return true; + var constant = model.GetConstantValue(expression, cancellationToken); if (constant.HasValue) return constant.Value is not null; diff --git a/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs b/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs index 8c3a5cea4..7e87e0fb7 100644 --- a/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs +++ b/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs @@ -307,6 +307,13 @@ static object M(string c, bool on) => TextBlock(""hi"") .Foreground((on ? ""red"" : ""white"") ?? ""teal"") .BorderBrush((on ? c : ""red"") ?? {|REACTOR_DYM_006:""navy""|});"); + [Fact] + public Task Coalesce_fallback_is_dead_when_the_other_branch_throws() => + // Execution either yields "red" or throws, so "orange" is unreachable. + RunAsync(@" static object M(string c, bool on) => TextBlock(""hi"") + .Background((on ? ""red"" : throw new global::System.InvalidOperationException()) ?? ""orange"") + .Foreground((on ? c : throw new global::System.InvalidOperationException()) ?? {|REACTOR_DYM_006:""teal""|});"); + [Fact] public Task Switch_pruning_compares_the_converted_pattern_constant() => // `1` is an int literal but the pattern converts it to the governing `long`. From 67fba9a7566ecd8bf7f56bb17ddaa9ce2e173e7d Mon Sep 17 00:00:00 2001 From: Alexandre Zollinger Chohfi Date: Thu, 15 Oct 2026 12:42:18 -0700 Subject: [PATCH 08/10] Address review round 6 Skip arguments that reach the color parameter through a user-defined conversion: the operator's result, not the source strings, is parsed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b58aacd0-29b6-46c1-9603-b8a8b4eecbd8 --- src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs | 4 ++++ .../AnalyzerTests/ColorStringLiteralAnalyzerTests.cs | 11 +++++++++++ 2 files changed, 15 insertions(+) diff --git a/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs b/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs index fc36a5074..3c781a562 100644 --- a/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs +++ b/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs @@ -208,6 +208,10 @@ private static void AnalyzeInvocation(SyntaxNodeAnalysisContext context, Targets } if (colorArg is null || colorArg.ArgumentKind != ArgumentKind.Explicit) return; + // Through a user-defined conversion (e.g. a `ColorToken` with `implicit operator string`) the + // source strings never reach BrushHelper — the operator's result does — so the syntax says nothing. + if (colorArg.Value is IConversionOperation { Conversion.IsUserDefined: true }) + return; string? resourceKey = null; if (targets.ResourceSet is not null diff --git a/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs b/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs index 7e87e0fb7..e4fe3f282 100644 --- a/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs +++ b/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs @@ -307,6 +307,17 @@ static object M(string c, bool on) => TextBlock(""hi"") .Foreground((on ? ""red"" : ""white"") ?? ""teal"") .BorderBrush((on ? c : ""red"") ?? {|REACTOR_DYM_006:""navy""|});"); + [Fact] + public Task Silent_through_a_user_defined_conversion() => + // `token ?? "orange"` is a ColorToken; its operator decides the string BrushHelper receives, + // so "orange" is never parsed. Positive control: once the coalesce is over strings, it is. + RunAsync(@" sealed class ColorToken + { + public static implicit operator ColorToken(string s) => new ColorToken(); + public static implicit operator string(ColorToken t) => ""red""; + } + static object M(ColorToken token) => BrushHelper.Parse(token ?? ""orange"") ?? BrushHelper.Parse((string)token ?? {|REACTOR_DYM_006:""orange""|});"); + [Fact] public Task Coalesce_fallback_is_dead_when_the_other_branch_throws() => // Execution either yields "red" or throws, so "orange" is unreachable. From ca579ccd82e4240c7667b520802a3ed24a4319ad Mon Sep 17 00:00:00 2001 From: Alexandre Zollinger Chohfi Date: Thu, 15 Oct 2026 12:59:53 -0700 Subject: [PATCH 09/10] Address review round 7 Treat any non-string or user-defined-conversion sub-expression as a boundary while walking selector branches, not just the whole argument. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b58aacd0-29b6-46c1-9603-b8a8b4eecbd8 --- .../ColorStringLiteralAnalyzer.cs | 19 +++++++++++++++++++ .../ColorStringLiteralAnalyzerTests.cs | 4 +++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs b/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs index 3c781a562..980c95aff 100644 --- a/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs +++ b/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs @@ -271,11 +271,28 @@ private static void AnalyzeInvocation(SyntaxNodeAnalysisContext context, Targets yield break; } + if (!IsPlainString(expression, model, cancellationToken)) + yield break; + foreach (var child in LiveChildren(expression, model, cancellationToken)) foreach (var item in ConstantStrings(child, model, cancellationToken)) yield return item; } + /// + /// Whether a non-constant sub-expression is itself a string value that flows on unchanged. A branch + /// of another type (e.g. token ?? "orange" where token is a ColorToken with + /// implicit operator string) or one reaching string through a user-defined conversion is a + /// boundary: whatever strings it contains, the operator's result is what gets parsed. + /// + private static bool IsPlainString(ExpressionSyntax expression, SemanticModel model, CancellationToken cancellationToken) + { + var typeInfo = model.GetTypeInfo(expression, cancellationToken); + if ((typeInfo.Type ?? typeInfo.ConvertedType)?.SpecialType != SpecialType.System_String) + return false; + return !model.GetConversion(expression, cancellationToken).IsUserDefined; + } + /// /// The sub-expressions a value-selecting shape can actually yield. Empty for any other shape. /// @@ -305,6 +322,8 @@ private static bool IsKnownNonNull(ExpressionSyntax expression, SemanticModel mo { if (expression is ThrowExpressionSyntax) return true; + if (!IsPlainString(expression, model, cancellationToken)) + return false; var constant = model.GetConstantValue(expression, cancellationToken); if (constant.HasValue) diff --git a/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs b/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs index e4fe3f282..d61cf4367 100644 --- a/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs +++ b/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs @@ -316,7 +316,9 @@ public Task Silent_through_a_user_defined_conversion() => public static implicit operator ColorToken(string s) => new ColorToken(); public static implicit operator string(ColorToken t) => ""red""; } - static object M(ColorToken token) => BrushHelper.Parse(token ?? ""orange"") ?? BrushHelper.Parse((string)token ?? {|REACTOR_DYM_006:""orange""|});"); + static object M(ColorToken token, bool on) => BrushHelper.Parse(token ?? ""orange"") + ?? BrushHelper.Parse(on switch { true => token ?? ""orange"", _ => ""red"" }) + ?? BrushHelper.Parse((string)token ?? {|REACTOR_DYM_006:""orange""|});"); [Fact] public Task Coalesce_fallback_is_dead_when_the_other_branch_throws() => From e34df1c9e05c96d41616ddc5e95bb7bfd0077f4b Mon Sep 17 00:00:00 2001 From: Alexandre Zollinger Chohfi Date: Thu, 15 Oct 2026 13:14:46 -0700 Subject: [PATCH 10/10] Address review round 8 A ?? whose left side reaches string through a user-defined conversion is never provably non-null, so its outer fallback stays analyzed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b58aacd0-29b6-46c1-9603-b8a8b4eecbd8 --- src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs | 7 +++++++ .../AnalyzerTests/ColorStringLiteralAnalyzerTests.cs | 10 ++++++++++ 2 files changed, 17 insertions(+) diff --git a/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs b/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs index 980c95aff..568cf95ff 100644 --- a/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs +++ b/src/Reactor.Analyzers/ColorStringLiteralAnalyzer.cs @@ -330,7 +330,14 @@ private static bool IsKnownNonNull(ExpressionSyntax expression, SemanticModel mo return constant.Value is not null; if (expression is BinaryExpressionSyntax b && b.IsKind(SyntaxKind.CoalesceExpression)) + { + // A non-null left value can still become null through a user-defined conversion + // (`token ?? "red"` with an `implicit operator string` that returns null), so such a + // coalesce is never provably non-null, whatever its right side is. + if (model.GetOperation(b, cancellationToken) is ICoalesceOperation { ValueConversion.IsUserDefined: true }) + return false; return IsKnownNonNull(b.Left, model, cancellationToken) || IsKnownNonNull(b.Right, model, cancellationToken); + } var any = false; foreach (var child in LiveChildren(expression, model, cancellationToken)) diff --git a/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs b/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs index d61cf4367..4b1662936 100644 --- a/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs +++ b/tests/Reactor.Tests/AnalyzerTests/ColorStringLiteralAnalyzerTests.cs @@ -320,6 +320,16 @@ static object M(ColorToken token, bool on) => BrushHelper.Parse(token ?? ""orang ?? BrushHelper.Parse(on switch { true => token ?? ""orange"", _ => ""red"" }) ?? BrushHelper.Parse((string)token ?? {|REACTOR_DYM_006:""orange""|});"); + [Fact] + public Task Coalesce_over_a_one_way_user_defined_conversion_is_not_provably_non_null() => + // `token ?? "red"` is a string, but a non-null token converts through an operator that may + // return null, so the outer "orange" fallback is reachable. + RunAsync(@" sealed class NullableToken + { + public static implicit operator string(NullableToken t) => null!; + } + static object M(NullableToken token) => TextBlock(""hi"").Background((token ?? ""red"") ?? {|REACTOR_DYM_006:""orange""|});"); + [Fact] public Task Coalesce_fallback_is_dead_when_the_other_branch_throws() => // Execution either yields "red" or throws, so "orange" is unreachable.