Skip to content

fix(js/ts): compare array keys by reference in Dictionary #5239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
MangelMaxime wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(js/ts): compare array keys by reference in Dictionary #5239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 34 additions & 10 deletions src/Fable.Transforms/Replacements.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,23 @@ let makeEqualityComparer (com: ICompiler) ctx typArg =
"GetHashCode", Delegate([ x ], structuralHash com None (IdentExpr x), None, Tags.empty)
]

/// Matches EqualityComparer<T>.Default, which compares arrays by reference in .NET
let makeDefaultEqualityComparer (com: ICompiler) ctx typArg =
match typArg with
| Array(_, (MutableArray | ImmutableArray)) ->
let x = makeUniqueIdent com ctx typArg "x"
let y = makeUniqueIdent com ctx typArg "y"

let hash =
Helper.LibCall(com, "Util", "physicalHash", Int32.Number, [ IdentExpr x ])

objExpr
[
"Equals", Delegate([ x; y ], makeEqOp None (IdentExpr x) (IdentExpr y) BinaryEqual, None, Tags.empty)
"GetHashCode", Delegate([ x ], hash, None, Tags.empty)
]
| _ -> makeEqualityComparer com ctx typArg

// TODO: Try to detect at compile-time if the object already implements `Compare`?
let inline makeComparerFromEqualityComparer e = e // leave it as is, if implementation supports it
// Helper.LibCall(com, "Util", "comparerFromEqualityComparer", Any, [e])
Expand All @@ -833,21 +850,28 @@ let makeMap (com: ICompiler) ctx r t methName args genArg =
let makeDictionaryWithComparer com r t sourceSeq comparer =
Helper.LibCall(com, "MutableMap", "Dictionary", t, [ sourceSeq; comparer ], isConstructor = true, ?loc = r)

let makeDictionary (com: ICompiler) ctx r t sourceSeq =
let private makeDictionaryWithKeyComparer makeKeyComparer (com: ICompiler) ctx r t sourceSeq =
match t with
| DeclaredType(_, [ key; _ ]) when not (isCompatibleWithNativeComparison com key) ->
// makeComparer com ctx key
makeEqualityComparer com ctx key |> makeDictionaryWithComparer com r t sourceSeq
makeKeyComparer com ctx key |> makeDictionaryWithComparer com r t sourceSeq
| _ -> Helper.GlobalCall("Map", t, [ sourceSeq ], isConstructor = true, ?loc = r)

let makeDictionary (com: ICompiler) ctx r t sourceSeq =
makeDictionaryWithKeyComparer makeEqualityComparer com ctx r t sourceSeq

let makeBclDictionary (com: ICompiler) ctx r t sourceSeq =
makeDictionaryWithKeyComparer makeDefaultEqualityComparer com ctx r t sourceSeq

let makeHashSetWithComparer com r t sourceSeq comparer =
Helper.LibCall(com, "MutableSet", "HashSet", t, [ sourceSeq; comparer ], isConstructor = true, ?loc = r)

let makeHashSet (com: ICompiler) ctx r t sourceSeq =
match t with
| DeclaredType(_, [ key ]) when not (isCompatibleWithNativeComparison com key) ->
// makeComparer com ctx key
makeEqualityComparer com ctx key |> makeHashSetWithComparer com r t sourceSeq
makeDefaultEqualityComparer com ctx key
|> makeHashSetWithComparer com r t sourceSeq
| _ -> Helper.GlobalCall("Set", t, [ sourceSeq ], isConstructor = true, ?loc = r)

let tryEntityIdent (com: Compiler) entFullName =
Expand Down Expand Up @@ -3206,13 +3230,13 @@ let dictionaries (com: ICompiler) (ctx: Context) r t (i: CallInfo) (thisArg: Exp
match i.CompiledName, thisArg with
| ".ctor", _ ->
match i.SignatureArgTypes, args with
| ([] | [ Number _ ]), _ -> makeDictionary com ctx r t (makeArray Any []) |> Some
| [ IDictionary ], [ arg ] -> makeDictionary com ctx r t arg |> Some
| ([] | [ Number _ ]), _ -> makeBclDictionary com ctx r t (makeArray Any []) |> Some
| [ IDictionary ], [ arg ] -> makeBclDictionary com ctx r t arg |> Some
| [ IDictionary; IEqualityComparer ], [ arg; eqComp ] ->
makeComparerFromEqualityComparer eqComp
|> makeDictionaryWithComparer com r t arg
|> Some
| [ IEnumerable ], [ arg ] -> makeDictionary com ctx r t arg |> Some
| [ IEnumerable ], [ arg ] -> makeBclDictionary com ctx r t arg |> Some
| [ IEnumerable; IEqualityComparer ], [ arg; eqComp ] ->
makeComparerFromEqualityComparer eqComp
|> makeDictionaryWithComparer com r t arg
Expand Down Expand Up @@ -5192,8 +5216,8 @@ let tryBaseConstructor com ctx (ent: EntityRef) (argTypes: Lazy<Type list>) genA
| Types.dictionary ->
let args =
match argTypes.Value, args with
| ([] | [ Number _ ]), _ -> [ makeArray Any []; makeEqualityComparer com ctx (Seq.head genArgs) ]
| [ IDictionary ], [ arg ] -> [ arg; makeEqualityComparer com ctx (Seq.head genArgs) ]
| ([] | [ Number _ ]), _ -> [ makeArray Any []; makeDefaultEqualityComparer com ctx (Seq.head genArgs) ]
| [ IDictionary ], [ arg ] -> [ arg; makeDefaultEqualityComparer com ctx (Seq.head genArgs) ]
| [ IDictionary; IEqualityComparer ], [ arg; eqComp ] -> [ arg; makeComparerFromEqualityComparer eqComp ]
| [ IEqualityComparer ], [ eqComp ]
| [ Number _; IEqualityComparer ], [ _; eqComp ] ->
Expand All @@ -5206,8 +5230,8 @@ let tryBaseConstructor com ctx (ent: EntityRef) (argTypes: Lazy<Type list>) genA
| Types.hashset ->
let args =
match argTypes.Value, args with
| ([] | [ Number _ ]), _ -> [ makeArray Any []; makeEqualityComparer com ctx (Seq.head genArgs) ]
| [ IEnumerable ], [ arg ] -> [ arg; makeEqualityComparer com ctx (Seq.head genArgs) ]
| ([] | [ Number _ ]), _ -> [ makeArray Any []; makeDefaultEqualityComparer com ctx (Seq.head genArgs) ]
| [ IEnumerable ], [ arg ] -> [ arg; makeDefaultEqualityComparer com ctx (Seq.head genArgs) ]
| [ IEnumerable; IEqualityComparer ], [ arg; eqComp ] -> [ arg; makeComparerFromEqualityComparer eqComp ]
| [ IEqualityComparer ], [ eqComp ]
| [ Number _; IEqualityComparer ], [ _; eqComp ] ->
Expand Down
35 changes: 35 additions & 0 deletions tests/Js/Main/DictionaryTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ type Table<'K, 'V when 'K: equality and 'K: not null> () =

member _.Dic = dic

#if !FABLE_COMPILER_TYPESCRIPT
type ArrayKeyDictionary() =
inherit Dictionary<int[], int>()
#endif

let tests =
testList "Dictionaries" [
testCase "Dictionary KeyValuePattern works" <| fun () -> // See #509
Expand Down Expand Up @@ -384,4 +389,34 @@ let tests =
coll.Remove(KeyValuePair("D", 3)) |> equal false
coll.Remove(KeyValuePair("B", 2)) |> equal true
coll.Count |> equal 2

testCase "Dictionary compares array keys by reference" <| fun _ -> // See #5165
let key = [| 1 |]
let dic = Dictionary<int[], int>()
dic.[key] <- 1
dic.[[| 1 |]] <- 2
dic.Count |> equal 2
dic.ContainsKey key |> equal true
dic.ContainsKey [| 1 |] |> equal false
dic.[key] |> equal 1
let copy = Dictionary<int[], int>(dic)
copy.Count |> equal 2
copy.ContainsKey key |> equal true
copy.ContainsKey [| 1 |] |> equal false

#if !FABLE_COMPILER_TYPESCRIPT
testCase "Dictionary subclasses compare array keys by reference" <| fun _ ->
let key = [| 1 |]
let dic = ArrayKeyDictionary()
dic.[key] <- 1
dic.ContainsKey key |> equal true
dic.ContainsKey [| 1 |] |> equal false
#endif

testCase "Dictionary with a structural comparer compares array keys by value" <| fun _ ->
let dic = Dictionary<int[], int>(HashIdentity.Structural)
dic.[[| 1 |]] <- 1
dic.ContainsKey [| 1 |] |> equal true
(dict [ [| 1 |], 1 ]).ContainsKey [| 1 |] |> equal true
(readOnlyDict [ [| 1 |], 1 ]).ContainsKey [| 1 |] |> equal true
]
31 changes: 31 additions & 0 deletions tests/Js/Main/HashSetTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ let inline hashSet l =
[<ReferenceEquality>]
type RefEqRecord = { a: int; b: int }

#if !FABLE_COMPILER_TYPESCRIPT
type ArrayHashSet() =
inherit HashSet<int[]>()
#endif

type MyRecord = { a: int }

type R = { i: int; s: string }
Expand Down Expand Up @@ -416,4 +421,30 @@ let tests =
coll.Remove(("D", 3)) |> equal false
coll.Remove(("B", 2)) |> equal true
coll.Count |> equal 2

testCase "HashSet compares arrays by reference" <| fun _ -> // See #5165
let item = [| 1 |]
let xs = HashSet<int[]>()
xs.Add item |> equal true
xs.Add item |> equal false
xs.Add [| 1 |] |> equal true
xs.Count |> equal 2
xs.Contains item |> equal true
xs.Contains [| 1 |] |> equal false
HashSet<int[]>([ [| 1 |]; [| 1 |] ]).Count |> equal 2

#if !FABLE_COMPILER_TYPESCRIPT
testCase "HashSet subclasses compare arrays by reference" <| fun _ ->
let item = [| 1 |]
let xs = ArrayHashSet()
xs.Add item |> equal true
xs.Contains item |> equal true
xs.Contains [| 1 |] |> equal false
#endif

testCase "HashSet with a structural comparer compares arrays by value" <| fun _ ->
let xs = HashSet<int[]>(HashIdentity.Structural)
xs.Add [| 1 |] |> equal true
xs.Add [| 1 |] |> equal false
xs.Contains [| 1 |] |> equal true
]
Loading