From f60396f599c3076d3d04fe475919b82d6ac5bc32 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sun, 8 Mar 2026 18:50:22 +0000 Subject: [PATCH 1/4] fix: PersistentHashMap.containsKey now returns true for null/None/unit values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ContainsKey was comparing the result of INode.find (which returns the value as obj) against null. This caused false negatives when the stored value is null at the .NET level — which happens for None, unit, and explicitly null values. Fix: use INode.tryFind (which already existed on all node types) in ContainsKey for both PersistentHashMap and TransientHashMap. tryFind returns an option, so Some null (i.e. key found, value is null) is correctly distinguished from None (key not found). Activate the two previously-pending tests and add three new tests covering None, unit, and mixed Some/None scenarios. Closes #85 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/FSharpx.Collections/PersistentHashMap.fs | 4 ++-- .../PersistentHashMapTest.fs | 24 ++++++++++++++++++- .../TransientHashMapTest.fs | 2 +- 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/FSharpx.Collections/PersistentHashMap.fs b/src/FSharpx.Collections/PersistentHashMap.fs index a41d598f..343550d8 100644 --- a/src/FSharpx.Collections/PersistentHashMap.fs +++ b/src/FSharpx.Collections/PersistentHashMap.fs @@ -698,7 +698,7 @@ type internal TransientHashMap<[] 'T, 'S when 'T: equalit member this.ContainsKey(key: 'T) = if key = Unchecked.defaultof<'T> then this.hasNull else if this.root = Unchecked.defaultof then false - else this.root.find(0, hash(key), key) <> null + else this.root.tryFind(0, hash(key), key).IsSome member this.Add(key: 'T, value: 'S) = if key = Unchecked.defaultof<'T> then @@ -793,7 +793,7 @@ and PersistentHashMap<[] 'T, 'S when 'T: equality and 'S: member this.ContainsKey(key: 'T) = if key = Unchecked.defaultof<'T> then this.hasNull else if this.root = Unchecked.defaultof then false - else this.root.find(0, hash(key), key) <> null + else this.root.tryFind(0, hash(key), key).IsSome static member ofSeq(items: ('T * 'S) seq) = let mutable ret = TransientHashMap<'T, 'S>.Empty() diff --git a/tests/FSharpx.Collections.Tests/PersistentHashMapTest.fs b/tests/FSharpx.Collections.Tests/PersistentHashMapTest.fs index f43a614e..7c762cfd 100644 --- a/tests/FSharpx.Collections.Tests/PersistentHashMapTest.fs +++ b/tests/FSharpx.Collections.Tests/PersistentHashMapTest.fs @@ -40,12 +40,34 @@ module PersistentHashMapTests = } //https://github.com/fsprojects/FSharpx.Collections/issues/85 - ptest "can add None value to empty map" { + test "containsKey returns true for None value" { let x = PersistentHashMap.Empty() Expect.isTrue "PersistentHashMap.containsKey" (x.Add("Hello", None) |> PersistentHashMap.containsKey "Hello") } + test "containsKey returns true for unit value" { + let map = + PersistentHashMap.empty + |> PersistentHashMap.add "key" () + + Expect.isTrue "containsKey with unit value" (PersistentHashMap.containsKey "key" map) + Expect.isFalse "containsKey absent key" (PersistentHashMap.containsKey "other" map) + } + + test "containsKey returns true for None value after multiple adds" { + let map = + PersistentHashMap.empty + |> PersistentHashMap.add "a" (None: string option) + |> PersistentHashMap.add "b" (Some "hello") + |> PersistentHashMap.add "c" None + + Expect.isTrue "key a with None" (PersistentHashMap.containsKey "a" map) + Expect.isTrue "key b with Some" (PersistentHashMap.containsKey "b" map) + Expect.isTrue "key c with None" (PersistentHashMap.containsKey "c" map) + Expect.isFalse "absent key" (PersistentHashMap.containsKey "d" map) + } + test "can PersistentHashMap.add PersistentHashMap.empty string as key to PersistentHashMap.empty map" { Expect.isFalse "PersistentHashMap.empty" <| PersistentHashMap.containsKey "" PersistentHashMap.empty diff --git a/tests/FSharpx.Collections.Tests/TransientHashMapTest.fs b/tests/FSharpx.Collections.Tests/TransientHashMapTest.fs index 78fdfa2f..d8c78b7f 100644 --- a/tests/FSharpx.Collections.Tests/TransientHashMapTest.fs +++ b/tests/FSharpx.Collections.Tests/TransientHashMapTest.fs @@ -67,7 +67,7 @@ module TransientHashMapTests = } //https://github.com/fsprojects/FSharpx.Collections/issues/85 - ptest "can add None value to empty map" { + test "can add None value to empty map" { let x = TransientHashMap.Empty() Expect.isTrue From 4b3b854d8a79800e5fdac3a442db42d26d50e253 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sun, 8 Mar 2026 18:56:50 +0000 Subject: [PATCH 2/4] ci: trigger checks From 99ee9c1f2876ca3230bdfb14f7aa79ecf5ab32dc Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sun, 8 Mar 2026 21:34:21 +0000 Subject: [PATCH 3/4] style: apply Fantomas formatting to PersistentHashMapTest.fs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/FSharpx.Collections.Tests/PersistentHashMapTest.fs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/FSharpx.Collections.Tests/PersistentHashMapTest.fs b/tests/FSharpx.Collections.Tests/PersistentHashMapTest.fs index 7c762cfd..d5dbe6f4 100644 --- a/tests/FSharpx.Collections.Tests/PersistentHashMapTest.fs +++ b/tests/FSharpx.Collections.Tests/PersistentHashMapTest.fs @@ -47,9 +47,7 @@ module PersistentHashMapTests = } test "containsKey returns true for unit value" { - let map = - PersistentHashMap.empty - |> PersistentHashMap.add "key" () + let map = PersistentHashMap.empty |> PersistentHashMap.add "key" () Expect.isTrue "containsKey with unit value" (PersistentHashMap.containsKey "key" map) Expect.isFalse "containsKey absent key" (PersistentHashMap.containsKey "other" map) From 4cd402ea7f291ff82f74ede85ff9035167a460a7 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Sun, 8 Mar 2026 21:38:20 +0000 Subject: [PATCH 4/4] perf: add dedicated INode.containsKey returning bool, avoiding option allocation Replace the tryFind-based ContainsKey with a new containsKey method on INode that returns bool directly. This avoids allocating an 'option' discriminated union on every ContainsKey call, improving performance for this hot path. Implement containsKey on all three INode types: - HashCollisionNode: check findIndex >= 0 and key equality - ArrayNode: delegate to child node.containsKey - BitmapIndexedNode: check bitmap bit, then key equality or recurse into sub-node Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/FSharpx.Collections/PersistentHashMap.fs | 31 ++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/src/FSharpx.Collections/PersistentHashMap.fs b/src/FSharpx.Collections/PersistentHashMap.fs index 343550d8..86259e22 100644 --- a/src/FSharpx.Collections/PersistentHashMap.fs +++ b/src/FSharpx.Collections/PersistentHashMap.fs @@ -13,6 +13,7 @@ type internal INode = abstract member assoc: Thread ref * int * int * obj * obj * Box -> INode abstract member find: int * int * obj -> obj abstract member tryFind: int * int * obj -> obj option + abstract member containsKey: int * int * obj -> bool abstract member without: int * int * obj -> INode abstract member without: Thread ref * int * int * obj * Box -> INode abstract member nodeSeq: unit -> (obj * obj) seq @@ -220,6 +221,10 @@ and private HashCollisionNode(thread, hashCollisionKey, count', array': obj[]) = else None + member this.containsKey(shift, hash, key) = + let idx = this.findIndex(key) + idx >= 0 && key = this.array.[idx] + member this.without(shift, hashKey, key) = let idx = this.findIndex(key) @@ -345,6 +350,13 @@ and private ArrayNode(thread, count', array': INode[]) = else node.tryFind(shift + 5, hash, key) + member this.containsKey(shift, hash, key) = + let idx = mask(hash, shift) + let node = this.array.[idx] + + node <> Unchecked.defaultof + && node.containsKey(shift + 5, hash, key) + member this.without(shift, hashKey, key) = let idx = mask(hashKey, shift) let node = this.array.[idx] @@ -470,6 +482,21 @@ and private BitmapIndexedNode(thread, bitmap', array': obj[]) = else None + member this.containsKey(shift, hash, key) = + let bit = bitpos(hash, shift) + + if this.bitmap &&& bit = 0 then + false + else + let idx' = index(this.bitmap, bit) * 2 + let keyOrNull = this.array.[idx'] + let valOrNode = this.array.[idx' + 1] + + if keyOrNull = null then + (valOrNode :?> INode).containsKey(shift + 5, hash, key) + else + key = keyOrNull + member this.assoc(shift, hashKey, key, value, addedLeaf) = let bit = bitpos(hashKey, shift) let idx' = index(this.bitmap, bit) * 2 @@ -698,7 +725,7 @@ type internal TransientHashMap<[] 'T, 'S when 'T: equalit member this.ContainsKey(key: 'T) = if key = Unchecked.defaultof<'T> then this.hasNull else if this.root = Unchecked.defaultof then false - else this.root.tryFind(0, hash(key), key).IsSome + else this.root.containsKey(0, hash(key), key) member this.Add(key: 'T, value: 'S) = if key = Unchecked.defaultof<'T> then @@ -793,7 +820,7 @@ and PersistentHashMap<[] 'T, 'S when 'T: equality and 'S: member this.ContainsKey(key: 'T) = if key = Unchecked.defaultof<'T> then this.hasNull else if this.root = Unchecked.defaultof then false - else this.root.tryFind(0, hash(key), key).IsSome + else this.root.containsKey(0, hash(key), key) static member ofSeq(items: ('T * 'S) seq) = let mutable ret = TransientHashMap<'T, 'S>.Empty()