Fix LINQ handling of iterator.Take(...).Last(...) #112680
Conversation
When the Take amount is larger than the number of elements in the source `Iterator<T>`, Last ends up throwing an exception and LastOrDefault ends up returning the default value, rather than returning the last value in the taken region. As part of fixing this, I sured up the tests to try to cover more such sequences of operations. In doing so, the tests got a lot slower, so I tracked down and fixed places where we were doing a lot of unnecessary work.
|
Tagging subscribers to this area: @dotnet/area-system-linq |
There was a problem hiding this comment.
PR Overview
This pull request fixes a regression in LINQ behavior for Iterator when using Take(...).Last(...), ensuring that when the requested take amount exceeds the available elements the last element is returned rather than throwing an exception. In addition, the PR consolidates and streamlines test coverage by replacing custom transform helpers with a unified CreateSources method and by refactoring various test cases for consistency and performance.
- Fixed comparison logic in the SkipTake optimizations.
- Updated IdentityTransforms to return a more flexible IEnumerable and added extra transforms.
- Consolidated test helper functions by replacing IdentityTransforms and GuaranteeNotIList with CreateSources.
- Refactored tests for TakeLast, SkipLast, Single, and other LINQ operators.
Changes
| File | Description |
|---|---|
| src/libraries/System.Linq/src/System/Linq/SkipTake.SpeedOpt.cs | Adjusted condition in the Take method to properly branch when count equals or exceeds _minIndexInclusive. |
| src/libraries/System.Linq/tests/EnumerableTests.cs | Updated IdentityTransforms with additional transforms and replaced custom empty literal usages. |
| src/libraries/System.Linq/tests/SelectManyTests.cs | Simplified the ParameterizedTests by using an internal loop rather than MemberData. |
| Other test files | Replaced redundant helpers (IdentityTransforms, GuaranteeNotIList) with CreateSources and refactored tests for clarity and performance. |
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/libraries/System.Linq/src/System/Linq/SkipTake.SpeedOpt.cs:429
- Changing the condition from '>= _minIndexInclusive' to '> _minIndexInclusive' affects the boundary behavior; please confirm that this change correctly handles the case when count equals _minIndexInclusive without introducing an off-by-one error.
count > _minIndexInclusive
Tip: Leave feedback on Copilot's review comments with the 👎 and 👍 buttons to help improve review quality. Learn more
| // another one that has equivalent contents. | ||
| var identityTransforms = IdentityTransforms<T>(); | ||
|
|
||
| // We run the transforms N^2 times, by testing all transforms |
There was a problem hiding this comment.
I'm puzzled by the purpose this method, it all seems entirely unnecessary. Am I missing something?
There was a problem hiding this comment.
The purpose of the method I deleted because it served no worthwhile purpose? :)
There was a problem hiding this comment.
Yeah. I'm legitimately curious about what motivated its inclusion back then.
There was a problem hiding this comment.
Misguided, I think. The stated intent is to validate that all transformations are equivalent to all other transformations, but that doesn't require N^2 tests, and we also don't need to test the test transformations themselves.
I guess it didn't hurt much when it was fast because there were only a few transformations, but my change causes N to grow from 13 to 1099, which means N^2 grows from 169 to 1,207,801... and that difference is very noticeable :)
| @@ -426,9 +426,12 @@ public override Iterator<TSource> Take(int count) | |||
| { | |||
| if (_source is Iterator<TSource> iterator && | |||
| iterator.GetCount(onlyIfCheap: true) is int count && | |||
| count >= _minIndexInclusive) | |||
| count > _minIndexInclusive) | |||
There was a problem hiding this comment.
Here we miss a fast path if count != -1 but count <= _minIndexInclusive.
There was a problem hiding this comment.
The purpose of this change is to fix a bug. I don't want to add additional optimizations as part of it. Please feel free to submit a follow-up PR after this goes in.
|
/backport to release/9.0-staging |
|
Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/13415170353 |
|
@stephentoub backporting to "release/9.0-staging" failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Fix LINQ handling of iterator.Take(...).Last(...)
Using index info to reconstruct a base tree...
M src/libraries/System.Linq/src/System/Linq/SkipTake.SpeedOpt.cs
M src/libraries/System.Linq/tests/AggregateByTests.cs
M src/libraries/System.Linq/tests/ChunkTests.cs
M src/libraries/System.Linq/tests/ConcatTests.cs
M src/libraries/System.Linq/tests/CountTests.cs
M src/libraries/System.Linq/tests/EnumerableTests.cs
M src/libraries/System.Linq/tests/SelectManyTests.cs
M src/libraries/System.Linq/tests/SingleOrDefaultTests.cs
M src/libraries/System.Linq/tests/SingleTests.cs
M src/libraries/System.Linq/tests/SkipLastTests.cs
M src/libraries/System.Linq/tests/SkipTests.cs
M src/libraries/System.Linq/tests/TakeLastTests.cs
M src/libraries/System.Linq/tests/TakeTests.cs
M src/libraries/System.Linq/tests/WhereTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Linq/tests/WhereTests.cs
Auto-merging src/libraries/System.Linq/tests/TakeTests.cs
CONFLICT (content): Merge conflict in src/libraries/System.Linq/tests/TakeTests.cs
Auto-merging src/libraries/System.Linq/tests/TakeLastTests.cs
Auto-merging src/libraries/System.Linq/tests/SkipTests.cs
CONFLICT (content): Merge conflict in src/libraries/System.Linq/tests/SkipTests.cs
Auto-merging src/libraries/System.Linq/tests/SkipLastTests.cs
Auto-merging src/libraries/System.Linq/tests/SingleTests.cs
CONFLICT (content): Merge conflict in src/libraries/System.Linq/tests/SingleTests.cs
Auto-merging src/libraries/System.Linq/tests/SingleOrDefaultTests.cs
CONFLICT (content): Merge conflict in src/libraries/System.Linq/tests/SingleOrDefaultTests.cs
Auto-merging src/libraries/System.Linq/tests/SelectManyTests.cs
CONFLICT (content): Merge conflict in src/libraries/System.Linq/tests/SelectManyTests.cs
Auto-merging src/libraries/System.Linq/tests/EnumerableTests.cs
CONFLICT (content): Merge conflict in src/libraries/System.Linq/tests/EnumerableTests.cs
Auto-merging src/libraries/System.Linq/tests/CountTests.cs
CONFLICT (content): Merge conflict in src/libraries/System.Linq/tests/CountTests.cs
Auto-merging src/libraries/System.Linq/tests/ConcatTests.cs
Auto-merging src/libraries/System.Linq/tests/ChunkTests.cs
Auto-merging src/libraries/System.Linq/tests/AggregateByTests.cs
CONFLICT (content): Merge conflict in src/libraries/System.Linq/tests/AggregateByTests.cs
Auto-merging src/libraries/System.Linq/src/System/Linq/SkipTake.SpeedOpt.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Fix LINQ handling of iterator.Take(...).Last(...)
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
* main: (27 commits) Fold null checks against known non-null values (dotnet#109164) JIT: Always track the context for late devirt (dotnet#112396) JIT: array allocation fixes (dotnet#112676) [H/3] Fix test closing connection too fast (dotnet#112691) Fix LINQ handling of iterator.Take(...).Last(...) (dotnet#112680) [browser][MT] move wasm MT CI legs to extra-platforms (dotnet#112690) JIT: Don't use `Compiler::compFloatingPointUsed` to check if FP kills are needed (dotnet#112668) [LoongArch64] Fix a typo within PR#112166. (dotnet#112672) Fix new EH hang on DebugBreak (dotnet#112640) Use encode callback instead of renting a buffer to write to in DSAKeyFormatHelper Move some links to other doc (dotnet#112574) Reflection-based XmlSerializer - Deserialize empty collections and allow for sub-types in collection items. (dotnet#111723) JIT: Use `fgCalledCount` for OSR method entry weight (dotnet#112662) Use Avx10.2 Instructions in Floating Point Conversions (dotnet#111775) Expose StressLog via CDAC and port StressLogAnalyzer to managed code (dotnet#104999) JIT: Use linear block order for MinOpts in LSRA (dotnet#108147) Update dependencies from https://github.com/dotnet/arcade build 20250213.2 (dotnet#112625) JIT: Clean up and optimize call arg lowering (dotnet#112639) Update dependencies from https://github.com/dotnet/emsdk build 20250217.1 (dotnet#112645) JIT: Support `FIELD_LIST` for returns (dotnet#112308) ...
When the Take amount is larger than the number of elements in the source `Iterator<T>`, Last ends up throwing an exception and LastOrDefault ends up returning the default value, rather than returning the last value in the taken region. As part of fixing this, I sured up the tests to try to cover more such sequences of operations. In doing so, the tests got a lot slower, so I tracked down and fixed places where we were doing a lot of unnecessary work.
When the Take amount is larger than the number of elements in the source `Iterator<T>`, Last ends up throwing an exception and LastOrDefault ends up returning the default value, rather than returning the last value in the taken region. As part of fixing this, I sured up the tests to try to cover more such sequences of operations. In doing so, the tests got a lot slower, so I tracked down and fixed places where we were doing a lot of unnecessary work.
When the
Takeamount is larger than the number of elements in the sourceIterator<T>,Lastends up throwing an exception andLastOrDefaultends up returning the default value, rather than returning the last value in the taken region. This is not an issue when the source is a not anIterator<T>, e.g. it doesn't affect cases where the source is anIList<T>or an arbitraryIEnumerable<T>. This is a regression in .NET 9.As part of fixing this, I sured up the tests to try to cover more such sequences of operations. In doing so, the tests got a lot slower, so I tracked down and fixed places where we were doing a lot of unnecessary work.