Runtime: IEnumerator.Current does not throw after hitting the end of the collection.

[alexperovich]

This issue is in the List<T>.Enumerator class. When the collection is enumerated to then end and then modified, accessing IEnumerator.Current returns default(T) rather than throwing. According to the documentation it should throw.

Simple Test case:

var list = new List<int>{0,1};
var enumerator = ((IEnumerable)list).GetEnumerator();
while (enumerator.MoveNext()){}
list.Add(2);
Assert.Throws<InvalidOperationException>(() => enumerator.Current);

This test fails currently with enumerator.Current returning 0.

[Maxwe11]

MSDN:

If changes are made to the collection, such as adding, modifying, or deleting elements, the enumerator is irrecoverably invalidated and the next call to MoveNext or Reset throws an InvalidOperationException. If the collection is modified between MoveNext and Current, Current returns the element that it is set to, even if the enumerator is already invalidated.

So why it should throw?

[alexperovich]

The enumerator has already hit the end of the collection. So current is set to an invalid element. It should throw because it is not set to a valid element.
There is a dissagreement between the segment you quoted and the following.

Current also throws an exception if the last call to MoveNext returned false, which indicates the end of the collection.

My perspective is that Current has been set to a state that throws an exception. Modifying the collection shouldn't change that.

[KrzysztofCwalina]

This is a performance optimization. If we wanted Current to throw, we would have to do a check in get_Current. Such check would hurt performance in 99+% of cases where people use enumerators through foreach statement (which will never call Current past the end of the list).

If you are arguing that we should fix the docs, then I tend to agree. If you are arguing that we should add the check, then I am not sure it's a good tradeoff. It will be a significant perf hit in some scenarios.

[alexperovich]

We should probably fix the docs. Though for non generic IEnumerable we already check that the enumerator is not in the bounds of the list, and the non generic IEnumerator will almost never be used in foreach loops unless the developer explicitly casts to IEnumerable

[ellismg]

Let's just do the documentation update.

[joshfree]

@rpetrusha could you follow up here?

[JonHanna]

Just found this after opening an issue requesting the same doc changes. I think the docs on when IEnumerator<T>.Current is undefined are fine, so copying those to IEnumerator.Current would be good.

[karelz]

@alexperovich @ianhays @safern can you please list the affected MSDN docs and suggest text change? Thanks!

@mairaw will be able to help us get the docs changed.

[alexperovich]

The affected docs are IEnumerator.Current (https://msdn.microsoft.com/en-us/library/system.collections.ienumerator.current(v=vs.110).aspx) and IEnumerator<T>.Current (https://msdn.microsoft.com/en-us/library/58e146b7(v=vs.110).aspx)

I believe the consensus was that the "Remarks" section of IEnumerator<T>.Current be copied to the same section in the docs for IEnumerator.Current. For consistency we may want to copy the summary and property value descriptions as well. The description for IEnumerator.Current (The current element in the collection.) is not entirely accurate.

[karelz]

Thanks.
@mairaw assigned to you - please let us know if you need more details or if it is not clear. Thank you!

[mairaw]

Thanks @karelz. I've made the updates to https://msdn.microsoft.com/en-US/library/khfze649(VS.110).aspx. Let me know if it looks ok and we can close this issue.

[safern]

Thanks @mairaw looks good :)