Name the members of tuples and records built in expression trees

docs/release-notes/.FSharp.Core/11.0.200.md

@@ -6,5 +6,8 @@
 
 * Correct `FSharpValue.PreComputeTuplePropertyInfo` to report that struct tuples store elements in fields, not properties, and recommend supported tuple readers instead of reporting a valid index as out of range. ([Issue #20801](https://github.com/dotnet/fsharp/issues/20801), [PR #20815](https://github.com/dotnet/fsharp/pull/20815))
 * Quotation debug text from `Expr.ToString()`, `Expr.ToString(full)`, and `%A` now prints struct tuple construction as `NewStructTuple`; reference tuples remain `NewTuple`. ([Issue #20800](https://github.com/dotnet/fsharp/issues/20800), [PR #20814](https://github.com/dotnet/fsharp/pull/20814))
+* Fix struct tuples in query expressions: selecting one no longer fails with "unreachable", and the items of one that a later operator reads are taken from its fields instead of failing with `ArgumentException: Struct tuples store their elements in fields, not properties`. ([Issue #15522](https://github.com/dotnet/fsharp/issues/15522), [PR #20894](https://github.com/dotnet/fsharp/pull/20894))
+* Fix LINQ providers failing on queries composed over a tuple or record projection (for example the tuples of a nested query taken apart by the next `for`, or a record read by a later `Where`): tuple, struct tuple, record and anonymous record constructions now name the member each argument becomes, as C# anonymous types do. ([Issue #15133](https://github.com/dotnet/fsharp/issues/15133), [Issue #40](https://github.com/dotnet/fsharp/issues/40), [PR #20897](https://github.com/dotnet/fsharp/pull/20897))
+* Fix `distinct` after a member of a tuple or record that an earlier operator selected: `for (city, _) in (select (b.Address.City, b.Id)) do select city; distinct` returned a city once for each id, because the provider applied `DISTINCT` to both columns of the earlier projection, and now returns each city once. ([PR #20897](https://github.com/dotnet/fsharp/pull/20897))
 
 ### Changed

docs/release-notes/PENDING_BREAKING_CHANGES.md

@@ -52,6 +52,24 @@ a.Equals(b)  // Was: false, Now: true
 // New: if (node.NodeType == ExpressionType.ArrayIndex)
 ```
 
+### Tuple and record constructions name their members
+
+Constructing a tuple, struct tuple, record or anonymous record now gives a `NewExpression` whose `Members` say which property (or `ValueTuple` field) each argument becomes, as C# anonymous types do, so providers can bind a later read such as `t.Item1` or `r.Name` to the argument.
+
+```fsharp
+<@ fun (c: Customer) -> (c.Id, c.Name) @>
+// Was: new Tuple`2(c.Id, c.Name)
+// Now: new Tuple`2(Item1 = c.Id, Item2 = c.Name)
+```
+
+A record whose fields have no public getter is still built without `Members`.
+
+The SQL that a provider writes names its columns after the members (`SELECT "b"."Id" AS "Item1", ...`), which shows in tests that compare SQL text.
+
+A `groupBy` on an item of a tuple that an earlier operator selected, `for (city, id) in (select (b.Address.City, b.Id)) do groupBy city into g; select (g.Key, g.Count())`, failed in EF Core with "could not be translated" and now fails inside EF Core with `KeyNotFoundException: The given key 'Item1' was not present in the dictionary`. It translates together with the change to groupings in #20899.
+
+Together with the change to F# operators in #20900, a `leftOuterJoin` that follows two `for`s and a `join`, and tests the joined row with `isNull`, fails in EF Core 10 for SQL Server with "Nullable object must have a value": `for c in db.Customers do for o in c.Orders do join d in db.Customers on (c.City = d.City); leftOuterJoin p in db.Products on (d.Id = p.Id) into products; for p in products do select (c.Name, o.Id, d.Name, (if isNull p then "" else p.Name))`. EF Core translates it with either change alone.
+
 ---
 
 ## IQueryable Type Preservation 🟡

src/FSharp.Core/Linq.fs

@@ -78,6 +78,25 @@ module LeafExpressionConverter =
 
     let getNonNullableType typ = match Nullable.GetUnderlyingType typ with null -> typ | t -> t
 
+    let tupleMemberNames = [| "Item1"; "Item2"; "Item3"; "Item4"; "Item5"; "Item6"; "Item7"; "Rest" |]
+
+    /// The properties of a System.Tuple, or the fields of a System.ValueTuple, that its constructor arguments become.
+    let tupleMembers (tupleTy: Type) =
+        [| for name in tupleMemberNames.[.. tupleTy.GetGenericArguments().Length - 1] ->
+               if tupleTy.IsValueType then tupleTy.GetField name :> MemberInfo else tupleTy.GetProperty name :> MemberInfo |]
+
+    /// FSharp.Core's own AnonymousObject types, which the query translation builds, and which name their members
+    /// Item1, Item2 and so on
+    let isAnonymousObject (ty: Type) =
+        ty.IsGenericType
+        && obj.ReferenceEquals (ty.Assembly, typeof<unit>.Assembly)
+        && ty.Name.StartsWith ("AnonymousObject", StringComparison.Ordinal)
+
+    let hasPublicGetter (property: PropertyInfo) =
+        match property.GetGetMethod() with
+        | null -> false
+        | _ -> true
+
     // https://github.com/dotnet/runtime/blob/fa779e8cb2b5868a0ac2fd4215f39ffb91f0dab0/src/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs#L72
     /// Can LINQ Expressions' BinaryExpression's (Left/Right)Shift construct a SimpleBinaryExpression from the type in question? Otherwise, use the F# operator as the user-defined method.
     let isLinqExpressionsInteger typ =
@@ -475,7 +494,14 @@ module LeafExpressionConverter =
 
         | TupleGet(arg, n) ->
              let argP = ConvExprToLinqInContext env arg
-             let rec build ty argP n =
+             let rec build (ty: Type) argP n =
+                 // A struct tuple holds its elements in fields, which PreComputeTuplePropertyInfo cannot return
+                 if ty.IsValueType then
+                     if n < 7 then
+                         Expression.Field(argP, ty.GetField("Item" + string (n + 1))) |> asExpr
+                     else
+                         build (ty.GetGenericArguments().[7]) (Expression.Field(argP, ty.GetField "Rest") |> asExpr) (n - 7)
+                 else
                  match FSharpValue.PreComputeTuplePropertyInfo(ty, n) with
                  | propInfo, None ->
                      Expression.Property(argP, propInfo) |> asExpr
@@ -515,8 +541,15 @@ module LeafExpressionConverter =
             // Anonymous type initialization generated by LinqQueries
             | NewAnonymousObjectQ(ctor, args) ->
                 let argsR = ConvExprsToLinq env args
-                let props = ctor.DeclaringType.GetProperties()
-                Expression.New(ctor, argsR, [| for p in props -> (p :> MemberInfo) |]) |> asExpr
+                let anonTy = ctor.DeclaringType
+                // The arguments become the items in the order of their numbers. The constructor of any other type, which
+                // NewAnonymousObjectHelper can mark as well, keeps the properties its type has.
+                let members =
+                    if isAnonymousObject anonTy then
+                        [| for i in 1 .. args.Length -> anonTy.GetProperty("Item" + string i) :> MemberInfo |]
+                    else
+                        [| for p in anonTy.GetProperties() -> p :> MemberInfo |]
+                Expression.New(ctor, argsR, members) |> asExpr
 
             // Do the same thing as C# compiler for string addition
             | PlusQ (_, GenericArgs [|ty1; ty2; ty3|], [x1; x2])
@@ -700,7 +733,13 @@ module LeafExpressionConverter =
         // Expr.New*
         | NewRecord(recdTy, args) ->
             let ctorInfo = FSharpValue.PreComputeRecordConstructorInfo(recdTy, showAll)
-            Expression.New(ctorInfo, ConvExprsToLinq env args) |> asExpr
+            let argsP = ConvExprsToLinq env args
+            let fields = FSharpType.GetRecordFields(recdTy, showAll)
+            // Expression.New rejects a member it cannot read publicly, such as a field of a record with a private representation
+            if fields |> Array.forall hasPublicGetter then
+                Expression.New(ctorInfo, argsP, [| for field in fields -> field :> MemberInfo |]) |> asExpr
+            else
+                Expression.New(ctorInfo, argsP) |> asExpr
 
         | NewArray(ty, args) ->
             Expression.NewArrayInit(ty, ConvExprsToLinq env args) |> asExpr
@@ -750,10 +789,10 @@ module LeafExpressionConverter =
              let argsP = ConvExprsToLinq env args
              let rec build ty (argsP: Expression array) =
                  match FSharpValue.PreComputeTupleConstructorInfo ty with
-                 | ctorInfo, None -> Expression.New(ctorInfo, argsP) |> asExpr
+                 | ctorInfo, None -> Expression.New(ctorInfo, argsP, tupleMembers ty) |> asExpr
                  | ctorInfo, Some nestedTy ->
                      let n = ctorInfo.GetParameters().Length - 1
-                     Expression.New(ctorInfo, Array.append argsP.[0..n-1] [| build nestedTy argsP.[n..] |]) |> asExpr
+                     Expression.New(ctorInfo, Array.append argsP.[0..n-1] [| build nestedTy argsP.[n..] |], tupleMembers ty) |> asExpr
              build tupTy argsP
 
         | IfThenElse(g, t, e) ->

src/FSharp.Core/Query.fs

@@ -1156,8 +1156,9 @@ module Query =
     /// immutable-to-mutable conversion information, convert it back to an expression involving immutable tuples or records.
     let rec ConvMutableToImmutable conv mutExpr =
         match conv with
-        | TupleConv convs ->
-            Expr.NewTuple (convs |> List.mapi (fun i conv -> ConvMutableToImmutable conv (AnonymousObjectGet (mutExpr, i))))
+        | TupleConv (isStruct, convs) ->
+            let elements = convs |> List.mapi (fun i conv -> ConvMutableToImmutable conv (AnonymousObjectGet (mutExpr, i)))
+            if isStruct then Expr.NewStructTuple elements else Expr.NewTuple elements
         | RecordConv (typ, convs) ->
             Expr.NewRecord (typ, convs |> List.mapi (fun i conv -> ConvMutableToImmutable conv (AnonymousObjectGet (mutExpr, i))))
 

src/FSharp.Core/QueryExtensions.fs

@@ -108,14 +108,6 @@ module internal Adapters =
 
     let anonObjectTypes = tupleTypes |> Array.map snd
 
-    let tupleToAnonTypeMap =
-        let t = Dictionary<Type, Type>()
-
-        for k, v in tupleTypes do
-            t.[k] <- v
-
-        t
-
     let anonToTupleTypeMap =
         let t = Dictionary<Type, Type>()
 
@@ -173,18 +165,13 @@ module internal Adapters =
 
         walk i e e.Type
 
+    /// Reference and struct tuples of the same arity share one anonymous object type.
     let RewriteTupleType (ty: Type) conv =
-        // Tuples are generic, so lookup only for generic types
-        assert ty.IsGenericType
-        let generic = ty.GetGenericTypeDefinition()
-
-        match tupleToAnonTypeMap.TryGetValue generic with
-        | true, mutableTupleType ->
-            // Recursively transform type arguments
-            mutableTupleType.MakeGenericType(ty.GetGenericArguments() |> Array.toList |> conv |> Array.ofList)
-        | _ ->
-            assert false
-            failwith "unreachable"
+        assert (FSharpType.IsTuple ty)
+        let elementTypes = ty.GetGenericArguments()
+        let mutableTupleType = anonObjectTypes.[elementTypes.Length - 1]
+        // Recursively transform type arguments
+        mutableTupleType.MakeGenericType(elementTypes |> Array.toList |> conv |> Array.ofList)
 
     let (|RecordFieldGetSimplification|_|) (expr: Expr) =
         match expr with
@@ -206,7 +193,7 @@ module internal Adapters =
     /// that we can invert the process in final query results.
     [<NoComparison; NoEquality>]
     type ConversionDescription =
-        | TupleConv of ConversionDescription list
+        | TupleConv of isStruct: bool * ConversionDescription list
         | RecordConv of Type * ConversionDescription list
         | GroupingConv (* origKeyType: *) of Type (* origElemType: *) * Type * ConversionDescription
         | SeqConv of ConversionDescription
@@ -216,18 +203,18 @@ module internal Adapters =
     /// "yield" or "select", convert it to a type involving anonymous objects according to the conversion data.
     let rec ConvImmutableTypeToMutableType conv ty =
         match conv with
-        | TupleConv convs ->
+        | TupleConv(isStruct, convs) ->
             assert (FSharpType.IsTuple ty)
 
             match convs with
             | x1 :: x2 :: x3 :: x4 :: x5 :: x6 :: x7 :: x8 :: tail ->
-                let els = [ x1; x2; x3; x4; x5; x6; x7; TupleConv(x8 :: tail) ]
+                let els = [ x1; x2; x3; x4; x5; x6; x7; TupleConv(isStruct, x8 :: tail) ]
                 RewriteTupleType ty (List.map2 ConvImmutableTypeToMutableType els)
             | _ -> RewriteTupleType ty (List.map2 ConvImmutableTypeToMutableType convs)
         | RecordConv(_, convs) ->
             assert (isPartiallyImmutableRecord ty)
             let types = [| for f in FSharpType.GetRecordFields ty -> f.PropertyType |]
-            ConvImmutableTypeToMutableType (TupleConv convs) (FSharpType.MakeTupleType types)
+            ConvImmutableTypeToMutableType (TupleConv(false, convs)) (FSharpType.MakeTupleType types)
         | GroupingConv(_keyTy, _elemTy, conv) ->
             assert ty.IsGenericType
             assert (ty.GetGenericTypeDefinition() = typedefof<IGrouping<_, _>>)
@@ -324,7 +311,7 @@ module internal Adapters =
             let argExprsNow, argScripts =
                 exprs |> List.map (ProduceMoreMutables tipf) |> List.unzip
 
-            NewAnonymousObject argExprsNow, TupleConv argScripts
+            NewAnonymousObject argExprsNow, TupleConv(expr.Type.IsValueType, argScripts)
 
         // Replace immutable records by anonymous objects
         | Patterns.NewRecord(typ, args) when isPartiallyImmutableRecord typ ->

tests/FSharp.Core.UnitTests/FSharp.Core.UnitTests.fsproj

@@ -92,6 +92,16 @@
     <Compile Include="FSharp.Core\Microsoft.FSharp.Quotations\FSharpQuotations.fs" />
     <Compile Include="FSharp.Core\Microsoft.FSharp.Linq\NullableOperators.fs" />
     <Compile Include="FSharp.Core\Microsoft.FSharp.Linq\QueryTests.fs" />
+    <Compile Include="FSharp.Core\Microsoft.FSharp.Linq\QueryProviderShape.fs" />
+    <Compile Include="FSharp.Core\Microsoft.FSharp.Linq\QueryScenarioDomain.fs" />
+    <Compile Include="FSharp.Core\Microsoft.FSharp.Linq\QueryScenario.fs" />
+    <Compile Include="FSharp.Core\Microsoft.FSharp.Linq\QueryProjectionScenarios.fs" />
+    <Compile Include="FSharp.Core\Microsoft.FSharp.Linq\QueryOperatorScenarios.fs" />
+    <Compile Include="FSharp.Core\Microsoft.FSharp.Linq\QueryKeyScenarios.fs" />
+    <Compile Include="FSharp.Core\Microsoft.FSharp.Linq\QueryStructureScenarios.fs" />
+    <Compile Include="FSharp.Core\Microsoft.FSharp.Linq\QueryMethodSyntaxScenarios.fs" />
+    <Compile Include="FSharp.Core\Microsoft.FSharp.Linq\QueryShapeTests.fs" />
+    <Compile Include="FSharp.Core\Microsoft.FSharp.Linq\ExpressionNewMembersTests.fs" />
     <Compile Include="FSharp.Core\XmlDocumentationValidation.fs" />
     <Compile Include="Interop\CSharpCollectionExpressions.fs" />
     <Compile Include="StructTuples.fs" />

tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Linq/ExpressionNewMembersTests.fs

@@ -0,0 +1,85 @@
+// Copyright (c) Microsoft Corporation.  All Rights Reserved.  See License.txt in the project root for license information.
+
+module FSharp.Core.UnitTests.Linq.ExpressionNewMembersTests
+
+open System
+open System.Linq.Expressions
+open Xunit
+open Microsoft.FSharp.Linq.RuntimeHelpers
+
+type PrivateRepresentation = private { Hidden: int }
+
+type OnlyId(id: int) =
+    member _.Id = id
+
+let private construction (lambda: Expression<'T>) =
+    match lambda.Body with
+    | :? NewExpression as construction -> construction
+    | body -> failwith $"Expected a New, got {body}"
+
+let private memberNames (construction: NewExpression) =
+    [ for m in construction.Members -> m.Name ]
+
+[<Fact>]
+let ``Tuple construction names the property each argument becomes`` () =
+    let tuple = construction (LeafExpressionConverter.QuotationToLambdaExpression <@ Func<int, int * string>(fun x -> (x, "a")) @>)
+    Assert.Equal<string list>([ "Item1"; "Item2" ], memberNames tuple)
+    Assert.All(tuple.Members, fun m -> Assert.Equal(Reflection.MemberTypes.Property, m.MemberType))
+
+[<Fact>]
+let ``Struct tuple construction names the field each argument becomes`` () =
+    let tuple =
+        construction (LeafExpressionConverter.QuotationToLambdaExpression <@ Func<int, struct (int * string)>(fun x -> struct (x, "a")) @>)
+
+    Assert.Equal<string list>([ "Item1"; "Item2" ], memberNames tuple)
+    Assert.All(tuple.Members, fun m -> Assert.Equal(Reflection.MemberTypes.Field, m.MemberType))
+
+[<Fact>]
+let ``Tuple of nine names Rest and the members of the nested tuple`` () =
+    let tuple =
+        construction (
+            LeafExpressionConverter.QuotationToLambdaExpression
+                <@ Func<int, _>(fun x -> (x, x, x, x, x, x, x, x, x)) @>
+        )
+
+    Assert.Equal<string list>([ "Item1"; "Item2"; "Item3"; "Item4"; "Item5"; "Item6"; "Item7"; "Rest" ], memberNames tuple)
+
+    match tuple.Arguments.[7] with
+    | :? NewExpression as rest -> Assert.Equal<string list>([ "Item1"; "Item2" ], memberNames rest)
+    | rest -> Assert.Fail $"Expected Rest to be a New, got {rest}"
+
+[<Fact>]
+let ``Record construction names the field each argument becomes`` () =
+    let record =
+        construction (LeafExpressionConverter.QuotationToLambdaExpression <@ Func<Customer, _>(fun c -> { PersonName = c.Name; PersonAge = c.Age }) @>)
+
+    Assert.Equal<string list>([ "PersonName"; "PersonAge" ], memberNames record)
+
+[<Fact>]
+let ``Anonymous record construction names the field each argument becomes`` () =
+    let record =
+        construction (LeafExpressionConverter.QuotationToLambdaExpression <@ Func<Customer, _>(fun c -> {| Age = c.Age; Name = c.Name |}) @>)
+
+    Assert.Equal<string list>([ "Age"; "Name" ], memberNames record)
+
+[<Fact>]
+let ``Record whose fields cannot be read publicly is built without members`` () =
+    let record = construction (LeafExpressionConverter.QuotationToLambdaExpression <@ Func<int, _>(fun x -> { Hidden = x }) @>)
+    Assert.Null record.Members
+
+[<Fact>]
+let ``Constructor marked as an anonymous object keeps the properties of its type`` () =
+    let lambda =
+        LeafExpressionConverter.QuotationToLambdaExpression
+            <@ Func<int, OnlyId>(fun x -> LeafExpressionConverter.NewAnonymousObjectHelper(OnlyId x)) @>
+
+    Assert.Equal<string list>([ "Id" ], memberNames (construction lambda))
+    Assert.Equal(3, lambda.Compile().Invoke(3).Id)
+
+[<Fact>]
+let ``Anonymous object of the query translation names its items`` () =
+    let lambda =
+        LeafExpressionConverter.QuotationToLambdaExpression
+            <@ Func<int, _>(fun x -> LeafExpressionConverter.NewAnonymousObjectHelper(AnonymousObject<int, string>(x, "a"))) @>
+
+    Assert.Equal<string list>([ "Item1"; "Item2" ], memberNames (construction lambda))