You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Add REACTOR_DYM_006: flag color strings BrushHelper can't parse - #1384
BrushHelper.ParseColor only understands red, green, blue, white, black, gray/grey, lightgray/lightgrey, transparent, #RRGGBB and #AARRGGBB. Anything else silently becomes a gray brush, so .Background("orange"), .Foreground("#888") or .Resources(r => r.Set("ContentControlThemeFontFamily", "Consolas")) compile and quietly render wrong.
This adds a new REACTOR_DYM_006 analyzer (Warning, Reactor.DidYouMean), a code fix, and a matching mur check rule. The analyzer flags compile-time-constant strings passed to BrushHelper.Parse, ResourceBuilder.Set(string, string), and the string overloads of Background/Foreground/BorderBrush/WithBorder (generic and RichText).
What it catches. Calls are matched by symbol identity, so look-alike APIs in other namespaces never trigger it. It checks literals, consts, constant concatenation, and each constant branch of ?: / ?? / switch. It skips branches that provably can't run (a constant condition, a ?? whose left side is provably non-null, a throw branch, a constant switch). It also stops where a value reaches the color parameter through a user-defined conversion, because then the operator's result is what gets parsed.
Missing #: six or eight hex digits get one ("FF0000" → "#FF0000").
CSS / WinUI color names: 139 names map to their exact hex ("orange" → "#FFA500"). The table is cross-checked against System.Drawing.KnownColor.
*FontFamily resource key: suggests new FontFamily("…") (escaped, and renaming a color: named argument). This only turns on when ResourceBuilder.Set(string, FontFamily) exists (Add typed ResourceBuilder overloads for native styling values #1370). Until then, the message points at the .FontFamily("…") element modifier instead.
No fix: malformed hex and unknown names get guidance only.
mur check. This mistake raises no compiler error, so the new ColorStringLiteralRule hooks onto the analyzer's own ID. It is the first rule to do that. It reads the rewrite from the analyzer's message instead of re-deriving it, so it can't disagree with the code fix. Its → try: suffix keeps the rewrite, or the .FontFamily(...) advice, visible after mur check cuts the message short. The suggested type is namespace-qualified, so the text compiles without extra usings.
BrushHelper gets an internal TryParseColor (no behavior change) so parity tests can compare against the runtime grammar exactly. The analyzer can't reference src/Reactor, so it carries its own copy of that grammar.
The new rule found five places in the samples that really did render gray. They're fixed here: "AliceBlue" ×3 in wct-controls and "#888" ×2 in ReactorHostControlDemo. The warning and its TreatWarningsAsErrors impact are recorded in CHANGELOG.md.
Linked issue / spec
Fixes#1375. Design recorded in docs/specs/061-in-build-did-you-mean.md §7.3.
Test plan
New unit tests:
ColorStringLiteralAnalyzerTests: every covered API, each did-you-mean case, constant shapes, dead-branch pruning (constant conditions, nested ??, throw branches, constant switches with converted pattern constants), user-defined conversion boundaries, FontFamily with and without the overload (including named arguments and escaped values), code-fix round-trips, and a test against the live Reactor assembly that pins the analyzer's targets.
ColorStringParityTests: the analyzer's grammar vs BrushHelper.TryParseColor over edge cases, plus the escaping of the FontFamily suggestion.
ColorStringLiteralRuleTests: the CLI rule, driven by real analyzer output. One test applies each → try: suggestion, recompiles, and confirms the diagnostic is gone; another checks the final truncated mur check line.
DidYouMeanAnalyzerParityTests: the CLI rule and the code fix suggest the same rewrite.
ParseColor_Resolves_Named_And_Hex: exact color values for every built-in name, plus the gray fallback.
Mutation checks: removing "grey" from the analyzer grammar, removing the constant-branch pruning, and removing the user-defined-conversion boundary each make the tests fail.
dotnet test tests/Reactor.Tests -c Release -p:Platform=x64 → 14,693 passed, 64 skipped, 0 failed. On the final head, --filter-class "*AnalyzerTests*" --filter-class "*CheckCommandTests*" → 3,435 passed, 0 failed.
dotnet build Reactor.slnx --no-restore -c Release → no CS/REACTOR errors in any project that built (before the sample fixes, it reported exactly the five sample hits above). The local NuGet feed is unreachable, so that run also logged NU1900/NU1301 restore errors; CI's Build solution job is the authoritative gate and is green.
WctControls and ReactorHostControlDemo built in Release with 0 warnings after the fixes.
dotnet run --project tools/Reactor.DocPipeline -- compile --topic analyzer-architecture → compiled.
Risk / breaking changes
New build-time warning for consumers. Unlike REACTOR_DYM_001–005, this fires on code that compiles. Projects with TreatWarningsAsErrors will fail to build when they pass an unparseable constant color string. Those values already render gray, so the build failure exposes an existing bug rather than creating a new one. Severity can be tuned per project in .editorconfig.
Overlap with REACTOR_THEME_001. THEME_001 already flags any literal passed to .Background/.Foreground/.WithBorder as a style issue. Both can fire on the same literal.
No public API change.BrushHelper.TryParseColor and FallbackGray are internal. Runtime parsing behavior is unchanged.
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
- 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
Artifact sizes for e34df1c vs the base branch (3584661).
Packages (compressed .nupkg)
Artifact
base
PR
Δ
Microsoft.UI.Reactor.nupkg
1.81 MB
1.82 MB
+9.38 KB (+0.51%)
⚠️
Microsoft.UI.Reactor.Advanced.nupkg
493.3 KB
493.3 KB
+4 B (0.00%)
≈
Microsoft.UI.Reactor.Devtools.nupkg
288.2 KB
288.2 KB
+0 B (0.00%)
≈
Assemblies in Microsoft.UI.Reactor
Artifact
base
PR
Δ
Reactor.Analyzers.dll
386.0 KB
410.5 KB
+24.5 KB (+6.35%)
⚠️
Reactor.dll
2.54 MB
2.54 MB
+0 B (0.00%)
≈
Reactor.Localization.Generator.dll
16.0 KB
16.0 KB
+0 B (0.00%)
≈
Reactor.Wrappers.Abstractions.dll
10.5 KB
10.5 KB
+0 B (0.00%)
≈
Reactor.Wrappers.Generator.dll
99.5 KB
99.5 KB
+0 B (0.00%)
≈
Assemblies in Microsoft.UI.Reactor.Advanced
Artifact
base
PR
Δ
Reactor.Advanced.dll
1.00 MB
1.00 MB
+0 B (0.00%)
≈
Assemblies in Microsoft.UI.Reactor.Devtools
Artifact
base
PR
Δ
Microsoft.UI.Reactor.Devtools.dll
788.0 KB
788.0 KB
+0 B (0.00%)
≈
✅ smaller / ⚠️ larger / ≈ within noise. Sizes come from a Release dotnet pack on the CI runner: packages are the compressed .nupkg download size, assemblies the uncompressed DLL inside it. workflow run.
Coverage for e34df1c vs the base branch (dac50f9) — unit + selftest merged.
Metric
base
PR
Δ
Line
86.74%
86.74%
0.00 pp
≈
Branch
77.44% (968/1250)
77.36% (967/1250)
-0.08 pp
≈
No coverage change beyond the noise floor. ✅
✅ higher / ⚠️ lower / ≈ within noise. Δ is in percentage points; coverage is unit + selftest merged (Debug x64) on the CI runner. Cobertura reports attached to the workflow run as artifacts.
- 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
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
For Set("ContentControlThemeFontFamily", "Consolas") without the FontFamily overload, this returns no suggestion. CheckCommand.Diag.Format then truncates the diagnostic to 100 characters (CheckCommand.cs:380–398), hiding both the resource-type explanation and the .FontFamily("Consolas") workaround. This affects the ResourceBuilder surface shipped here. Preserve the fallback guidance in the CLI output even when no replacement expression is available, and add a regression test for the final formatted output, not just the rule result.
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
Nonblocking: add an entry under CHANGELOG.md → [Unreleased] → Added for this default-enabled warning, citing spec 061 §7.3 and issue #1375. Mention that previously compiling code can fail with TreatWarningsAsErrors and that severity can be adjusted in .editorconfig. CHANGELOG.md:13–19 requires significant changes and their originating spec to be recorded; the analyzer inventory does not explain this upgrade impact.
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
A throwing branch is treated as potentially null here. For example, .Background((on ? "red" : throw new InvalidOperationException()) ?? "orange") reports the unreachable "orange", although execution either passes "red" or throws. This can fail valid warnings-as-errors builds. Treat throw expressions as unable to produce null when checking whether the fallback is reachable, and add a regression test for this case.
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
Skip user-defined argument conversions before walking the source expression. For a reference type ColorToken with implicit conversions both from and to string, BrushHelper.Parse(token ?? "orange") first produces a ColorToken, then converts it to a string. If that conversion returns "red", the parser never receives "orange", but this syntax walk still reports it. That false warning can break valid warnings-as-errors builds. Guard the conversion and add a negative regression test for this coalesce case.
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
The conversion guard only checks the whole argument; this recursive walk loses conversions on selector branches. With the ColorToken operators in Silent_through_a_user_defined_conversion, BrushHelper.Parse(on switch { true => token ?? "orange", _ => "red" }) receives "red" on either path. However, the walker enters the token-valued coalesce and reports "orange", which can fail valid warnings-as-errors builds. Preserve user-defined conversion boundaries when traversing branches and computing non-null results, and add this nested-switch regression alongside the existing top-level conversion test.
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
A non-null right operand does not prove that ?? returns a non-null string when the left operand uses a user-defined conversion. For example, give ColorToken only an implicit conversion to string that returns null. With a non-null token, .Background((token ?? "red") ?? "orange") passes "orange" to the parser, but this proof treats the inner coalesce as non-null and skips the reachable fallback, missing REACTOR_DYM_006. Treat a user-defined ICoalesceOperation.ValueConversion as unknown here, and add a regression test with that one-way, null-returning conversion; the existing conversion test instead defines conversions in both directions.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Summary
BrushHelper.ParseColoronly understandsred,green,blue,white,black,gray/grey,lightgray/lightgrey,transparent,#RRGGBBand#AARRGGBB. Anything else silently becomes a gray brush, so.Background("orange"),.Foreground("#888")or.Resources(r => r.Set("ContentControlThemeFontFamily", "Consolas"))compile and quietly render wrong.This adds a new
REACTOR_DYM_006analyzer (Warning,Reactor.DidYouMean), a code fix, and a matchingmur checkrule. The analyzer flags compile-time-constant strings passed toBrushHelper.Parse,ResourceBuilder.Set(string, string), and thestringoverloads ofBackground/Foreground/BorderBrush/WithBorder(generic and RichText).What it catches. Calls are matched by symbol identity, so look-alike APIs in other namespaces never trigger it. It checks literals,
consts, constant concatenation, and each constant branch of?:/??/ switch. It skips branches that provably can't run (a constant condition, a??whose left side is provably non-null, athrowbranch, a constant switch). It also stops where a value reaches the color parameter through a user-defined conversion, because then the operator's result is what gets parsed.Did-you-mean, applied by the code fix:
#RGB/#ARGB→#RRGGBB/#AARRGGBB("#888"→"#888888").#: six or eight hex digits get one ("FF0000"→"#FF0000")."orange"→"#FFA500"). The table is cross-checked againstSystem.Drawing.KnownColor.*FontFamilyresource key: suggestsnew FontFamily("…")(escaped, and renaming acolor:named argument). This only turns on whenResourceBuilder.Set(string, FontFamily)exists (Add typed ResourceBuilder overloads for native styling values #1370). Until then, the message points at the.FontFamily("…")element modifier instead.mur check. This mistake raises no compiler error, so the newColorStringLiteralRulehooks onto the analyzer's own ID. It is the first rule to do that. It reads the rewrite from the analyzer's message instead of re-deriving it, so it can't disagree with the code fix. Its→ try:suffix keeps the rewrite, or the.FontFamily(...)advice, visible aftermur checkcuts the message short. The suggested type is namespace-qualified, so the text compiles without extrausings.BrushHelpergets an internalTryParseColor(no behavior change) so parity tests can compare against the runtime grammar exactly. The analyzer can't referencesrc/Reactor, so it carries its own copy of that grammar.The new rule found five places in the samples that really did render gray. They're fixed here:
"AliceBlue"×3 in wct-controls and"#888"×2 in ReactorHostControlDemo. The warning and itsTreatWarningsAsErrorsimpact are recorded inCHANGELOG.md.Linked issue / spec
Fixes #1375. Design recorded in
docs/specs/061-in-build-did-you-mean.md§7.3.Test plan
ColorStringLiteralAnalyzerTests: every covered API, each did-you-mean case, constant shapes, dead-branch pruning (constant conditions, nested??,throwbranches, constant switches with converted pattern constants), user-defined conversion boundaries, FontFamily with and without the overload (including named arguments and escaped values), code-fix round-trips, and a test against the live Reactor assembly that pins the analyzer's targets.ColorStringParityTests: the analyzer's grammar vsBrushHelper.TryParseColorover edge cases, plus the escaping of the FontFamily suggestion.ColorStringLiteralRuleTests: the CLI rule, driven by real analyzer output. One test applies each→ try:suggestion, recompiles, and confirms the diagnostic is gone; another checks the final truncatedmur checkline.DidYouMeanAnalyzerParityTests: the CLI rule and the code fix suggest the same rewrite.ParseColor_Resolves_Named_And_Hex: exact color values for every built-in name, plus the gray fallback."grey"from the analyzer grammar, removing the constant-branch pruning, and removing the user-defined-conversion boundary each make the tests fail.dotnet build tests/Reactor.Tests -c Release -p:Platform=x64 -p:SkipSignaturesGen=true→ succeeded, no warnings.dotnet test tests/Reactor.Tests -c Release -p:Platform=x64→ 14,693 passed, 64 skipped, 0 failed. On the final head,--filter-class "*AnalyzerTests*" --filter-class "*CheckCommandTests*"→ 3,435 passed, 0 failed.dotnet build Reactor.slnx --no-restore -c Release→ no CS/REACTOR errors in any project that built (before the sample fixes, it reported exactly the five sample hits above). The local NuGet feed is unreachable, so that run also logged NU1900/NU1301 restore errors; CI'sBuild solutionjob is the authoritative gate and is green.WctControlsandReactorHostControlDemobuilt in Release with 0 warnings after the fixes.dotnet run --project tools/Reactor.DocPipeline -- compile --topic analyzer-architecture→ compiled.Risk / breaking changes
REACTOR_DYM_001–005, this fires on code that compiles. Projects withTreatWarningsAsErrorswill fail to build when they pass an unparseable constant color string. Those values already render gray, so the build failure exposes an existing bug rather than creating a new one. Severity can be tuned per project in.editorconfig.REACTOR_THEME_001. THEME_001 already flags any literal passed to.Background/.Foreground/.WithBorderas a style issue. Both can fire on the same literal.BrushHelper.TryParseColorandFallbackGrayare internal. Runtime parsing behavior is unchanged.FlexLayoutFixtures, perf_bench).