Skip to content

Commit

Permalink
Remove Reflect::type_descriptor (#90)
Browse files Browse the repository at this point in the history
The idea with `Reflect::type_descriptor` was to get the type descriptor
for the reflected value however it had one big footgun: `<Value as
Reflect>::type_descriptor` returns an opaque type, _not_ the type the
`Value` was created from.

For example:

```rust
fn show_editor_ui(reflect: &mut dyn Reflect) {
    let type_descriptor = reflect.type_descriptor();
    // show ui...
}

struct Foo {}

let foo = Foo {};

// convert the `foo` into a `Value`, perhaps to serialize
// it and send across FFI
let mut foo_value = foo.to_value();

// because `Value` implements `Reflect` it works as a `&dyn mut Reflect`
// but our `show_editor_ui` wont really work because we've lost the type
// information when we called `foo.to_value()`
show_editor_ui(&mut foo_value);
```

The solution is to capture the type descriptor explicitly and store that
together with `foo_value`:

```rust
fn show_editor_ui(reflect: &mut dyn Reflect, type_descriptor: TypeDescriptor) {
    // show ui...
}

// store and serialize both the value and the original type descriptor
let mut foo_value = foo.to_value();
let foo_type_descriptor = <Foo as DescribeType>::type_descriptor();

show_editor_ui(&mut foo_value, foo_type_descriptor);
```

I think because of this it makes sense to remove
`Reflect::type_descriptor` as it should make it easier to users to do
the right thing.

I also considered making a `TypedValue` that is like `Value` but
preserves the type descriptor but I ran into a bunch of technical issues
with that.
  • Loading branch information
davidpdrsn committed Mar 15, 2023
1 parent 8b6bfc6 commit 2e9834e
Show file tree
Hide file tree
Showing 7 changed files with 5 additions and 41 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

# Unreleased

- **breaking:** Remove `Reflect::type_descriptor` ([#90])
- **fixed:** Fully qualify `FromReflect` in generated code ([#107])
- **added:** Implement `Reflect`, and friends, for `Infallible` ([#111])

[#90]: https://github.com/EmbarkStudios/mirror-mirror/pull/90
[#107]: https://github.com/EmbarkStudios/mirror-mirror/pull/107
[#111]: https://github.com/EmbarkStudios/mirror-mirror/pull/111

Expand Down Expand Up @@ -81,8 +83,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
# 0.1.2 (03. February, 2023)

- **added:** Add `impl From<Key> for KeyPath` ([#88])
- **breaking:** Remove `Reflect::type_descriptor`. Instead capture the type
descriptor explicitly with `<T as DescribeType::type_descriptor()` ([#90])

[#88]: https://github.com/EmbarkStudios/mirror-mirror/pull/88
[#90]: https://github.com/EmbarkStudios/mirror-mirror/pull/90

# 0.1.1 (17. January, 2023)

Expand Down
7 changes: 0 additions & 7 deletions crates/mirror-mirror-macros/src/derive_reflect/enum_.rs
Original file line number Diff line number Diff line change
Expand Up @@ -294,12 +294,6 @@ fn expand_reflect(
}
};

let fn_type_info = quote! {
fn type_descriptor(&self) -> Cow<'static, TypeDescriptor> {
<Self as DescribeType>::type_descriptor()
}
};

let fn_debug = attrs.fn_debug_tokens();
let fn_clone_reflect = attrs.fn_clone_reflect_tokens();

Expand Down Expand Up @@ -327,7 +321,6 @@ fn expand_reflect(
self
}

#fn_type_info
#fn_patch
#fn_to_value
#fn_clone_reflect
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,12 +132,6 @@ fn expand_reflect(
}
};

let fn_type_info = quote! {
fn type_descriptor(&self) -> Cow<'static, TypeDescriptor> {
<Self as DescribeType>::type_descriptor()
}
};

let fn_debug = attrs.fn_debug_tokens();
let fn_clone_reflect = attrs.fn_clone_reflect_tokens();

Expand Down Expand Up @@ -165,7 +159,6 @@ fn expand_reflect(
self
}

#fn_type_info
#fn_patch
#fn_to_value
#fn_clone_reflect
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,12 +134,6 @@ fn expand_reflect(
}
};

let fn_type_info = quote! {
fn type_descriptor(&self) -> Cow<'static, TypeDescriptor> {
<Self as DescribeType>::type_descriptor()
}
};

let fn_debug = attrs.fn_debug_tokens();
let fn_clone_reflect = attrs.fn_clone_reflect_tokens();
let Generics {
Expand All @@ -166,7 +160,6 @@ fn expand_reflect(
self
}

#fn_type_info
#fn_patch
#fn_to_value
#fn_clone_reflect
Expand Down
6 changes: 0 additions & 6 deletions crates/mirror-mirror/src/foreign_impls/boxed.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use alloc::borrow::Cow;
use alloc::boxed::Box;
use core::any::Any;
use core::fmt;
Expand All @@ -12,7 +11,6 @@ use crate::Reflect;
use crate::ReflectMut;
use crate::ReflectOwned;
use crate::ReflectRef;
use crate::TypeDescriptor;
use crate::Value;

impl<T> DescribeType for Box<T>
Expand All @@ -28,10 +26,6 @@ impl<T> Reflect for Box<T>
where
T: Reflect + DescribeType,
{
fn type_descriptor(&self) -> Cow<'static, TypeDescriptor> {
<T as DescribeType>::type_descriptor()
}

fn as_any(&self) -> &dyn Any {
<T as Reflect>::as_any(self)
}
Expand Down
9 changes: 0 additions & 9 deletions crates/mirror-mirror/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,6 @@

extern crate alloc;

use alloc::borrow::Cow;
use alloc::borrow::ToOwned;
use alloc::boxed::Box;
use alloc::string::String;
Expand All @@ -271,12 +270,6 @@ use crate::enum_::VariantKind;

macro_rules! trivial_reflect_methods {
() => {
fn type_descriptor(
&self,
) -> alloc::borrow::Cow<'static, $crate::type_info::TypeDescriptor> {
<Self as $crate::type_info::DescribeType>::type_descriptor()
}

fn as_any(&self) -> &dyn Any {
self
}
Expand Down Expand Up @@ -369,8 +362,6 @@ pub use self::value::Value;

/// A reflected type.
pub trait Reflect: Any + Send + 'static {
fn type_descriptor(&self) -> Cow<'static, TypeDescriptor>;

fn as_any(&self) -> &dyn Any;

fn as_any_mut(&mut self) -> &mut dyn Any;
Expand Down
5 changes: 0 additions & 5 deletions crates/mirror-mirror/src/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ use crate::ReflectRef;
use crate::ScalarMut;
use crate::ScalarOwned;
use crate::ScalarRef;
use crate::TypeDescriptor;

/// A type erased value type.
///
Expand Down Expand Up @@ -190,10 +189,6 @@ impl DescribeType for Value {
}

impl Reflect for Value {
fn type_descriptor(&self) -> alloc::borrow::Cow<'static, TypeDescriptor> {
<Self as DescribeType>::type_descriptor()
}

fn as_any(&self) -> &dyn Any {
for_each_variant!(self, inner => inner)
}
Expand Down

0 comments on commit 2e9834e

Please sign in to comment.