Experimental refactoring of GodotFfi

[lilizoey]

This is not finalized because this is a refactoring that i just randomly tried today. Before i work more on it and finish it up (beyond making it compile if it doesn't compile on all targets yet) i'd want to see if this is something we want to actually do.

So basically, i had an issue while working on #621 where the names of GodotFfi methods (as well as the types they use) don't entirely reflect what the methods do or are intended to do (and some other minor correctness things). So a summary here:

from_sys

This creates a new value of type Self from an ffi pointer, however this is documented to not increment any refcounts or anything. Which is weird. If it creates a Self it really should be a fully valid value of type Self right?

So instead i split this in two:

• new_from_sys(ptr: ..) -> Self, which does in fact create a new value from a sys pointer. Including incrementing refcounts and such.
• borrow_sys<'a>(ptr: ..) -> &'a Self which creates a reference out of the pointer.

Might consider adding a third one, which has the old behavior. I.e it constructs a Self without incrementing refcounts or anything, explicitly for the case where we want that behavior. It could be called new_from_sys_weak or something?

Unfortunately, borrow_sys cannot be implemented for RawGd, so i had to split it into a new trait GodotFfiBorrowable. However it's still fairly useful in that we can now explicitly borrow a reference to types like StringName. It is also used to implement new_from_sys in some cases.

This has cleaned up some ugly code, like we dont need to remember to std::mem::forget(string_name.clone()) whenever we create one. And we can use StringName::borrow_sys(ptr) instead of ManuallyDrop::new(StringName::from_sys(ptr)) where we used that pattern.

from_sys_init/from_sys_init_default

From the name, one would assume it takes a sys pointer and returns a Self. But they dont. They create a brand new value from an initialization function. So i renamed them to new_with_uninit and new_with_init.

Now since new_with_init(from_sys_init_default) is gonna pass along an initialized pointer, i changed it from taking a closure which takes a pointer, to taking a closure FnOnce(&mut Self). Which also makes this a safe function.

new_with_init doesn't have a default implementation anymore, instead the ffi_methods macro will try to implement it with Default::default(). Which will fail in some cases so needs to be manually implemented in those cases.

Using Const pointers

Many places just use the non-const version of pointers even if the const one is more accurate. i changed this around a bit so now:

• sys() returns a const pointer
• removed sys_const()
• sys_mut() doesn't have a default impl (we cannot implement this correctly through calling sys())
• new_from_sys() takes a const pointer

This does mean we need to convert const-pointers into their non-const form sometimes where we didnt before. But overall it actually worked fairly well in most places. And it does help with some type safety in various places, like making sure we use sys_mut() when it's actually possible to and more correct to do so.

New traits

SysPtr

AsUninit is replaced by SysPtr which contains conversions for both uninit and const pointers.

[lilizoey]

@Bromeon i think some of these refactoring ideas are fairly useful. But i didn't like discuss these ideas or anything before trying them. So i'm just wondering if you think this seems useful enough to proceed with? any comments etc.

I dont mind scrapping this or massively overhauling it. It was an experiment that i wasn't sure was gonna pan out, but it worked out so far so just wanna see what you think.

[GodotRust]

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

[Bromeon]

Thanks a lot for this! 💪
I'll write down my thoughts on the individual changes below:

---

This creates a new value of type Self from an ffi pointer, however this is documented to not increment any refcounts or anything. Which is weird. If it creates a Self it really should be a fully valid value of type Self right?

So instead i split this in two:

• new_from_sys(ptr: ..) -> Self, which does in fact create a new value from a sys pointer. Including incrementing refcounts and such.
• borrow_sys<'a>(ptr: ..) -> &'a Self which creates a reference out of the pointer.

That sounds like a good change, if it helps simplify code.

---

Might consider adding a third one, which has the old behavior. I.e it constructs a Self without incrementing refcounts or anything, explicitly for the case where we want that behavior. It could be called new_from_sys_weak or something?

But the current code seems to work without it, so why would we add a 3rd one?

---

Unfortunately, borrow_sys cannot be implemented for RawGd, so i had to split it into a new trait GodotFfiBorrowable. However it's still fairly useful in that we can now explicitly borrow a reference to types like StringName. It is also used to implement new_from_sys in some cases.

GodotFfiBorrowable

For types that can be represented directly by the sys-pointer, thus we can simply convert a pointer into a reference.

I don't really like the proliferation of these low-level traits, it's already near-impossible for a new contributor to navigate along GodotFfi, GodotNullableFfi, GodotConvert, GodotType, AsUninit, ...

Can we not add this function without a trait? A lot of the FFI methods are generated through macros anyway, so there doesn't need to be compile-time polymorphism.

---

from_sys_init/from_sys_init_default

From the name, one would assume it takes a sys pointer and returns a Self. But they dont. They create a brand new value from an initialization function. So i renamed them to new_with_uninit and new_with_init.

The existing functions are supposed to be read as "from sys-init" and "from sys-init, default" -- not "from sys + init", "from sys + init default" 🙂 but yes, maybe naming can be improved.

I'm not sure about the new_ prefix though -- this will also appear first in IDE suggestions (unfortunately despite #[doc(hidden)]) so it might confuse users. And the "sys" part is somewhat essential, highlighting the low-levelness of the operation as opposed to higher-level "new/from/with" constructors.

Another option would be from_sys_init_fn + from_sys_uninit_fn? This would be also close to Gd::from_init_fn, but on a lower level. The added _fn suffix might help clear up the confusion that it doesn't take a sys pointer.

---

Using Const pointers

Many places just use the non-const version of pointers even if the const one is more accurate. i changed this around a bit so now:

• sys() returns a const pointer
• removed sys_const()
• sys_mut() doesn't have a default impl (we cannot implement this correctly through calling sys())
• new_from_sys() takes a const pointer

This is a great change, long overdue. I wanted to look at this when I added sys_const but it required too many changes at the time. Still need to look at the details, but sounds good from a high-level perspective 🙂

---

New traits

HasOpaque

For types that are implemented as a wrapper around an opaque field. Makes the ffi_methods macro no longer tied to the opaque field being named opaque and lets us reference the type more explicitly (fewer as _ coercions).

It does make the macro a bit more ugly though.

Again I'm kind of skeptical to move "conventions" (as in naming) to "type system constraints" (traits), especially if there's both pros/cons to the change. Having a field named opaque didn't strike me as a big problem -- and it's not like we accidentally introduce bugs if this field would be removed/renamed, as the compiler would immediately complain.

And this change in particular adds a significant amount of boilerplate code for each type, which wasn't previously necessary:

impl<T: GodotType> sys::HasOpaque for Array<T> {
    type Opaque = sys::types::OpaqueArray;

    unsafe fn borrow_opaque<'a>(ptr: *const Self::Opaque) -> &'a Self {
        &*(ptr as *mut Self)
    }

    fn from_opaque(opaque: Self::Opaque) -> Self {
        Self {
            opaque,
            _phantom: PhantomData,
        }
    }

    fn opaque(&self) -> &Self::Opaque {
        &self.opaque
    }

    fn opaque_mut(&mut self) -> &mut Self::Opaque {
        &mut self.opaque
    }
}

---

AsConst

Like AsUninit but for converting between sys-pointers const and mut variants.

We had existing sys::to_const_ptr and sys::force_mut_ptr functions, I guess they are then used less often.

I also wonder if AsUninit + AsConst could be combined to a single trait, as GDExtension typically provides the 3 of a kind (const, mut, uninit)?

[lilizoey]

Thanks a lot for this! 💪 I'll write down my thoughts on the individual changes below:

This creates a new value of type Self from an ffi pointer, however this is documented to not increment any refcounts or anything. Which is weird. If it creates a Self it really should be a fully valid value of type Self right?
So instead i split this in two:

• new_from_sys(ptr: ..) -> Self, which does in fact create a new value from a sys pointer. Including incrementing refcounts and such.
• borrow_sys<'a>(ptr: ..) -> &'a Self which creates a reference out of the pointer.

That sounds like a good change, if it helps simplify code.

Might consider adding a third one, which has the old behavior. I.e it constructs a Self without incrementing refcounts or anything, explicitly for the case where we want that behavior. It could be called new_from_sys_weak or something?

But the current code seems to work without it, so why would we add a 3rd one?

The main case i was thinking would be if we want to do something like:

1. pass a value to godot without decrementing refcount
2. get value back from godot without incrementing refcount.

Which may be useful in some cases, in #621 i had a case where i wanted that but im unsure if it's actually needed there. So it's probably fine to not include that here and add it in #621 if it really truly is needed, and even then maybe just adding special cased methods for it if it's only needed for one or two types.

Unfortunately, borrow_sys cannot be implemented for RawGd, so i had to split it into a new trait GodotFfiBorrowable. However it's still fairly useful in that we can now explicitly borrow a reference to types like StringName. It is also used to implement new_from_sys in some cases.

GodotFfiBorrowable

For types that can be represented directly by the sys-pointer, thus we can simply convert a pointer into a reference.

I don't really like the proliferation of these low-level traits, it's already near-impossible for a new contributor to navigate along GodotFfi, GodotNullableFfi, GodotConvert, GodotType, AsUninit, ...

Can we not add this function without a trait? A lot of the FFI methods are generated through macros anyway, so there doesn't need to be compile-time polymorphism.

That's fair, im not sure if it's something that is needed as a separate trair. Maybe it's possible that some trait bound can be added to the method to keep it in GodotFfi, or i can just have it be manually implemented for each type. (well probably with a macro tbh).

from_sys_init/from_sys_init_default

From the name, one would assume it takes a sys pointer and returns a Self. But they dont. They create a brand new value from an initialization function. So i renamed them to new_with_uninit and new_with_init.

The existing functions are supposed to be read as "from sys-init" and "from sys-init, default" -- not "from sys + init", "from sys + init default" 🙂 but yes, maybe naming can be improved.

Maybe by just renaming from_sys it's more clear that this isn't a function that's like from_sys. Unfortunately we use the name from_sys in a lot of places, so to make this stick we should then probably rename all of them to some new pattern like new_from_sys. Which probably wouldn't be too hard but it'd be a lot more stuff to change.

I'm not sure about the new_ prefix though -- this will also appear first in IDE suggestions (unfortunately despite #[doc(hidden)]) so it might confuse users. And the "sys" part is somewhat essential, highlighting the low-levelness of the operation as opposed to higher-level "new/from/with" constructors.

huh, i haven't noticed that actually (the doc hidden limitation).

Another option would be from_sys_init_fn + from_sys_uninit_fn? This would be also close to Gd::from_init_fn, but on a lower level. The added _fn suffix might help clear up the confusion that it doesn't take a sys pointer.

i really find the from_* naming here confusing personally. Since from to me implies that the object is provided by the caller somehow, but here it's provided by the function and the caller merely initializes the provided object/pointer properly.

A minor inconsistency is also that from_*fn doesn't necessarily mean "calls function to construct object" since we have Callable::from_fn.

maybe new_with_sys_init/new_with_sys_uninit?

Unrelated but thinking about it, new_with_init probably still needs to be unsafe. Since while it passes a valid object, it may pass an object whose safety invariant is broken (like for RawGd it currently does that).

Using Const pointers

Many places just use the non-const version of pointers even if the const one is more accurate. i changed this around a bit so now:

• sys() returns a const pointer
• removed sys_const()
• sys_mut() doesn't have a default impl (we cannot implement this correctly through calling sys())
• new_from_sys() takes a const pointer

This is a great change, long overdue. I wanted to look at this when I added sys_const but it required too many changes at the time. Still need to look at the details, but sounds good from a high-level perspective 🙂

The biggest issue is that some ffi functions that really should take const pointers, dont. like variant conversion functions. but other than that it was fairly pain free.

New traits

HasOpaque

For types that are implemented as a wrapper around an opaque field. Makes the ffi_methods macro no longer tied to the opaque field being named opaque and lets us reference the type more explicitly (fewer as _ coercions).
It does make the macro a bit more ugly though.

Again I'm kind of skeptical to move "conventions" (as in naming) to "type system constraints" (traits), especially if there's both pros/cons to the change. Having a field named opaque didn't strike me as a big problem -- and it's not like we accidentally introduce bugs if this field would be removed/renamed, as the compiler would immediately complain.

And this change in particular adds a significant amount of boilerplate code for each type, which wasn't previously necessary:

impl<T: GodotType> sys::HasOpaque for Array<T> {
    type Opaque = sys::types::OpaqueArray;

    unsafe fn borrow_opaque<'a>(ptr: *const Self::Opaque) -> &'a Self {
        &*(ptr as *mut Self)
    }

    fn from_opaque(opaque: Self::Opaque) -> Self {
        Self {
            opaque,
            _phantom: PhantomData,
        }
    }

    fn opaque(&self) -> &Self::Opaque {
        &self.opaque
    }

    fn opaque_mut(&mut self) -> &mut Self::Opaque {
        &mut self.opaque
    }
}

Yeah i think i mostly agree, it didn't turn out as useful as I'd hoped. Unless i can add a blanket impl for GodotFfi for types that implement HasOpaque (should probably be renamed to IsOpaque or something) then i dont think this is worth it. Since with a blanket impl it'd be much simpler to just implement HasOpaque than all the GodotFfi methods. But even then im a bit skeptical, this would likely run afoul with orphan rules very quickly. Negative trait impls + coherence checking would be great here.

AsConst

Like AsUninit but for converting between sys-pointers const and mut variants.

We had existing sys::to_const_ptr and sys::force_mut_ptr functions, I guess they are then used less often.

I also wonder if AsUninit + AsConst could be combined to a single trait, as GDExtension typically provides the 3 of a kind (const, mut, uninit)?

Maybe we can have a trait

pub trait SysPtr {
  type MutPtr;
  type ConstPtr;
  type UninitPtr;

  fn as_mut_ptr(self) -> Self::MutPtr;
  fn as_const_ptr(self) -> Self::ConstPtr;
  fn as_uninit_ptr(self) -> Self::UninitPtr; 
}

and just implement that for all the relevant pointer types? Then converting between them is fairly straightforward. though as designed this trait wouldn't guarantee that ptr.as_const_ptr().as_mut_ptr() == ptr.as_mut_ptr().

So maybe just have a literal combination of AsUninit + AsConst. But then we need to have a different method for const -> mut and uninit -> init even though they convert to the same type. But maybe that's fine, it's more explicit at least.

Maybe this trait should be unsafe just so that we can rely on it being implemented properly? for instance that like T1::Ptr !=T2::Ptr when T1 != T2. I dont think it's necessary since we control both sides of this and can just manually ensure it's correctly implemented when relying on such properties. Ill look into that.

[lilizoey]

So an interesting thing i now found, by turning all the godot-ffi types that use Opaque into #[repr(transparent)] we can actually collapse the entire ffi_methods! macro into just having a single case for everything.

So instead of having three different cases *mut Opaque, Opaque (this case was actually unused already), and *mut Self. All of them can just use the *mut Self logic.

The only negative impact is that we have some superfluous value.clone()s for some of the Copy types in some places. But that is something the compiler should be easily able to optimize out.

And now almost all the GodotFfi implementations except for RawGd's just look like

unsafe impl GodotFfi for StringName {
    fn variant_type() -> sys::VariantType {
        sys::VariantType::StringName
    }

    ffi_methods! { type sys::GDExtensionTypePtr; ..}
}

RawGd is still rather complicated cause it needs a lot more special-cased logic. It doesn't even use ffi_methods! macro and hasn't for a while.

[Bromeon]

The problem is that #[repr(transparent)] makes it impossible for us to ever add fields in the future. Doing so can be handy for caching or debug diagnostics, I'm not sure I'd want to lose that ability.

Opaque (this case was actually unused already)

Hm, interesting 🤔 I guess we stopped using it at some point and forgot to remove it. I thought at least some of the builtins needed it, but apparently not.

[lilizoey]

The problem is that #[repr(transparent)] makes it impossible for us to ever add fields in the future. Doing so can be handy for caching or debug diagnostics, I'm not sure I'd want to lose that ability.

Doing that would already make a lot of things more complicated though. And would basically require us to rewrite most things anyway. Like the reason Gd has a bespoke implementation is almost entirely because it has a single extra field. If we just represented Gd by the object pointer we could remove most of the custom logic, only thing that would remain is the weird version-dependent logic for passing as arguments/returning from functions (which we could still do while using the current logic for ffi_methods since we can just selectively use it for some methods). Besides we dont need it at the moment. And if we really do need a different case there, it's not too hard to add it back in again.

We can also do similar to what we did for ClassName where we just made a new type for that.

Opaque (this case was actually unused already)

Hm, interesting 🤔 I guess we stopped using it at some point and forgot to remove it. I thought at least some of the builtins needed it, but apparently not.

I think it was only used for Gd

[Bromeon]

Besides we dont need it at the moment. And if we really do need a different case there, it's not too hard to add it back in again.

That code exists in a working state now, and while the macro impl is not the most beautiful, it's also not particularly complex. Plus, once #[repr(transparent)] is there, more code may start relying on it (e.g. for direct transmutes), which can make it very hard to ever go back without introducing bugs. Overall, the threshold to experiment with other fields becomes much higher than it is now.

But now that Opaque seems no longer needed, maybe it's possible to simplify the dispatching between *mut Opaque and *mut Self somehow 🤔

[lilizoey]

Besides we dont need it at the moment. And if we really do need a different case there, it's not too hard to add it back in again.

That code exists in a working state now, and while the macro impl is not the most beautiful, it's also not particularly complex. Plus, once #[repr(transparent)] is there, more code may start relying on it (e.g. for direct transmutes), which can make it very hard to ever go back without introducing bugs.

We can already do most of those things with single-field #[repr(C)] struct, which is what they currently are, only way to prevent that would be to make them just normal #[repr(Rust)] structs. But im also not entirely sure when it would be right for anyone to really do that? Like #[repr(transparent)] would only definitely let you transmute between the opaque inner type and the outer type, but i dont think we ever use those opaque types except in the GodotFfi impls.

But now that Opaque seems no longer needed, maybe it's possible to simplify the dispatching between *mut Opaque and *mut Self somehow 🤔

I dont really think there's much that really could be simplified, we could change some minor syntactic stuff but most of it would still need to be there.

[Bromeon]

#[repr(C)] is part of the public API -- we promise our users that they can use [real; 3] in a layout-compatible way to Vector3, for example. For those builtins, we cannot add any fields without breaking this promise.

I wonder why Array, Dictionary, PackedArray, StringName, NodePath Callable, Signal were declared as #[repr(C)] -- that feels wrong. We should only do that when we can actually exploit the memory layout:

• for GString, which can be stored contiguously in PackedStringArray
• for Color, which can be stored contiguously in PackedColorArray
• for the geometric types with all public fields, as adding fields would anyway be breaking change
• for Variant, which can be converted from contiguous argument arrays into &[Variant] slices
  • actually, on first glance I'm not even sure if we use that, as FFI typically has &[&Variant], so needs conversion anyway...

---

Also, it looks like FFI declaration is slightly more complex in very few cases? E.g. for Rid or Plane -- is this because they don't have a Default impl?

-    ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. }
+    ffi_methods! { type sys::GDExtensionTypePtr;
+        fn new_from_sys;
+        fn sys;
+        fn sys_mut;
+        fn new_with_uninit;
+        fn from_arg_ptr;
+        fn move_return_ptr;
+    }
+
+    unsafe fn new_with_init(init_fn: impl FnOnce(&mut Self)) -> Self
+    where
+        Self: Sized,
+    {
+        let mut rid = Rid::Invalid;
+        init_fn(&mut rid);
+
+        rid
+    }

[lilizoey]

#[repr(C)] is part of the public API -- we promise our users that they can use [real; 3] in a layout-compatible way to Vector3, for example. For those builtins, we cannot add any fields without breaking this promise.

Sure, i was referring to the opaque types, for these types we can already use the equivalent logic.

I wonder why Array, Dictionary, PackedArray, StringName, NodePath Callable, Signal were declared as #[repr(C)] -- that feels wrong. We should only do that when we can actually exploit the memory layout:

* for `GString`, which can be stored contiguously in `PackedStringArray`

* for `Color`, which can be stored contiguously in `PackedColorArray`

* for the geometric types with all public fields, as adding fields would anyway be breaking change

* for `Variant`, which can be converted from contiguous argument arrays into `&[Variant]` slices
  
  * actually, on first glance I'm not even sure if we use that, as FFI typically has `&[&Variant]`, so needs conversion anyway...

I think it was mostly to make sure these types had the same memory layout as the equivalent types in godot-cpp? i dont know if we really need that? but it's likely an expectation people would have.

Also, it looks like FFI declaration is slightly more complex in very few cases? E.g. for Rid or Plane -- is this because they don't have a Default impl?

Yes, ffi_methods for new_with_init uses Default, and i dont default implement new_with_init like we used to using new_with_uninit so now every type that doesn't implement Default must reimplement it.

We've had several issues before with from_sys_init_default being implemented wrong because it needed to be overridden, so i figured it'd be safer to just require it to be reimplemented for every type.

[Bromeon]

I think it was mostly to make sure these types had the same memory layout as the equivalent types in godot-cpp? i dont know if we really need that? but it's likely an expectation people would have.

Not sure -- I don't think people use godot-rust and godot-cpp simultaneously, and as long as we don't document such guarantees, it's not something "official". It's also quite involved to know the actual sizes (depending on float/double, 32/64 configuration), and should anyway only matter for extremely low-level things?

That we provide them for vectors and the other builtins I mentioned makes sense.

But apart from those, we should probably remove both #[repr(C)] and #[repr(transparent)], as it can limit us unnecessarily in the future. For Gd, I was glad I could add diagnostics and cached ID as extra fields. It would be nice if such changes were easily possible for other types, without requiring non-local refactorings due to over-reliance on the layout.

[lilizoey]

Im changing it, but also apparently we are assuming that some of these types are transparent already by doing things like *ptr = self, instead of going through GodotFfi. So i'll see if i can't find all those places and fix them up too.

We're also very explicitly relying on it being possible to cast a sys-ptr to a *mut Variant. So that can't really be changed atm without bigger changes.

[lilizoey]

Another issue:

If we add extra state to refcounted types, it then becomes impossible to optimize accesses to those types by treating pointers to them as references thus avoiding refcounting. For instance:

pub unsafe extern "C" fn get_virtual<T: cap::ImplementsGodotVirtual>(
    _class_user_data: *mut std::ffi::c_void,
    name: sys::GDExtensionConstStringNamePtr,
) -> sys::GDExtensionClassCallVirtual {
    let borrowed_string = StringName::borrow_string_sys(name);
    let method_name = borrowed_string.to_string();

    T::__virtual_call(method_name.as_str())
}

Here i avoid incrementing the stringname's refcount by merely borrowing it, by turning the pointer into a &StringName. However if there is extra state then it wouldn't be possible to soundly treat a GDExtensionStringNamePtr as a &StringName. We'd instead need to construct a new StringName and increment the refcount here.

[Bromeon]

You have some good points!

Im changing it, but also apparently we are assuming that some of these types are transparent already by doing things like *ptr = self, instead of going through GodotFfi. So i'll see if i can't find all those places and fix them up too.

That would be appreciated, thanks!

---

We're also very explicitly relying on it being possible to cast a sys-ptr to a *mut Variant. So that can't really be changed atm without bigger changes.

I think that's fine though -- Variant is in my above list where explicit layout makes sense. So having #[repr(transparent)] should be OK here.

---

If we add extra state to refcounted types, it then becomes impossible to optimize accesses to those types by treating pointers to them as references thus avoiding refcounting.

True. If we do such a refactor, we'd have to remove borrow_string_sys() and similar functions, and fall back to slower clones.

Could you add a static_assert_eq_size! check into the borrow_* functions, so this cannot happen accidentally? Maybe this also makes sense for the Variant function mentioned above?

---

On another note, my CallError PR adds a specialized Variant::from_var_sys_init_result() function, which might introduce a small merge conflict. It should be relatively simple to adjust to your new scheme though. Would be nice if I could merge that PR today 🙂

[lilizoey]

I started cleaning up a couple of things in script_istance.rs because i happened to need to touch it a bit, but i stopped cause it turned out to be a fair bit and think i'll leave that for a separate PR instead.

[Bromeon]

Thanks a lot!

[Bromeon]

Maybe minor thing, but would it be possible to order those in a way so that constructors, getters and other functions are grouped respectively?

[Bromeon]

borrow_ref_slice should be enough, as it's no longer about the sys pointers (where we typically use "var" to differentiate variant from type pointers).

[Bromeon]

Would it make sense to add a string_sys_mut() method?

[lilizoey]

it already exists but it takes a &mut self, but here we have a &self.

[Bromeon]

I see. Pity that these methods aren't const-correct in Godot.

Maybe if they add a v2 of GDExtensionPropertyInfo, such things could be adjusted.

[Bromeon]

I'd use the more common return_val.sys_mut() for recognizability here, even if it means bringing GodotFfi into scope.

[Bromeon]

See above

[lilizoey]

Just realized that these actually don't need this note in this or the next one, since we're not transmuting a pointer to a reference here. We're turning a pointer into a *const Variant here but in the previous one we turn it into a *const &Variant.

[Bromeon]

See above.

Btw, if this comment appears multiple times, we should maybe mention it once before all functions.

[Bromeon]

Suggested change

	/// This will increment reference counts if the type is reference counted. If you need to avoid this then a `borrow_sys` method should
	/// usually be used. Which is a method that takes a sys-pointer and returns it as a `&Self` reference. This must be manually implemented for
	/// each relevant type as not all types can be borrowed like this.
	/// This will increment reference counts if the type is reference counted. If you need to avoid this, then a `borrow_sys` associated function should
	/// usually be used. That function takes a sys-pointer and returns it as a `&Self` reference. This must be manually implemented for
	/// each relevant type, as not all types can be borrowed like this.

[Bromeon]

Is there an example where such an invariant is necessary?

So it's generally not possible to make this method safe?

[lilizoey]

I believe RawGd requires it, atm with how its safety invariant is structured. Since for RawGd the rtti info and object pointer must remain unchanged after construction, but if you use this method you'd need to overwrite them with more appropriate values. i can check if it's possible to rework some things to make it safe though.

[Bromeon]

Ah don't worry, if there's a concrete case that needs it, it's fine to keep unsafe. Maybe add a brief comment, I was just not sure if this was leftover from refactoring.

[lilizoey]

Actually thinking about this, does RawGd even need it? like sure we set the object pointer and rtti info to be a null-object. But as long as we're careful to never let this constructed RawGd leak to user-code before we change the object pointer/rtti info to the correct info, then it is impossible to tell that the RawGd had this happen to it.

So i guess yeah it can be a safe function, it just means that user-code could only ever use it to construct a null-object. Which is probably fine? I mean it's #[doc(hidden)] now anyway so it's private.

[Bromeon]

Since it's #[doc(hidden)], we don't provide any guarantees towards the user. It's fine to assume users will never call it.

[Bromeon]

With this, do we still need static_assert_eq_size? Or would other checks also benefit from alignment comparison?

[lilizoey]

I dont think we do, really what we want to check in all the cases we use static_assert_eq_size is that the two types have compatible layouts. Which means they should all have the same size and alignment.

[lilizoey]

I'm not entirely sure what these CI errors are, they seem unrelated to my PR?

[Bromeon]

There was a problem with the nightly repo. I fixed some things, after rerun they passed.

Thanks a lot!

[Bromeon]

MSRV and other CI issues addressed in #651.