Implement virtual methods that accept or return pointers (per #191)

[Mercerenies]

This MR adds all missing virtual functions which were previously omitted due to an argument or return value being a pointer. All pointers are translated into Rust-side raw pointers (*const T or *mut T, as appropriate). All virtual trait functions which either take or return a pointer are marked as unsafe.

All native_structures structs in the JSON file have been translated into Rust-side structures (with #[repr(C)] to ensure binary compatibility) and placed in the godot::native directory. These are all pure data structs (all fields are public and they have no methods). Many of the pointer functions take or return pointers to these native structures, so being able to construct and access them Rust-side is essential for this functionality.

There is one double pointer in the JSON API. const uint8_t** appears in several of the networking functions. I believe the correct translation of this type (preserving const) is *mut *const u8, which is what the code in this MR uses.

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-272

[Bromeon]

Thanks a lot for this pull request! 😀

Added some comments in the code.

I'm not very sure if generally implementing ToVariant/FromVariant for pointer types is desired. Is there any use case outside of native structures? If not, we may have to bite the bullet and split the SignatureTuple trait, as discussed on Discord 🤔 more thoughts on this?

If you rebase this code on latest master, the godot-itest failure should go away and the license-guard one should become more expressive (every file needs a license header).

[Mercerenies]

I'm not very sure if generally implementing ToVariant/FromVariant for pointer types is desired. Is there any use case outside of native structures? If not, we may have to bite the bullet and split the SignatureTuple trait, as discussed on Discord thinking more thoughts on this?

Regarding this, I don't think there's a use case in "ordinary" Godot code (read: Anything that doesn't end in the word "Extension"). But I personally think it's a lot nicer tunneling through a Variant than splitting (I wasn't a fan of the splitting idea, which is why I was hoping you or lili had an alternative suggestion). And there's precedent: we already have nontrivial conversions in ToVariant and FromVariant for things like smaller fixed-size integer types and the infamous Option<Gd<T>>. So I think it's reasonable to treat FromVariant / ToVariant as a general "serialization-to-Godot" framework, and under that interpretation casting a pointer to an integer seems like a perfectly reasonable serialization technique in my mind.

[Bromeon]

The question is: should this be part of the public API?

So I think it's reasonable to treat FromVariant / ToVariant as a general "serialization-to-Godot" framework, and under that interpretation casting a pointer to an integer seems like a perfectly reasonable serialization technique in my mind.

I'm actually more worried about the other side: "deserialization-from-Godot", aka. FromVariant, aka. construct pointers out of thin air. So far, the Variant conversions are something very high-level, relatively easy to reason about, with no real error potential (Result catches those).

If we allow Variant(Int) to be converted to raw pointers, this can very easily lead to invalid and unchecked results.

let variant = 32.to_variant();
let ptr = variant.to::<*mut String>();

While it is technically safe, we essentially committed a mem::transmute() through variant conversions. You do need extra unsafe to dereference the pointers, but still -- this seems very easy to use wrong and brings FFI complexity into the high-level Variant API.

---

And there's precedent: we already have nontrivial conversions in ToVariant and FromVariant for things like smaller fixed-size integer types and the infamous Option<Gd<T>>.

But they always return a valid result, or the conversion fails as Result::Err. AFAIK it's not possible to construct invalid values (otherwise it might be a bug).

---

Regarding this, I don't think there's a use case in "ordinary" Godot code (read: Anything that doesn't end in the word "Extension").

Maybe that's an indicator that it deserves a more specialized API than the high-level Variant one (or go through the split traits directly). We also need to consider that adding raw pointer conversions increases the complexity of the Variant API, while 99% of users won't ever benefit from those corner cases.

---

Either way, if you modify it, please preserve the current commit (Variant conversions to/from pointers) and add more on top; this might be something interesting to potentially revisit at some point. Even if it's just for the design rationale.

[Bromeon]

Your last merge seems to have gone wrong; you are now reverting changes from the master branch.

I just mentioned a few files, but many more are affected.

[Bromeon]

Your seem to accidentally revert several unrelated changes from master. Please undo those changes.

[Bromeon]

Also in this file.

[Bromeon]

And here.

[Mercerenies]

@Bromeon Oof, sorry about the botched rebase. I think it should be repaired now. Let me know if anything still looks suspicious.

I see your argument regarding FromVariant producing raw pointers. I'm going to think about it a bit and take another look at it tomorrow. If I don't have any better ideas, I'll implement the split traits. If I do, I'll mention it.

[Mercerenies]

Added commit to split the signature tuple trait. It ended up being far simpler than I thought (the macro calls just automagically know which trait they need; there wasn't a lot of manual correction to be done).

[lilizoey]

I'm actually more worried about the other side: "deserialization-from-Godot", aka. FromVariant, aka. construct pointers out of thin air. So far, the Variant conversions are something very high-level, relatively easy to reason about, with no real error potential (Result catches those).

If we allow Variant(Int) to be converted to raw pointers, this can very easily lead to invalid and unchecked results.

let variant = 32.to_variant();
let ptr = variant.to::<*mut String>();

While it is technically safe, we essentially committed a mem::transmute() through variant conversions. You do need extra unsafe to dereference the pointers, but still -- this seems very easy to use wrong and brings FFI complexity into the high-level Variant API.

This is equivalent to doing

let a: usize = 32;
let ptr = a as *mut String;

Which is entirely legal in safe rust. i dont think this is an argument against allowing this conversion. Unless you're suggesting we change to using strict provenance, then this is entirely fine within rust. But using strict provenance would be a big change.

---

But they always return a valid result, or the conversion fails as Result::Err. AFAIK it's not possible to construct invalid values (otherwise it might be a bug).

You can't construct invalid values in the sense of unsafety or unsoundness. But we can return semantically invalid values. Like returning a Rect2 with negative size. A pointer we can't dereference is just a pointer we can't use. But you already know to guard against these issues when you're dealing with pointers in rust.

as is not very complicated in rust in general, and saying that FromVariant/ToVariant becomes overly complicated because we allow it to do conversion that as does seems very strange to me.

---

An alternative solution here would be to leverage the Via in GodotFuncmarshal. And only require in Signature that the Via of GodotFuncMarshal implements FromVariant/ToVariant. Then we implement GodotFuncMarshal for pointers using i64 as the Via. This would then not expose FromVariant/ToVariant conversions of pointers directly, but would still allow us to pass around pointers through functions.

This would require a fair bit of rewriting though, especially as it seems like we're misusing the Via a bit, using Infallible for the Via to indicate a conversion is infallible. In reality GodotFuncMarshal should probably look like:

pub trait GodotFuncMarshal: Sized {
    /// Intermediate type through which Self is converted.
    type Via;
    /// The type returned when conversion fails
    type Error: Debug;
    ...
}

[Bromeon]

This is equivalent to doing

let a: usize = 32;
let ptr = a as *mut String;

Which is entirely legal in safe rust

I'm aware of that, which is precisely why I mentioned "technically safe". You can write 1000i64 as i8, it's safe as well. Is it good practice though?

as casts are the crowbar in the Rust type system. Their power is equivalent to static_cast, const_cast and reinterpret_cast of C++ combined. Which says a lot, given that C++'s motivation of introducing those keywords was to split the power of C-style casts in the first place. When it comes to pointer casts, I found that as is most often better used inside abstractions that have very defined conversions (such as GodotFfi or sys::force_mut_ptr).

Memory safety is irrelevant here, what I'm talking about is the potential to introduce bugs. And I'm not a fan of APIs that make this easy, especially if they mostly exist to serve internal implementation.

---

An alternative solution here would be to leverage the Via in GodotFuncmarshal.

It looks like the latest revision of this PR works without ToVariant/FromVariant and is relatively simple, so that looks good to me.

[Bromeon]

bors try

[bors]

try

Build failed:

• full-ci

[Mercerenies]

@Bromeon Are those bors failures merge conflicts? They don't look like parts of the codebase I've touched. What is bors doing that ./check.sh isn't?

[Bromeon]

No, those point to an upstream change with Godot (the gdextension_interface.h changed, and we need to update first).

I'm already working on this, no action needed from your side 🙂

[Bromeon]

bors try

[bors]

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• full-ci

[Bromeon]

bors r+

[Bromeon]

bors p=10

[bors]

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• full-ci

[Bromeon]

Thanks a lot for this addition and the patience! 🚀