Merge #93 #94 #97

93: Add some documentation about memory management and thread safety r=Bromeon a=ttencate



94: Add Contributing.md r=Bromeon a=ttencate



97: Add Typechecking to FromVariant r=Bromeon a=sayaks

Also add some testing for `Variant::get_type()`.

This *should* fix #95, at least if `try_from_variant` breaks now it's an indication of a deeper issue and not just that there is no implementation.

Co-authored-by: Thomas ten Cate <[email protected]>
Co-authored-by: Lili Zoey <[email protected]>

Author: 3 people

Contributing.md

@@ -0,0 +1,64 @@
+# Contributing to `gdextension`
+
+At this stage, we appreciate if users experiment with the library, use it in small projects and report issues and bugs they encounter.
+
+If you plan to make bigger contributions, make sure to discuss them in a [GitHub issue] first. Since the library is evolving quickly, this avoids that multiple people work on the same thing or implement features in a way that doesn't work with other parts. Also don't hesitate to talk to the developers in the `#dev-gdextension` channel on [Discord]!
+
+## Check script
+
+The script `check.sh` in the project root can be used to mimic a CI run locally. It's useful to run this before you commit, push or create a pull request:
+
+```
+$ check.sh
+```
+
+At the time of writing, this will run formatting, clippy, unit tests and integration tests. More checks may be added in the future. Run `./check.sh --help` to see all available options.
+
+If you like, you can set this as a pre-commit hook in your local clone of the repository:
+
+```
+$ ln -sf check.sh .git/hooks/pre-commit
+```
+
+## Unit tests
+
+Because most of `gdextension` interacts with the Godot engine, which is not available from the test executable, unit tests (using `cargo test` and the `#[test]` attribute) are pretty limited in scope.
+
+Because additional flags might be needed, the preferred way to run unit tests is through the `check.sh` script:
+
+```
+$ ./check.sh test
+```
+
+## Integration tests
+
+The `itest/` directory contains a suite of integration tests that actually exercise `gdextension` from within Godot.
+
+The `itest/rust` directory is a Rust `cdylib` library project that can be loaded as a GDExtension in Godot, with an entry point for running integration tests. The `itest/godot` directory contains the Godot project that loads this library and invokes the test suite.
+
+You can run the integration tests like this:
+
+```
+$ ./check.sh itest
+```
+
+Just like when compiling the crate, the `GODOT4_BIN` environment variable can be used to supply the path and filename of your Godot executable.
+
+## Formatting
+
+`rustfmt` is used to format code. `check.sh` only warns about formatting issues, but does not fix them. To do that, run:
+
+```
+$ cargo fmt
+```
+
+## Clippy
+
+`clippy` is used for additional lint warnings not implemented in `rustc`. This, too, is best run through `check.sh`:
+
+```
+$ check.sh clippy
+```
+
+[GitHub issue]: https://github.com/godot-rust/gdextension/issues
+[Discord]: https://discord.gg/aKUCJ8rJsc

ReadMe.md

@@ -90,12 +90,7 @@ those changes available (and only those, no surrounding code).
 
 ## Contributing
 
-At this stage, we appreciate if users experiment with the library, use it in small projects and report issues and bugs they encounter.
-
-If you plan to make bigger contributions, make sure to discuss them in a GitHub issue first. Since the library is evolving quickly, this
-avoids that multiple people work on the same thing or implement features in a way that doesn't work with other parts. Also don't hesitate
-to talk to the developers in the `#gdext-dev` channel on [Discord]!
-
+Contributions are very welcome! If you want to help out, see [`Contributing.md`](Contributing.md) for some pointers on getting started!
 
 [Godot]: https://godotengine.org
 [`gdnative`]: https://github.com/godot-rust/gdnative

godot-core/src/builtin/variant/impls.rs

@@ -48,7 +48,9 @@ macro_rules! impl_variant_traits {
                 // void String::operator=(const String &p_str) { _cowdata._ref(p_str._cowdata); }
                 // does a copy-on-write and explodes if this->_cowdata is not initialized.
                 // We can thus NOT use Self::from_sys_init().
-
+                if variant.get_type() != Self::variant_type() {
+                    return Err(VariantConversionError)
+                }
                 let mut value = <$T>::default();
                 let result = unsafe {
                     let converter = sys::builtin_fn!($to_fn);

godot/src/lib.rs

@@ -4,6 +4,70 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
+//! Rust bindings for GDExtension, the extension API of [Godot](https://godotengine.org/) 4.
+//!
+//! This documentation is a work in progress.
+//!
+//! # Kinds of types
+//!
+//! Godot is written in C++, which doesn't have the same strict guarantees about safety and
+//! mutability that Rust does. As a result, not everything in this crate will look and feel
+//! entirely "Rusty". We distinguish four different kinds of types:
+//!
+//! 1. **Value types**: `i64`, `f64`, and mathematical types like
+//!    [`Vector2`][crate::builtin::Vector2] and [`Color`][crate::builtin::Color].
+//!
+//!    These are the simplest to understand and to work with. They implement `Clone` and often
+//!    `Copy` as well. They are implemented with the same memory layout as their counterparts in
+//!    Godot itself, and typically have public fields. <br><br>
+//!
+//! 2. **Copy-on-write types**: [`GodotString`][crate::builtin::GodotString],
+//!    [`StringName`][crate::builtin::StringName], and `Packed*Array` types.
+//!
+//!    These mostly act like value types, similar to Rust's own `Vec`. You can `Clone` them to get
+//!    a full copy of the entire object, as you would expect.
+//!
+//!    Under the hood in Godot, these types are implemented with copy-on-write, so that data can be
+//!    shared until one of the copies needs to be modified. However, this performance optimization
+//!    is entirely hidden from the API and you don't normally need to worry about it. <br><br>
+//!
+//! 3. **Reference-counted types**: [`Array`][crate::builtin::Array],
+//!    [`Dictionary`][crate::builtin::Dictionary], and [`Gd<T>`][crate::obj::Gd] where `T` inherits
+//!    from [`RefCounted`][crate::engine::RefCounted].
+//!
+//!    These types may share their underlying data between multiple instances: changes to one
+//!    instance are visible in another. Think of them as `Rc<RefCell<...>>` but without any runtime
+//!    borrow checking.
+//!
+//!    Since there is no way to prevent or even detect this sharing from Rust, you need to be more
+//!    careful when using such types. For example, when iterating over an `Array`, make sure that
+//!    it isn't being modified at the same time through another reference.
+//!
+//!    To avoid confusion these types don't implement `Clone`. You can use
+//!    [`Share`][crate::obj::Share] to create a new reference to the same instance, and
+//!    type-specific methods such as
+//!    [`Array::duplicate_deep()`][crate::builtin::Array::duplicate_deep] to make actual
+//!    copies. <br><br>
+//!
+//! 4. **Manually managed types**: [`Gd<T>`][crate::obj::Gd] where `T` inherits from
+//!    [`Object`][crate::engine::Object] but not from [`RefCounted`][crate::engine::RefCounted];
+//!    most notably, this includes all `Node` classes.
+//!
+//!    These also share data, but do not use reference counting to manage their memory. Instead,
+//!    you must either hand over ownership to Godot (e.g. by adding a node to the scene tree) or
+//!    free them manually using [`Gd::free()`][crate::obj::Gd::free]. <br><br>
+//!
+//! # Thread safety
+//!
+//! [Godot's own thread safety
+//! rules](https://docs.godotengine.org/en/latest/tutorials/performance/thread_safe_apis.html)
+//! apply. Types in this crate implement (or don't implement) `Send` and `Sync` wherever
+//! appropriate, but the Rust compiler cannot check what happens to an object through C++ or
+//! GDScript.
+//!
+//! As a rule of thumb, if you must use threading, prefer to use Rust threads instead of Godot
+//! threads.
+
 #[doc(inline)]
 pub use godot_core::{builtin, engine, log, obj, sys};
 

itest/rust/src/variant_test.rs

@@ -10,6 +10,7 @@ use godot::builtin::{
 };
 use godot::engine::Node3D;
 use godot::obj::InstanceId;
+use godot::prelude::{Array, Dictionary, VariantConversionError};
 use godot::sys::{GodotFfi, VariantOperator, VariantType};
 use std::cmp::Ordering;
 use std::fmt::{Debug, Display};
@@ -27,6 +28,8 @@ pub fn run() -> bool {
     ok &= variant_sys_conversion();
     ok &= variant_sys_conversion2();
     ok &= variant_null_object_is_nil();
+    ok &= variant_conversion_fails();
+    ok &= variant_type_correct();
     ok
 }
 
@@ -238,6 +241,52 @@ fn variant_sys_conversion2() {
     */
 }
 
+#[itest]
+fn variant_conversion_fails() {
+    assert_eq!(
+        "hello".to_variant().try_to::<i64>(),
+        Err(VariantConversionError)
+    );
+    assert_eq!(28.to_variant().try_to::<f32>(), Err(VariantConversionError));
+    assert_eq!(
+        10.to_variant().try_to::<bool>(),
+        Err(VariantConversionError)
+    );
+    assert_eq!(
+        false.to_variant().try_to::<String>(),
+        Err(VariantConversionError)
+    );
+    assert_eq!(
+        Array::default().to_variant().try_to::<StringName>(),
+        Err(VariantConversionError)
+    );
+    assert_eq!(
+        Dictionary::default().to_variant().try_to::<Array>(),
+        Err(VariantConversionError)
+    );
+    assert_eq!(
+        Variant::nil().to_variant().try_to::<Dictionary>(),
+        Err(VariantConversionError)
+    );
+}
+
+#[itest]
+fn variant_type_correct() {
+    assert_eq!(Variant::nil().get_type(), VariantType::Nil);
+    assert_eq!(0.to_variant().get_type(), VariantType::Int);
+    assert_eq!(3.8.to_variant().get_type(), VariantType::Float);
+    assert_eq!(false.to_variant().get_type(), VariantType::Bool);
+    assert_eq!("string".to_variant().get_type(), VariantType::String);
+    assert_eq!(
+        StringName::from("string_name").to_variant().get_type(),
+        VariantType::StringName
+    );
+    assert_eq!(Array::default().to_variant().get_type(), VariantType::Array);
+    assert_eq!(
+        Dictionary::default().to_variant().get_type(),
+        VariantType::Dictionary
+    );
+}
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 
 fn roundtrip<T>(value: T)