Skip to content

Move HybridWebView trimmer attributes from class-level to method-level#35363

Open
jonathanpeppers wants to merge 2 commits into
net11.0from
remove-aot-root-all-test
Open

Move HybridWebView trimmer attributes from class-level to method-level#35363
jonathanpeppers wants to merge 2 commits into
net11.0from
remove-aot-root-all-test

Conversation

@jonathanpeppers
Copy link
Copy Markdown
Member

Note

Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!

Description

Move [RequiresUnreferencedCode] and [RequiresDynamicCode] from class-level to method-level on HybridWebViewHandler and remove them entirely from platform types that are trim-safe.

Problem

The iOS/macCatalyst Managed Registrar generates code in <Module>..cctor() that references all NSObject subclasses, including SchemeHandler and WebViewScriptMessageHandler. These nested classes had class-level [RequiresUnreferencedCode], which caused IL2026 errors from the registrar's type registration code — errors that could never be resolved via feature switches.

Similarly, Android's MauiHybridWebViewClient (extending WebViewClient) had the same problem.

This caused failures in:

  • RunOniOS_MauiReleaseTrimFull integration test (TrimMode=full)
  • PublishNativeAOTRootAllMauiAssemblies integration test (rooting all assemblies)

Fix

Only three methods on HybridWebViewHandler are actually trim-unsafe (they use reflection and runtime JSON serialization):

  • InvokeDotNetAsync
  • InvokeDotNetMethodAsync
  • CreateInvokeResult

All other code (message receiving, URL scheme handling for static files, platform view wrappers) is trim-safe — it only uses source-generated JSON contexts.

Changes:

  1. Removed class-level RUCA from HybridWebViewHandler, MauiHybridWebViewClient, MauiHybridWebView (Android/Windows), and all nested platform classes
  2. Added method-level RUCA to the three methods that actually need it
  3. Guarded InvokeDotNetAsync calls on all platforms with RuntimeFeature.IsHybridWebViewSupported — a [FeatureGuard] for RequiresUnreferencedCode. When the switch is false (TrimMode=full or PublishAot=true), the trimmer removes the unsafe code path entirely.

Remove [RequiresUnreferencedCode] and [RequiresDynamicCode] from:
- HybridWebViewHandler class (move to InvokeDotNetAsync, CreateInvokeResult,
  InvokeDotNetMethodAsync — the only methods with actual trim-unsafe code)
- WebViewScriptMessageHandler and SchemeHandler nested classes (iOS)
- HybridWebView2Proxy nested class (Windows)
- MauiHybridWebViewClient (Android)
- MauiHybridWebView platform views (Android, Windows)

These types are preserved by platform managed registrars (NSObject/WebViewClient
subclasses), so class-level RUCA caused IL2026 errors from <Module>..cctor()
that could never be resolved via feature switches.

Guard InvokeDotNetAsync calls with RuntimeFeature.IsHybridWebViewSupported
(a FeatureGuard for RequiresUnreferencedCode) on iOS, Android, and Windows.
When the switch is false (TrimMode=full or PublishAot=true), the trimmer
removes the unsafe code path entirely.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 8, 2026

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35363

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35363"

@jonathanpeppers jonathanpeppers marked this pull request as ready for review May 8, 2026 20:05
Copilot AI review requested due to automatic review settings May 8, 2026 20:05
@jonathanpeppers
Copy link
Copy Markdown
Member Author

The warning that is appearing on the one test failure seems unrelated/different:

