Fix dotnet/corefx#14961 and tweak perf in ImmutableArray (dotnet/core…

…fx#15296)

Commit migrated from dotnet/corefx@be3d8cd

src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/IImmutableArray.cs

@@ -22,7 +22,5 @@ internal interface IImmutableArray
         /// Gets an untyped reference to the array.
         /// </summary>
         Array Array { get; }
-
-        void ThrowInvalidOperationIfNotInitialized();
     }
 }

src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray.cs

@@ -106,14 +106,18 @@ public static ImmutableArray<T> CreateRange<T>(IEnumerable<T> items)
             var immutableArray = items as IImmutableArray;
             if (immutableArray != null)
             {
-                immutableArray.ThrowInvalidOperationIfNotInitialized();
+                Array array = immutableArray.Array;
+                if (array == null)
+                {
+                    throw new InvalidOperationException(SR.InvalidOperationOnDefaultArray);
+                }
 
-                // immutableArray.Array must not be null at this point, and we know it's an
+                // `array` must not be null at this point, and we know it's an
                 // ImmutableArray<T> or ImmutableArray<SomethingDerivedFromT> as they are
                 // the only types that could be both IEnumerable<T> and IImmutableArray.
                 // As such, we know that items is either an ImmutableArray<T> or
                 // ImmutableArray<TypeDerivedFromT>, and we can cast the array to T[].
-                return new ImmutableArray<T>((T[])immutableArray.Array);
+                return new ImmutableArray<T>((T[])array);
             }
 
             // We don't recognize the source as an array that is safe to use.

src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.Builder.cs

@@ -245,6 +245,12 @@ public void AddRange(IEnumerable<T> items)
                 if (items.TryGetCount(out count))
                 {
                     this.EnsureCapacity(this.Count + count);
+
+                    if (items.TryCopyTo(_elements, _count))
+                    {
+                        _count += count;
+                        return;
+                    }
                 }
 
                 foreach (var item in items)

src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.cs

