TINKERPOP-3029 Fix enumeration for .NET 8 #2424
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.6-dev #2424 +/- ##
=============================================
+ Coverage 75.16% 75.17% +0.01%
- Complexity 12316 12322 +6
=============================================
Files 1057 1057
Lines 63470 63470
Branches 6936 6936
=============================================
+ Hits 47706 47716 +10
+ Misses 13191 13187 -4
+ Partials 2573 2567 -6 ☔ View full report in Codecov by Sentry. |
| if (_fetchedNext) return _nextAvailable; | ||
|
|
||
| if (!_nextAvailable || _nextAvailable && TraverserEnumerator.Current?.Bulk == 0) |
There was a problem hiding this comment.
can be simplified
| if (!_nextAvailable || _nextAvailable && TraverserEnumerator.Current?.Bulk == 0) | |
| if (!_nextAvailable || TraverserEnumerator.Current?.Bulk == 0) |
There was a problem hiding this comment.
This code will also throw an exception if called without first starting the iterator: You will need to track that the iterator has not been started.
private object GetCurrent()
{
// Use dynamic to object to prevent runtime dynamic conversion evaluation
return TraverserEnumerator.Current?.Object;
}
Please review the new .Net code here
There was a problem hiding this comment.
@EricSites sure, but DefaultTraversal is an IEnumerator itself so GetCurrent() just forwards to TraverserEnumerator.Current. If it's illegal to call TraverserEnumerator.Current before calling TraverserEnumerator.MoveNext(), then it's also illegal to call GetCurrent() before calling MoveNext().
We could of course use a flag to check whether enumeration has been started in GetCurrent() and return null otherwise, but I don't think it's a good idea to deviate from the default behavior of .NET enumerators.
Did you see GetCurrent() in any stack trace?
`IEnumerable<T>.Current` has been changed in .NET 8. Before .NET 8, it simply returned `null` if `MoveNext()` wasn't called first. With .NET 8 however it throws an exception. Since this has apparently already been undefined behavior before, we should fix this irrespective of .NET 8: dotnet/runtime#85243 (comment) The problem can be reproduced by executing the Gherkin tests with .NET without the change in `DefaultTraversal` (by changing the `TargetFramework` in `Gremlin.Net.IntegrationTest.csproj` to `net8.0`). .NET 8 needs to be installed for this of course.
FlorianHockmann
force-pushed
the
TINKERPOP-3029
branch
from
76df767 to
8fda42f
Compare
|
I added a CHANGELOG / upgrade docs entry and applied the suggestion by @vkagamlyk. |
|
thank you @FlorianHockmann! VOTE+1 |
|
VOTE +1 |
https://issues.apache.org/jira/browse/TINKERPOP-3029
IEnumerable<T>.Currenthas been changed in .NET 8. Before .NET 8, it simply returnednullifMoveNext()wasn't called first. With .NET 8 however it throws an exception.Since this has apparently already been undefined behavior before, we should fix this irrespective of .NET 8:
dotnet/runtime#85243 (comment)
The problem can be reproduced by executing the Gherkin tests with .NET without the change in
DefaultTraversal(by changing theTargetFrameworkinGremlin.Net.IntegrationTest.csprojtonet8.0). .NET 8 needs to be installed for this of course.VOTE +1