Fix LargeArrayBuilder.CopyTo returning incorrect end-of-copy position (…

…#23865) * Fix LargeArrayBuilder.CopyTo returning incorrect end-of-copy position (#23817) * Fix #23680 * PR Feedback * More tests * More tests * Not using a local function
dotnet · Sep 9, 2017 · 6d38f59 · 6d38f59
1 parent 4a8a1ab
commit 6d38f59
Show file tree

Hide file tree

Showing 4 changed files with 276 additions and 20 deletions.
diff --git a/src/Common/src/System/Collections/Generic/LargeArrayBuilder.cs b/src/Common/src/System/Collections/Generic/LargeArrayBuilder.cs
@@ -42,6 +42,20 @@ internal CopyPosition(int row, int column)
  /// </summary>
  internal int Column { get; }
 
+ /// <summary>
+ /// If this position is at the end of the current buffer, returns the position
+ /// at the start of the next buffer. Otherwise, returns this position.
+ /// </summary>
+ /// <param name="endColumn">The length of the current buffer.</param>
+ public CopyPosition Normalize(int endColumn)
+ {
+ Debug.Assert(Column <= endColumn);
+
+ return Column == endColumn ?
+ new CopyPosition(Row + 1, 0) :
+ this;
+ }
+
  /// <summary>
  /// Gets a string suitable for display in the debugger.
  /// </summary>
@@ -186,7 +200,7 @@ public void CopyTo(T[] array, int arrayIndex, int count)
  arrayIndex += toCopy;
  }
  }
- 
+
  /// <summary>
  /// Copies the contents of this builder to the specified array.
  /// </summary>
@@ -197,9 +211,10 @@ public void CopyTo(T[] array, int arrayIndex, int count)
  /// <returns>The position in this builder that was copied up to.</returns>
  public CopyPosition CopyTo(CopyPosition position, T[] array, int arrayIndex, int count)
  {
+ Debug.Assert(array != null);
  Debug.Assert(arrayIndex >= 0);
- Debug.Assert(count >= 0 && count <= Count);
- Debug.Assert(array?.Length - arrayIndex >= count);
+ Debug.Assert(count > 0 && count <= Count);
+ Debug.Assert(array.Length - arrayIndex >= count);
 
  // Go through each buffer, which contains one 'row' of items.
  // The index in each buffer is referred to as the 'column'.
@@ -216,25 +231,33 @@ public CopyPosition CopyTo(CopyPosition position, T[] array, int arrayIndex, int
  int row = position.Row;
  int column = position.Column;
 
- for (; count > 0; row++, column = 0)
- {
- T[] buffer = GetBuffer(index: row);
+ T[] buffer = GetBuffer(row);
+ Debug.Assert(buffer.Length > column);
 
- // During this iteration, copy until we satisfy `count` or reach the
- // end of the current buffer.
- int copyCount = Math.Min(buffer.Length, count);
+ int copied = Math.Min(buffer.Length - column, count);
+ Array.Copy(buffer, column, array, arrayIndex, copied);
 
- if (copyCount > 0)
- {
- Array.Copy(buffer, column, array, arrayIndex, copyCount);
+ arrayIndex += copied;
+ count -= copied;
 
- arrayIndex += copyCount;
- count -= copyCount;
- column += copyCount;
- }
+ if (count == 0)
+ {
+ return new CopyPosition(row, column + copied).Normalize(buffer.Length);
  }
 
- return new CopyPosition(row: row, column: column);
+ do
+ {
+ buffer = GetBuffer(++row);
+ Debug.Assert(buffer.Length > 0);
+
+ copied = Math.Min(buffer.Length, count);
+ Array.Copy(buffer, 0, array, arrayIndex, copied);
+
+ arrayIndex += copied;
+ count -= copied;
+ } while (count > 0);
+
+ return new CopyPosition(row, copied).Normalize(buffer.Length);
  }
 
  /// <summary>

diff --git a/src/Common/src/System/Collections/Generic/SparseArrayBuilder.cs b/src/Common/src/System/Collections/Generic/SparseArrayBuilder.cs
@@ -113,9 +113,10 @@ public SparseArrayBuilder(bool initialize)
  /// <param name="count">The number of items to copy.</param>
  public void CopyTo(T[] array, int arrayIndex, int count)
  {
+ Debug.Assert(array != null);
  Debug.Assert(arrayIndex >= 0);
  Debug.Assert(count >= 0 && count <= Count);
- Debug.Assert(array?.Length - arrayIndex >= count);
+ Debug.Assert(array.Length - arrayIndex >= count);
 
  int copied = 0;
  var position = CopyPosition.Start;
@@ -149,8 +150,11 @@ public void CopyTo(T[] array, int arrayIndex, int count)
  count -= reservedCount;
  }
 
- // Finish copying after the final marker.
- _builder.CopyTo(position, array, arrayIndex, count);
+ if (count > 0)
+ {
+ // Finish copying after the final marker.
+ _builder.CopyTo(position, array, arrayIndex, count);
+ }
  }
 
  /// <summary>

diff --git a/src/System.Linq/tests/ConcatTests.cs b/src/System.Linq/tests/ConcatTests.cs
@@ -421,5 +421,123 @@ public void GetEnumerableOfConcatCollectionChainFollowedByEnumerableNodeShouldBe
  Assert.Equal(0xf00, en.Current);
  }
  }
+
+ [Theory]
+ [MemberData(nameof(GetToArrayDataSources))]
+ public void CollectionInterleavedWithLazyEnumerables_ToArray(IEnumerable<int>[] arrays)
+ {
+ // See https://github.com/dotnet/corefx/issues/23680
+
+ IEnumerable<int> concats = arrays[0];
+
+ for (int i = 1; i < arrays.Length; i++)
+ {
+ concats = concats.Concat(arrays[i]);
+ }
+
+ int[] results = concats.ToArray();
+
+ for (int i = 0; i < results.Length; i++)
+ {
+ Assert.Equal(i, results[i]);
+ }
+ }
+
+ private static IEnumerable<object[]> GetToArrayDataSources()
+ {
+ // Marker at the end
+ yield return new object[]
+ {
+ new IEnumerable<int>[]
+ {
+ new TestEnumerable<int>(new int[] { 0 }),
+ new TestEnumerable<int>(new int[] { 1 }),
+ new TestEnumerable<int>(new int[] { 2 }),
+ new int[] { 3 },
+ }
+ };
+
+ // Marker at beginning
+ yield return new object[]
+ {
+ new IEnumerable<int>[]
+ {
+ new int[] { 0 },
+ new TestEnumerable<int>(new int[] { 1 }),
+ new TestEnumerable<int>(new int[] { 2 }),
+ new TestEnumerable<int>(new int[] { 3 }),
+ }
+ };
+
+ // Marker in middle
+ yield return new object[]
+ {
+ new IEnumerable<int>[]
+ {
+ new TestEnumerable<int>(new int[] { 0 }),
+ new int[] { 1 },
+ new TestEnumerable<int>(new int[] { 2 }),
+ }
+ };
+
+ // Non-marker in middle
+ yield return new object[]
+ {
+ new IEnumerable<int>[]
+ {
+ new int[] { 0 },
+ new TestEnumerable<int>(new int[] { 1 }),
+ new int[] { 2 },
+ }
+ };
+
+ // Big arrays (marker in middle)
+ yield return new object[]
+ {
+ new IEnumerable<int>[]
+ {
+ new TestEnumerable<int>(Enumerable.Range(0, 100).ToArray()),
+ Enumerable.Range(100, 100).ToArray(),
+ new TestEnumerable<int>(Enumerable.Range(200, 100).ToArray()),
+ }
+ };
+
+ // Big arrays (non-marker in middle)
+ yield return new object[]
+ {
+ new IEnumerable<int>[]
+ {
+ Enumerable.Range(0, 100).ToArray(),
+ new TestEnumerable<int>(Enumerable.Range(100, 100).ToArray()),
+ Enumerable.Range(200, 100).ToArray(),
+ }
+ };
+
+ // Interleaved (first marker)
+ yield return new object[]
+ {
+ new IEnumerable<int>[]
+ {
+ new int[] { 0 },
+ new TestEnumerable<int>(new int[] { 1 }),
+ new int[] { 2 },
+ new TestEnumerable<int>(new int[] { 3 }),
+ new int[] { 4 },
+ }
+ };
+
+ // Interleaved (first non-marker)
+ yield return new object[]
+ {
+ new IEnumerable<int>[]
+ {
+ new TestEnumerable<int>(new int[] { 0 }),
+ new int[] { 1 },
+ new TestEnumerable<int>(new int[] { 2 }),
+ new int[] { 3 },
+ new TestEnumerable<int>(new int[] { 4 }),
+ }
+ };
+ }
  }
 }
