Enumerators incorrectly allow continued enumerator after disposal

[jcouv]

The current implementation allows continuing the enumeration of an enumerator after its disposal:

// Prints: True42 disposed True43
// Expected: True42 disposed False42

var enumerator = C.GetEnumerator();

System.Console.Write(enumerator.MoveNext());
System.Console.Write(enumerator.Current);

enumerator.Dispose();
System.Console.Write(" disposed ");

System.Console.Write(enumerator.MoveNext());
System.Console.Write(enumerator.Current);

class C
{

    public static System.Collections.Generic.IEnumerator<int> GetEnumerator()
    {
        yield return 42;
        yield return 43;
    }
}

According to Spec for MoveNext and Dispose for enumerators, MoveNext should return false after disposal:

The precise action performed by MoveNext depends on the state of the enumerator object when MoveNext is invoked:
[...]
If the state of the enumerator object is after, invoking MoveNext returns false.

The Dispose method is used to clean up the iteration by bringing the enumerator object to the after state.

• If the state of the enumerator object is before, invoking Dispose changes the state to after.
• If the state of the enumerator object is running, the result of invoking Dispose is unspecified.
• If the state of the enumerator object is suspended, invoking Dispose:
  • Changes the state to running.
  • Executes any finally blocks as if the last executed yield return statement were a yield break statement. If this causes an exception to be thrown and propagated out of the iterator body, the state of the enumerator object is set to after and the exception is propagated to the caller of the Dispose method.
  • Changes the state to after.
• If the state of the enumerator object is after, invoking Dispose has no affect.

[jaredpar]

Do we have a driving customer scenario for breaking behavior here?

[jcouv]

@jaredpar This surfaced when the runtime repo tried to update their reference to roslyn. Since last week, we include logic to clear locals in the Dispose() method. This caused one or two runtime tests to crash due to NRE.

The options are:

1. let customers encounter such regressions and fix their code
2. revert the change to clear locals during disposal from last week
3. enforce that we cannot re-enter user code after disposal (ie. the proposed fix)

[Sergio0694]

Just curious, but could you explain this bit in the changelog?

"The state machine for enumerators incorrectly allowed resuming execution after the enumerator was disposed"

Why was that incorrect? The spec explicitly says doing so is UB, no? By eg. updating the logic to do manual cleanup from Dispose(), isn't that introducing overhead just to further allow code to take a dependency on an implementation detail for what is undefined behavior? 😅

[jcouv]

Why was that incorrect? The spec explicitly says doing so is UB, no?

The case that is unspecified behavior is when Dispose() is called on a state machine that is "running".
The scenario described in OP has a state machine that is "suspended" (it reached a yield return). In that case, the spec says the state should be updated to "after" by Dispose(). Then MoveNext() should return false from there on.