Make ComponentModel intrinsic TypeConverters linker-safe (#39973)
authorLayomi Akinrinade <laakinri@microsoft.com>
Fri, 7 Aug 2020 00:35:14 +0000 (17:35 -0700)
committerGitHub <noreply@github.com>
Fri, 7 Aug 2020 00:35:14 +0000 (17:35 -0700)
* Make ComponentModel intrinsic TypeConverters linker-safe

* Avoid reflection in converter func lookup

* Re-add code path that supports editors

* Consolidate intrinsic converter caches, move converter-specific code to new method, add trimming tests

* Address feedback

* Revert change to GetIntrinsicTypeEditor method

* Handle inheritance with Uri and CultureInfo types

* Add heirarchy-related comments to cache docs

src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.ReflectedTypeData.cs
src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs
src/libraries/System.ComponentModel.TypeConverter/tests/TrimmingTests/TypeConverterTest.cs [new file with mode: 0644]
src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs

index d2b7d59..70c8024 100644 (file)
@@ -218,7 +218,7 @@ namespace System.ComponentModel
                     {
                         // 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);
+                        _converter = GetIntrinsicTypeConverter(_type);
                         Debug.Assert(_converter != null, "There is no intrinsic setup in the hashtable for the Object type");
                     }
                 }
@@ -348,7 +348,7 @@ namespace System.ComponentModel
                     Hashtable intrinsicEditors = GetEditorTable(editorBaseType);
                     if (intrinsicEditors != null)
                     {
-                        editor = SearchIntrinsicTable(intrinsicEditors, _type);
+                        editor = GetIntrinsicTypeEditor(intrinsicEditors, _type);
                     }
 
                     // As a quick sanity check, check to see that the editor we got back is of
index 5423539..ff4cd11 100644 (file)
@@ -5,7 +5,6 @@ using System.Collections;
 using System.Collections.Generic;
 using System.ComponentModel.Design;
 using System.Diagnostics;
-using System.Drawing;
 using System.Globalization;
 using System.Linq;
 using System.Reflection;
@@ -38,7 +37,7 @@ namespace System.ComponentModel
         // This is where we store the various converters, etc for the intrinsic types.
         //
         private static Hashtable s_editorTables;
-        private static Hashtable s_intrinsicTypeConverters;
+        private static Dictionary<object, IntrinsicTypeConverterData> s_intrinsicTypeConverters;
 
         // For converters, etc that are bound to class attribute data, rather than a class
         // type, we have special key sentinel values that we put into the hash table.
@@ -94,45 +93,88 @@ namespace System.ComponentModel
         private static Hashtable EditorTables => LazyInitializer.EnsureInitialized(ref s_editorTables, () => new Hashtable(4));
 
         /// <summary>
-        /// This is a table we create for intrinsic types.
-        /// There should be entries here ONLY for intrinsic
-        /// types, as all other types we should be able to
-        /// add attributes directly as metadata.
+        /// Provides a way to create <see cref="TypeConverter"/> instances, and cache them where applicable.
         /// </summary>
-        private static Hashtable IntrinsicTypeConverters => LazyInitializer.EnsureInitialized(ref s_intrinsicTypeConverters, () => new Hashtable
+        private class IntrinsicTypeConverterData
+        {
+            private readonly Func<Type, TypeConverter> _constructionFunc;
+
+            private readonly bool _cacheConverterInstance;
+
+            private TypeConverter _converterInstance;
+
+            /// <summary>
+            /// Creates a new instance of <see cref="IntrinsicTypeConverterData"/>.
+            /// </summary>
+            /// <param name="constructionFunc">
+            /// A func that creates a new <see cref="TypeConverter"/> instance.
+            /// </param>
+            /// <param name="cacheConverterInstance">
+            /// Indicates whether to cache created <see cref="TypeConverter"/> instances. This is false when the converter handles multiple types,
+            /// specifically <see cref="EnumConverter"/>, <see cref="NullableConverter"/>, and <see cref="ReferenceConverter"/>.
+            /// </param>
+            public IntrinsicTypeConverterData(Func<Type, TypeConverter> constructionFunc, bool cacheConverterInstance = true)
+            {
+                _constructionFunc = constructionFunc;
+                _cacheConverterInstance = cacheConverterInstance;
+            }
+
+            public TypeConverter GetOrCreateConverterInstance(Type innerType)
+            {
+                if (!_cacheConverterInstance)
+                {
+                    return _constructionFunc(innerType);
+                }
+
+                _converterInstance ??= _constructionFunc(innerType);
+                return _converterInstance;
+            }
+        }
+
+        /// <summary>
+        /// This is a table we create for intrinsic types.There should be entries here ONLY for
+        /// intrinsic types, as all other types we should be able to add attributes directly as metadata.
+        /// </summary>
+        /// <remarks>
+        /// <see cref="Uri"/> and <see cref="CultureInfo"/> are the only types that can be inherited for which
+        /// we have intrinsic converters for. The appropriate converter needs to be fetched when look-ups are done
+        /// for deriving types. When adding to this cache, consider whether the type can be inherited and add the
+        /// appropriate logic to handle this in <see cref="GetIntrinsicTypeConverter(Type)"/> below.
+        /// </remarks>
+        private static Dictionary<object, IntrinsicTypeConverterData> IntrinsicTypeConverters
+            => LazyInitializer.EnsureInitialized(ref s_intrinsicTypeConverters, () => new Dictionary<object, IntrinsicTypeConverterData>(27)
         {
             // 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),
+            [typeof(bool)] = new IntrinsicTypeConverterData((type) => new BooleanConverter()),
+            [typeof(byte)] = new IntrinsicTypeConverterData((type) => new ByteConverter()),
+            [typeof(sbyte)] = new IntrinsicTypeConverterData((type) => new SByteConverter()),
+            [typeof(char)] = new IntrinsicTypeConverterData((type) => new CharConverter()),
+            [typeof(double)] = new IntrinsicTypeConverterData((type) => new DoubleConverter()),
+            [typeof(string)] = new IntrinsicTypeConverterData((type) => new StringConverter()),
+            [typeof(int)] = new IntrinsicTypeConverterData((type) => new Int32Converter()),
+            [typeof(short)] = new IntrinsicTypeConverterData((type) => new Int16Converter()),
+            [typeof(long)] = new IntrinsicTypeConverterData((type) => new Int64Converter()),
+            [typeof(float)] = new IntrinsicTypeConverterData((type) => new SingleConverter()),
+            [typeof(ushort)] = new IntrinsicTypeConverterData((type) => new UInt16Converter()),
+            [typeof(uint)] = new IntrinsicTypeConverterData((type) => new UInt32Converter()),
+            [typeof(ulong)] = new IntrinsicTypeConverterData((type) => new UInt64Converter()),
+            [typeof(object)] = new IntrinsicTypeConverterData((type) => new TypeConverter()),
+            [typeof(CultureInfo)] = new IntrinsicTypeConverterData((type) => new CultureInfoConverter()),
+            [typeof(DateTime)] = new IntrinsicTypeConverterData((type) => new DateTimeConverter()),
+            [typeof(DateTimeOffset)] = new IntrinsicTypeConverterData((type) => new DateTimeOffsetConverter()),
+            [typeof(decimal)] = new IntrinsicTypeConverterData((type) => new DecimalConverter()),
+            [typeof(TimeSpan)] = new IntrinsicTypeConverterData((type) => new TimeSpanConverter()),
+            [typeof(Guid)] = new IntrinsicTypeConverterData((type) => new GuidConverter()),
+            [typeof(Uri)] = new IntrinsicTypeConverterData((type) => new UriTypeConverter()),
+            [typeof(Version)] = new IntrinsicTypeConverterData((type) => new 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),
-            [s_intrinsicReferenceKey] = typeof(ReferenceConverter),
+            [typeof(Array)] = new IntrinsicTypeConverterData((type) => new ArrayConverter()),
+            [typeof(ICollection)] = new IntrinsicTypeConverterData((type) => new CollectionConverter()),
+            [typeof(Enum)] = new IntrinsicTypeConverterData((type) => new EnumConverter(type), cacheConverterInstance: false),
+            [s_intrinsicNullableKey] = new IntrinsicTypeConverterData((type) => new NullableConverter(type), cacheConverterInstance: false),
+            [s_intrinsicReferenceKey] = new IntrinsicTypeConverterData((type) => new ReferenceConverter(type), cacheConverterInstance: false),
         });
 
         private static Hashtable PropertyCache => LazyInitializer.EnsureInitialized(ref s_propertyCache, () => new Hashtable());
@@ -1216,14 +1258,14 @@ namespace System.ComponentModel
 
         /// <summary>
         /// Searches the provided intrinsic hashtable for a match with the object type.
-        /// At the beginning, the hashtable contains types for the various converters.
+        /// At the beginning, the hashtable contains types for the various editors.
         /// As this table is searched, the types for these objects
         /// are replaced with instances, so we only create as needed. This method
         /// does the search up the base class hierarchy and will create instances
         /// for types as needed. These instances are stored back into the table
         /// for the base type, and for the original component type, for fast access.
         /// </summary>
-        private static object SearchIntrinsicTable(Hashtable table, Type callingType)
+        private static object GetIntrinsicTypeEditor(Hashtable table, Type callingType)
         {
             object hashEntry = null;
 
@@ -1338,5 +1380,76 @@ namespace System.ComponentModel
 
             return hashEntry;
         }
+
+        /// <summary>
+        /// Searches the intrinsic converter dictionary for a match with the object type.
+        /// The strongly-typed dictionary maps object types to converter data objects which lazily
+        /// creates (and caches for re-use, where applicable) converter instances.
+        /// </summary>
+        private static TypeConverter GetIntrinsicTypeConverter(Type callingType)
+        {
+            TypeConverter converter;
+
+            // We take a lock on this dictionary. Nothing in this code calls out to
+            // other methods that lock, so it should be fairly safe to grab this lock.
+            lock (IntrinsicTypeConverters)
+            {
+                if (!IntrinsicTypeConverters.TryGetValue(callingType, out IntrinsicTypeConverterData converterData))
+                {
+                    if (callingType.IsEnum)
+                    {
+                        converterData = IntrinsicTypeConverters[typeof(Enum)];
+                    }
+                    else if (callingType.IsArray)
+                    {
+                        converterData = IntrinsicTypeConverters[typeof(Array)];
+                    }
+                    else if (callingType.IsGenericType && callingType.GetGenericTypeDefinition() == typeof(Nullable<>))
+                    {
+                        converterData = IntrinsicTypeConverters[s_intrinsicNullableKey];
+                    }
+                    else if (typeof(ICollection).IsAssignableFrom(callingType))
+                    {
+                        converterData = IntrinsicTypeConverters[typeof(ICollection)];
+                    }
+                    else if (callingType.IsInterface)
+                    {
+                        converterData = IntrinsicTypeConverters[s_intrinsicReferenceKey];
+                    }
+                    else
+                    {
+                        // Uri and CultureInfo are the only types that can be derived from for which we have intrinsic converters.
+                        // Check if the calling type derives from either and return the appropriate converter.
+
+                        // We should have fetched converters for these types above.
+                        Debug.Assert(callingType != typeof(Uri) && callingType != typeof(CultureInfo));
+
+                        Type key = null;
+
+                        Type baseType = callingType.BaseType;
+                        while (baseType != null && baseType != typeof(object))
+                        {
+                            if (baseType == typeof(Uri) || baseType == typeof(CultureInfo))
+                            {
+                                key = baseType;
+                                break;
+                            }
+
+                            baseType = baseType.BaseType;
+                        }
+
+                        // Handle other reference and value types. An instance of TypeConverter itself created and returned below.
+                        key ??= typeof(object);
+
+                        converterData = IntrinsicTypeConverters[key];
+                    }
+                }
+
+                // This needs to be done within the lock as the dictionary value may be mutated in the following method call.
+                converter = converterData.GetOrCreateConverterInstance(callingType);
+            }
+
+            return converter;
+        }
     }
 }
diff --git a/src/libraries/System.ComponentModel.TypeConverter/tests/TrimmingTests/TypeConverterTest.cs b/src/libraries/System.ComponentModel.TypeConverter/tests/TrimmingTests/TypeConverterTest.cs
new file mode 100644 (file)
index 0000000..dca3536
--- /dev/null
@@ -0,0 +1,182 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Collections;
+using System.ComponentModel;
+using System.Globalization;
+
+/// <summary>
+/// Tests that the relevant constructors of intrinsic TypeConverter types are preserved when needed in a trimmed application.
+/// </summary>
+class Program
+{
+    static int Main(string[] args)
+    {
+        if (!RunTest(targetType: typeof(bool), expectedConverterType: typeof(BooleanConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(byte), expectedConverterType: typeof(ByteConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(sbyte), expectedConverterType: typeof(SByteConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(char), expectedConverterType: typeof(CharConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(double), expectedConverterType: typeof(DoubleConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(string), expectedConverterType: typeof(StringConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(short), expectedConverterType: typeof(Int16Converter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(int), expectedConverterType: typeof(Int32Converter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(long), expectedConverterType: typeof(Int64Converter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(float), expectedConverterType: typeof(SingleConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(ushort), expectedConverterType: typeof(UInt16Converter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(uint), expectedConverterType: typeof(UInt32Converter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(ulong), expectedConverterType: typeof(UInt64Converter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(object), expectedConverterType: typeof(TypeConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(DateTime), expectedConverterType: typeof(DateTimeConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(DateTimeOffset), expectedConverterType: typeof(DateTimeOffsetConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(decimal), expectedConverterType: typeof(DecimalConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(TimeSpan), expectedConverterType: typeof(TimeSpanConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(Guid), expectedConverterType: typeof(GuidConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(Array), expectedConverterType: typeof(ArrayConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(ICollection), expectedConverterType: typeof(CollectionConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(Enum), expectedConverterType: typeof(EnumConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(DayOfWeek), expectedConverterType: typeof(EnumConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(SomeValueType?), expectedConverterType: typeof(NullableConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(ClassWithNoConverter), expectedConverterType: typeof(TypeConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(Uri), expectedConverterType: typeof(UriTypeConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(CultureInfo), expectedConverterType: typeof(CultureInfoConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(Version), expectedConverterType: typeof(VersionConverter)))
+        {
+            return -1;
+        }
+
+        if (!RunTest(targetType: typeof(IFooComponent), expectedConverterType: typeof(ReferenceConverter)))
+        {
+            return -1;
+        }
+
+        return 100;
+    }
+
+    private static bool RunTest(Type targetType, Type expectedConverterType)
+    {
+        TypeConverter retrievedConverter = TypeDescriptor.GetConverter(targetType);
+        return retrievedConverter.GetType() == expectedConverterType && retrievedConverter.CanConvertTo(typeof(string));
+    }
+
+    private struct SomeValueType
+    {
+    }
+
+    // TypeDescriptor should default to the TypeConverter in this case.
+    private class ClassWithNoConverter
+    {
+    }
+
+    private interface IFooComponent
+    {
+    }
+}
index 16c67b7..39a37f6 100644 (file)
@@ -469,7 +469,9 @@ namespace System.ComponentModel.Tests
         [InlineData(typeof(TimeSpan), typeof(TimeSpanConverter))]
         [InlineData(typeof(Guid), typeof(GuidConverter))]
         [InlineData(typeof(Array), typeof(ArrayConverter))]
+        [InlineData(typeof(int[]), typeof(ArrayConverter))]
         [InlineData(typeof(ICollection), typeof(CollectionConverter))]
+        [InlineData(typeof(Stack), typeof(CollectionConverter))]
         [InlineData(typeof(Enum), typeof(EnumConverter))]
         [InlineData(typeof(SomeEnum), typeof(EnumConverter))]
         [InlineData(typeof(SomeValueType?), typeof(NullableConverter))]
@@ -482,7 +484,11 @@ namespace System.ComponentModel.Tests
         [InlineData(typeof(ClassIBase), typeof(IBaseConverter))]
         [InlineData(typeof(ClassIDerived), typeof(IBaseConverter))]
         [InlineData(typeof(Uri), typeof(UriTypeConverter))]
+        [InlineData(typeof(DerivedUri), typeof(UriTypeConverter))]
+        [InlineData(typeof(TwiceDerivedUri), typeof(UriTypeConverter))]
         [InlineData(typeof(CultureInfo), typeof(CultureInfoConverter))]
+        [InlineData(typeof(DerivedCultureInfo), typeof(CultureInfoConverter))]
+        [InlineData(typeof(TwiceDerivedCultureInfo), typeof(CultureInfoConverter))]
         [InlineData(typeof(Version), typeof(VersionConverter))]
         [InlineData(typeof(IComponent), typeof(ComponentConverter))]
         [InlineData(typeof(IFooComponent), typeof(ReferenceConverter))]
@@ -1157,5 +1163,27 @@ namespace System.ComponentModel.Tests
         {
             bool Flag { get; set; }
         }
+
+        class DerivedUri : Uri
+        {
+            protected DerivedUri() : base("https://hello")
+            {
+            }
+        }
+
+        class TwiceDerivedUri : DerivedUri
+        {
+        }
+
+        class DerivedCultureInfo : CultureInfo
+        {
+            protected DerivedCultureInfo() : base("hello")
+            {
+            }
+        }
+
+        class TwiceDerivedCultureInfo : DerivedCultureInfo
+        {
+        }
     }
 }