Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional support for opt-in public member serialization? #342

Open
MaxSavenkov opened this issue Oct 17, 2024 · 6 comments
Open

Optional support for opt-in public member serialization? #342

MaxSavenkov opened this issue Oct 17, 2024 · 6 comments

Comments

@MaxSavenkov
Copy link

When switching to MemoryPack from another serialization solution, it would be useful to have an option to avoid automatically serializing all public members of MemoryPackable classes, because marking them all with MemoryPackIgnore is too much work and makes code ugly. I propose to add an option to MemoryPackable attribute, or, if it's too much work for generator, maybe add a separate MemoryPack.OptIn attriubte?

@PragmaGame
Copy link

Is it possible in MemoryPack not to serealize the base class fields?

@MaxSavenkov
Copy link
Author

@PragmaGame With a hack. You can declare those fields as "private new" in derived classes, and don't mark them with MemoryPackInclude. This should prevent their serialization, I think. But in general, this is very suspicious, so you might want to rethink your hierarchy if you really need this.

@Grimeh
Copy link

Grimeh commented Jan 1, 2025

I'd like to echo this, it would be incredibly helpful for using MemoryPack in existing frameworks.

As an example, if I'm using Godot and want to define a class that is both MemoryPackable and accessible via GDScript (done by inheriting from GodotObject). To do so I must use the workaround described above (thanks @MaxSavenkov!) and explicitly shadow NativeInstance in order to be able to add [MemoryPackIgnore]:

[MemoryPackIgnore]
public new IntPtr NativeInstance => base.NativeInstance;

GodotObject.NativeInstance is actually an expression-bodied property, I very surprised that this is included by default since it cannot be deserialised. It will silently fail to generate correct deserialisation code if there is no custom constructor that manually accounts for this. But that's a problem for a separate issue.

@Grimeh
Copy link

Grimeh commented Jan 1, 2025

If possible I'd love to have more options than a blanket "don't include anything by default". In my case I'd like to disable auto-inclusion of all properties.

Perhaps a new AutoIncludeMode enum, or AutoIncludeFlags bitflag enum?

public enum AutoIncludeMode {
    // No fields or properties are included by default
    OptIn, // alternatively 'None'
    // Only fields are included, no properties
    IncludeFields,
    // Only fields and auto-properties are included
    IncludeAutoProperties,
    // All public fields and public properties are included
    IncludeProperties, // alternatively 'IncludeAllPublic'
}

Or:

[System.Flags]
public enum AutoIncludeFlags {
    None = 0,
    // Include public fields
    Fields = 1 << 0,
    // Include public auto-properties
    AutoProperties = 1 << 1,
    // Include expression-bodied properties
    ExpressionProperties = 1 << 2,

    All = Fields | AutoProperties | ExpressionProperties,
}

(note that I'm lumping all non-auto-properties into ExpressionProperties above, I'm not sure how best to express that differentiation)

Which would allow more control over the automatically included members, from least to most inclusion:

  1. AutoIncludeMode.OptIn / AutoIncludeFlags.None - Don't include anything by default (described in OP)
  2. AutoIncludeMode.IncludeFields / AutoIncludeFlags.Fields - Don't include properties by default, ie. only public fields are included by default
  3. AutoIncludeMode.IncludeAutoProperties / (AutoIncludeFlags.Fields | AutoIncludeFlags.AutoProperties) Don't include non-auto-properties by default, ie. properties that don't clearly represent a value in memory
    • Doesn't include public int MyExprBodiedProp => this.myPrivateInt
    • Doesn't include public int MyWrapperProp { get => this.myPrivateInt; set => this.myPrivateInt = value; }
    • DOES include public int MyAutoProp { get; set; }
    • DOES include public int MyAutoProp { get; private set; }
  4. (DEFAULT) AutoIncludeMode.IncludeProperties / AutoIncludeFlags.All - Include all public members, this is the current default behaviour

AutoIncludeFlags would allow finer-grain control (eg. excluding fields and including properties) but I imagine 99% of use cases would be covered by AutoIncludeMode.

Personally 2 or 3 is what I expected the default behaviour to be, but I use properties quite sparingly and so my expectations are probably not aligned with most dotnet programmers.


I recognise that changing the default behaviour at this point in time is probably not desirable, nor is it possible without some very nasty surprises for users (even if gated behind a major version).

It would be theoretically possible to configure the project-wide default behaviour via preprocessor defines (assuming source generators are affected by them) but that may be undesirable since identical class definitions in two separate dotnet projects (eg. client, server) could produce incompatible schemas if they have different ConstantDefines.

@MaxSavenkov
Copy link
Author

Fine-grained control seems an overkill? Opt-in/Opt-out switch + Include/Ignore attributes should take care of most cases, imo. To base inclusion of a member on field/property distinction seems like inviting troubles, since this is not a natural approach to type design. But maybe you have an use-case for this? I'm writing my own serialization library right now, because MemoryPack doesn't quite fits my needs, and right now it's opt-in-only (you must mark each field/property), since it's the safest approach in a huge legacy code-base worked on by many people of different levels of skill. But if you can make an argument for more fine-grained control, I might include it in my library :)

@Grimeh
Copy link

Grimeh commented Jan 2, 2025

I don't think it's overkill to differentiate between fields and properties at all. Properties are ambiguous by nature so it's a necessary level of control for serialisation.

Properties may or may not be "field-like" in the sense of representing object state. They're used to refer to transient data just as often as they wrap object state in my experience (not even auto-properties will always be a direct 1-to-1 mapping).

I'll admit the distinction between auto-properties and other properties is probably overkill, that's mostly there for convenience since it seems to be common practice to use auto implemented properties in place of fields in modern C#.

From the perspective of the API consumer they look and feel the same as fields, so it does feel "natural" to treat them the same. I assume that's why a few C# serialisation libraries I've used serialise them by default, just seems to be the defacto default at this point.

While fields and properties may look and feel the same, they don't behave the same. That disconnect leads to issues when they're treated the same, at least in the context of serialisation. Fields always represent data, usually object state, so it's usually a safe bet to assume the user will want to serialise them. On the other hand, properties are syntactic sugar for a get method and/or a set method for... whatever you want, really. That distinction is important regardless of experience level.

Unity is a good example. As you may already know, Unity doesn't serialise public properties by default (and thus aren't visible in the editor inspector), this is a common cause of confusion for junior programmers who are (understandably) under the impression that properties are "fancy fields" and are treated as such. So they search for a fix, find the [field: SerializeField] hack and the "fancy field" misunderstanding is reinforced.

But then someone renames the property, adds [FormerlySerializedAs("MyProperty")], and any existing value silently fails to load since MyProperty isn't the name of the compiler-generated backing field, <MyProperty>k__BackingField is. The name of the backing field isn't standardised and may be changed without warning so the fact that the [field: SerializeField] hack works at all is kind of by accident.

While that example isn't directly applicable to MemoryPack, it's this lack of distinction that leads to situations like the above (and #344/#357). So I think we do more harm than good when we gloss over the distinction, we shouldn't conflate the two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants