Skip to content

Deep cloning a ConcurrentDictionary, SortedList or ImmutableDictionary through IDictionary drops its key comparer: lookups start failing, and a reference-equality source throws ArgumentException #83

Description

@matt-edmondson

What's wrong

CloneDictionary (DeepClone/DeepCloneContainerExtensions.cs:227-237) keeps the source's key comparer only when the runtime type is Dictionary<,> or SortedDictionary<,>. Every other IDictionary<TKey,TValue> / IReadOnlyDictionary<TKey,TValue> falls through to the default arm:

_ => new Dictionary<TKey, TValue>(),   // line 234: default comparer, default kind

The #77 fix covered only those two types. Other common dictionaries also expose their comparer, but it is thrown away:

The clone therefore compares keys differently from the source:

  1. Lookups silently fail when the source comparer is coarser than the default (for example OrdinalIgnoreCase).
  2. Cloning throws when the source comparer is finer than the default. For example, with ReferenceEqualityComparer and two equal but distinct keys, AddClonedPairs (line 249) calls dest.Add on a default-comparer Dictionary, and that throws on the duplicate.
  3. A SortedList comes back as an unsorted Dictionary, so it stops keeping order on later inserts. This is the same problem Dictionary DeepClone() drops the source's key comparer, so a cloned case-insensitive dictionary stops finding keys #77 fixed for SortedDictionary.

Reproduction

using System.Collections.Concurrent;
using System.Collections.Immutable;
using ktsu.DeepClone;

var cd = new ConcurrentDictionary<string,int>(StringComparer.OrdinalIgnoreCase); cd["Key"] = 1;
var c1 = ((IDictionary<string,int>)cd).DeepClone();
Console.WriteLine($"{cd.ContainsKey("key")} {c1.ContainsKey("key")}");

var sl = new SortedList<string,int>(Comparer<string>.Create((x,y)=>string.CompareOrdinal(y,x))) { ["a"]=1, ["c"]=3, ["b"]=2 };
var c2 = ((IDictionary<string,int>)sl).DeepClone();
c2["d"] = 4; sl["d"] = 4;
Console.WriteLine($"{string.Join(",", sl.Keys)} | {c2.GetType().Name} {string.Join(",", c2.Keys)}");

var im = ImmutableDictionary.Create<string,int>(StringComparer.OrdinalIgnoreCase).Add("Key", 1);
Console.WriteLine(((IDictionary<string,int>)im).DeepClone().ContainsKey("key"));

var rc = new ConcurrentDictionary<object,int>(ReferenceEqualityComparer.Instance);
rc[new string('k',1)] = 1; rc[new string('k',1)] = 2;              // Count == 2
((IDictionary<object,int>)rc).DeepClone();                          // throws
// The same data in a Dictionary<object,int>(ReferenceEqualityComparer) clones fine (Count 2).

Observed (net10.0, current main 3d31b66):

ConcurrentDictionary: source['key'] found=True, clone['key'] found=False
SortedList: source keys=d,c,b,a | clone type=Dictionary`2, clone keys=c,b,a,d
ImmutableDictionary: source['key'] found=True, clone['key'] found=False
ArgumentException: An item with the same key has already been added. Key: k

Expected: each clone finds "key" as its source does, the SortedList clone stays sorted with the source's comparer, and the reference-equality clone has 2 entries rather than throwing.

Why it matters

A deep clone should look up keys the same way its source does. Because the call succeeds and returns a dictionary that looks correct, the failure shows up later as missing keys. The reference-equality case turns a valid source into a runtime exception. #77 fixed this for Dictionary and SortedDictionary, and the other BCL dictionaries with comparers still have the same bug.

Suggested fix / acceptance criteria

  • Add arms to CloneDictionary for types that expose a comparer. Each arm should create the same kind of dictionary (or a mutable one with the same comparer):
    • ConcurrentDictionary<K,V> c becomes new ConcurrentDictionary<K,V>(c.Comparer), under #if NET5_0_OR_GREATER because Comparer does not exist on netstandard2.x.
    • SortedList<K,V> s becomes new SortedList<K,V>(s.Count, s.Comparer)
    • For ImmutableDictionary / ImmutableSortedDictionary, return a mutable Dictionary / SortedDictionary built with KeyComparer, or build through the immutable type's builder. Either way, keep the comparer.
  • Add tests for each type that check the comparer is kept (case-insensitive lookup works, a ReferenceEqualityComparer source with equal keys clones without throwing, and a SortedList clone keeps its order and comparer).
  • Update the <remarks> on both DeepClone overloads (lines ~129-131 and ~200) so they list the types whose comparer is kept.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingreadyFully specified; implement as written

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions