Bug: force-included properties without accessible setters are silently ignored

[MaxSavenkov]

[MemoryPackable]
class A
{
    [MemoryPackInclude]
    public string SomeProperty { get; private set; }
}

When generating code for such class, MemoryPack will silently skip setting SomeProperty (but will generate code to write/read it), because it doesn't have accessible setter (because it will be unable to properly generate code for SET label, but since this is not a readonly field, it doesn't get passed to constructor, so both paths fail). This leads to many hours of debugging, because if you're adding attributes to existing fields, private, or omitted setters are easy to miss (doubly so, if you're doing it using some kind of automation in a big project).

I propose extending the check for readonly fields to properties that are marked with [MemoryPackInclude] attribute, because it explicitly tells the generator I want this property serialized. If it is absent, and the field is public, the situation becomes less clear, but maybe a warning would appropriate, too?

[Grimeh]

Pretty sure the presence of an explicit [MemoryPackInclude] won't change anything in your example since all public properties are included by default, auto-property or not.

Having said that, I tried to repro this based on your example but it seems to generate valid code for me. Unless I'm misunderstanding what you mean by "will silently skip setting SomeProperty" and you're not referring to the generated Deserialize method?

Here's an example definition:

[MemoryPackable]
public partial class Test {
	public int X { get; private set; } = 42;

	public void DebugSetX(int v) {
		X = v;
	}
}

Which generates the following:

    SET:
        
        value.@X = __X;
        goto READ_END;

    NEW:
        value = new Test()
        {
            @X = __X
        };

Seems to generate valid code for SET: and NEW:. Since it's in a partial declaration of the same type the private setter is accessible and it compiles successfully.

A rudimentary test seems to give correct results (Log.Info logs to the console):

var x = new Test(); // x.X is now 42
x.DebugSetX(231);   // sets x.X to 231
var bin = MemoryPackSerializer.Serialize(x);
var y = MemoryPackSerializer.Deserialize<Test>(bin);
Log.Info($"y.X: {y!.X}"); // if y.X is the default value of 42 or 0, something has gone wrong during deserialisation

This prints y.X: 231 which confirms that it successfully deserialises the non-default value. [MemoryPackInclude] doesn't seem to affect the generated code or results.

Perhaps the behaviour you're seeing is caused by something else?

[Grimeh]

It just occurred to me that we may be seeing the same issue.

Are you perhaps having seeing this issue specifically with readonly properties? Ie. properties with no setter? Eg. public int MyProperty => myInt or public int MyInt { get; }.
If so, it might be worth updating the title to replace "force-included properties without accessible setters" with "properties without setters", since private setters seem to work OK.

I've opened #357, which concerns changing the default inclusion behaviour of certain members. This issue seems focused on adding a warning so I don't think they're dupes even if they do refer to the same problem.

[MaxSavenkov]

Hm, I already forgot the exact circumstances :( The example in the first message does seem somewhat incorrect, possibly because I use heavily modified version of MemoryPack, which includes [MemoryPackableOptIn] attribute, which turns on opt-in mode, in which cases MemoryPackInclude becomes necessary, which is probably why it is used in this example.

But you're correct that this code should work as-is. The problem probably arises either in case there is no set accessor, as you suggested. Additionally, DIFFERENT problem happens when class is inherited:

[MemoryPackable]
public partial class A
{
    public int Property { get; private set; }

    public void Set(int value)
    {
        Property = value;
    }
}

[MemoryPackable]
public partial class B : A
{
}

In this case, invalid code will be generated for B. This just needs (yet another) diagnostic, I guess.

[Grimeh]

Oh huh, I hadn't considered private member inheritance. As you said, same issue, can't assign during deserialisation.

Sorry, my bad, private is accessible so the title is accurate. The example in the original post might be worth updating to your inheritance example and/or my missing setter example.