dotnet / linker

388 stars 127 forks source link

.NET 6.0 removing ctor for Activator.CreateInstance()-created types #1633

Open jonathanpeppers opened 3 years ago

jonathanpeppers commented 3 years ago

In getting our tests passing for .NET 6, we have a test where the linker is removing the .ctor for this type:

https://github.com/xamarin/xamarin-android/blob/9bc00032eebc30e91a66114eff9026c6a3b0e4d7/tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs#L282-L284 https://github.com/xamarin/xamarin-android/blob/9bc00032eebc30e91a66114eff9026c6a3b0e4d7/tests/MSBuildDeviceIntegration/Resources/LinkDescTest/MainActivityReplacement.cs#L35-L45

Given the type:

namespace Library1 { public class SomeClass { } }

It is only used with code such as:

var asm = typeof(Library1.SomeClass).Assembly;
var o = Activator.CreateInstance(asm.GetType("Library1.SomeClass"));

typeof() works fine, but the .ctor has been linked away.

This seems like it might be a common case that works today with current Xamarin.

Here are the assemblies from the linker output: linked.zip

UnnamedProject.dll is the root assembly, and Library1.dll contains this type.

MichalStrehovsky commented 3 years ago

Assembly.GetType is marked as linker unfriendly: https://github.com/dotnet/runtime/blob/223fd3ac34ea7cb77c445ee32c7fdfd2a0dd8caa/src/libraries/System.Private.CoreLib/src/System/Reflection/Assembly.cs#L103-L104 and linker warns when it sees it used.

The recommendation is to use Type.GetType that linker intrinsically recognizes.

We could add intrinsic recognition to Assembly.GetType, but based on API usage telemetry, it's a rarely used API (around 1% on NuGet.org) and it's unclear whether the intrinsic recognition would be able to actually help those 1% (we would need to see the assembly being operated on statically, and that requires quite a bit of luck). It's about limiting the API surface the linker needs to hardcode (linker doesn't currently understand typeof(X).Assembly either).

Is there a reason to believe this specific pattern would be common in Xamarin apps?

jonathanpeppers commented 3 years ago

Current stable Xamarin.Forms has a DI pattern, such as:

[assembly: Dependency(typeof(DeviceOrientationService))]

public class DeviceOrientationService : IDeviceOrientationService
{
    public DeviceOrientation GetOrientation() { ... }
}

https://docs.microsoft.com/xamarin/xamarin-forms/app-fundamentals/dependency-service/introduction

The implementation for this looks like it might break:

https://github.com/xamarin/Xamarin.Forms/blob/4e704d871f3379b58c6efa66b9942fb41df4de51/Xamarin.Forms.Core/DependencyResolver.cs

Let me actually test if this works or not in a Xamarin.Forms app.

jonathanpeppers commented 3 years ago

It does seem like the implementation in Xamarin.Forms breaks, I was able to reproduce it here:

https://github.com/xamarin/net6-samples/commit/b62b9599370d8b0cd0034bdeb048dce0fb570feb

It fails in a spot in Xamarin.Forms internally unless I use @(TrimmerRootDescriptor):

https://github.com/xamarin/Xamarin.Forms/blob/4e704d871f3379b58c6efa66b9942fb41df4de51/Xamarin.Forms.Core/Application.cs#L37

The HelloService.cs example works. It would probably need to be moved to another assembly to break, as HelloForms.dll was the root assembly.

We need to update these net6-samples, so others can try it. I used a local .NET 6 build.

MichalStrehovsky commented 3 years ago

Is Xamarin planning to do more aggressive trimming modes in .NET 6?

DI is one of the things that are fundamentally incompatible with more aggressive trim modes. Unless we add extra steps that specialcase the Xamarin DI, I wouldn't expect it to work with more aggressive trimming.

I see some investigation is happening to use Microsoft.Extensions for DI in Xamarin - source generators are being investigated there to make that DI trimming friendly: https://github.com/dotnet/runtime/issues/44432

eerhardt commented 3 years ago

source generators are being investigated there to make that DI trimming friendly

Note that as of now, the source generator for Microsoft.Extensions.DependencyInjection isn't scheduled for net6.0. Microsoft.Extensions.DependencyInjection was mostly made trimming friendly in net5.0 - https://github.com/dotnet/runtime/pull/40227. The only remaining ILLink warnings happen when registering open generic types in DI.

