Several FFI Sections: Strings, Errors, and API Design

rust-unofficial / patterns

A catalogue of Rust design patterns, anti-patterns and idioms

https://rust-unofficial.github.io/patterns/

Mozilla Public License 2.0

7.98k stars 363 forks source link

Several FFI Sections: Strings, Errors, and API Design #106

Closed jhwgh1968 closed 3 years ago

jhwgh1968 commented 3 years ago

This is still in draft because it's early, but I wanted to open this PR to claim it and show where I'm going. Let me know if this is the right approach, or if I missed conversation occurring elsewhere and jumped the gun.

Looking through the PRs and the Issues, it looked like no one was tackling FFI. Well, I volunteer!

It's a big subject, of course, so I'm only offering one small part: exporting Rust type APIs to other languages, using object-based API design.

The example may seem rather obscure, but I think it's a good illustration. It is also one I am very familiar with. I learned Rust by trying to port the QDBM library from C (before finding something off-the-shelf). I got about half way done, and learned a lot about C bindings and memory safety! :smile: I figure this is the best way to capture a lot of that knowledge, and share it with the world.

jhwgh1968 commented 3 years ago

That turned into more typing than I was expecting! But I think it's all pretty good -- or at least, worth reviewing. If it's better to break this up a bit, I can do that as well.

simonsan commented 3 years ago

@jhwgh1968 Hey there, nice work! As a first step could you rebase your branch on master please and link the corresponding new files in SUMMARY.md? We'll review this PR the upcoming days.

MarcoIeni commented 3 years ago

CI is failing, can you fix that? Thanks!

simonsan commented 3 years ago

@jhwgh1968 Not sure how well FFI and doc tests work, if there is an example that you can't get through doc test please mark it with

```rust,ignore

jhwgh1968 commented 3 years ago

Thanks for the tip, @simonsan. Although I did end up putting ignore on most blocks, the CI did catch a couple typos and silly things first!

Fingers crossed, CI should pass on the version I just pushed. Once it does, then, I'll get to the other minor comments.

simonsan commented 3 years ago

@jhwgh1968 Please don't force-push so we can actually keep commit history, we'll squash it at the end ;-)

jhwgh1968 commented 3 years ago

Overall you definitely improved things for me. Thank you!

I have revised the bug you mentioned, and added the explanation you requested. I also fixed a couple of minor things, related to word choice and grammar, mostly in my original. In case it's helpful, I finally found a way to do footnotes that works, and looks the way I intended.

In my mind, at least, it is ready to merge!

simonsan commented 3 years ago

Nice! Will get to it after merging the markdownlinter, so we can let it run over this one as well ;-)

simonsan commented 3 years ago

Let's ship it! Great job! <3

simonsan commented 3 years ago

@jhwgh1968 What's your reddit username? (if you have one)

simonsan commented 3 years ago

@Michael-F-Bryan Much appreciated! If you both @jhwgh1968 could condense something into a new pull request that would be great and really much appreciated. Improvement by iteration, so to say. ;-)

Michael-F-Bryan commented 3 years ago

@jhwgh1968 sorry for being late to the party and posting a review after the PR was merged. Would you be interested in making a follow-up?

I've added review comments for most of the points, but there are a couple systemic issues I identified:

Unwinding across the FFI boundary is UB, so all calls to non-trivial Rust code in FFI functions must be wrapped in std::panic::catch_unwind()
Almost all of your FFI bindings should unsafe fn's because their memory safety depends on the caller doing the right thing, so not marking them as unsafe is technically unsound
No null pointer checks
You can tell some of the code hasn't been compiled and run, for example the get_error_from_ffi() function will always panic because you call CString::new() with a buffer that contains a null (the terminator)
Functions not documenting their safety invariants and whether they borrow/own the values passed in from C

simonsan commented 3 years ago

@Michael-F-Bryan If it's not so comfortable for you both to discuss in this closed and merged PR it might be really best to open a new one with your improvements on the topic I feel? If I could do that I would open this one again to encourage discussing on it, but sadly Github doesn't allow it.