[release/8.0] Remove base type rooting for types in rooted assemblies #93355
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
When rooting entire assemblies NativeAOT also roots all base types of all the types in such assembly. Regardless of which assembly the base type comes from. Historically this was necessary to make certain combinations of generic instantiations to work, but that's no longer the case. The compiler has better mechanism of tracking necessary things for generics now. This rooting behavior brings in more code than necessary. This is specifically problematic when using the assembly rooting as a mechanism to test library trim/AOT compatibility. If the tested library depends on another library which has some incompatible code in it, but it's not used, ideally the compiler should remove the unused code and thus not see the incompatibilities. Rooting entire base types sometimes breaks that behavior and produces unnecessary warnings. The product change is really just "don't root the base type", all of the rest of the change is tests. Modified existing test to validate more things around rooting behavior across assemblies. And then infrastructure changes to make it possible to use this test from NativeAOT. This brings the behavior of NativeAOT and trimmer much closer with regard to assembly rooting.
Rooting from rd.xml is kept as before (rooting a type will also root its base types). Given that rd.xml is already very problematic and tricky and people rely on all kinds of "accidental" behavior, it's not worth the break - there's not much complexity in keeping this behavior there. Update trimming tests which started to fail after the infra updates. Mostly disable the tests for NativeAOT as they don't make sense on AOT.
github-actions
bot
requested review from
marek-safar and
MichalStrehovsky
as code owners
dotnet-issue-labeler
bot
added
the
needs-area-label
|
@agocke @vitek-karas @MichalStrehovsky can you please fill out the template, confirm the CI failures are unrelated, and send an email to Tactics requesting approval? |
|
@MichalStrehovsky could you please review the description of the PR - I tried to capture what you described in email. Please feel free to fix/change it as you see fit. |
|
@vitek-karas @agocke don't forget to send an email to Tactics requesting approval. I couldn't find one yet. |
carlossanlop
added
Servicing-consider
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsBackport of #92864 to release/8.0 /cc @agocke @vitek-karas Customer ImpactThe original issue was discovered when implementing an AOT test for a subset of Azure-SDK's assemblies. It caused unnecessary code be brought into the test app and reported warnings in that code. This introduces effective noise to the test, as it will produce warnings which there's no good way to resolve them. In an actual end-user app, the unnecessary code is correctly removed (as it doesn't use assembly rooting) and the warnings are not reported. TestingThe change adds tests targeting the new behavior. The change was also tested against the Azure-SDK test app, and it does solve the problem there. RiskFor applications which follow our supported usage of Native AOT, that is which rely on static analysis warnings and make sure to correctly handle all such warnings, this change has no effect. In short applications with zero AOT/trim warnings are not affected by this change. For these apps the risk is very low as the affected feature is not used by these apps. These are the majority of NativeAOT apps. Applications which rely on unsupported behavior and ignore AOT/trim analysis warnings may be broken by this change since it potentially removes more code from the app. The new behavior is only trigged when using the
|
agocke
added
Servicing-approved
|
This was approved and should be ready to merge |
|
@vitek-karas @agocke @sbomer there is a CI failure that seems related to this change. I opened an issue for it, but I found it in this PR only: #93621 |
|
@carlossanlop thanks for isolating it - I'll fix it tomorrow (it's very likely new code depending on changes only made in main - it's a test code change, so not really impactful to the backport as such) |
|
Thanks for fixing it, @vitek-karas. Merging now. |
ghost
locked as resolved and limited conversation to collaborators
Backport of #92864 to release/8.0
/cc @agocke @vitek-karas
Customer Impact
The original issue was discovered when implementing an AOT test for a subset of Azure-SDK's assemblies. It caused unnecessary code be brought into the test app and reported warnings in that code. This introduces effective noise to the test, as it will produce warnings which there's no good way to resolve them. In an actual end-user app, the unnecessary code is correctly removed (as it doesn't use assembly rooting) and the warnings are not reported.
Testing
The change adds tests targeting the new behavior. The change was also tested against the Azure-SDK test app, and it does solve the problem there.
Risk
For applications which follow our supported usage of Native AOT, that is which rely on static analysis warnings and make sure to correctly handle all such warnings, this change has no effect. In short applications with zero AOT/trim warnings are not affected by this change. For these apps the risk is very low as the affected feature is not used by these apps. These are the majority of NativeAOT apps.
Applications which rely on unsupported behavior and ignore AOT/trim analysis warnings may be broken by this change since it potentially removes more code from the app. The new behavior is only trigged when using the
TrimmerRootAssemblyMSBuild item group, which in itself is not a supported usage of NativeAOT. Applications in this group can be broken by many types of changes, even a simple refactoring of code inside the framework can lead to such breaks as the applications rely on "by accident" behavior which happens to work in a specific release.