refactor: Remove conditional branches no compilation can take - #24089
refactor: Remove conditional branches no compilation can take#24089MartinZikmund merged 18 commits into
Conversation
Sorry, something went wrong.
|
Claude finished @MartinZikmund's task in 6m 26s —— View job PR Review — refactor: Remove conditional branches no compilation can take
Overall verdict: Ship (3 low-severity findings, all optional)The methodology is rigorous and correctly applied. The three-input check — define set from MSBuild, shared-file exclusion, and configuration-dependent symbol protection — produces correct results, and the mid-stream correction (retaining WASM_SKIA, NETSTANDARD, WINUI, __ANDROID__ after discovering their sites are all in shared files) shows the guard working as designed. The cleanup is conservative in all the right places: DEBUG, feature flags, MUX annotations, parked ported code, and vendor-synced sources are all untouched. The HAS_NATIVE_IMPLICIT_POINTER_CAPTURE removal is particularly well-verified: UIElement.ReRoutePointerSequenceTo does not exist anywhere in the tree (confirmed by search), so those branches were never compilable — removing the symbol definition from CrossTargetting.targets is the correct final step. Findings (3 low, all cosmetic/documentation)
A fourth inline comment is left on code-style.md:39 — the existing wording "#if IS_UNIT_TESTS gates code per build flavor" doesn't communicate that the symbol is absent from Uno.UI's define set, which is exactly the condition this PR cleaned up. Suggested tightening to prevent the same mistake in new code. No correctness, behavioural, or build-system issues found across the 5 new commits. |
Sorry, something went wrong.
|
The build 227932 found UI Test snapshots differences: skia-linux-screenshots: 73, skia-windows-screenshots: 123 Details
|
Sorry, something went wrong.
|
⚠️⚠️ The build 227933 has failed on Uno.UI - docs. |
Sorry, something went wrong.
|
⚠️⚠️ The build 227932 has failed on Uno.UI - CI. |
Sorry, something went wrong.
|
Claude finished @MartinZikmund's task in 6m 16s —— View job PR Review — refactor: Remove conditional branches no compilation can take
Overall verdict: Ship (1 new low-severity cosmetic finding)The five new commits (0f042b28 through a6c55835) are correct and safe. All three previous findings are fixed. The methodology continues to be applied rigorously — UNO_HAS_UIELEMENT_IMPLICIT_PINNING correctly stays defined in CrossTargetting.targets for iOS/tvOS and consumed by XamlFileGenerator, while its never-reachable branches in Uno.UI are removed. The non-existent UIElement.ReRoutePointerSequenceTo call is gone cleanly. Verification of key changes
Findings
Working-tree heads-upThe environment's working tree has unstaged changes in three .claude/rules/ files (.claude/rules/code-style.md, .claude/rules/build-system.md, .claude/rules/platform-targeting.md) that revert improvements committed in a6c55835. These are not part of the PR but could cause confusion if accidentally committed. Running git restore .claude/rules/ will discard them and restore the committed (improved) state. |
Sorry, something went wrong.
|
Claude finished @MartinZikmund's task in 2m 57s —— View job PR Review — refactor: Remove conditional branches no compilation can take
Overall verdict: Ship (1 cosmetic nit remaining)Four commits added since the previous review cycle, all clean:
Findings (1 low, cosmetic)
The fix is a one-liner; a suggestion is attached to the inline comment. No correctness, behavioural, or build-system issues found in the new commits. |
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24089/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-24089/docs/index.html |
Sorry, something went wrong.
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-24089/docs/index.html |
Sorry, something went wrong.
|
The build 227950 found UI Test snapshots differences: skia-linux-screenshots: 73, skia-windows-screenshots: 126 Details
|
Sorry, something went wrong.
|
⚠️⚠️ The build 227950 has failed on Uno.UI - CI. |
Sorry, something went wrong.
Spec 058 estimated 'roughly 15' files shared between Uno.UI and other projects; the sweep established 41, spanning Uno.UI.Composition, Uno.UI.SourceGenerators, Uno.UI.Toolkit.Windows, the Skia runtimes and the SamplesApp heads. The spec is the reference for future sweeps, so it carries the measured number. The code-style rule described IS_UNIT_TESTS as gating code per build flavor without saying where it is valid. It is defined for Uno.UI.UnitTests and the view-library helpers but absent from Uno.UI's define set, so an #if written in Uno.UI source is dead on arrival. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GP2BeVCQzY3sQB7QBv173u
Three residues of the dead-branch removal: a blank line before the closing brace in InputManager.Pointers.cs, a blank line at the top of the TargetPropertyPath class body, and VirtualizingPanelLayout.Stub.cs, which the IS_UNIT_TESTS removal left holding nothing but a UTF-8 BOM. Uno.UI.UnitTests consumes Uno.UI through a ProjectReference rather than by globbing its sources, so no compilation ever saw that file's contents. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015HhivECGyuqHTQ3J5gULjW
Uno.UI defines UNO_HAS_ENHANCED_LIFECYCLE unconditionally, so the `#if !UNO_HAS_ENHANCED_LIFECYCLE` fallback for the removed native targets could never compile. Removing it orphans the Microsoft.UI.Xaml.Input using, which goes too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011vgZ3cDuKKFqDCPDLpWSPu
The file carried two identical `#if !IS_UNIT_TESTS` guards and only one was unwrapped. Both are always true for Uno.UI, which consumes no IS_UNIT_TESTS define. The two doc comments describing the scavenger as disabled under that symbol no longer match anything. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011vgZ3cDuKKFqDCPDLpWSPu
Name the three projects that define IS_UNIT_TESTS and why the symbol cannot reach Uno.UI source: they consume it via ProjectReference, so it only applies to files they Compile-link. Record that UNO_HAS_UIELEMENT_IMPLICIT_PINNING stays defined for iOS/tvOS. XamlFileGenerator emits it into generated x:Bind code, where it makes Bindings.Owner a WeakReference; only the Uno.UI sites were dead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011vgZ3cDuKKFqDCPDLpWSPu
|
Claude finished @MartinZikmund's task in 3m 55s —— View job PR Review — refactor: Remove conditional branches no compilation can take
Overall verdict: Ship (1 low-severity finding, new)Three commits since the previous review (2026-08-25) directly address Xiaoy312's reviewer questions:
HAS_NATIVE_IMPLICIT_POINTER_CAPTURE was already correctly removed from CrossTargetting.targets (iOS, tvOS, Android sections) in an earlier commit; no references remain in the tree. Finding (1 low)
An inline suggestion is attached. Everything else is clean. |
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24089/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-24089/docs/index.html |
Sorry, something went wrong.
Uno.UI defines UNO_HAS_ENHANCED_LIFECYCLE unconditionally, so every `#if !UNO_HAS_ENHANCED_LIFECYCLE` block in it is code no compilation can take. 0cf8cc6 removed one; these are the remaining three. SplitMenuFlyoutItem carried the same OnProcessKeyboardAccelerators fallback as MenuFlyoutSubItem, orphaning the Microsoft.UI.Xaml.Input using with it. ViewManager called OnUnoBeforeElementPrepared, a method not declared anywhere in the tree. CalendarViewBaseItem's `#else` arm declared a parameterless EnterImpl() overload whose only caller was the dead line in the Loaded handler, so the arm folded with the call. The always-true `#if UNO_HAS_ENHANCED_LIFECYCLE` sites stay parked, and the Uno.UI.RuntimeTests / SamplesApp sites are untouched — those compile under WinAppSDK without the symbol, where the branches are live. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qzt3TUqSg3yqfwBWm3Tj4V
§4 noted a single exception taken during review. Three more sites turned out to be dead the same way, so the note now lists all four with what each guarded, states the always-false/always-true line the exception follows, and records why the RuntimeTests and SamplesApp sites are live rather than dead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qzt3TUqSg3yqfwBWm3Tj4V
|
Claude finished @MartinZikmund's task in 1m 23s —— View job PR Review — refactor: Remove conditional branches no compilation can take
Overall verdict: Ship (no findings)Two commits since the previous review (2026-08-26T07:04), both correct and complete.
Verification
The diff is clean and ready to merge. No new findings. |
Sorry, something went wrong.
The note said the runtime-test sites were live and left it there, which read as an endorsement. Native WinUI has enhanced lifecycle, so the six sites that use the symbol unpaired put it on the legacy Uno path — four skip a test there and two assert Uno's divergence against real WinUI. Records the correct count (15 + 1, not 13), the ten correctly-paired sites, and points at #24201 for the fix, which changes what the WinUI test leg runs and so does not belong in this sweep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qzt3TUqSg3yqfwBWm3Tj4V
|
Claude finished @MartinZikmund's task in 1m 11s —— View job PR Review — refactor: Remove conditional branches no compilation can take
Overall verdict: Ship (no findings)One new commit since the previous review (9b0e7ea3 docs: Correct the WinUI enhanced-lifecycle note). It is correct and complete. New commit — 9b0e7ea3Spec §4's note on the enhanced-lifecycle runtime-test sites was previously accurate about them being "live, not dead" but stopped short — it implied endorsement without characterizing the 6 problematic sites. The correction:
Working-tree heads-up (environment only, not a PR issue)The CI runner's working tree carries an unstaged modification to .claude/rules/code-style.md that reverts the improvements committed earlier in this branch. This is local runner state — the committed version (0cfb5fcf) carries the correct wording, and there is no action required on the PR itself. The diff is clean and ready to merge. No new findings. |
Sorry, something went wrong.
Uno.UI builds UnoRuntimeIdentifier=Skia, which defines the flag, so all three of its sites were constant. Two had rotted past compiling: ContentPresenter.EnterImpl called UpdateBorder() and CalendarViewBaseItem assigned _borderRenderer, and neither is declared anywhere in the tree. Removing the latter left an empty Loaded handler, so the subscription went with it. Border keeps this.UpdateBackground() — a real IBorderInfoProvider extension — now unwrapped. With no reference left anywhere, the define and its documentation line go from Uno.CrossTargetting.targets, the same end state HAS_NATIVE_IMPLICIT_POINTER_CAPTURE reached in f629d38. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qzt3TUqSg3yqfwBWm3Tj4V
§4 said the feature flags were best handled one at a time by whoever owns them. This one was small enough to finish, so the section now records it as done rather than parked: the three sites and their disposition, and the removal of the define itself. Adds two distinctions the sweep surfaced. A flag whose dead side no longer compiles is finished rather than parked — that is what separated this one from its neighbours. And this flag was Skia-only, never defined for WebAssembly, so it and UNO_HAS_ENHANCED_LIFECYCLE were not interchangeable despite both reading as "the modern path". Also notes, without fixing, the third dead UpdateBorder() call sitting in a parked `#if !__SKIA__` arm in Border.cs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qzt3TUqSg3yqfwBWm3Tj4V
|
Claude finished @MartinZikmund's task in 1m 28s —— View job PR Review — refactor: Remove conditional branches no compilation can take
Overall verdict: Ship (no findings)Two commits since the previous review (2026-08-26T08:16), both correct and complete.
Verification
No prior findings remain open. The diff is clean and ready to merge. |
Sorry, something went wrong.
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-24089/docs/index.html |
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24089/wasm-skia-net9/index.html |
Sorry, something went wrong.
The type-name sanitizer compared against a literal U+007F byte embedded in the source, which is invisible in review and depends on the file surviving an encoding round-trip. Spell it as an escape instead. Carried over from #24087, which is otherwise already merged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011vgZ3cDuKKFqDCPDLpWSPu
UNOB0020 was only exercised for an android head and only rejected for browserwasm; it now runs for ios and tvos, and the negative case covers desktop and an unset identifier. The selector tests claimed the assets were untouched while asserting only the remove-lists. PackageCacheFixture cleanup also tolerates UnauthorizedAccessException, which Windows raises instead of IOException when a handle is still open. Carried over from #24087, which is otherwise already merged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011vgZ3cDuKKFqDCPDLpWSPu
|
Claude finished @MartinZikmund's task in 1m 1s —— View job PR Review — refactor: Remove conditional branches no compilation can take
Overall verdict: Ship (no findings)Two commits since the previous review (2026-08-26T08:26), both correct and complete. Both are carried over from #24087.
Verification
No prior findings remain open. The diff is clean and ready to merge. |
Sorry, something went wrong.
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-24089/wasm-skia-net9/index.html |
Sorry, something went wrong.
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-24089/docs/index.html |
Sorry, something went wrong.
|
The build 230306 found UI Test snapshots differences: skia-linux-screenshots: 73, skia-windows-screenshots: 208 Details
|
Sorry, something went wrong.
GitHub Issue: part of #8339
PR Type:
🔄 Refactoring (no functional changes, no api changes)
What changed? 🚀
Removal of #if branches that no compilation can take, in src/Uno.UI and src/Uno.UI.Composition. No
behaviour change: in every case the surviving arm is the one that was already being compiled.
How the branches were established, rather than assumed. A conditional is only dead if its expression is
constant for every compilation the file takes part in, which needs three inputs:
Uno.UI.Composition, Uno.UI.SourceGenerators, Uno.UI.Toolkit.Windows, the Skia runtimes and the
SamplesApp heads. Their conditionals are excluded from the sweep.
define set captured from one configuration cannot prove it constant.
The simplification is three-valued: only symbols verified against the above are folded, every other symbol stays
unknown, and a conditional that stays unknown is left byte-for-byte alone. Compound expressions are simplified
rather than guessed at — !__CROSSRUNTIME__ && !IS_UNIT_TESTS becomes !__CROSSRUNTIME__, and the residue of an
earlier sweep, #if false || false || IS_UNIT_TESTS || false || __NETSTD_REFERENCE__, collapses.
What was removed:
Uno.UI.Composition also drops XAMARIN from its DefineConstants: nothing branched on it, neither its own
sources nor the four Uno.UI files it compiles.
Four symbols were not removed despite reading as always-false. NETSTANDARD, WASM_SKIA, WINUI and
__ANDROID__ have every site inside a shared file where the symbol really is defined — the WebView native
sources compiled by the WebAssembly and Android runtimes, the sources compiled by Uno.UI.SourceGenerators, and
VisibleBoundsPadding.cs compiled by the WinAppSDK Toolkit. Sweeping them would have deleted live code.
Also not touched, deliberately: the 144 bare #if false blocks, which are an author's disable rather than a
dead symbol and want individual triage; the ten completed-migration feature flags, which want their owners; and
__SKIA__ / HAS_UNO, which should change once with the drawing-backend work rather than twice. HAS_UNO in
MUX-ported files is provenance for the next upstream sync, not a condition.
After this, src/Uno.UI has no always-false conditional outside those categories: what remains is 50 sites of
parked ported code, 62 of developer diagnostics, 26 of MUX port fidelity, and DEBUG.
Two of the removed blocks had rotted rather than merely gone cold: the HAS_NATIVE_IMPLICIT_POINTER_CAPTURE arm
calls UIElement.ReRoutePointerSequenceTo, a method that exists nowhere in the tree, so it could not have
compiled had the symbol ever been defined.
PR Checklist ✅
Validation
When_Gregorian_FixedDate* tests, which assume a UTC machine and are green on CI —
Windows.Globalization.Calendar lives in src/Uno.UWP, which this branch does not touch.
🤖 Generated with Claude Code
https://claude.ai/code/session_01PTBMNbcPLwzdYrtPzY8s9Z