dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.88k stars 783 forks source link

Query headOrDefault and exactlyOneOrDefault with a struct/tuple #3845

Open Thorium opened 6 years ago

Thorium commented 6 years ago

What should the following syntax do:

open System
open System.Linq

let myQueryable = [|(1,1); (2,2)|].AsQueryable()
let a, b =
    query {
        for x in myQueryable do
        where(x = (3,3))
        headOrDefault
    }

Currently it compiles, but produces a runtime System.NullReferenceException. A thing that we shouldn't have in FSharp, right?

Edit: The reason is...well, read the replies.

vasily-kirichenko commented 6 years ago

It's interesting that if you change it to

let x =
   query {
        for x in myQueryable do
        where(x = (3,3))
        headOrDefault
   }

then the error gone and x is null.

vasily-kirichenko commented 6 years ago

The reason is that there is no default value for a tuple / struct.

Wrong, there is a default value for any type.

vasily-kirichenko commented 6 years ago

It fails trying to deconstuc null tuple:

image

A minimal repro:

> let x, y: int * int = Unchecked.defaultof<_>;;
System.NullReferenceException: Object reference not set to an instance of an object.
   at <StartupCode$FSI_0003>.$FSI_0003.main@()
vasily-kirichenko commented 6 years ago

So I don't see why it's a bug.

vasily-kirichenko commented 6 years ago

note:

member __.HeadOrDefault (source:QuerySource<'T,'Q>) = Enumerable.FirstOrDefault source.Source
giuliohome commented 6 years ago

I guess that A1

open System
open System.Linq

let myQueryable1 = [|1; 2|].AsQueryable()
let myQueryable2 = [|4; 3; 2|].AsQueryable()
let z =
    query {
        for x in myQueryable1 do
        join y in myQueryable2 on (x=y)
        where(x = 2)

    }
    |> Seq.tryPick Some 
    |> function
    | Some x -> box x
    | None -> null

printfn "%A" z // (2,2)

and A0

open System
open System.Linq

let myQueryable1 = [|1; 2|].AsQueryable()
let myQueryable2 = [|4; 3|].AsQueryable()
let z =
    query {
        for x in myQueryable1 do
        join y in myQueryable2 on (x=y)
        where(x = 2)
    }  
    |> Seq.tryPick Some 
    |> function
    | Some x -> box x
    | None -> null

printfn "%A" z // <null>

should be the equivalent of B1

open System
open System.Linq

let myQueryable1 = [|1; 2|].AsQueryable()
let myQueryable2 = [|4; 3; 2|].AsQueryable()
let z =
    query {
        for x in myQueryable1 do
        join y in myQueryable2 on (x=y)
        where(x = 2)
        headOrDefault
    } 

printfn "%A" z // (2,2)

and B0

open System
open System.Linq

let myQueryable1 = [|1; 2|].AsQueryable()
let myQueryable2 = [|4; 3|].AsQueryable()
let z =
    query {
        for x in myQueryable1 do
        join y in myQueryable2 on (x=y)
        where(x = 2)
        headOrDefault
    } 

printfn "%A" z  // NullReferenceException

but the latter throws...

giuliohome commented 6 years ago

Notice also that C# equivalent does not throw

using System;
using System.Linq;

namespace CheckLinq
{
    class Program
    {
        static void Main(string[] args)
        {
            int[] xa = { 1, 2 };
            var xq = xa.AsQueryable();
            int[] ya = { 4, 3 }; //or {4,3,2} if you like to see a result which is not null
            var yq = ya.AsQueryable();
            var query = from x in xq
                        join y in yq
                        on x equals y
                        select new { x, y};
            var z = query.FirstOrDefault();
            Console.WriteLine("res: {0}", z);
            Console.ReadLine();
        }
    }
}
dsyme commented 6 years ago

I believe the underlying issue is that headOrDefault doesn't apply a constraint that the type admits the null literal, e.g. unlike this:

> let f (x : 'T when 'T : null) = x;;
val f : x:'T -> 'T when 'T : null

> f (1,2);;

  f (1,2);;
  ---^^^

stdin(10,4): error FS0001: The type '('a * 'b)' does not have 'null' as a proper
 value

In that case this would give a type error. I've forgotten why we didn't add this constraint - perhaps it was an oversight. It could be added now I think without breakin binary compat (though would break source that relied on this).

... then the error gone and x is null....

I'm not actually happy with this behavior. We should ideally be avoiding the construction of a null tuple value via this route. However it can be done and it would be a breaking change to change it now, so we will likely leave it like this.

giuliohome commented 6 years ago

cross-referenced here

Thorium commented 6 years ago

"or default" kind of hints about the null option...

giuliohome commented 6 years ago

... but one has to remember to box it so that it can be checked with isNull.

I think the more general question here is why a C# function returning System.Tuple<'a, 'b> interops and compiles back as an F# function returning ref tuple 'a * 'b ?

and as far as hints are concerned - since I'm afraid that nothing is going to change at core level - I'd suggest that the CRUD sample in the SQLProvider docs is very important and should be rewritten from

let updateEmployee (employee: Employee2) =
    let foundEmployee = query {
        for p in ctx.Public.Employee2 do
        where (p.Id = employee.Id)
        exactlyOneOrDefault
    }
    if not (isNull foundEmployee) then
        foundEmployee.FirstName <- employee.FirstName
        foundEmployee.LastName <- employee.LastName
    ctx.SubmitUpdates()

to

let updateEmployee (employee: Employee2) =
    let foundEmployeeMaybe = query {
        for p in ctx.Public.Employee2 do
        where (p.Id = employee.Id)
        select Some p
        exactlyOneOrDefault
    }
    match foundEmployeeMaybe with
    | Some foundEmployee ->
        foundEmployee.FirstName <- employee.FirstName
        foundEmployee.LastName <- employee.LastName
        ctx.SubmitUpdates()

which would lead to a more idiomatic syntax, readily extensible to the case of a join.

samuela commented 5 years ago

Could we just get this to return an Option? That would make things so much easier. The nulls all over don't seem very idiomatic.

jon-peel commented 6 months ago

Hello, what was the final outcome on this?

I am working with a similar query, and having a query return null when using F# makes it very difficult to work with. Is there any guide lines on using exactlyOneOrDefault/headOrDefault in a query expression

I have read though https://learn.microsoft.com/en-us/dotnet/fsharp/language-reference/query-expressions a number of but found nothing. If I try and Google the problem I am brought to this thread.