@vitek-karas Here is the intrinsic change we discussed on discord

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

It does not look right to me to mark the type as instantiated just because it shows up as a return type of an intrinsic.

Should the GetType in Mono be marked as having a dependency on RuntimeType constructor using DynamicDependencyAttribute instead?

This problem doesn't happen with a normal CoreCLR .NET 8 BCL.

The correctness of this and similar patterns in CoreCLR CoreLib is guaranteed by rooting all types in https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/corelib.h . For example, this line https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/corelib.h#L160 guarantees that RuntimeType is considered instantiated by the trimmer.

@jkotas A little more context here https://discord.com/channels/732297728826277939/751137004007456849/1349389472545898497

Should the GetType in Mono be marked as having a dependency on RuntimeType constructor using DynamicDependencyAttribute instead?

Yes it could and that would fix this issue as well.

It does not look right to me to mark the type as instantiated just because it shows up as a return type of an intrinsic.

What is your concern with this approach? It changes the handling of 1 method across the CoreCLR BCL and Mono BCL. What is the value in not doing this? Using DynamicDependencyAttribute to mark the ctor will lead to a mark as instantiated anyways. An instance of the returned type can be created without invoking an instance constructor, this approach covers that scenario.

I don't think the instantiation optimization is worth attempting to retain on types that could (and probably are in most cases) created on the native side.

Finding this bug was incredibly obscure. I got lucky that an existing test combine with the mono bcl assemblies caught it.

Maybe if the trimming analyzer issued warnings on all [Intrinsic] methods with the recursive self calling pattern and force them to declare a dependency. I guess that could work and prevent future issues. Is that worth the effort?

Here's a PR for a similar issue #113437

System.Type is abstract type, keeping such ctor for Type alone should not make this scenario fully working and you are probably relying on some other assumption somewhere else. Using DynamicDependencyAttribute for correct type looks to me as cleaner solution as well.

What is your concern with this approach?

• It is not the correct model. We use the Intrinsic attribute liberally. With this change, we need to worry about Intrinsic attribute rooting additional methods unnecessarily.
• It is not a complete fix for the issue as @marek-safar pointed out.

Maybe if the trimming analyzer issued warnings on all [Intrinsic] methods with the recursive self calling pattern and force them to declare a dependency.

Vast majority of [Intrinsic] methods do not have any extra type decencies. I do not think this would help since people would just copy&paste the boiler plate dummy dependency everywhere, and the few cases that need it would miss it anyway.

A correct by construction solution would be to require references to types referenced by the VM to be captured on the managed side in some form and stop calling Type.GetType(string) equivalent in the VM. For example, the intrinsic expansion of a method would be only allowed to reference types and methods that are referenced in the DynamicDependencyAttributes on the instrinsic method.

@jkotas @marek-safar What are your thoughts on #113437 ? Should that change happen?

Here's some data. In the 10.0.0-preview.1.25080.5 System.Private.CoreLib.dll I'm seeing 2633 methods with the [Intrinsic] attribute. Disclaimer, I didn't check out parameters, I don't think it would dramatically change the theme.

2620 of those have a value type as the result return type. ValueTypes are always marked as instantiated so this PR would have zero impact on those methods.

That leaves 13 methods that could see a behavior change from this PR. Of those 13 methods, 4 are returning System.Object .
System.Object is always going to be marked as instantiated for a variety of reasons, but the first reason is because ILLink.Descriptors.xml will preserve the .ctor().

