Skip to content

Add Option.ofUnchecked function - #392

Merged
gdziadkiewicz merged 3 commits into
fsprojects:masterfrom
Micha-kun:master
Jun 3, 2020
Merged

gdziadkiewicz merged 3 commits into
fsprojects:masterfrom
Micha-kun:master

Conversation

@Micha-kun

@Micha-kun Micha-kun commented Jun 1, 2020

Copy link
Copy Markdown
Contributor

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
gdziadkiewicz self-requested a review June 1, 2020 16:20
@gdziadkiewicz

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Collaborator

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

@gdziadkiewicz

gdziadkiewicz commented Jun 1, 2020

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Contributor Author

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...

Comment thread src/FSharpx.Extras/ComputationExpressions/Option.fs Outdated
@gdziadkiewicz
gdziadkiewicz merged commit c0c5b25 into fsprojects:master Jun 3, 2020
@gdziadkiewicz

Copy link
Copy Markdown
Collaborator

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

@Micha-kun

Copy link
Copy Markdown
Contributor Author

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

@gdziadkiewicz

Copy link
Copy Markdown
Collaborator

I just released new version 2.5.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants