copilot review addressed

Author: Thorium

docs/RELEASE_NOTES.md

@@ -2,7 +2,7 @@
 - Fix: parameters of a sub-query nested in another sub-query are bound (IN, EXISTS, UNION and the like)
 - Fix: groupBy keys with option or value-option columns, and with x.IsSome / x.IsNone in a tuple key
 - Fix: a grouped sub-query of IN / NOT IN that selects an aggregate selects it without the group keys, on both sides of a union too, #746
-- IN / NOT IN sub-queries with a projection computed in .NET (e.g. `g.Max(...) + 1L`) fail with an error instead of comparing against the uncomputed column
+- IN / NOT IN sub-queries with a projection computed in .NET (e.g. `g.Max(...) + 1L`, a narrowing conversion, a user function) fail with an error instead of comparing against the uncomputed column
 - Design time: a provider instance that goes away closes its connection instead of disposing it, as the cached types of other instances still use it ("Cannot access a disposed object")
 - PostgreSQL: identity columns count as generated, so the typed ``Create(...)`` no longer takes them. Breaking for code that passed a value to a BY DEFAULT identity column: use Create() and set it, or Create(data). A saved ContextSchemaPath cache keeps the old signature until regenerated.
 

src/SQLProvider.Common/SqlRuntime.QueryExpression.fs

@@ -632,15 +632,67 @@ module internal QueryExpressionTransformer =
 
         newProjection, projectionMap, groupProjectionMap
 
-    /// A projection that reads one selected column as it is: a column, an aggregate or a group key, through casts and
-    /// an option's Value. Anything else is computed in .NET after the rows are read.
+    /// Numeric conversions that keep every value of the source type.
+    let private wideningConversions =
+        let integralTo (from:Type) (targets:Type list) = targets |> List.map (fun t -> from, t)
+        [ yield! integralTo typeof<sbyte>  [typeof<int16>; typeof<int32>; typeof<int64>; typeof<single>; typeof<double>; typeof<decimal>]
+          yield! integralTo typeof<byte>   [typeof<int16>; typeof<uint16>; typeof<int32>; typeof<uint32>; typeof<int64>; typeof<uint64>; typeof<single>; typeof<double>; typeof<decimal>]
+          yield! integralTo typeof<int16>  [typeof<int32>; typeof<int64>; typeof<single>; typeof<double>; typeof<decimal>]
+          yield! integralTo typeof<uint16> [typeof<int32>; typeof<uint32>; typeof<int64>; typeof<uint64>; typeof<single>; typeof<double>; typeof<decimal>]
+          yield! integralTo typeof<int32>  [typeof<int64>; typeof<double>; typeof<decimal>]
+          yield! integralTo typeof<uint32> [typeof<int64>; typeof<uint64>; typeof<double>; typeof<decimal>]
+          yield! integralTo typeof<int64>  [typeof<decimal>]
+          yield! integralTo typeof<uint64> [typeof<decimal>]
+          typeof<single>, typeof<double> ]
+
+    /// A conversion that keeps the value: identity, Nullable wrapping, a reference cast, boxing or unboxing, an enum and
+    /// its underlying type, or a numeric widening. A user-defined conversion operator runs .NET code, so it doesn't.
+    let private isValuePreserving (u:UnaryExpression) =
+        let underlying (t:Type) = match Nullable.GetUnderlyingType t with null -> t | n -> n
+        let source, target = underlying u.Operand.Type, underlying u.Type
+        isNull u.Method
+        && (Type.(=)(source, target)
+            || not source.IsValueType || not target.IsValueType
+            || (source.IsEnum && Type.(=)(Enum.GetUnderlyingType source, target))
+            || (target.IsEnum && Type.(=)(Enum.GetUnderlyingType target, source))
+            || wideningConversions |> List.exists (fun (s, t) -> Type.(=)(s, source) && Type.(=)(t, target)))
+
+    /// The row a projection reads: its parameter, a tuple or anonymous-object item of it, or a joined table's row of
+    /// it, through casts.
+    let rec private isRowReference (e:Expression) =
+        match e with
+        | :? ParameterExpression -> true
+        | :? MethodCallExpression as c when Type.(=)(c.Method.DeclaringType, typeof<SqlEntity>) && (c.Method.Name = "GetSubTable" || c.Method.Name = "GetSubTableNullable") ->
+            not (isNull c.Object) && c.Arguments |> Seq.forall (fun a -> a :? ConstantExpression) && isRowReference c.Object
+        | :? UnaryExpression as u when (u.NodeType = ExpressionType.Convert || u.NodeType = ExpressionType.TypeAs) && isNull u.Method ->
+            isRowReference u.Operand
+        | :? MemberExpression as m when not (isNull m.Expression) ->
+            let owner = m.Member.DeclaringType
+            (Microsoft.FSharp.Reflection.FSharpType.IsTuple owner || owner.Name.StartsWith "AnonymousObject" || owner.Name.Contains "AnonymousType")
+            && isRowReference m.Expression
+        | _ -> false
+
+    /// A projection that reads one selected column as it is: a column, an aggregate or a group key, through
+    /// value-preserving casts and an option's Value. Anything else is computed in .NET after the rows are read.
     let rec private isColumnRead (e:Expression) =
         match e with
         | :? UnaryExpression as u when u.NodeType = ExpressionType.Convert || u.NodeType = ExpressionType.ConvertChecked || u.NodeType = ExpressionType.TypeAs ->
-            isColumnRead u.Operand
-        | :? MemberExpression as m when m.Member.Name = "Value" && not (isNull m.Expression) -> isColumnRead m.Expression
-        | :? MemberExpression as m when m.Member.Name = "Key" -> true
-        | :? MethodCallExpression as c -> c.Method.Name.StartsWith "GetColumn" || c.Method.Name.StartsWith "Aggregate"
+            isValuePreserving u && isColumnRead u.Operand
+        | :? MemberExpression as m when m.Member.Name = "Value" && not (isNull m.Expression) ->
+            (Utilities.isCOpt m.Member.DeclaringType || Utilities.isVOpt m.Member.DeclaringType || not (isNull (Nullable.GetUnderlyingType m.Expression.Type)))
+            && isColumnRead m.Expression
+        | :? MemberExpression as m when m.Member.Name = "Key" && not (isNull m.Expression) ->
+            let owner = m.Member.DeclaringType
+            (Utilities.isGrp owner || (owner.IsGenericType && Type.(=)(owner.GetGenericTypeDefinition(), typedefof<GroupResultItems<_,_>>)))
+            && isRowReference m.Expression
+        | :? MethodCallExpression as c when not (isNull c.Object) && c.Arguments |> Seq.forall (fun a -> a :? ConstantExpression) ->
+            let owner = c.Method.DeclaringType
+            if Type.(=)(owner, typeof<IColumnHolder>) then
+                (c.Method.Name = "GetColumn" || c.Method.Name = "GetColumnOption" || c.Method.Name = "GetColumnValueOption")
+                && isRowReference c.Object
+            else
+                owner.IsGenericType && Type.(=)(owner.GetGenericTypeDefinition(), typedefof<GroupResultItems<_,_>>)
+                && c.Method.Name.StartsWith "Aggregate" && isRowReference c.Object
         | _ -> false
 
     /// As convertExpression; `membershipSubquery` true is for a sub-query of IN / NOT IN, which may return one column

tests/SqlProvider.Tests/more/SubqueryCompositionTests.fs

@@ -569,6 +569,115 @@ let ``an IN sub-query calling an F# function the database cannot run is refused`
 
     StringAssert.StartsWith("IN / NOT IN needs a sub-query that selects one column", error.Message)
 
+[<Test; SqlCheck.NoSql>]
+let ``an IN sub-query converting an aggregate to a narrower number in .NET is refused``() =
+    let roundedAverages =
+        query {
+            for detail in dc.Main.OrderDetails do
+            groupBy detail.ProductId into g
+            select (int64 (g.Average(fun detail -> detail.UnitPrice)))
+        }
+
+    let error =
+        Assert.Throws<Exception>(fun () ->
+            query {
+                for product in dc.Main.Products do
+                where (roundedAverages.Contains product.ProductId)
+                select product.ProductId
+            } |> Seq.toArray |> ignore)
+
+    StringAssert.StartsWith("IN / NOT IN needs a sub-query that selects one column", error.Message)
+
+let private GetColumnDoubled (id: int64) = id * 2L
+
+[<Test; SqlCheck.NoSql>]
+let ``an IN sub-query calling a function named like a column read is refused``() =
+    let doubledIds =
+        query {
+            for order in dc.Main.Orders do
+            select (GetColumnDoubled order.OrderId)
+        }
+
+    let error =
+        Assert.Throws<Exception>(fun () ->
+            query {
+                for order in dc.Main.Orders do
+                where (doubledIds.Contains order.OrderId)
+                select order.OrderId
+            } |> Seq.toArray |> ignore)
+
+    StringAssert.StartsWith("IN / NOT IN needs a sub-query that selects one column", error.Message)
+
+type private DoubledId(id: int64) =
+    member _.Key = id * 2L
+
+[<Test; SqlCheck.NoSql>]
+let ``an IN sub-query reading a Key that is not a group key is refused``() =
+    let doubledIds =
+        query {
+            for order in dc.Main.Orders do
+            select (DoubledId(order.OrderId).Key)
+        }
+
+    let error =
+        Assert.Throws<Exception>(fun () ->
+            query {
+                for order in dc.Main.Orders do
+                where (doubledIds.Contains order.OrderId)
+                select order.OrderId
+            } |> Seq.toArray |> ignore)
+
+    StringAssert.StartsWith("IN / NOT IN needs a sub-query that selects one column", error.Message)
+
+[<Test; SqlCheck.ExpectSql("GROUP BY", "IN")>]
+let ``an IN sub-query selecting a group key still runs``() =
+    let popular =
+        query {
+            for detail in dc.Main.OrderDetails do
+            groupBy detail.ProductId into g
+            where (g.Count() > 40)
+            select g.Key
+        }
+
+    let result =
+        query {
+            for product in dc.Main.Products do
+            where (popular.Contains product.ProductId)
+            select product.ProductId
+        } |> Seq.toArray |> Array.sort
+
+    let expected =
+        query { for d in dc.Main.OrderDetails do select d.ProductId }
+        |> Seq.countBy id |> Seq.filter (fun (_, n) -> n > 40) |> Seq.map fst |> Seq.toArray |> Array.sort
+
+    Assert.IsNotEmpty expected
+    Assert.AreEqual(expected, result)
+
+[<Test; SqlCheck.ExpectSql("GROUP BY", "HAVING", "MAX(", "IN")>]
+let ``a grouped IN sub-query with a having filter selects only its aggregate``() =
+    let latestOfPopular =
+        query {
+            for detail in dc.Main.OrderDetails do
+            groupBy detail.ProductId into g
+            where (g.Count() > 40)
+            select (g.Max(fun detail -> detail.OrderId))
+        }
+
+    let result =
+        query {
+            for order in dc.Main.Orders do
+            where (latestOfPopular.Contains order.OrderId)
+            select order.OrderId
+        } |> Seq.toArray |> Array.sort
+
+    let expected =
+        query { for d in dc.Main.OrderDetails do select (d.ProductId, d.OrderId) }
+        |> Seq.groupBy fst |> Seq.filter (fun (_, rows) -> Seq.length rows > 40)
+        |> Seq.map (fun (_, rows) -> rows |> Seq.map snd |> Seq.max) |> Seq.distinct |> Seq.toArray |> Array.sort
+
+    Assert.IsNotEmpty expected
+    Assert.AreEqual(expected, result)
+
 [<Test; SqlCheck.ExpectSql("IN", "+ 1")>]
 let ``an IN sub-query with arithmetic the database computes still runs``() =
     let nextIds =