ComponentModel IntrinsicTypeConverters are not linker safe

[eerhardt]

Calling TypeDescriptor.GetConverter(typeof(Guid)), or passing other low-level system types, in a trimmed app can cause failures because we are using reflection to create the object.

Underneath, we have a map of Type => Converter Type:

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs

Lines 103 to 136 in 4f9ae42

	private static Hashtable IntrinsicTypeConverters => LazyInitializer.EnsureInitialized(ref s_intrinsicTypeConverters, () => new Hashtable
	{
	// Add the intrinsics
	//
	[typeof(bool)] = typeof(BooleanConverter),
	[typeof(byte)] = typeof(ByteConverter),
	[typeof(sbyte)] = typeof(SByteConverter),
	[typeof(char)] = typeof(CharConverter),
	[typeof(double)] = typeof(DoubleConverter),
	[typeof(string)] = typeof(StringConverter),
	[typeof(int)] = typeof(Int32Converter),
	[typeof(short)] = typeof(Int16Converter),
	[typeof(long)] = typeof(Int64Converter),
	[typeof(float)] = typeof(SingleConverter),
	[typeof(ushort)] = typeof(UInt16Converter),
	[typeof(uint)] = typeof(UInt32Converter),
	[typeof(ulong)] = typeof(UInt64Converter),
	[typeof(object)] = typeof(TypeConverter),
	[typeof(void)] = typeof(TypeConverter),
	[typeof(CultureInfo)] = typeof(CultureInfoConverter),
	[typeof(DateTime)] = typeof(DateTimeConverter),
	[typeof(DateTimeOffset)] = typeof(DateTimeOffsetConverter),
	[typeof(decimal)] = typeof(DecimalConverter),
	[typeof(TimeSpan)] = typeof(TimeSpanConverter),
	[typeof(Guid)] = typeof(GuidConverter),
	[typeof(Uri)] = typeof(UriTypeConverter),
	[typeof(Version)] = typeof(VersionConverter),
	// Special cases for things that are not bound to a specific type
	//
	[typeof(Array)] = typeof(ArrayConverter),
	[typeof(ICollection)] = typeof(CollectionConverter),
	[typeof(Enum)] = typeof(EnumConverter),
	[s_intrinsicNullableKey] = typeof(NullableConverter),
	});

And then we use that Converter Type and create an instance of it using reflection:

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.ReflectedTypeData.cs

Lines 220 to 222 in 4f9ae42

	// We did not get a converter. Traverse up the base class chain until
	// we find one in the stock hashtable.
	_converter = (TypeConverter)SearchIntrinsicTable(IntrinsicTypeConverters, _type);

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs

Lines 1322 to 1336 in 4f9ae42

	// If the entry is a type, create an instance of it and then
	// replace the entry. This way we only need to create once.
	// We can only do this if the object doesn't want a type
	// in its constructor.
	//
	Type type = hashEntry as Type;
	if (type != null)
	{
	hashEntry = CreateInstance(type, callingType);
	if (type.GetConstructor(s_typeConstructor) == null)
	{
	table[callingType] = hashEntry;
	}
	}

However, the linker cannot tell that it is supposed to preserve the constructor on these converters because it can't see through adding types to a collection.

Blazor is currently working around this issue here: https://github.com/dotnet/aspnetcore/blob/e906c2067be5552bf15f0bd2a9c793a6ad4d3dc6/src/Razor/Microsoft.NET.Sdk.Razor/src/build/netstandard2.0/LinkerWorkaround.xml#L11-L13

See related dotnet/aspnetcore#23262

[ghost]

Tagging subscribers to this area: @safern
Notify danmosemsft if you want to be subscribed.

[MichalStrehovsky]

I've been thinking about these kinds of patterns in the past. I think the safest way to approach this is through a wrapper class. Something like:

struct ReflectableHashtable
{
    private Hashtable _table;

    public void Add(Type x, [DynamicallyAccessed(X)] Type y) => _table.Add(x, y);

    [UnconditionalSuppressMessage(blah)]
    [DynamicallyAccessed(X)]
    public this[Type k] => _table[k];
}

Linker cannot fully see through this, so it needs a suppression, but it's a small struct and it's easy to audit that it is in fact safe.

[eerhardt]

That's a good idea, I'll see if I can get that to work.

In mono, it looks like it was annotated using PreserveDependency to preserve the constructors:

https://github.com/mono/mono/blob/be6f43c80e88ec0613c35714bac9c110d4012acb/mcs/class/referencesource/System/compmod/system/componentmodel/ReflectTypeDescriptionProvider.cs#L109-L136

Which can easily get out of sync.

One other option I was playing around with was to use a Func<T>, or similar, to create the objects, so that way the code just uses the constructors directly without reflection.

[ericstj]

@safern is contributing #39854 which will need to be included as well.