From fb0345145082e7f7962d6ffa3f051deae7640164 Mon Sep 17 00:00:00 2001 From: Phillip Carter Date: Tue, 8 Oct 2019 10:44:16 -0700 Subject: [PATCH 1/8] Fix some issues in ValueNone implementation --- src/fsharp/FSharp.Core/prim-types.fs | 9 +++++---- .../FSharp.Core/Microsoft.FSharp.Core/OptionModule.fs | 5 +++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/fsharp/FSharp.Core/prim-types.fs b/src/fsharp/FSharp.Core/prim-types.fs index 542852e5da9..7d19271111b 100644 --- a/src/fsharp/FSharp.Core/prim-types.fs +++ b/src/fsharp/FSharp.Core/prim-types.fs @@ -2942,11 +2942,12 @@ namespace Microsoft.FSharp.Core [] member x.IsSome = match x with ValueSome _ -> true | _ -> false - static member op_Implicit (value) : 'T option = Some(value) + static member op_Implicit (value) : 'T voption = ValueSome(value) - override x.ToString() = - // x is non-null, hence ValueSome - "ValueSome("^anyToStringShowingNull x.Value^")" + override x.ToString() = + match x with + | ValueNone -> "ValueNone" + | ValueSome y -> anyToStringShowingNull y and 'T voption = ValueOption<'T> diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/OptionModule.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/OptionModule.fs index 3fb4395cd69..142dc689bca 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/OptionModule.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/OptionModule.fs @@ -222,6 +222,11 @@ type ValueOptionTests() = let assertWasNotCalledThunk () = raise (exn "Thunk should not have been called.") + [] + member _.``Null ValueOption gives a "ValueNone" when calling ToString`` () = + Assert.AreEqual("ValueNone", ValueNone.ToString()) + Assert.AreEqual("ValueNone", string ValueNone) + [] member this.ValueOptionBasics () = Assert.AreEqual((ValueNone: int voption), (ValueNone: int voption)) From 3068483524584874ac1b80ce9a06bd5d56bff32e Mon Sep 17 00:00:00 2001 From: Phillip Carter Date: Tue, 8 Oct 2019 11:04:34 -0700 Subject: [PATCH 2/8] More debug display --- src/fsharp/FSharp.Core/prim-types.fs | 14 ++++++++++++-- src/fsharp/FSharp.Core/prim-types.fsi | 5 +++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/fsharp/FSharp.Core/prim-types.fs b/src/fsharp/FSharp.Core/prim-types.fs index 7d19271111b..6528c9eab0f 100644 --- a/src/fsharp/FSharp.Core/prim-types.fs +++ b/src/fsharp/FSharp.Core/prim-types.fs @@ -2883,7 +2883,7 @@ namespace Microsoft.FSharp.Core //------------------------------------------------------------------------- [] - [] + [] [] [] [] @@ -2907,6 +2907,11 @@ namespace Microsoft.FSharp.Core static member Some (value) : 'T option = Some(value) static member op_Implicit (value) : 'T option = Some(value) + + member private x.DebugDisplay = + match x with + | None -> "None" + | Some x -> String.Format("ValueSome({0})", x.ToString()) override x.ToString() = // x is non-null, hence Some @@ -2924,7 +2929,7 @@ namespace Microsoft.FSharp.Core [] [] [] - [] + [] type ValueOption<'T> = | ValueNone : 'T voption | ValueSome : 'T -> 'T voption @@ -2943,6 +2948,11 @@ namespace Microsoft.FSharp.Core member x.IsSome = match x with ValueSome _ -> true | _ -> false static member op_Implicit (value) : 'T voption = ValueSome(value) + + member private x.DebugDisplay = + match x with + | ValueNone -> "ValueNone" + | ValueSome x -> String.Format("ValueSome({0})", x.ToString()) override x.ToString() = match x with diff --git a/src/fsharp/FSharp.Core/prim-types.fsi b/src/fsharp/FSharp.Core/prim-types.fsi index 7ba3b3ae4d4..4c3777bfb0b 100644 --- a/src/fsharp/FSharp.Core/prim-types.fsi +++ b/src/fsharp/FSharp.Core/prim-types.fsi @@ -1860,6 +1860,11 @@ namespace Microsoft.FSharp.Core /// Return 'true' if the value option is a 'ValueNone' value. member IsNone : bool + + /// Implicitly converts a value into an optional that is a 'ValueSome' value. + /// The input value + /// A voption representing the value. + static member op_Implicit : value:'T -> 'T voption /// The type of optional values, represented as structs. /// From f2e27f3949644097c47f6b3f735291d3869af356 Mon Sep 17 00:00:00 2001 From: Phillip Carter Date: Tue, 8 Oct 2019 11:06:44 -0700 Subject: [PATCH 3/8] Some for options in debug display --- src/fsharp/FSharp.Core/prim-types.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fsharp/FSharp.Core/prim-types.fs b/src/fsharp/FSharp.Core/prim-types.fs index 6528c9eab0f..19db08e0b7b 100644 --- a/src/fsharp/FSharp.Core/prim-types.fs +++ b/src/fsharp/FSharp.Core/prim-types.fs @@ -2911,7 +2911,7 @@ namespace Microsoft.FSharp.Core member private x.DebugDisplay = match x with | None -> "None" - | Some x -> String.Format("ValueSome({0})", x.ToString()) + | Some x -> String.Format("Some({0})", x.ToString()) override x.ToString() = // x is non-null, hence Some From 49cb15f1ec7ebdd9e999e673d1751934544b2a41 Mon Sep 17 00:00:00 2001 From: Phillip Carter Date: Tue, 8 Oct 2019 11:10:14 -0700 Subject: [PATCH 4/8] Apply suggestions from code review Co-Authored-By: Eugene Auduchinok --- src/fsharp/FSharp.Core/prim-types.fsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fsharp/FSharp.Core/prim-types.fsi b/src/fsharp/FSharp.Core/prim-types.fsi index 4c3777bfb0b..cd28701e67e 100644 --- a/src/fsharp/FSharp.Core/prim-types.fsi +++ b/src/fsharp/FSharp.Core/prim-types.fsi @@ -1864,7 +1864,7 @@ namespace Microsoft.FSharp.Core /// Implicitly converts a value into an optional that is a 'ValueSome' value. /// The input value /// A voption representing the value. - static member op_Implicit : value:'T -> 'T voption + static member op_Implicit: value: 'T -> 'T voption /// The type of optional values, represented as structs. /// From 06b1d9127c51c7add2388750ac9d6f76846e0087 Mon Sep 17 00:00:00 2001 From: Phillip Carter Date: Tue, 8 Oct 2019 11:18:00 -0700 Subject: [PATCH 5/8] Probably better stringing --- src/fsharp/FSharp.Core/prim-types.fs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/fsharp/FSharp.Core/prim-types.fs b/src/fsharp/FSharp.Core/prim-types.fs index 19db08e0b7b..664e1d4c02a 100644 --- a/src/fsharp/FSharp.Core/prim-types.fs +++ b/src/fsharp/FSharp.Core/prim-types.fs @@ -2911,7 +2911,7 @@ namespace Microsoft.FSharp.Core member private x.DebugDisplay = match x with | None -> "None" - | Some x -> String.Format("Some({0})", x.ToString()) + | Some _ -> String.Format("Some({0})", anyToStringShowingNull x.Value) override x.ToString() = // x is non-null, hence Some @@ -2952,12 +2952,12 @@ namespace Microsoft.FSharp.Core member private x.DebugDisplay = match x with | ValueNone -> "ValueNone" - | ValueSome x -> String.Format("ValueSome({0})", x.ToString()) + | ValueSome _ -> String.Format("ValueSome({0})", anyToStringShowingNull x.Value) override x.ToString() = match x with | ValueNone -> "ValueNone" - | ValueSome y -> anyToStringShowingNull y + | ValueSome _ -> anyToStringShowingNull x.Value and 'T voption = ValueOption<'T> From 14b23f8aff5ae1f1be2c39cb3440ac883c579db3 Mon Sep 17 00:00:00 2001 From: Phillip Carter Date: Tue, 8 Oct 2019 11:50:19 -0700 Subject: [PATCH 6/8] Add them baselines --- tests/FSharp.Core.UnitTests/SurfaceArea.coreclr.fs | 1 + tests/FSharp.Core.UnitTests/SurfaceArea.net40.fs | 1 + 2 files changed, 2 insertions(+) diff --git a/tests/FSharp.Core.UnitTests/SurfaceArea.coreclr.fs b/tests/FSharp.Core.UnitTests/SurfaceArea.coreclr.fs index 561871089f0..7e3d81efd29 100644 --- a/tests/FSharp.Core.UnitTests/SurfaceArea.coreclr.fs +++ b/tests/FSharp.Core.UnitTests/SurfaceArea.coreclr.fs @@ -1746,6 +1746,7 @@ Microsoft.FSharp.Core.FSharpValueOption`1[T]: Microsoft.FSharp.Core.FSharpValueO Microsoft.FSharp.Core.FSharpValueOption`1[T]: Microsoft.FSharp.Core.FSharpValueOption`1[T] ValueNone Microsoft.FSharp.Core.FSharpValueOption`1[T]: Microsoft.FSharp.Core.FSharpValueOption`1[T] get_None() Microsoft.FSharp.Core.FSharpValueOption`1[T]: Microsoft.FSharp.Core.FSharpValueOption`1[T] get_ValueNone() +Microsoft.FSharp.Core.FSharpValueOption`1[T]: Microsoft.FSharp.Core.FSharpValueOption`1[T] op_Implicit(T) Microsoft.FSharp.Core.FSharpValueOption`1[T]: System.String ToString() Microsoft.FSharp.Core.FSharpValueOption`1[T]: T Item Microsoft.FSharp.Core.FSharpValueOption`1[T]: T Value diff --git a/tests/FSharp.Core.UnitTests/SurfaceArea.net40.fs b/tests/FSharp.Core.UnitTests/SurfaceArea.net40.fs index 7a6f7f9d334..8499ecc0eba 100644 --- a/tests/FSharp.Core.UnitTests/SurfaceArea.net40.fs +++ b/tests/FSharp.Core.UnitTests/SurfaceArea.net40.fs @@ -1746,6 +1746,7 @@ Microsoft.FSharp.Core.FSharpValueOption`1[T]: Microsoft.FSharp.Core.FSharpValueO Microsoft.FSharp.Core.FSharpValueOption`1[T]: Microsoft.FSharp.Core.FSharpValueOption`1[T] ValueNone Microsoft.FSharp.Core.FSharpValueOption`1[T]: Microsoft.FSharp.Core.FSharpValueOption`1[T] get_None() Microsoft.FSharp.Core.FSharpValueOption`1[T]: Microsoft.FSharp.Core.FSharpValueOption`1[T] get_ValueNone() +Microsoft.FSharp.Core.FSharpValueOption`1[T]: Microsoft.FSharp.Core.FSharpValueOption`1[T] op_Implicit(T) Microsoft.FSharp.Core.FSharpValueOption`1[T]: System.String ToString() Microsoft.FSharp.Core.FSharpValueOption`1[T]: T Item Microsoft.FSharp.Core.FSharpValueOption`1[T]: T Value From 39419027236a05c7519cb68c72a81e8c6fbe4cce Mon Sep 17 00:00:00 2001 From: Phillip Carter Date: Fri, 11 Oct 2019 14:53:58 -0700 Subject: [PATCH 7/8] Add sprintfn tests --- .../FSharp.Core/Microsoft.FSharp.Core/OptionModule.fs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/OptionModule.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/OptionModule.fs index 142dc689bca..93bd919b035 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/OptionModule.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/OptionModule.fs @@ -223,9 +223,13 @@ type ValueOptionTests() = let assertWasNotCalledThunk () = raise (exn "Thunk should not have been called.") [] - member _.``Null ValueOption gives a "ValueNone" when calling ToString`` () = + member _.``ValueNone gives "ValueNone" when calling ToString`` () = Assert.AreEqual("ValueNone", ValueNone.ToString()) Assert.AreEqual("ValueNone", string ValueNone) + + member _.``ValueNone with sprintf`` () = + Assert.AreEqual("ValueNone", sprintf "%O" (ValueNone.ToString())) + Assert.AreEqual("ValueNone", sprintf "%A" ValueNone) [] member this.ValueOptionBasics () = From cbaab6b83402657b6504497888b81acda6a80740 Mon Sep 17 00:00:00 2001 From: Phillip Carter Date: Sat, 12 Oct 2019 09:28:14 -0700 Subject: [PATCH 8/8] Update OptionModule.fs --- .../FSharp.Core/Microsoft.FSharp.Core/OptionModule.fs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/OptionModule.fs b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/OptionModule.fs index 93bd919b035..eaebc3df744 100644 --- a/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/OptionModule.fs +++ b/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Core/OptionModule.fs @@ -227,6 +227,7 @@ type ValueOptionTests() = Assert.AreEqual("ValueNone", ValueNone.ToString()) Assert.AreEqual("ValueNone", string ValueNone) + [] member _.``ValueNone with sprintf`` () = Assert.AreEqual("ValueNone", sprintf "%O" (ValueNone.ToString())) Assert.AreEqual("ValueNone", sprintf "%A" ValueNone) @@ -453,4 +454,4 @@ type ValueOptionTests() = member this.MapBindEquivalenceProperties () = let fn x = x + 3 Assert.AreEqual(ValueOption.map fn ValueNone, ValueOption.bind (fn >> ValueSome) ValueNone) - Assert.AreEqual(ValueOption.map fn (ValueSome 5), ValueOption.bind (fn >> ValueSome) (ValueSome 5)) \ No newline at end of file + Assert.AreEqual(ValueOption.map fn (ValueSome 5), ValueOption.bind (fn >> ValueSome) (ValueSome 5))