dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.16k stars 4.72k forks source link

[API Proposal]: trim-safe RuntimeHelpers.RunClassConstructor(Type) overload #103679

Closed Sergio0694 closed 3 months ago

Sergio0694 commented 4 months ago

Background and motivation

In the XAML compiler, when targeting modern .NET, we need to invoke RuntimeHelpers.RunClassConstructor to make sure that the static constructor for a given type is initialized before some other code runs. Most scenarios for this revolve around dependency properties, which in some cases need to be initialized and registered in the WinRT runtime before some other code runs (eg. before the activation factory for a given type is retrieved). To make this trim-safe, the lifted XAML compiler was updated with a bunch of [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] annotations on all such types, and a suppression on the method calling RuntimeHelpers.RunClassConstructor with these types.

You can see the use in the XAML compiler here.

However, talking with @agocke and @SingleAccretion today, they pointed out that this pattern is not supported, and it's not guaranteed to work. So this proposal is for a safe, annotated overload of the same method, taking a Type instance instead.

API Proposal

 namespace System.Diagnostics.CodeAnalysis
 {
     public enum DynamicallyAccessedMemberTypes
     {
+        StaticConstructor = 0x4000,
     }
 }

 namespace System.Runtime.CompilerServices
 {
     public static partial class RuntimeHelpers
     {
+        public static void RunClassConstructor([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.StaticConstructor)] Type type);
     }
 }

API Usage

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.StaticConstructor)]
public Type UnderlyingType => _underlyingType;

private void RunInitializerImplementation()
{
    RuntimeHelpers.RunClassConstructor(UnderlyingType.TypeHandle);
}
dotnet-policy-service[bot] commented 4 months ago

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar See info in area-owners.md if you want to be subscribed.

dotnet-policy-service[bot] commented 4 months ago

Tagging subscribers to this area: @dotnet/area-system-reflection See info in area-owners.md if you want to be subscribed.

teo-tsirpanis commented 4 months ago

This new DynamicallyAccessedMemberTypes value will essentially have only this use case. Could instead the trimmer just always preserve the class constructors of all preserved types? How wasteful would it be?

agocke commented 4 months ago

@sbomer and @MichalStrehovsky for review

jkotas commented 4 months ago

In the XAML compiler, when targeting modern .NET, we need to invoke RuntimeHelpers.RunClassConstructor to make sure that the static constructor for a given type is initialized before some other code runs.

Can you make it explicit by introducing static void Initialize() method on the types that you need to run the static constructors for?

Static constructors are meant to be internal type implementation details. It is an anti-pattern to use them as part of the public type contract.

Sergio0694 commented 4 months ago

I think I remember asking the same a while back and was told that doing so wasn't possible, but I can't recall the exact reason right now (cc. @manodasanW do you remember?). I know at the very least there was a concern about the fact that the type might often be beforefieldinit, because it might just have a bunch of static readonly fields for dependency properties, in which case calling Initialize() wouldn't actually cause them to be initialized, right? Ie.:

public sealed class MyControl : UserControl
{
    public static readonly DependencyProperty FooProperty = DependencyProperty.Register(
        "Foo",
        typeof(int),
        typeof(MyControl),
        null);
}
jkotas commented 4 months ago

You can either generate explicit constructor or you can do a dummy access of one of the static fields in the Initialize method.

public sealed class MyControl : UserControl
{
    public static readonly DependencyProperty FooProperty = DependencyProperty.Register(
        "Foo",
        typeof(int),
        typeof(MyControl),
        null);

    static MyControl()
    {
    }

    public static void Initialize()
    {
    }
}
MichalStrehovsky commented 4 months ago

How does this work for:

public class ControlBase : UserControl
{
    public static readonly DependencyProperty FooProperty = DependencyProperty.Register(
        "Foo",
        typeof(int),
        typeof(MyControl),
        null);
}

public class ControlDerived : ControlBase { }

Does XAML recurse from ControlDerived and call RunClassConstructor on ControlBase? (Because RunClassConstructor on ControlDerived will not run the base cctor.) If so, the proposed extension would likely not help. I would not expect it to keep things recursively.