@@ -613,7 +613,7 @@ public ImmutableArray<T> InsertRange(int index, ImmutableArray<T> items)
         {
             var self = this;
             self.ThrowNullRefIfNotInitialized();
-            ThrowNullRefIfNotInitialized(items);
+            items.ThrowNullRefIfNotInitialized();
             Requires.Range(index >= 0 && index <= self.Length, nameof(index));
 
             if (self.IsEmpty)
@@ -692,6 +692,7 @@ public ImmutableArray<T> AddRange(ImmutableArray<T> items)
         public ImmutableArray<T> SetItem(int index, T item)
         {
             var self = this;
+            self.ThrowNullRefIfNotInitialized();
             Requires.Range(index >= 0 && index < self.Length, nameof(index));
 
             T[] tmp = new T[self.Length];
@@ -728,7 +729,7 @@ public ImmutableArray<T> Replace(T oldValue, T newValue)
         public ImmutableArray<T> Replace(T oldValue, T newValue, IEqualityComparer<T> equalityComparer)
         {
             var self = this;
-            int index = self.IndexOf(oldValue, equalityComparer);
+            int index = self.IndexOf(oldValue, 0, self.Length, equalityComparer);
             if (index < 0)
             {
                 throw new ArgumentException(SR.CannotFindOldValue, nameof(oldValue));
@@ -764,7 +765,7 @@ public ImmutableArray<T> Remove(T item, IEqualityComparer<T> equalityComparer)
         {
             var self = this;
             self.ThrowNullRefIfNotInitialized();
-            int index = self.IndexOf(item, equalityComparer);
+            int index = self.IndexOf(item, 0, self.Length, equalityComparer);
             return index < 0
                 ? self
                 : self.RemoveAt(index);
@@ -791,6 +792,7 @@ public ImmutableArray<T> RemoveAt(int index)
         public ImmutableArray<T> RemoveRange(int index, int length)
         {
             var self = this;
+            self.ThrowNullRefIfNotInitialized();
             Requires.Range(index >= 0 && index <= self.Length, nameof(index));
             Requires.Range(length >= 0 && index + length <= self.Length, nameof(length));
 
@@ -836,18 +838,18 @@ public ImmutableArray<T> RemoveRange(IEnumerable<T> items, IEqualityComparer<T>
             self.ThrowNullRefIfNotInitialized();
             Requires.NotNull(items, nameof(items));
 
-            var indexesToRemove = new SortedSet<int>();
+            var indicesToRemove = new SortedSet<int>();
             foreach (var item in items)
             {
-                int index = self.IndexOf(item, equalityComparer);
-                while (index >= 0 && !indexesToRemove.Add(index) && index + 1 < self.Length)
+                int index = self.IndexOf(item, 0, self.Length, equalityComparer);
+                while (index >= 0 && !indicesToRemove.Add(index) && index + 1 < self.Length)
                 {
                     // This is a duplicate of one we've found. Try hard to find another instance in the list to remove.
                     index = self.IndexOf(item, index + 1, equalityComparer);
                 }
             }
 
-            return self.RemoveAtRange(indexesToRemove);
+            return self.RemoveAtRange(indicesToRemove);
         }
 
         /// <summary>
@@ -917,22 +919,22 @@ public ImmutableArray<T> RemoveAll(Predicate<T> match)
                 return self;
             }
 
-            List<int> removeIndexes = null;
+            List<int> removeIndices = null;
             for (int i = 0; i < self.array.Length; i++)
             {
                 if (match(self.array[i]))
                 {
-                    if (removeIndexes == null)
+                    if (removeIndices == null)
                     {
-                        removeIndexes = new List<int>();
+                        removeIndices = new List<int>();
                     }
 
-                    removeIndexes.Add(i);
+                    removeIndices.Add(i);
                 }
             }
 
-            return removeIndexes != null ?
-                self.RemoveAtRange(removeIndexes) :
+            return removeIndices != null ?
+                self.RemoveAtRange(removeIndices) :
                 self;
         }
 
@@ -1566,16 +1568,16 @@ bool IStructuralEquatable.Equals(object other, IEqualityComparer comparer)
                 var theirs = other as IImmutableArray;
                 if (theirs != null)
                 {
-                    if (self.array == null && theirs.Array == null)
+                    otherArray = theirs.Array;
+
+                    if (self.array == null && otherArray == null)
                     {
                         return true;
                     }
                     else if (self.array == null)
                     {
                         return false;
                     }
-
-                    otherArray = theirs.Array;
                 }
             }
 
@@ -1617,16 +1619,16 @@ int IStructuralComparable.CompareTo(object other, IComparer comparer)
                 var theirs = other as IImmutableArray;
                 if (theirs != null)
                 {
-                    if (self.array == null && theirs.Array == null)
+                    otherArray = theirs.Array;
+
+                    if (self.array == null && otherArray == null)
                     {
                         return 0;
                     }
-                    else if (self.array == null ^ theirs.Array == null)
+                    else if (self.array == null ^ otherArray == null)
                     {
                         throw new ArgumentException(SR.ArrayInitializedStateNotEqual, nameof(other));
                     }
-
-                    otherArray = theirs.Array;
                 }
             }
 
@@ -1673,33 +1675,28 @@ private void ThrowInvalidOperationIfNotInitialized()
             }
         }
 
-        void IImmutableArray.ThrowInvalidOperationIfNotInitialized()
-        {
-            this.ThrowInvalidOperationIfNotInitialized();
-        }
-
         /// <summary>
-        /// Returns an array with items at the specified indexes removed.
+        /// Returns an array with items at the specified indices removed.
         /// </summary>
-        /// <param name="indexesToRemove">A **sorted set** of indexes to elements that should be omitted from the returned array.</param>
+        /// <param name="indicesToRemove">A **sorted set** of indices to elements that should be omitted from the returned array.</param>
         /// <returns>The new array.</returns>
-        private ImmutableArray<T> RemoveAtRange(ICollection<int> indexesToRemove)
+        private ImmutableArray<T> RemoveAtRange(ICollection<int> indicesToRemove)
         {
             var self = this;
             self.ThrowNullRefIfNotInitialized();
-            Requires.NotNull(indexesToRemove, nameof(indexesToRemove));
+            Requires.NotNull(indicesToRemove, nameof(indicesToRemove));
 
-            if (indexesToRemove.Count == 0)
+            if (indicesToRemove.Count == 0)
             {
                 // Be sure to return a !IsDefault instance.
                 return self;
             }
 
-            var newArray = new T[self.Length - indexesToRemove.Count];
+            var newArray = new T[self.Length - indicesToRemove.Count];
             int copied = 0;
             int removed = 0;
             int lastIndexRemoved = -1;
-            foreach (var indexToRemove in indexesToRemove)
+            foreach (var indexToRemove in indicesToRemove)
             {
                 int copyLength = lastIndexRemoved == -1 ? indexToRemove : (indexToRemove - lastIndexRemoved - 1);
                 Debug.Assert(indexToRemove > lastIndexRemoved); // We require that the input be a sorted set.
@@ -1713,13 +1710,5 @@ private ImmutableArray<T> RemoveAtRange(ICollection<int> indexesToRemove)
 
             return new ImmutableArray<T>(newArray);
         }
-
-        /// <summary>
-        /// Throws a <see cref="NullReferenceException"/> if the specified array is uninitialized.
-        /// </summary>
-        private static void ThrowNullRefIfNotInitialized(ImmutableArray<T> array)
-        {
-            array.ThrowNullRefIfNotInitialized();
-        }
     }
 }

src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableExtensions.cs

@@ -106,7 +106,11 @@ internal static int GetCount<T>(ref IEnumerable<T> sequence)
         /// </remarks>
         internal static bool TryCopyTo<T>(this IEnumerable<T> sequence, T[] array, int arrayIndex)
         {
-            // IList is the GCD of what the following 2 types implement.
+            Debug.Assert(sequence != null);
+            Debug.Assert(array != null);
+            Debug.Assert(arrayIndex >= 0 && arrayIndex <= array.Length);
+
+            // IList is the GCD of what the following types implement.
             var listInterface = sequence as IList<T>;
             if (listInterface != null)
             {
@@ -117,6 +121,16 @@ internal static bool TryCopyTo<T>(this IEnumerable<T> sequence, T[] array, int a
                     return true;
                 }
 
+                // Array.Copy can throw an ArrayTypeMismatchException if the underlying type of
+                // the destination array is not typeof(T[]), but is assignment-compatible with T[].
+                // See https://github.com/dotnet/corefx/issues/2241 for more info.
+                if (sequence.GetType() == typeof(T[]))
+                {
+                    var sourceArray = (T[])sequence;
+                    Array.Copy(sourceArray, 0, array, arrayIndex, sourceArray.Length);
+                    return true;
+                }
+
                 if (sequence is ImmutableArray<T>)
                 {
                     var immutable = (ImmutableArray<T>)sequence;

src/libraries/System.Collections.Immutable/tests/ImmutableArrayBuilderTest.cs

@@ -75,6 +75,9 @@ public void AddRangeIEnumerable()
             builder.AddRange((IEnumerable<int>)new[] { 2 });
             Assert.Equal(2, builder.Count);
 
+            builder.AddRange((IEnumerable<int>)new int[0]);
+            Assert.Equal(2, builder.Count);
+
             // Exceed capacity
             builder.AddRange(Enumerable.Range(3, 2)); // use an enumerable without a breakable Count
             Assert.Equal(4, builder.Count);

src/libraries/System.Collections.Immutable/tests/ImmutableArrayTest.cs

@@ -1127,7 +1127,7 @@ public void RemoveAtInvalid(IEnumerable<int> source)
         }
 
         [Theory]
-        [InlineData(-1, Skip = "#14961")]
+        [InlineData(-1)]
         [InlineData(0)]
         [InlineData(1)]
         public void RemoveAtDefaultInvalid(int index)
@@ -1217,7 +1217,7 @@ public void RemoveRangeIndexLengthInvalid(IEnumerable<int> source)
         }
 
         [Theory]
-        [InlineData(-1, 0, Skip = "#14961")]
+        [InlineData(-1, 0)]
         [InlineData(0, -1)]
         [InlineData(0, 0)]
         [InlineData(1, -1)]
@@ -1446,9 +1446,8 @@ public void ReplaceDefaultInvalid()
         {
             Assert.All(SharedEqualityComparers<int>(), comparer =>
             {
-                // Uncomment when #14961 is fixed.
-                // Assert.Throws<NullReferenceException>(() => s_emptyDefault.Replace(123, 123));
-                // Assert.Throws<NullReferenceException>(() => s_emptyDefault.Replace(123, 123, comparer));
+                Assert.Throws<NullReferenceException>(() => s_emptyDefault.Replace(123, 123));
+                Assert.Throws<NullReferenceException>(() => s_emptyDefault.Replace(123, 123, comparer));
 
                 Assert.Throws<InvalidOperationException>(() => ((IImmutableList<int>)s_emptyDefault).Replace(123, 123));
                 Assert.Throws<InvalidOperationException>(() => ((IImmutableList<int>)s_emptyDefault).Replace(123, 123, comparer));
@@ -1485,7 +1484,7 @@ public void SetItemInvalid(IEnumerable<int> source)
         }
 
         [Theory]
-        [InlineData(-1, Skip = "#14961")]
+        [InlineData(-1)]
         [InlineData(0)]
         [InlineData(1)]
         public void SetItemDefaultInvalid(int index)