Reduce boolean boxes across the framework#35596
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35596Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35596" |
|
Hey there @@pictos! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
github-actions
Bot
added
area-controls-general
|
/review -b feature/refactor-copilot-yml |
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 1 findings
See inline comments for details.
| global using Microsoft.Maui; | ||
| global using Microsoft.Maui.Handlers; | ||
| global using Microsoft.Maui.Platform; | ||
| global using Microsoft.Maui.Controls.Internals; |
There was a problem hiding this comment.
[critical] Platform-Specific Code Scoping / Build - Adding Microsoft.Maui.Controls.Internals as a global using imports every internal Controls type into every file in this project. On iOS files that also use Foundation and unqualified [Preserve], this makes PreserveAttribute ambiguous between Foundation.PreserveAttribute and Microsoft.Maui.Controls.Internals.PreserveAttribute, causing the iOS build to fail. Please keep BooleanBoxes references explicit, use a targeted alias/static using, or add local usings only where needed instead of globally importing the whole Internals namespace.
MauiBot
added
s/agent-review-incomplete
s/agent-fix-win
|
/review -b feature/refactor-copilot-yml |
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 1 findings
See inline comments for details.
|
/review -b feature/refactor-copilot-yml |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
I don't think the bot suggestion makes sense. I believe to have one API is better |
pictos
force-pushed
the
pj/avoid-bool-boxing
branch
from
dfa1325 to
63cd5ab
Compare
|
/review -b feature/enhanced-reviewer -p android |
github-actions
Bot
added
the
s/agent-review-in-progress
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@pictos — new AI review results are available based on this last commit:
63cd5ab.
Add production regression tests and fix coerce functions boxing To request a fresh review after new comments or commits, comment/review rerun.
Review Sessions — click to expand
Gate — Test Before & After Fix
Gate Result: ❌ FAILED
Platform: ANDROID · Base: main · Merge base: dd5b6d2e
🩺 Test does not reproduce the bug — ran the same in both states (PASS without fix, PASS with fix). The repro test is not exercising the issue. Strengthen the test before reviewing the fix.
| Test | Without Fix (expect FAIL) | With Fix (expect PASS) |
|---|---|---|
🧪 BooleanBoxesTests BooleanBoxesTests |
❌ PASS — 112s | ✅ PASS — 110s |
🔴 Without fix — 🧪 BooleanBoxesTests: PASS ❌ · 112s
Determining projects to restore...
Restored /home/vsts/work/1/s/src/TestUtils/src/TestUtils/TestUtils.csproj (in 4.22 sec).
Restored /home/vsts/work/1/s/src/Graphics/src/Graphics/Graphics.csproj (in 148 ms).
Restored /home/vsts/work/1/s/src/Essentials/src/Essentials.csproj (in 3.33 sec).
Restored /home/vsts/work/1/s/src/Core/src/Core.csproj (in 415 ms).
Restored /home/vsts/work/1/s/src/Core/maps/src/Maps.csproj (in 9.59 sec).
Restored /home/vsts/work/1/s/src/Controls/src/Xaml/Controls.Xaml.csproj (in 128 ms).
Restored /home/vsts/work/1/s/src/Controls/src/Core/Controls.Core.csproj (in 92 ms).
Restored /home/vsts/work/1/s/src/Controls/Maps/src/Controls.Maps.csproj (in 53 ms).
Restored /home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj (in 1.69 sec).
1 of 10 projects are up-to-date for restore.
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14353332
Graphics -> /home/vsts/work/1/s/artifacts/bin/Graphics/Debug/net10.0/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14353332
Essentials -> /home/vsts/work/1/s/artifacts/bin/Essentials/Debug/net10.0/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14353332
Core -> /home/vsts/work/1/s/artifacts/bin/Core/Debug/net10.0/Microsoft.Maui.dll
Controls.BindingSourceGen -> /home/vsts/work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14353332
Maps -> /home/vsts/work/1/s/artifacts/bin/Maps/Debug/net10.0/Microsoft.Maui.Maps.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14353332
Controls.Core -> /home/vsts/work/1/s/artifacts/bin/Controls.Core/Debug/net10.0/Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14353332
Controls.Xaml -> /home/vsts/work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0/Microsoft.Maui.Controls.Xaml.dll
##vso[build.updatebuildnumber]10.0.80-ci+azdo.14353332
Controls.Maps -> /home/vsts/work/1/s/artifacts/bin/Controls.Maps/Debug/net10.0/Microsoft.Maui.Controls.Maps.dll
TestUtils -> /home/vsts/work/1/s/artifacts/bin/TestUtils/Debug/netstandard2.0/Microsoft.Maui.TestUtils.dll
Controls.Core.UnitTests -> /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.dll
Test run for /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.dll (.NETCoreApp,Version=v10.0)
VSTest version 18.0.1 (x64)
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.8.2+699d445a1a (64-bit .NET 10.0.0)
[xUnit.net 00:00:00.16] Discovering: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.55] Discovered: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.56] Starting: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.64] Finished: Microsoft.Maui.Controls.Core.UnitTests
Passed FalseBoxIsBoxedFalse [11 ms]
Passed BoxNullableNullReturnsNull [< 1 ms]
Passed BoxFalseReturnsFalseBox [< 1 ms]
Passed BoxReturnsSameReferenceOnRepeatedCalls [< 1 ms]
Passed BoxNullableFalseReturnsFalseBox [< 1 ms]
Passed BoxTrueReturnsTrueBox [< 1 ms]
Passed TrueBoxIsBoxedTrue [< 1 ms]
Passed BoxNullableTrueReturnsTrueBox [< 1 ms]
Passed TrueBoxAndFalseBoxAreDifferentInstances [< 1 ms]
Test Run Successful.
Total tests: 9
Passed: 9
Total time: 2.2338 Seconds
🟢 With fix — 🧪 BooleanBoxesTests: PASS ✅ · 110s
Determining projects to restore...
Restored /home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj (in 1.04 sec).
Restored /home/vsts/work/1/s/src/Controls/src/Xaml/Controls.Xaml.csproj (in 114 ms).
Restored /home/vsts/work/1/s/src/Controls/Maps/src/Controls.Maps.csproj (in 1.29 sec).
Restored /home/vsts/work/1/s/src/Controls/src/Core/Controls.Core.csproj (in 52 ms).
Restored /home/vsts/work/1/s/src/TestUtils/src/TestUtils/TestUtils.csproj (in 13 ms).
Restored /home/vsts/work/1/s/src/Controls/src/BindingSourceGen/Controls.BindingSourceGen.csproj (in 62 ms).
Restored /home/vsts/work/1/s/src/Essentials/src/Essentials.csproj (in 24 ms).
Restored /home/vsts/work/1/s/src/Graphics/src/Graphics/Graphics.csproj (in 49 ms).
Restored /home/vsts/work/1/s/src/Core/maps/src/Maps.csproj (in 75 ms).
Restored /home/vsts/work/1/s/src/Core/src/Core.csproj (in 101 ms).
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14353332
Graphics -> /home/vsts/work/1/s/artifacts/bin/Graphics/Debug/net10.0/Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14353332
Essentials -> /home/vsts/work/1/s/artifacts/bin/Essentials/Debug/net10.0/Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14353332
Core -> /home/vsts/work/1/s/artifacts/bin/Core/Debug/net10.0/Microsoft.Maui.dll
Controls.BindingSourceGen -> /home/vsts/work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14353332
Maps -> /home/vsts/work/1/s/artifacts/bin/Maps/Debug/net10.0/Microsoft.Maui.Maps.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14353332
Controls.Core -> /home/vsts/work/1/s/artifacts/bin/Controls.Core/Debug/net10.0/Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14353332
##vso[build.updatebuildnumber]10.0.90-ci+azdo.14353332
Controls.Maps -> /home/vsts/work/1/s/artifacts/bin/Controls.Maps/Debug/net10.0/Microsoft.Maui.Controls.Maps.dll
Controls.Xaml -> /home/vsts/work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0/Microsoft.Maui.Controls.Xaml.dll
TestUtils -> /home/vsts/work/1/s/artifacts/bin/TestUtils/Debug/netstandard2.0/Microsoft.Maui.TestUtils.dll
Controls.Core.UnitTests -> /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.dll
Test run for /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.dll (.NETCoreApp,Version=v10.0)
VSTest version 18.0.1 (x64)
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.8.2+699d445a1a (64-bit .NET 10.0.0)
[xUnit.net 00:00:00.16] Discovering: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.37] Discovered: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.38] Starting: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.47] Finished: Microsoft.Maui.Controls.Core.UnitTests
Passed FalseBoxIsBoxedFalse [11 ms]
Passed BoxNullableNullReturnsNull [< 1 ms]
Passed BoxFalseReturnsFalseBox [< 1 ms]
Passed BoxReturnsSameReferenceOnRepeatedCalls [< 1 ms]
Passed BoxNullableFalseReturnsFalseBox [< 1 ms]
Passed BoxTrueReturnsTrueBox [< 1 ms]
Passed TrueBoxIsBoxedTrue [< 1 ms]
Passed BoxNullableTrueReturnsTrueBox [< 1 ms]
Passed TrueBoxAndFalseBoxAreDifferentInstances [< 1 ms]
Test Run Successful.
Total tests: 9
Passed: 9
Total time: 2.1658 Seconds
⚠️ Failure Details
- ❌ BooleanBoxesTests PASSED without fix (should fail) — tests don't catch the bug
📁 Fix files reverted (79 files)
eng/Versions.propseng/pipelines/ci-copilot.ymlsrc/Controls/src/Core/ActivityIndicator/ActivityIndicator.cssrc/Controls/src/Core/AppLinkEntry.cssrc/Controls/src/Core/Cells/Cell.cssrc/Controls/src/Core/Cells/SwitchCell.cssrc/Controls/src/Core/CheckBox/CheckBox.cssrc/Controls/src/Core/Compatibility/Handlers/ListView/Android/ListViewAdapter.cssrc/Controls/src/Core/Compatibility/Handlers/Shell/Android/ShellFlyoutRenderer.cssrc/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellPageRendererTracker.cssrc/Controls/src/Core/CompressedLayout.cssrc/Controls/src/Core/ContentPage/ContentPage.cssrc/Controls/src/Core/DragAndDrop/DragGestureRecognizer.cssrc/Controls/src/Core/DragAndDrop/DropGestureRecognizer.cssrc/Controls/src/Core/Entry/Entry.cssrc/Controls/src/Core/FlyoutPage/FlyoutPage.cssrc/Controls/src/Core/FontImageSource.cssrc/Controls/src/Core/Frame/Frame.cssrc/Controls/src/Core/Handlers/Items/Tizen/ItemTemplateAdaptor.cssrc/Controls/src/Core/Handlers/Shell/Tizen/ShellContentItemView.cssrc/Controls/src/Core/Handlers/Shell/Tizen/ShellFlyoutItemView.cssrc/Controls/src/Core/Handlers/Shell/Tizen/ShellSectionItemView.cssrc/Controls/src/Core/IndicatorView/IndicatorView.cssrc/Controls/src/Core/InputView/InputView.cssrc/Controls/src/Core/Interactivity/MultiCondition.cssrc/Controls/src/Core/Interactivity/PropertyCondition.cssrc/Controls/src/Core/Items/CarouselView.cssrc/Controls/src/Core/Items/GroupableItemsView.cssrc/Controls/src/Core/Items/ReorderableItemsView.cssrc/Controls/src/Core/Layout/Layout.cssrc/Controls/src/Core/ListView/ListView.cssrc/Controls/src/Core/Menu/MenuBar.cssrc/Controls/src/Core/Menu/MenuBarItem.cssrc/Controls/src/Core/Menu/MenuItem.cssrc/Controls/src/Core/NavigationPage/NavigationPage.cssrc/Controls/src/Core/Page/Page.cssrc/Controls/src/Core/PlatformConfiguration/AndroidSpecific/AppCompat/Application.cssrc/Controls/src/Core/PlatformConfiguration/AndroidSpecific/Button.cssrc/Controls/src/Core/PlatformConfiguration/AndroidSpecific/ImageButton.cssrc/Controls/src/Core/PlatformConfiguration/AndroidSpecific/ListView.cssrc/Controls/src/Core/PlatformConfiguration/AndroidSpecific/TabbedPage.cssrc/Controls/src/Core/PlatformConfiguration/AndroidSpecific/ViewCell.cssrc/Controls/src/Core/PlatformConfiguration/AndroidSpecific/VisualElement.cssrc/Controls/src/Core/PlatformConfiguration/AndroidSpecific/WebView.cssrc/Controls/src/Core/PlatformConfiguration/TizenSpecific/Application.cssrc/Controls/src/Core/PlatformConfiguration/TizenSpecific/NavigationPage.cssrc/Controls/src/Core/PlatformConfiguration/TizenSpecific/VisualElement.cssrc/Controls/src/Core/PlatformConfiguration/WindowsSpecific/InputView.cssrc/Controls/src/Core/PlatformConfiguration/WindowsSpecific/Label.cssrc/Controls/src/Core/PlatformConfiguration/WindowsSpecific/Page.cssrc/Controls/src/Core/PlatformConfiguration/WindowsSpecific/SearchBar.cssrc/Controls/src/Core/PlatformConfiguration/WindowsSpecific/TabbedPage.cssrc/Controls/src/Core/PlatformConfiguration/WindowsSpecific/VisualElement.cssrc/Controls/src/Core/PlatformConfiguration/WindowsSpecific/WebView.cssrc/Controls/src/Core/PlatformConfiguration/iOSSpecific/Application.cssrc/Controls/src/Core/PlatformConfiguration/iOSSpecific/Entry.cssrc/Controls/src/Core/PlatformConfiguration/iOSSpecific/FlyoutPage.cssrc/Controls/src/Core/PlatformConfiguration/iOSSpecific/ListView.cssrc/Controls/src/Core/PlatformConfiguration/iOSSpecific/NavigationPage.cssrc/Controls/src/Core/PlatformConfiguration/iOSSpecific/Page.cssrc/Controls/src/Core/PlatformConfiguration/iOSSpecific/ScrollView.cssrc/Controls/src/Core/PlatformConfiguration/iOSSpecific/Slider.cssrc/Controls/src/Core/PlatformConfiguration/iOSSpecific/VisualElement.cssrc/Controls/src/Core/RadioButton/RadioButton.cssrc/Controls/src/Core/RefreshView/RefreshView.cssrc/Controls/src/Core/Shapes/ArcSegment.cssrc/Controls/src/Core/Shapes/PathFigure.cssrc/Controls/src/Core/Shell/BackButtonBehavior.cssrc/Controls/src/Core/Shell/BaseShellItem.cssrc/Controls/src/Core/Shell/SearchHandler.cssrc/Controls/src/Core/Shell/Shell.cssrc/Controls/src/Core/StateTrigger.cssrc/Controls/src/Core/SwipeView/SwipeItem.cssrc/Controls/src/Core/Switch/Switch.cssrc/Controls/src/Core/TabbedPage/TabbedPage.Tizen.cssrc/Controls/src/Core/TableView/TableView.cssrc/Controls/src/Core/TemplatedItemsList.cssrc/Controls/src/Core/UriImageSource.cssrc/Controls/src/Core/VisualElement/VisualElement.cs
New files (not reverted):
src/Controls/src/Core/Internals/BooleanBoxes.cs
UI Tests — CheckBox,CollectionView,Entry,FlyoutPage,Frame,IndicatorView,Layout,ListView,Navigation,Page,RadioButton,RefreshView,Shape,Shell,SwipeView,Switch,TabbedPage,TableView
Detected UI test categories: CheckBox,CollectionView,Entry,FlyoutPage,Frame,IndicatorView,Layout,ListView,Navigation,Page,RadioButton,RefreshView,Shape,Shell,SwipeView,Switch,TabbedPage,TableView
✅ Deep UI tests — 23 passed, 0 failed across 1 category on platform-pool agent (replaces in-process counts above).
🧪 UI Test Execution Results (deep, platform pool)
| Category | Tests | Snapshot diffs |
|---|---|---|
CheckBox |
23/23 ✓ | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
Regression Cross-Reference
🔍 Regression Cross-Reference
🟡 Overlaps with prior bug-fix PRs — same files modified, but no exact line revert detected.
| File | Fix PR | Fixed issue(s) |
|---|---|---|
src/Controls/src/Core/FlyoutPage/FlyoutPage.cs |
#34485 | #33355 |
🧪 Regression Tests to Verify
These tests were added by the overlapping fix PRs. Running them to verify no side-effect regressions:
| Fix PR | Type | Test | Filter |
|---|---|---|---|
| #34485 | DeviceTest | MemoryTests | MemoryTests |
🧪 Regression Test Results
⏭️ SKIPPED — 0 passed, 0 failed, 1 skipped
| Fix PR | Test | Type | Result |
|---|---|---|---|
| #34485 | MemoryTests | DeviceTest | ⏭️ ERROR |
Pre-Flight — Context & Validation
Issue: #unknown - unavailable (GitHub CLI authentication unavailable)
PR: #35596 - unavailable from GitHub; local branch pr-review-35596
Platforms Affected: Android primary test target; Controls/Core behavior broadly; CI/workflow files also changed in PR diff
Files Changed: 82 implementation/infrastructure, 2 test/documentation-support (local diff against origin/main; exact PR metadata unavailable)
Key Findings
- GitHub CLI authentication is unavailable, so PR body, linked issue, comments, prior reviews, and required-check status could not be fetched.
- Local diff shows the PR primarily introduces
BooleanBoxesand rewrites many Controls bool bindable-property defaults/setters to use shared boxed bool instances. - Local diff also includes Copilot review pipeline/script changes unrelated to the Controls allocation fix.
- Independent code review found the Controls BooleanBoxes strategy behaviorally sound, but flagged
.github/workflows/review-trigger.ymlbroadeningpull-requestspermission fromreadtowriteas an unrelated least-privilege issue.
Code Review Summary
Verdict: NEEDS_CHANGES
Confidence: low
Errors: 1 | Warnings: 1 | Suggestions: 1
Key code review findings:
- ❌
.github/workflows/review-trigger.yml:154changespull-requests: readtopull-requests: write; job appears to only need PR read access and issue label write access. ⚠️ eng/pipelines/ci-copilot.yml:1782invokes token aggregation through a repo-relative path instead of the trusted-copy convention.- 💡
src/Controls/tests/Core.UnitTests/BooleanBoxesTests.cs:2has an unused import.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #35596 | Broad Controls call-site replacement: bool bindable-property defaults use BooleanBoxes.TrueBox/FalseBox, setters use BooleanBoxes.Box(value) |
⏳ PENDING (Gate failed before this task; not re-run) | Many Controls files + tests | Original PR fix; broad but directly avoids call-site boxing for changed setters. |
Code Review — Deep Analysis
Code Review — PR #35596
Independent Assessment
What this changes: Adds cached boxed Boolean values across Controls bindable properties/setters, plus Copilot review token-usage telemetry and review workflow/script updates.
Inferred motivation: Reduce bool boxing allocations and improve Copilot review observability/retry behavior.
Reconciliation with PR Narrative
Author claims: Unavailable — gh is unauthenticated.
Agreement/disagreement: PR narrative, linked issues, comments unavailable; reviewed local diff against origin/main.
Prior Review Reconciliation
Prior reviews unavailable: gh authentication is unavailable, so top-level reviews, inline comments, and issue comments could not be queried.
Blast Radius Assessment
- Runs for all instances: Yes — many shared Controls bindable properties and CI workflow paths.
- Startup impact: Low for Controls; CI workflow impact is infrastructure-wide.
- Static/shared state: Yes — immutable boxed bool singletons.
CI Status
- Required-check result: unavailable
- Classification: undetermined
- Action taken: gh pr checks --required unavailable due missing auth; confidence capped low.
Findings
❌ Error — Unnecessary PR write permission
.github/workflows/review-trigger.yml:154 changes pull-requests: read to pull-requests: write, but the job only reads PR metadata and manages labels through issues: write. This violates least-privilege guidance and grants unnecessary PR mutation capabilities. Revert to pull-requests: read.
⚠️ Warning — Trusted script convention deviation
eng/pipelines/ci-copilot.yml:1782 invokes .github/scripts/shared/Aggregate-CopilotTokenUsage.ps1 via repo-relative path instead of the trusted-copy convention. Risk appears low in this fresh checkout stage, but this should be documented or aligned with $TRUSTED conventions.
💡 Suggestion — Test cleanup
src/Controls/tests/Core.UnitTests/BooleanBoxesTests.cs:2 has an unused Microsoft.Maui.Controls.Shapes import and lacks a trailing newline.
Failure-Mode Probing
- Unused feature path: BooleanBoxes changes preserve bool values; cached boxes are immutable.
defaultValueCreator:Shell.NavBarHasShadowPropertystill boxes creator results, so optimization is incomplete but not behavioral.- Workflow compromise: broadened
pull-requests: writeincreases token blast radius unnecessarily.
Verdict: NEEDS_CHANGES
Confidence: low — CI/PR narrative/prior reviews unavailable.
Summary: Main Controls optimization looks sound, and PowerShell parsing plus aggregation smoke test passed. The workflow permission expansion should be fixed before merge.
Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Central bool canonicalization in BindableObject/BindableProperty plus internal bool SetValue overloads |
2 infra files + reverted broad call-site changes | Primary BooleanBoxes tests passed; Android MemoryTests hung in xharness instrumentation. Most allocation-aggressive central alternative. | |
| 2 | try-fix | Default-value-only canonicalization in BindableProperty |
❌ FAIL | 1 infra file + reverted broad call-site changes | Setter storage assertions failed; explicit bool sets still box before SetValue(object). |
| 3 | try-fix | Storage-boundary canonicalization in BindableObject.SetValueCore/BindableProperty, no bool overloads |
2 infra files + reverted broad call-site changes | Primary BooleanBoxes tests passed; Android MemoryTests timed out in xharness instrumentation. Cleanest non-invasive central alternative, but does not remove transient call-site boxing. | |
| PR | PR #35596 | Broad call-site replacement using BooleanBoxes defaults and setters |
❌ Gate failed (pre-existing gate result) | Many Controls files | Original PR; gate was already failed before this task and was not re-run per instruction. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| maui-expert-reviewer | 1 | Yes | Proposed central canonicalization, default-only canonicalization, internal typed helper/factory, and specialized bool storage. |
| maui-expert-reviewer | 2 | No | After candidate results, reviewer found no non-trivial remaining approaches: remaining options are trivial variants of candidates 1/3 or too invasive (BindableProperty<T>/unboxed storage rewrite). |
Exhausted: Yes
Selected Fix: None proven — candidates 1 and 3 passed primary tests but mandatory Android MemoryTests were blocked by xharness instrumentation timeout; candidate 2 failed primary tests. Candidate 3 is the best unproven alternative on simplicity/centralization, while candidate 1 is stronger for transient allocation reduction but adds overload complexity. Neither can be declared passing without Android MemoryTests completion.
Per-Candidate Details
- try-fix-1: see
../try-fix-1/content.md,fix.diff,test-output.log,regression-output.log, andanalysis.md. - try-fix-2: see
../try-fix-2/content.md,fix.diff,test-output.log, andanalysis.md. - try-fix-3: see
../try-fix-3/content.md,fix.diff,test-output.log,regression-output.log, andanalysis.md.
Report — Final Recommendation
Comparative Report — PR #35596
Candidates compared
| Rank | Candidate | Regression status | Assessment |
|---|---|---|---|
| 1 | pr-plus-reviewer |
Gate failed / Android regression undetermined | Best available candidate. It preserves the PR's direct call-site boxing reduction and applies the expert reviewer's required security fix by restoring pull-requests: read in review-trigger.yml. |
| 2 | pr |
Gate failed / Android regression undetermined | The BooleanBoxes fix is sound, but the raw PR keeps the over-scoped pull-requests: write workflow permission, so it ranks below the reviewer-applied PR candidate. |
| 3 | try-fix-1 |
Primary PASS; Android MemoryTests BLOCKED | Central canonicalization plus bool SetValue overloads is allocation-strong and broad, but the mandatory Android regression did not complete. It also adds API-surface/internal-overload complexity. |
| 4 | try-fix-3 |
Primary PASS; Android MemoryTests BLOCKED | Clean central storage-boundary canonicalization with a small diff, but it does not remove transient call-site boxing and the Android regression did not complete. |
| 5 | try-fix-2 |
Primary FAIL; regression skipped | Fails the BooleanBoxes setter-storage assertions because default-only canonicalization does not fix explicit setter boxing. Per ranking rules, this failed candidate must remain below non-failing/unblocked candidates. |
Analysis
No candidate has a fully passing Android regression result in the recorded evidence. try-fix-2 is disqualified by primary test failure. try-fix-1 and try-fix-3 both passed the targeted BooleanBoxes tests, but their mandatory Android MemoryTests were blocked by xharness instrumentation timeout, leaving them unproven. Between those alternatives, try-fix-1 is stronger for transient allocation reduction while try-fix-3 is simpler, but neither can outrank the PR path without successful regression evidence.
The raw PR fix directly addresses the observed allocation pattern by updating boolean bindable-property defaults and setters to use shared boxes. The expert reviewer found the BooleanBoxes implementation sound and identified one required non-functional correction: revert .github/workflows/review-trigger.yml from pull-requests: write to pull-requests: read. Applying that correction yields pr-plus-reviewer, which is strictly better than pr and avoids choosing an unproven central rewrite.
Winner
pr-plus-reviewer is the winning candidate. It keeps the submitted PR fix, applies the expert reviewer's actionable security feedback, and avoids adopting try-fix candidates that either failed primary tests or were blocked before completing Android regression validation.
Future Action — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
MauiBot
removed
the
s/agent-review-in-progress
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
LGTM! Could you please resolve conflicts, so that I can merge?
Replace raw true/false/default(bool) literals with BooleanBoxes.TrueBox and BooleanBoxes.FalseBox in BindableProperty.Create defaultValue arguments across all bool-typed BindableProperties in Controls.Core. This avoids repeated boxing allocations on every property default-value access. Added using Microsoft.Maui.Controls.Internals; to 26 files that did not already have it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> # Conflicts: # src/Controls/src/Core/SwipeView/SwipeItem.cs
…ols.Core Replace raw true/false literals with BooleanBoxes.TrueBox/FalseBox in SetValue and SetValueFromRenderer calls for bool-typed BindableProperties. Added using Microsoft.Maui.Controls.Internals to ShellPageRendererTracker.cs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ties in Controls.Core Replace raw true/false/default(bool) literals with BooleanBoxes.TrueBox/FalseBox in BindableProperty.CreateAttached and CreateAttachedReadOnly calls for bool-typed properties. Added using Microsoft.Maui.Controls.Internals to 6 files that did not already have it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… Controls.Core Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…cator The global using Microsoft.Maui.Controls.Internals caused CS0104 ambiguous reference errors for PreserveAttribute in iOS files that also import Foundation. Remove the global using and add an explicit using to the one file that was missing it (ActivityIndicator.cs). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…boxing All public bool property setters that previously called SetValue(Prop, value) now call SetValue(Prop, BooleanBoxes.Box(value)) to eliminate per-assignment heap allocations. This completes the boxing optimization across: - VisualElement (IsVisible, IsEnabled, InputTransparent, IsFocused) - CheckBox, Switch, RadioButton (IsChecked, IsToggled) - ActivityIndicator, RefreshView, ListView, CarouselView, and others - SetterSpecificity overloads (FromHandler) also updated Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add BooleanBoxesProductionTests class with Assert.Same checks on: * DefaultValue for representative BindableProperties * GetValue() after setter calls (verifies cached box is stored) - Fix CoerceIsEnabledProperty and CoerceInputTransparentProperty in VisualElement to return BooleanBoxes.Box(...) / BooleanBoxes.FalseBox instead of raw bool literals (which would allocate a new box) - Fix CoerceIsEnabledProperty in MenuItem similarly Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
pictos
force-pushed
the
pj/avoid-bool-boxing
branch
from
63cd5ab to
f0dbf63
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
3 participants
Description of Change
This is a follow up from #35303, that implements the
BooleanBoxesclass and use inside theControlprojectUse the same solution that the WPF team uses, cache the boxed
trueandfalsevalues, and use it instead of boxing a new boolean on every operation.Issues Fixed
Relates with #35302