That said, keeping cctor on a single type should be achievable with the existing API surface, it's just a trimming/AOT mini-feature. DynamicallyAccessedMemberKinds.NonPublicConstructor already includes the static constructor (because that's how reflection APIs are structured as well).

using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;

Run(typeof(Foo));

static void Run([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.NonPublicConstructors)] Type t)
{
    Console.WriteLine(t.GetConstructor(System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static, []));
    RuntimeHelpers.RunClassConstructor(t.TypeHandle);
}

class Foo
{
    static int i = Init();
    private static int Init()
    {
        Console.WriteLine("Hello");
        return 42;
    }
}

This will print

Void .cctor()
Hello

in a trimmed app but there's a trimming warning. The trimming warning should be fixable (was briefly discussed in https://github.com/dotnet/linker/pull/1121#discussion_r412018299).

There's a gotcha however. In native AOT, the static constructor is reflection-visible, but not runnable. So the above program would print:

Void .cctor()

As part of fixing the trimming warning, we'd need to fix this up too (either drop the "static constructor is reflection-visible but not Invoke()-able" blocking and introduce the "I can run the cctor how many times I like" footgun to native AOT as well, or do something else so that RunClassConstructor works.

Sergio0694 commented 4 months ago

"Does XAML recurse from ControlDerived and call RunClassConstructor on ControlBase?"

XAML will track all types, including base types, though I can't say what the exact logic it uses to trigger those constructors is off the top of my head. @manodasanW do you know by any chance if it'll take care of invoking base constructors too?

"In native AOT, the static constructor is reflection-visible, but not runnable."

This... Is the part that I find the most interesting. Just to triple check I'm reading this correctly, are you saying that no RuntimeHelpers.RunClassConstructor call will ever do anything on NativeAOT? Because if so... We do have several WinUI 3 apps running on NativeAOT, like the WinUI Gallery App, and as far as I know there's no reported issues around eg. dependency properties not working. So if it is in fact the case that none of those calls are actually doing anything on NativeAOT, this kinda makes me question whether it is even true that this is actually needed in the first place. I mean something doesn't add up 🤔

MichalStrehovsky commented 4 months ago

This... Is the part that I find the most interesting. Just to triple check I'm reading this correctly, are you saying that no RuntimeHelpers.RunClassConstructor call will ever do anything on NativeAOT

RuntimeHelpers.RunClassConstructor is not implemented on top of reflection. It will work as long as you don't see static analysis warnings.

What doesn't work is calling t.GetConstructor(System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static, [])).Invoke(null, []). That one never worked starting from .NET Native.

MichalStrehovsky commented 4 months ago

It will work as long as you don't see static analysis warnings.

(Suppressing the warning doesn't make it magically work. The suppression in XAML is invalid, including for PublishTrimmed. I rarely see a valid suppression in code outside dotnet/runtime repo. It's pretty much always a bug.)

Sergio0694 commented 4 months ago

Bear with me, I'm not entirely sure I'm following 😅

"It will work as long as you don't see static analysis warnings."

How could you ever not have static analysis warnings for RunClassConstructor, given it's just annotated as [RequiresUnreferencedCode]? Would it not always warn except for the special case with typeof(Foo).TypeHandle?

"What doesn't work is calling t.GetConstructor(System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static, [])).Invoke(null, [])."

That part I get. But in your previous example, unless I'm reading it wrong, you're just calling RunClassConstructor to invoke the constructor, and you said that on NAOT it will not print "Hello", even though you also had that trimming annotation. So like, does this method actually ever work on NAOT or not? And if so, and it only does so under the condition that you have no warning, then I guess this is the same as my previous question: how can you have no warning for a call to it?

"The suppression in XAML is invalid"

Right, so it seems either way we should find some fix before claiming "we officially support AOT". Ignoring the fact that by what you said it would seem like we technically shouldn't be able to say we even officially support trimming on its own then...

MichalStrehovsky commented 4 months ago

How could you ever not have static analysis warnings for RunClassConstructor, given it's just annotated as [RequiresUnreferencedCode]? Would it not always warn except for the special case with typeof(Foo).TypeHandle?