diff --git a/src/System.Linq/tests/SelectManyTests.cs b/src/System.Linq/tests/SelectManyTests.cs
@@ -477,5 +477,116 @@ public void ThrowOverflowExceptionOnConstituentLargeCounts(int[] counts)
  IEnumerable<int> iterator = counts.SelectMany(c => Enumerable.Range(1, c));
  Assert.Throws<OverflowException>(() => iterator.Count());
  }
+
+ [Theory]
+ [MemberData(nameof(GetToArrayDataSources))]
+ public void CollectionInterleavedWithLazyEnumerables_ToArray(IEnumerable<int>[] arrays)
+ {
+ // See https://github.com/dotnet/corefx/issues/23680
+
+ int[] results = arrays.SelectMany(ar => ar).ToArray();
+
+ for (int i = 0; i < results.Length; i++)
+ {
+ Assert.Equal(i, results[i]);
+ }
+ }
+
+ private static IEnumerable<object[]> GetToArrayDataSources()
+ {
+ // Marker at the end
+ yield return new object[]
+ {
+ new IEnumerable<int>[]
+ {
+ new TestEnumerable<int>(new int[] { 0 }),
+ new TestEnumerable<int>(new int[] { 1 }),
+ new TestEnumerable<int>(new int[] { 2 }),
+ new int[] { 3 },
+ }
+ };
+
+ // Marker at beginning
+ yield return new object[]
+ {
+ new IEnumerable<int>[]
+ {
+ new int[] { 0 },
+ new TestEnumerable<int>(new int[] { 1 }),
+ new TestEnumerable<int>(new int[] { 2 }),
+ new TestEnumerable<int>(new int[] { 3 }),
+ }
+ };
+
+ // Marker in middle
+ yield return new object[]
+ {
+ new IEnumerable<int>[]
+ {
+ new TestEnumerable<int>(new int[] { 0 }),
+ new int[] { 1 },
+ new TestEnumerable<int>(new int[] { 2 }),
+ }
+ };
+
+ // Non-marker in middle
+ yield return new object[]
+ {
+ new IEnumerable<int>[]
+ {
+ new int[] { 0 },
+ new TestEnumerable<int>(new int[] { 1 }),
+ new int[] { 2 },
+ }
+ };
+
+ // Big arrays (marker in middle)
+ yield return new object[]
+ {
+ new IEnumerable<int>[]
+ {
+ new TestEnumerable<int>(Enumerable.Range(0, 100).ToArray()),
+ Enumerable.Range(100, 100).ToArray(),
+ new TestEnumerable<int>(Enumerable.Range(200, 100).ToArray()),
+ }
+ };
+
+ // Big arrays (non-marker in middle)
+ yield return new object[]
+ {
+ new IEnumerable<int>[]
+ {
+ Enumerable.Range(0, 100).ToArray(),
+ new TestEnumerable<int>(Enumerable.Range(100, 100).ToArray()),
+ Enumerable.Range(200, 100).ToArray(),
+ }
+ };
+
+ // Interleaved (first marker)
+ yield return new object[]
+ {
+ new IEnumerable<int>[]
+ {
+ new int[] { 0 },
+ new TestEnumerable<int>(new int[] { 1 }),
+ new int[] { 2 },
+ new TestEnumerable<int>(new int[] { 3 }),
+ new int[] { 4 },
+ }
+ };
+
+ // Interleaved (first non-marker)
+ yield return new object[]
+ {
+ new IEnumerable<int>[]
+ {
+ new TestEnumerable<int>(new int[] { 0 }),
+ new int[] { 1 },
+ new TestEnumerable<int>(new int[] { 2 }),
+ new int[] { 3 },
+ new TestEnumerable<int>(new int[] { 4 }),
+ }
+ };
+ }
  }
 }