Simplify collection enumerators

[pentp]

Simplify tracking of enumerator states for "not started" & "ended".

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull Request Overview

This PR standardizes enumerator state tracking by replacing disparate sentinel values and logic with unified negative index conventions and centralized exception throwing via ThrowHelper.

• Consolidated GetInvalidOperationException_EnumCurrent to treat only -1 as "not started" and other negatives as "ended"
• Updated MoveNext/Current implementations across collection enumerators to use negative sentinels and ThrowHelper methods
• Simplified fields and removed legacy dummy objects in several enumerators

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs	Adjusted exception helper to use index == -1 for not started
src/libraries/System.Private.CoreLib/src/System/Collections/Generic/List.cs	Simplified MoveNext flow and replaced sentinel with -1
src/libraries/System.Private.CoreLib/src/System/Collections/Generic/HashSet.cs	Updated MoveNext and Current to use -1 sentinel
src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs	Simplified iteration logic and removed old sentinel scheme
src/libraries/System.Private.CoreLib/src/System/Collections/BitArray.cs	Converted _currentElement to _current and unified sentinel use
src/libraries/System.Private.CoreLib/src/System/Collections/ArrayList.cs	Removed dummy object, updated sentinel and exception logic
src/libraries/System.Private.CoreLib/src/System/CharEnumerator.cs	Aligned Current throw to use local index variable
src/libraries/System.Private.CoreLib/src/System/ArraySegment.cs	Updated MoveNext/Current to use unified negatives
src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs	Simplified MoveNext, Current, and removed old exception paths
src/libraries/System.Collections/src/System/Collections/Generic/SortedSet.cs	Reordered field grouping for _version
src/libraries/System.Collections/src/System/Collections/Generic/SortedList.cs	Updated sentinel logic in MoveNext and Current
src/libraries/System.Collections/src/System/Collections/Generic/PriorityQueue.cs	Removed outdated sentinel assignment in MoveNextRare
src/libraries/System.Collections/src/System/Collections/Generic/LinkedList.cs	Replaced index tracking with boolean _valid flag
src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs	Cleaned up GetEnumerator and DictionaryEnumerator usage

Comments suppressed due to low confidence (1)

[pentp]

There's one weird test case failing: System.Collections.ObjectModel.Tests.IListTestKeyedCollectionStringStringBadKey.EnumeratePastEndThenModify it goes through the enumerator to the end (MoveNext returns false), adds items into the list and then expects IEnumerator.Current not to throw but return the default value instead 🙄

[eiriktsarpalis]

"valid" is somewhat ambiguous. How about something like _hasCurrent?

[eiriktsarpalis]

There's one weird test case failing: System.Collections.ObjectModel.Tests.IListTestKeyedCollectionStringStringBadKey.EnumeratePastEndThenModify it goes through the enumerator to the end (MoveNext returns false), adds items into the list and then expects IEnumerator.Current not to throw but return the default value instead 🙄

I think that's concerning and we shouldn't be regressing behaviour unless there is good reason to do so. While there are merits in each the fixes individually, I think bundling them up together in one large set of changes without clear indication on what they are intended to improve is too risky particularly given we're very close to wrapping up .NET 10 development.

I would recommend starting small, updating at most 3 closely related and high-impact collections, including benchmarks demonstrating any performance benefits (or regressions!) that this change might introduce.

[pentp]

I'm pretty sure that the EnumeratePastEndThenModify test is actually incorrect in the sense that it's testing undefined behavior - it's enumerating an IEnumerable and then after it finishes validates that Current throws, but then it modifies the underlying collection and expects Current not to throw an exception but MoveNext to throw an exception instead.
@stephentoub you changed some of the enumerators recently in their undefined behavior part to throw less/different exceptions - could this test also be updated in a similar way?

[stephentoub]

Elsewhere such tests include "UndefinedBehavior" in their name, e.g.

runtime/src/libraries/Common/tests/System/Collections/IEnumerable.NonGeneric.Tests.cs

Line 315 in 757dcc2

public virtual void Enumerator_Current_BeforeFirstMoveNext_UndefinedBehavior(int count)

We have the tests so that we know when we're changing something, but we're ok accepting the change when it's worth it.

Eirik's points still stand, though.

[jeffhandley]

@pentp As @eiriktsarpalis recommended, it would be better to start with a smaller scope. If you'd like to keep this PR and scope it down, that is fine; or you could close this PR and open fresh, targeted PRs.

Thank you for the PR nonetheless.