typeof(Foo).TypeHandle is intrinsically recognized and works - that's why there's no warning. Intrinsics are just bonus behaviors that make things better if they kick in. We should probably document them all at some point; they're contractual (removing any would introduce warnings into existing code).

That part I get. But in your previous example, unless I'm reading it wrong, you're just calling RunClassConstructor to invoke the constructor, and you said that on NAOT it will not print "Hello", even though you also had that trimming annotation. So like, does this method actually ever work on NAOT or not?

This is what I meant with "RuntimeHelpers.RunClassConstructor is not implemented on top of reflection" - the NonPublishConstructors annotation will keep the metadata for the cctor, but it's just the metadata and not the method body. And even if we had the method body, RunClassConstructor wouldn't use it because it instead uses the native data structures that are used to trigger the cctor for real. It's an implementation detail, but important to know if one were to suppress the warning (one cannot suppress it due to this implementation detail).

It's fixable, it's just work, same as making sure a suppression is not needed in the first place. The "cctor logic is disconnected from reflection" behavior was inherited from .NET Native where we probably had it due to reflection blocking, but we no longer have reflection blocking and this could be cleaned up. But it's only worth the effort if we're fixing the trimming analysis for RunClassConstructors to also intrinsically accept System.Type that was annotated as NonPublicConstructors. Right now inability to reflection-invoke cctors is just a bullet point at https://github.com/dotnet/runtime/issues/69919.

dotnet-policy-service[bot] commented 4 months ago

Tagging subscribers to this area: @dotnet/area-system-runtime See info in area-owners.md if you want to be subscribed.

dotnet-policy-service[bot] commented 4 months ago

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas See info in area-owners.md if you want to be subscribed.

Sergio0694 commented 3 months ago

"The "cctor logic is disconnected from reflection" behavior was inherited from .NET Native where we probably had it due to reflection blocking, but we no longer have reflection blocking and this could be cleaned up."

@MichalStrehovsky it's kinda interesting, because the System XAML compiler also generates that RunClassConstructor call for all types that are discovered by the XAML compiler. I'm not entirely sure how that even works on .NET Native, is there special logic that causes those special runtime data structures to always be preserved for those types in some other way? Because there's no annotations nor anything obvious that I can see there 🤔

I'm thinking perhaps a good enough way to fix this (at least for now) would be to update the XAML compiler to explicitly pass a delegate for each type, that calls RunClassConstructor(typeof(TheType).TypeHandle). I assume this will bloat binary size a little bit, but then again so would those trim annotations on all those types anyway. And this approach would have the advantage of not requiring .NET 9 (nor any other runtime changes) in order to work. Would this solution make sense?

I guess the real issue is the fact we rely on static constructors in the first place, but that doesn't seem fixable...

MichalStrehovsky commented 3 months ago

@MichalStrehovsky it's kinda interesting, because the System XAML compiler also generates that RunClassConstructor call for all types that are discovered by the XAML compiler. I'm not entirely sure how that even works on .NET Native, is there special logic that causes those special runtime data structures to always be preserved for those types in some other way?

The .NET Native ILC parses XAML, including decompiling and analyzing XBF files. I would not be surprised if this is handled by that, or it only works by pure luck.

I'm thinking perhaps a good enough way to fix this (at least for now) would be to update the XAML compiler to explicitly pass a delegate for each type, that calls RunClassConstructor(typeof(TheType).TypeHandle). I assume this will bloat binary size a little bit, but then again so would those trim annotations on all those types anyway.

That would work.

But also I'm making a fix for .NET 9 (delete a .NET Native leftover around how static constructors are tracked: #103946, fix the analysis not to warn in case the Type was annotated as keeping non-public ctors: #103947). Alternatively, you could:

The NonPublicFields | PublicFields annotation will also keep the magic data structure to support RunClassConstructor, as long as there is at least a single static field on the type (which I assume there is). It's relying on an implementation detail, but only for .NET 8, so it might be okay. It will be a size regression, but the lambda would also be a size regression. Measure if it's acceptable. The problem would go away eventually since we have a fix. I assume we can multitarget this.