Skip to content

Build a query's final tuple or record in one Select - #20898

Open
xperiandri wants to merge 12 commits into
dotnet:mainfrom
xperiandri:fix/query-fuse-final-select
Open

xperiandri wants to merge 12 commits into
dotnet:mainfrom
xperiandri:fix/query-fuse-final-select

Conversation

@xperiandri

@xperiandri xperiandri commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #20894

A query ending in select (c.Id, c.Name) now gives the provider one Select(c => new Tuple(c.Id, c.Name)), as C# does, instead of a Select of an internal AnonymousObject followed by a second Select converting it.

Details

🤖 Generated with Claude Code

xperiandri and others added 3 commits November 1, 2026 22:03
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>
Copilot AI balanced review requested due to automatic review settings November 1, 2026 21:16
@xperiandri
xperiandri requested a review from a team as a code owner November 1, 2026 21:16
@github-actions

github-actions Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

✅ No release notes required

@xperiandri

Copy link
Copy Markdown
Contributor Author

Covered cases

EF Core SQLite translates 100 of 137 scenarios, 99 without this change; it handled the two-Select form already. New: a sorted nested query of records selected in a tuple.

With #20897 in as well, Cosmos translates a tuple or struct tuple selected after where, a record after sortBy, a nested tuple, a tuple of nine, and a tuple before distinct.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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>
@xperiandri
xperiandri force-pushed the fix/query-fuse-final-select branch from 5c2bae0 to f4f49c9 Compare November 1, 2026 22:52

@pcshrosbree pcshrosbree left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }) } throws Type mismatch when building 'args' on main and works here.
  • The added nested-sort scenario does not exercise the new IEnumerable coercion in TransInnerNoCheck. With the coercion disabled, a probe with the shape of the PR's test still passes, because the merged Select already handles that shape. With the coercion still disabled, select (x, query { for y in ys do sortBy y }) throws InvalidCastException. Adding take 2 after the outer select changes the error to Type 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.

T-Gro
T-Gro previously approved these changes Nov 2, 2026

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 🕵️ LGTM

@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Nov 2, 2026
@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label Nov 2, 2026
@T-Gro
T-Gro self-requested a review November 2, 2026 12:47
xperiandri and others added 2 commits November 2, 2026 14:24
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>
@xperiandri
xperiandri force-pushed the fix/query-fuse-final-select branch from f4f49c9 to c1cff07 Compare November 2, 2026 18:57
@T-Gro
T-Gro dismissed their stale review November 2, 2026 19:01

Dismissed: scalar nested-sort regression coverage, release-note work, and the workflow gate remain unresolved.

@xperiandri

Copy link
Copy Markdown
Contributor Author

Thanks @pcshrosbree. Changes since f4f49c9:

  • The nested sort crash, Type mismatch when building 'args' for select (x, query { for y in [20; 10] do sortBy y }), has a line in the release note.
  • a049dc5 adds the two scenarios you suggested, sorted nested query over a sequence selected in a tuple and the same with take. Both fail without the IEnumerable coercion in TransInnerNoCheck: the first with InvalidCastException, the second with Type mismatch when building 'args'.
  • c1cff07 keeps the lets of a record with fields written out of order in front of the construction when the final Select is fused. It does nothing on this branch alone. With Replace lets by their values in lambdas handed to a LINQ provider #20895 merged, the tests of record fields out of order fail without it, since the fused conversion read the body once for each field and evaluated each let that often.

The nested queries that still build an AnonymousObject, and the Select that join adds to copy it, stay as a follow-up.

@github-actions

This comment has been minimized.

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 🕵️ Re-reviewed all recent changes as of c1cff07; LGTM

xperiandri and others added 4 commits November 4, 2026 10:23
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>
xperiandri and others added 2 commits November 4, 2026 10:39
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>
@xperiandri
xperiandri force-pushed the fix/query-fuse-final-select branch from c1cff07 to c9fe40c Compare November 4, 2026 19:54
@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Build Failure Analysis

Summary - Both Windows NativeAOT variants fail during restore because the installed .NET 11 RC2 SDK implicitly selects vulnerable Microsoft.DotNet.ILCompiler 10.0.13 for the net10.0 test project.

Root cause: SDK-supplied NativeAOT pack triggers NU1903

All 14 supplied binlogs were queried. The classic-metadata and compressed-metadata restore legs fail identically; the other 12 builds succeed. The SDK's Microsoft.NETCoreSdk.BundledVersions.props pins ILCompilerPackVersion="10.0.13" for net10.0, and NativeAOT_Test.fsproj enables PublishAot and treats warnings as errors. This dependency is implicit and none of the PR's changed files controls its version.

Affected errors

  • tests/AheadOfTime/NativeAOT/NativeAOT_Test.fsproj - NU1903: Package 'Microsoft.DotNet.ILCompiler' 10.0.13 has a known high severity vulnerability (GHSA-p3jp-9v83-76w9) in 1_0 ... classic_metadata.
  • tests/AheadOfTime/NativeAOT/NativeAOT_Test.fsproj - the same NU1903 in 2_0 ... compressed_metadata.

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 - binlog_errors and binlog_overview completed for all 14 binlogs: 12 succeeded and 2 failed only at restore with NU1903. dotnet --version && dotnet restore tests/AheadOfTime/NativeAOT/NativeAOT_Test.fsproj --runtime win-x64 -p:TreatWarningsAsErrors=true --no-cache --force-evaluate exited 155 before restore because the required repository SDK is unavailable on this runner, so no replacement could be validated. No after-change command was run because no source change was made.

Build evidence
Variant Result Failed target/task Evidence
classic metadata Failed Restore / RestoreTask Microsoft.DotNet.ILCompiler 10.0.13, NU1903
compressed metadata Failed Restore / RestoreTask Microsoft.DotNet.ILCompiler 10.0.13, NU1903
Remaining 12 binlogs Succeeded None 0 errors each

The failing binlogs identify SDK 11.0.100-rc.2.26504.105; its bundled versions file selects ILCompiler 10.0.13 for net10.0. Analysis run: https://github.com/dotnet/fsharp/actions/runs/41916542875

Structured data:

{
  "artifact_kind": "analysis",
  "fix_delivery": "none",
  "fix_status": "blocked",
  "workflow_artifact": "build-failure-analysis"
}

Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · copilot · gpt56 · 121.3 AIC · ⌖ 12.1 AIC · ⊞ 23K | [History]( · ◷)

@pcshrosbree

Copy link
Copy Markdown

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 calls

At the top level, select { Second = c.Next(); First = c.Next() } produces the expected values and call count on the merged tree.

From reading the code, convertUnderLets moves the conversion under the lets only at the top of the select body. When a tuple item is a Let expression, it reaches the RecordConv case of ConvMutableToImmutable. That case reads each field with AnonymousObjectGet (mutExpr, i) on the whole Let, so the lets run once per field. Keeping a leading Let around the recursive conversion in ConvMutableToImmutable should address this nesting; I have not tested that change. Moving the lets to the top would change the evaluation order: they would run before the tuple's earlier items.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-reviewed PR reviewed by AI review council

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

4 participants