Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Commit 6d38f59

Browse files
OmarTawfikdanmoseley
OmarTawfik
authored and
danmoseley
committed
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
1 parent 4a8a1ab commit 6d38f59

File tree

4 files changed

+276
-20
lines changed

4 files changed

+276
-20
lines changed

‎src/Common/src/System/Collections/Generic/LargeArrayBuilder.cs‎

Lines changed: 40 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,20 @@ internal CopyPosition(int row, int column)
4242
/// </summary>
4343
internal int Column { get; }
4444

45+
/// <summary>
46+
/// If this position is at the end of the current buffer, returns the position
47+
/// at the start of the next buffer. Otherwise, returns this position.
48+
/// </summary>
49+
/// <param name="endColumn">The length of the current buffer.</param>
50+
public CopyPosition Normalize(int endColumn)
51+
{
52+
Debug.Assert(Column <= endColumn);
53+
54+
return Column == endColumn ?
55+
new CopyPosition(Row + 1, 0) :
56+
this;
57+
}
58+
4559
/// <summary>
4660
/// Gets a string suitable for display in the debugger.
4761
/// </summary>
@@ -186,7 +200,7 @@ public void CopyTo(T[] array, int arrayIndex, int count)
186200
arrayIndex += toCopy;
187201
}
188202
}
189-
203+
190204
/// <summary>
191205
/// Copies the contents of this builder to the specified array.
192206
/// </summary>
@@ -197,9 +211,10 @@ public void CopyTo(T[] array, int arrayIndex, int count)
197211
/// <returns>The position in this builder that was copied up to.</returns>
198212
public CopyPosition CopyTo(CopyPosition position, T[] array, int arrayIndex, int count)
199213
{
214+
Debug.Assert(array != null);
200215
Debug.Assert(arrayIndex >= 0);
201-
Debug.Assert(count >= 0 && count <= Count);
202-
Debug.Assert(array?.Length - arrayIndex >= count);
216+
Debug.Assert(count > 0 && count <= Count);
217+
Debug.Assert(array.Length - arrayIndex >= count);
203218

204219
// Go through each buffer, which contains one 'row' of items.
205220
// 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
216231
int row = position.Row;
217232
int column = position.Column;
218233

219-
for (; count > 0; row++, column = 0)
220-
{
221-
T[] buffer = GetBuffer(index: row);
234+
T[] buffer = GetBuffer(row);
235+
Debug.Assert(buffer.Length > column);
222236

223-
// During this iteration, copy until we satisfy `count` or reach the
224-
// end of the current buffer.
225-
int copyCount = Math.Min(buffer.Length, count);
237+
int copied = Math.Min(buffer.Length - column, count);
238+
Array.Copy(buffer, column, array, arrayIndex, copied);
226239

227-
if (copyCount > 0)
228-
{
229-
Array.Copy(buffer, column, array, arrayIndex, copyCount);
240+
arrayIndex += copied;
241+
count -= copied;
230242

231-
arrayIndex += copyCount;
232-
count -= copyCount;
233-
column += copyCount;
234-
}
243+
if (count == 0)
244+
{
245+
return new CopyPosition(row, column + copied).Normalize(buffer.Length);
235246
}
236247

237-
return new CopyPosition(row: row, column: column);
248+
do
249+
{
250+
buffer = GetBuffer(++row);
251+
Debug.Assert(buffer.Length > 0);
252+
253+
copied = Math.Min(buffer.Length, count);
254+
Array.Copy(buffer, 0, array, arrayIndex, copied);
255+
256+
arrayIndex += copied;
257+
count -= copied;
258+
} while (count > 0);
259+
260+
return new CopyPosition(row, copied).Normalize(buffer.Length);
238261
}
239262

240263
/// <summary>

‎src/Common/src/System/Collections/Generic/SparseArrayBuilder.cs‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -113,9 +113,10 @@ public SparseArrayBuilder(bool initialize)
113113
/// <param name="count">The number of items to copy.</param>
114114
public void CopyTo(T[] array, int arrayIndex, int count)
115115
{
116+
Debug.Assert(array != null);
116117
Debug.Assert(arrayIndex >= 0);
117118
Debug.Assert(count >= 0 && count <= Count);
118-
Debug.Assert(array?.Length - arrayIndex >= count);
119+
Debug.Assert(array.Length - arrayIndex >= count);
119120

120121
int copied = 0;
121122
var position = CopyPosition.Start;
@@ -149,8 +150,11 @@ public void CopyTo(T[] array, int arrayIndex, int count)
149150
count -= reservedCount;
150151
}
151152

152-
// Finish copying after the final marker.
153-
_builder.CopyTo(position, array, arrayIndex, count);
153+
if (count > 0)
154+
{
155+
// Finish copying after the final marker.
156+
_builder.CopyTo(position, array, arrayIndex, count);
157+
}
154158
}
155159

