C# / dotnet target for `diplomat-tool` #124

CBenoit · 2022-01-20T23:39:41Z

Hi,

I've been writing a dotnet target for diplomat-tool in order to expose a C# API to picky.
I actually used it to generate fully functional bindings to picky in my experiment repository. The only missing piece is a way to return a buffer of bytes (basically something like the DiplomatWriteable, but using std::io::Write instead of std::fmt::Write I guess).

I still have a few corner cases to fix, but I figured I could open this PR as a draft.

Todo before I consider the MVP complete

Fix generated code when bool type is in return position.
Special case to handle results using the unit () type as error.

Future improvements

Proper handling of Option of enum in parameters
Support custom types in managed non-opaque struct's fields (aka: avoid double freeing). Currently the only way to access custom types of non-opaque structs is to use the "raw" API with pointers.

Manishearth · 2022-01-20T23:44:35Z

Wow, this is amazing! cc @sffc @nciric

I can't commit to maintaining this backend, and I may still be making changes to the rest of the code here: I'm kinda fine with landing this here with the understanding that the backend may occasionally break (which I can always tag you in for). Does that seem okay to you? In general Diplomat is still kinda unstable so users should be installing specific git hashes (or cargo versions) anyway. Over time this should become less of a problem, it's just that right now there are still missing features and in the process of adding these features I can't (at the moment) commit to making sure the dotnet backend works too.

Another option is to create a separate dotnet repo that lives underneath rust-diplomat where the tool lives. Eventually I would like all diplomat plugins to be convenient to write this way, but that might not be the case right now, so I'm okay with adding more backends here for now, and it will certainly be the most convenient for you if it's in this repo for now.

(We may have more bandwidth to work on dotnet ourselves in the future, it's certainly something we'd like to support, and we're happy to make space for this backend to be worked on right now as well, just that we may not be able to commit to working on it too)

Manishearth · 2022-01-20T23:48:09Z

The only missing piece is a way to return a buffer of bytes (basically something like the DiplomatWriteable, but using std::io::Write instead of std::fmt::Write I guess).

Yeah I think we need some kind of DiplomatWriteable-like thing that allows one to return Vec<T>s. It's complicated though, since not all T types can interchange cleanly. Might be okay to restrict it to bytes.

In the meantime, it should be possible to define your own custom vector type that has an API exposed over FFI. That's the solution for the general case anyway.

nciric · 2022-01-20T23:59:01Z

Yay! Having C#/.net target is great. чет, 20. јан 2022. у 15:48 Manish Goregaokar ***@***.***> је написао/ла:

…

The only missing piece is a way to return a buffer of bytes (basically something like the DiplomatWriteable, but using std::io::Write instead of std::fmt::Write I guess). Yeah I think we need some kind of DiplomatWriteable thing that allows one to return Vec<T>s. It's complicated though, since not all T types can interchange cleanly. — Reply to this email directly, view it on GitHub <#124 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA7GEKXRVH4NEDU62GTDVLTUXCNMHANCNFSM5MOC56RA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

CBenoit · 2022-01-21T00:23:17Z

The only missing piece is a way to return a buffer of bytes (basically something like the DiplomatWriteable, but using std::io::Write instead of std::fmt::Write I guess).

Yeah I think we need some kind of DiplomatWriteable-like thing that allows one to return Vec<T>s. It's complicated though, since not all T types can interchange cleanly. Might be okay to restrict it to bytes.

Yeah, restricting to bytes (and maybe other primitive types) sounds like a good stopgap. Not urgent, since one can work around using a custom vector type as you mentioned, but still nice to have.

I can't commit to maintaining this backend, and I may still be making changes to the rest of the code here: I'm kinda fine with landing this here with the understanding that the backend may occasionally break (which I can always tag you in for). Does that seem okay to you? In general Diplomat is still kinda unstable so users should be installing specific git hashes (or cargo versions) anyway. Over time this should become less of a problem, it's just that right now there are still missing features and in the process of adding these features I can't (at the moment) commit to making sure the dotnet backend works too.

Another option is to create a separate dotnet repo that lives underneath rust-diplomat where the tool lives. Eventually I would like all diplomat plugins to be convenient to write this way, but that might not be the case right now, so I'm okay with adding more backends here for now, and it will certainly be the most convenient for you if it's in this repo for now.

(We may have more bandwidth to work on dotnet ourselves in the future, it's certainly something we'd like to support, and we're happy to make space for this backend to be worked on right now as well, just that we may not be able to commit to working on it too)

Sure, this sounds good to me 👍
As you said, I'll be working with a specific git hash, update when appropriate and send PR to fix breakages.
I was aware I would have to maintain the backend mostly myself for a while when I started working on it. It's some work but that's still a win considering I can now basically write a thin Rust layer and get idiomatic C# bindings for free. Writing wrappers by hand, while not complex, is still a pretty repetitive, time-consuming and somewhat error-prone task. As a bonus, I also halved the size of my Rust-side ffi modules thanks to diplomat macros. Great potential 😄

I'm happy to see you guys are interested as well!

Manishearth · 2022-01-21T00:59:43Z

Sounds great! Once this is ready and lands I'll be sure to tag you (but not block on you) for any changes to the dotnet module. I may feature-gate the dotnet module and add a separate CI job that does dotnet generation and testing if it ends up breaking often, but I suspect that may not be necessary.

Manishearth · 2022-01-21T01:01:33Z

Yeah, restricting to bytes (and maybe other primitive types) sounds like a good stopgap. Not urgent, since one can work around using a custom vector type as you mentioned, but still nice to have.

If you'd like to add such a feature I'd be happy to help out! Writeable is certainly a bit annoying implementation-wise so duplicating it is probably similarly annoying, but maybe not too bad.

CBenoit · 2022-01-22T00:47:16Z

Current state: C# bindings for feature_tests are correctly generated. I ported C++ unit tests to C# xunit, and got everything green.
I consider the MVP ready to be merged!

I may feature-gate the dotnet module and add a separate CI job that does dotnet generation and testing if it ends up breaking often, but I suspect that may not be necessary.

This sounds good.

Yeah, restricting to bytes (and maybe other primitive types) sounds like a good stopgap. Not urgent, since one can work around using a custom vector type as you mentioned, but still nice to have.

If you'd like to add such a feature I'd be happy to help out! Writeable is certainly a bit annoying implementation-wise so duplicating it is probably similarly annoying, but maybe not too bad.

Great! I'll give it a try for my next contribution then 😄

nciric

Should we be checking in:
.editorconfig, .gitignore?
Anything under Generated (I assume those files are a product of running the diplomat-tool c# command.

Manishearth · 2022-01-24T22:14:00Z

We should check in any .gitignores in the generated folder to get rid of build artifacts, and should ignore editor configs.

Most non-build artifact files under example/ and feature-tests/ should be committed. It should be possible to run the tests without regen

CBenoit · 2022-01-25T00:00:30Z

I rebased on main to fix conflicts and removed the .editorconfig file from the dotnet project

Manishearth · 2022-01-27T23:13:26Z

Thanks for your patience (been rather busy this week), I've started reviewing: For future changes can you push them as individual commits without squashing/rebasing? We use squash merges and GitHub isn't good at remembering review state between rebases.

If you need to rebase over main it's fine to do that, but please do that as a separate step.

CBenoit · 2022-01-28T00:58:39Z

Thanks for your patience (been rather busy this week), I've started reviewing: For future changes can you push them as individual commits without squashing/rebasing? We use squash merges and GitHub isn't good at remembering review state between rebases.

If you need to rebase over main it's fine to do that, but please do that as a separate step.

Oh, sorry if i caused any inconvenience. I’ll refrain from squashing my commits next time 👍
Also, since PR got big enough already, I think I’ll only add commits addressing change requests and open new PRs for anything new. Smaller changes are easier to review after all. Thank you for your time!

Manishearth · 2022-01-28T08:11:09Z

Oh, sorry if i caused any inconvenience. I’ll refrain from squashing my commits next time +1

Oh, no worries, I hadn't started yet and planned to let you know when I did

feature_tests/dotnet/Tests/Ownership.cs

Manishearth

Looking pretty good so far! I haven't done a thorough review but I don't think I'll be able to do one and I'm happy to land this without that level of review since we discussed potentially disabling this if things break.

One common thread here is the handling of opaque/non-opaque types, which has cropped up a couple times. Might be worth discussing further.

(I would like to eventually get all my thoughts on this documented in the diplomat book so that backend writers have it easy, but I don't have time for that right now)

tool/src/dotnet/Runtime.cs

tool/src/dotnet/conversions.rs

tool/src/dotnet/idiomatic.rs

tool/tests/snapshots/dotnet_target__boolean_type@MyStruct.cs.snap

feature_tests/src/ownership.rs

CBenoit · 2022-01-30T19:26:41Z

Looking pretty good so far! I haven't done a thorough review but I don't think I'll be able to do one and I'm happy to land this without that level of review since we discussed potentially disabling this if things break.

Yes, as long as I didn’t break anything for other backends it should be safe! I’m pretty satisfied with a general review for stuff like how opaque/non-opaque types are managed.
(I also have plans for refactoring the dotnet backend itself, especially thinking about future support for tera templates, so I don’t even expect the detailed implementation to remain in it’s current state for long)

One common thread here is the handling of opaque/non-opaque types, which has cropped up a couple times. Might be worth discussing further.

I’ll indeed revisit non-opaque types following your comments 👍

My understanding so far: non-opaque structs should be as dumb as it comes: fully managed by C#, no destructor. Since destructor do not need to be run, I don’t even need to manually allocate the raw struct on heap like I did in current implementation (it was mostly done for simplicity assuming destructor could be run… because I didn’t want to manage Raw.MyStruct along a Raw.MyStruct* depending on whether it was allocated by Rust or copied to C#).

I’ll also remove the "move semantic" from generated C# classes.

Manishearth · 2022-01-31T02:44:00Z

My understanding so far: non-opaque structs should be as dumb as it comes: fully managed by C#, no destructor. Since destructor do not need to be run, I don’t even need to manually allocate the raw struct on heap like I did in current implementation (it was mostly done for simplicity assuming destructor could be run… because I didn’t want to manage Raw.MyStruct along a Raw.MyStruct* depending on whether it was allocated by Rust or copied to C#).

Correct. WASM does need to allocate the raw struct because it needs a place to read it from (a quirk of how WASM memory works), but you do have a stack-able Raw.MyStruct anyway so you don't need to allocate anything at all.

As a followup we probably should tighten some of the restrictions here.

This test was taking references to non-opaque structs. However, Diplomat doesn't allow this pattern and only opaque types can be passed by reference.

CBenoit · 2022-02-02T16:42:11Z

Okay, I think I addressed all current concerns.

Non-opaque structs are now plain old data (POD) with no Drop/Destruction logic whatsoever.
Move-semantic in generated C# bindings is not a thing anymore.
Added a few comments when appropriate.
Fixed tests that were taking references to non-opaque structs.
Opened a few follow-up issues.

Let me know if there any remaining concern!

Manishearth

minor issues.

I think in the long run C# non opaques will not be able to cache the full struct locally since they'll also support references to opaque types and such. But it's fine for now

Manishearth · 2022-02-02T16:56:08Z

tool/src/c/types.rs

                struct MyStruct {
-                    a: Box<MyStruct>,
+                    a: Box<MyOpaqueStruct>,


nit: I think this needs to be & MyOpaqueStruct and we can later enforce that non opaque structs are fully Copy

Good point, I changed that

CBenoit · 2022-02-02T17:07:22Z

minor issues.

I think in the long run C# non opaques will not be able to cache the full struct locally since they'll also support references to opaque types and such. But it's fine for now

Absolutely, I'll open a follow-up issue about that.

Except cpp one because it currently panics

Manishearth · 2022-02-02T17:11:14Z

I think we're good now! Thanks for all this work! We've found a bunch of followups to fix, which I might take a stab at if I get time, but if you plan to pick stuff up please leave a comment so we don't end up duplicating work 😄

CBenoit · 2022-02-02T17:12:00Z

Just letting you know: I also changed tho pointer_types test for dotnet and js backends (cpp currently doesn't support structs so I ignored that)

Manishearth · 2022-02-02T17:15:54Z

CPP does support structs, fwiw. But that's fine, I think it might not support refs-in-structs yet

CBenoit · 2022-02-02T17:17:34Z

Sorry I indeed meant refs-in-structs (forgot to re-read myself).

I think we're good now! Thanks for all this work! We've found a bunch of followups to fix, which I might take a stab at if I get time, but if you plan to pick stuff up please leave a comment so we don't end up duplicating work smile

Great! Definitely going to let you know! 😄

CBenoit marked this pull request as draft January 20, 2022 23:42

CBenoit force-pushed the csharp-backend branch 6 times, most recently from 8c19b1f to ff5afc1 Compare January 22, 2022 00:35

CBenoit marked this pull request as ready for review January 22, 2022 00:39

CBenoit force-pushed the csharp-backend branch 6 times, most recently from 3be0548 to 0bcdbd8 Compare January 24, 2022 18:16

nciric reviewed Jan 24, 2022

View reviewed changes

CBenoit force-pushed the csharp-backend branch from 3f00dfb to a595bb6 Compare January 24, 2022 23:58

CBenoit force-pushed the csharp-backend branch from f7a5f59 to 0aa6bc5 Compare January 26, 2022 17:41

CBenoit added 3 commits January 26, 2022 12:44

tool: dotnet backend MVP

825dba4

tool: generate insta snapshots for dotnet

3af59e9

feature_tests: add dotnet bindings

b8e38de

CBenoit force-pushed the csharp-backend branch from 0aa6bc5 to 0923e6a Compare January 26, 2022 17:44

CBenoit force-pushed the csharp-backend branch from 5ceacb6 to 6838b8d Compare January 27, 2022 22:30

CBenoit commented Jan 29, 2022

View reviewed changes

feature_tests/dotnet/Tests/Ownership.cs Outdated Show resolved Hide resolved

Fix typo

f95bfb4

Manishearth requested changes Jan 30, 2022

View reviewed changes

Revert support for moved objects

5ead06e

CBenoit mentioned this pull request Feb 2, 2022

Get rid of _destroy symbols for non-opaque structs #130

Open

CBenoit added 4 commits February 2, 2022 11:02

Add a few explanatory comments

a7200dd

Simplify non-opaque structs into POD

5502178

Update pointer_types test

6bb65fe

This test was taking references to non-opaque structs. However, Diplomat doesn't allow this pattern and only opaque types can be passed by reference.

Fix clippy lint

99494f7

CBenoit added 3 commits February 2, 2022 11:48

Add C# test demonstrating mutation of POD struct

df86999

Clean generated C# doc

138c658

Regenerate feature_tests

f1cf050

Manishearth reviewed Feb 2, 2022

View reviewed changes

Fix C pointer_types test

2d2bac0

Manishearth approved these changes Feb 2, 2022

View reviewed changes

Apply previous correction to other backends

82c2414

Except cpp one because it currently panics

Manishearth merged commit b971915 into rust-diplomat:main Feb 2, 2022

CBenoit deleted the csharp-backend branch September 29, 2022 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C# / dotnet target for `diplomat-tool` #124

C# / dotnet target for `diplomat-tool` #124

CBenoit commented Jan 20, 2022 •

edited

Manishearth commented Jan 20, 2022 •

edited

Manishearth commented Jan 20, 2022 •

edited

nciric commented Jan 20, 2022 via email

CBenoit commented Jan 21, 2022 •

edited

Manishearth commented Jan 21, 2022 •

edited

Manishearth commented Jan 21, 2022

CBenoit commented Jan 22, 2022

nciric left a comment

Manishearth commented Jan 24, 2022

CBenoit commented Jan 25, 2022 •

edited

Manishearth commented Jan 27, 2022

CBenoit commented Jan 28, 2022

Manishearth commented Jan 28, 2022

Manishearth left a comment

CBenoit commented Jan 30, 2022 •

edited

Manishearth commented Jan 31, 2022

CBenoit commented Feb 2, 2022 •

edited

Manishearth left a comment •

edited

Manishearth Feb 2, 2022

CBenoit Feb 2, 2022

CBenoit commented Feb 2, 2022

Manishearth commented Feb 2, 2022

CBenoit commented Feb 2, 2022

Manishearth commented Feb 2, 2022

CBenoit commented Feb 2, 2022 •

edited

C# / dotnet target for diplomat-tool #124

C# / dotnet target for diplomat-tool #124

Conversation

CBenoit commented Jan 20, 2022 • edited

Todo before I consider the MVP complete

Future improvements

Manishearth commented Jan 20, 2022 • edited

Manishearth commented Jan 20, 2022 • edited

nciric commented Jan 20, 2022 via email

CBenoit commented Jan 21, 2022 • edited

Manishearth commented Jan 21, 2022 • edited

Manishearth commented Jan 21, 2022

CBenoit commented Jan 22, 2022

nciric left a comment

Choose a reason for hiding this comment

Manishearth commented Jan 24, 2022

CBenoit commented Jan 25, 2022 • edited

Manishearth commented Jan 27, 2022

CBenoit commented Jan 28, 2022

Manishearth commented Jan 28, 2022

Manishearth left a comment

Choose a reason for hiding this comment

CBenoit commented Jan 30, 2022 • edited

Manishearth commented Jan 31, 2022

CBenoit commented Feb 2, 2022 • edited

Manishearth left a comment • edited

Choose a reason for hiding this comment

Manishearth Feb 2, 2022

Choose a reason for hiding this comment

CBenoit Feb 2, 2022

Choose a reason for hiding this comment

CBenoit commented Feb 2, 2022

Manishearth commented Feb 2, 2022

CBenoit commented Feb 2, 2022

Manishearth commented Feb 2, 2022

CBenoit commented Feb 2, 2022 • edited

C# / dotnet target for `diplomat-tool` #124

C# / dotnet target for `diplomat-tool` #124

CBenoit commented Jan 20, 2022 •

edited

Manishearth commented Jan 20, 2022 •

edited

Manishearth commented Jan 20, 2022 •

edited

CBenoit commented Jan 21, 2022 •

edited

Manishearth commented Jan 21, 2022 •

edited

CBenoit commented Jan 25, 2022 •

edited

CBenoit commented Jan 30, 2022 •

edited

CBenoit commented Feb 2, 2022 •

edited

Manishearth left a comment •

edited

CBenoit commented Feb 2, 2022 •

edited