Reflection: try_apply function

[feyokorenhof]

Objective

• Have a non panicking version of Reflect.apply().
• Fixes Non-panicking Version of Reflect.apply() #6182

Solution

• I basically copied the existing code for apply and altered it to return a new error type ApplyError.

• The way it works now is that the error 'bubbles' up all the way up to the caller with an appropriate error so that the user can handle it on their end.

• Note: this 'version' of my implementation takes in a &mut self so It will leave data partially mutated when an error occurs. It is up to the user to handle the error and, for example, clone the data beforehand.

Definition:

fn try_apply(&mut self, value: &dyn Reflect) -> Result<(), ApplyError>;

Implementation for list:

#[inline]
pub fn list_try_apply<L: List>(a: &mut L, b: &dyn Reflect) -> Result<(), ApplyError> {
    if let ReflectRef::List(list_value) = b.reflect_ref() {
        for (i, value) in list_value.iter().enumerate() {
            if i < a.len() {
                if let Some(v) = a.get_mut(i) {
                    v.try_apply(value)?;
                }
            } else {
                List::push(a, value.clone_value());
            }
        }
    } else {
        return Err(ApplyError::MismatchedTypes("list".to_string()));
    }
    Ok(())
}

Changelog

• Added new function try_apply to all the types that implement Reflect.
• Added new error ApplyError in reflect.rs. (not finished, I half-assed the errors, will fix).
• Added documentation for this function in reflect.rs (would like some feedback on how to write this nicely).
• Added try_apply to the examples in utility.rs and type_info.rs.

Migration Guide

No breaking changes

Additional information

This is just a draft so that I can get some feedback on the implementation and thought process.

This version of try_apply takes in a &mut self and potentially leaves the value it was called on in a partially mutated state. We've had some discussion on wether it would be preferred to take in:

• An immutable reference to self and return a Box<dyn Reflect> as result.
• A mutable reference and clone once at the top so that we can return the original untouched data to the user when an error occurs.
• A Box<Self> and return a Box<dyn Reflect>. (I tried to implement this but I haven't figured it out quite yet).

[MrGVSV]

Considerations

To give further details on the alternatives, here's a quick summary of some of the considerations to be made:

1. The error state. How do we handle a failed apply?
2. The success state. Do we return a new value? If so, what is that underlying return type?
3. The receiver type. How accessible is this method? Is requiring ownership a deal-breaker?
4. The amount of cloning involved. How much data are we willing to clone?

&self

Taking &self is good for 1 and 3. Erroring is guaranteed to leave us in a valid state. And &self is definitively the easiest method to access, needing only an immutable reference.

However, it fails in 2 and 4. We're forced to not only return a value, but that returned value is almost always going to be a Dynamic type (e.g. DynamicStruct, etc.). This may not be ideal, and will require the cloning of all data (including the stuff that isn't being patched).

&mut self

Taking &mut self is good for 2, 3, and 4. It should simply mutate the original, meaning we don't need to return a new object. The receiver type of &mut self is not too bad (especially considering it's expected in most contexts when mutating data in Rust). And lastly, we don't really need to clone more data than needed (just the data that's being applied).

However, this really fails on 1 since an error will likely leave the data in an invalid state. As mentioned by @feyokorenhof, the user will have to handle this manually and ensure they take the proper precautions before and after calling the method.

self: Box<Self>

Taking self: Box<Self> is good for 1 (debatable), 2, and 4. The success state is simply the new value. And taking ownership is nice because it also means we don't need to clone any more than necessary. The error state is okay in that we don't end up in any broken state upon failure. Although, this does mean we consume the invalid data, so the user would need to prepare for that beforehand by cloning the data (similar to the &mut self case).

However, this does not do well on 3. It's certainly not ideal to require full ownership of the data in order to modify it.

Possible Solutions

The &mut self as it is, seems like the best solution— save for the bad error state. There are a couple solutions that we could try (there's almost certainly more, but these are the ones I'll suggest for now):

1. Do a deep dry run before running the method. This would require an additional method on Reflect dedicated to the dry run (not great) and would mean we need to recurse into the data structure twice— once to check for errors and once more to actually do the thing.
2. Clone the value at the top level and re-apply upon failure.¹ This method should² never fail when applying a Dynamic representation to its concrete (or Dynamic) counterpart. If we can uphold that, then we can clone the value at the top level, do the thing, and re-apply the cloned value if the apply fails. This would probably require a method for "top-level" apply calls and "nested" apply calls which may not be ideal. It also negatively impacts the benefits to 4 afforded by taking &mut self.

Footnotes

1. This is sorta already possible with the current implementation. The only difference is that this would take that responsibility out of the user's hands and perform the operation automatically. ↩

2. In theory. If this fails, we might want to panic. ↩

[james7132]

Can this be simplified with the ? operator? Same elsewhere in this PR.

[feyokorenhof]

Ah yes probably. I completely forgot about that syntax. Thanks!

[soqb]

I think the current implementation of taking in &mut self and returning Result<(), ApplyError> is the best option. With adequate docs users can just be advised to clone beforehand if they need an unmodified value.

[feyokorenhof]

It seems the build is partly failing. It runs fine on my machine (Mac m1).

CI gives this error:

error[E0046]: not all trait items implemented, missing: `try_apply`

impl Reflect for &'static Path {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `try_apply` in implementation

I have double checked every file but can't find the implementation for &'static Path anywhere so I assume it gets derived automatically.

Also: these are the tests I have used:

Tests

mod test {
  use bevy::reflect::{DynamicEnum, DynamicList, DynamicVariant, Reflect};

  #[test]
  fn test_try_apply_values() {
      // Strings
      let mut s1 = String::from("Hello");
      let s2 = String::from("World");
      // Ok
      let result = s1.try_apply(s2.as_reflect());
      assert!(result.is_ok());
      // Err
      let result = s1.try_apply(0.as_reflect());
      assert!(result.is_err());

      // Floats
      let mut f1 = 10.0;
      let f2 = 15.0;
      // Ok
      let result = f1.try_apply(f2.as_reflect());
      assert!(result.is_ok());
      assert_eq!(f1, 15.0);
      // Err
      let result = f1.try_apply(String::from("hi").as_reflect());
      assert!(result.is_err());

      // Integers
      let mut i1 = 1;
      let i2 = 2;
      // Ok
      let result = i1.try_apply(i2.as_reflect());
      assert!(result.is_ok());
      assert_eq!(i1, 2);
      // Err
      let result = i1.try_apply(String::from("ha").as_reflect());
      assert!(result.is_err());

      // Booleans
      let mut b1 = true;
      let b2 = false;
      // Ok
      let result = b1.try_apply(b2.as_reflect());
      assert!(result.is_ok());
      assert_eq!(b1, false);
      // Err
      let result = b1.try_apply(String::from("ho").as_reflect());
      assert!(result.is_err());
  }

  #[test]
  fn test_try_apply_lists() {
      // Lists
      let mut l1 = DynamicList::default();
      l1.push("hi".to_string());
      let mut l2 = DynamicList::default();
      l2.push("da".to_string());
      l2.push(0);
      let mut l3 = DynamicList::default();
      l3.push(0);
      l3.push("ba".to_string());
      // Ok
      let result = l1.try_apply(l2.as_reflect());
      assert!(result.is_ok());
      // Err
      let result = l1.try_apply(l3.as_reflect());
      assert!(result.is_err());

      // Arrays
      let mut a1 = [0, 1, 2];
      let a2 = [2, 1, 0];
      let a3 = [true, false, true];
      // Ok
      let result = a1.try_apply(a2.as_reflect());
      assert!(result.is_ok());
      // Err
      let result = a1.try_apply(a3.as_reflect());
      assert!(result.is_err());

      // Vectors
      let mut v1 = vec![0, 1, 2];
      let v2 = vec![2, 1, 0];
      let v3 = vec![true, false, true];
      // Ok
      let result = v1.try_apply(v2.as_reflect());
      assert!(result.is_ok());
      assert_eq!(v1, v2);
      // Err
      let result = v1.try_apply(v3.as_reflect());
      assert!(result.is_err());
  }