156160
/// <summary>

‎src/System.Linq/tests/ConcatTests.cs‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,5 +421,123 @@ public void GetEnumerableOfConcatCollectionChainFollowedByEnumerableNodeShouldBe
421421
Assert.Equal(0xf00, en.Current);
422422
}
423423
}
424+
425+
[Theory]
426+
[MemberData(nameof(GetToArrayDataSources))]
427+
public void CollectionInterleavedWithLazyEnumerables_ToArray(IEnumerable<int>[] arrays)
428+
{
429+
// See https://github.com/dotnet/corefx/issues/23680
430+
431+
IEnumerable<int> concats = arrays[0];
432+
433+
for (int i = 1; i < arrays.Length; i++)
434+
{
435+
concats = concats.Concat(arrays[i]);
436+
}
437+
438+
int[] results = concats.ToArray();
439+
440+
for (int i = 0; i < results.Length; i++)
441+
{
442+
Assert.Equal(i, results[i]);
443+
}
444+
}
445+
446+
private static IEnumerable<object[]> GetToArrayDataSources()
447+
{
448+
// Marker at the end
449+
yield return new object[]
450+
{
451+
new IEnumerable<int>[]
452+
{
453+
new TestEnumerable<int>(new int[] { 0 }),
454+
new TestEnumerable<int>(new int[] { 1 }),
455+
new TestEnumerable<int>(new int[] { 2 }),
456+
new int[] { 3 },
457+
}
458+
};
459+
460+
// Marker at beginning
461+
yield return new object[]
462+
{
463+
new IEnumerable<int>[]
464+
{
465+
new int[] { 0 },
466+
new TestEnumerable<int>(new int[] { 1 }),
467+
new TestEnumerable<int>(new int[] { 2 }),
468+
new TestEnumerable<int>(new int[] { 3 }),
469+
}
470+
};
471+
472+
// Marker in middle
473+
yield return new object[]
474+
{
475+
new IEnumerable<int>[]
476+
{
477+
new TestEnumerable<int>(new int[] { 0 }),
478+
new int[] { 1 },
479+
new TestEnumerable<int>(new int[] { 2 }),
480+
}
481+
};
482+
483+
// Non-marker in middle
484+
yield return new object[]
485+
{
486+
new IEnumerable<int>[]
487+
{
488+
new int[] { 0 },
489+
new TestEnumerable<int>(new int[] { 1 }),
490+
new int[] { 2 },
491+
}
492+
};
493+
494+
// Big arrays (marker in middle)
495+
yield return new object[]
496+
{
497+
new IEnumerable<int>[]
498+
{
499+
new TestEnumerable<int>(Enumerable.Range(0, 100).ToArray()),
500+
Enumerable.Range(100, 100).ToArray(),
501+
new TestEnumerable<int>(Enumerable.Range(200, 100).ToArray()),
502+
}
503+
};
504+
505+
// Big arrays (non-marker in middle)
506+
yield return new object[]
507+
{
508+
new IEnumerable<int>[]
509+
{
510+
Enumerable.Range(0, 100).ToArray(),
511+
new TestEnumerable<int>(Enumerable.Range(100, 100).ToArray()),
512+
Enumerable.Range(200, 100).ToArray(),
513+
}
514+
};
515+
516+
// Interleaved (first marker)
517+
yield return new object[]
518+
{
519+
new IEnumerable<int>[]
520+
{
521+
new int[] { 0 },
522+
new TestEnumerable<int>(new int[] { 1 }),
523+
new int[] { 2 },
524+
new TestEnumerable<int>(new int[] { 3 }),
525+
new int[] { 4 },
526+
}
527+
};
528+
529+
// Interleaved (first non-marker)
530+
yield return new object[]
531+
{
532+
new IEnumerable<int>[]
533+
{
534+
new TestEnumerable<int>(new int[] { 0 }),
535+
new int[] { 1 },
536+
new TestEnumerable<int>(new int[] { 2 }),
537+
new int[] { 3 },
538+
new TestEnumerable<int>(new int[] { 4 }),
539+
}
540+
};
541+
}
424542
}
425543
}

‎src/System.Linq/tests/SelectManyTests.cs‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -477,5 +477,116 @@ public void ThrowOverflowExceptionOnConstituentLargeCounts(int[] counts)
477477
IEnumerable<int> iterator = counts.SelectMany(c => Enumerable.Range(1, c));
478478
Assert.Throws<OverflowException>(() => iterator.Count());
479479
}
480+
481+
[Theory]
482+
[MemberData(nameof(GetToArrayDataSources))]
483+
public void CollectionInterleavedWithLazyEnumerables_ToArray(IEnumerable<int>[] arrays)
484+
{
485+
// See https://github.com/dotnet/corefx/issues/23680
486+
487+
int[] results = arrays.SelectMany(ar => ar).ToArray();
488+
489+
for (int i = 0; i < results.Length; i++)
490+
{
491+
Assert.Equal(i, results[i]);
492+
}
493+
}
494+
495+
private static IEnumerable<object[]> GetToArrayDataSources()
496+
{
497+
// Marker at the end
498+
yield return new object[]
499+
{
500+
new IEnumerable<int>[]
501+
{
502+
new TestEnumerable<int>(new int[] { 0 }),
503+
new TestEnumerable<int>(new int[] { 1 }),
504+
new TestEnumerable<int>(new int[] { 2 }),
505+
new int[] { 3 },
506+
}
507+
};
508+
509+
// Marker at beginning
510+
yield return new object[]
511+
{
512+
new IEnumerable<int>[]
513+
{
514+
new int[] { 0 },
515+
new TestEnumerable<int>(new int[] { 1 }),
516+
new TestEnumerable<int>(new int[] { 2 }),
517+
new TestEnumerable<int>(new int[] { 3 }),
518+
}
519+
};
520+
521+
// Marker in middle
522+
yield return new object[]
523+
{
524+
new IEnumerable<int>[]
525+
{
526+
new TestEnumerable<int>(new int[] { 0 }),
527+
new int[] { 1 },
528+
new TestEnumerable<int>(new int[] { 2 }),
529+
}
530+
};
531+
532+
// Non-marker in middle
533+
yield return new object[]
534+
{
535+
new IEnumerable<int>[]
536+
{
537+
new int[] { 0 },
538+
new TestEnumerable<int>(new int[] { 1 }),
539+
new int[] { 2 },
540+
}
541+
};
542+
543+
// Big arrays (marker in middle)
544+
yield return new object[]
545+
{
546+
new IEnumerable<int>[]
547+
{
548+
new TestEnumerable<int>(Enumerable.Range(0, 100).ToArray()),
549+
Enumerable.Range(100, 100).ToArray(),
550+
new TestEnumerable<int>(Enumerable.Range(200, 100).ToArray()),
551+
}
552+
};
553+
554+
// Big arrays (non-marker in middle)
555+
yield return new object[]
556+
{
557+
new IEnumerable<int>[]
558+
{
559+
Enumerable.Range(0, 100).ToArray(),
560+
new TestEnumerable<int>(Enumerable.Range(100, 100).ToArray()),
561+
Enumerable.Range(200, 100).ToArray(),
562+
}
563+
};
564+
565+
// Interleaved (first marker)
566+
yield return new object[]
567+
{
568+
new IEnumerable<int>[]
569+
{
570+
new int[] { 0 },
571+
new TestEnumerable<int>(new int[] { 1 }),
572+
new int[] { 2 },
573+
new TestEnumerable<int>(new int[] { 3 }),
574+
new int[] { 4 },
575+
}
576+
};
577+
578+
// Interleaved (first non-marker)
579+
yield return new object[]
580+
{
581+
new IEnumerable<int>[]
582+
{
583+
new TestEnumerable<int>(new int[] { 0 }),
584+
new int[] { 1 },
585+
new TestEnumerable<int>(new int[] { 2 }),
586+
new int[] { 3 },
587+
new TestEnumerable<int>(new int[] { 4 }),
588+
}
589+
};
590+
}
480591
}
481592
}

0 commit comments

Comments
 (0)