Merge #748

748: Fix `Dictionary::get` unsoundness and add a few convenience functions r=Bromeon a=Waridley

For some strange reason `godot_dictionary_get` calls `Dictionary::operator[]` instead of `Dictionary::get`, which ends up inserting `Nil` if the key doesn't already exist in the dictionary.
https://github.com/godotengine/godot/blame/3.3/modules/gdnative/gdnative/dictionary.cpp#L126
This means it is unsound to call `godot_dictionary_get` on a `Dictionary<Shared>`. This PR re-implements godot-rust's `Dictionary::get` in terms of `godot_dictionary_get_with_default` (the behavior originally intended and likely expected by most Rust users), and adds `Dictionary::get_or_insert_nil` to provide the original behavior if it is indeed desired, but only on `Dictionary<T: LocalThreadAccess>` and an `unsafe` counterpart for `Dictionary<Shared>`, like the other mutating functions on `Dictionary`. 

I also updated the documentation of `get_ref` and `get_mut_ref` since they call `operator[]`. Perhaps they should also be moved to non-shared access, but I figured that's less important since they're unsafe anyway?

I also added `try_get` and `get_non_nil` while I was at it, to simplify checking if a key exists and checking that its corresponding value is not `Nil` respectively.

Co-authored-by: Waridley <Waridley64@gmail.com>

Author: bors[bot]

gdnative-core/src/core_types/dictionary.rs

@@ -69,21 +69,49 @@ impl<Access: ThreadAccess> Dictionary<Access> {
         unsafe { (get_api().godot_dictionary_has_all)(self.sys(), keys.sys()) }
     }
 
-    /// Returns a copy of the value corresponding to the key.
+    /// Returns a copy of the value corresponding to the key if it exists.
     #[inline]
-    pub fn get<K>(&self, key: K) -> Variant
+    pub fn get<K>(&self, key: K) -> Option<Variant>
     where
         K: ToVariant + ToVariantEq,
+    {
+        let key = key.to_variant();
+        if self.contains(&key) {
+            // This should never return the default Nil, but there isn't a safe way to otherwise check
+            // if the entry exists in a single API call.
+            Some(self.get_or_nil(key))
+        } else {
+            None
+        }
+    }
+
+    /// Returns a copy of the value corresponding to the key, or `default` if it doesn't exist
+    #[inline]
+    pub fn get_or<K, D>(&self, key: K, default: D) -> Variant
+    where
+        K: ToVariant + ToVariantEq,
+        D: ToVariant,
     {
         unsafe {
-            Variant((get_api().godot_dictionary_get)(
+            Variant((get_api().godot_dictionary_get_with_default)(
                 self.sys(),
                 key.to_variant().sys(),
+                default.to_variant().sys(),
             ))
         }
     }
 
-    /// Update an existing element corresponding ot the key.
+    /// Returns a copy of the element corresponding to the key, or `Nil` if it doesn't exist.
+    /// Shorthand for `self.get_or(key, Variant::new())`.
+    #[inline]
+    pub fn get_or_nil<K>(&self, key: K) -> Variant
+    where
+        K: ToVariant + ToVariantEq,
+    {
+        self.get_or(key, Variant::new())
+    }
+
+    /// Update an existing element corresponding to the key.
     ///
     /// # Panics
     ///
@@ -106,12 +134,14 @@ impl<Access: ThreadAccess> Dictionary<Access> {
         }
     }
 
-    /// Returns a reference to the value corresponding to the key.
+    /// Returns a reference to the value corresponding to the key, inserting a `Nil` value first if
+    /// it does not exist.
     ///
     /// # Safety
     ///
     /// The returned reference is invalidated if the same container is mutated through another
-    /// reference.
+    /// reference, and other references may be invalidated if the entry does not already exist
+    /// (which causes this function to insert `Nil` and thus possibly re-allocate).
     ///
     /// `Variant` is reference-counted and thus cheaply cloned. Consider using `get` instead.
     #[inline]
@@ -125,13 +155,16 @@ impl<Access: ThreadAccess> Dictionary<Access> {
         ))
     }
 
-    /// Returns a mutable reference to the value corresponding to the key.
+    /// Returns a mutable reference to the value corresponding to the key, inserting a `Nil` value
+    /// first if it does not exist.
     ///
     /// # Safety
     ///
     /// The returned reference is invalidated if the same container is mutated through another
-    /// reference. It is possible to create two mutable references to the same memory location
-    /// if the same `key` is provided, causing undefined behavior.
+    /// reference, and other references may be invalidated if the `key` does not already exist
+    /// (which causes this function to insert `Nil` and thus possibly re-allocate). It is also
+    /// possible to create two mutable references to the same memory location if the same `key`
+    /// is provided, causing undefined behavior.
     #[inline]
     #[allow(clippy::mut_from_ref)]
     pub unsafe fn get_mut_ref<K>(&self, key: K) -> &mut Variant
@@ -425,7 +458,7 @@ unsafe fn iter_next<Access: ThreadAccess>(
         None
     } else {
         let key = Variant::cast_ref(next_ptr).clone();
-        let value = dic.get(&key);
+        let value = dic.get_or_nil(&key);
         *last_key = Some(key.clone());
         Some((key, value))
     }
@@ -591,7 +624,7 @@ godot_test!(test_dictionary {
     let mut iter_keys = HashSet::new();
     let expected_keys = ["foo", "bar"].iter().map(|&s| s.to_string()).collect::<HashSet<_>>();
     for (key, value) in &dict {
-        assert_eq!(value, dict.get(&key));
+        assert_eq!(Some(value), dict.get(&key));
         if !iter_keys.insert(key.to_string()) {
             panic!("key is already contained in set: {:?}", key);
         }

gdnative-core/src/core_types/variant.rs

@@ -1753,7 +1753,7 @@ impl<T: FromVariant, E: FromVariant> FromVariant for Result<T, E> {
 
         match key.as_str() {
             "Ok" => {
-                let val = T::from_variant(&dict.get(key_variant)).map_err(|err| {
+                let val = T::from_variant(&dict.get_or_nil(key_variant)).map_err(|err| {
                     FVE::InvalidEnumVariant {
                         variant: "Ok",
                         error: Box::new(err),
@@ -1762,7 +1762,7 @@ impl<T: FromVariant, E: FromVariant> FromVariant for Result<T, E> {
                 Ok(Ok(val))
             }
             "Err" => {
-                let err = E::from_variant(&dict.get(key_variant)).map_err(|err| {
+                let err = E::from_variant(&dict.get_or_nil(key_variant)).map_err(|err| {
                     FVE::InvalidEnumVariant {
                         variant: "Err",
                         error: Box::new(err),
@@ -1912,11 +1912,11 @@ godot_test!(
     test_variant_result {
         let variant = Result::<i64, ()>::Ok(42_i64).to_variant();
         let dict = variant.try_to_dictionary().expect("should be dic");
-        assert_eq!(Some(42), dict.get("Ok").try_to_i64());
+        assert_eq!(Some(42), dict.get("Ok").and_then(|v| v.try_to_i64()));
 
         let variant = Result::<(), i64>::Err(54_i64).to_variant();
         let dict = variant.try_to_dictionary().expect("should be dic");
-        assert_eq!(Some(54), dict.get("Err").try_to_i64());
+        assert_eq!(Some(54), dict.get("Err").and_then(|v| v.try_to_i64()));
 
         let variant = Variant::from_bool(true);
         assert_eq!(

gdnative-derive/src/variant/from.rs

@@ -87,7 +87,7 @@ pub(crate) fn expand_from_variant(derive_data: DeriveData) -> Result<TokenStream
                         match __key.as_str() {
                             #(
                                 #ref_var_ident_string_literals => {
-                                    let #var_input_ident_iter = &__dict.get(&__keys.get(0));
+                                    let #var_input_ident_iter = &__dict.get_or_nil(&__keys.get(0));
                                     (#var_from_variants).map_err(|err| FVE::InvalidEnumVariant {
                                         variant: #ref_var_ident_string_literals,
                                         error: Box::new(err),

gdnative-derive/src/variant/repr.rs

@@ -280,7 +280,7 @@ impl VariantRepr {
                 let name_string_literals =
                     name_strings.iter().map(|string| Literal::string(&string));
 
-                let expr_variant = &quote!(&__dict.get(&__key));
+                let expr_variant = &quote!(&__dict.get_or_nil(&__key));
                 let exprs = non_skipped_fields
                     .iter()
                     .map(|f| f.from_variant(expr_variant));

test/src/test_derive.rs

@@ -83,19 +83,25 @@ fn test_derive_to_variant() -> bool {
 
         let variant = data.to_variant();
         let dictionary = variant.try_to_dictionary().expect("should be dictionary");
-        assert_eq!(Some(42), dictionary.get("foo").try_to_i64());
-        assert_eq!(Some(54.0), dictionary.get("bar").try_to_f64());
+        assert_eq!(Some(42), dictionary.get("foo").and_then(|v| v.try_to_i64()));
+        assert_eq!(
+            Some(54.0),
+            dictionary.get("bar").and_then(|v| v.try_to_f64())
+        );
         assert_eq!(
             Some("*mut ()".into()),
-            dictionary.get("ptr").try_to_string()
+            dictionary.get("ptr").and_then(|v| v.try_to_string())
         );
         assert!(!dictionary.contains("skipped"));
 
         let enum_dict = dictionary
             .get("baz")
-            .try_to_dictionary()
+            .and_then(|v| v.try_to_dictionary())
             .expect("should be dictionary");
-        assert_eq!(Some(true), enum_dict.get("Foo").try_to_bool());
+        assert_eq!(
+            Some(true),
+            enum_dict.get("Foo").and_then(|v| v.try_to_bool())
+        );
 
         assert_eq!(
             Ok(ToVar::<f64, i128> {
@@ -146,7 +152,7 @@ fn test_derive_owned_to_variant() -> bool {
         let dictionary = variant.try_to_dictionary().expect("should be dictionary");
         let array = dictionary
             .get("arr")
-            .try_to_array()
+            .and_then(|v| v.try_to_array())
             .expect("should be array");
         assert_eq!(3, array.len());
         assert_eq!(