Fix ReadOnlyDictionary and improve DebuggerAttributes

* Fix debugger type proxy attributes for Key and Value collections of ReadOnlyDictionary * Separate methods to test validity of the DebuggerTypeProxy attribute * ValidateDebuggerTypeProxyProperties - to verify debugger type proxy in most cases * CreateDebuggerTypeProxyWithNullArgument - to verify that the constructor of the debugger type proxy throws when its argument is null. * Simplify and make more uniform debugger type proxy tests A prerequisite for dotnet#94289
arturek · Nov 9, 2023 · 4b8524d · 4b8524d
1 parent 5184067
commit 4b8524d
Show file tree

Hide file tree

Showing 14 changed files with 50 additions and 102 deletions.
diff --git a/src/libraries/Common/tests/System/Collections/DebugView.Tests.cs b/src/libraries/Common/tests/System/Collections/DebugView.Tests.cs
@@ -202,8 +202,7 @@ public static void TestDebuggerAttributes_List(object obj)
         [MemberData(nameof(TestDebuggerAttributes_Inputs))]
         public static void TestDebuggerAttributes_Null(object obj)
         {
-            Type proxyType = DebuggerAttributes.GetProxyType(obj);
-            TargetInvocationException tie = Assert.Throws<TargetInvocationException>(() => Activator.CreateInstance(proxyType, (object)null));
+            TargetInvocationException tie = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.CreateDebuggerTypeProxyWithNullArgument(obj.GetType()));
             Assert.IsType<ArgumentNullException>(tie.InnerException);
         }
     }

diff --git a/src/libraries/Common/tests/System/Diagnostics/DebuggerAttributes.cs b/src/libraries/Common/tests/System/Diagnostics/DebuggerAttributes.cs
@@ -31,7 +31,7 @@ internal static object GetFieldValue(object obj, string fieldName)
 
         internal static void InvokeDebuggerTypeProxyProperties(object obj)
         {
-            DebuggerAttributeInfo info = ValidateDebuggerTypeProxyProperties(obj.GetType(), obj);
+            DebuggerAttributeInfo info = ValidateDebuggerTypeProxyProperties(obj);
             foreach (PropertyInfo pi in info.Properties)
             {
                 pi.GetValue(info.Instance, null);
@@ -40,17 +40,7 @@ internal static void InvokeDebuggerTypeProxyProperties(object obj)
 
         internal static DebuggerAttributeInfo ValidateDebuggerTypeProxyProperties(object obj)
         {
-            return ValidateDebuggerTypeProxyProperties(obj.GetType(), obj);
-        }
-
-        internal static DebuggerAttributeInfo ValidateDebuggerTypeProxyProperties(Type type, object obj)
-        {
-            return ValidateDebuggerTypeProxyProperties(type, type.GenericTypeArguments, obj);
-        }
-
-        internal static DebuggerAttributeInfo ValidateDebuggerTypeProxyProperties(Type type, Type[] genericTypeArguments, object obj)
-        {
-            Type proxyType = GetProxyType(type, genericTypeArguments);
+            Type proxyType = GetProxyType(obj);
 
             // Create an instance of the proxy type, and make sure we can access all of the instance properties
             // on the type without exception
@@ -63,35 +53,54 @@ internal static DebuggerAttributeInfo ValidateDebuggerTypeProxyProperties(Type t
             };
         }
 
-        public static DebuggerBrowsableState? GetDebuggerBrowsableState(MemberInfo info)
+        internal static void CreateDebuggerTypeProxyWithNullArgument(Type type)
+        {
+            Type proxyType = GetProxyType(type);
+            Activator.CreateInstance(proxyType, [null]);
+        }
+
+        internal static DebuggerBrowsableState? GetDebuggerBrowsableState(MemberInfo info)
         {
             CustomAttributeData debuggerBrowsableAttribute = info.CustomAttributes
                 .SingleOrDefault(a => a.AttributeType == typeof(DebuggerBrowsableAttribute));
             // Enums in attribute constructors are boxed as ints, so cast to int? first.
             return (DebuggerBrowsableState?)(int?)debuggerBrowsableAttribute?.ConstructorArguments.Single().Value;
         }
 
-        public static IEnumerable<FieldInfo> GetDebuggerVisibleFields(Type debuggerAttributeType)
+        internal static IEnumerable<FieldInfo> GetDebuggerVisibleFields(Type debuggerAttributeType)
         {
             // The debugger doesn't evaluate non-public members of type proxies.
             IEnumerable<FieldInfo> visibleFields = debuggerAttributeType.GetFields()
                 .Where(fi => fi.IsPublic && GetDebuggerBrowsableState(fi) != DebuggerBrowsableState.Never);
             return visibleFields;
         }
 
-        public static IEnumerable<PropertyInfo> GetDebuggerVisibleProperties(Type debuggerAttributeType)
+        internal static IEnumerable<PropertyInfo> GetDebuggerVisibleProperties(Type debuggerAttributeType)
         {
             // The debugger doesn't evaluate non-public members of type proxies. GetGetMethod returns null if the getter is non-public.
             IEnumerable<PropertyInfo> visibleProperties = debuggerAttributeType.GetProperties()
                 .Where(pi => pi.GetGetMethod() != null && GetDebuggerBrowsableState(pi) != DebuggerBrowsableState.Never);
             return visibleProperties;
         }
 
-        public static object GetProxyObject(object obj) => Activator.CreateInstance(GetProxyType(obj), obj);
+        internal static object GetProxyObject(object obj) => Activator.CreateInstance(GetProxyType(obj), obj);
+
+        internal static Type GetProxyType(object obj) => GetProxyType(obj.GetType());
+
+        internal static Type GetProxyType(Type type)
+        {
+            CustomAttributeData cad = FindAttribute(type, attributeType: typeof(DebuggerTypeProxyAttribute));
 
-        public static Type GetProxyType(object obj) => GetProxyType(obj.GetType());
+            Type proxyType = cad.ConstructorArguments[0].ArgumentType == typeof(Type) ?
+                (Type)cad.ConstructorArguments[0].Value :
+                Type.GetType((string)cad.ConstructorArguments[0].Value);
+            if (type.GenericTypeArguments.Length > 0)
+            {
+                proxyType = proxyType.MakeGenericType(type.GenericTypeArguments);
+            }
 
-        public static Type GetProxyType(Type type) => GetProxyType(type, type.GenericTypeArguments);
+            return proxyType;
+        }
 
         internal static DebuggerDisplayResult ValidateFullyDebuggerDisplayReferences(object obj)
         {
@@ -136,21 +145,6 @@ private static CustomAttributeData FindAttribute(Type type, Type attributeType)
             throw new InvalidOperationException($"Expected one {attributeType.Name} on {type}.");
         }
 
-        private static Type GetProxyType(Type type, Type[] genericTypeArguments)
-        {
-            CustomAttributeData cad = FindAttribute(type, attributeType: typeof(DebuggerTypeProxyAttribute));
-
-            Type proxyType = cad.ConstructorArguments[0].ArgumentType == typeof(Type) ?
-                (Type)cad.ConstructorArguments[0].Value :
-                Type.GetType((string)cad.ConstructorArguments[0].Value);
-            if (genericTypeArguments.Length > 0)
-            {
-                proxyType = proxyType.MakeGenericType(genericTypeArguments);
-            }
-
-            return proxyType;
-        }
-
         private static string FormatDebuggerDisplayNamedArgument(string argumentName, CustomAttributeData debuggerDisplayAttributeData, object obj)
         {
             CustomAttributeNamedArgument namedAttribute = debuggerDisplayAttributeData.NamedArguments.FirstOrDefault(na => na.MemberName == argumentName);

diff --git a/src/libraries/System.Collections.NonGeneric/tests/ArrayListTests.cs b/src/libraries/System.Collections.NonGeneric/tests/ArrayListTests.cs
@@ -84,17 +84,8 @@ public static void DebuggerAttribute()
             DebuggerAttributes.ValidateDebuggerDisplayReferences(new ArrayList());
             DebuggerAttributes.ValidateDebuggerTypeProxyProperties(new ArrayList() { "a", 1, "b", 2 });
 
-            bool threwNull = false;
-            try
-            {
-                DebuggerAttributes.ValidateDebuggerTypeProxyProperties(typeof(ArrayList), null);
-            }
-            catch (TargetInvocationException ex)
-            {
-                threwNull = ex.InnerException is ArgumentNullException;
-            }
-
-            Assert.True(threwNull);
+            TargetInvocationException tie = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.CreateDebuggerTypeProxyWithNullArgument(typeof(ArrayList)));
+            Assert.IsType<ArgumentNullException>(tie.InnerException);
         }
 
         [Fact]

diff --git a/src/libraries/System.Collections.NonGeneric/tests/HashtableTests.cs b/src/libraries/System.Collections.NonGeneric/tests/HashtableTests.cs
@@ -356,19 +356,10 @@ public void DebuggerAttribute()
 
             var hash = new Hashtable() { { "a", 1 }, { "b", 2 } };
             DebuggerAttributes.ValidateDebuggerTypeProxyProperties(hash);
-            DebuggerAttributes.ValidateDebuggerTypeProxyProperties(typeof(Hashtable), Hashtable.Synchronized(hash));
+            DebuggerAttributes.ValidateDebuggerTypeProxyProperties(Hashtable.Synchronized(hash));
 
-            bool threwNull = false;
-            try
-            {
-                DebuggerAttributes.ValidateDebuggerTypeProxyProperties(typeof(Hashtable), null);
-            }
-            catch (TargetInvocationException ex)
-            {
-                threwNull = ex.InnerException is ArgumentNullException;
-            }
-
-            Assert.True(threwNull);
+            TargetInvocationException tie = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.CreateDebuggerTypeProxyWithNullArgument(typeof(Hashtable)));
+            Assert.IsType<ArgumentNullException>(tie.InnerException);
         }
 
         [Fact]

