Merge #1015 #1020

bors[bot] · chitoyuu · web-flow · commit 90502118b03d · 2023-01-31T17:32:01.000Z
1015: Add `#[non_exhaustive]` to most public enums r=chitoyuu a=chitoyuu Additionally, utilizes the `clippy::exhaustive_enums` lint to prevent new exhaustive ones from appearing in the public interface. This makes it non-breaking to add new variants to enums that aren't intended to be exhaustively matched on. Close #987 1020: Remove Export impls with surprising behavior r=chitoyuu a=chitoyuu Also added documentation on the rationales behind leaving `Export` unimplemented for standard Rust collections, pointing to alternatives. Close #1009 Co-authored-by: Chitose Yuuzaki <chitoyuu@potatoes.gay>
diff --git a/.github/workflows/full-ci.yml b/.github/workflows/full-ci.yml
@@ -74,7 +74,7 @@ jobs:
           rust: ${{ matrix.rust.toolchain }}
           components: clippy
       - name: "Check clippy"
-        run: cargo clippy --workspace --features ${GDRUST_FEATURES} -- -D clippy::style -D clippy::complexity -D clippy::perf -D clippy::dbg_macro -D clippy::todo -D clippy::unimplemented
+        run: cargo clippy --workspace --features ${GDRUST_FEATURES} -- -D clippy::style -D clippy::complexity -D clippy::perf -D clippy::dbg_macro -D clippy::todo -D clippy::unimplemented -D warnings
 
   cargo-deny-machete:
     runs-on: ubuntu-latest
diff --git a/examples/property-export/GDScriptPrinter.gd b/examples/property-export/GDScriptPrinter.gd
@@ -4,17 +4,17 @@ func _ready():
 	var rust = get_node("../PropertyExport")
 
 	print("\n-----------------------------------------------------------------")
-	print("Print from GDScript (note the lexicographically ordered map/set):")
-	print("  Vec (name):");
+	print("Print from GDScript:")
+	print("  PoolArray<GodotString>:");
 	for name in rust.name_vec:
 		print("  * %s" % name)
 
-	print("\n  HashMap (string -> color):")
+	print("\n  Dictionary (string -> color):")
 	for string in rust.color_map:
 		var color = rust.color_map[string]
 		print("  * %s -> #%s" % [string, color.to_html(false)]);
 
-	print("\n  HashSet (ID):")
+	print("\n  PoolArray<i32>:")
 	for id in rust.id_set:
 		print("  * %s" % id)
 
diff --git a/examples/property-export/Main.tscn b/examples/property-export/Main.tscn
@@ -12,13 +12,13 @@ library = ExtResource( 1 )
 
 [node name="PropertyExport" type="Node" parent="."]
 script = SubResource( 1 )
-name_vec = [ "Godot", "Godette", "Go ." ]
+name_vec = PoolStringArray("Godot", "Godette", "Go .")
 color_map = {
 "blue": Color( 0.184314, 0.160784, 0.8, 1 ),
 "green": Color( 0.0941176, 0.447059, 0.192157, 1 ),
 "teal": Color( 0.0941176, 0.423529, 0.564706, 1 )
 }
-id_set = [ 21, 77, 8, 90 ]
+id_set = PoolIntArray(21, 77, 8, 90)
 
 [node name="GDScriptPrinter" type="Node" parent="."]
 script = ExtResource( 2 )
diff --git a/examples/property-export/src/lib.rs b/examples/property-export/src/lib.rs
@@ -1,18 +1,16 @@
 use gdnative::prelude::*;
 
-use std::collections::{HashMap, HashSet};
-
 #[derive(NativeClass, Default)]
 #[inherit(Node)]
 pub struct PropertyExport {
     #[property]
-    name_vec: Vec<String>,
+    name_vec: PoolArray<GodotString>,
 
     #[property]
-    color_map: HashMap<GodotString, Color>,
+    color_map: Dictionary,
 
     #[property]
-    id_set: HashSet<i64>,
+    id_set: PoolArray<i32>,
 }
 
 #[methods]
@@ -24,19 +22,20 @@ impl PropertyExport {
     #[method]
     fn _ready(&self) {
         godot_print!("------------------------------------------------------------------");
-        godot_print!("Print from Rust (note the unordered map/set):");
-        godot_print!("  Vec (name):");
-        for name in &self.name_vec {
+        godot_print!("Print from Rust:");
+        godot_print!("  PoolArray<GodotString>:");
+        for name in &*self.name_vec.read() {
             godot_print!("  * {}", name);
         }
 
-        godot_print!("\n  HashMap (string -> color):");
+        godot_print!("\n  Dictionary (string -> color):");
         for (string, color) in &self.color_map {
+            let color = Color::from_variant(&color).unwrap();
             godot_print!("  * {} -> #{}", string, color.to_html(false));
         }
 
-        godot_print!("\n  HashSet (ID):");
-        for id in &self.id_set {
+        godot_print!("\n  PoolArray<i32>:");
+        for id in &*self.id_set.read() {
             godot_print!("  * {}", id);
         }
     }
diff --git a/gdnative-async/src/lib.rs b/gdnative-async/src/lib.rs
@@ -6,6 +6,8 @@
 //!
 //! This crate assumes that all user non-Rust code follow the official threading guidelines.
 
+#![warn(clippy::exhaustive_enums)]
+
 // Workaround for macros that expect the `gdnative` crate.
 extern crate gdnative_core as gdnative;
 
diff --git a/gdnative-core/src/core_types/error.rs b/gdnative-core/src/core_types/error.rs
@@ -5,6 +5,7 @@ use crate::sys;
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[repr(u32)]
+#[non_exhaustive]
 pub enum GodotError {
     Failed = sys::godot_error_GODOT_FAILED as u32,
     Unavailable = sys::godot_error_GODOT_ERR_UNAVAILABLE as u32,
diff --git a/gdnative-core/src/core_types/geom/rect2.rs b/gdnative-core/src/core_types/geom/rect2.rs
@@ -267,6 +267,7 @@ pub struct MarginError(i64);
 ///
 /// [margin]: https://docs.godotengine.org/en/stable/classes/class_%40globalscope.html#enum-globalscope-margin
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]
+#[non_exhaustive]
 pub enum Margin {
     Left,
     Top,
diff --git a/gdnative-core/src/core_types/string.rs b/gdnative-core/src/core_types/string.rs
@@ -390,6 +390,7 @@ pub struct GodotChar(libc::wchar_t);
 
 /// Error indicating that a `GodotChar` cannot be converted to a `char`.
 #[derive(Debug)]
+#[non_exhaustive]
 pub enum GodotCharError {
     /// The character cannot be represented as a Unicode code point.
     InvalidCodePoint,
diff --git a/gdnative-core/src/core_types/variant.rs b/gdnative-core/src/core_types/variant.rs
@@ -81,6 +81,7 @@ macro_rules! decl_variant_type {
     ) => {
         #[repr(u32)]
         #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
+        #[non_exhaustive]
         pub enum VariantType {
             $(
                 $variant = $c_const as u32,
@@ -99,6 +100,7 @@ macro_rules! decl_variant_type {
         /// For `Variant`s containing objects, the original `Variant` is returned unchanged, due to
         /// the limitations of statically-determined memory management.
         #[repr(u32)]
+        #[non_exhaustive]
         pub enum VariantDispatch {
             $(
                 $variant $( ($inner) )?,
@@ -186,6 +188,7 @@ impl VariantType {
 #[allow(clippy::unnecessary_cast)] // False positives: casts necessary for cross-platform
 #[repr(u32)]
 #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
+#[non_exhaustive]
 pub enum CallError {
     InvalidMethod =
         sys::godot_variant_call_error_error_GODOT_CALL_ERROR_CALL_ERROR_INVALID_METHOD as u32,
@@ -236,6 +239,7 @@ impl std::error::Error for CallError {}
 #[allow(clippy::unnecessary_cast)] // False positives: casts necessary for cross-platform
 #[repr(u32)]
 #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
+#[non_exhaustive]
 pub enum VariantOperator {
     // Comparison
     Equal = sys::godot_variant_operator_GODOT_VARIANT_OP_EQUAL as u32,
@@ -856,6 +860,7 @@ pub trait CoerceFromVariant: Sized + private::Sealed {
 }
 
 #[derive(Clone, PartialEq, Eq, Debug)]
+#[non_exhaustive]
 /// Error type returned by `FromVariant::from_variant`.
 pub enum FromVariantError {
     /// An unspecified error.
@@ -924,11 +929,13 @@ pub enum FromVariantError {
 }
 
 #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]
+#[non_exhaustive]
 pub enum VariantEnumRepr {
     ExternallyTagged,
 }
 
 #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]
+#[non_exhaustive]
 pub enum VariantStructRepr {
     Unit,
     Tuple,
diff --git a/gdnative-core/src/core_types/vector3.rs b/gdnative-core/src/core_types/vector3.rs
@@ -15,7 +15,10 @@ pub struct Vector3 {
     pub z: f32,
 }
 
-#[allow(clippy::unnecessary_cast)] // False positives: casts necessary for cross-platform
+#[allow(
+    clippy::unnecessary_cast, // False positives: casts necessary for cross-platform
+    clippy::exhaustive_enums, // explicitly exhaustive since there can't be more axes for Vector3
+)]
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
 #[repr(u32)]
 pub enum Axis {
diff --git a/gdnative-core/src/export/method.rs b/gdnative-core/src/export/method.rs
@@ -97,6 +97,7 @@ type ScriptMethodFn = unsafe extern "C" fn(
 ) -> sys::godot_variant;
 
 #[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)]
+#[non_exhaustive]
 pub enum RpcMode {
     Disabled,
     Remote,
@@ -483,6 +484,7 @@ varargs_into_tuple!(A, B, C, D, E, F, G, H, I, J, K, L);
 /// For methods that return this error, see [`Varargs::check_length()`], [`Varargs::get()`] or [`Varargs::get_opt()`].
 /// Another context where this type is used is when destructuring `Varargs` into tuples.
 #[derive(Debug)]
+#[non_exhaustive]
 pub enum VarargsError {
     /// At least one argument type mismatches.
     InvalidArgumentType {
diff --git a/gdnative-core/src/export/property.rs b/gdnative-core/src/export/property.rs
@@ -23,6 +23,38 @@ mod invalid_accessor;
 pub mod hint;
 
 /// Trait for exportable types.
+///
+/// ## Rust collections
+///
+/// `Export` is intentionally unimplemented for standard Rust collections, such as [`Vec`] or
+/// [`HashMap`][std::collections::HashMap]. The reason is that such types exhibit surprising
+/// behavior when used from GDScript, due to how [`ToVariant`]/[`FromVariant`] conversions work
+/// for these types.
+///
+/// Godot has no concept of Rust collections, and cannot operate on them. Whenever a standard
+/// collection is converted to [`Variant`] via [`ToVariant`], what actually happens is that:
+///
+/// - First, a new Godot collection of the corresponding "kind" is allocated.
+/// - Then, the Rust collection is iterated over, and each element is converted and inserted into
+///   the new collection, possibly triggering many more allocations in the process.
+///
+/// With properties, this whole process happens anew *with each access to the property*, which
+/// means that:
+///
+/// - Modifying such properties from the remote debugger, or calling methods on the property
+///   directly from GDScript (e.g. `thing.exported_vec.append("foo")`) do not produce the desired
+///   behavior by the user.
+/// - Seemingly innocuous expressions such as
+///   `thing.exported_vec[0] + thing.exported_vec[1] + thing.exported_vec[2]` can be much more
+///   expensive computationally than what the user would expect.
+///
+/// As such, we do not allow these types to be exported as properties directly as a precaution.
+/// If you wish to export collections to GDScript, consider the following options:
+///
+/// - Exporting a [`Variant`] collection such as [`VariantArray`] or [`Dictionary`] explicitly,
+///   embracing their respective semantics.
+/// - Exporting not a property, but methods that have to be explicitly called, to set clear
+///   expectations that the return value might be expensive to produce.
 pub trait Export: crate::core_types::ToVariant {
     /// A type-specific hint type that is valid for the type being exported.
     ///
@@ -455,8 +487,6 @@ pub struct Property<T> {
 }
 
 mod impl_export {
-    use std::collections::{HashMap, HashSet};
-
     use super::*;
 
     /// Hint type indicating that there are no hints available for the time being.
@@ -615,41 +645,4 @@ mod impl_export {
             hint.unwrap_or_default().export_info()
         }
     }
-
-    impl<K, V> Export for HashMap<K, V>
-    where
-        K: std::hash::Hash + ToVariantEq + ToVariant,
-        V: ToVariant,
-    {
-        type Hint = NoHint;
-
-        #[inline]
-        fn export_info(_hint: Option<Self::Hint>) -> ExportInfo {
-            ExportInfo::new(VariantType::Dictionary)
-        }
-    }
-
-    impl<T> Export for HashSet<T>
-    where
-        T: ToVariant,
-    {
-        type Hint = NoHint;
-
-        #[inline]
-        fn export_info(_hint: Option<Self::Hint>) -> ExportInfo {
-            ExportInfo::new(VariantType::VariantArray)
-        }
-    }
-
-    impl<T> Export for Vec<T>
-    where
-        T: ToVariant,
-    {
-        type Hint = NoHint;
-
-        #[inline]
-        fn export_info(_hint: Option<Self::Hint>) -> ExportInfo {
-            ExportInfo::new(VariantType::VariantArray)
-        }
-    }
 }
diff --git a/gdnative-core/src/export/property/hint.rs b/gdnative-core/src/export/property/hint.rs
@@ -145,6 +145,7 @@ impl EnumHint {
 
 /// Possible hints for integers.
 #[derive(Clone, Debug)]
+#[non_exhaustive]
 pub enum IntHint<T> {
     /// Hints that an integer or float property should be within a range.
     Range(RangeHint<T>),
@@ -260,6 +261,7 @@ impl ExpEasingHint {
 
 /// Possible hints for floats.
 #[derive(Clone, Debug)]
+#[non_exhaustive]
 pub enum FloatHint<T> {
     /// Hints that an integer or float property should be within a range.
     Range(RangeHint<T>),
@@ -325,6 +327,7 @@ impl<T> From<ExpEasingHint> for FloatHint<T> {
 
 /// Possible hints for strings.
 #[derive(Clone, Debug)]
+#[non_exhaustive]
 pub enum StringHint {
     /// Hints that an integer, float or string property is an enumerated value to pick in a list.
     Enum(EnumHint),
@@ -373,6 +376,7 @@ impl StringHint {
 
 /// Possible hints for `Color`.
 #[derive(Clone, Debug)]
+#[non_exhaustive]
 pub enum ColorHint {
     /// Hints that a color property should be edited without changing its alpha component.
     NoAlpha,
diff --git a/gdnative-core/src/export/user_data.rs b/gdnative-core/src/export/user_data.rs
@@ -165,6 +165,7 @@ pub type DefaultUserData<T> = LocalCellData<T>;
 
 /// Error type indicating that an operation can't fail.
 #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug)]
+#[allow(clippy::exhaustive_enums)] // explicitly uninhabited
 pub enum Infallible {}
 
 impl std::fmt::Display for Infallible {
@@ -186,6 +187,7 @@ impl std::fmt::Display for Infallible {
 /// As there is no universal way to deal with such situations, behavior of locking wrappers can
 /// be customized using this enum.
 #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug)]
+#[non_exhaustive]
 pub enum DeadlockPolicy {
     /// Block on all locks. Deadlocks are possible.
     Allow,
@@ -223,6 +225,7 @@ impl LockOptions for DefaultLockPolicy {
 
 /// Error indicating that a lock wasn't obtained.
 #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug)]
+#[non_exhaustive]
 pub enum LockFailed {
     Pessimistic,
     Timeout(Duration),
@@ -562,6 +565,7 @@ mod local_cell {
 
     /// Error indicating that a borrow has failed.
     #[derive(Copy, Clone, Eq, PartialEq, Hash, Debug)]
+    #[non_exhaustive]
     pub enum LocalCellError {
         DifferentThread {
             original: ThreadId,
diff --git a/gdnative-core/src/lib.rs b/gdnative-core/src/lib.rs
@@ -19,7 +19,7 @@
 //!
 //! [thread-safety]: https://docs.godotengine.org/en/stable/tutorials/threads/thread_safe_apis.html
 
-#![deny(clippy::missing_inline_in_public_items)]
+#![warn(clippy::missing_inline_in_public_items, clippy::exhaustive_enums)]
 #![allow(
     clippy::transmute_ptr_to_ptr,
     clippy::missing_safety_doc,
diff --git a/gdnative-core/src/object/memory.rs b/gdnative-core/src/object/memory.rs
@@ -4,9 +4,11 @@
 use crate::object::bounds::MemorySpec;
 
 /// Marker that indicates that a type is manually managed.
+#[allow(clippy::exhaustive_enums)] // explicitly uninhabited
 pub enum ManuallyManaged {}
 
 /// Marker that indicates that a type is reference-counted.
+#[allow(clippy::exhaustive_enums)] // explicitly uninhabited
 pub enum RefCounted {}
 
 /// Trait to parameterize over the memory management markers [`ManuallyManaged`] and [`RefCounted`].
diff --git a/gdnative-core/src/private.rs b/gdnative-core/src/private.rs
@@ -179,6 +179,12 @@ unsafe fn report_init_error(
                 f((*options).gd_native_library, message.as_ptr());
             }
         }
+        _ => {
+            if let Some(f) = (*options).report_loading_error {
+                let message = CString::new(format!("{error:?}")).unwrap();
+                f((*options).gd_native_library, message.as_ptr());
+            }
+        }
     }
 }
 
diff --git a/gdnative-sys/src/lib.rs b/gdnative-sys/src/lib.rs
