From 8a894b58b16137c42101e3d0126d7c2a05061098 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Thu, 8 Aug 2024 08:53:38 -0400 Subject: [PATCH] Revert "Use ArgumentNullException.ThrowIfNull in more places (#105380)" (#106108) Specifically the changes to System.Collections.Concurrent (ConcurrentDictionary) --- .../src/Resources/Strings.resx | 3 + .../Concurrent/ConcurrentDictionary.cs | 143 ++++++++++++++---- .../src/System/ThrowHelper.cs | 8 +- .../ConcurrentDictionaryTests.cs | 2 +- 4 files changed, 127 insertions(+), 29 deletions(-) diff --git a/src/libraries/System.Collections.Concurrent/src/Resources/Strings.resx b/src/libraries/System.Collections.Concurrent/src/Resources/Strings.resx index 5edafe5a5db987..b15258417a5b23 100644 --- a/src/libraries/System.Collections.Concurrent/src/Resources/Strings.resx +++ b/src/libraries/System.Collections.Concurrent/src/Resources/Strings.resx @@ -126,6 +126,9 @@ The key already existed in the dictionary. + + TKey is a reference type and item.Key is null. + The key was of an incorrect type for this dictionary. diff --git a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs index 8a7afaede743f2..5748725a154a31 100644 --- a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs +++ b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs @@ -329,7 +329,10 @@ private void InitializeFromCollection(IEnumerable> co { foreach (KeyValuePair pair in collection) { - ArgumentNullException.ThrowIfNull(pair.Key, "key"); + if (pair.Key is null) + { + ThrowHelper.ThrowKeyNullException(); + } if (!TryAddInternal(_tables, pair.Key, null, pair.Value, updateIfExists: false, acquireLock: false, out _)) { @@ -357,7 +360,10 @@ private void InitializeFromCollection(IEnumerable> co /// The contains too many elements. public bool TryAdd(TKey key, TValue value) { - ArgumentNullException.ThrowIfNull(key); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } return TryAddInternal(_tables, key, null, value, updateIfExists: false, acquireLock: true, out _); } @@ -383,7 +389,10 @@ public bool TryAdd(TKey key, TValue value) /// is a null reference (Nothing in Visual Basic). public bool TryRemove(TKey key, [MaybeNullWhen(false)] out TValue value) { - ArgumentNullException.ThrowIfNull(key); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } return TryRemoveInternal(key, out value, matchValue: false, default); } @@ -405,7 +414,10 @@ public bool TryRemove(TKey key, [MaybeNullWhen(false)] out TValue value) /// public bool TryRemove(KeyValuePair item) { - ArgumentNullException.ThrowIfNull(item.Key); + if (item.Key is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(item), SR.ConcurrentDictionary_ItemKeyIsNull); + } return TryRemoveInternal(item.Key, out _, matchValue: true, item.Value); } @@ -503,7 +515,10 @@ private bool TryRemoveInternal(TKey key, [MaybeNullWhen(false)] out TValue value /// is a null reference (Nothing in Visual Basic). public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) { - ArgumentNullException.ThrowIfNull(key); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } Tables tables = _tables; @@ -589,7 +604,10 @@ private static bool TryGetValueInternal(Tables tables, TKey key, int hashcode, [ /// is a null reference. public bool TryUpdate(TKey key, TValue newValue, TValue comparisonValue) { - ArgumentNullException.ThrowIfNull(key); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } return TryUpdateInternal(_tables, key, null, newValue, comparisonValue); } @@ -1080,7 +1098,10 @@ public TValue this[TKey key] } set { - ArgumentNullException.ThrowIfNull(key); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } TryAddInternal(_tables, key, null, value, updateIfExists: true, acquireLock: true, out _); } @@ -1184,8 +1205,15 @@ private int GetCountNoLocks() /// if the key was not in the dictionary. public TValue GetOrAdd(TKey key, Func valueFactory) { - ArgumentNullException.ThrowIfNull(key); - ArgumentNullException.ThrowIfNull(valueFactory); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } + + if (valueFactory is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(valueFactory)); + } Tables tables = _tables; @@ -1219,8 +1247,15 @@ public TValue GetOrAdd(TKey key, Func valueFactory) public TValue GetOrAdd(TKey key, Func valueFactory, TArg factoryArgument) where TArg : allows ref struct { - ArgumentNullException.ThrowIfNull(key); - ArgumentNullException.ThrowIfNull(valueFactory); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } + + if (valueFactory is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(valueFactory)); + } Tables tables = _tables; @@ -1249,7 +1284,10 @@ public TValue GetOrAdd(TKey key, Func valueFactory, TA /// key is already in the dictionary, or the new value if the key was not in the dictionary. public TValue GetOrAdd(TKey key, TValue value) { - ArgumentNullException.ThrowIfNull(key); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } Tables tables = _tables; @@ -1288,9 +1326,20 @@ public TValue AddOrUpdate( TKey key, Func addValueFactory, Func updateValueFactory, TArg factoryArgument) where TArg : allows ref struct { - ArgumentNullException.ThrowIfNull(key); - ArgumentNullException.ThrowIfNull(addValueFactory); - ArgumentNullException.ThrowIfNull(updateValueFactory); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } + + if (addValueFactory is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(addValueFactory)); + } + + if (updateValueFactory is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(updateValueFactory)); + } Tables tables = _tables; @@ -1350,9 +1399,20 @@ public TValue AddOrUpdate( /// absent) or the result of updateValueFactory (if the key was present). public TValue AddOrUpdate(TKey key, Func addValueFactory, Func updateValueFactory) { - ArgumentNullException.ThrowIfNull(key); - ArgumentNullException.ThrowIfNull(addValueFactory); - ArgumentNullException.ThrowIfNull(updateValueFactory); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } + + if (addValueFactory is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(addValueFactory)); + } + + if (updateValueFactory is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(updateValueFactory)); + } Tables tables = _tables; @@ -1410,8 +1470,15 @@ public TValue AddOrUpdate(TKey key, Func addValueFactory, Func public TValue AddOrUpdate(TKey key, TValue addValue, Func updateValueFactory) { - ArgumentNullException.ThrowIfNull(key); - ArgumentNullException.ThrowIfNull(updateValueFactory); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } + + if (updateValueFactory is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(updateValueFactory)); + } Tables tables = _tables; @@ -1632,7 +1699,11 @@ bool ICollection>.Remove(KeyValuePair k /// void IDictionary.Add(object key, object? value) { - ArgumentNullException.ThrowIfNull(key); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } + if (!(key is TKey)) { throw new ArgumentException(SR.ConcurrentDictionary_TypeOfKeyIncorrect); @@ -1655,7 +1726,10 @@ void IDictionary.Add(object key, object? value) /// (Nothing in Visual Basic). bool IDictionary.Contains(object key) { - ArgumentNullException.ThrowIfNull(key); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } return key is TKey tkey && ContainsKey(tkey); } @@ -1703,7 +1777,10 @@ bool IDictionary.Contains(object key) /// (Nothing in Visual Basic). void IDictionary.Remove(object key) { - ArgumentNullException.ThrowIfNull(key); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } if (key is TKey tkey) { @@ -1741,7 +1818,10 @@ void IDictionary.Remove(object key) { get { - ArgumentNullException.ThrowIfNull(key); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } if (key is TKey tkey && TryGetValue(tkey, out TValue? value)) { @@ -1752,7 +1832,10 @@ void IDictionary.Remove(object key) } set { - ArgumentNullException.ThrowIfNull(key); + if (key is null) + { + ThrowHelper.ThrowKeyNullException(); + } if (!(key is TKey)) { @@ -2408,7 +2491,10 @@ private bool TryAdd(TAlternateKey key, TValue value, bool updateIfExists, out TV } TKey actualKey = comparer.Create(key); - ArgumentNullException.ThrowIfNull(actualKey, nameof(key)); + if (actualKey is null) + { + ThrowHelper.ThrowKeyNullException(); + } // The key was not found in the bucket. Insert the key-value pair. var resultNode = new Node(actualKey, value, hashcode, bucket); @@ -2638,7 +2724,10 @@ internal sealed class IDictionaryDebugView where TKey : notnull public IDictionaryDebugView(IDictionary dictionary) { - ArgumentNullException.ThrowIfNull(dictionary); + if (dictionary is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(dictionary)); + } _dictionary = dictionary; } diff --git a/src/libraries/System.Collections.Concurrent/src/System/ThrowHelper.cs b/src/libraries/System.Collections.Concurrent/src/System/ThrowHelper.cs index ea06bf6a3c307f..bb8a35bac4e414 100644 --- a/src/libraries/System.Collections.Concurrent/src/System/ThrowHelper.cs +++ b/src/libraries/System.Collections.Concurrent/src/System/ThrowHelper.cs @@ -8,7 +8,13 @@ namespace System internal static class ThrowHelper { [DoesNotReturn] - internal static void ThrowKeyNullException() => throw new ArgumentNullException("key"); + internal static void ThrowKeyNullException() => ThrowArgumentNullException("key"); + + [DoesNotReturn] + internal static void ThrowArgumentNullException(string name) => throw new ArgumentNullException(name); + + [DoesNotReturn] + internal static void ThrowArgumentNullException(string name, string message) => throw new ArgumentNullException(name, message); [DoesNotReturn] internal static void ThrowValueNullException() => throw new ArgumentException(SR.ConcurrentDictionary_TypeOfValueIncorrect); diff --git a/src/libraries/System.Collections.Concurrent/tests/ConcurrentDictionary/ConcurrentDictionaryTests.cs b/src/libraries/System.Collections.Concurrent/tests/ConcurrentDictionary/ConcurrentDictionaryTests.cs index 500d8b88f9dccf..c92689a6b034f4 100644 --- a/src/libraries/System.Collections.Concurrent/tests/ConcurrentDictionary/ConcurrentDictionaryTests.cs +++ b/src/libraries/System.Collections.Concurrent/tests/ConcurrentDictionary/ConcurrentDictionaryTests.cs @@ -449,7 +449,7 @@ public static void TestRemove3() [Fact] public static void TryRemove_KeyValuePair_ArgumentValidation() { - AssertExtensions.Throws("item.Key", () => new ConcurrentDictionary().TryRemove(new KeyValuePair(null, 42))); + AssertExtensions.Throws("item", () => new ConcurrentDictionary().TryRemove(new KeyValuePair(null, 42))); new ConcurrentDictionary().TryRemove(new KeyValuePair(0, 0)); // no error when using default value type new ConcurrentDictionary().TryRemove(new KeyValuePair(0, 0)); // or nullable }