jonathanpeppers commented 3 years ago

Is Xamarin planning to do more aggressive trimming modes in .NET 6?

In current Xamarin (pre .NET 6), the linker modes we have are a little different than .NET 5+:

https://docs.microsoft.com/xamarin/android/deploy-test/linker#linker-behavior

Users can opt into $(AndroidLinkMode) = Full for Release builds, and by default SdkOnly trims framework assemblies like the BCL or Android/iOS libraries. We were planning on supporting these, as otherwise users migrating to .NET 6 will get larger apps.

The Link All Assemblies / Full option would be basically the same as setting %(TrimMode)=Link on every assembly, so this is what our current .NET 6 implementation does:

https://github.com/xamarin/xamarin-android/blob/b0d61764e5a5c0dec30a8faf2ef60967d4622c1b/src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.ILLink.targets#L18-L22

marek-safar commented 3 years ago

Is Xamarin planning to do more aggressive trimming modes in .NET 6?

The answer is that not by default but users won't be blocked to do that. I'm not sure we need to support "Full" mode this way unless you want to be in the pain to fight with the official dotnet CLI trimming settings.

jonathanpeppers commented 3 years ago

Do we need to consider a completely different UI here?

image image

The defaults we have right now seem to work, if you just have a default/empty .csproj such as:

https://github.com/xamarin/net6-samples/blob/4304e7d6909aa01f99e5bd91c5d83c0c0ee93806/HelloAndroid/HelloAndroid.csproj

For a .NET 5 console app, there is only a single Trim unused assemblies checkbox in the Publish dialogs:

image

marek-safar commented 3 years ago

Do we need to consider a completely different UI here?

A bit off-topic on this issue but nonetheless.

I don't think you should consider any Android-specific UI for trimming. Firstly the same thing should work from CLI and secondly there is no need to specialize anything at UI level to be Android-specific.

I'd suggest migrating from the old SDK/All concept (which is inaccurate in today's XA anyway) into common .NET Core PublishTrimmed setup. You could do that by simply setting PublishTrimmed to true in the default template and ignore whatever is set in today's projects.

For more details about how the trimming will be controlled internally see for example https://github.com/mono/linker/issues/1269#issuecomment-728385960

vitek-karas commented 3 years ago

On the original topic - how did this work before? AFAIK linker never really recognized Assembly.GetType and if full trimming was enabled I would expect even the "old" linker to remove the .ctor.

jonathanpeppers commented 3 years ago

@vitek-karas I made a sample, you can build this in Release mode:

https://github.com/jonathanpeppers/AndroidLinkModeFull

I used our (somewhat documented) feature of dumping the linker-dependencies.xml.gz file.

When running:

> illinkanalyzer --types linker-dependencies.xml.gz

I see this, but it doesn't explain why the .ctor is kept:

--- TypeDef:AndroidClassLibrary.Class1 dependencies --------------------
Dependency #1
    TypeDef:AndroidClassLibrary.Class1
    | Method:System.Void AndroidApp.MainActivity::FabOnClick(System.Object,System.EventArgs) [4 deps]
    | TypeDef:AndroidApp.MainActivity [2 deps]
    | Assembly:AndroidApp, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null [2 deps]
    | Other:Mono.Linker.Steps.ResolveFromAssemblyStep
jonathanpeppers commented 3 years ago

Oh, this gave more info:

> illinkanalyzer -a linker-dependencies.xml.gz
...
Dependency #85
    Method:System.Void System.Object::.ctor()
    | Method:System.Void AndroidClassLibrary.Class1::.ctor() [2 deps]
    | Other:Reflection-System.Void AndroidClassLibrary.Class1::.ctor() [1 deps]
    | Method:System.Void AndroidApp.MainActivity::FabOnClick(System.Object,System.EventArgs) [4 deps]
    | TypeDef:AndroidApp.MainActivity [2 deps]
    | Assembly:AndroidApp, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null [2 deps]
    | Other:Mono.Linker.Steps.ResolveFromAssemblyStep
marek-safar commented 3 years ago

I think it'd be valuable to actually find out what pattern all this code is trying to persist. There is a lot of Android customization (e.g. Android.Runtime.Preserve) which might not work well going forward and has a possibly better alternative. I was talking to @vitek-karas and he will follow up to understand the scenario fully.