views:

592

answers:

7

When I want to make a value type read-only outside of my class I do this:

public class myClassInt
{
    private int m_i;
    public int i {
     get { return m_i; }
    }

    public myClassInt(int i)
    {
     m_i = i;
    }
}

What can I do to make a List<T> type readonly (so they can't add/remove elements to/from it) outside of my class? Now I just declare it public:

public class myClassList
{
    public List<int> li;
    public  myClassList()
    {
     li = new List<int>();
     li.Add(1);
     li.Add(2);
     li.Add(3);
    }
}
+13  A: 

You can expose it AsReadOnly. That is, return a read-only IList<T> wrapper. For example ...

public ReadOnlyCollection<int> List
{
    get { return _lst.AsReadOnly(); }
}

Just returning an IEnumerable<T> is not sufficient. For example ...

void Main()
{
    var el = new ExposeList();
    var lst = el.ListEnumerator;
    var oops = (IList<int>)lst;
    oops.Add( 4 );  // mutates list

    var rol = el.ReadOnly;
    var oops2 = (IList<int>)rol;

    oops2.Add( 5 );  // raises exception
}

class ExposeList
{
  private List<int> _lst = new List<int>() { 1, 2, 3 };

  public IEnumerable<int> ListEnumerator
  {
     get { return _lst; }
  }

  public ReadOnlyCollection<int> ReadOnly
  {
     get { return _lst.AsReadOnly(); }
  }
}

Steve's answer also has a clever way to avoid the cast.

JP Alioto
I added an example...feel free to roll back if you object.
Adam Robinson
See my answer for how to circumvent this.
Daniel Earwicker
+1  A: 

Eric Lippert has a series of articles on Immutability In C# on his blog.

The first article in the series can be found here.

You might also find useful Jon Skeet's answer to a similar question.

Winston Smith
A: 
public List<int> li;

Don't declare public fields, it's generally considered bad practice... wrap it in a property instead.

You can expose your collection as a ReadOnlyCollection :

private List<int> li;
public ReadOnlyCollection<int> List
{
    get { return li.AsReadOnly(); }
}
Thomas Levesque
+4  A: 

JP's answer regarding returning IEnumerable<int> is correct (you can down-cast to a list), but here is a technique that prevents the down-cast.

class ExposeList
{
  private List<int> _lst = new List<int>() { 1, 2, 3 };

  public IEnumerable<int> ListEnumerator
  {
     get { return _lst.Select(x => x); }  // Identity transformation.
  }

  public ReadOnlyCollection<int> ReadOnly
  {
     get { return _lst.AsReadOnly(); }
  }
}

The identity transformation during enumeration effectively creates a compiler-generated iterator - a new type which is not related to _lst in any way.

Steve Guidi
could maybe use `foreach(var i in _lst) yield return i;` as well and avoid the select? Probably doesn't matter too much though...
Svish
See my answer for how to circumvent this.
Daniel Earwicker
@Svish: using an identity select produces the same code you provided. In other words, the select is a method call that invokes your code.
Steve Guidi
A: 
public class MyClassList
{
    private List<int> _lst = new List<int>() { 1, 2, 3 };

    public IEnumerable<int> ListEnumerator
    {
        get { return _lst.AsReadOnly(); }
    }

}

To check it

    MyClassList  myClassList = new MyClassList();
    var lst= (IList<int>)myClassList.ListEnumerator  ;
    lst.Add(4); //At this point ypu will get exception Collection is read-only.
Raghav Khunger
+4  A: 

There is limited value in attempting to hide information to such an extent. The type of the property should tell users what they're allowed to do with it. If a user decides they want to abuse your API, they will find a way. Blocking them from casting doesn't stop them:

public static class Circumventions
{
    public static IList<T> AsWritable<T>(this IEnumerable<T> source)
    {
        return source.GetType()
            .GetFields(BindingFlags.Public |
                       BindingFlags.NonPublic | 
                       BindingFlags.Instance)
            .Select(f => f.GetValue(source))
            .OfType<IList<T>>()
            .First();
    }
}

With that one method, we can circumvent the three answers given on this question so far:

List<int> a = new List<int> {1, 2, 3, 4, 5};

IList<int> b = a.AsReadOnly(); // block modification...

IList<int> c = b.AsWritable(); // ... but unblock it again

c.Add(6);
Debug.Assert(a.Count == 6); // we've modified the original

IEnumerable<int> d = a.Select(x => x); // okay, try this...

IList<int> e = d.AsWritable(); // no, can still get round it

e.Add(7);
Debug.Assert(a.Count == 7); // modified original again

Also:

public static class AlexeyR
{
    public static IEnumerable<T> AsReallyReadOnly<T>(this IEnumerable<T> source)
    {
        foreach (T t in source) yield return t;
    }
}

IEnumerable<int> f = a.AsReallyReadOnly(); // really?

IList<int> g = f.AsWritable(); // apparently not!
g.Add(8);
Debug.Assert(a.Count == 8); // modified original again

To reiterate... this kind of "arms race" can go on for as long as you like!

The only way to stop this is to completely break the link with the source list, which means you have to make a complete copy of the original list. This is what the BCL does when it returns arrays. The downside of this is that you are imposing a potentially large cost on 99.9% of your users every time they want readonly access to some data, because you are worried about the hackery of 00.1% of users.

Or you could just refuse to support uses of your API that circumvent the static type system.

If you want a property to return a read-only list with random access, return something that implements:

public interface IReadOnlyList<T> : IEnumerable<T>
{
    int Count { get; }
    T this[int index] { get; }
}

If (as is much more common) it only needs to be enumerable sequentially, just return IEnumerable:

public class MyClassList
{
    private List<int> li = new List<int> { 1, 2, 3 };

    public IEnumerable<int> MyList
    {
        get { return li; }
    }
}
Daniel Earwicker
While I agree on your general point, see my answer. With obfuscated sources, this _could_ be enough.
Alexey Romanov
See updated version - this is what my general point really was: it's an arms race. We can keep going until you are forced to make a complete copy of the original list, to avoid holding any references to the original.
Daniel Earwicker
NB. I am doing all my reverse engineering with the debugger. Don't need to see any source.
Daniel Earwicker
I disagree with your logic somewhat. You can call private members with reflection and "#define private public" works in C++, but you wouldn't argue that you should never make anything private again. The point of the interface is not to subvert the malicious developer, but to indicate to a developer what the correct usage of the interface is. AsReadOnly does that quite well.
JP Alioto
Why do you use `First(v => (v as IList<T>) != null)`? Couldn't you have used `First(v => v is IList<T>)`? Anyways, I see your point, but have to agree with JP. Also, protecting oneself against a simple cast I would say can sometimes be a nice thing to do. But when people start using reflection on your code, all bets are off anyways so no need to bother kind of :p
Svish
@JP - clearly I wouldn't argue that nothing should be private. To make something private is to use the static type system to declare how to use the class correctly at compile time, which is exactly what I'm suggesting is the right answer. Returning IList<T> (which at compile time suggests mutability) is the wrong answer. To return an immutable list, return a static type that only has public read operations.
Daniel Earwicker
@Svish - I would argue that when users ignore static types and start speculatively casting, all bets are off. They are doing the same thing as reflection but with a more convenient syntax (although that can be emulated, as shown above). The point I think others are missing is that `IList` is the wrong static type to return if the list is supposed to be immutable. `IEnumerable` is the right choice if forward-iteration is all that is required. If readonly random access is really needed, use something like `IReadOnlyList<T>` (above). Get the static type right, and you've done all you really can.
Daniel Earwicker
Some more good thoughts in the comments to this answer: http://stackoverflow.com/questions/1228176/what-are-your-c-commandments/1232531#1232531
Daniel Earwicker
The trouble is that a lot of programmers are dumb and would consider casting safe (whether it is or isn't does not matter they still do it). However in my experience even the dumbest programmers think twice before using reflection's BindingFlags.NonPublic operation. That is assuming they found this obscure part of the framework in the first place.
Martin Brown
"The trouble is that a lot of programmers are dumb" - yes, but whose trouble is it?
Daniel Earwicker
"but whose trouble is it?", unfortumatly arround here it me that normally ends up dealing with the support calls. If you don't have to provide support for your code, you are very lucky.
Martin Brown
I'm guessing if user called you and said "Why can't this API work with my toaster?" you'd probably say something like "RTFM, it's not designed to work with a toaster." And yet if they say "I tried casting an IEnumerable to an IList so I could add things to it, and it seemed to work, but then later the program crashed", what's up with saying "It's not designed to work that way, that's why we made it an IEnumerable"? You've made a decision to support users who abuse your API, at your own unlimited cost. And RE: your point about dumb programmers - they can always come to SO and ask for help! :)
Daniel Earwicker
A: 
public static IEnumerable<T> AsReallyReadOnly<T>(this IEnumerable<T> source)
{
    foreach (T t in source) yield return t;
}

if I add to Earwicker's example

...
IEnumerable<int> f = a.AsReallyReadOnly();
IList<int> g = f.AsWritable(); // finally can't get around it

g.Add(8);
Debug.Assert(a.Count == 78);

I get InvalidOperationException: Sequence contains no matching element.

Alexey Romanov
Fixed now - I was only looking for private fields of the correct static type. If I relax it to private and public fields of the correct dynamic type, it gets around it.
Daniel Earwicker