Remove custom enumerator from ConcurrentBag (#117353)

ConcurrentBag's snapshot enumeration semantics are obtained by creating an array of the contents and then enumerating that array. This was done with a custom enumerator out of an abundance of caution about maintaining semantics for operations with undefined behavior, e.g. using Current after MoveNext returns false. We've since stopped trying to maintain bug-for-bug compatibility in such undefined areas. As such, this just deletes the custom enumerator and uses Array's. This not only reduces code but has better performance characteristics (though the whole ToArray as part of enumeration already dominates the enumeration costs).

Author: stephentoub

src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentBag.cs

@@ -459,7 +459,7 @@ public void Clear()
         /// <see cref="GetEnumerator"/> was called.  The enumerator is safe to use
         /// concurrently with reads from and writes to the bag.
         /// </remarks>
-        public IEnumerator<T> GetEnumerator() => new Enumerator(ToArray());
+        public IEnumerator<T> GetEnumerator() => ((IEnumerable<T>)ToArray()).GetEnumerator();
 
         /// <summary>
         /// Returns an enumerator that iterates through the <see
@@ -1074,61 +1074,5 @@ internal enum Operation
             Add,
             Take
         };
-
-        /// <summary>Provides an enumerator for the bag.</summary>
-        /// <remarks>
-        /// The original implementation of ConcurrentBag used a <see cref="List{T}"/> as part of
-        /// the GetEnumerator implementation.  That list was then changed to be an array, but array's
-        /// GetEnumerator has different behavior than does list's, in particular for the case where
-        /// Current is used after MoveNext returns false.  To avoid any concerns around compatibility,
-        /// we use a custom enumerator rather than just returning array's. This enumerator provides
-        /// the essential elements of both list's and array's enumerators.
-        /// </remarks>
-        private sealed class Enumerator : IEnumerator<T>
-        {
-            private readonly T[] _array;
-            private T? _current;
-            private int _index;
-
-            public Enumerator(T[] array)
-            {
-                Debug.Assert(array != null);
-                _array = array;
-            }
-
-            public bool MoveNext()
-            {
-                if (_index < _array.Length)
-                {
-                    _current = _array[_index++];
-                    return true;
-                }
-
-                _index = _array.Length + 1;
-                return false;
-            }
-
-            public T Current => _current!;
-
-            object? IEnumerator.Current
-            {
-                get
-                {
-                    if (_index == 0 || _index == _array.Length + 1)
-                    {
-                        throw new InvalidOperationException(SR.ConcurrentBag_Enumerator_EnumerationNotStartedOrAlreadyFinished);
-                    }
-                    return Current;
-                }
-            }
-
-            public void Reset()
-            {
-                _index = 0;
-                _current = default;
-            }
-
-            public void Dispose() { }
-        }
     }
 }

src/libraries/System.Collections.Concurrent/tests/ConcurrentBagTests.cs

@@ -11,6 +11,8 @@ namespace System.Collections.Concurrent.Tests
 {
     public class ConcurrentBagTests : ProducerConsumerCollectionTests
     {
+        protected override bool Enumerator_Current_UndefinedOperation_Throws => true;
+        protected override bool Enumerator_Empty_UsesSingletonInstance => true;
         protected override IProducerConsumerCollection<T> CreateProducerConsumerCollection<T>() => new ConcurrentBag<T>();
         protected override IProducerConsumerCollection<int> CreateProducerConsumerCollection(IEnumerable<int> collection) => new ConcurrentBag<int>(collection);
         protected override bool IsEmpty(IProducerConsumerCollection<int> pcc) => ((ConcurrentBag<int>)pcc).IsEmpty;