mbraceproject / FsPickler

A fast multi-format message serializer for .NET
http://mbraceproject.github.io/FsPickler/
MIT License
324 stars 52 forks source link

Default serializable behavior in C# #74

Closed jasmh closed 8 years ago

jasmh commented 8 years ago

Hi,

first, I find FsPickler absolutely fantastic. I've been working with .NET since day 1, so far I have been using BinarySerializer, DataContract, NetDataContract and Newtonsoft.Json extensively and have been playing around with Wire and ProtoBuff. After reading your Technical Overview, I can totally understand and appreciate everything you wrote there.

I find only one thing missing that would make your library perfect, make it way more popular in the C# community and practically render most if not all the other .NET serialization libraries obsolete. However, this one thing may be problematic considering the philosophy in your specs, but I thought I should still ask and see what you think about it.

Maybe I'm wrong, but I assume that your primary target environment was F# and that making everything work in C# too was more of a side effect than an actual goal. Stating the obvious, the main difference between the two environments is the default behavior of the [Serializable] attribute, where, unlike in C#, it's by default automatically added for every type in F#. Without debating why such a decision was made by the .NET framework designers, I am wondering how you would design your library if such a default behavior wasn't introduced in F#. Would you require it to be present?

IMHO, your library is perfect for F#, but this default .NET framework behavior presents a major problem when using it in C# and I am not the only one that thinks that. I think that one of the reasons why Newtonsoft.Json is so popular is that, unlike those other .NET serializers, it doesn't force you to decorate POCO with anything. I know a lot of people that are using it for this very purpose despite it being much slower than the alternative solutions.

I am also aware that you introduced a dynamic way of marking something as serializable so that one can even handle inaccessible classes in third-party libraries, but it's all unnecessary manual work. Like with Newtonsoft.Json, I wouldn't mind if your default POCO serialization logic would automatically kick in if a class doesn't satisfy any of those special conditions that you have (is marked with ISerializable or [DataContract] or is Exception class, etc.) and if the default logic can't resolve it, since a class is not a simple POCO, then an error happens and one needs to resort to writing a custom pickler.

In other words, would you possibly consider providing some sort of mechanism to treat every class as [Serializable] by default without having to specify that explicitly? Maybe some sort of configuration flag or a static property (false by default), that, if turned on, would remove the necessity for mandatory [Serializable] attribute?

I would understand if you don't want to introduce such a behavior, but in that case could you at least help me to achieve this myself. When going through your code, I think I found the place responsible for handling this behavior. I believe it's in the internal ClassFieldPickler.Create method where you are throwing the NonSerializableTypeException if none of the serializable conditions are satisfied. Would that be the only thing that needs to be changed or is it more complex than that?

eiriktsarpalis commented 8 years ago

Hi Jasmin,

Thanks for the detailed feedback. The original motivation for barring anything that does not satisfy the System.Type.IsSerializable property really stems from the fact that most of the mscorlib types that do not specify it are unsafe to serialize, such as FileStream and WebClient. I must admit that this decision has been made easier by the fact that serialization in F# is opt-out as opposed to opt-in. But it seems that this really has hurt interop with C# libraries that neglect to specify the Serializable attribute in types that should have been so.

I recently added a feature that makes declaring types serializable by means of predicates. For instance, you could basically whitelist everything by declaring:

FsPickler.DeclareSerializable (fun (_ : Type) -> true)

at the beginning of your program. Obviously, you might want to refine this by excluding mscorlib types which can include runtime internals such as pointers:

FsPickler.DeclareSerializable (fun (t : Type) -> t.Assembly <> typeof<int>.Assembly)
eiriktsarpalis commented 8 years ago

BTW, I always thought that adding a Serializable annotation at a type definition does not affect its POCO-ness, given that it this is a runtime-integrated attribute that does not introduce any third-party serialization dependencies.

jasmh commented 8 years ago

Why did I even doubt, THANK YOU IMMENSELY, and such an elegant solution. I don't know how I missed it, you should make it more prominent in your overview/tutorial articles.

As for your remark, I completely agree, [Serializable] is just a marker attribute that by default doesn't do anything and hence doesn't afect class's POCO-ness. It's not about that. It's about it being false by default in C#. Since the majority of classes in most libraries are POCOs or complex/combined POCOs, i.e. they are serializable from their field values, the default value should have been set to true by default and forced to false via attribute like it is in F#. This way it's up to us developers to be careful and whenever we are designing a library to decorate every single thing with [Serializable] in case someone would need it in some serialization scenario. Call it laziness or violation of YAGNI principle, but I see that as too much boilerplate work. It would be fun to see how many C# libraries out there are actually doing this. It's a shame that you can't reuse someone's open source work just because they didn't predict that it should be run in some sort of remoting environment. Anyway, I guess Microsoft will never change this, since it would be a huge breaking change now. Besides, why bother, when you solved the whole thing with a simple lambda :)...

Once again, thanks. All the best...

eiriktsarpalis commented 8 years ago

Cool, assuming then that I can close this.