mgholam / fastJSON

Smallest, fastest polymorphic JSON serializer
https://www.codeproject.com/Articles/159450/fastJSON-Smallest-Fastest-Polymorphic-JSON-Seriali
MIT License
479 stars 147 forks source link

InvalidProgramException in the presence of static properties in structs #126

Closed pdinklag closed 3 years ago

pdinklag commented 3 years ago

For some reason, static properties are evaluated for JSON serialization: https://github.com/mgholam/fastJSON/blob/7980c6ec0d5a988b8d82bb6ca094ba1dafa3d9c1/fastJSON/Reflection.cs#L244 https://github.com/mgholam/fastJSON/blob/7980c6ec0d5a988b8d82bb6ca094ba1dafa3d9c1/fastJSON/Reflection.cs#L741

This results in invalid IL code to be generated when attempting to serialize or deserialize a struct that has any sort of public static property. Consider the following example:

using fastJSON;
using UnityEngine;

public class QuickTest : MonoBehaviour
{
    private struct Dummy
    {
        public static int StaticGetter => 0;
    }

    private class Data
    {
        public Dummy dummy;

        public Data()
        {
        }

        public Data(Dummy dummy)
        {
            this.dummy = dummy;
        }
    }

    private void Start()
    {
        JSON.ToJSON(new Data(new Dummy()));
    }
}

Executing Start results in the following exception:

InvalidProgramException: Invalid IL code in (wrapper dynamic-method) QuickTest/Dummy:_cgm (object): IL_0013: ret       

System.Delegate.CreateDelegate (System.Type type, System.Object firstArgument, System.Reflection.MethodInfo method, System.Boolean throwOnBindFailure, System.Boolean allowClosed) (at <9577ac7a62ef43179789031239ba8798>:0)
System.Delegate.CreateDelegate (System.Type type, System.Object firstArgument, System.Reflection.MethodInfo method) (at <9577ac7a62ef43179789031239ba8798>:0)
System.Reflection.Emit.DynamicMethod.CreateDelegate (System.Type delegateType) (at <9577ac7a62ef43179789031239ba8798>:0)
fastJSON.Reflection.CreateGetMethod (System.Type type, System.Reflection.PropertyInfo propertyInfo) (at Assets/Plugins/fastJSON/Scripts/Reflection.cs:732)
fastJSON.Reflection.GetGetters (System.Type type, System.Collections.Generic.List`1[T] IgnoreAttributes) (at Assets/Plugins/fastJSON/Scripts/Reflection.cs:792)
fastJSON.JSONSerializer.WriteObject (System.Object obj) (at Assets/Plugins/fastJSON/Scripts/JsonSerializer.cs:489)
fastJSON.JSONSerializer.WriteValue (System.Object obj) (at Assets/Plugins/fastJSON/Scripts/JsonSerializer.cs:165)
fastJSON.JSONSerializer.WritePair (System.String name, System.Object value) (at Assets/Plugins/fastJSON/Scripts/JsonSerializer.cs:544)
fastJSON.JSONSerializer.WriteObject (System.Object obj) (at Assets/Plugins/fastJSON/Scripts/JsonSerializer.cs:510)
fastJSON.JSONSerializer.WriteValue (System.Object obj) (at Assets/Plugins/fastJSON/Scripts/JsonSerializer.cs:165)
fastJSON.JSONSerializer.ConvertToJSON (System.Object obj) (at Assets/Plugins/fastJSON/Scripts/JsonSerializer.cs:39)
fastJSON.JSON.ToJSON (System.Object obj, fastJSON.JSONParameters param) (at Assets/Plugins/fastJSON/Scripts/JSON.cs:236)
fastJSON.JSON.ToJSON (System.Object obj) (at Assets/Plugins/fastJSON/Scripts/JSON.cs:212)
QuickTest.Start () (at Assets/Temp/QuickTest.cs:27)

The same code works fine if either of the following modifications is made:

  1. Make Dummy a class instead of a struct.
  2. Remove the static getter from Dummy.

So there's some kind of issue with static properties in structs, but not classes. Due to this, in engines such as Unity, you currently cannot serialize the very common Vector3 struct, because it has static getters. Even beyond Unity, it is a very common pattern to have static getters providing default instances of structs. As a workaround, removing the BindingFlags.Static flag from the lines stated above fixes the issue for me.

On a more philosophical level, I don't know why static properties are even considered for JSON serialization at all. This can have very bad unintended side effects and it's not something one would expect at all. Maybe this should be made optional?

EDIT: I should add that registering a custom serializer via JSON.RegisterCustomType does not help. The whole reflection part should be skipped if a type has a custom serializer.

mgholam commented 3 years ago

Hmmm... the rationale is that anything readable "should" be in the json.

So if you remove the static binding flag, then the data is not in the json? or there is no exception?

pdinklag commented 3 years ago

Hmmm... the rationale is that anything readable "should" be in the json.

OK, I believe whether static properties should be serialized may depend on the use case.

What I use fastJSON for right now is serialize data to later deserialize it in the same application, e.g., data that simply needs to be saved and loaded at a later point (e.g., user settings), or information transmitted between hosts in a network. I don't really want deserialization to write into static fields, because I think of a JSON document to represent an instance of something. In my opinion, there is no static in JSON and static fields shouldn't be serialized. But that's just my point of view, I realize that there may be other use cases where it makes sense.

I could resolve this simply by writing data holder classes that are used for the sole purpose of JSON serialization. However, doing that for every little struct that has a static field would blow up my code quite a lot.

So if you remove the static binding flag, then the data is not in the json? or there is no exception?

Both! The data is not in the JSON, because fastJSON won't even inspect static fields and properties anymore. That's fine, because I don't want or need static data to be serialized in my case.

Removing the flag also fixes the exception. The exception must be related to static properties in structs - it doesn't happen for classes. It complains about the ret instruction, which I find pretty weird, but I don't know the IL good enough to be able to tell what exactly the problem is here.

mgholam commented 3 years ago

Checkout v2.4.0.4, it skips static on structs.