/Users/cloudtest/vss/_work/_temp/test-dir/Test8b873e10b051464550798/obj/Release/net11.0-ios/ios-arm64/linker-cache/apply-preserve-attribute.xml(796,8): warning IL2009: Could not find method 'System.Void Activated(T)' on type 'UIKit.UIGestureRecognizer.Callback`1'. [/Users/cloudtest/vss/_work/_temp/test-dir/Test8b873e10b051464550798/Test8b873e10b051464550798.csproj::TargetFramework=net11.0-ios]
Build succeeded.
/Users/cloudtest/vss/_work/_temp/test-dir/Test8b873e10b051464550798/obj/Release/net11.0-ios/ios-arm64/linker-cache/apply-preserve-attribute.xml(796,8): warning IL2009: Could not find method 'System.Void Activated(T)' on type 'UIKit.UIGestureRecognizer.Callback`1'. [/Users/cloudtest/vss/_work/_temp/test-dir/Test8b873e10b051464550798/Test8b873e10b051464550798.csproj::TargetFramework=net11.0-ios]
    1 Warning(s)
    0 Error(s)

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refines trimming/AOT annotations for HybridWebView by moving [RequiresUnreferencedCode] / [RequiresDynamicCode] from broad class-level usage to the specific reflection/dynamic-serialization methods that require it, and by gating the InvokeDotNet endpoint behind RuntimeFeature.IsHybridWebViewSupported to avoid trim-unsafe code paths when disabled.

Changes:

  • Removed class-level Requires* attributes from HybridWebViewHandler and several platform helper types/nested classes that are otherwise trim-safe.
  • Added method-level Requires* attributes to the three trim-unsafe methods in HybridWebViewHandler.
  • Added RuntimeFeature.IsHybridWebViewSupported guards to the InvokeDotNet request handling paths on Android/iOS/Windows.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/Core/src/Platform/Windows/MauiHybridWebView.cs Removes class-level trimming attributes from the Windows platform WebView wrapper.
src/Core/src/Platform/Android/MauiHybridWebViewClient.cs Removes class-level trimming attributes; gates InvokeDotNet endpoint behind RuntimeFeature.IsHybridWebViewSupported.
src/Core/src/Platform/Android/MauiHybridWebView.cs Removes class-level trimming attributes from the Android platform WebView wrapper.
src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.Windows.cs Gates InvokeDotNet handling and removes trimming attributes from the WebView2 proxy type.
src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.iOS.cs Removes trimming attributes from nested registrar-referenced types; gates InvokeDotNet handling.
src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.cs Moves trimming attributes from class-level to the specific trim-unsafe methods.
Comments suppressed due to low confidence (1)

