Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The new SZGenericArrayEnumerator unexpectedly throws errors for empty collections #94256

Closed
icom85 opened this issue Nov 1, 2023 · 8 comments

Comments

@icom85
Copy link
Member

icom85 commented Nov 1, 2023

Description

Hello! We encountered an issue related to the base SZGenericArrayEnumerator type and its Current position. This seems to be a breaking change as the issue got introduced in .NET 8 preview 2 - https://versionsof.net/core/8.0/8.0.0-preview.2/.

Reproduction Steps

Create an empty list like this:

 List<int> items = new List<int>();

Imagine that you have a more generic method accepting IEnumerable as a parameter and you call that method and pass the empty list. Inside the method you want to move and access the current item:

private void GetNext(IEnumerable<int> items)
{
    IEnumerator<int> enumerator = items.GetEnumerator();
    enumerator.MoveNext();
    int current = enumerator.Current;
}

If you check for the Current directly, you will get: System.InvalidOperationException: 'Enumeration has not started. Call MoveNext.'
If you call MoveNext() and then check the Current, you will get: System.InvalidOperationException: 'Enumeration already finished.'

The type of the enumerator is: SZGenericArrayEnumerator.
image

Prior to NET 8 preview 2, in the same setup you would get a System.Collections.Generic.List.Enumerator and you would not get errors, the Current would be 0.

It seems that the new and concrete implementation for the enumerator that is returned for the base types is causing the error. This may cause runtime issues with existing application if they are run in .net8.0.

Expected behavior

The Current should not throw.

Actual behavior

Errors are thrown.

Regression?

Yes, the behavior was different in .NET 8 preview 1 and before.

Known Workarounds

Use MoveNext and only access the Current item if the enumerator has successfully moved to the next position.

private void GetNext(IEnumerable<int> items)
{
    IEnumerator<int> enumerator = items.GetEnumerator();
    if (enumerator.MoveNext())
    {
        int current = enumerator.Current;
    }
}

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 1, 2023
@ghost
Copy link

ghost commented Nov 1, 2023

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

Issue Details

Description

Hello! We encountered an issue related to the base SZGenericArrayEnumerator type and its Current position. This seems to be a breaking change as the issue got introduced in .NET 8 preview 2 - https://versionsof.net/core/8.0/8.0.0-preview.2/.

Reproduction Steps

Create an empty list like this:

 List<int> items = new List<int>();

Imagine that you have a more generic method accepting IEnumerable as a parameter and you call that method and pass the empty list. Inside the method you want to move and access the current item:

private void EnumerateCollection(IEnumerable<int> items)
{
    IEnumerator<int> enumerator = items.GetEnumerator();
    enumerator.MoveNext();
    int current = enumerator.Current;
}

If you check for the Current directly, you will get: System.InvalidOperationException: 'Enumeration has not started. Call MoveNext.'
If you call MoveNext() and then check the Current, you will get: System.InvalidOperationException: 'Enumeration already finished.'

The type of the enumerator is: SZGenericArrayEnumerator.
image

Prior to NET 8 preview 2, in the same setup you would get a System.Collections.Generic.List.Enumerator and you would not get errors, the Current would be 0.

It seems that the new and concrete implementation for the enumerator that is returned for the base types is causing the error. This may cause runtime issues with existing application if they are run in .net8.0.

Expected behavior

The Current should not throw.

Actual behavior

Errors are thrown.

Regression?

Yes, the behavior was different in .NET 8 preview 1 and before.

Known Workarounds

Use MoveNext and only access the Current item if the enumerator has successfully moved to the next position.

private void EnumerateCollection(IEnumerable<int> items)
{
    IEnumerator<int> enumerator = items.GetEnumerator();
    if (enumerator.MoveNext())
    {
        int current = enumerator.Current;
    }
}

Configuration

No response

Other information

No response

Author: icom85
Assignees: -
Labels:

area-System.Linq

Milestone: -

@lilinus
Copy link
Contributor

lilinus commented Nov 1, 2023

I says in the documentation for IEnumerable that Current is undefined if:

The last call to MoveNext returned false, which indicates the end of the collection.

I wouldnt expect any particular behaivour where it explicitly says it is undefined?

@ghost
Copy link

ghost commented Nov 1, 2023

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

Issue Details

Description

Hello! We encountered an issue related to the base SZGenericArrayEnumerator type and its Current position. This seems to be a breaking change as the issue got introduced in .NET 8 preview 2 - https://versionsof.net/core/8.0/8.0.0-preview.2/.

Reproduction Steps

Create an empty list like this:

 List<int> items = new List<int>();

Imagine that you have a more generic method accepting IEnumerable as a parameter and you call that method and pass the empty list. Inside the method you want to move and access the current item:

private void GetNext(IEnumerable<int> items)
{
    IEnumerator<int> enumerator = items.GetEnumerator();
    enumerator.MoveNext();
    int current = enumerator.Current;
}

If you check for the Current directly, you will get: System.InvalidOperationException: 'Enumeration has not started. Call MoveNext.'
If you call MoveNext() and then check the Current, you will get: System.InvalidOperationException: 'Enumeration already finished.'

The type of the enumerator is: SZGenericArrayEnumerator.
image

Prior to NET 8 preview 2, in the same setup you would get a System.Collections.Generic.List.Enumerator and you would not get errors, the Current would be 0.

It seems that the new and concrete implementation for the enumerator that is returned for the base types is causing the error. This may cause runtime issues with existing application if they are run in .net8.0.

Expected behavior

The Current should not throw.

Actual behavior

Errors are thrown.

Regression?

Yes, the behavior was different in .NET 8 preview 1 and before.

Known Workarounds

Use MoveNext and only access the Current item if the enumerator has successfully moved to the next position.

private void GetNext(IEnumerable<int> items)
{
    IEnumerator<int> enumerator = items.GetEnumerator();
    if (enumerator.MoveNext())
    {
        int current = enumerator.Current;
    }
}

Configuration

No response

Other information

No response

Author: icom85
Assignees: -
Labels:

area-System.Runtime, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

This is still within contract for IEnumerator types. You're not supposed to evaluate Current unless the earlier call to MoveNext() has returned true.

@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Nov 1, 2023
@icom85
Copy link
Member Author

icom85 commented Nov 1, 2023

I agree that you should not evaluate Current unless, its MoveNext() is successful. Still, this is a breaking change compared to previous versions and it may break existing applications. In my opinion it should be listed and documented.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Nov 1, 2023

I wouldn't agree with that. Calling Current after MoveNext() returns false is undefined behavior and users shouldn't be relying on that.

Put differently, what is a valid return value for Current on an empty enumerator and what correct application would rely on that producing a result?

@icom85
Copy link
Member Author

icom85 commented Nov 1, 2023

Let's say that the undefined behavior has changed 🙂 I understand that you don't want to consider this an issue because of the incorrect usage. Just have in mind that many existing applications for one reason or another may depend on this undefined behavior and upgrading to .NET8 may cause issues.

@teo-tsirpanis teo-tsirpanis closed this as not planned Won't fix, can't repro, duplicate, stale Nov 1, 2023
@Joe4evr
Copy link
Contributor

Joe4evr commented Nov 2, 2023

Sounds like a skill issue. :trollface:

But for real, relying on Undefined Behavior is a programming cardinal sin and you should repent. If this were in C/C++ you could find the compiler rewriting your method eliding any and all evaluation, and even time-travel that result up to all its callers.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants