Addressed codereview feedback

Author: alexey-zakharov

src/libraries/System.ComponentModel.TypeConverter/System.ComponentModel.TypeConverter.slnx

@@ -265,6 +265,11 @@
     </Project>
     <Project Path="tests/UnloadableTestTypes/UnloadableTestTypes.csproj">
       <BuildType Solution="Checked|*" Project="Release" />
+      <Build Solution="*|arm" Project="false" />
+      <Build Solution="*|arm64" Project="false" />
+      <Build Solution="Checked|Any CPU" Project="false" />
+      <Build Solution="Checked|x64" Project="false" />
+      <Build Solution="Checked|x86" Project="false" />
     </Project>
   </Folder>
   <Folder Name="/tools/" />

src/libraries/System.ComponentModel.TypeConverter/src/System.ComponentModel.TypeConverter.csproj

@@ -15,7 +15,7 @@
     <Compile Include="System\ComponentModel\ByteConverter.cs" />
     <Compile Include="System\ComponentModel\CharConverter.cs" />
     <Compile Include="System\ComponentModel\CollectionConverter.cs" />
-    <Compile Include="System\ComponentModel\ContextAwareConcurrentHashtable.cs" />
+    <Compile Include="System\ComponentModel\CollectibleKeyConcurrentHashtable.cs" />
     <Compile Include="System\ComponentModel\DateOnlyConverter.cs" />
     <Compile Include="System\ComponentModel\DateTimeConverter.cs" />
     <Compile Include="System\ComponentModel\DateTimeOffsetConverter.cs" />

src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ContextAwareConcurrentHashtable.cs renamed to src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/CollectibleKeyConcurrentHashtable.cs

@@ -15,7 +15,7 @@ namespace System.ComponentModel
     /// Uses ConditionalWeakTable for the collectible keys (if MemberInfo.IsCollectible is true) and
     /// ConcurrentDictionary for non-collectible keys.
     /// </summary>
-    internal sealed class ContextAwareConcurrentHashtable<TKey, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] TValue> : IEnumerable<KeyValuePair<TKey, TValue>>
+    internal sealed class CollectibleKeyConcurrentHashtable<TKey, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] TValue> : IEnumerable<KeyValuePair<TKey, TValue>>
         where TKey : MemberInfo
         where TValue : class?
     {
@@ -37,12 +37,12 @@ public TValue? this[TKey key]
                 }
                 else
                 {
-                    _collectibleTable.AddOrUpdate(key, value!);
+                    _collectibleTable.AddOrUpdate(key, value);
                 }
             }
         }
 
-        public bool Contains(TKey key)
+        public bool ContainsKey(TKey key)
         {
             return !key.IsCollectible ? _defaultTable.ContainsKey(key) : _collectibleTable.TryGetValue(key, out _);
         }
@@ -130,6 +130,7 @@ public void Reset()
             {
                 _defaultEnumerator.Reset();
                 _collectibleEnumerator.Reset();
+                _enumeratingCollectibleEnumerator = false;
                 Current = default;
             }
         }

src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ContextAwareHashtable.cs

@@ -15,7 +15,7 @@ namespace System.ComponentModel
     internal sealed class ContextAwareHashtable
     {
         private readonly Hashtable _defaultTable = new Hashtable();
-        private readonly ConditionalWeakTable<object, object> _collectibleTable = new ConditionalWeakTable<object, object>();
+        private readonly ConditionalWeakTable<object, object?> _collectibleTable = new ConditionalWeakTable<object, object?>();
 
         public object? this[MemberInfo key]
         {
@@ -28,11 +28,11 @@ public object? this[MemberInfo key]
             {
                 if (!key.IsCollectible)
                 {
-                    _defaultTable[key] = value!;
+                    _defaultTable[key] = value;
                 }
                 else
                 {
-                    _collectibleTable.AddOrUpdate(key, value!);
+                    _collectibleTable.AddOrUpdate(key, value);
                 }
             }
         }

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

@@ -23,7 +23,7 @@ namespace System.ComponentModel
     internal sealed partial class ReflectTypeDescriptionProvider : TypeDescriptionProvider
     {
         // ReflectedTypeData contains all of the type information we have gathered for a given type.
-        private readonly ContextAwareConcurrentHashtable<Type, ReflectedTypeData> _typeData = new ContextAwareConcurrentHashtable<Type, ReflectedTypeData>();
+        private readonly CollectibleKeyConcurrentHashtable<Type, ReflectedTypeData> _typeData = new CollectibleKeyConcurrentHashtable<Type, ReflectedTypeData>();
 
         // This is the signature we look for when creating types that are generic, but
         // want to know what type they are dealing with. Enums are a good example of this;
@@ -981,14 +981,14 @@ private ReflectedTypeData GetTypeDataFromRegisteredType(Type type)
         {
             Type componentType = typeof(T);
 
-            if (_typeData.Contains(componentType))
+            if (_typeData.ContainsKey(componentType))
             {
                 return;
             }
 
             lock (TypeDescriptor.s_commonSyncObject)
             {
-                if (_typeData.Contains(componentType))
+                if (_typeData.ContainsKey(componentType))
                 {
                     return;
                 }

src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs

@@ -65,12 +65,12 @@ public sealed class TypeDescriptor
         internal static readonly object s_commonSyncObject = new object();
 
         // A direct mapping from type to provider.
-        private static readonly ContextAwareConcurrentHashtable<Type, TypeDescriptionNode> s_providerTypeTable = new ContextAwareConcurrentHashtable<Type, TypeDescriptionNode>();
+        private static readonly CollectibleKeyConcurrentHashtable<Type, TypeDescriptionNode> s_providerTypeTable = new CollectibleKeyConcurrentHashtable<Type, TypeDescriptionNode>();
 
         // Tracks DefaultTypeDescriptionProviderAttributes.
         // A value of `null` indicates initialization is in progress.
         // A value of s_initializedDefaultProvider indicates the provider is initialized.
-        private static readonly ContextAwareConcurrentHashtable<Type, object?> s_defaultProviderInitialized = new ContextAwareConcurrentHashtable<Type, object?>();
+        private static readonly CollectibleKeyConcurrentHashtable<Type, object?> s_defaultProviderInitialized = new CollectibleKeyConcurrentHashtable<Type, object?>();
 
         private static readonly object s_initializedDefaultProvider = new object();
 
@@ -361,7 +361,7 @@ private static void AddDefaultProvider(Type type)
         {
             bool providerAdded = false;
 
-            if (s_defaultProviderInitialized.Contains(type))
+            if (s_defaultProviderInitialized.ContainsKey(type))
             {
                 // Either another thread finished initializing for this type, or we are recursing on the same thread.
                 return;

src/libraries/System.ComponentModel.TypeConverter/tests/TypeDescriptorTests.cs

@@ -1591,6 +1591,8 @@ public static void TypeDescriptor_DoesNotKeepUnloadableTypes()
                     Assert.NotNull(collectibleAttributeType);
                     Attribute? collectibleAttribute = collectibleType.GetCustomAttribute(collectibleAttributeType);
                     Assert.NotNull(collectibleAttribute);
+                    Type? collectibleProviderType = assembly.GetType("UnloadableTestTypes.SimpleTypeDescriptionProvider");
+                    Assert.NotNull(collectibleProviderType);
 
                     // Cache the type's cachable entities
                     AttributeCollection attributes = TypeDescriptor.GetAttributes(collectibleType);
@@ -1602,8 +1604,10 @@ public static void TypeDescriptor_DoesNotKeepUnloadableTypes()
                     PropertyDescriptorCollection properties = TypeDescriptor.GetProperties(collectibleType);
                     Assert.Equal(2, properties.Count);
 
+                    // Test that the provider is the expected collectible one
                     TypeDescriptionProvider provider = TypeDescriptor.GetProvider(collectibleType);
                     Assert.NotNull(provider);
+                    Assert.IsType(collectibleProviderType, provider);
 
                     // Refresh the assembly causing the ReflectTypeDescriptionProvider caches to be purged.
                     TypeDescriptor.Refresh(assembly);
@@ -1613,14 +1617,19 @@ public static void TypeDescriptor_DoesNotKeepUnloadableTypes()
                     Assert.NotSame(attributes, TypeDescriptor.GetAttributes(collectibleType));
                     Assert.NotSame(events, TypeDescriptor.GetEvents(collectibleType));
                     Assert.NotSame(properties, TypeDescriptor.GetProperties(collectibleType));
+                    Assert.NotSame(provider, TypeDescriptor.GetProvider(collectibleType));
                 },
                 out var weakRef);
 
+            // Force garbage collection to ensure the ALC is unloaded and the types are collected.
             for (int i = 0; weakRef.IsAlive && i < 10; i++)
             {
                 GC.Collect();
                 GC.WaitForPendingFinalizers();
             }
+
+            // Assert that the weak reference to the ALC is no longer alive,
+            // indicating that the unloaded AssemblyLoadContext has been at least collected.
             Assert.True(!weakRef.IsAlive);
         }
 

src/libraries/System.ComponentModel.TypeConverter/tests/UnloadableTestTypes/UnloadableTestTypes.cs

@@ -2,10 +2,12 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
+using System.ComponentModel;
 
 namespace UnloadableTestTypes
 {
     [SimpleType]
+    [TypeDescriptionProvider(typeof(SimpleTypeDescriptionProvider))]
     public class SimpleType
     {
         public string P1 { get; set; }
@@ -19,4 +21,20 @@ public void OnActionEvent()
 
     [AttributeUsage(AttributeTargets.All)]
     public sealed class SimpleTypeAttribute : Attribute { }
+
+    public sealed class SimpleTypeDescriptionProvider : TypeDescriptionProvider
+    {
+        public SimpleTypeDescriptionProvider() { }
+
+        public override ICustomTypeDescriptor GetTypeDescriptor(Type objectType, object instance)
+        {
+            var baseDescriptor = base.GetTypeDescriptor(objectType, instance);
+            return new SimpleTypeDescriptor(baseDescriptor);
+        }
+
+        private sealed class SimpleTypeDescriptor : CustomTypeDescriptor
+        {
+            public SimpleTypeDescriptor(ICustomTypeDescriptor parent) : base(parent) { }
+        }
+    }
 }