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

Ignore members with default values #89

Closed
AArnott opened this issue Nov 16, 2024 · 5 comments · Fixed by #95
Closed

Ignore members with default values #89

AArnott opened this issue Nov 16, 2024 · 5 comments · Fixed by #95
Assignees
Labels
enhancement New feature or request

Comments

@AArnott
Copy link
Owner

AArnott commented Nov 16, 2024

The second most voted feature request on MessagePack-CSharp requests for reducing serialized payloads by omitting properties with default values.

This most readily makes sense when serializing objects as maps, where the property name can be dropped along with the 0/nil value which itself is just one byte. When the object is serialized as an array of values, serialization payload savings are only feasible if they happen to all be at the end of the array such that the array can be truncated.

Default values could potentially be different than default(TPropertyType), since the object constructor can initialize these fields to other values. Respecting [DefaultValueAttribute(value)] seems like a no-brainer.
Another option would be a property naming pattern where property X does not serialize if property ShouldXSerialize returns false, which gives ultimate runtime support for controlling what gets serialized.

CC: @eiriktsarpalis

@AArnott
Copy link
Owner Author

AArnott commented Nov 16, 2024

@eiriktsarpalis One of your test cases in the exhaustive set fail when I turn this setting on, because it sets a bunch of default values explicitly. And no [DefaultValue] attribute exists either. As a result, I don't serialize values that are default(TPropertyType), which ends up allowing the default constructor parameter value apply, leading to a change in value.
Any suggestions as to how I can determine what to consider the default value for at least this cases?

public record RecordWithSpecialValueDefaultParams(double d1 = double.PositiveInfinity, double d2 = double.NegativeInfinity, double d3 = double.NaN, double? dn1 = double.PositiveInfinity, double? dn2 = double.NegativeInfinity, double? dn3 = double.NaN, float f1 = float.PositiveInfinity, float f2 = float.NegativeInfinity, float f3 = float.NaN, float? fn1 = float.PositiveInfinity, float? fn2 = float.NegativeInfinity, float? fn3 = float.NaN, string s = "\"\ud83d\ude00\ud83c\udc04\r\n\ud83e\udd2f\ud801\udc00\ud801\udc28\"", char c = '\'') : IShapeable<RecordWithSpecialValueDefaultParams>

It looks like your source generated ArgumentStateConstructorFunc retains the default values, so if I created that struct and knew how to read those values (as opposed to just setting them, which is all I do at present), then that would help. But it seems it would only help the primary constructor/init property case, while ordinary get/set properties can also have explicit defaults.

@eiriktsarpalis
Copy link
Collaborator

I'm not sure I entirely follow the requirement, but is it that you need getters exposed on the argument state type? What about comparing against IPropertyShape.DefaultValue before calling the setter?

@AArnott
Copy link
Owner Author

AArnott commented Nov 16, 2024

What about comparing against IPropertyShape.DefaultValue before calling the setter?

That would be awesome. Except there is no such property. You link to IConstructorParameterShape, which does have that property. I hadn't noticed that and I can and should use that. But for the properties that are not on the constructor (and/or types with a default constructor and no init properties), I'll still need the default values there. e.g.

[GenerateShape]
partial class A
{
   public int Age { get; set; } = 18;
   public int Field = 15;
}

@AArnott
Copy link
Owner Author

AArnott commented Nov 16, 2024

Now, default parameters are limited to values that C# can express as constants, whereas property and field initializers are allowed to be any runtime-constructible value. However, we know that these initializers are limited in that they cannot access this, so they are essentially statically executable. So theoretically your source generator could lift those initializers out of context to run them within a shape delegate. It could get very tricky though, because you'd have to keep every binding the same, and C# context would change so you might actually have to change the syntax around.

A simpler scope for the fix would be to start with only supporting literal expressions in field and property initializers. But that's more limited even than 'any constant expression' that default parameters can specify...

@AArnott
Copy link
Owner Author

AArnott commented Nov 16, 2024

Although, I guess field and property initializers not being supported and instead asking users to apply a DefaultValueAttribute would match their previous experience with reflection-based serialization. So combining that with your constructor default parameter feature may be sufficient.

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

Successfully merging a pull request may close this issue.

2 participants