Add Trim/NativeAOT safety rules to code review skill#34962
Open
PureWeen wants to merge 1 commit into
Open
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 <223556219+Copilot@users.noreply.github.com>
Contributor
|
🚀 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" |
Contributor
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.
Suggested change
| | **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.
Suggested change
| Uses independence-first assessment (code before narrative) with 364 lines of | |
| Uses independence-first assessment (code before narrative) with |
This was referenced Apr 16, 2026
This was referenced Apr 17, 2026
This was referenced May 17, 2026
Draft
Contributor
|
/review -b feature/refactor-copilot-yml |
Collaborator
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.