Some cleanup and more accurate tracking of safety invariants #603
Conversation
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-603 |
There was a problem hiding this comment.
Good catch on Inherits! Also NoBase is a nice way to make things more explicit.
We may want to revisit manually implementing Inherits when it comes to the builder API. Conceptually, it should be possible to inherit from any base in a safe way -- the unsafety can only occur if there are inconsistencies between GodotClass::Base and Inherits.
Probably, a good approach would be to only let the user implement GodotClass by hand, and then have a declarative macro that writes the impl Inherits, just like it is already the case with Bounds. Then using that macro would be safe, but manually implementing Inherits would not -- there'd never be a reason to do the latter though.
Could you add an empty line before each of the // SAFETY: comments (except when it's the start of a block of course)?
| // SAFETY: `Object` is a `#[repr(C)]` struct with its first field being the object pointer. | ||
| let obj_ptr = unsafe { *(obj as *const Object as *const GDExtensionObjectPtr) }; |
There was a problem hiding this comment.
I assume the reliance on field order here is due to the fact that the fields themselves are private and thus can't be directly accessed?
There was a problem hiding this comment.
yeah, i was gonna just add a #[doc(hidden)] method to Object to do this instead tbh since then i dont need to do this.
Add the ability to add unsafe + safety docs to arbitrary methods through codegen More accurately track the safety invariant of `Array`: - Make all methods of `InnerArray` that return `VariantArray` unsafe - Make `Array::as_inner()` unsafe - Add `Array::as_inner_ref()` to allow safe usage of immutable methods - Reword some safety docs
lilizoey
force-pushed
the
refactor/minor-cleanups
branch
2 times, most recently
from
ac01406
to
14c46e6
Compare
it's mostly in draft just cause i wanted to take a final look at it but didn't have much energy to do so for a bit, i'll look over it now tho and just merge if im happy |
lilizoey
force-pushed
the
refactor/minor-cleanups
branch
from
14c46e6
to
24a6002
Compare
lilizoey
force-pushed
the
refactor/minor-cleanups
branch
from
24a6002
to
de439bc
Compare
|
Sounds good, go ahead merging as soon as you're ready! 👍 |
Probably gonna split this PR up later since the
Array-related changes ended up being a lot bigger than i expected. So leaving it as a draft until i do that at least.Removes some unused traits
From what i can tell,
EngineClassandExportableObjectare now actually unused (beyond a test using a method fromEngineClass). So i removed them.Makes
InheritsunsafeWe rely on
Inheritsbeing implemented correctly forupcastto be safe.Replace
()withNoBaseIt seemed weird to use
()in this situation, so i made it a dedicated unconstructable type instead.More accurately track
Array's safety invariantArray's safety invariant is that it only containts values matching its type, but that could very easily be circumvented internally in our code. You could simply call
self.as_inner().duplicate(). This PR treatsself.as_inner()as equivalent to doingself.assume_type::<Variant>(). This revealed the fact that we actually violated the safety invariant withresize, as it insertsVariant::nil()when increasing the size.To support this properly i made it possible to mark arbitrary methods in the codegen as unsafe with safety docs, and currently just added all array methods that return
VariantArrayto that, since they can just bypass safety checks by coercing the array to a `VariantArray.I removed
resizesince it can be used to circumvent the safety invariant, instead there's nowshrinkandresize_with.