  #[test]
  fn test_try_apply_structs() {
      #[derive(Reflect, Debug)]
      struct Person {
          name: String,
          age: i32,
      }

      #[derive(Reflect, Debug)]
      struct Alien {
          name: String,
      }

      #[derive(Reflect, Debug)]
      struct Weird {
          name: i32,
      }

      #[derive(Reflect, Debug)]
      struct NestedI32 {
          name: String,
          values: Vec<i32>,
      }

      #[derive(Reflect, Debug)]
      struct NestedString {
          name: String,
          values: Vec<String>,
      }

      let mut person1 = Person {
          name: "Hello".to_string(),
          age: 21,
      };

      let person2 = Person {
          name: "World".to_string(),
          age: 18,
      };

      let alien = Alien {
          name: "X Æ A-12".to_string(),
      };

      let weird = Weird { name: 1 };

      let mut nested1 = NestedI32 {
          name: String::from("Nested Struct"),
          values: vec![0, 1, 2, 3],
      };

      let nested2 = NestedI32 {
          name: String::from("Nested Struct 2"),
          values: vec![3, 2, 1, 0],
      };

      let nested3 = NestedString {
          name: String::from("Nested String Struct"),
          values: vec![String::from("hi")],
      };

      // Ok
      let result = person1.try_apply(person2.as_reflect());
      assert!(result.is_ok());

      // Ok
      let result = person1.try_apply(alien.as_reflect());
      assert!(result.is_ok());

      // Err
      let result = person1.try_apply(weird.as_reflect());
      assert!(result.is_err());

      let result = person1.try_apply(0.as_reflect());
      assert!(result.is_err());

      // Ok
      let result = nested1.try_apply(nested2.as_reflect());
      assert!(result.is_ok());

      // Err
      let result = nested1.try_apply(nested3.as_reflect());
      assert!(result.is_err());
  }

  #[test]
  fn test_try_apply_enums() {
      let mut value = Some(123);
      let mut dyn_enum =
          DynamicEnum::new(Reflect::type_name(&value), "None", DynamicVariant::Unit);
      let result = value.try_apply(&dyn_enum);
      assert!(result.is_ok());
      assert_eq!(None, value);

      let result = dyn_enum.try_apply(value.as_reflect());
      assert!(result.is_ok());
  }

  #[test]
  fn test_try_apply_tuples() {
      let mut t1 = (true, String::from("hi"));
      let t2 = (false, String::from("ha"));
      let t3 = (true, 0);

      let result = t1.try_apply(t2.as_reflect());
      // Ok
      assert!(result.is_ok());
      assert_eq!(t1, (false, String::from("ha")));
      // Err
      let result = t1.try_apply(t3.as_reflect());
      assert!(result.is_err());
  }
}

[soqb]

you're going to rebase with #6755 in that case.

[feyokorenhof]

examples/reflection/reflection.rs:

// You can "apply" Reflect implementations on top of other Reflect implementations.
// This will only set fields with the same name, and it will fail if the types don't match.
// You can use this to "patch" your types with new values.
value.apply(&patch);
assert_eq!(value.a, 4);

here it only mentions it will fail and not panic. Should we be more clear here or even mention both apply and try_apply?

[alice-i-cecile]

@feyokorenhof are you still working on this? Totally fine if not, just let me know so I can nominate this for adoption.

[feyokorenhof]

@alice-i-cecile hey. Not any time soon no sorry! If it’s still open when I have some time I would be happy to take a look at it again but by all means put It up for adoption 😃

[alice-i-cecile]

Awesome, thanks for the reply and for the initial draft :) Regardless of what happens I'll make sure you get credited in the release notes when this ships.

[Brezak]

I'd like to pick this up.

[alice-i-cecile]

Awesome, thanks @Brezak!