Incorrect results produced with AutoCompile == true.

[GoogleCodeExporter]

Observe the attached program. 
Basically it runs the following code:

[DataContract]
public class I
{
  [DataMember(Order = 1)]
  public Dictionary<string, TBase> Data { get; set; }
}

public static bool Test(bool autoCompile, bool isDefault)
{
  var m = isDefault ? RuntimeTypeModel.Default : TypeModel.Create();
  m.AutoCompile = autoCompile;
  m.Add(typeof(TBase), false).AddSubType(1, typeof(T));

  var o = new I { Data = new Dictionary<string, TBase> { { "C", new T { 7 } } } };
  using (var ms = new MemoryStream())
  {
    Serializer.Serialize(ms, o);
    ms.Position = 0;
    var o2 = Serializer.Deserialize<I>(ms);
    return ((T)o.Data["C"]).Count == ((T)o2.Data["C"]).Count;
  }
}

The code is run for all the permutations of:
TBase : { IList<int>, ICollection<int>, IEnumerable<int> }
autoCompile : { true, false }
isDefault : { true, false }

What is the expected output? What do you see instead?

The expected output is that all the 12 lines of output print True.
However, the actual output is not always True! The problems occur when TBase is 
 IEnumerable<int>:
 * autoCompile = true AND isDefault = true yield False
 * isDefault = false yield InvalidOperationException

See the attached file output.txt for the complete output.

What version of the product are you using? On what operating system?

v2, the most recent trunk revision (407)

Please provide any additional information below.

The most strange result is for the default model. Seems like the compiled 
default model produces different results than the same non compiled model.

Original issue reported on code.google.com by mark.kha...@gmail.com on 12 Jun 2011 at 9:09

Attachments:

• program.cs
• output.txt

[GoogleCodeExporter]

I have added another TBase option - object. The results are similar to  those 
of IEnumerable<int>, i.e. the compiled default model produces an incorrect 
result, while a new model throws the same InvalidOperationException.

Now, I might be doing something wrong during the creation of a new model, but 
the default model should produce identical results be it compiled or not, 
shouldn't it?

Original comment by mark.kha...@gmail.com on 12 Jun 2011 at 9:41

[GoogleCodeExporter]

Original comment by marc.gravell on 13 Jun 2011 at 6:21

• Changed state: Started

[GoogleCodeExporter]

Enumerable (repeated) data is limited to the format defined in the protobuf 
specification. Inheritance cannot be supported here without significant 
variance from the specification.

The model has been changed to raise appropriate messages here.

Original comment by marc.gravell on 13 Jun 2011 at 8:06

• Changed state: WontFix

[GoogleCodeExporter]

But how is it possible that it does work correctly with the default model
when AutoCompile == false? What is the relevance of the protobuf
specification to the fact that you compile the decorators? This sounds a bit
strange. Can you explain, please?

Original comment by mark.kha...@gmail.com on 13 Jun 2011 at 8:33

[GoogleCodeExporter]

Ultimately, it is a behaviour in an unanticipated and unsupportable scenario. 
The behaviour is not defined. The significance of compilation is simply 2 
different undefined implementations.

Original comment by marc.gravell on 13 Jun 2011 at 9:29

[GoogleCodeExporter]

I understand what you are saying and it is acceptable, but only from the 
mathematical point of view. Observe it from the pragmatic point of view. It is 
like if compiled Regex behaved differently than the non compiled one.

Now, I understand the technical difficulty. Is it because IEnumerable<T> does 
not have the Add method? May I suggest a possible solution to this problem? 

You seem to select the serializer based on the type passed to the Add method, 
even if that type is an interface or an abstract type while it is clear that 
such types are never instantiated. Such a type must have either subtypes or the 
construct type associated with it. Take the first subtype and select the 
serializer based on it. Next, make sure that all the other subtypes are 
compatible with this serializer. If not - abort with exception.

This approach can be extended to concrete types as well, like object. Only 
there we have two choices - either apply the currently existing serializer 
selection procedure (because, after all, the type is concrete) or the new one, 
proposed above. The actual choice may be governed by some kind of a flag.

Does it make any sense to you?

Original comment by mark.kha...@gmail.com on 13 Jun 2011 at 9:57

