fsprojects / FSharpx.Extras

Functional programming and other utilities from the original "fsharpx" project
https://fsprojects.github.io/FSharpx.Extras/
The Unlicense
683 stars 146 forks source link

Add Option.ofUnchecked function #392

Closed Micha-kun closed 4 years ago

Micha-kun commented 4 years ago

I use this function vastly when getting data from external sources like DB. You can define a Record that, in F#, "cannot be null" but when you are getting data from DB using tools like Dapper, qhen you do an action like "FirstOrDefault" from a Record, when "Default", it generates a null Record. That null Record cannot be converted "easily" to option because compiler says that "Record = null" cannot compile. So it's required to check with Unchecked.defaultof to see if it's null (or box it to see if obj is null).

gdziadkiewicz commented 4 years ago

Could you also add code to the C# test project to show the expected use case in which C# generates F# type which shouldn't be null?

Micha-kun commented 4 years ago

You think it's needed for C#? For C#, the Option.ofObj it's enough because, in C#, that F# Record is already a class and can check with null easily. This function is primarily for F# usage.

gdziadkiewicz commented 4 years ago

You are right, I misread your code. What's better - Unchecked.defaultof or box?

gdziadkiewicz commented 4 years ago

The current implementation allows calling this function with value types/structs and the behavior for default values of those makes me scratch my head and ask the question if this is really what we want. See the code sample:

>     let ofUnchecked (x: 'a) =
-         match Unchecked.defaultof<'a> = x with
-         | true -> None
-         | false -> Some x;;
val ofUnchecked : x:'a -> 'a option when 'a : equality
> ofUnchecked (System.DateTime(1,1,1));;
val it : System.DateTime option = None
> ofUnchecked (0);;                     
val it : int option = None

Assuming that this is a bug, not a feature, I see two solutions.

  1. Use box - with box for structs ofUnchecked === Some
  2. constraint the function to only accept reference types as in
    let ofUnchecked (x: 'a when 'a : not struct) = ...

Assuming that this is a feature it should be explicitly documented in the docs string. Maybe

/// Option.ofUnchecked (Unchecked.defaultof<'a>()) === None

?

Assuming that the struct behaviour was a bug, another problematic part IMO is that equality constraint - it's dishonest, we don't really need equality, we just need null checking and in such terms, the solution with box is better. See sample code that presents code which we would probably like to compile :

> [<NoEquality;NoComparison>] 
- type Dog = {X:int};;       
type Dog =
  { X: int }
> ofUnchecked {X=42};;        
Option.fs(21,13): error FS0001: The type 'Dog' does not support the 'equality' constraint because it has the 'NoEquality' attribute

Let me know what is your opinion on those potential issues.

Micha-kun commented 4 years ago

Thnks for your insights! I thought about the "not struct" restriction but not cared enough 😅 But that equality think was really unexpected. And it's a really one good case. I'll check what can be done. About box vs Unchecked.default, well maybe the Unchecked.default is the one generating the Equality check problem...

gdziadkiewicz commented 4 years ago

I will release new version later today. Thank you very much for your contribution!

Micha-kun commented 4 years ago

You Welcome! It was a good learning exercise, thank you for your reviewing!

gdziadkiewicz commented 4 years ago

I just released new version 2.5.0