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

Features/incremental source generator #4039

Conversation

luizfbicalho
Copy link
Contributor

Closes #3068

@rockfordlhotka I added nullable reference types on the generator, please check if the changes to enable nullable that I added are ok for you

- Updated Target Frameworks in Csla.Generators.CSharp.Tests.csproj to net8.0.
- Removed AutoSerializableSyntaxReceiver.cs, indicating a shift in serialization strategy.
- Refactored DefinitionExtractionContext.cs to use SemanticModel directly.
- Introduced IncrementalSerializationPartialsGenerator.cs for efficient incremental generation.
- Removed SerializationPartialGenerator.cs and SerializationPartialsGenerator.cs, consolidating functionality.
- Updated Csla.Generators.CSharp.csproj with stricter code analysis and removed unnecessary folder inclusion.
- Modified ExceptionExtensions.cs for consistent exception message formatting.
- Added `Microsoft.CodeAnalysis.CSharp` namespace to `IncrementalSerializationPartialsGenerator.cs` for enhanced C# syntax manipulation.
- Refined the predicate in `Initialize` for attribute discovery to target only partial classes or structs with `AutoSerializableAttribute`.
- Simplified type definition extraction in `Initialize` by directly returning the method call's result, removing unnecessary variable declaration.
- Optimized `SerializationPartialBuilder` instantiation by reusing a single instance for all classes in the loop, reducing object creation overhead.
- Streamlined `generationResults` declaration and assignment in `Initialize` for conciseness.
Enabled Nullable Reference Types in the project file to improve null value handling across the codebase. Updated property types in `AddressPOCO.cs` and related POCO classes to support nullable reference types, ensuring properties can explicitly be null. Initialized properties with default values to avoid null issues. Cleaned up unused `using` directives in several files for better readability. Enhanced code generation to detect nullable fields/properties and adjusted serialization logic to comply with nullable reference types, including refined child object serialization and improved deserialization handling for nullable types. This comprehensive update enhances code maintainability and adopts modern C# practices.
This commit introduces several changes aimed at improving the handling of nullable reference types in the `SerializationPartialBuilder` class across different files. Specifically, the changes include:

- Modification of the `SerializationPartialBuilder` constructor in `IncrementalSerializationPartialsGenerator.cs` to accept a new boolean parameter indicating whether nullable context options annotations are enabled. This allows the builder to generate code that respects the project's nullable settings.

- Update to the `SerializationPartialBuilder` class in `SerializationPartialBuilder.cs` to include a new `nullable` parameter in its constructor. This parameter enables the generated code to correctly handle nullable reference types according to the project's settings.

- The `BuildPartialTypeDefinition` method in `SerializationPartialBuilder.cs` has been modified to conditionally add the `#nullable enable` directive at the beginning of the generated file, based on the `nullable` parameter. This ensures that the generated code respects the nullable settings of the project.

- Modification of the `AppendDeserializeChildFragment` method in `SerializationPartialBuilder.cs` to conditionally append a null-coalescing operation when deserializing non-nullable member types. This adjustment is made based on the `nullable` parameter and ensures that the deserialized value does not violate the type's nullability constraints.
- Refactored `GetFieldTypeNullable` in `FieldDefinitionExtractor.cs` and `PropertyDefinitionExtractor.cs` for more concise nullable type checks.
- Added documentation to `GetFieldTypeNullable` method in both files, improving clarity on its purpose and usage.
- Corrected a spelling error in `ExtractedMemberTypeDefinition.cs` for the `IsAutoSerializable` property comment.
- Introduced a `Nullable` property in `ExtractedMemberTypeDefinition.cs` to explicitly track type nullability.
- Removed unused, commented-out code in `SerializationPartialBuilder.cs` to clean up the codebase.
@rockfordlhotka
Copy link
Member

@luizfbicalho I've asked @TheCakeMonster for input here. I am not sure this code is current or valid anymore. It was never released into production.

This commit refactors the incremental generator to directly integrate nullable context options into its initialization and generation processes. A key addition is the extraction of nullable context options from the compilation provider, storing this information in a variable for later use. The process of combining compilation provider data with class declarations now includes nullable context information, streamlining the inclusion of nullable annotations in the generation process. Adjustments to the source output registration and `SerializationPartialBuilder` instantiation reflect this integration, simplifying the handling of nullable annotations and enhancing the generator's overall efficiency in dealing with nullable contexts.
@TheCakeMonster
Copy link
Contributor

@rockfordlhotka You are probably right that it isn't entirely current, but only in the sense that I have not checked it recently. I think it's a useful facility, so it's worth getting it released, unless there is a more modern replacement? I thought I saw an email of a merged PR for being able to serialize POCO types go past in the whizz of emails there have been in the past few days, but I can't keep up with them all at the moment.

The idea of switching to incremental source generation is a good one. It's quite a lot more efficient, I think, so it improves the build and development experience. @JasonBock is, of course, much more knowledgeable in this area than me; I simply did the grunt work, including changing it after his review.

There were a couple of reasons why it didn't get released:

  1. I couldn't remember how to change the nuget build to get it included. This is not something I know anything about, as I've never been in a position to need nuspec. This is the main reason.
  2. I started to wonder if this suffered the same security problem as BinaryFormatter, in that it might be capable of deserializing unintended types. I don't think it suffers quite the same problem, but that's very different from being able to say that I know that it doesn't.

Copy link
Contributor

@TheCakeMonster TheCakeMonster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A worthy set of changes, in my opinion. However, I can't approve them as such, just because I don't have experience of incremental generators. However, I know a man who does @JasonBock ;-)

@luizfbicalho
Copy link
Contributor Author

A worthy set of changes, in my opinion. However, I can't approve them as such, just because I don't have experience of incremental generators. However, I know a man who does @JasonBock ;-)

It's funny that I studied his videos to implement this generator

Enhanced the GenerateSingleLineExceptionMessage method in ExceptionExtensions.cs to better handle newline characters across Windows and Unix systems. The method now correctly replaces both `\r\n` and `\n` with semicolon separators, ensuring consistent single-line exception messages across different operating environments.
@rockfordlhotka rockfordlhotka requested review from rockfordlhotka and removed request for StefanOssendorf and TheCakeMonster June 26, 2024 00:50
@luizfbicalho
Copy link
Contributor Author

Is there anything else I can do about this PR?

@rockfordlhotka
Copy link
Member

Not right now. I'll review it when I get a chance.

Copy link
Member

@rockfordlhotka rockfordlhotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luizfbicalho

I don't see any issues with the code in the PR.

I am left wondering how it is actually used? I (like probably most CSLA devs) aren't familiar with this technology, and I'm not sure how I'd use it in my code?

For example, can I use this all by itself and MobileFormatter will "just work"? Or should it integrate into MobileFormatter using the new ability to add custom serializers? Should I use this instead of the built-in PocoSerializer that is reflection-based? If so, how?

I guess what I'm looking for is a readme.md in the Source/Csla.Generators` directory that tells someone how to get started.

rockfordlhotka and others added 4 commits July 3, 2024 14:16
Added a new section to the `readme.md` file titled "Using the Source Generator with AddressPOCO in CSLA.NET". This section provides a detailed guide on leveraging the CSLA.NET framework's source generator feature, focusing on the `AddressPOCO.cs` class. It includes prerequisites, a step-by-step guide on defining a POCO class with the `[AutoSerializable]` attribute, an explanation of the attribute and source generators, the process and benefits of using this approach for serialization, and an example of generated source code. This comprehensive guide aims to assist developers in utilizing source generators for efficient code generation and serialization within the CSLA.NET framework.
@luizfbicalho
Copy link
Contributor Author

I guess what I'm looking for is a readme.md in the Source/Csla.Generators` directory that tells someone how to get started.

I just added it,let me know what do you think of it

Copy link
Member

@rockfordlhotka rockfordlhotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the readme, thank you!

Just the change about the location of the AutoSerializable type and building the NuGet package and we should be good.

Source/Csla.Generators/readme.md Show resolved Hide resolved
@rockfordlhotka rockfordlhotka self-requested a review July 6, 2024 20:02
- Imported `Directory.Package.props` for centralized package management.
- Removed `GeneratePackageOnBuild` to disable auto NuGet package generation.
- Added `AssemblyName`, `RootNamespace`, and `PackageId` as `Csla.Generators.CSharp` for standardization.
- Introduced project `Description` and `Title` for NuGet metadata.
- Configured assembly signing with `SignAssembly` and specified `AssemblyOriginatorKeyFile`.
- Enabled XML documentation file generation with `GenerateDocumentationFile`.
- Added `PackageTags` "CSLA;Roslyn;Generator" for better NuGet discoverability.
- Disabled automatic generation of various assembly attributes to allow manual specification or because they are unnecessary.
Re-added the entire content of AutoSerializableAttribute.cs that was previously removed, including file header comments, namespace declaration, and the class definition. This restoration marks the class with the AttributeUsage attribute for automatic serialization of types, reinstating the file's original state with copyright notice and summary description.
@rockfordlhotka rockfordlhotka merged commit cf69a6c into MarimerLLC:main Jul 6, 2024
1 check passed
@luizfbicalho luizfbicalho deleted the features/incremental-source-generator branch July 9, 2024 17:05
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

Successfully merging this pull request may close these issues.

Upgrade source generators to the more efficient incremental generator style
4 participants