Fix various issues with pointer calls by lilizoey · Pull Request #204 · godot-rust/gdext (original) (raw)

The trait itself is not unsafe to implement -- some of its methods are unsafe to call.

I know it's a border case because implementing GodotFfi essentially means enabling it for potentially unsafe implementations, but this is rather a problem with the macro. If you expand the macro, you have actual unsafe on each method that is unsafe to call. So maybe the macro should be prefixed with unsafe_.

If the safety invariants i've listed for GodotFfi are violated, that is a buggy implementation. I don't think we should require the caller of from_arg_ptr and move_return_ptr to ensure the implementation is correct. And there is no way that we can have rust check that the implementation is correct for us.

Not having the unsafe on the trait would mean that if someone re-implemented GodotString without properly incrementing the refcount in from_arg_ptr, that would be a correct implementation. However it would break every single one of our macros to generate pointer calls.

So we'd then have a weird situation where every single use of #[unsafe_godot_api] is also implicitly a statement of "i have checked that everyone who implements GodotFfi for a type correctly initialized and cleans up values in from_arg_ptr and move_return_ptr".

So in my opinion, we should allow people who call from_arg_ptr and move_return_ptr to assume that they will properly handle initialization/cleanup of the values they're called with/they return.

That is what unsafe on traits is for, establishing safety invariants that users of the trait are allowed to rely on for safety. As mentioned in your quote:

This is commonly because the trait has invariants that other unsafe code relies on being upheld, and that these invariants cannot be expressed any other way.

unsafe on a function call can only express invariants related to the call of the function. You could not express the invariant of "from_arg_ptr was implemented to take a pointer to an argument in a pointer-call, and returns a properly initialized value of this type". Since that is not within the scope of what the caller should check, a violation of that is just a bug.

It's not my fault for instance if i call a function add(a: i32, b: i32) -> i32 function with add(1,2) and it returns 25 when the documentation said it would add the two values. That is a buggy implementation.

There's also just the fact that we have unsafe code relying on this invariant. Like our SignatureTuple impls.