Skip to content

IEnumerable<T>.DeepClone() returns a lazy view: edits to the cloned items are lost, and the clone changes whenever the source changes #80

Description

@matt-edmondson

What's wrong

DeepCloneContainerExtensions.DeepClone<T>(this IEnumerable<T> source) (DeepClone/DeepCloneContainerExtensions.cs:116-117) is implemented as:

public static IEnumerable<T> DeepClone<T>(this IEnumerable<T> source) =>
	source.Select(DeepClone);

Select runs lazily, so no cloning happens when the method is called. What comes back is a view over the live source, and every enumeration of it calls DeepClone() on every element again. The XML doc (lines 104-105) promises "A new collection containing deep clones of the original items", and the method name suggests a snapshot. What callers actually get is:

  • the clone is not independent of the source. If the source list is changed or cleared after the call, the "clone" changes with it.
  • each enumeration yields a fresh set of clone instances, so changes made to items during one enumeration are silently gone the next time the sequence is read.
  • the extra enumerations are expensive. Count(), Any(), First(), or a second foreach clone the whole object graph again each time.

List<T>, arrays, Queue<T>, LinkedList<T>, and the other collections the README lists as supported all bind to this overload, so the problem affects the library's main use case.

Failure scenario

class Item : DeepCloneable<Item> { public string Name = "orig"; ... }

var src = new List<Item> { new() };
IEnumerable<Item> clone = src.DeepClone();

foreach (var x in clone) x.Name = "edited";
clone.First().Name;                              // "orig"  (expected "edited")
ReferenceEquals(clone.First(), clone.First());   // false

src.Clear();
clone.Count();                                   // 0      (expected 1: the clone should not track the source)

Reproduced by compiling the library sources into a net10.0 console app.

Suggested fix

Make the clone when the method is called, and null-check the source like the other overloads do:

public static IEnumerable<T> DeepClone<T>(this IEnumerable<T> source)
{
	Ensure.NotNull(source);
	return source.Select(DeepClone).ToArray(); // or ToList()
}

(Stack<T>.DeepClone at line 235 also has no Ensure.NotNull, unlike its sibling overloads.) Add a test that edits the items of a cloned sequence and then enumerates it again, and one that changes the source after cloning.

Activity

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

Metadata

Metadata

Labels

bugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions