Fix IAsyncEnumerable SkipLast for count = 0

[thibault-reigner]

Bugfix

Fixes bug where calling SkipLast(0) over a custom sequence (not being a AsyncIteratorBase :

reactive/Ix.NET/Source/System.Linq.Async/System/Linq/Operators/SkipLast.cs

Line 36 in 85f1eb7

if (source is AsyncIteratorBase<TSource>)

)
would result in Corecalling Dequeue over an empty queue, as queue.Count == count (both value being equal to 0)

reactive/Ix.NET/Source/System.Linq.Async/System/Linq/Operators/SkipLast.cs

Line 58 in 85f1eb7

yield return queue.Dequeue();

As a fix, I rewrote the Core method using await foreach and matching its synchronous implementation for IEnumerable as this version, aside being easier to read, also handles this particular case.

reactive/Ix.NET/Source/System.Interactive/System/Linq/Operators/SkipLast.cs

Line 33 in 93386a9

private static IEnumerable<TSource> SkipLastCore<TSource>(this IEnumerable<TSource> source, int count)

I have also edited the unit test SkipLast_Zero_NoAlias so this one fails with current version of the code.

[danielcweber]

Could this, instead, throw early for invalid values of count resp. just return the identity enumerable for count == 0 ?

[thibault-reigner]

Could this, instead, throw early for invalid values of count resp. just return the identity enumerable for count == 0 ?

If I am not mistaken this is already handled in the SkipLast method itself, Core only being called if the value of count is valid, or if there is a need to really return a new enumerable as stated in the comment

reactive/Ix.NET/Source/System.Linq.Async/System/Linq/Operators/SkipLast.cs

Lines 34 to 35 in ff6e92a

	// Return source if not actually skipping, but only if it's a type from here, to avoid
	// issues if collections are used as keys or otherwise must not be aliased.

which the test

reactive/Ix.NET/Source/System.Linq.Async.Tests/System/Linq/Operators/SkipLast.cs

Line 67 in ff6e92a

public async Task SkipLast_Zero_NoAlias()

also enforces.

[danielcweber]

I see...so there is a case in which we have to return source unchanged but have to 'hide' the object identity. I wasn't aware of that.

I am thinking about not touching the implementation of SkipLast, but instead having a generic way to wrap source but keep its behaviour. AsAsyncEnumerable comes to mind, but that'll only return the object itself.

Is there more cases like this one, where Core is called only to hide source? If so, could you provide a more generic solution, like e.g. a Wrap method? I think that would be the cleanest solution because we wouldn't touch any implementations and correctness of the changes would be immediately obvious.

In any case, good find!

[thibault-reigner]

I see...so there is a case in which we have to return source unchanged but have to 'hide' the object identity. I wasn't aware of that.

I am thinking about not touching the implementation of SkipLast, but instead having a generic way to wrap source but keep its behaviour. AsAsyncEnumerable comes to mind, but that'll only return the object itself.

Is there more cases like this one, where Core is called only to hide source? If so, could you provide a more generic solution, like e.g. a Wrap method? I think that would be the cleanest solution because we wouldn't touch any implementations and correctness of the changes would be immediately obvious.

In any case, good find!

I see...so there is a case in which we have to return source unchanged but have to 'hide' the object identity. I wasn't aware of that.

I am thinking about not touching the implementation of SkipLast, but instead having a generic way to wrap source but keep its behaviour. AsAsyncEnumerable comes to mind, but that'll only return the object itself.

Is there more cases like this one, where Core is called only to hide source? If so, could you provide a more generic solution, like e.g. a Wrap method? I think that would be the cleanest solution because we wouldn't touch any implementations and correctness of the changes would be immediately obvious.

In any case, good find!

This seems to be a bit out of scope of this PR and quite some work, as it requires to search for such pattern in all extensions methods of IAsyncEnumerable.

Also, I noticed that there is an extensive use of GetAsyncEnumerator() + MoveNextAsync() instead of await foreach in this library, most likely due to the fact it predates the introduction of IAsyncEnumerable and C# 8 await foreach.
It led to extra complexity, and here bug, with no easy way to compare methods with their synchronous variations for IEnumerable (which here was bug-free), so maybe a transition would highlight some extra bugs but also potential factorization.

Do you think such rework would be worth it to spot places where some simplfying logic such as the Wrap you suggest are relevant ?

[thibault-reigner]

@danielcweber Requested changes were done in e818952