Add Trim/NativeAOT safety rules to code review skill#34962
Conversation
Add Section 23 to review-rules.md covering IL2026/IL3050 annotation chain analysis, the extract-method pattern for feature-guarded types, and why [UnconditionalSuppressMessage] is almost always wrong for these warnings. Sourced from the HybridWebViewHandler incident (Issue #34867, PR #34958) where AI agents consistently recommended suppression instead of the correct structural fix when they didn't read the source code first. Co-authored-by: Copilot <[email protected]>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34962Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34962" |
There was a problem hiding this comment.
Pull request overview
This PR updates the .github/skills/code-review skill documentation to add explicit Trim/NativeAOT safety guidance (focused on IL2026/IL3050 annotation-chain analysis) motivated by the HybridWebViewHandler IL3050 incident.
Changes:
- Added Section 23 (“Trim / NativeAOT Safety”) to
review-rules.mdwith six concrete review checks to discourage suppression-based “fixes” and encourage correct structural patterns. - Updated
SKILL.mdto reflect the expanded rule set and added a new “Safety” category callout.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/skills/code-review/references/review-rules.md | Adds a new Section 23 with reviewer heuristics for Trim/NativeAOT (IL2026/IL3050) issues and recommended fix patterns. |
| .github/skills/code-review/SKILL.md | Updates the skill metadata/description and lists “Safety” as a review category referencing the new section. |
| |-------|-----------------| | ||
| | **`[UnconditionalSuppressMessage]` for IL2026/IL3050 is almost always wrong** | If a PR adds `[UnconditionalSuppressMessage("AOT", "IL3050:...")]` or `[UnconditionalSuppressMessage("Trimming", "IL2026:...")]`, it is very likely hiding a real problem rather than fixing it. The correct response is to restructure the code so the analyzer can prove safety — typically via the extract-method pattern below. Only accept suppression if the PR includes proof that the code path is genuinely unconditionally safe (e.g., the type is explicitly preserved). (Issue #34867 — simonrozsival) | | ||
| | **`[FeatureGuard]` does NOT suppress indirect annotation chains** | `[FeatureGuard(typeof(RequiresDynamicCodeAttribute))]` on a `RuntimeFeature` property suppresses IL3050 only for **direct** calls to `[RequiresDynamicCode]` methods inside the guarded block. It does **not** suppress warnings from indirect chains — e.g., generic type parameters with `[DynamicallyAccessedMembers]` that trace back to an annotated type. If a PR claims "the feature guard handles it" but the warning goes through a generic like `AddHandler<T, TRender>()`, the guard is NOT sufficient. (Issue #34867 — simonrozsival) | | ||
| | **Extract-method pattern for feature-guarded annotated types** | When registering a type annotated with `[RequiresDynamicCode]` or `[RequiresUnreferencedCode]` inside a `[FeatureGuard]`-protected block, and the registration goes through a generic API with `[DynamicallyAccessedMembers]`, extract the call into a separate method annotated with the same attributes. This converts the indirect chain into a direct call that the `[FeatureGuard]` can suppress. Reference: `AppHostBuilderExtensions.AddHybridWebViewHandler()` in `src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs`. (PR #34958 — simonrozsival) | |
There was a problem hiding this comment.
The reference to AppHostBuilderExtensions.AddHybridWebViewHandler() appears to be stale/incorrect: there is no AddHybridWebViewHandler method in src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs (current code registers HybridWebView inside AddControlsHandlers / SetupDefaults). Please update this reference to the actual helper/method name used in the repo (or link to a stable permalink to the relevant PR/commit) so reviewers can find the example quickly.
| | **Extract-method pattern for feature-guarded annotated types** | When registering a type annotated with `[RequiresDynamicCode]` or `[RequiresUnreferencedCode]` inside a `[FeatureGuard]`-protected block, and the registration goes through a generic API with `[DynamicallyAccessedMembers]`, extract the call into a separate method annotated with the same attributes. This converts the indirect chain into a direct call that the `[FeatureGuard]` can suppress. Reference: `AppHostBuilderExtensions.AddHybridWebViewHandler()` in `src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs`. (PR #34958 — simonrozsival) | | |
| | **Extract-method pattern for feature-guarded annotated types** | When registering a type annotated with `[RequiresDynamicCode]` or `[RequiresUnreferencedCode]` inside a `[FeatureGuard]`-protected block, and the registration goes through a generic API with `[DynamicallyAccessedMembers]`, extract the call into a separate method annotated with the same attributes. This converts the indirect chain into a direct call that the `[FeatureGuard]` can suppress. Reference: `HybridWebView` registration in `AddControlsHandlers` / `SetupDefaults` in `src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs`. (PR #34958 — simonrozsival) | |
| description: >- | ||
| Deep code review of PR changes for correctness, safety, and MAUI conventions. | ||
| Uses independence-first assessment (code before narrative) with 345 lines of | ||
| Uses independence-first assessment (code before narrative) with 364 lines of |
There was a problem hiding this comment.
The SKILL description says the rules file has "364 lines", but references/review-rules.md currently has 365 lines. Consider either updating the number or (more robust) removing the hard-coded line count so it doesn't drift each time rules are edited.
| Uses independence-first assessment (code before narrative) with 364 lines of | |
| Uses independence-first assessment (code before narrative) with |
|
/review -b feature/refactor-copilot-yml |
This comment has been minimized.
This comment has been minimized.
PureWeen
added
the
area-ai-agents
|
/review -b feature/enhanced-reviewer -p android |
|
|
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
🤖 Multi-model code review — Request changes
Three models reviewed this PR independently (Claude Opus 4.8, GPT-5.5, Gemini 3.1 Pro), then cross-pollinated.
Consensus: ❌ Request changes — valuable section, but two technical inaccuracies in a skill that shapes future reviews.
The core guidance is sound ✅
The headline technical claim — that [FeatureGuard(typeof(RequiresDynamicCodeAttribute))] suppresses IL3050 only for direct [RequiresDynamicCode] calls, not for indirect chains (generic params with [DynamicallyAccessedMembers] that trace back to an annotated type) — is correct (all three models independently verified it against how the trimmer/ILC propagates annotations). The "suppression is almost always the wrong fix" framing and the #pragma warning disable ILxxxx equivalence are also fair. The line-count bump (364) and markdown are fine.
Two accuracy fixes before merge
- Branch-mismatched code reference (line 344) — the extract-method example cites
AppHostBuilderExtensions.AddHybridWebViewHandler(), but that method exists only on net11.0 (where the fix PR #34958 merged), not on this PR'smainbase — here the registration is still inline (AddHandler<HybridWebView, HybridWebViewHandler>()inside theRuntimeFeature.IsHybridWebViewSupportedguard, ~lines 130-133). A rule added tomainthat points at code absent frommainwill confuse reviewers — update it to matchmain(or note it's net11.0-only / pending #34958). - Incorrect IL claim (line 345) — "accessing a
conston the type embeds the type reference in the caller's IL metadata" is false: aconst stringis folded to anldstrliteral at compile time with noTypeRef/MemberRefto the declaring type (Opus also noted the originating commit 2 was a cosmetic "address review" edit, not a trimmer fix). The "use a local const instead" advice is harmless but its stated reason is a misconception (it would be true for astatic readonlyfield, not aconst).
Since this is the code-review skill itself, getting the rules technically exact matters — otherwise reviewers will cite phantom code or flag non-issues.
Independent verdicts: GPT-5.5 — NEEDS_CHANGES (high) · Gemini 3.1 Pro — NEEDS_CHANGES · Opus 4.8 — NEEDS_DISCUSSION (high). Consensus: keep the section (the FeatureGuard insight is genuinely useful and was empirically verified), fix the two inaccuracies.
| |-------|-----------------| | ||
| | **`[UnconditionalSuppressMessage]` for IL2026/IL3050 is almost always wrong** | If a PR adds `[UnconditionalSuppressMessage("AOT", "IL3050:...")]` or `[UnconditionalSuppressMessage("Trimming", "IL2026:...")]`, it is very likely hiding a real problem rather than fixing it. The correct response is to restructure the code so the analyzer can prove safety — typically via the extract-method pattern below. Only accept suppression if the PR includes proof that the code path is genuinely unconditionally safe (e.g., the type is explicitly preserved). (Issue #34867 — simonrozsival) | | ||
| | **`[FeatureGuard]` does NOT suppress indirect annotation chains** | `[FeatureGuard(typeof(RequiresDynamicCodeAttribute))]` on a `RuntimeFeature` property suppresses IL3050 only for **direct** calls to `[RequiresDynamicCode]` methods inside the guarded block. It does **not** suppress warnings from indirect chains — e.g., generic type parameters with `[DynamicallyAccessedMembers]` that trace back to an annotated type. If a PR claims "the feature guard handles it" but the warning goes through a generic like `AddHandler<T, TRender>()`, the guard is NOT sufficient. (Issue #34867 — simonrozsival) | | ||
| | **Extract-method pattern for feature-guarded annotated types** | When registering a type annotated with `[RequiresDynamicCode]` or `[RequiresUnreferencedCode]` inside a `[FeatureGuard]`-protected block, and the registration goes through a generic API with `[DynamicallyAccessedMembers]`, extract the call into a separate method annotated with the same attributes. This converts the indirect chain into a direct call that the `[FeatureGuard]` can suppress. Reference: `AppHostBuilderExtensions.AddHybridWebViewHandler()` in `src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs`. (PR #34958 — simonrozsival) | |
There was a problem hiding this comment.
AppHostBuilderExtensions.AddHybridWebViewHandler() as THE canonical extract-method example, but that method isn't on this PR's main base — grep -rn AddHybridWebViewHandler src/ finds nothing here, and the actual HybridWebView registration in that file (lines ~130-133) is still inline: if (RuntimeFeature.IsHybridWebViewSupported) { handlersCollection.AddHandler<HybridWebView, HybridWebViewHandler>(); }. The extracted method exists only on net11.0, where the fix PR #34958 merged. So this rule (added against main) points reviewers at code that isn't present on the branch it lives on. Please update the reference to match main (or explicitly note it's net11.0-only / pending #34958 flowing to main), so reviewers on main aren't sent to absent code.
| | **`[UnconditionalSuppressMessage]` for IL2026/IL3050 is almost always wrong** | If a PR adds `[UnconditionalSuppressMessage("AOT", "IL3050:...")]` or `[UnconditionalSuppressMessage("Trimming", "IL2026:...")]`, it is very likely hiding a real problem rather than fixing it. The correct response is to restructure the code so the analyzer can prove safety — typically via the extract-method pattern below. Only accept suppression if the PR includes proof that the code path is genuinely unconditionally safe (e.g., the type is explicitly preserved). (Issue #34867 — simonrozsival) | | ||
| | **`[FeatureGuard]` does NOT suppress indirect annotation chains** | `[FeatureGuard(typeof(RequiresDynamicCodeAttribute))]` on a `RuntimeFeature` property suppresses IL3050 only for **direct** calls to `[RequiresDynamicCode]` methods inside the guarded block. It does **not** suppress warnings from indirect chains — e.g., generic type parameters with `[DynamicallyAccessedMembers]` that trace back to an annotated type. If a PR claims "the feature guard handles it" but the warning goes through a generic like `AddHandler<T, TRender>()`, the guard is NOT sufficient. (Issue #34867 — simonrozsival) | | ||
| | **Extract-method pattern for feature-guarded annotated types** | When registering a type annotated with `[RequiresDynamicCode]` or `[RequiresUnreferencedCode]` inside a `[FeatureGuard]`-protected block, and the registration goes through a generic API with `[DynamicallyAccessedMembers]`, extract the call into a separate method annotated with the same attributes. This converts the indirect chain into a direct call that the `[FeatureGuard]` can suppress. Reference: `AppHostBuilderExtensions.AddHybridWebViewHandler()` in `src/Controls/src/Core/Hosting/AppHostBuilderExtensions.cs`. (PR #34958 — simonrozsival) | | ||
| | **Don't reference annotated types from the extracted helper's attributes** | When creating the extracted helper method, use a local `const string` for the annotation message instead of referencing a const on the annotated type (e.g., `HybridWebViewHandler.DynamicFeatures`). Accessing a const on the type embeds the type reference in the caller's IL metadata, re-introducing the very problem the extraction was meant to solve. (PR #34958, commit 2 — simonrozsival) | |
There was a problem hiding this comment.
const (compile-time constant) like HybridWebViewHandler.DynamicFeatures is folded to its literal value at compile time — the C# compiler emits an ldstr of the string and does not emit a TypeRef/MemberRef to the declaring type. So referencing another type's const string (even in an attribute argument) does not re-introduce a type reference, and the stated rationale for "use a local const string instead" is a misconception. (Using a local const is harmless, but teaching reviewers this false IL claim will cause them to flag non-issues.) If the intent was about a static readonly field — which would emit a member/type reference — please say that instead, since the const case behaves differently.
Summary
Add Section 23 (Trim / NativeAOT Safety) to the code review skill's
review-rules.md, covering 6 checks for IL2026/IL3050 annotation chain analysis.Motivation
During investigation of Issue #34867 (HybridWebViewHandler IL3050 warnings), we ran a controlled experiment where multiple AI models were asked to diagnose and fix the issue:
[UnconditionalSuppressMessage]Models that failed shared a shallow heuristic: feature switch guards the code → warning is false positive → suppress it. They didn't reason about why
[FeatureGuard]wasn't working (indirect annotation chain through[DynamicallyAccessedMembers]on generic type params).The correct fix (PR #34958 by @simonrozsival) extracts the annotated type reference into a separate method with matching
[RequiresDynamicCode]/[RequiresUnreferencedCode]attributes, converting the indirect chain into a direct call that the[FeatureGuard]can suppress.What changed
review-rules.md: New Section 23 with 6 review checks:[UnconditionalSuppressMessage]for IL2026/IL3050 is almost always wrong[FeatureGuard]does NOT suppress indirect annotation chains#pragma warning disable ILxxxxis equally wrongSKILL.md: Added "Safety" category referencing the new sectionWhy the code review skill (not just an instruction file)
The existing
trim-aot.instructions.mdusesapplyToglobs (**/RuntimeFeature.cs,**/*Handler*.cs, etc.) that only fire when editing matching files. This means:The code review skill is the right home because it's always explicitly invoked and will be integrated into the try-fix pipeline.