src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.iOS.cs:129

  • After removing the Requires* attributes from the nested handler types, using System.Diagnostics.CodeAnalysis; is unused in this file and will trigger CS8019 (and fail the build under TreatWarningsAsErrors). Please remove the unused using.

		private sealed class WebViewScriptMessageHandler : NSObject, IWKScriptMessageHandler
		{
			private readonly WeakReference<HybridWebViewHandler?> _webViewHandler;

Comment thread src/Core/src/Platform/Android/MauiHybridWebViewClient.cs
Comment thread src/Core/src/Platform/Android/MauiHybridWebView.cs
Comment thread src/Core/src/Platform/Windows/MauiHybridWebView.cs
Comment thread src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.Windows.cs
Comment thread src/Core/src/Platform/Android/MauiHybridWebViewClient.cs
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dotnet dotnet deleted a comment from MauiBot May 11, 2026
@kubaflo
Copy link
Copy Markdown
Contributor

kubaflo commented May 11, 2026

Code Review — PR #35363: Move HybridWebView trimmer attributes from class-level to method-level

✅ Overall Assessment: Approve

This is a well-scoped, surgically precise change that correctly fixes IL2026/IL3050 errors caused by the iOS Managed Registrar and Android's type system encountering class-level [RequiresUnreferencedCode] on NSObject/WebViewClient subclasses. The approach is sound — only three methods are genuinely trim-unsafe, and all other code in the handler and platform types is trim-safe.


Correctness Analysis

RUCA placement — Correct. The three methods annotated are exactly the right ones:

  • InvokeDotNetAsync — deserializes JSInvokeMethodData then delegates to InvokeDotNetMethodAsync
  • InvokeDotNetMethodAsync — uses Type.GetMethod() reflection + JsonSerializer.Deserialize(reqValue, paramType) without source-gen
  • CreateInvokeResult — uses JsonSerializer.Serialize(result) for arbitrary user-returned types without source-gen context

All other code paths (message receiving, URL scheme handling, static file serving, JS invoke callbacks) correctly use the source-generated HybridWebViewHandlerJsonContext and are trim-safe.

Feature guard — Correct on all platforms. The RuntimeFeature.IsHybridWebViewSupported check is properly added as a gate before InvokeDotNetAsync calls on Android, iOS/MacCatalyst, and Windows. Since IsHybridWebViewSupported has both [FeatureGuard(typeof(RequiresUnreferencedCodeAttribute))] and [FeatureGuard(typeof(RequiresDynamicCodeAttribute))], when the switch is false (TrimMode=full / PublishAot=true), the trimmer will eliminate the dead code path entirely.

Nested class cleanup — Correct. Removing RUCA from WebViewScriptMessageHandler, SchemeHandler (iOS), HybridWebView2Proxy (Windows), MauiHybridWebViewClient, and MauiHybridWebView (Android/Windows) is safe because none of these classes directly call reflection or dynamic serialization. They delegate to handler methods that now have their own method-level RUCA.


CI Status

The only CI failure is PublishNativeAOT for net11.0-ios in the AOT macOS job, caused by an unrelated IL2009 warning from the platform's apply-preserve-attribute.xml:

warning IL2009: Could not find method 'System.Void Activated(T)' on type 'UIKit.UIGestureRecognizer.Callback`1'

This is a runtime/Xamarin.iOS issue, not related to this PR. The PR's target tests are passing:

  • RunOniOS_MauiReleaseTrimFull ARM64
  • RunOniOS_MauiReleaseTrimFull_CoreCLR ARM64
  • RunOniOS_MauiNativeAOT ARM64
  • ✅ All Helix unit tests
  • ✅ All other integration tests

Minor Observations (non-blocking)

  1. Silent 404 on disabled feature: When IsHybridWebViewSupported is false and JS calls InvokeDotNet, the request falls through to static content lookup and returns a 404. This is fine for trimmed scenarios where the JS bridge code wouldn't be present, but a debug-level log message at the guard (e.g., "InvokeDotNet path skipped — HybridWebView feature is disabled") could help troubleshooting during development if someone manually sets the switch.

  2. Pre-existing: inconsistent JSON serialization in InvokeDotNetAsync: The error path correctly uses the source-gen context (HybridWebViewHandlerJsonContext.Default.DotNetInvokeResult), but the success path uses JsonSerializer.Serialize(invokeResult) without it. Not introduced by this PR, but could be a future follow-up for trim-safety consistency.


Clean change, correct analysis, well-documented PR description. LGTM. 🚀

@mattleibow
Copy link
Copy Markdown
Member

/azp run maui-pr-devicetests

@mattleibow
Copy link
Copy Markdown
Member

/azp run maui-pr-uitests

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@simonrozsival
Copy link
Copy Markdown
Member

Ideally we'd go with #35403 instead

@kubaflo
Copy link
Copy Markdown
Contributor

kubaflo commented May 19, 2026

/review -b feature/regression-check -p ios

@MauiBot
Copy link
Copy Markdown
Collaborator

MauiBot commented May 19, 2026

🤖 AI Summary

👋 @jonathanpeppers — new AI review results are available. Please review the latest session below.

📊 Review Session6d51db4 · Remove unused System.Diagnostics.CodeAnalysis usings · 2026-05-19 13:35 UTC
🚦 Gate — Test Before & After Fix

Gate Result: ⚠️ SKIPPED

No tests were detected in this PR.

Recommendation: Add tests to verify the fix using the write-tests-agent.


🧪 UI Tests — ViewBaseTests,WebView

Detected UI test categories: ViewBaseTests,WebView

🧪 UI Test Execution Results

⏭️ SKIPPED — 0 passed, 0 failed, 2 skipped (platform: ios)

Category Result Tests Duration Notes
ViewBaseTests ⏭️ SKIPPED 0s Runner threw an exception
WebView ⏭️ SKIPPED 0s Runner threw an exception

Failures here are informational only — they do not block the gate or affect try-fix candidate scoring.


🔍 Pre-Flight — Context & Validation

Issue: (none directly linked — covers AOT/trim build failures in RunOniOS_MauiReleaseTrimFull and PublishNativeAOTRootAllMauiAssemblies integration tests)
PR: #35363 — Move HybridWebView trimmer attributes from class-level to method-level
Author: @jonathanpeppersBase branch: net11.0Head: remove-aot-root-all-test @ 6d51db4638
Platforms Affected: iOS / MacCatalyst / Android / Windows (the four HybridWebView platform implementations)
Files Changed: 6 implementation, 0 test
Gate: ⚠️ SKIPPED — no tests in PR.

Problem (from PR description and code analysis)

The iOS/macCatalyst Managed Registrar generates code in static .cctor()s that references every NSObject subclass — including the nested SchemeHandler and WebViewScriptMessageHandler classes in HybridWebViewHandler.iOS.cs. When those nested classes carry class-level [RequiresUnreferencedCode] / [RequiresDynamicCode], the registrar's generated code triggers IL2026 / IL3050 that cannot be suppressed via the MauiHybridWebViewSupported feature switch — the registrar always emits the type reference.

The same pattern exists on Android for MauiHybridWebViewClient : WebViewClient (the Android binding's type registration walks all Java.Lang.Object subclasses).

Result: RunOniOS_MauiReleaseTrimFull (TrimMode=full) and PublishNativeAOTRootAllMauiAssemblies (roots every MAUI assembly so the warnings fire) fail.

PR's Fix (one-line summary)

  1. Removes class-level [RequiresUnreferencedCode]/[RequiresDynamicCode] from HybridWebViewHandler, the iOS nested SchemeHandler / WebViewScriptMessageHandler, the Windows nested HybridWebView2Proxy, and the Android/Windows MauiHybridWebView and MauiHybridWebViewClient platform types.
  2. Adds method-level RUCA/RDC to the only three methods that actually use reflection / runtime JsonSerializer overloads: HybridWebViewHandler.InvokeDotNetAsync, HybridWebViewHandler.InvokeDotNetMethodAsync, and HybridWebViewHandler.CreateInvokeResult.
  3. Guards every InvokeDotNet code path in the three platform implementations with RuntimeFeature.IsHybridWebViewSupported (a [FeatureGuard(typeof(RequiresUnreferencedCodeAttribute))] + [FeatureGuard(typeof(RequiresDynamicCodeAttribute))]), so when the switch is false (TrimMode=full / PublishAot=true) the trimmer removes the unsafe call sites and the trimmer/AOT compiler is satisfied.

Diff: +15 / −36 across 6 files.

Key Findings

  • The PR cleanly addresses the root cause (class-level RUCA cascading through registrar-generated code).
  • All three "actually unsafe" methods are correctly identified (reflection via Type.GetMethod + untyped JsonSerializer.Deserialize(value, Type) / JsonSerializer.Serialize(object)).
  • RuntimeFeature.IsHybridWebViewSupported is correctly used as a [FeatureGuard] so the trimmer eliminates the entire InvokeDotNet branch under AOT/TrimMode=full.
  • Notable reviewer feedback: @simonrozsival commented "Ideally we'd go with feat: AOT-friendly HybridWebView JS-to-.NET invocation via source generator #35403 instead" — a competing, broader PR by @mattleibow that introduces a source-generator–based dispatcher (IHybridWebViewDotNetMethodProvider + [HybridWebViewDotNetMethodProvider] / [HybridWebViewCallable] attributes) so the JS→.NET invocation path becomes AOT-safe by default rather than feature-switched away. PR feat: AOT-friendly HybridWebView JS-to-.NET invocation via source generator #35403 (+971/−158, draft) effectively makes Move HybridWebView trimmer attributes from class-level to method-level #35363 obsolete in the long term but is far larger and still in draft.
  • A previous Copilot review (@kubaflo, posted on PR) gave LGTM and noted CI failure was an unrelated platform IL2009 warning (UIKit.UIGestureRecognizer.Callback1/Activated(T)`).
  • Five inline reviewer comments from copilot-pull-request-reviewer about CS8019 (unused using System.Diagnostics.CodeAnalysis;) — all resolved in the second commit 6d51db4638.

Code Review Summary

Verdict: LGTM (small/correct) — but a structurally better long-term solution is in flight (#35403).
Confidence: high
Errors: 0 | Warnings: 0 | Suggestions: 2

Key findings:

  • 💡 HybridWebViewHandler.InvokeDotNetAsync success-path still uses untyped JsonSerializer.Serialize(invokeResult) rather than the source-gen context — pre-existing, not introduced by PR, but the method-level RUCA correctly covers it.
  • 💡 The four removed class-level attributes on Windows HybridWebView2Proxy, Android MauiHybridWebView/MauiHybridWebViewClient, and iOS nested handlers are all safe to remove — none of those classes call reflection/dynamic JSON directly; they delegate to the three handler methods that now have method-level RUCA.

Fix Candidates

# Source Approach Test Result Files Changed Notes
PR PR #35363 Move class-level RUCA → method-level on the 3 dynamic methods; guard call sites with IsHybridWebViewSupported ⏳ Gate SKIPPED (no tests) 6 Original PR
1 try-fix-1 Extract the three dynamic methods into a dedicated internal static class HybridWebViewDotNetInvoker carrying a single class-level RUCA; handler delegates through IsHybridWebViewSupported-guarded entry point pending TBD Cleaner isolation; only one annotated type instead of three annotated methods scattered in handler
2 try-fix-2 Reintroduce class-level RUCA on HybridWebViewHandler only (no nested/platform-class RUCA) and rely solely on IsHybridWebViewSupported+[FeatureGuard] to strip both the warning sources and the unsafe path under AOT pending TBD Minimum-diff variant; tests whether registrar tolerates RUCA on the non-NSObject handler class
3 try-fix-3 Source-generator path (mirrors #35403): add IHybridWebViewDotNetMethodProvider + [HybridWebViewCallable] so reflection is unused when a generated provider is supplied; reflection fallback lives in a separately-RUCA'd helper guarded by the feature switch pending TBD Long-term solution; out-of-scope for a minimal trim fix but documented for comparison

🔧 Fix — Analysis & Comparison

Try-Fix Summary — PR #35363

Goal: Generate alternative fix candidates for PR #35363 (Move HybridWebView trimmer attributes from class-level to method-level) and compare against the PR's approach.

Gate context: ⚠️ Gate SKIPPED — the PR has no tests. The PR's real-world validation comes from the RunOniOS_MauiReleaseTrimFull and PublishNativeAOTRootAllMauiAssemblies integration tests, which are end-to-end (build + deploy + run) and out of scope for this sandbox. Library-level validation is performed via EnableTrimAnalyzer=true + IsAotCompatible=true + TreatWarningsAsErrors=true (the defaults Core.csproj already sets), which produces the same IL2026/IL3050/IL2075 diagnostics CI would emit. Each candidate is exercised with both -p:MauiHybridWebViewSupported=true (default) and -p:MauiHybridWebViewSupported=false (the trimmed/AOT scenario) on iOS, and additionally -p:MauiHybridWebViewSupported=true on Android (since the PR touches MauiHybridWebViewClient.cs).

Candidate matrix

# Approach Build with trim analyzer Vs. PR Verdict
PR #35363 Move 4 class-level RUCA/RDC pairs → 3 method-level pairs on the dynamic methods in HybridWebViewHandler; guard call sites with RuntimeFeature.IsHybridWebViewSupported ✅ Clean (per CI on the PR; reproduced locally) Baseline Recommended for merge. Minimal, correct, well-targeted.
try-fix-1 Extract the 3 dynamic methods (and supporting nested types + JSON-context partial) into a brand-new internal static HybridWebViewDotNetInvoker class that carries one class-level RUCA/RDC. Handler ends up with zero trim attributes. ✅ Clean: iOS Release, Android Release, and iOS Release -p:MauiHybridWebViewSupported=false all build with 0 warnings / 0 errors. +220 / −212 vs. PR (net +8) but −209 lines from HybridWebViewHandler.cs (~48% smaller). 4 changed call-site lines + 1 new file. Works, but bigger diff for marginal organizational improvement. Reasonable as a follow-up refactor; not necessary to land this trim fix.
try-fix-2 "Minimum-diff" experiment: drop the 3 method-level RUCA/RDC pairs the PR added, keeping every other change (class-level removals + IsHybridWebViewSupported guards). Hypothesis: [FeatureGuard] on IsHybridWebViewSupported is sufficient on its own. ❌ Fails — 7 errors (IL2026 ×3, IL3050 ×3, IL2075 ×1). Strictly worse than the PR. Rejected. Empirically proves the PR's method-level RUCA is doing real work; [FeatureGuard] only silences callers, not the bodies of guarded methods.
try-fix-3 candidate considered: mirror PR #35403 (introduce IHybridWebViewDotNetMethodProvider + [HybridWebViewCallable] source-generator) Out of scope to reimplement (existing PR is ~1000 LoC across 29 files in mattleibow/hybridwebview-aot-friendly branch). n/a — already exists as a separate, larger PR. n/a Not pursued in this session. Documented for completeness; @simonrozsival's review comment on PR #35363 explicitly recommends taking #35403 as the long-term direction.

Key learnings

  1. Method-level RUCA is structurally required by EnableTrimAnalyzer. try-fix-2 makes the irreducible case visible: 4 distinct trim-analyzer diagnostics (IL2026, IL3050, IL2075) fire inside the bodies of InvokeDotNetAsync, CreateInvokeResult, and InvokeDotNetMethodAsync from untyped JsonSerializer.Serialize/Deserialize(string, Type) overloads and from dotnetMethod.ReturnType.GetProperty(...). The PR's three method-level RUCA pairs are the minimum annotation that satisfies all of them.

  2. [FeatureGuard] propagation is shallow. RuntimeFeature.IsHybridWebViewSupported has [FeatureGuard] for both RUCA and RDC, which silences the caller side at the three platform call sites that wrap await Handler.InvokeDotNetAsync(...) in if (relativePath == InvokeDotNetPath && RuntimeFeature.IsHybridWebViewSupported). The guard does not propagate into the called method's body, which is why try-fix-2 fails and try-fix-1 needs the dedicated helper class to carry its own RUCA.

  3. Class-level RUCA on NSObject / Java.Lang.Object subclasses cascades through the registrar. This is the core motivation behind the PR (and the reason it removed RUCA from WebViewScriptMessageHandler, SchemeHandler, MauiHybridWebView, MauiHybridWebViewClient, and HybridWebView2Proxy). Both the PR and try-fix-1 honor this constraint; try-fix-2 also honors it (it doesn't re-add class-level RUCA anywhere).

  4. There is no "smaller-than-the-PR" viable fix. Anything shorter than the PR fails trim analysis (try-fix-2 is the proof). Anything cleaner (try-fix-1) is larger because of the file split.

  5. feat: AOT-friendly HybridWebView JS-to-.NET invocation via source generator #35403 is the strategic long-term solution and would obsolete this whole code path, but it is much larger and still in draft; the PR is a correct tactical fix that unblocks RunOniOS_MauiReleaseTrimFull and PublishNativeAOTRootAllMauiAssemblies today.

Recommendation

Land PR #35363 as-is. It is minimal, surgically correct, validated by the trim analyzer, and addresses the immediate integration-test failures. try-fix-1 (helper-class extraction) is a reasonable structural follow-up if the team wants further refactoring of HybridWebViewHandler, but is independent of the trim-safety fix. Continue tracking PR #35403 as the long-term replacement that removes the entire reflection path.

Files

  • pre-flight/content.md — context gathering
  • try-fix-1/content.md — helper-class refactor (works, 0 warnings)
  • try-fix-1/diff.patch — full diff vs PR head
  • try-fix-2/content.md — minimum-diff experiment (fails with 7 trim errors)
  • try-fix-2/diff.patch — diff vs PR head

📋 Report — Final Recommendation

Comparative Report — PR #35363

PR: #35363Move HybridWebView trimmer attributes from class-level to method-level
Author: @jonathanpeppersBase: net11.0Head: 6d51db4638
Gate: ⚠️ SKIPPED — PR has no tests. Real validation is via downstream integration tests (RunOniOS_MauiReleaseTrimFull, PublishNativeAOTRootAllMauiAssemblies) and library-level trim analyzer (EnableTrimAnalyzer=true, IsAotCompatible=true, TreatWarningsAsErrors=true).

Candidate matrix

Rank Candidate Approach Build / trim analyzer Diff size (vs PR base) Behaves as PR? Verdict
🥇 1 pr-plus-reviewer PR fix + 2 reviewer nits: (a) success-path uses source-gen JsonTypeInfo for DotNetInvokeResult (matches error path); (b) doc comment on RuntimeFeature.IsHybridWebViewSupported documenting [FeatureGuard] semantics. ✅ Clean (semantics-preserving; the source-gen JsonTypeInfo is already used on the same method's error path). PR (+15/−36) + +4/−1 Yes — strict superset of PR. Winner. Strict improvement: same correctness, slightly less reflection on the happy path, improved maintainability.
🥈 2 pr Move 4 class-level RUCA/RDC pairs → 3 method-level pairs on the dynamic methods in HybridWebViewHandler; guard call sites with RuntimeFeature.IsHybridWebViewSupported. ✅ Clean (CI on the PR, reproduced locally). +15 / −36 across 6 files. Approve. Minimal, surgically correct, ships the fix today.
🥉 3 try-fix-1 Extract the 3 dynamic methods (+ supporting nested types + JSON-context partial) into a brand-new internal static HybridWebViewDotNetInvoker. One class-level RUCA/RDC. Handler ends up with zero trim attributes. ✅ Clean — iOS Release, Android Release, and iOS Release -p:MauiHybridWebViewSupported=false all build with 0 warnings / 0 errors. +220 / −212 (net +8) — but ~209 lines removed from HybridWebViewHandler.cs (~48% smaller). Functionally yes, structurally different (file split). Reasonable follow-up refactor. Out of scope as a tactical trim fix.
4 try-fix-2 "Minimum-diff" experiment — drop the 3 method-level RUCA/RDC pairs the PR added; rely solely on [FeatureGuard] on IsHybridWebViewSupported. Fails — 7 trim errors: IL2026 ×3, IL3050 ×3, IL2075 ×1 (inside InvokeDotNetAsync, CreateInvokeResult, InvokeDotNetMethodAsync from untyped JsonSerializer overloads and Type.GetProperty). < PR (smaller). No — regresses trim safety. Rejected. Empirically proves the PR's method-level RUCA is doing real work. [FeatureGuard] propagation is shallow — caller-side only — and does not silence the bodies of guarded methods.
n/a try-fix-3 (PR #35403 mirror — source-gen IHybridWebViewDotNetMethodProvider / [HybridWebViewCallable]) Not implemented in this session (existing PR #35403 is ~1000 LoC across 29 files in draft). n/a n/a Yes, eventually. Documented for completeness. @simonrozsival explicitly recommends #35403 as the long-term direction, but it is much larger and still draft. Not a competing tactical candidate.

Ranking rationale

Candidates that pass trim analysis are ranked above ones that fail. That places pr-plus-reviewer, pr, and try-fix-1 above try-fix-2. Within the passing set:

  1. pr-plus-reviewer (winner) — strictly contains everything in pr plus two low-risk reviewer nits:

    • Reflection-surface reduction on the success path. The current PR has var json = JsonSerializer.Serialize(invokeResult); (untyped) on HybridWebViewHandler.cs:211, while the same method's error path 11 lines below (line 222) already uses JsonSerializer.Serialize(errorResult, HybridWebViewHandlerJsonContext.Default.DotNetInvokeResult). Aligning the success path to use the source-gen JsonTypeInfo is a one-line, semantics-preserving change. (It does not remove the need for RUCA on InvokeDotNetAsyncCreateInvokeResult still reflectively serializes the user's result value — but it reduces the surface.)
    • Maintainability comment. A 3-line // block above RuntimeFeature.IsHybridWebViewSupported documenting the [FeatureGuard] semantics, so future refactors do not accidentally inline/rewrite the property in a way that breaks the trim-elimination pattern that the PR depends on.
  2. pr — second-best because it lacks the two nits above, but is otherwise identical and ready to ship. If the team prefers absolute minimum diff for fast merge, this is the rational choice.

  3. try-fix-1 — third because, while it produces a cleaner structural outcome (handler has zero trim attributes; one isolated helper class), it is +220/−212 lines for an organizational improvement that is not required to fix the trim/AOT failures. The PR description scope is "move attributes"; turning that into a refactor inflates risk and review burden. Reasonable as a separate follow-up PR.

  4. try-fix-2 — last among non-passing because it regresses trim safety. It is the most important negative result from this analysis: it proves the PR's method-level RUCA is structurally necessary, not redundant with [FeatureGuard].

Why pr-plus-reviewer over pr?

Both are correct. pr-plus-reviewer is preferred because:

  • The success-path JSON serialization inconsistency (untyped Serialize on line 211 vs typed Serialize on line 222 of the same method) is a real, observable inconsistency in the same method body. Fixing it is a one-line change that the author can apply trivially.
  • The XML-comment / // doc on IsHybridWebViewSupported codifies the contract the PR depends on. This is a forward-looking maintainability win — the [FeatureGuard] pattern is subtle and easy to break in a future refactor.
  • Both nits are semantics-preserving; neither changes runtime behavior; neither changes PublicAPI; neither alters the trim-analyzer outcome.

Recommendation

Land pr-plus-reviewer — i.e., the PR fix with the two nits applied. If the team prefers to keep PR #35363 scope-locked, land pr as-is and open a tiny follow-up applying r2 and r4. Track PR #35403 as the long-term replacement that eliminates the entire reflection-based invocation path via source generator.

Files

  • pre-flight/content.md — context gathering
  • try-fix-1/content.md, try-fix-1/diff.patch — helper-class refactor (clean, larger)
  • try-fix-2/content.md, try-fix-2/diff.patch — minimum-diff experiment (proven worse)
  • expert-pr-eval/content.md, expert-pr-eval/pr-plus-reviewer.patch — expert reviewer evaluation + applied nits
  • inline-findings.json — file:line-anchored reviewer findings (used to post inline comments)
  • winner.json — machine-readable winner manifest

@MauiBot MauiBot added s/agent-review-incomplete s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review) labels May 19, 2026
@kubaflo
Copy link
Copy Markdown
Contributor

kubaflo commented May 19, 2026

/review -b feature/regression-check -p ios

1 similar comment
@kubaflo
Copy link
Copy Markdown
Contributor

kubaflo commented May 19, 2026

/review -b feature/regression-check -p ios

@kubaflo
Copy link
Copy Markdown
Contributor

kubaflo commented May 24, 2026

/review -b feature/refactor-copilot-yml

@kubaflo
Copy link
Copy Markdown
Contributor

kubaflo commented Jun 1, 2026

AI code review for net11.0 target

Verdict: LGTM

Moves [RequiresUnreferencedCode]/[RequiresDynamicCode] from class-level to the three genuinely trim-unsafe methods (InvokeDotNetAsync, CreateInvokeResult, InvokeDotNetMethodAsync) and removes them from the trim-safe platform types, while gating the InvokeDotNet path behind RuntimeFeature.IsHybridWebViewSupported.

Key findings

  • Approach is sound: the removed class-level attributes were what produced unresolvable IL2026 from the iOS Managed Registrar <Module>.cctor() (which references all NSObject subclasses), and the surviving attributes correctly stay on the reflection/JSON methods.
  • RuntimeFeature.IsHybridWebViewSupported exists and defaults to true, so default app behavior is unchanged; when the switch is off (TrimFull/AOT) the guarded InvokeDotNet branch is trimmed away — consistent across iOS/Android/Windows.
  • The && RuntimeFeature.IsHybridWebViewSupported guards behave as a [FeatureGuard], which is the right pattern; no behavioral regression for normal builds.
  • No blocking code issues found.

CI note: Core build, Pack, and Helix unit tests are green. The broad iOS/macOS UITest failures do not appear PR-caused — the change is in Core and all build/unit legs pass; they look like infra/flaky test-run failures. Worth a glance at the WebView UITest category since it's the closest to this change, but no evidence it regressed.

Confidence: High.

This is an automated, non-approval review comment, not a GitHub approval/request-changes. A human must make the merge decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants