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

Automatically generate properties for packet fields #21

Draft
wants to merge 55 commits into
base: dev
Choose a base branch
from

Conversation

TSRBerry
Copy link
Owner

@TSRBerry TSRBerry commented Dec 6, 2024

This PR adds a source generator to generate property implementations for packet fields.

C# 13 (.NET 9.0) adds support for partial properties and source generators are able to provide implementations for them.
This generator looks for the new PacketField attribute which can be applied to properties and generates the appropriate implementation based on the provided arguments and the property type.

It handles every packet field that's currently implemented by RyuSOCKS.

The generator currently supports the following features:

  • Constant and dynamic offsets for packet fields
  • Constant and dynamic lengths for packet fields
  • Little and big endian support for packet fields
  • Generation for the following property types:
    • simple numeric types (byte, sbyte, ushort, short, ushort, int, uint, long, ulong)
    • enums
    • strings
    • arrays of enums and simple numeric types
    • System.Net.IPAddress
  • Min/Max length checks for arrays and strings
  • Invocation of custom validation methods before executing the setter/getter functions

There are a few things that I want to do before this gets merged:

  • Add tests
  • Check other projects like https://github.com/dotpcap/packetnet and figure out if it makes sense to turn this generator into a standalone NuGet package
  • Fix all the FIXME comments
  • Create issues for all TODO comments
  • Consider implementing AddVerificationMethodIfNecessary() in a different way to make it less awkward to use
  • Restructure the helper properties and methods of the RyuSOCKS packets

Copy link
Contributor

@AnonymusNW AnonymusNW left a comment

Choose a reason for hiding this comment

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

Made some comments about possible improvements.
Also why are there multiple classes just called "TestPacket" or similiar?
In e.g.: RyuSocks.Generator.Test/data/PacketGenerator/Namespaces/TestPacket.cs

@TSRBerry
Copy link
Owner Author

Made some comments about possible improvements. Also why are there multiple classes just called "TestPacket" or similiar? In e.g.: RyuSocks.Generator.Test/data/PacketGenerator/Namespaces/TestPacket.cs

Thank you very much! I'll address your comments shortly.

All the files in RyuSocks.Generator.Test/data are used by PacketGeneratorTests.GeneratedSources_AsExpected(). It reads the source file, runs the generator and compares the results to the expected generated files (and also checks for compilation errors).
I recently learned that this seems to be called snapshot testing?

@TSRBerry TSRBerry requested a review from AnonymusNW December 11, 2024 21:07
@TSRBerry TSRBerry force-pushed the feature/packet-generator branch 3 times, most recently from 9633fd3 to 1972bb5 Compare December 12, 2024 18:27
Copy link
Contributor

@AnonymusNW AnonymusNW left a comment

Choose a reason for hiding this comment

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

lgtm

@TSRBerry TSRBerry force-pushed the feature/packet-generator branch from 1972bb5 to 4d6bde7 Compare December 16, 2024 22:00
@TSRBerry TSRBerry force-pushed the feature/packet-generator branch from 9a8e29f to c4c450b Compare January 4, 2025 19:58
@TSRBerry
Copy link
Owner Author

TSRBerry commented Jan 4, 2025

With the latest commits, all FIXME comments have been fixed!

We now support all string encodings which are exposed by System.Text.Encoding.
.NET Core also exposes Latin1 encoding, which is not supported, since we currently aren't able to test it properly.

The test coverage for the new encoding code is ~99%, the uncovered code is about exceptions, which I plan to add tests for soon.
And with that we also achieved ~99% test coverage of AccessorGenerator.Simple.cs!

@TSRBerry TSRBerry requested a review from AnonymusNW January 7, 2025 09:37
Copy link
Contributor

@AnonymusNW AnonymusNW left a comment

Choose a reason for hiding this comment

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

two remarks, else lgtm

@TSRBerry TSRBerry force-pushed the feature/packet-generator branch from da5e41a to 7f7c0e0 Compare February 16, 2025 01:29
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.

2 participants