Repository navigation
Build a query's final tuple or record in one Select - #20898
xperiandri wants to merge 12 commits into
Conversation
Every query test so far runs over AsQueryable() and compares rows, so a tree that only LINQ to Objects can run passes. These scenarios also check the tree: the query must stay on its provider, and must not contain what a C# expression lambda never does - a block, a parameter nothing binds, a call into FSharp.Core, a join nested in a collection selector, a member read from a tuple built without naming its members. A scenario names what is wrong with its tree today. The name has to be removed by the change that makes it untrue, because a scenario fails when its tree is cleaner than it says. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Selecting a struct tuple failed with "unreachable": the tuple was replaced by an anonymous object, but only reference tuples had an anonymous object type to be replaced by, and the way back always built a reference tuple. Both kinds now share the anonymous object of their arity, and the way back builds the kind the query asked for. Taking a struct tuple apart in a lambda failed with a tuple index out of range, because a struct tuple has fields where a reference tuple has properties. Fixes dotnet#15522 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
✅ No release notes required |
Covered casesEF Core SQLite translates 100 of 137 scenarios, 99 without this change; it handled the two- With #20897 in as well, Cosmos translates a tuple or struct tuple selected after |
There was a problem hiding this comment.
🟡 Changes recommended
The shape checks miss unnamed-projection member reads in nested queries, two-argument SelectMany, and inner Join inputs, allowing invalid query trees to pass validation.
3 open issues
What changed in this PR
This PR simplifies FSharp.Core query translation so a final tuple or record projection reaches the LINQ provider in one Select. It builds on #20894’s struct-tuple support and provider-shape tests; Cosmos translation also depends on #20897.
Changes:
- Construct final tuples and records directly, while preserving separate conversion when another operator follows.
- Correct type coercion for sorted sequence queries nested inside projections.
- Add query-result and expression-shape coverage, and document the behavior changes.
| File | Description |
|---|---|
| tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Linq/QueryStructureScenarios.fs | Covers query composition and final projection shapes. |
| tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Linq/QueryShapeTests.fs | Runs scenario groups as parameterized tests. |
| tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Linq/QueryScenarioDomain.fs | Supplies shared query entities and sample data. |
| tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Linq/QueryScenario.fs | Compares results, provider retention, and expression shapes. |
| tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Linq/QueryProviderShape.fs | Captures query trees and detects translation obstacles. |
| tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Linq/QueryProjectionScenarios.fs | Covers scalar, tuple, record, and object projections. |
| tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Linq/QueryOperatorScenarios.fs | Covers filtering, sorting, joins, and aggregate operators. |
| tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Linq/QueryMethodSyntaxScenarios.fs | Covers direct LINQ calls and composed projections. |
| tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Linq/QueryKeyScenarios.fs | Covers join and grouping key shapes. |
| tests/FSharp.Core.UnitTests/FSharp.Core.UnitTests.fsproj | Registers the query scenario infrastructure and tests. |
| src/FSharp.Core/QueryExtensions.fs | Simplifies anonymous-object reads and supports struct-tuple conversion. |
| src/FSharp.Core/Query.fs | Combines final projection conversion and coerces nested sequence types. |
| src/FSharp.Core/Linq.fs | Reads struct-tuple elements through fields. |
| docs/release-notes/PENDING_BREAKING_CHANGES.md | Documents the changed provider-visible projection shape. |
| docs/release-notes/.FSharp.Core/11.0.200.md | Records struct-tuple fixes and single-Select projections. |
Review effort: Balanced
The shape tests followed only the first source of each operator. Each lambda parameter is now tied to the sequence it stands for, so the check also covers the inner input of a join, the second parameter of a join result selector, a collection selector of SelectMany, an identity Select, a chain inside the lambda of another operator and a query captured from earlier. A value scenario now has to end in a Queryable operator that gives the value: a count made over the rows of the provider leaves it. The validator itself is tested on trees built by hand. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
5c2bae0 to
f4f49c9
Compare
pcshrosbree
left a comment
There was a problem hiding this comment.
At the current head, results and evaluation counts match #20894 for the query shapes that ran on both #20894 and this PR, and EF Core 10.0.14 returns the same rows after groupBy. Please document the nested-sort crash this also fixes and add tests for the new IEnumerable coercion.
- This also fixes a crash that the release note doesn't list. Over an
IQueryable,query { for x in xs do select (x, query { for y in [20; 10] do sortBy y }) }throwsType mismatch when building 'args'on main and works here. - The added nested-sort scenario does not exercise the new
IEnumerablecoercion inTransInnerNoCheck. With the coercion disabled, a probe with the shape of the PR's test still passes, because the mergedSelectalready handles that shape. With the coercion still disabled,select (x, query { for y in ys do sortBy y })throwsInvalidCastException. Addingtake 2after the outerselectchanges the error toType mismatch when building 'args'. Those two would make good scenarios.
A possible follow-up: nested queries still build an AnonymousObject and convert it in a separate Select, and join still adds a Select that copies one AnonymousObject into another before the final Select.
A value scenario was accepted when the last execution at the provider was an operator that gives a value. A count made by the client over rows taken from the provider passed whenever a function it ran asked the provider for a count of its own, because that count came last. Now every execution has to be such an operator, and so does the enumeration of a source taken as it is, which is recorded as an execution too. The violations of a tree are compared with every occurrence: a tree with one block was equal to a tree with two. The scenarios that have several of a kind are pinned with their number. The validator tests build a tree with an invoked lambda and one with an unbound parameter, which no scenario has, and the two client counts. The release note names the error a struct tuple gave on main. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…y what they hold The shape tests did not see an unnamed element that passes through a result selector giving one of its parameters back, in SelectMany, Join and Zip, and they skipped the element parameter of Aggregate together with the accumulated value. Both are checked now. A function with a seed takes the accumulated value and an element, and one without a seed takes two elements. A scenario compared an entity by the text it makes of itself, which names its key only: a result with the right key and the wrong name or age passed. An object of a class of the scenarios is now compared by every property that gives a plain value. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
f4f49c9 to
c1cff07
Compare
Dismissed: scalar nested-sort regression coverage, release-note work, and the workflow gate remain unresolved.
|
Thanks @pcshrosbree. Changes since f4f49c9:
The nested queries that still build an |
This comment has been minimized.
This comment has been minimized.
String.Contains(string, StringComparison) does not exist on .NET Framework, so the test project failed to compile for net472 and all the Windows legs of the CI went red. IndexOf with an ordinal comparison does the same on both. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Reflection promises no order of the properties of a class. On .NET Framework one scenario listed the properties of the first order it rendered in another order than those of the three after it, so the expected and the actual rows of the same orders differed as text. The names decide the order now. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
A query ending in select (c.Id, c.Name) reached the provider as two projections: a Select building an internal AnonymousObject, then a second Select turning it into the tuple the query returns. Records and nested tuples went the same way. The second projection is the only one a query that ends there needs, and a provider that does not know AnonymousObject (Cosmos rejects constructors it cannot map) or that the result is later composed over sees one operator more than C# would give it. When the last operator is such a select, the conversion back to the tuple or record is now folded into it, so the provider gets Select(c => new Tuple(c.Id, c.Name)), the shape C# produces. Queries that end in another operator keep the separate conversion. The folded select converts its own body, which for a nested query over a sequence that ends in a sort is an IOrderedEnumerable. Such a nested query is now handed on as the sequence it is used as, the way one over an IQueryable already was, which also lets a select of it in a tuple work where it failed on a type mismatch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
A nested query over a sequence that ends in a sort is an ordered sequence where the tuple it is selected in wants a sequence. Two scenarios cover it, alone and followed by take, which fail without the coercion to IEnumerable. The release note names the crash that this fixes. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The fused Select converts the body of its projection once for each part of the record or tuple, so a body that is some lets in front of the construction had those lets evaluated once for each part. A record with fields written out of order is such a body once the lets of the compiler are kept in front of it. The conversion goes under the lets now, and each is evaluated once. On its own this branch never sees such a body, so no test here can fail without the change. The tests of record fields written out of order fail on the merge of the two branches without it. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
c1cff07 to
c9fe40c
Compare
Build Failure AnalysisSummary - Both Windows NativeAOT variants fail during restore because the installed .NET 11 RC2 SDK implicitly selects vulnerable Root cause: SDK-supplied NativeAOT pack triggers NU1903All 14 supplied binlogs were queried. The classic-metadata and compressed-metadata restore legs fail identically; the other 12 builds succeed. The SDK's Affected errors
Fix attempt - blocked Delivery - none No code was published. Silencing NuGet audit or warning-as-error would weaken security policy, while overriding an SDK-bundled NativeAOT pack without an established compatible fixed version is not a safe change. The appropriate resolution is to consume an SDK servicing build that bundles a non-vulnerable net10 ILCompiler pack, then rerun the AOT legs. Checks - Build evidence
The failing binlogs identify SDK Structured data: {
"artifact_kind": "analysis",
"fix_delivery": "none",
"fix_status": "blocked",
"workflow_artifact": "build-failure-analysis"
}
|
|
I merged this PR's head (c9fe40c) with #20895's (a49f7d9) locally and built FSharp.Core. On the merged tree, a record whose fields are written in a different order from their declaration has its fields evaluated twice when it is an item of a selected tuple: type R = { First: int; Second: int }
// c.Next() counts its calls and returns the count so far
query { for x in [ 1; 2 ].AsQueryable() do select (x, { Second = c.Next(); First = c.Next() }) } |> Seq.toList
// merged: [(1, { First = 2; Second = 3 }); (2, { First = 6; Second = 7 })], 8 calls
// each PR alone, and the base: [(1, { First = 2; Second = 1 }); (2, { First = 4; Second = 3 })], 4 callsAt the top level, From reading the code, |

Stacked on #20894
A query ending in
select (c.Id, c.Name)now gives the provider oneSelect(c => new Tuple(c.Id, c.Name)), as C# does, instead of aSelectof an internalAnonymousObjectfollowed by a secondSelectconverting it.Details
Selectform. The gain is for providers that rejectAnonymousObject: Cosmos translates these queries once Name the members of tuples and records built in expression trees #20897 is in as well.🤖 Generated with Claude Code