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

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

Open
jonathanpeppers opened this issue Nov 17, 2020 · 13 comments
Open

Comments

@jonathanpeppers
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

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 added a commit to dotnet/maui-samples that referenced this issue Nov 17, 2020
The `HelloService.cs` example in this app works, but the case in
Xamarin.Forms startup here does not:

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

I suspect `HelloService.cs` works because `HelloForms.dll` is the root
assembly.

Using `@(TrimmerRootDescriptor)` solved the issue here.
@jonathanpeppers
Copy link
Member Author

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

dotnet/maui-samples@b62b959

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
Copy link
Member

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: dotnet/runtime#44432

@eerhardt
Copy link
Member

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 - dotnet/runtime#40227. The only remaining ILLink warnings happen when registering open generic types in DI.

@jonathanpeppers
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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 #1269 (comment)

@vitek-karas
Copy link
Member

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
Copy link
Member Author

@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
Copy link
Member Author

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants