Add support for non-unrolled csharp arrays and nested structs

[arsher]

I have been using a fork of this library for a while now as I really wanted to avoid unrolling arrays in generated C# code and also wanted to have strings directly marshalled into public generated types. However over time it became more and more complex to keep it up to date with the latest changes. So I cleaned my code up a bit and made it work with the latest master to open this PR. I also believe that this should not be breaking existing functionality in any way.

I understand that larger changes typically go through an issue discussion first, but I wanted to check if there's interest in merging these improvements before opening an issue. If this aligns with the project’s direction, I’m happy to refine or split it further based on your feedback.

Looking forward to your thoughts.

[ralfbiedert]

First impression, this looks great, and your timing is perfect, I was just about to refactor some more codegen infra.

Minor nit so far, can you just move the csharp_reference_project_arrays into the main csharp_reference_project and make the tests fit in there?

Still reviewing ...

[arsher]

First impression, this looks great, and your timing is perfect, I was just about to refactor some more codegen infra.

Minor nit so far, can you just move the csharp_reference_project_arrays into the main csharp_reference_project and make the tests fit in there?

Still reviewing ...

The main reason I kept them separate is because the generated code is very different, but can have separate interop files generated for the non-unrolled version and merge the tests for sure. I'm looking into that next then.

[ralfbiedert]

Oh I see now! Would you say the non-unrolled versions are now "strictly better" than the unrolled ones?

The unrolled ones were ugly to start with, and if your marshaller takes care of that the better way forward might be to make your's the default and only have some smaller unit tests (e.g., akin to csharp_write_types for the unrolled flavors).

Or asked differently, why would I not always disable unrolling now?

[arsher]

Oh I see now! Would you say the non-unrolled versions are now "strictly better" than the unrolled ones?

The unrolled ones were ugly to start with, and if your marshaller takes care of that the better way forward might be to make your's the default and only have some smaller unit tests (e.g., akin to csharp_write_types for the unrolled flavors).

Or asked differently, why would I not always disable unrolling now?

Well, that is a good point, the main reason would be backwards compatibility. I wanted to avoid introducing any breaking change to the default behavior. I agree that in general this looks better though. If you think it's fine to introduce new defaults in the next version, I'm all for it.

[ralfbiedert]

Upcoming 0.15 is a good time to make that change, let's do it!

[ralfbiedert]

Ah one more, could you rebase / force-push your PR so that I can merge this via rebase? Right now there's something blocking that:

This branch cannot be rebased due to conflicts

[arsher]

Upcoming 0.15 is a good time to make that change, let's do it!

Sounds good, I'll merge the tests and change the default then.

Ah one more, could you rebase / force-push your PR so that I can merge this via rebase? Right now there's something blocking that:

Oh shoot, that might be because I merged master into it, it's kind of a reflex 😅 I'm not used to rebase workflows. I'll fix it.

[arsher]

So removed the new test project, changed the defaults and did a quick rebase. I'm hoping it looks better now.

Btw. regrading timing, there are a few issues in the dotnet/runtime repo that relate to this. There were plans to add better support for strings and structs in P/Invoke source gen, but they first have been pushed to .NET 9 and now to .NET 10, so unsure when it will be available sadly. When those improvements happen a lot of this can probably go as well when targeting the latest version.

[ralfbiedert]

Thanks a lot!

strings and structs in P/Invoke source gen, [...] When those improvements happen a lot of this can probably go as well when targeting the latest version.

Yes, better support there would be great!

In retrospect we got stuck on ancient and weird .NET code gen for too long because of Unity. I don't mind sticking with the latest (or latest-1) .NET moving forward, primarily moderated by the bang-for-the-buck it brings.

[ralfbiedert]

Follow up I didn't notice before. In the array marshaller you do:

    public partial struct Array
    {
        public byte[] data; // <- unspecified size
    }

and then later

var source = new ReadOnlySpan<byte>(managed.data, 0, Math.Min(16, managed.data.Length));
var dest = new Span<byte>(result.data, 16);
source.CopyTo(dest);

If I understand this correctly this silently truncates and / or leaves uninitialized (probably default initialized) elements. Shouldn't this have a size check and throw an exception if the managed array is anything else in size than the native one?

In other words, I'd say it's a programming error to use this with anything else than its designated size. As an implication, maybe Array should also have some helper constructor (method, ...?) to initialize it with its designated size.

[arsher]

Follow up I didn't notice before. In the array marshaller you do:

    public partial struct Array
    {
        public byte[] data; // <- unspecified size
    }

and then later

var source = new ReadOnlySpan<byte>(managed.data, 0, Math.Min(16, managed.data.Length));
var dest = new Span<byte>(result.data, 16);
source.CopyTo(dest);

If I understand this correctly this silently truncates and / or leaves uninitialized (probably default initialized) elements. Shouldn't this have a size check and throw an exception if the managed array is anything else in size than the native one?

Yes I was debating this with myself as for my purposes truncation was the correct behavior. In hindsight I should have added some docs to it at least. I think you are right though, in general this is not expected behavior. I'll change this to throw, and create a new PR. I can just have overloads in my code.

[ralfbiedert]

And for something totally different since I haven't used customer marshallers before: Would this actually allow for generic structs in C#? Like right now the biggest code gen PITA is having to do SliceU8, SliceU16, ... and all of that. Could we now have a C# Slice<T> or Option<T> and have some generic marshalling impl under the hood, maybe with some IMarshallable implemented for supported T?

[arsher]

And for something totally different since I haven't used customer marshallers before: Would this actually allow for generic structs in C#? Like right now the biggest code gen PITA is having to do SliceU8, SliceU16, ... and all of that. Could we now have a C# Slice<T> or Option<T> and have some generic marshalling impl under the hood, maybe with some IMarshallable implemented for supported T?

Documentation suggests generic marshallers should work, I'm happy to look into this as well.

[ralfbiedert]

Documentation suggests generic marshallers should work, I'm happy to look into this as well.

If that worked that'd be a massive improvement, I'd love to accept a PR for that!