Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Trimming] Enable trimming and AOT analyzers in Core #21076

Merged
merged 9 commits into from
Mar 28, 2024

Conversation

simonrozsival
Copy link
Member

Description of Change

This PR marks the Core project as AOT-compatible and it fixes all the warnings reported by the analyzers which are now enabled.

Issues Fixed

Contributes to #18658

/cc @jonathanpeppers @vitek-karas

@simonrozsival simonrozsival added the t/app-size Application Size / Trimming (sub: perf) label Mar 7, 2024
@simonrozsival simonrozsival requested a review from a team as a code owner March 7, 2024 11:59
Comment on lines +16 to +17
[RequiresDynamicCode("The GetImageSourceServiceType method is not AOT compatible. Use GetImageSourceService instead.")]
[RequiresUnreferencedCode("The GetImageSourceServiceType method is not trimming compatible. Use GetImageSourceService instead.")]
Copy link
Member

Choose a reason for hiding this comment

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

Just for future reference:
We've been burned in other projects by adding RUC (and other annotations) onto interface members. The problem is that doing so effectively marks all implementations of the interface with RUC. But RUC is a statement about the implementation, not about the interface.

For example Type.GetMembers - this has some annotation on it, but it's an abstract class. The reflection based implementation needs that annotation, but we also have MetadataLoadContext which also implements Type and that one doesn't need the annotation. But there's no way to tell that to the system and it will produce warnings and so on. Not that we will change Type.GetMembers, but it shows how the statement applies to all implementations, regardless if they are affected or not. (and also, removing the annotation from the interface member is a breaking change!)

I guess in this specific case, we basically don't care, since the methods are obsolete anyway. But it's a good rule to remember for the future: "Think hard if the annotation should be on the interface, really hard!"

Copy link
Member Author

Choose a reason for hiding this comment

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

I struggled with this one. It feels wrong adding those attributes to the interface. I just couldn't figure out how else to apply the annotation in this case. I arrived at the same conclusion that we don't care too much since we already marked it obsolte and we don't know about any customer who would have their custom implementation of IImageSourceProvider at the moment.

@simonrozsival simonrozsival changed the title [Trimming] Mark Core as AOT compatible [Trimming] Enable trimming and AOT analyzers in Core Mar 7, 2024
@simonrozsival
Copy link
Member Author

The analyzers reported the following issues on Windows:

D:\a\1\s\src\Core\src\obj\Release\net9.0-windows10.0.20348\XamlTypeInfo.g.cs(1749,13): error IL2059: Unrecognized value passed to the parameter 'type' of method 'System.Runtime.CompilerServices.RuntimeHelpers.RunClassConstructor(RuntimeTypeHandle)'. It's not possible to guarantee the availability of the target static constructor. [D:\a\1\s\src\Core\src\Core.csproj::TargetFramework=net9.0-windows10.0.20348]
D:\a\1\s\src\Core\src\Platform\Windows\FrameworkElementExtensions.cs(182,9): error IL2072: 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.All' in call to 'Microsoft.Maui.Platform.ReflectionExtensions.GetField(Type, Func<FieldInfo, Boolean>)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [D:\a\1\s\src\Core\src\Core.csproj::TargetFramework=net9.0-windows10.0.20348]
D:\a\1\s\src\Core\src\obj\Release\net9.0-windows10.0.19041\XamlTypeInfo.g.cs(1749,13): error IL2059: Unrecognized value passed to the parameter 'type' of method 'System.Runtime.CompilerServices.RuntimeHelpers.RunClassConstructor(RuntimeTypeHandle)'. It's not possible to guarantee the availability of the target static constructor. [D:\a\1\s\src\Core\src\Core.csproj::TargetFramework=net9.0-windows10.0.19041]
D:\a\1\s\src\Core\src\Platform\Windows\FrameworkElementExtensions.cs(182,9): error IL2072: 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.All' in call to 'Microsoft.Maui.Platform.ReflectionExtensions.GetField(Type, Func<FieldInfo, Boolean>)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [D:\a\1\s\src\Core\src\Core.csproj::TargetFramework=net9.0-windows10.0.19041]

We could do something about FrameworkElementExtensions.cs but since there is also a warning coming from the source generated by WinUI, I suggest we don't enable the analyzers on Windows for now.

@jonathanpeppers
Copy link
Member

jonathanpeppers commented Mar 11, 2024

The analyzers reported the following issues on Windows:
...

My "hope" is we would soon be able to put something like this in the project template:

<!-- greenfield apps should have an easier time with this -->
<PropertyGroup Condition="'$(Configuration)' == 'Release'">
  <TrimMode>full</TrimMode>
</PropertyGroup>

Then in the iOS/Android workloads, we would do similar for the iOS/Android templates there.

But if we do this in MAUI, will there be trimming warnings from the WindowsAppSDK we can't do much about?

@vitek-karas
Copy link
Member

I asked and apparently they're aware of it, and hopefully will fix it soon.

@jonathanpeppers
Copy link
Member

I have sent WindowsAppSdk PRs in the past, and that seemed to move things along more quickly. So, if the changes are simple, we can consider that.

@simonrozsival
Copy link
Member Author

The failing test seems unrelated

@rmarinho rmarinho merged commit 873dda0 into dotnet:net9.0 Mar 28, 2024
45 of 47 checks passed
@simonrozsival simonrozsival deleted the aot-compatible-core branch March 28, 2024 13:08
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2024
@Eilon Eilon added the area-publishing Issues with the app packaging/publishing process (ipk/apk/msix/trimming) label May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-publishing Issues with the app packaging/publishing process (ipk/apk/msix/trimming) fixed-in-9.0.0-preview.3.10457 t/app-size Application Size / Trimming (sub: perf)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants