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

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

Closed
AArnott opened this issue Jan 25, 2024 · 7 comments · Fixed by #1802
Closed
Assignees
Milestone

Comments

@AArnott
Copy link
Collaborator

AArnott commented Jan 25, 2024

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).

@AArnott AArnott added this to the v3.0 milestone Jan 25, 2024
@AArnott AArnott self-assigned this Jan 25, 2024
@neuecc
Copy link
Member

neuecc commented Jan 26, 2024

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
Copy link

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
Copy link
Collaborator Author

AArnott commented Mar 14, 2024

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

@pikpok-lihhern
Copy link

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
Copy link

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
Copy link
Collaborator Author

AArnott commented Apr 3, 2024

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
Copy link
Collaborator Author

AArnott commented Apr 20, 2024

I've begun work on this.

AArnott added a commit that referenced this issue Apr 22, 2024
As the formatter must have access (per C# rules) to the private members, the formatter must be nested under the data type.
This requires the data type (and any nesting types of that data type) to be declared as `partial` so that the source generated code can add the formatter as an `internal` member of it.
Since traditionally data types haven't been required to be partial, we only nest the formatter under the data type when private members must be serialized; otherwise we continue to generate the formatter as nested under the generated resolver.

Closes #1745
AArnott added a commit that referenced this issue Apr 27, 2024
As the formatter must have access (per C# rules) to the private members, the formatter must be nested under the data type.
This requires the data type (and any nesting types of that data type) to be declared as `partial` so that the source generated code can add the formatter as an `internal` member of it.
Since traditionally data types haven't been required to be partial, we only nest the formatter under the data type when private members must be serialized; otherwise we continue to generate the formatter as nested under the generated resolver.

Closes #1745
@AArnott AArnott closed this as completed May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants