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

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

Closed
Sergio0694 opened this issue Jun 19, 2024 · 17 comments · Fixed by #103947
Closed

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

Sergio0694 opened this issue Jun 19, 2024 · 17 comments · Fixed by #103947
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-NativeAOT-coreclr linkable-framework Issues associated with delivering a linker friendly framework
Milestone

Comments

@Sergio0694
Copy link
Contributor

Sergio0694 commented Jun 19, 2024

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);
}
@Sergio0694 Sergio0694 added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Reflection linkable-framework Issues associated with delivering a linker friendly framework labels Jun 19, 2024
Copy link
Contributor

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 dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jun 19, 2024
Copy link
Contributor

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

@teo-tsirpanis
Copy link
Contributor

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

agocke commented Jun 19, 2024

@sbomer and @MichalStrehovsky for review

@jkotas
Copy link
Member

jkotas commented Jun 19, 2024

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

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

jkotas commented Jun 19, 2024

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

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 dotnet/linker#1121 (comment)).

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

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

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

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

Sergio0694 commented Jun 19, 2024

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

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 #69919.

Copy link
Contributor

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

Copy link
Contributor

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

@agocke agocke added this to the Future milestone Jun 20, 2024
@Sergio0694
Copy link
Contributor Author

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

@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.

@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Jun 26, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-NativeAOT-coreclr linkable-framework Issues associated with delivering a linker friendly framework
Projects
Archived in project
6 participants