Remove redundant check in Reflect::downcast method

[alice-i-cecile]

When attempting to downcast to a type, we check if the type is correct here. However, the Any::downcast method used inside this call does this anyways.

We should remove this check, and then match on the returned Result from the Any version.

Spotted by @cart in #5010 here.

[alice-i-cecile]

We can't [do this] without changing the error type from Box to Box

From @PROMETHIA-27.

[DevinLeamy]

Hi. I've got this!

[DevinLeamy]

I've attempted to take the prescribed approach to resolve this. self.into_any() takes ownership of self, however,
so to satisfy the borrow checker I've had to clone self and return the resulting clone (in case of a failed downcast), because I can't return Err(self) in the match.

I would prefer to not create a copy, however, being new to Rust I'm not sure how to avoid this. I considered
casting from the resulting Box<dyn Any> held in Err(_), however, this seems to require introducing a new trait.

Am I missing something here? 😄

pub fn downcast<T: Reflect>(self: Box<dyn Reflect>) -> Result<Box<T>, Box<dyn Reflect>> {
        let copy_of_self = self.clone_value();

        match self.into_any().downcast::<T>() {
            Ok(downcasted) => Ok(downcasted),
            Err(_) => Err(copy_of_self),
        }
}

[PROMETHIA-27]

This won't work. clone_value() doesn't return the original type, it returns a Dynamic*** type. This would silently throw out the original instance if you called and got an error back. It also would almost certainly have a greater performance overhead than a redundant TypeId check. Like I said on the original issue, I don't think we can do this, period, at least safely. And the whole point of introducing that change was to make Reflect safer.

We can use unsafe and eliminate the Any check by creating a new trait, GetTypeId, and doing

impl<T: Reflect> GetTypeId for Reflect {
    fn get_type_id(&self) -> TypeId {
        TypeId::of::<T>()
    }
}

and then using that and a manual comparison instead of Any::is(). Then we copy the (unsafe) code from Any::downcast.
This should be sound- but there's a potential issue around Box<dyn Reflect>, which was the motivation for make-reflect-safe to begin with. If a Box<Box<dyn Reflect>> is casted to Box<dyn Reflect>, I'm not completely certain of which type's GetTypeId implementation would be called. It should be the correct one, but I'd want to test it thoroughly and have other people who are more used to unsafe sign off on it.

Overall I don't think this issue is actually a good first issue, and I think it's a lot of effort for a microoptimization.

[DevinLeamy]

Thanks for the response. Learned a few things. I'll take myself out of the running for this issue 👍