diff --git a/src/libraries/System.Collections.NonGeneric/tests/QueueTests.cs b/src/libraries/System.Collections.NonGeneric/tests/QueueTests.cs
@@ -112,17 +112,8 @@ public static void DebuggerAttribute()
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsDebuggerTypeProxyAttributeSupported))]
         public static void DebuggerAttribute_NullQueue_ThrowsArgumentNullException()
         {
-            bool threwNull = false;
-            try
-            {
-                DebuggerAttributes.ValidateDebuggerTypeProxyProperties(typeof(Queue), null);
-            }
-            catch (TargetInvocationException ex)
-            {
-                threwNull = ex.InnerException is ArgumentNullException;
-            }
-
-            Assert.True(threwNull);
+            TargetInvocationException tie = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.CreateDebuggerTypeProxyWithNullArgument(typeof(Queue)));
+            Assert.IsType<ArgumentNullException>(tie.InnerException);
         }
 
         [Theory]

diff --git a/src/libraries/System.Collections.NonGeneric/tests/SortedListTests.cs b/src/libraries/System.Collections.NonGeneric/tests/SortedListTests.cs
@@ -243,17 +243,8 @@ public void DebuggerAttribute_SynchronizedList()
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsDebuggerTypeProxyAttributeSupported))]
         public void DebuggerAttribute_NullSortedList_ThrowsArgumentNullException()
         {
-            bool threwNull = false;
-            try
-            {
-                DebuggerAttributes.ValidateDebuggerTypeProxyProperties(typeof(SortedList), null);
-            }
-            catch (TargetInvocationException ex)
-            {
-                threwNull = ex.InnerException is ArgumentNullException;
-            }
-
-            Assert.True(threwNull);
+            TargetInvocationException tie = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.CreateDebuggerTypeProxyWithNullArgument(typeof(ArrayList)));
+            Assert.IsType<ArgumentNullException>(tie.InnerException);
         }
 
         [Fact]

diff --git a/src/libraries/System.Collections.NonGeneric/tests/StackTests.cs b/src/libraries/System.Collections.NonGeneric/tests/StackTests.cs
@@ -90,17 +90,8 @@ public static void DebuggerAttribute()
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsDebuggerTypeProxyAttributeSupported))]
         public static void DebuggerAttribute_NullStack_ThrowsArgumentNullException()
         {
-            bool threwNull = false;
-            try
-            {
-                DebuggerAttributes.ValidateDebuggerTypeProxyProperties(typeof(Stack), null);
-            }
-            catch (TargetInvocationException ex)
-            {
-                threwNull = ex.InnerException is ArgumentNullException;
-            }
-
-            Assert.True(threwNull);
+            TargetInvocationException tie = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.CreateDebuggerTypeProxyWithNullArgument(typeof(Stack)));
+            Assert.IsType<ArgumentNullException>(tie.InnerException);
         }
 
         [Fact]

diff --git a/...bjectModel/tests/ObservableCollection/ObservableCollection_ConstructorAndPropertyTests.cs b/...bjectModel/tests/ObservableCollection/ObservableCollection_ConstructorAndPropertyTests.cs
@@ -133,7 +133,7 @@ public static void DebuggerAttributeTests()
         [ActiveIssue("https://github.com/dotnet/runtime/issues/57588", typeof(PlatformDetection), nameof(PlatformDetection.IsBuiltWithAggressiveTrimming), nameof(PlatformDetection.IsBrowser))]
         public static void DebuggerAttribute_NullCollection_ThrowsArgumentNullException()
         {
-            TargetInvocationException ex = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.ValidateDebuggerTypeProxyProperties(typeof(ObservableCollection<int>), null));
+            TargetInvocationException ex = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.CreateDebuggerTypeProxyWithNullArgument(typeof(ObservableCollection<int>)));
             ArgumentNullException argumentNullException = Assert.IsType<ArgumentNullException>(ex.InnerException);
         }
 

diff --git a/src/libraries/System.ObjectModel/tests/ReadOnlyDictionary/ReadOnlyDictionaryTests.cs b/src/libraries/System.ObjectModel/tests/ReadOnlyDictionary/ReadOnlyDictionaryTests.cs
@@ -243,13 +243,13 @@ public static void DebuggerAttributeTests()
             Assert.Equal(dict.Count, itemArray.Length);
 
             DebuggerAttributes.ValidateDebuggerDisplayReferences(dict.Keys);
-            info = DebuggerAttributes.ValidateDebuggerTypeProxyProperties(typeof(ReadOnlyDictionary<int, int>.KeyCollection), new Type[] { typeof(int) }, dict.Keys);
+            info = DebuggerAttributes.ValidateDebuggerTypeProxyProperties(dict.Keys);
             itemProperty = info.Properties.Single(pr => pr.GetCustomAttribute<DebuggerBrowsableAttribute>().State == DebuggerBrowsableState.RootHidden);
             int[] items = itemProperty.GetValue(info.Instance) as int[];
             Assert.Equal(dict.Keys, items);
 
             DebuggerAttributes.ValidateDebuggerDisplayReferences(dict.Values);
-            info = DebuggerAttributes.ValidateDebuggerTypeProxyProperties(typeof(ReadOnlyDictionary<int, int>.KeyCollection), new Type[] { typeof(int) }, dict.Values);
+            info = DebuggerAttributes.ValidateDebuggerTypeProxyProperties(dict.Values);
             itemProperty = info.Properties.Single(pr => pr.GetCustomAttribute<DebuggerBrowsableAttribute>().State == DebuggerBrowsableState.RootHidden);
             items = itemProperty.GetValue(info.Instance) as int[];
             Assert.Equal(dict.Values, items);
@@ -259,7 +259,7 @@ public static void DebuggerAttributeTests()
         [ActiveIssue("https://github.com/dotnet/runtime/issues/57588", typeof(PlatformDetection), nameof(PlatformDetection.IsBuiltWithAggressiveTrimming), nameof(PlatformDetection.IsBrowser))]
         public static void DebuggerAttribute_NullDictionary_ThrowsArgumentNullException()
         {
-            TargetInvocationException ex = Assert.Throws<TargetInvocationException>(() =>   DebuggerAttributes.ValidateDebuggerTypeProxyProperties(typeof(ReadOnlyDictionary<int, int>), null));
+            TargetInvocationException ex = Assert.Throws<TargetInvocationException>(() =>   DebuggerAttributes.CreateDebuggerTypeProxyWithNullArgument(typeof(ReadOnlyDictionary<int, int>)));
             ArgumentNullException argumentNullException = Assert.IsType<ArgumentNullException>(ex.InnerException);
             Assert.Equal("dictionary", argumentNullException.ParamName);
         }
@@ -268,7 +268,7 @@ public static void DebuggerAttribute_NullDictionary_ThrowsArgumentNullException(
         [ActiveIssue("https://github.com/dotnet/runtime/issues/57588", typeof(PlatformDetection), nameof(PlatformDetection.IsBuiltWithAggressiveTrimming), nameof(PlatformDetection.IsBrowser))]
         public static void DebuggerAttribute_NullDictionaryKeys_ThrowsArgumentNullException()
         {
-            TargetInvocationException ex = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.ValidateDebuggerTypeProxyProperties(typeof(ReadOnlyDictionary<int, int>.KeyCollection), new Type[] { typeof(int) }, null));
+            TargetInvocationException ex = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.CreateDebuggerTypeProxyWithNullArgument(typeof(ReadOnlyDictionary<int, int>.KeyCollection)));
             ArgumentNullException argumentNullException = Assert.IsType<ArgumentNullException>(ex.InnerException);
             Assert.Equal("collection", argumentNullException.ParamName);
         }

diff --git a/...ystem.ObjectModel/tests/ReadOnlyObservableCollection/ReadOnlyObservableCollectionTests.cs b/...ystem.ObjectModel/tests/ReadOnlyObservableCollection/ReadOnlyObservableCollectionTests.cs
@@ -220,7 +220,7 @@ public static void DebuggerAttribute_Tests()
         [ActiveIssue("https://github.com/dotnet/runtime/issues/57588", typeof(PlatformDetection), nameof(PlatformDetection.IsBuiltWithAggressiveTrimming), nameof(PlatformDetection.IsBrowser))]
         public static void DebuggerAttribute_NullCollection_ThrowsArgumentNullException()
         {
-            TargetInvocationException ex = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.ValidateDebuggerTypeProxyProperties(typeof(ReadOnlyObservableCollection<int>), null));
+            TargetInvocationException ex = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.CreateDebuggerTypeProxyWithNullArgument(typeof(ReadOnlyObservableCollection<int>)));
             ArgumentNullException argumentNullException = Assert.IsType<ArgumentNullException>(ex.InnerException);
         }
     }

diff --git a/...libraries/System.Private.CoreLib/src/System/Collections/ObjectModel/ReadOnlyDictionary.cs b/...libraries/System.Private.CoreLib/src/System/Collections/ObjectModel/ReadOnlyDictionary.cs
@@ -247,7 +247,7 @@ public DictionaryEntry Entry
 
         IEnumerable<TValue> IReadOnlyDictionary<TKey, TValue>.Values => Values;
 
-        [DebuggerTypeProxy(typeof(ICollectionDebugView<>))]
+        [DebuggerTypeProxy(typeof(DictionaryKeyCollectionDebugView<,>))]
         [DebuggerDisplay("Count = {Count}")]
         public sealed class KeyCollection : ICollection<TKey>, ICollection, IReadOnlyCollection<TKey>
         {
@@ -303,7 +303,7 @@ void ICollection.CopyTo(Array array, int index)
             object ICollection.SyncRoot => (_collection is ICollection coll) ? coll.SyncRoot : this;
         }
 
-        [DebuggerTypeProxy(typeof(ICollectionDebugView<>))]
+        [DebuggerTypeProxy(typeof(DictionaryValueCollectionDebugView<,>))]
         [DebuggerDisplay("Count = {Count}")]
         public sealed class ValueCollection : ICollection<TValue>, ICollection, IReadOnlyCollection<TValue>
         {

diff --git a/...libraries/System.Text.RegularExpressions/tests/FunctionalTests/CaptureCollectionTests2.cs b/...libraries/System.Text.RegularExpressions/tests/FunctionalTests/CaptureCollectionTests2.cs
@@ -176,7 +176,7 @@ public static void DebuggerAttributeTests()
         [Fact]
         public static void DebuggerAttributeTests_Null()
         {
-            TargetInvocationException ex = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.ValidateDebuggerTypeProxyProperties(typeof(CaptureCollection), null));
+            TargetInvocationException ex = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.CreateDebuggerTypeProxyWithNullArgument(typeof(CaptureCollection)));
             Assert.IsType<ArgumentNullException>(ex.InnerException);
         }
     }

diff --git a/src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/GroupCollectionTests2.cs b/src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/GroupCollectionTests2.cs
@@ -173,7 +173,7 @@ public static void DebuggerAttributeTests()
         [Fact]
         public static void DebuggerAttributeTests_Null()
         {
-            TargetInvocationException ex = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.ValidateDebuggerTypeProxyProperties(typeof(GroupCollection), null));
+            TargetInvocationException ex = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.CreateDebuggerTypeProxyWithNullArgument(typeof(GroupCollection)));
             Assert.IsType<ArgumentNullException>(ex.InnerException);
         }
     }

diff --git a/src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/MatchCollectionTests2.cs b/src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/MatchCollectionTests2.cs
@@ -170,7 +170,7 @@ public static void DebuggerAttributeTests()
         [Fact]
         public static void DebuggerAttributeTests_Null()
         {
-            TargetInvocationException ex = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.ValidateDebuggerTypeProxyProperties(typeof(MatchCollection), null));
+            TargetInvocationException ex = Assert.Throws<TargetInvocationException>(() => DebuggerAttributes.CreateDebuggerTypeProxyWithNullArgument(typeof(MatchCollection)));
             Assert.IsType<ArgumentNullException>(ex.InnerException);
         }
     }