More robust encode/decode APIs with error handling and clear docs

Bromeon · Bromeon · commit a0d308f71ac8 · 2024-12-31T17:01:11.000+01:00
diff --git a/godot-core/src/builtin/collections/packed_array.rs b/godot-core/src/builtin/collections/packed_array.rs
@@ -5,6 +5,10 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/.
  */
 
+// Result<..., ()> is used. But we don't have more error info. https://rust-lang.github.io/rust-clippy/master/index.html#result_unit_err.
+// We may want to change () to something like godot::meta::IoError, or a domain-specific one, in the future.
+#![allow(clippy::result_unit_err)]
+
 use godot_ffi as sys;
 
 use crate::builtin::*;
@@ -835,119 +839,184 @@ impl<'r> PackedTraits for meta::CowArg<'r, GString> {
 
 macro_rules! declare_encode_decode {
     // $Via could be inferred, but ensures we have the correct type expectations.
-    ($Ty:ty, $encode_fn:ident, $decode_fn:ident, $Via:ty) => {
-        #[doc = concat!("Encodes a value of type `", stringify!($Ty), "` at position `byte_offset`.")]
+    ($Ty:ty, $bytes:literal, $encode_fn:ident, $decode_fn:ident, $Via:ty) => {
+        #[doc = concat!("Encodes `", stringify!($Ty), "` as ", stringify!($bytes), " byte(s) at position `byte_offset`.")]
+        ///
+        /// Returns `Err` if there is not enough space left to write the value, and does nothing in that case.
         ///
-        /// Godot will print an error on encode failure (not enough space left). If you want to detect this programmatically,
-        /// you need to check the offset manually -- or write directly to a Rust data structure, such as `Vec<u8>`.
-        pub fn $encode_fn(&self, byte_offset: usize, value: $Ty) {
-            // We trust Godot that indeed only fitting values are returned.
+        /// **Note:** byte order and encoding pattern is an implementation detail. For portable byte representation and faster encoding, use
+        /// [`as_mut_slice()`][Self::as_mut_slice] and the various Rust standard APIs such as
+        #[doc = concat!("[`", stringify!($Ty), "::to_be_bytes()`].")]
+        pub fn $encode_fn(&mut self, byte_offset: usize, value: $Ty) -> Result<(), ()> {
+            // sys::static_assert!(std::mem::size_of::<$Ty>() == $bytes); -- used for testing, can't keep enabled due to half-floats.
+
+            if byte_offset + $bytes > self.len() {
+                return Err(());
+            }
+
             self.as_inner()
                 .$encode_fn(byte_offset as i64, value as $Via);
+            Ok(())
         }
 
-        #[doc = concat!("Decodes a value of type `", stringify!($Ty), "` at position `byte_offset`.")]
+        #[doc = concat!("Decodes `", stringify!($Ty), "` from ", stringify!($bytes), " byte(s) at position `byte_offset`.")]
+        ///
+        /// Returns `Err` if there is not enough space left to read the value. In case Godot has other error conditions for decoding, it may
+        /// return zero and print an error.
         ///
-        /// Godot will print an error on decode failure (not enough space left), and return `0`. If you want to detect this programmatically,
-        /// you need to check the offset manually -- or convert the packed array to a Rust data structure, such as `Vec<u8>`.
-        pub fn $decode_fn(&self, byte_offset: usize) -> $Ty {
-            // We trust Godot that indeed only fitting values are returned.
+        /// **Note:** byte order and encoding pattern is an implementation detail. For portable byte representation and faster decoding, use
+        /// [`as_slice()`][Self::as_slice] and the various Rust standard APIs such as
+        #[doc = concat!("[`", stringify!($Ty), "::from_be_bytes()`].")]
+        pub fn $decode_fn(&self, byte_offset: usize) -> Result<$Ty, ()> {
+            if byte_offset + $bytes > self.len() {
+                return Err(());
+            }
+
             let decoded: $Via = self.as_inner().$decode_fn(byte_offset as i64);
-            decoded as $Ty
+            Ok(decoded as $Ty)
         }
     };
 }
 
 impl PackedByteArray {
-    declare_encode_decode!(u8, encode_u8, decode_u8, i64);
-    declare_encode_decode!(i8, encode_s8, decode_s8, i64);
-    declare_encode_decode!(u16, encode_u16, decode_u16, i64);
-    declare_encode_decode!(i16, encode_s16, decode_s16, i64);
-    declare_encode_decode!(u32, encode_u32, decode_u32, i64);
-    declare_encode_decode!(i32, encode_s32, decode_s32, i64);
-    declare_encode_decode!(u64, encode_u64, decode_u64, i64);
-    declare_encode_decode!(i64, encode_s64, decode_s64, i64);
-    declare_encode_decode!(f32, encode_half, decode_half, f64);
-    declare_encode_decode!(f32, encode_float, decode_float, f64);
-    declare_encode_decode!(f64, encode_double, decode_double, f64);
-
-    /// Encodes a `Variant` as bytes. Returns number of bytes written.
+    declare_encode_decode!(u8, 1, encode_u8, decode_u8, i64);
+    declare_encode_decode!(i8, 1, encode_s8, decode_s8, i64);
+    declare_encode_decode!(u16, 2, encode_u16, decode_u16, i64);
+    declare_encode_decode!(i16, 2, encode_s16, decode_s16, i64);
+    declare_encode_decode!(u32, 4, encode_u32, decode_u32, i64);
+    declare_encode_decode!(i32, 4, encode_s32, decode_s32, i64);
+    declare_encode_decode!(u64, 8, encode_u64, decode_u64, i64);
+    declare_encode_decode!(i64, 8, encode_s64, decode_s64, i64);
+    declare_encode_decode!(f32, 2, encode_half, decode_half, f64);
+    declare_encode_decode!(f32, 4, encode_float, decode_float, f64);
+    declare_encode_decode!(f64, 8, encode_double, decode_double, f64);
+
+    /// Encodes a `Variant` as bytes. Returns number of bytes written, or `Err` on encoding failure.
     ///
     /// Sufficient space must be allocated, depending on the encoded variant's size. If `allow_objects` is false, [`VariantType::OBJECT`] values
     /// are not permitted and will instead be serialized as ID-only. You should set `allow_objects` to false by default.
     pub fn encode_var(
-        &self,
+        &mut self,
         byte_offset: usize,
         value: impl AsArg<Variant>,
         allow_objects: bool,
-    ) -> usize {
+    ) -> Result<usize, ()> {
         meta::arg_into_ref!(value);
 
         let bytes_written: i64 =
             self.as_inner()
                 .encode_var(byte_offset as i64, value, allow_objects);
-        bytes_written as usize
-    }
 
-    /// Returns `true` if a valid `Variant` value can be decoded at `byte_offset`.
-    ///
-    /// Returns `false` otherwise, or when the value is of type [`VariantType::OBJECT`] and `allow_objects` is `false`.
-    ///
-    /// # Security
-    /// You should set `allow_objects` to `false` unless you have a good reason not to. Decoding objects (e.g. coming from remote sources)
-    /// can cause arbitrary code execution.
-    pub fn has_encoded_var(&self, byte_offset: usize, allow_objects: bool) -> bool {
-        self.as_inner()
-            .has_encoded_var(byte_offset as i64, allow_objects)
+        if bytes_written == -1 {
+            Err(())
+        } else {
+            Ok(bytes_written as usize)
+        }
     }
 
-    /// Decodes a `Variant` from bytes and returns it.
+    /// Decodes a `Variant` from bytes and returns it, alongside the number of bytes read.
     ///
-    /// Returns [`Variant::nil()`] if a valid variant can't be decoded, or the value is of type [`VariantType::OBJECT`] and `allow_objects`
-    /// is `false`.
+    /// Returns `Err` on decoding error. If you store legit `NIL` variants inside the byte array, use
+    /// [`decode_var_allow_nil()`][Self::decode_var_allow_nil] instead.
     ///
-    /// To know how many bytes the decoded variant took, you need to separately call [`decode_var_size()`][Self::decode_var_size].
+    /// # API design
+    /// Godot offers three separate methods `decode_var()`, `decode_var_size()` and `has_encoded_var()`. That comes with several problems:
+    /// - `has_encoded_var()` is practically useless, because it performs the full decoding work and then throws away the variant.
+    ///   `decode_var()` can do all that and more.
+    /// - Both `has_encoded_var()` and `decode_var_size()` are unreliable. They don't tell whether an actual variant has been written at
+    ///   the location. They interpret garbage as `Variant::nil()` and return `true` or `4`, respectively. This can very easily cause bugs
+    ///   because surprisingly, some users may expect that `has_encoded_var()` returns _whether a variant has been encoded_.
+    /// - The underlying C++ implementation has all the necessary information (whether a variant is there, how big it is and its value) but the
+    ///   GDExtension API returns only one info at a time, requiring re-decoding on each call.
+    ///
+    /// godot-rust mitigates this somewhat, with the following design:
+    /// - `decode_var()` treats all `NIL`s as errors. This is most often the desired behavior, and if not, `decode_var_allow_nil()` can be used.
+    ///   It's also the only way to detect errors at all -- once you store legit `NIL` values, you can no longer differentiate them from garbage.
+    /// - `decode_var()` returns both the decoded variant and its size. This requires two decoding runs, but only if the variant is actually
+    ///   valid. Again, in many cases, a user needs the size to know where follow-up data in the buffer starts.
+    /// - `decode_var_size()` and `has_encoded_var()` are not exposed.
     ///
     /// # Security
     /// You should set `allow_objects` to `false` unless you have a good reason not to. Decoding objects (e.g. coming from remote sources)
     /// can cause arbitrary code execution.
-    pub fn decode_var(&self, byte_offset: usize, allow_objects: bool) -> Variant {
-        self.as_inner()
-            .decode_var(byte_offset as i64, allow_objects)
+    #[doc(alias = "has_encoded_var", alias = "decode_var_size")]
+    #[inline]
+    pub fn decode_var(
+        &self,
+        byte_offset: usize,
+        allow_objects: bool,
+    ) -> Result<(Variant, usize), ()> {
+        let variant = self
+            .as_inner()
+            .decode_var(byte_offset as i64, allow_objects);
+
+        if variant.is_nil() {
+            return Err(());
+        }
+
+        // It's unfortunate that this does another full decoding, but decode_var() is barely useful without also knowing the size, as it won't
+        // be possible to know where to start reading any follow-up data. Furthermore, decode_var_size() often returns true when there's in fact
+        // no variant written at that place, it just interprets "nil", treats it as valid, and happily returns 4 bytes.
+        //
+        // So we combine the two calls for the sake of convenience and to avoid accidental usage.
+        let size: i64 = self
+            .as_inner()
+            .decode_var_size(byte_offset as i64, allow_objects);
+        debug_assert_ne!(size, -1); // must not happen if we just decoded variant.
+
+        Ok((variant, size as usize))
     }
 
-    /// Decodes a `Variant` from bytes and returns it.
+    /// Unreliable `Variant` decoding, allowing `NIL`.
+    ///
+    /// <div class="warning">
+    /// <p>This method is highly unreliable and will try to interpret anything into variants, even zeroed memory or random byte patterns.
+    /// Only use it if you need a 1:1 equivalent of Godot's <code>decode_var()</code> and <code>decode_var_size()</code> functions.</p>
     ///
-    /// Returns [`Variant::nil()`] if a valid variant can't be decoded, or the value is of type [`VariantType::OBJECT`] and `allow_objects`
-    /// is `false`.
+    /// <p>In the majority of cases, <a href="struct.PackedByteArray.html#method.decode_var" title="method godot::builtin::PackedByteArray::decode_var">
+    /// <code>decode_var()</code></a> is the better choice, as it’s much easier to use correctly. See also its section about the rationale
+    /// behind the current API design.</p>
+    /// </div>
     ///
-    /// This method is designed to be called in combination with [`decode_var()`][Self::decode_var].
+    /// Returns a tuple of two elements:
+    /// 1. the decoded variant. This is [`Variant::nil()`] if a valid variant can't be decoded, or the value is of type [`VariantType::OBJECT`]
+    ///    and `allow_objects` is `false`.
+    /// 2. The number of bytes the variant occupies. This is `0` if running out of space, but most other failures are not recognized.
     ///
     /// # Security
     /// You should set `allow_objects` to `false` unless you have a good reason not to. Decoding objects (e.g. coming from remote sources)
     /// can cause arbitrary code execution.
-    pub fn decode_var_size(&self, byte_offset: usize, allow_objects: bool) -> usize {
-        let size: i64 = self
-            .as_inner()
-            .decode_var_size(byte_offset as i64, allow_objects);
+    #[inline]
+    pub fn decode_var_allow_nil(
+        &self,
+        byte_offset: usize,
+        allow_objects: bool,
+    ) -> (Variant, usize) {
+        let byte_offset = byte_offset as i64;
+
+        let variant = self.as_inner().decode_var(byte_offset, allow_objects);
+        let decoded_size = self.as_inner().decode_var_size(byte_offset, allow_objects);
+        let decoded_size = decoded_size.try_into().unwrap_or_else(|_| {
+            panic!("unexpected value {decoded_size} returned from decode_var_size()")
+        });
 
-        size as usize
+        (variant, decoded_size)
     }
 
     /// Returns a new `PackedByteArray`, with the data of this array compressed.
     ///
-    /// On failure, Godot prints an error and this method returns `None`. (Note that any empty results coming from Godot are mapped to `None`
+    /// On failure, Godot prints an error and this method returns `Err`. (Note that any empty results coming from Godot are mapped to `Err`
     /// in Rust.)
-    pub fn compress(&self, compression_mode: CompressionMode) -> Option<PackedByteArray> {
+    pub fn compress(&self, compression_mode: CompressionMode) -> Result<PackedByteArray, ()> {
         let compressed: PackedByteArray = self.as_inner().compress(compression_mode.ord() as i64);
-        populated_or_none(compressed)
+        populated_or_err(compressed)
     }
 
     /// Returns a new `PackedByteArray`, with the data of this array decompressed.
     ///
     /// Set `buffer_size` to the size of the uncompressed data.
     ///
-    /// On failure, Godot prints an error and this method returns `None`. (Note that any empty results coming from Godot are mapped to `None`
+    /// On failure, Godot prints an error and this method returns `Err`. (Note that any empty results coming from Godot are mapped to `Err`
     /// in Rust.)
     ///
     /// **Note:** Decompression is not guaranteed to work with data not compressed by Godot, for example if data compressed with the deflate
@@ -956,12 +1025,12 @@ impl PackedByteArray {
         &self,
         buffer_size: usize,
         compression_mode: CompressionMode,
-    ) -> Option<PackedByteArray> {
+    ) -> Result<PackedByteArray, ()> {
         let decompressed: PackedByteArray = self
             .as_inner()
             .decompress(buffer_size as i64, compression_mode.ord() as i64);
 
-        populated_or_none(decompressed)
+        populated_or_err(decompressed)
     }
 
     /// Returns a new `PackedByteArray`, with the data of this array decompressed, and without fixed decompression buffer.
@@ -976,26 +1045,29 @@ impl PackedByteArray {
     /// `max_output_size`. Passing `None` will allow for unbounded output. If any positive value is passed, and the decompression exceeds that
     /// amount in bytes, then an error will be returned.
     ///
+    /// On failure, Godot prints an error and this method returns `Err`. (Note that any empty results coming from Godot are mapped to `Err`
+    /// in Rust.)
+    ///
     /// **Note:** Decompression is not guaranteed to work with data not compressed by Godot, for example if data compressed with the deflate
     /// compression mode lacks a checksum or header.
     pub fn decompress_dynamic(
         &self,
         max_output_size: Option<usize>,
         compression_mode: CompressionMode,
-    ) -> Option<PackedByteArray> {
+    ) -> Result<PackedByteArray, ()> {
         let max_output_size = max_output_size.map(|i| i as i64).unwrap_or(-1);
         let decompressed: PackedByteArray = self
             .as_inner()
             .decompress_dynamic(max_output_size, compression_mode.ord() as i64);
 
-        populated_or_none(decompressed)
+        populated_or_err(decompressed)
     }
 }
 
-fn populated_or_none(array: PackedByteArray) -> Option<PackedByteArray> {
+fn populated_or_err(array: PackedByteArray) -> Result<PackedByteArray, ()> {
     if array.is_empty() {
-        None
+        Err(())
     } else {
-        Some(array)
+        Ok(array)
     }
 }
diff --git a/itest/rust/src/builtin_tests/containers/packed_array_test.rs b/itest/rust/src/builtin_tests/containers/packed_array_test.rs
@@ -7,9 +7,10 @@
 
 use crate::framework::{expect_panic, itest};
 use godot::builtin::{
-    Color, GString, PackedByteArray, PackedColorArray, PackedFloat32Array, PackedInt32Array,
-    PackedStringArray,
+    dict, Color, GString, PackedByteArray, PackedColorArray, PackedFloat32Array, PackedInt32Array,
+    PackedStringArray, Variant,
 };
+use godot::prelude::ToGodot;
 
 #[itest]
 fn packed_array_default() {
@@ -305,3 +306,71 @@ fn packed_array_format() {
     let a = PackedByteArray::new();
     assert_eq!(format!("{a}"), "[]");
 }
+
+#[itest]
+fn packed_byte_array_encode_decode() {
+    let a = PackedByteArray::from(&[0xAB, 0xCD, 0x12]);
+
+    assert_eq!(a.decode_u8(0), Ok(0xAB));
+    assert_eq!(a.decode_u8(2), Ok(0x12));
+    assert_eq!(a.decode_u16(1), Ok(0x12CD)); // Currently little endian, but this may change.
+    assert_eq!(a.decode_u16(2), Err(()));
+    assert_eq!(a.decode_u32(0), Err(()));
+
+    let mut a = a;
+    a.encode_u16(1, 0xEF34).unwrap();
+    assert_eq!(a.decode_u8(0), Ok(0xAB));
+    assert_eq!(a.decode_u8(1), Ok(0x34));
+    assert_eq!(a.decode_u8(2), Ok(0xEF));
+}
+
+#[itest]
+fn packed_byte_array_encode_decode_variant() {
+    let variant = dict! {
+        "s": "some string",
+        "i": -12345,
+    }
+    .to_variant();
+
+    let mut a = PackedByteArray::new();
+    a.resize(40);
+
+    // NIL is a valid, encodable value.
+    let nil = a.encode_var(3, &Variant::nil(), false);
+    assert_eq!(nil, Ok(4));
+
+    let bytes = a.encode_var(3, &variant, false);
+    assert_eq!(bytes, Err(()));
+
+    a.resize(80);
+    let bytes = a.encode_var(3, &variant, false);
+    assert_eq!(bytes, Ok(60)); // Size may change; in that case we only need to verify is_ok().
+
+    // Decoding. Detects garbage.
+    let decoded = a.decode_var(3, false).expect("decode_var() succeeds");
+    assert_eq!(decoded.0, variant);
+    assert_eq!(decoded.1, 60);
+
+    let decoded = a.decode_var(4, false);
+    assert_eq!(decoded, Err(()));
+
+    // Decoding with NILs.
+    let decoded = a.decode_var_allow_nil(3, false);
+    assert_eq!(decoded.0, variant);
+    assert_eq!(decoded.1, 60);
+
+    // Interprets garbage as NIL Variant with size 4.
+    let decoded = a.decode_var_allow_nil(4, false);
+    assert_eq!(decoded.0, Variant::nil());
+    assert_eq!(decoded.1, 4);
+
+    // Even last 4 bytes (still zeroed memory) is allegedly a variant.
+    let decoded = a.decode_var_allow_nil(76, false);
+    assert_eq!(decoded.0, Variant::nil());
+    assert_eq!(decoded.1, 4);
+
+    // Only running out of size "fails".
+    let decoded = a.decode_var_allow_nil(77, false);
+    assert_eq!(decoded.0, Variant::nil());
+    assert_eq!(decoded.1, 0);
+}
