MessagePack-CSharp / MessagePack-CSharp

Extremely Fast MessagePack Serializer for C#(.NET, .NET Core, Unity, Xamarin). / msgpack.org[C#]
Other
5.69k stars 694 forks source link

Source generate formatters as nested under the types they format for private member access #1745

Closed AArnott closed 5 months ago

AArnott commented 8 months ago

AOT formatters cannot access private members of the types they format today. This is a blocker for "always on" AOT source generation of MessagePack serializable types.

As a solution, I propose that we generate formatters as nested types within the types that they format. This would be a substantial breaking change for existing users because they have to change their type declarations to be partial. To minimize this, we should only generate the formatters as nested when they need to access private or protected members. For all-public formatters, they can remain as nested types under the source generated resolver.

@pCYSl5EDgo shared some code to help us get this right, since nesting formatters under the type to be formatted may require multiple levels of nesting, for a variety of types all the way up to the namespace (which may or may not be there itself).

neuecc commented 8 months ago

Making it partial and allows private sounds good. MemoryPack also adopts this design. Moreover, in MessagePack, the current necessity of passing AllowPrivate doesn't make for a great experience. Compared to this, enabling specific private classes to be partial seems like a step forward and makes things easier.

In line with this, the Analyzer should also be modified. Currently, writing a Key on a private member seems to be ignored without any warnings. At this point, an error message suggesting to make the class partial would be more user-friendly and easier to understand.

pikpok-lihhern commented 7 months ago

As a mid-way solution that doesn't break existing users, it would be great to allow formatter to also work with internal types and accessors. I think this makes sense for many projects that decouple their codebase via multiple assemblies.

AArnott commented 7 months ago

@pikpok-lihhern you're right. But we're not going to support both modes. It's hard enough to support one.

pikpok-lihhern commented 7 months ago

Yes, that's also fair. 🙂 Besides, it doesn't take a lot of work to upgrade the classes to partial for this to work anyway. 👍🏼

I'm looking forward to this as it will be very useful for how we setup our immutable data models. Much better than managing a constructor with a long list of parameters.

We've been avoiding the constructors approach and marking the properties with internal accessors, then before running MPC, we replace internals to public to trick MPC to generate the formatters. 😅 It's really gross, this is going to be a much better solution.

AndriySvyryd commented 6 months ago

Alternatively, you could use UnsafeAccessorAttribute for non-public members. It was specifically introduced to make serializers AOT-compatible and is almost as fast as accessing the member directly.

AArnott commented 6 months ago

That would be cool. But it requires .NET 8+, which leaves most existing .NET code unsupported. In a few years maybe that approach will make sense.

AArnott commented 6 months ago

I've begun work on this.