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

Properties without a setter shouldn't be included by default #357

Open
Grimeh opened this issue Jan 1, 2025 · 1 comment
Open

Properties without a setter shouldn't be included by default #357

Grimeh opened this issue Jan 1, 2025 · 1 comment

Comments

@Grimeh
Copy link

Grimeh commented Jan 1, 2025

Hi! I really like MemoryPack (and pretty much everything else published under Cysharp, for that matter).

I initially ran into an issue with how all public members are included by default (described in #342), but that prompted a further dig into why a certain property was being included by default.

Expression bodied properties (ie. properties defined with =>) and readonly properties (ie. lacking a setter) are included by default if they are public. This is problematic because:

  • expression-bodied properties don't always represent/correspond to an actual value in memory, in my experience it's common for them to represent transient (ie. computed) values or straight up constants
    • imo this runs counter to expectations, or maybe just my mental model of how serialising runtime state should work
  • without a setter deserialisation cannot be accomplished without manual implementation via [MemoryPackConstructor]

Example:

[MemoryPackable]
public partial class MyClass {
	private int myInt;

	// these are all included without any warnings
	public int MyIntWrapper => myInt;

	public int MyIntSqrd => myInt * myInt;
	public float Pi => 3.14f;

	public int MyReadonlyInt { get; }
}

This produces the following sections in the generated MyClass.Deserialize:

[...]

            if (value == null)
            {
                reader.ReadUnmanaged(out __MyIntWrapper, out __MyIntSqrd, out __Pi, out __MyReadonlyInt);


                goto NEW;
            }
            else
            {
                __MyIntWrapper = value.@MyIntWrapper;
                __MyIntSqrd = value.@MyIntSqrd;
                __Pi = value.@Pi;
                __MyReadonlyInt = value.@MyReadonlyInt;

                reader.ReadUnmanaged(out __MyIntWrapper);
                reader.ReadUnmanaged(out __MyIntSqrd);
                reader.ReadUnmanaged(out __Pi);
                reader.ReadUnmanaged(out __MyReadonlyInt);

                goto SET;
            }

[...]

    SET:
        

        goto READ_END;

    NEW:
        value = new MyClass()
        {

        };

[...]

These values are serialised and will be read from the reader, but cannot be written to the result.
From what I can see the intended workaround for this is to use a constructor to manually handle this (with [MemoryPackConstructor] if there are multiple constructors).

There should be a warning if there is an included property without a setter and no parameterised constructor. This seems like a bug, and probably warrants its own issue (#344 might be referring to this problem but the example given isn't specific to properties lacking setters).

My intuition says that properties without setters shouldn't be serialised at all, particularly expression-bodied properties since they usually represent transient values. The lack of a setter is perfectly adequate justification for excluding them by default and requiring explicit inclusion from the user via [MemoryPackInclude], imo. This would make it clear to the user that this is a special case and a warning (described above) would quickly explain that further work is required for that to work.

@Grimeh Grimeh changed the title Expression bodied properties shouldn't be included by default Properties without a setter shouldn't be included by default Jan 1, 2025
@Darkar25
Copy link

Darkar25 commented Jan 5, 2025

I have one thing to add:
This applies to ICollection properties too.
But in my case i assumed that if the collection is read-only (i.e. automatically generated by the class) the MemoryPack would just check it for null and use the Add method provided by the ICollection interface, but after looking at the generated code it appears to not be the case. It serializes the collection, deserializes it into a temporary field and then... just does nothing with it?
This seemed quite strange to me until i stumbled upon your issue and then it became very clear to me why it behaves like that.

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

2 participants