Allow user-defined NIFs to be `unsafe`.

[jorendorff]

In Rust, a safe function must never crash, even if the caller passes invalid arguments (unless the caller has already triggered undefined behavior -- safe functions are not required to cope with that).

This implies that almost all NIFs must be marked unsafe: they are well-behaved not because they can handle all possible arguments but because the caller (the Erlang VM) is careful and only passes correct values. Any other callers must exercise equal care or risk crashing. Of course other callers are very rare, but possible—one NIF might delegate some work to another.

With this commit, erlang_nif-sys users can mark their NIFs unsafe.

Happily, the changes are backwards-compatible, because any time Rust expects an unsafe function, it's OK to pass a safe function instead.

[jorendorff]

Most of the change here is to test code that directly tests internal parts of nif_init!. Many of those parts are now unsafe to touch, which I think is a good thing!

In particular, the closure returned by get_entry!() is unsafe to call because of the mut static. It could cause a data race, which is undefined behavior. I think it might be possible to eliminate that, though.

[goertzenator]

I don't entirely agree with this usage and interpretation of unsafe. It is true that any function taking a raw pointer has an "unchecked contract" and can potentially crash, but a quick check of other material (links below) show that unsafe is not being used in those cases. I think it is just understood that when you supply a raw pointer you need to take care to provide a valid one, and you don't need unsafe to remind you of that. If I haven't swayed you, I encourage you start a discussion about this on reddit and let others weight in. This is an interesting subject.

To my eyes, putting unsafe on all callbacks tells me that there are unchecked contracts beyond raw pointers that I need to see to when writing my nif. There are none of course**, and I think all this unsafe creates clutter and will just confuse the average nif writer. If there are any unsafe usages that will make things better for nif writers, please point them out.

** You correctly point out that nif_init() has a "thread unsafe" contract, but marking the function unsafe doesn't help reduce the chance of errors here. On the contrary, the fine-grained unsafe()s inside that function have been removed making it harder to see the actual dangerous spots. Maybe this should be a nested function: an unsafe wrapper calling a safe function with fine-grained internal unsafes. hmm... In any case, this is an internal function that the user doesn't need to worry about.

• https://doc.rust-lang.org/book/ffi.html
• https://github.com/dcuddeback/libusb-sys/blob/master/src/lib.rs#L195-L197

[jorendorff]

I think it is just understood that when you supply a raw pointer you need to take care to provide a valid one, and you don't need unsafe to remind you of that.

The authors of the Rust standard library, at least, disagree. Every standard library function that dereferences a pointer argument or converts it to a reference or some other safe type is unsafe.

I can take this to the forums, though.

I think all this unsafe creates clutter and will just confuse the average nif writer.

The change is backward-compatible. NIF writers can just keep doing whatever they were doing before.

All I really want is the ability to mark unsafe functions as unsafe without erlang_nif-sys making an issue of it. Let me write a less invasive patch for that and see how it looks.

[jorendorff]

OK, posted. No real idea what is going to happen here.

https://users.rust-lang.org/t/should-ffi-callbacks-into-rust-be-marked-unsafe/8985

[goertzenator]

Could you use a variation of this to wrap unsafe NIFs in a safe wrapper? This could be buried in Rustler's nif_init and not exposed to the Rustler user.

Thanks for posting, will keep an eye on it.

[jorendorff]

The new patch is a lot smaller and does not affect any of the example code in the documentation.

It almost doesn't affect the documentation at all, unless people look at struct ErlNifFunc. But they don't have any real reason to, since nif_init!() handles all that for them...

Of course, if you still don't want to take it, that is 100% fine. :)

[goertzenator]

Don't worry, we'll get something merged in to make your life easier. I'm still not convinced that making all the callbacks unsafe is the way to go, so I have a branch allowing unsafe callbacks with changes confined to the nif_init! macro. I'd like to wait a few days and see what shows up on that forum post.. my mind may yet be changed. ;)

[goertzenator]

@retep998, we are having a discussion about whether C callbacks should be marked as unsafe or not. I note that your project winapi-rs marks its' callbacks as unsafe. I invite you to put your 2 cents into the discussion at https://users.rust-lang.org/t/should-ffi-callbacks-into-rust-be-marked-unsafe/8985. We would love to hear your reasons why you went with unsafe over safe.

[goertzenator]

Thank you for your input @retep998.

@jorendorff, consider my mind changed. This revised PR looks just right. Thank you for patiently jumping through the hoops.

[jorendorff]

Thanks!

[jorendorff]

I missed a few that are listed in gen_api.erl, not in lib.rs. I'll file a new PR later.

[goertzenator]

I've finished all my macro and test enhancements for now, so once your next PR is in I'll release a new rev on crates.io.