Added an override for IsCollectible in RuntimeType to fix inconsistency between Mono and CoreCLR

[Venkad000]

In Mono, calling IsCollectible on a Type was returned True, whereas in CoreCLR it returned False for the same scenario. This discrepancy was caused by a missing override of IsCollectible in the RuntimeType class in Mono.

The discrepancy could also be seen here link

[Venkad000]

@uweigand @giritrivedi @saitama951

[saitama951]

cc: @nealef

[jkotas]

Should this just return false given that collectible assemblies are not implemented in Mono?

This is not a correct implementation in case collectible assemblies were ever implemented in Mono. Correct implementation would have to take into account all generic instantiation arguments.

[jkotas]

Does MethodInfo, etc. have the same bug? If I am reading the code correctly, the following program is going to print true on Mono:

public class Program
{
    public static void Main() { Console.WriteLine(typeof(Program).GetMethod("Main").IsCollectible); }
}

We have a lot of advanced tests for collectible property that are disabled on Mono, but we are missing the basic ones. Could you please add some basic tests
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Reflection/IsCollectibleTests.cs ? Something like:

public class BasicIsCollectibleTests
{
    [Fact]
    public void CoreLib_IsCollectibleFalse
    {
        Assert.False(typeof(object).IsCollectible);
        Assert.False(typeof(object).Assembly.IsCollectible);
        Assert.False(typeof(object).GetMethod("ToString).IsCollectible);
        Assert.False(typeof(object).GetConstructor(Types.Empty).IsCollectible);
        Assert.False(typeof(IntPtr).GetField("Zero").IsCollectible);
        Assert.False(typeof(IntPtr).GetProperty("Zero").IsCollectible);
        Assert.False(typeof(AppDomain).GetEvent("ProcessExit").IsCollectible);
   }
}

[dotnet-policy-service]

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

[Venkad000]

Yes, MethodInfo and PropertyInfo too have differing values. They are true in Mono but are false in CoreCLR.
Unless an assembly is loaded via a custom AssemblyLoadContext with isCollectible: true, it is always going to have IsCollectible as False right?

[jkotas]

Yes, that's correct on CoreCLR.

Mono does not support collectible assemblies. isCollectible: true is ignored on Mono. The easiest way to fix this issue on Mono is to unconditionally return false from IsCollectible properties of runtime provided reflection types.

[Venkad000]

@dotnet-policy-service agree company="IBM"

[Venkad000]

@dotnet-policy-service agree company="IBM"

[jkotas]

It turns out NativeAOT has the same bug. Could you please add overrides bool IsCollectible => false for NativeAOT to make the new test to pass on NativeAOT?

A good place to add them is:

runtime/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/General/ThunkedApis.cs

Lines 90 to 128 in e163124

	internal abstract partial class RuntimeConstructorInfo
	{
	public sealed override MethodImplAttributes GetMethodImplementationFlags() => MethodImplementationFlags;
	// Partial trust doesn't exist in Aot so these legacy apis are meaningless. Will report everything as SecurityCritical by fiat.
	public sealed override bool IsSecurityCritical => true;
	public sealed override bool IsSecuritySafeCritical => false;
	public sealed override bool IsSecurityTransparent => false;
	}
	}
	namespace System.Reflection.Runtime.EventInfos
	{
	internal abstract partial class RuntimeEventInfo
	{
	public sealed override MethodInfo GetAddMethod(bool nonPublic) => AddMethod.FilterAccessor(nonPublic);
	public sealed override MethodInfo GetRemoveMethod(bool nonPublic) => RemoveMethod.FilterAccessor(nonPublic);
	public sealed override MethodInfo GetRaiseMethod(bool nonPublic) => RaiseMethod?.FilterAccessor(nonPublic);
	}
	}
	namespace System.Reflection.Runtime.MethodInfos
	{
	internal abstract partial class RuntimeMethodInfo
	{
	public sealed override MethodImplAttributes GetMethodImplementationFlags() => MethodImplementationFlags;
	public sealed override ICustomAttributeProvider ReturnTypeCustomAttributes => ReturnParameter;
	// Partial trust doesn't exist in Aot so these legacy apis are meaningless. Will report everything as SecurityCritical by fiat.
	public sealed override bool IsSecurityCritical => true;
	public sealed override bool IsSecuritySafeCritical => false;
	public sealed override bool IsSecurityTransparent => false;
	}
	}
	namespace System.Reflection.Runtime.PropertyInfos
	{
	internal abstract partial class RuntimePropertyInfo
	{

[jkotas]

The new test is still failing on NAOT. I have pushed a fix to add missing FieldInfo override for NAOT.

[Venkad000]

Thank you!

[jkotas]

/ba-g Known issue #115955

[jkotas]

Thank you!