MemberwiseClone() doesn't like arrays?

[pinkfloydfan]

Hi all

I have a class that implements the ICloneable interface whereby the
Clone() method returns MemberwiseClone().

One of the elements of the class is an array and although the Clone
method appears to work, if I call a method that then adjusts the
members of that array on the new class, the original class is also
adjusted.

Does anyone know why this is the case and how to avoid it?

An simple example of this in action can be seen from the code is
below:

public class TestClone : ICloneable
{
private int[] data;
public int[] Data
{
get { return data; }
set { data = value; }
}
public TestClone() { }
public TestClone(int[] _data) { data = _data; }
public void AdjustData(int adjuster)
{
int nb=0;
foreach (int qq in Data)
{
this.Data[nb] += adjuster;
nb++;
}
}
public object Clone()
{
return MemberwiseClone();
}
}

class Program
{
static void Main(string[] args)
{
int[] testdata = {3,1};
TestClone test1 = new TestClone(testdata);
TestClone test2 = new TestClone();
test2 = (TestClone)test1.Clone();
test2.AdjustData(3);
Console.WriteLine("test1 value is {0}, test2 value is
{1}", test1.Data[0], test2.Data[0]);
Console.ReadLine();

}
}

The output of this is:
test1 value is 6, test2 value is 6

It should be:
test1 value is 3, test2 value is 6

 

[Marc Gravell]

Easy - it has done a *shallow* clone, probably by a blit; this means
that it has, as promised, copied the member variable exactly; the
clone *also* contains a field that is a reference to the managed
heap - i.e. the same object. This is a common problem with using
shallow clone on an object whose fields are references.

You could try serializiation? This is the simplest approach to get a
deep-clone (assuming your data is serializable).

Or simply implement Clone() yourself, and manually clone the array
(giving the new instance the cloned array). Of course, if the array is
an array of reference-types (Customer, perhaps), then note that now
you have 2 arrays pointing to the same Customer objects, so
original.Data[0].Name = "Fred" will also update
newClone.Data[0].Name...

Do you see the problem?

Marc

 

[pinkfloydfan]

I think I've got it.

I have to copy each member of the array into a new array.

Would I have to do this for string types as well?

 

[Marc Gravell]

Yes, you need to clone the array regardless of the type. Fortunately
you can simply use .Clone() on the array. The point I was trying to
make, however, is that if you have a class inside the array, you might
actually want to clone that too! Or you might not, it depends on the
scenario.

i.e.
SomeType[] newArray = new SomeType[orig.Length];
for(int i = 0; i < orig.Length; i++) {
newArray = orig.Clone();
}
(and so on...)
- otherwise (i.e. without the Clone() line), newArray[0].SomeProperty
= 5 will also update orig[0], since they point to the same SomeType
instance.

As it happens, "string" is a class, but we don't need to worry about
this because string is "immutable" - i.e. the contents can't be
changed (by regular code) once it has been created. The issue I'm
trying (and possibly failing) to describe only affects classes with
updateable properties.

Marc

 

[pinkfloydfan]

Brilliant! Thanks very much for your help Marc.

 

[Xiasma]

Marc,

The problem isn't quite that simple: when I call MemberwiseClone() from my
inherited ArrayList, there seems to be some private member which actually
holds the items in the array which is shallow copied, so changing the
contents of the arraylist inthe clone also changes the contents in the
original.

Take a look at the code below for a demonstration.
I have a class thinglist which contains various members (in this case, a
string called Text). When I clone my thinglist, I can change the members on
the new thinglist and that, as expected, doesn't affect my original.
However, if I were to call clonedthinglist.Clear(), the contents of my
original thinglist would also disappear.

FWIW, I'm using .Net 3.5 - a very simple console app.


<pre>
using System;
using System.Collections;

namespace ConsoleApplication3
{
public class thing
{
public string Text;
}

public class thinglist : ArrayList
{
public string Text;

public thinglist doClone()
{
return this.MemberwiseClone() as thinglist;
}
}

class Program
{
static void Main(string[] args)
{
thing th1 = new thing();
th1.Text = "th1";
thing th2 = new thing();
th1.Text = "th2";
thinglist tl1 = new thinglist();
tl1.Text = "tl1";
tl1.Add(th1);
tl1.Add(th2);

thinglist tl2 = tl1.doClone();
tl2.Text = tl2.Text + "_copy";
for(int i = 0; i < tl1.Count; i++)
{
thing newth = new thing();
newth.Text = (tl1 as thing).Text + "_copy";
tl2 = newth;
}

Console.WriteLine(tl1.Text);
foreach(thing th in tl1)
{
Console.WriteLine("tl1[]" + th.Text);
}
Console.WriteLine(tl2.Text);
foreach (thing th in tl2)
{
Console.WriteLine("tl2[]" + th.Text);
}
Console.ReadKey();
}
}
}
</pre>

 

[Jon Skeet [C# MVP]]

The problem isn't quite that simple: when I call MemberwiseClone() from my
inherited ArrayList, there seems to be some private member which actually
holds the items in the array which is shallow copied, so changing the
contents of the arraylist inthe clone also changes the contents in the
original.

That's exactly what I'd expect when taking a very shallow copy (and
MemberwiseClone is as shallow as it gets) of a mutable collection.

You'd probably see some interesting behaviour after adding enough
items to one ArrayList to cause a resize - suddenly they'd be
independent again.

Basically cloning anything non-trivial is fraught with problems.
Another argument in favour of immutable collections (See Eric
Lippert's blog for details.)

Jon

 

[Ade]

That's exactly what I'd expect when taking a very shallow copy (and
MemberwiseClone is as shallow as it gets) of a mutable collection.

I'd expect that if I changed a *property* of an item in the list, it
would be apparent in both the original and the clone, afterall the
items in the list are the same instances. However, behaviour suggests
the arraylist itself is actually a *wrapper* for a collection and,
whilst the wrapper has shallow-copied all members, the aggregated
collection is the very same one, rather than just the contents of the
collection, if you see what I mean :s

You'd probably see some interesting behaviour after adding enough
items to one ArrayList to cause a resize - suddenly they'd be
independent again.

I'm not sure I follow - calling clonedlist.clear() will clear both the
cloned and the originals items. Same goes for calling original.clear()

Basically cloning anything non-trivial is fraught with problems.
Another argument in favour of immutable collections (See Eric
Lippert's blog for details.)

Jon

Thanks, I'll take a look

 

[Marc Gravell]

Had to struggle to remember...

First - we were talking about arrays, not ArrayList - there is a
*huge* difference. To clone an ArrayList you would call Clone() - this
will internally worry about "doing the right thing" i.e. creating a
separate array. However, this can't know about your new type, so you
will need to write Clone yourself, as below.

Second; yes - the question of deep vs shallow copy is one that I
myself commented on:

<quote>
The point I was trying to
make, however, is that if you have a class inside the array, you
might
actually want to clone that too! Or you might not, it depends on the
scenario.</quote>

By default you have a shallow copy; this is expected. If you want a
deep clone you'll have to do it yourself. In 2.0 you could perhaps
insist that the items are cloneable.

By the way, I swapped the public field for a property; you might also
want to avoid using ArrayList if you are in 2.0; generics and things
like List<T>/Collection<T> largely deprecate ArrayList.

First code sample is ArrayList, and makes no pretence at a deep copy.

public class thinglist : ArrayList
{
// Text (1 line in C# 3)
private string text;
public string Text
{
get { return text; }
set { text = value; }
}
// ctors
public thinglist() { }
private thinglist(ICollection data) : base(data) { }
// clone
public override object Clone()
{
thinglist clone = new thinglist(this); // copies data
clone.Text = this.Text;
return clone;
}
}

Second example using generics with a constraint to enforce clonability
of contained items:
(used by NamedCollection<thing> etc)

public class NamedCollection<T> : Collection<T>, ICloneable
where T : ICloneable
{
private string text;
public string Text {
get { return text; }
set { text = value; }
}
public NamedCollection() { }

object ICloneable.Clone() { return Clone(); }
public virtual NamedCollection<T> Clone() {
NamedCollection<T> clone = new NamedCollection<T>();
clone.Text = Text;
foreach (T value in this) {
clone.Items.Add(value == null ? value : (T)value.Clone());
}
return clone;
}
}

 

[Marc Gravell]

However, if I were to call clonedthinglist.Clear(), the contents of my

original thinglist would also disappear.

That is because you uses MemberwiseClone() - that would have been fine
for an array (the original topic), but is not really suitable for this
example.

 

[Jon Skeet [C# MVP]]

I'd expect that if I changed a *property* of an item in the list, it
would be apparent in both the original and the clone, afterall the
items in the list are the same instances.

Yes, that will happen as well.

However, behaviour suggests
the arraylist itself is actually a *wrapper* for a collection and,
whilst the wrapper has shallow-copied all members, the aggregated
collection is the very same one, rather than just the contents of the
collection, if you see what I mean :s

Yes - but surely this is obvious in the name "ArrayList" isn't it? It's
a list implementation using an array as a backing store.

From the docs:

<quote>
Implements the IList interface using an array whose size is dynamically
increased as required.
</quote>

It's slightly misleading, in that the array itself isn't dynamically
resized, because arrays have a fixed size - but the ArrayList creates a
new list with a different size when it needs to, and effectively
ignores the old list.

I'm not sure I follow - calling clonedlist.clear() will clear both the
cloned and the originals items. Same goes for calling original.clear()

Try adding lots of items to just one of the lists first (so that the
size of the list is more than doubled, involving creating a new
buffer).

After that, I think you'll find the arrays are independent.

 

[Ade]

Had to struggle to remember...

First - we were talking about arrays, not ArrayList - there is a
*huge* difference.

Oops, my bad for not reading the first post properly :x

To clone an ArrayList you would call Clone() - this
will internally worry about "doing the right thing" i.e. creating a
separate array. However, this can't know about your new type, so you
will need to write Clone yourself, as below.

OK, I figured I could go that route, but I *Really* want to avoid that
as I have lots of classes inheriting from ArrayList, each with lots of
members. Further, I have lots of classes inheriting from those
classes, adding more members still. I was hoping for a more-readily
maintained solution, more along the lines of reflection, which could
say something like

<pseudocode>
public override object Clone()
{
ThingList clonedThingList = new ThingList();
foreach(Thing th in this)
{
clonedThingList.Add(th);
}
foreach(member_of_ThingList somemember)
{
clonedThingList.somemember = this.somemember;
}
return clonedThingList;
}

</pseudocode>
where the second foreach is, say, using reflection to find each member
aggregated in, such as ThingList.Caption, ThingList.Text, ThingList.X,
ThingList.Y, etc or whatever.

Am I talking utter nonsense?

Second; yes - the question of deep vs shallow copy is one that I
myself commented on:

Yep, followed all that and I *want* the items in my cloned arraylist
to be shallow copies; I'm more than happy with that bit.

By default you have a shallow copy; this is expected. If you want a
deep clone you'll have to do it yourself. In 2.0 you could perhaps
insist that the items are cloneable.

Yep, fine with all that.

By the way, I swapped the public field for a property; you might also
want to avoid using ArrayList if you are in 2.0; generics and things
like List<T>/Collection<T> largely deprecate ArrayList.

I intend to keep ArrayList for a variety of reasons, not least because
there's a stack of code already in place. However, I also need
something to build down to .net CF 1.1 (although the clone
functionality need not).


<snip>

Thanks

 

[Marc Gravell]

OK; maybe I got side-tracked by the deep-copy issue. If you just want
shallow... fine...

You can't use MemberwiseClone because it will copy the base-class
properties (such as the array reference) - but perhaps something
involving *encapsulation* (rather than inheritance) provides the
route? Then you only have to worry about cranking one field (the
reference to the backing list); note I've used a base-class to *just*
provide the encapsulation, with the specific functionality (Text etc)
in a subclass:

public class NamedList : CloneList
{
private string text;
public string Text
{
get { return text; }
set { text = value; }
}
}
public class CloneList : IList, ICloneable
{
ArrayList list = new ArrayList();

public object Clone()
{
NamedList clone = (NamedList) MemberwiseClone();
// now swap the backing list
clone.list = (ArrayList) list.Clone();
return clone;
}
public int Add(object value) {
return list.Add(value);
}
public void Clear() {
list.Clear();
}
public bool Contains(object value) {
return list.Contains(value);
}
public int IndexOf(object value) {
return list.IndexOf(value);
}
public void Insert(int index, object value) {
list.Insert(index, value);
}
public bool IsFixedSize {
get { return list.IsFixedSize; }
}
public bool IsReadOnly {
get { return list.IsReadOnly; }
}
public void Remove(object value) {
list.Remove(value);
}
public void RemoveAt(int index) {
list.RemoveAt(index);
}
public object this[int index] {
get {return list[index];}
set {list[index] = value;}
}
public void CopyTo(Array array, int index) {
list.CopyTo(array, index);
}
public int Count {
get { return list.Count; }
}
bool ICollection.IsSynchronized {
get { return list.IsSynchronized; }
}
object ICollection.SyncRoot {
get { return list.SyncRoot; }
}
public IEnumerator GetEnumerator() {
return list.GetEnumerator();
}
}

 

[Ade]

OK; maybe I got side-tracked by the deep-copy issue. If you just want
shallow... fine...

You can't use MemberwiseClone because it will copy the base-class
properties (such as the array reference) - but perhaps something
involving *encapsulation* (rather than inheritance) provides the
route? Then you only have to worry about cranking one field (the
reference to the backing list); note I've used a base-class to *just*
provide the encapsulation, with the specific functionality (Text etc)
in a subclass:

<snip>

thanks all.

I've solved it like this:

public override thinglist Clone()
{
thinglist clone = new thinglist();
FieldInfo[] fieldInfos = GetType().GetFields();
foreach (FieldInfo fi in fieldInfos)
{
fi.SetValue(clone, fi.GetValue(this));
}
clone.Clear();
foreach (thing th in this)
{
clone.Add(th);
}
return clone;
}
}

 

[Ade]

Nearly!

That only works with public fields which are present on the base type.

public override thinglist Clone()
{
thinglist clone = new thinglist();
FieldInfo[] fieldInfos =
GetType().GetFields(BindingFlags.NonPublic | BindingFlags.Instance |
BindingFlags.Public);
foreach (FieldInfo fi in fieldInfos)
{
fi.SetValue(clone, fi.GetValue(this));
}
clone.Clear();
foreach (thing th in this)
{
clone.Add(th);
}
return clone;
}

However, there's a problem here. If clone is a type inheriting from
thinglist, rather than a thinglist itself, it may contain members
which are not present in thinglist.
So, I need something /like/

Type mytype = GetType()
fi.SetValue(clone as mytype, fi.GetValue(this));

Except I can't work out the correct syntax.

:s