fsharp icon indicating copy to clipboard operation
fsharp copied to clipboard

ValueOption equality on expression trees causes crash

Open Thorium opened this issue 3 years ago • 8 comments

I'm trying to have an expression tree to compare a struct with the same item, and it fails?

open System
open System.Linq.Expressions
//let tester = Some "a" //works
//let tester = Guid.NewGuid() //works
let tester = ValueSome "b" //crash

let equalExpr = Expression.Equal(Expression.Constant(tester), Expression.Constant(tester))

//System.InvalidOperationException: The binary operator Equal is not defined for the types 
// 'Microsoft.FSharp.Core.FSharpValueOption`1[System.String]' and 
// 'Microsoft.FSharp.Core.FSharpValueOption`1[System.String]'.
//   at System.Linq.Expressions.Expression.GetEqualityComparisonOperator(ExpressionType binaryType, String opName, Expression left, Expression right, Boolean liftToNull)
//   at System.Linq.Expressions.Expression.Equal(Expression left, Expression right, Boolean liftToNull, MethodInfo method)

Thorium avatar Jun 23 '22 23:06 Thorium

Ok, I found a workaround that is enough good for me: I lie the types to the expression tree:

open System
open System.Linq.Expressions
//let tester = Some "a" //works
//let tester = Guid.NewGuid() //works
let tester = ValueSome "b"
let ty = typeof<obj>

let equalExpr = Expression.Equal(Expression.Constant(tester, ty), Expression.Constant(tester, ty))

Edit: Not very good solution if your expressions are not Constant. An example of full stack is here:

System.InvalidOperationException: The binary operator Equal is not defined for the types 'Microsoft.FSharp.Core.FSharpValueOption`1[System.String]' and 'Microsoft.FSharp.Core.FSharpValueOption`1[System.String]'.
   at System.Linq.Expressions.Expression.GetEqualityComparisonOperator(ExpressionType binaryType, String opName, Expression left, Expression right, Boolean liftToNull)
   at System.Linq.Expressions.Expression.Equal(Expression left, Expression right, Boolean liftToNull, MethodInfo method)
   at Microsoft.FSharp.Linq.RuntimeHelpers.LeafExpressionConverter.ConvExprToLinqInContext@364.Invoke(Tuple`2 tupledArg)
   at Microsoft.FSharp.Linq.RuntimeHelpers.LeafExpressionConverter.ConvExprToLinqInContext(ConvEnv env, FSharpExpr inp) in D:\a\_work\1\s\src\fsharp\FSharp.Core\Linq.fs:line 364
   at Microsoft.FSharp.Linq.RuntimeHelpers.LeafExpressionConverter.ConvExprToLinqInContext(ConvEnv env, FSharpExpr inp) in D:\a\_work\1\s\src\fsharp\FSharp.Core\Linq.fs:line 599
   at Microsoft.FSharp.Primitives.Basics.List.mapToFreshConsTail[a,b](FSharpList`1 cons, FSharpFunc`2 f, FSharpList`1 x) in D:\a\_work\1\s\src\fsharp\FSharp.Core\local.fs:line 237
   at Microsoft.FSharp.Primitives.Basics.List.map[T,TResult](FSharpFunc`2 mapping, FSharpList`1 x) in D:\a\_work\1\s\src\fsharp\FSharp.Core\local.fs:line 248
   at Microsoft.FSharp.Linq.RuntimeHelpers.LeafExpressionConverter.ConvExprsToLinq(ConvEnv env, FSharpList`1 es) in D:\a\_work\1\s\src\fsharp\FSharp.Core\Linq.fs:line 689
   at Microsoft.FSharp.Linq.RuntimeHelpers.LeafExpressionConverter.ConvExprToLinqInContext(ConvEnv env, FSharpExpr inp) in D:\a\_work\1\s\src\fsharp\FSharp.Core\Linq.fs:line 507
   at Microsoft.FSharp.Primitives.Basics.List.map[T,TResult](FSharpFunc`2 mapping, FSharpList`1 x) in D:\a\_work\1\s\src\fsharp\FSharp.Core\local.fs:line 246
   at Microsoft.FSharp.Linq.RuntimeHelpers.LeafExpressionConverter.ConvExprsToLinq(ConvEnv env, FSharpList`1 es) in D:\a\_work\1\s\src\fsharp\FSharp.Core\Linq.fs:line 689
   at Microsoft.FSharp.Linq.RuntimeHelpers.LeafExpressionConverter.ConvExprToLinqInContext(ConvEnv env, FSharpExpr inp) in D:\a\_work\1\s\src\fsharp\FSharp.Core\Linq.fs:line 507
   at Microsoft.FSharp.Linq.RuntimeHelpers.LeafExpressionConverter.ConvExprToLinqInContext(ConvEnv env, FSharpExpr inp) in D:\a\_work\1\s\src\fsharp\FSharp.Core\Linq.fs:line 599
   at Microsoft.FSharp.Linq.RuntimeHelpers.LeafExpressionConverter.EvaluateQuotation(FSharpExpr e) in D:\a\_work\1\s\src\fsharp\FSharp.Core\Linq.fs:line 713
   at Microsoft.FSharp.Linq.QueryModule.EvalNonNestedInner(CanEliminate canElim, FSharpExpr queryProducingSequence) in D:\a\_work\1\s\src\fsharp\FSharp.Core\Query.fs:line 1823
   at Microsoft.FSharp.Linq.QueryModule.clo@1926-18.Microsoft.FSharp.Linq.ForwardDeclarations.IQueryMethods.Execute[a,b](FSharpExpr`1 q) in D:\a\_work\1\s\src\fsharp\FSharp.Core\Query.fs:line 1928

Thorium avatar Jun 23 '22 23:06 Thorium

More practical example:

open System.Linq
let datasource1 = [1; 2].AsQueryable() // primary key
let datasource2 = [ValueSome 1; ValueSome 2].AsQueryable() //foreign key
query { 
   for d1 in datasource1 do
   for d2 in datasource2 do
   where (ValueSome(d1) = d2)
   select (d1, d2)
} |> Seq.toList;;

// System.InvalidOperationException: The binary operator Equal is not defined for the types 'Microsoft.FSharp.Core.FSharpValueOption`1[System.Int32]' and 'Microsoft.FSharp.Core.FSharpValueOption`1[System.Int32]

Thorium avatar Jun 24 '22 12:06 Thorium

The original issue can be reproduced with custom types:

open System
open System.Linq.Expressions

[<StructuralEquality; StructuralComparison>]
[<Struct>]
type Person(name:string, age:int) =
  member x.Name = name
  member x.Age = age

let a = Person("Joe", 42)
let a2 = Person("Joe", 42)
let equalExpr = Expression.Equal(Expression.Constant(a), Expression.Constant(a2))
// System.InvalidOperationException

In the case of LINQ it's often maintainability over performance, so I'd prefer some extra-boxing or whatever just to make things work correctly.

Edit: I move the another issue to separate issue

Thorium avatar Jul 12 '22 10:07 Thorium

For this you should provide the F# equality operator as a custom operator into the Expression.Equals constructor. F#'s equality is different from C#'s equality and System.Linq.Expressions uses C# equality by default.

Happypig375 avatar Jul 12 '22 13:07 Happypig375

let makeEq (left: System.Linq.Expressions.Expression) (right: System.Linq.Expressions.Expression) =
    System.Linq.Expressions.Expression.Equal(left, right, false, typeof<_ option>.Assembly.GetType("Microsoft.FSharp.Core.Operators").GetMethod("op_Equality").MakeGenericMethod left.Type)
let a = ValueSome 1
printfn $"🌄{System.Linq.Expressions.Expression.Lambda(makeEq (System.Linq.Expressions.Expression.Constant a) (System.Linq.Expressions.Expression.Constant a), [||]).Compile().DynamicInvoke[||]}"

Happypig375 avatar Jul 12 '22 13:07 Happypig375

This issue can be closed as by design

Happypig375 avatar Jul 12 '22 13:07 Happypig375

@Happypig375 will the Microsoft.FSharp.Linq.RuntimeHelpers.LeafExpressionConverter be using this custom operator constructor when you do F# Linq query?

Thorium avatar Jul 12 '22 13:07 Thorium

@Thorium Only after merging https://github.com/dotnet/fsharp/pull/11681. Currently, no.

Happypig375 avatar Jul 12 '22 14:07 Happypig375

https://github.com/dotnet/fsharp/pull/11681 is merged, this can be closed (by design, solution exists)

T-Gro avatar Dec 29 '22 13:12 T-Gro