Add skill for AOT-compatibility - #122
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new skill for making .NET projects compatible with Native AOT and trimming by systematically resolving IL trim/AOT analyzer warnings. It includes comprehensive documentation, polyfill examples, and a real-world conversion demonstrating the before and after states of an Azure ResourceManager project.
Changes:
- Added
dotnet-aot-compatskill with detailed documentation on resolving trim/AOT warnings - Provided comprehensive test fixtures showing AOT-compatible code patterns
- Included polyfills for annotation attributes on older target frameworks
Reviewed changes
Copilot reviewed 47 out of 930 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/dotnet-aot-compat/SKILL.md | Complete skill documentation explaining AOT compatibility concepts and step-by-step resolution procedures |
| skills/dotnet-aot-compat/tests/after/Azure.ResourceManager.csproj | Project file demonstrating AOT compatibility enablement with <IsAotCompatible>true</IsAotCompatible> |
| skills/dotnet-aot-compat/tests/after/ExperimentalAttribute.cs | Polyfill implementation for the ExperimentalAttribute on pre-.NET 8 frameworks |
| skills/dotnet-aot-compat/tests/after/GenericOperationSource.cs | Example showing proper use of [DynamicallyAccessedMembers] annotations for AOT compatibility |
| skills/dotnet-aot-compat/tests/after/ArmOperation.cs | Demonstrates AOT-compatible operation rehydration with proper member annotations |
| skills/dotnet-aot-compat/tests/after/ResourceManagerJsonContext.cs | Source-generated JSON serialization context for AOT compatibility |
| skills/dotnet-aot-compat/tests/after/Extensions/ArmClientBuilderExtensions.cs | Shows use of [RequiresDynamicCode] and [RequiresUnreferencedCode] attributes |
| skills/dotnet-aot-compat/tests/after/**/Custom/*.cs | Multiple custom resource implementations demonstrating AOT-compatible patterns |
| skills/dotnet-aot-compat/tests/after/Directory.Packages.props | Central package version management configuration |
| skills/dotnet-aot-compat/tests/after/Assets/Profile/2020-09-01-hybrid.json | Azure Stack profile configuration data |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// <summary> Determines if two <see cref="ArmEnvironment"/> values are the same. </summary> | ||
| public static bool operator ==(ArmEnvironment left, ArmEnvironment right) => left.Equals(right); | ||
|
|
||
| /// <summary> Determines if two <see cref="ArmEnvironment"/> values are not the same. </summary>internal |
There was a problem hiding this comment.
The comment internal appears on line 62 before the operator declaration but should either be removed or placed on its own line as a proper comment. As written, it would cause a compilation error.
| /// <summary> Determines if two <see cref="ArmEnvironment"/> values are not the same. </summary>internal | |
| /// <summary> Determines if two <see cref="ArmEnvironment"/> values are not the same. </summary> |
| switch (format) | ||
| { | ||
| case "J": | ||
| return ModelReaderWriter.Write(this, options, AzureResourceManagerContext.Default); |
There was a problem hiding this comment.
Reference to AzureResourceManagerContext.Default is used, but this type is not defined in any of the provided files. This would cause a compilation error unless this context is defined elsewhere in the project.
| return ModelReaderWriter.Write(this, options, AzureResourceManagerContext.Default); | |
| return ModelReaderWriter.Write(this, options, ResourceManagerJsonContext.Default); |
| private void AppendChildObject(StringBuilder stringBuilder, object childObject, ModelReaderWriterOptions options, int spaces, bool indentFirstLine) | ||
| { | ||
| string indent = new string(' ', spaces); | ||
| BinaryData data = ModelReaderWriter.Write(childObject, options, AzureResourceManagerContext.Default); |
There was a problem hiding this comment.
Same issue as above - AzureResourceManagerContext.Default is referenced but not defined in the provided files, which would cause compilation failure.
| BinaryData data = ModelReaderWriter.Write(childObject, options, AzureResourceManagerContext.Default); | |
| BinaryData data = ModelReaderWriter.Write(childObject, options); |
|
this is a very large addition, largely test. is there any concern with this source becoming public? |
|
Results are still being run -- this takes a long time because of the length of the task. This project is pulled from the Azure SDK and lightly modified. That's an open-source project, so I don't think there would be a problem with making it public. I'll try to de-dup with the other skill as well. |
got it. thoughts on using a smaller portion? Or is the value at this size? (eg. seeing if we can get the same value with a less expensive CI cost) |
|
Both the no-skill and skilled version are terrible -- no skill is 1.0/5.0 for trying to suppress everything with pragmas. Skilled is 1.2/5.0 for making some progress and fixing a few warnings correctly, but then spending the rest of the time scanning the code base before timing out. I think the real-world test is very good here. It showing that handing a real, large project to the agent is a significant problem. They can get distracted, take shortcuts, and generally do things they wouldn't do in a narrow hand-picked example. I'm making some changes to the skill and re-running and I'll see if that improves things. |
|
Alright, I've hit another problem. The LLM is cheating. It's pulling from the |
Also adds a real-world project conversion between the before and after tests.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Expand SKILL.md with routing signals, severity markers, stop signals, triage step, batch-fix strategy, IJsonModel guidance, and sub-agent dispatch pattern - Extract polyfill code block to references/polyfills.md - Add eval scenario and test fixtures at tests/dotnet/dotnet-aot-compat/ following repo convention (moved from skills/ to tests/<plugin>/) - Eval validates at +25-47% improvement across multiple runs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Warn against adding external package types to JsonSerializerContext (prevents the ResponseError compile-error trap) - Strengthen anti-exploration directive with concrete negative examples - Push sub-agent parallelization after 2 build-fix cycles Validated with 3-run eval (Opus 4.6): pass, rubric 4.4/5. Cross-judge rejudge (Sonnet 4.5, GPT-5.1-Codex, Gemini 3 Pro): all pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
I was able to push to your branch now. Here's what changed: SKILL.md improvements (commit e44121c):
Eval tests (commit 98b609f):
Validation results:
|
agocke
left a comment
There was a problem hiding this comment.
Overall I think the new changes are good, but I have worries about overfitting. I think we'll likely need to introduce more projects and see how it fairs with something completely different.
| ``` | ||
|
|
||
| When you annotate a parameter, **all callers** must now pass properly annotated types. This cascades outward — follow each caller and annotate or refactor as needed. | ||
| When you annotate a parameter, **all callers** must now pass properly annotated types. This cascades outward — follow each caller and annotate or refactor as needed. **The caller's annotation must include at least the same member types as the callee's.** If the callee requires `PublicConstructors | NonPublicConstructors`, the caller must specify the same or a superset — using only `NonPublicConstructors` will produce IL2091. |
There was a problem hiding this comment.
This feels like strangely-specific direction...
There was a problem hiding this comment.
It seems without some kind of bound, the models may get lost. Is it too weird?
|
|
||
| ## Common Gotchas | ||
|
|
||
| 1. **External types without AOT-safe serialization**: When a type comes from a dependency you can't modify (e.g., `ResponseError` from `Azure.Core`) and it lacks a source-generated serializer, `Options.GetConverter<T>()` is reflection-based and will produce IL warnings. First check if the type implements `IJsonModel<T>` (common in Azure SDK) — if so, bypass `JsonSerializer` entirely: |
There was a problem hiding this comment.
The note about ResponseError looks wrong -- I think it's overfitting to the specific test case.
There was a problem hiding this comment.
ResponseError was the last area the models got hung up on and couldn't finish the job. I don't know if taking this away will throw the models back into not being able to finish.
Do you have a 'medium sized' project? That might be a good way to tell. |
|
I ran this locally on Roslyn -- some amount of overfitting. But I don't want to get too into the weeds on this PR. Let's merge it and follow-up. |
Also adds a real-world project conversion between the before and after tests.