[WIP] Cleanup and almost make tests compile

Author: jamesqo

src/System.Linq/src/System/Linq/Skip.cs

@@ -152,10 +152,17 @@ private static IEnumerable<TSource> SkipLastIterator<TSource>(IEnumerable<TSourc
                 {
                     if (queue.Count == count)
                     {
-                        yield return queue.Dequeue();
+                        do
+                        {
+                            yield return queue.Dequeue();
+                            queue.Enqueue(e.Current);
+                        }
+                        while (e.MoveNext());
+                    }
+                    else
+                    {
+                        queue.Enqueue(e.Current);
                     }
-
-                    queue.Enqueue(e.Current);
                 }
             }
         }

src/System.Linq/tests/EnumerableTests.cs

@@ -285,29 +285,48 @@ protected static List<Func<IEnumerable<T>, IEnumerable<T>>> IdentityTransforms<T
                 e => e.Where(i => true)
             };
         }
-
-        protected class DelegateBasedEnumerator<T> : IEnumerator<T>
+        
+        protected sealed class DelegateIterator<TSource> : IEnumerable<TSource>, IEnumerator<TSource>
         {
-            public Func<bool> MoveNextWorker { get; set; }
-            public Func<T> CurrentWorker { get; set; }
-            public Action DisposeWorker { get; set; }
-            public Func<object> NonGenericCurrentWorker { get; set; }
-            public Action ResetWorker { get; set; }
-
-            public T Current => CurrentWorker();
-            public bool MoveNext() => MoveNextWorker();
-            public void Dispose() => DisposeWorker();
-            void IEnumerator.Reset() => ResetWorker();
-            object IEnumerator.Current => NonGenericCurrentWorker();
-        }
+            private readonly Func<IEnumerator<TSource>> _getEnumerator;
+            private readonly Func<bool> _moveNext;
+            private readonly Func<TSource> _current;
+            private readonly Func<IEnumerator> _explicitGetEnumerator;
+            private readonly Func<object> _explicitCurrent;
+            private readonly Action _reset;
+            private readonly Action _dispose;
+
+            public DelegateIterator(
+                Func<IEnumerator<TSource>> getEnumerator = null,
+                Func<bool> moveNext = null,
+                Func<TSource> current = null,
+                Func<IEnumerator> explicitGetEnumerator = null,
+                Func<object> explicitCurrent = null,
+                Action reset = null,
+                Action dispose = null)
+            {
+                _getEnumerator = getEnumerator ?? (() => { throw new NotImplementedException(); });
+                _moveNext = moveNext ?? (() => { throw new NotImplementedException(); });
+                _current = current ?? (() => { throw new NotImplementedException(); });
+                _explicitGetEnumerator = explicitGetEnumerator ?? (() => { throw new NotImplementedException(); });
+                _explicitCurrent = explicitCurrent ?? (() => { throw new NotImplementedException(); });
+                _reset = reset ?? (() => { throw new NotImplementedException(); });
+                _dispose = dispose ?? (() => { throw new NotImplementedException(); });
+            }
 
-        protected class DelegateBasedEnumerable<T> : IEnumerable<T>
-        {
-            public Func<IEnumerator<T>> GetEnumeratorWorker { get; set; }
-            public Func<IEnumerator> NonGenericGetEnumeratorWorker { get; set; }
+            public IEnumerator<TSource> GetEnumerator() => _getEnumerator();
 
-            public IEnumerator<T> GetEnumerator() => GetEnumeratorWorker();
-            IEnumerator IEnumerable.GetEnumerator() => NonGenericGetEnumeratorWorker();
+            public bool MoveNext() => _moveNext();
+
+            public TSource Current => _current();
+
+            IEnumerator IEnumerable.GetEnumerator() => _explicitGetEnumerator();
+
+            object IEnumerator.Current => _explicitCurrent();
+
+            void IEnumerator.Reset() => _reset();
+
+            void IDisposable.Dispose() => _dispose();
         }
     }
 }

src/System.Linq/tests/SelectManyTests.cs

@@ -409,32 +409,20 @@ public void DisposeAfterEnumeration(int sourceLength, int subLength)
             bool sourceDisposed = false;
             bool[] subCollectionDisposed = new bool[sourceLength];
 
-            var sourceEnumerator = new DelegateBasedEnumerator<int>
-            {
-                MoveNextWorker = () => ++sourceState <= sourceLength,
-                CurrentWorker = () => 0,
-                DisposeWorker = () => sourceDisposed = true
-            };
-
-            var source = new DelegateBasedEnumerable<int>
-            {
-                GetEnumeratorWorker = () => sourceEnumerator
-            };
+            DelegateIterator<int> source = null;
+            source = new DelegateIterator<int>(
+                getEnumerator: () => source,
+                moveNext: () => ++sourceState <= sourceLength,
+                current: () => 0,
+                dispose: () => sourceDisposed = true);
+            
+            DelegateIterator<int> subCollection = null;
+            subCollection = new DelegateIterator<int>(
+                getEnumerator: () => subCollection,
+                moveNext: () => ++subState[subIndex] <= subLength, // Return true `subLength` times.
+                current: () => subState[subIndex],
+                dispose: () => subCollectionDisposed[subIndex++] = true); // Record that Dispose was called, and move on to the next index.
 
-            var subEnumerator = new DelegateBasedEnumerator<int>
-            {
-                // MoveNext: Return true subLength times.
-                // Dispose: Record that Dispose was called & move to the next index.
-                MoveNextWorker = () => ++subState[subIndex] <= subLength,
-                CurrentWorker = () => subState[subIndex],
-                DisposeWorker = () => subCollectionDisposed[subIndex++] = true
-            };
-
-            var subCollection = new DelegateBasedEnumerable<int>
-            {
-                GetEnumeratorWorker = () => subEnumerator
-            };
-
             var iterator = source.SelectMany(_ => subCollection);
 
             int index = 0; // How much have we gone into the iterator?

src/System.Linq/tests/SkipLastTests.netcoreapp1.1.cs

@@ -4,83 +4,70 @@
 
 using System.Collections.Generic;
 using Xunit;
+using static System.Linq.Tests.SkipTakeData;
 
 namespace System.Linq.Tests
 {
     public class SkipLastTests : EnumerableTests
     {
         [Theory]
-        [MemberData(nameof(SkipLastData))]
+        [ClassData(typeof(SkipTakeData))]
         public void SkipLast(IEnumerable<int> source, int count)
         {
-            foreach (IEnumerable<int> equivalent in IdentityTransforms<int>().Select(t => t(source)))
+            Assert.All(IdentityTransforms<int>(), transform =>
             {
+                IEnumerable<int> equivalent = transform(source);
+
                 IEnumerable<int> expected = equivalent.Reverse().Skip(count).Reverse();
                 IEnumerable<int> actual = equivalent.SkipLast(count);
 
                 Assert.Equal(expected, actual);
-                Assert.Equal(expected.Count(), actual.Count()); // Count may be optimized. The above assert does not imply this will pass.
+                Assert.Equal(expected.Count(), actual.Count());
                 Assert.Equal(expected, actual.ToArray());
                 Assert.Equal(expected, actual.ToList());
 
                 Assert.Equal(expected.FirstOrDefault(), actual.FirstOrDefault());
                 Assert.Equal(expected.LastOrDefault(), actual.LastOrDefault());
 
-                // Likewise, ElementAt may be optimized.
-                int count = expected.Count();
-                for (int i = 0; i < count; i++)
+                Assert.All(Enumerable.Range(0, expected.Count()), index =>
                 {
-                    Assert.Equal(expected.ElementAt(i), actual.ElementAt(i));
-                }
+                    Assert.Equal(expected.ElementAt(index), actual.ElementAt(index));
+                });
 
                 Assert.Equal(0, actual.ElementAtOrDefault(-1));
                 Assert.Equal(0, actual.ElementAtOrDefault(actual.Count()));
-            }
-        }
-
-        public static IEnumerable<object[]> SkipLastData()
-        {
-            IEnumerable<int> counts = new[] { int.MinValue + 1, -1, 0, 1, 2, 3, 4, 5, int.MaxValue };
-            IEnumerable<IEnumerable<int>> enumerables = from count in counts
-                                                        select Enumerable.Range(count, Math.Min(100, Math.Abs(count)));
-            
-            return from count in counts
-                   from enumerable in enumerables
-                   select new object[] { count, enumerable };
+            });
         }
 
         [Theory]
-        [InlineData(1)]
-        [InlineData(3)]
-        [InlineData(10)]
+        [MemberData(nameof(EvaluationBehaviorData), MemberType = typeof(SkipTakeData))]
         public void EvaluationBehavior(int count)
         {
-            // We want to make sure no more than `count` items are ever evaluated
-            // ahead of the current position.
-            // As an example, if [0, 1, 2, 3, 4, 5].SkipLast(2) is called, then we
-            // should read in [0, 1, 2], yield 0, read in 3, yield 1, read in 4,
-            // yield 2, etc.
-
+            // We want to make sure no more than `count` items are ever evaluated ahead of the current position.
+            // As an example, if Enumerable.Range(1, 6).SkipLast(2) is called, then we should read in the first 3 items,
+            // yield 1, read in 4, yield 2, and so on.
             int index = 0;
+            int limit = count * 2;
+            
+            DelegateIterator<int> source = null;
+            source = new DelegateIterator<int>(
+                getEnumerator: () => source,
+                moveNext: () => ++index <= limit, // Stop once we go past the limit.
+                current: () => index, // Yield from 1 up to the limit, inclusive.
+                dispose: () => index = -1);
 
-            // Represents the range [1..2 * count].
-            var enumerable = EphemeralSequence(new DelegateBasedEnumerator<int>
-            {
-                MoveNextWorker = () => ++index <= 2 * count,
-                CurrentWorker = () => index
-            });
-
-            IEnumerator<int> iterator = enumerable.SkipLast(count).GetEnumerator();
-            Assert.Equal(0, index);
+            IEnumerator<int> iterator = source.SkipLast(count).GetEnumerator();
+            Assert.Equal(0, index); // Nothing should be done before MoveNext is called.
 
-            for (int i = 0; i < count; i++)
+            for (int i = 1; i <= count; i++)
             {
                 Assert.True(iterator.MoveNext());
                 Assert.Equal(i, iterator.Current);
                 Assert.Equal(count + i, index);
             }
 
             Assert.False(iterator.MoveNext());
+            Assert.Equal(-1, index);
         }
     }
 }

src/System.Linq/tests/SkipTakeData.cs

@@ -0,0 +1,36 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System.Collections;
+using System.Collections.Generic;
+
+namespace System.Linq.Tests
+{
+    public class SkipTakeData : IEnumerable<object[]>
+    {
+        private static IEnumerable<object[]> Data { get; } = CreateData();
+
+        private static IEnumerable<object[]> CreateData()
+        {
+            IEnumerable<int> sourceCounts = new[] { 1, 2, 3, 5, 8, 13, 55, 100, 250, 1000, 2500 };
+
+            IEnumerable<int> counts = new[] { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89, 100, 250, 500, 1000, 250000, int.MaxValue };
+            counts = counts.Concat(counts.Select(c => -c)).Append(0);
+
+            return from sourceCount in sourceCounts
+                   let source = Enumerable.Range(0, sourceCount)
+                   from count in counts
+                   select new object[] { source, count };
+        }
+
+        public IEnumerator<object[]> GetEnumerator() => Data.GetEnumerator();
+
+        IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
+
+        public static IEnumerable<object[]> EvaluationBehaviorData()
+        {
+            return Enumerable.Range(-1, 15).Select(count => new object[] { count });
+        }
+    }
+}

src/System.Linq/tests/System.Linq.Tests.csproj

@@ -59,6 +59,7 @@
     <Compile Include="Shuffler.cs" />
     <Compile Include="SingleOrDefaultTests.cs" />
     <Compile Include="SingleTests.cs" />
+    <Compile Include="SkipTakeData.cs" />
     <Compile Include="SkipTests.cs" />
     <Compile Include="SkipWhileTests.cs" />
     <Compile Include="SumTests.cs" />

src/System.Linq/tests/TakeLastTests.netcoreapp1.1.cs

@@ -4,80 +4,69 @@
 
 using System.Collections.Generic;
 using Xunit;
+using static System.Linq.Tests.SkipTakeData;
 
 namespace System.Linq.Tests
 {
     public class TakeLastTests : EnumerableTests
     {
+        // TODO: RunOnce tests. Same for SkipLast.
+
         [Theory]
-        [MemberData(nameof(TakeLastData))]
+        [ClassData(typeof(SkipTakeData))]
         public void TakeLast(IEnumerable<int> source, int count)
         {
-            foreach (IEnumerable<int> equivalent in IdentityTransforms<int>().Select(t => t(source)))
+            Assert.All(IdentityTransforms<int>(), transform =>
             {
+                IEnumerable<int> equivalent = transform(source);
+
                 IEnumerable<int> expected = equivalent.Reverse().Take(count).Reverse();
                 IEnumerable<int> actual = equivalent.TakeLast(count);
 
                 Assert.Equal(expected, actual);
-                Assert.Equal(expected.Count(), actual.Count()); // Count may be optimized. The above assert does not imply this will pass.
+                Assert.Equal(expected.Count(), actual.Count());
                 Assert.Equal(expected, actual.ToArray());
                 Assert.Equal(expected, actual.ToList());
 
                 Assert.Equal(expected.FirstOrDefault(), actual.FirstOrDefault());
                 Assert.Equal(expected.LastOrDefault(), actual.LastOrDefault());
 
-                // Likewise, ElementAt may be optimized.
-                int count = expected.Count();
-                for (int i = 0; i < count; i++)
+                Assert.All(Enumerable.Range(0, expected.Count()), index =>
                 {
-                    Assert.Equal(expected.ElementAt(i), actual.ElementAt(i));
-                }
+                    Assert.Equal(expected.ElementAt(index), actual.ElementAt(index));
+                });
 
                 Assert.Equal(0, actual.ElementAtOrDefault(-1));
                 Assert.Equal(0, actual.ElementAtOrDefault(actual.Count()));
-            }
-        }
-
-        public static IEnumerable<object[]> TakeLastData()
-        {
-            IEnumerable<int> counts = new[] { int.MinValue + 1, -1, 0, 1, 2, 3, 4, 5, int.MaxValue };
-            IEnumerable<IEnumerable<int>> enumerables = from count in counts
-                                                        select Enumerable.Range(count, Math.Min(100, Math.Abs(count)));
-            
-            return from count in counts
-                   from enumerable in enumerables
-                   select new object[] { count, enumerable };
+            });
         }
 
         [Theory]
-        [InlineData(1)]
-        [InlineData(3)]
-        [InlineData(10)]
+        [MemberData(nameof(EvaluationBehaviorData), MemberType = typeof(SkipTakeData))]
         public void EvaluationBehavior(int count)
         {
-            // After the first `MoveNext` call to TakeLast's enumerator,
-            // everything should be evaluated.
-
             int index = 0;
+            int limit = count * 2;
 
-            // Represents the range [1..2 * count].
-            var enumerable = EphemeralSequence(new DelegateBasedEnumerator<int>
-            {
-                MoveNextWorker = () => ++index <= 2 * count,
-                CurrentWorker = () => index
-            });
+            DelegateIterator<int> source = null;
+            source = new DelegateIterator<int>(
+                getEnumerator: () => source,
+                moveNext: () => ++index <= limit, // Stop once we go past the limit.
+                current: () => index, // Yield from 1 up to the limit, inclusive.
+                dispose: () => index = -1);
 
-            IEnumerator<int> iterator = enumerable.TakeLast(count).GetEnumerator();
-            Assert.Equal(0, index);
+            IEnumerator<int> iterator = source.TakeLast(count).GetEnumerator();
+            Assert.Equal(0, index); // Nothing should be done before MoveNext is called.
 
-            for (int i = 0; i < count; i++)
+            for (int i = 1; i <= count; i++)
             {
                 Assert.True(iterator.MoveNext());
                 Assert.Equal(count + i, iterator.Current);
-                Assert.Equal(2 * count, index);
+                Assert.Equal(limit, index); // After the first MoveNext call, everything should be evaluated.
             }
 
             Assert.False(iterator.MoveNext());
+            Assert.Equal(-1, index);
         }
     }
 }