dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.12k stars 4.04k forks source link

Enumerators fails to set state to "after" in various scenarios observable with `GetEnumerator()` #76089

Open jcouv opened 1 week ago

jcouv commented 1 week ago

The spec states that we should set state of the enumerator object to after in various scenarios:

It is possible to observe whether the state was properly set to after by checking whether GetEnumerator() re-uses the current state machine instance or allocates a new one.

In the scenarios above, enumerators don't properly set the state to after.

The impact is not too significant as we're making a change to ensure that Dispose() does set the state to after in non-exceptional cases, and this is only affecting an optimization. So in most normal usages (ie. where the enumerator is only used once or where it is disposed without exception thrown before we call GetEnumerator() again) the behavior is fine.

Note: this issue is referenced in tests. It also affects VB

        [Fact]
        public void StateAfterMoveNext_YieldBreak()
        {
            // Verify GetEnumerator
            string src2 = """
var enumerable = C.Produce(true);
var enumerator = enumerable.GetEnumerator();

System.Console.Write(enumerator.MoveNext());
System.Console.Write(!object.ReferenceEquals(enumerable, enumerable.GetEnumerator()));

System.Console.Write(!enumerator.MoveNext());
System.Console.Write(object.ReferenceEquals(enumerable, enumerable.GetEnumerator()));

class C
{
    public static System.Collections.Generic.IEnumerable<int> Produce(bool b)
    {
        yield return 42;
        if (b) yield break;
        yield return 43;
    }
}
""";
            // We're not setting the state to "after"/"finished"
            CompileAndVerify(src2, expectedOutput: "TrueTrueTrueFalse").VerifyDiagnostics();
        }
jaredpar commented 1 day ago

@jcouv why did you put this in Backlog?

jcouv commented 1 day ago

@jaredpar My reasoning was: this is an old problem, it is only observable if you enumerate the same enumerator multiple times, and nobody reported it. So the impact seems limited.