Now we are down to 9 methods. The next return type is System.Collections.Generic.IEnumerator1` which is returned by 2 methods. Interfaces are always marked instantiated so again, no behavior change for these.

Now we are down to 7 methods. The remaining methods are

    ReturnType : System.Type - 4 
        System.Type System.Object::GetType()
        System.Type System.Type::GetTypeFromHandle(System.RuntimeTypeHandle)
        System.Type System.Type::GetGenericTypeDefinition()
        System.Type System.Type::GetEnumUnderlyingType()
    ReturnType : System.Threading.Thread - 1 
        System.Threading.Thread System.Threading.Thread::get_CurrentThread()
    ReturnType : System.Collections.Generic.Comparer`1 - 1 
        System.Collections.Generic.Comparer`1<T> System.Collections.Generic.Comparer`1::get_Default()
    ReturnType : System.Collections.Generic.EqualityComparer`1 - 1 
        System.Collections.Generic.EqualityComparer`1<T> System.Collections.Generic.EqualityComparer`1::get_Default()

Why is it invalid for the linker to assume an instance of these return types could exist when these 7 methods are used?

I see the argument for adding DynamicDependencyAttributes. That change is probably necessary for correctness in most cases. Although it seems possible the native code could do the equivalent of RuntimeHelpers.GetUninitializedObject and create an instance of a type without ever calling an instance constructor. In that case DynamicDependencyAttributes would not technically be necessary but the linker would still need to know that an instance of the type could exist.

I'm struggling to understand why the not instantiated optimization is worth trying to maintain for methods that will execute code that is different from the IL that the linker is able to see. Incorrectly applying the not instantiated optimizations does not lead to simple runtime problems like MissingMethodExceptions. Incorrectly applying the not instantiated optimizations can lead to obscure runtime behavior changes. I don't think it's worth the risk to apply not instantiated optimizations to types coming out of methods to which the linker does not have visibility into the actual method body.

That change is probably necessary for correctness in most cases.

Right, 6 out of 7 methods in your list return more specialized type instance and this PR is not sufficient to make them correct.

Right, 6 out of 7 methods in your list return more specialized type instance and this PR is not sufficient to make them correct.

You are correct. This PR will not make them correct. I will open a separate PR to add the DynamicDependencyAttribute.

What this PR does is reduce the risk of missed dependencies causing obscure problems. And slightly unifies intrinsics and icalls/pinvoke so that there is less of a behavior difference.

What this PR does is reduce the risk of missed dependencies causing obscure problems. And slightly unifies intrinsics and icalls/pinvoke so that there is less of a behavior difference.

Don't we want this to cause problems? That is, it is a top-level goal for trimming to not "over-approximate" the app in order to make things work. That doesn't result in long-term customer success, it just hides bugs. It seems like the aggressive trimming in this case has actually caught a bug that will now be fixed, and everything will be happy.

Don't we want this to cause problems?

@agocke I want consistency more than anything between the two variations of GetType in the description. I'm happy with both holding together or both causing the same error.

That is, it is a top-level goal for trimming to not "over-approximate" the app in order to make things work.

This is a niche corner behavior where some precautions are already taken. For example here and here. Now that I look at AlwaysMarkTypeAsInstantiated that is sort of interesting. It's going to return true for System.RuntimeType.

You are correct, that is normally the rational that is followed. And we could follow that here. If we did, I'd argue we should also remove the MarkDefaultConstructor calls in my other PR. I didn't propose that as a solution because I wasn't sure what the appetite would be for changing an existing behavior. Would you agree that MarkDefaultConstructor call is also over-approximating? If so, are you comfortable with removing it?

Status update. Keep in mind I'm using UnityLinker + net7 ILLink (from the dotnet/linker fork) + net8 mono BCL.

1. I found

   runtime/src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/NullableAnnotations.cs

   Line 71 in 1fb6148

   PreserveSystemType ();

   which traces back to this change

Which is doing

		[Kept]
		static void PreserveSystemType ()
		{
			typeof (Type).RequiresNonPublicConstructors ();
		}

The net7 illink code doesn't have this change. So this problem was encountered before and the test was adjusted to make the failure go away.

2. I have been trying the DynamicDependency fix in our mono BCL and it's not working. The problem still happens with

        [DynamicDependency("#ctor()", typeof(RuntimeType))]
        [Intrinsic]
        public Type GetType() => GetType();

However, the following does fix the problem.

        [DynamicDependency("#ctor()", typeof(Type))]
        [Intrinsic]
        public Type GetType() => GetType();

3. Using ILLink from main. If I hack out PreserveSystemType from the NullableAnnoations test and hack ProcessInteropMethod to ignore System.Object.GetType(). I get the same failure that I see within the unity setup. Keep in mind System.RuntimeType is always marked instantiated due to AlwaysMarkTypeAsInstantiated in main. The net7 ILLink doesn't have this change. If I hack AlwaysMarkTypeAsInstantiated to return true for System.Type on main, then the test starts passing again. Which is good. It means the behavior is consistent with (2).

The findings from (2) and (3) lead me to believe that just because a derived type is marked as instantiated doesn't mean the base type will be. That seems like a bug if true. I'll put together a test to more explicitly verify whether or not that is happening.

I'm going to close this PR. I will address #113437 (comment) first.

I will circle back later and try to remove PreserveSystemType and fix the BCL. I need to do more digging to figure out why [DynamicDependency("#ctor()", typeof(RuntimeType))] didn't fix the issue.