[GoogleCodeExporter]

The issue, more specifically, is considering how to deserialize into such a 
property. Consider the case when it is non-null, non-settable and all we know 
is IEnumerable<T>. For the *reflection* model, it might be reasonable to check 
this for IList/IList<T>, but this is not an attractive option in the compiled 
version - the preference is to already know about a suitable Add method 
resolved from the container type. If the container type is IEnumerable<T>, this 
is then extremely problematic.

The actual items (and whether they are interfaces, etc) is largely orthogonal 
to the simple act of adding items to a collection.

Original comment by marc.gravell on 13 Jun 2011 at 10:46

[GoogleCodeExporter]

I do not understand something really basic. Is this statement from my previous 
respond correct: "Such a type must have either subtypes or the construct type 
associated with it"?

If yes, then I do not see a problem. We know what the actual type of that 
property, even when it is non-null and non-settable and it is the respective 
subtype or construct type. So, let us cast the IEnumerable<T> property in 
question to that type and continue as usual. What am I missing?

If no, then can you give an example?

Original comment by mark.kha...@gmail.com on 13 Jun 2011 at 10:54

[GoogleCodeExporter]

Marc?

Original comment by mark.kha...@gmail.com on 15 Jun 2011 at 4:38

[GoogleCodeExporter]

The issue is tied to the fundamental wire format. When serializing *an object*, 
I can squeeze additional type information into the object via a sub-message 
field, i.e. with classes Foo and Bar : Foo, I can serialize a Foo variable 
(that might actually be a Bar instance) as (text here for illustration only; 
unrelated to actual binary):

    Foo {
        [1 = bar Bar {
             [1 = barField1]
             ...
             [m = barFieldm]
        }]
        [2 = fooField2]
        ...
        [n = fooFieldn]
    }

which is fine for objects; however, LISTS of data are encoded (by the spec) 
simply as repeated fields, i.e. a Foo object with 5 Child items is encoded as

    Foo {
        [7 = child0]
        [7 = child1]
        [7 = child2]
        [7 = child3]
        [7 = child4]
    }

You'll notice that at no point is the *list itself* included. Hence there is 
nowhere to store any information about the concrete type of the list. The 
***only*** thing we know is that there are some repeated child elements, which 
might be there zero, once or many times.

Original comment by marc.gravell on 15 Jun 2011 at 10:21

[GoogleCodeExporter]

Still missing something:

   1. What is the difference between ICollection<int> and IEnumerable<int>,
   if all that matters is the wire format, which should be the same for both?
   After all, when TBase is ICollection<int> the code works and when TBase is
   IEnumerable<int> - it does not.
   2. It is not clear why TBase as IEnumerable<int> works in the *default
   uncompiled *model and does not work in the *default compiled *model.

Thanks.

Original comment by mark.kha...@gmail.com on 15 Jun 2011 at 10:53

[GoogleCodeExporter]

1: nothing; which is why neither is supported in this scenario
2: again, what you are seeing is (or rather: was) an **undefined scenario**; 
neither behaviour is "right" and neither is "wrong". The fluke that it worked 
via craziness doesn't make it a workable/supportable approach.

It is, however, no longer undefined. It should complain vigorously if you try 
this.

Original comment by marc.gravell on 15 Jun 2011 at 11:28

[GoogleCodeExporter]

Marc, may be I am stupid, but I still do not understand it. Let us observe your 
change to the file RuntimeTypeModel at revision 408, which does "complain 
vigorously":

if (type.IsInterface && typeof(IEnumerable).IsAssignableFrom(type) && 
GetListItemType(type) == null)
{
  throw new ArgumentException("IEnumerable[<T>] data cannot be used as a meta-type unless an Add method can be resolved");
}

This piece of code explains why ICollection<T> works and IEnumerable<T> does 
not - the GetListItemType finds the ICollection<T>.Add method and hence returns 
non null. So, this already breaks item (1) of your last response. And it is 
consistent with the behavior that I have observed - ICollection<int> as TBase 
indeed works for the default model, regardless of whether it is compiled or not.

Next comes the suggestion I made in my earlier response 
(http://code.google.com/p/protobuf-net/issues/detail?id=184#c6). 
GetListItemType(type) examines the interface type and naturally fails on 
IEnumerable<T>, because it declares neither Add nor indexer. Why would it be 
wrong to examine the first subtype instead? If it succeeds to determine the 
list item type, then make sure it is consistent with the rest of subtypes. If 
it does not succeed or there are conflicts between the subtypes - then 
"complain vigorously".

Please, tell me what is wrong with this reasoning?

Thanks.

Original comment by mark.kha...@gmail.com on 15 Jun 2011 at 12:09

[GoogleCodeExporter]

We were talking in the context of subclasses. I tried to go to some length to 
explain why subclasses of collections cannot work. When we eliminate the 
subject of sub-types (which does not apply - you should see "Repeated data (a 
list, collection, etc) has inbuilt behaviour and cannot be subclassed" ), my 
answer "1:" stands "as is".

Consider the difference between:

    public IList<SomeType> Items { get {...} }

and

    public IEnumerable<SomeType> Items { get {...} }

Now, I can't possibly change the object - or if I did, I can't store it (no 
set). The expected behaviour is that the data is added to the provided 
instance. The question of subclasses if also moot (as discussed previously) - 
we would never receive anything to say "the collection is actually 
one-of-these", as we never hear *anything* about the collection. Ever. It does 
not exist as far as the stream is concerned.

Even if subtypes did make sense here, taking the first is very brittle, and 
doesn't strike me as an expected behaviour (let alone: define "first" in a way 
that makes sense in all scenarios). Without an Add, there is nothing that can 
be done without making unreasonable assumptions about the underyling collection 
object (if any; the scenario shown could be an iterator block).

Original comment by marc.gravell on 15 Jun 2011 at 12:40

[GoogleCodeExporter]

I must admit, you are a very patient man. But I thank you for that, because I 
finally grok it.

Thank you very much.

Original comment by mark.kha...@gmail.com on 15 Jun 2011 at 12:55

[GoogleCodeExporter]

Hi.
I apologize for reviving this discussion. Let me go back to the 
MetaType.AddSubTyoe method:

public MetaType AddSubType(int fieldNumber, Type derivedType)
{
  if (!(type.IsClass || type.IsInterface) || type.IsSealed) {
    throw new InvalidOperationException("Sub-types can only be added to non-sealed classes");
  }
  if (typeof(IEnumerable).IsAssignableFrom(type))
  {
    throw new ArgumentException("Repeated data (a list, collection, etc) has inbuilt behaviour and cannot be subclassed");
  }
...

Now, have a look at the following short program:

    public interface IMobileObject { }
    public class MobileList<T> : List<T>, IMobileObject
    {
      public override bool Equals(object obj) { return this.SequenceEqual((IEnumerable<T>)obj); }
    }
    [ProtoContract]
    public class A : IMobileObject
    {
      [ProtoMember(1)]
      public int X { get; set; }
      public override bool Equals(object obj) { return ((A)obj).X == X; }
    }
    [ProtoContract]
    public class B
    {
      [ProtoMember(1)]
      public List<IMobileObject> Objects { get; set; }
    }
    static void Main()
    {
      var m = RuntimeTypeModel.Default;
      m.AutoCompile = false;                     // (*)
      m.Add(typeof(IMobileObject), false).AddSubType(1, typeof(A)).AddSubType(2, typeof(MobileList<int>));                        // (**)

      var b = new B { Objects = new List<IMobileObject> { new A { X = 3 }, new A { X = 17 }, new MobileList<int> { 3, 7 } } };
      using (var ms = new MemoryStream())
      {
        Serializer.Serialize(ms, b);
        ms.Position = 0;
        var b2 = Serializer.Deserialize<B>(ms);
        Debug.Assert(b.Objects[1].Equals(b2.Objects[1]));
        Debug.Assert(b.Objects[2].Equals(b2.Objects[2]));
      }
    }
  }

Note the line (**). It does not throw, implying that IMobileObject is indeed 
the superclass of MobileList<int>. 
Is it so by design? 

Original comment by mark.kha...@gmail.com on 20 Jun 2011 at 7:56

[GoogleCodeExporter]

I have changed a couple of things (just emailed you the svn diff output). It is 
possible that one of my changes produces the following side effect - the code 
from http://code.google.com/p/protobuf-net/issues/detail?id=184#c16 succeeds 
when AutoCompile == false and fails when AutoCompile == true.

Again, the scenario implemented in the code sample is currently legal (it 
passes your recent changes). So, the question should it be legal?

Original comment by mark.kha...@gmail.com on 20 Jun 2011 at 8:28

[GoogleCodeExporter]

That *probably* isn't going to hurt anything. It certainly doesn't have
anything *like* the same potential to screw things up, so I don't see a need
to aggressively disallow it. Unusual, perhaps.

Marc

Original comment by marc.gravell on 21 Jun 2011 at 5:12

[GoogleCodeExporter]

OK, then could you examine the patches I emailed to you yesterday? I think that 
by patching ListDecorator.cs I have created a situation where the sample code 
above works OK with AutoCompile == false and does not work with AutoCompile == 
true, where my fix made it work with AutoCompile == false.

Now, the patch is really innocent, it looked like a leftover from some old code 
before. Can you please, have a look at it? If you find that my patches are OK, 
then there is a bug with AutoCompile == true failing to deserialize the list 
from the example above.

Thanks a lot.

Original comment by mark.kha...@gmail.com on 21 Jun 2011 at 5:18

[GoogleCodeExporter]

The ListDecorator patch would, I believe, cause a serious regression in
LINQ-to-SQL's EntitySet in .NET 3.5 (fixed in .NET 4.0). I'd rather look at
this individual scenario and investigate what is happening. Looking at the
scenario sent, and running a few tests, if implemented "as is", this could
cause:

- deserialization callbacks to not be invoked when constructing a list that
is a subclass
- graph-tracking to not work correctly in this case (in the root-object
case)

I *might* be able to get that scenario to work, but at current it is looking
(contrary to my earlier replier) fairly problematic...

Marc

Original comment by marc.gravell on 21 Jun 2011 at 6:04

[GoogleCodeExporter]

Thanks, Marc. Only if you could be a bit more specific about your intents 
pertaining to this issue. Supporting this scenario makes me port our 
application much easier...

Original comment by mark.kha...@gmail.com on 21 Jun 2011 at 6:55

[GoogleCodeExporter]

Can you estimate roughly when do you think you would be able to turn your 
attention to it? I totally understand that this issue is of low priority, I 
just need to know, so that I can plan my porting tasks.

Thanks.

Original comment by mark.kha...@gmail.com on 26 Jun 2011 at 2:10

[GoogleCodeExporter]

Marc, may I open another issue for 
http://code.google.com/p/protobuf-net/issues/detail?id=184#c16 ? I just do not 
want it to fall between the chairs.

Thanks.

Original comment by mark.kha...@gmail.com on 13 Jul 2011 at 7:35

[GoogleCodeExporter]

that is indeed a case that won't work, due to the different ways lists vs 
single entities must be considered. I'll strengthen the detection there.

Original comment by marc.gravell on 13 Jul 2011 at 8:10

[GoogleCodeExporter]

Are you sure?
From http://code.google.com/p/protobuf-net/issues/detail?id=184#c20 it follows 
that the only problem is with "LINQ-to-SQL's EntitySet in .NET 3.5".

Does it mean that you have found a conceptual problem? Because, it seemed to me 
like a trivial fix, which worked (ignoring the "LINQ-to-SQL's EntitySet in .NET 
3.5" issue) and saved me a lot of work.

I am kind of taken aback by these news.

Original comment by mark.kha...@gmail.com on 13 Jul 2011 at 8:30

[GoogleCodeExporter]

I'm really not sure it *did* work, at least not in the complete range of 
scenarios of inheritance usage (serializing the base, deserializing the 
derived).

I sense your frustration, and I'm not intentionally trying to annoy, but there 
are some pretty fundamental issues between single instances and lists, and 
anything that tries to cross that bridge in a simple way is likely going to hit 
problems. I *am*, however, currently assessing a shim to inject a fake (not 
really there) layer in front of some list, to see if that solves a similar set 
of problems. 

Original comment by marc.gravell on 13 Jul 2011 at 10:51

[GoogleCodeExporter]

I really appreciate it.
Thanks.

Original comment by mark.kha...@gmail.com on 13 Jul 2011 at 11:04